From owner-svn-src-head@FreeBSD.ORG Thu Oct 28 18:15:52 2010 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 724261065693; Thu, 28 Oct 2010 18:15:52 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 40A348FC1D; Thu, 28 Oct 2010 18:15:52 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id CE70A46B49; Thu, 28 Oct 2010 14:15:51 -0400 (EDT) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 8D0E78A029; Thu, 28 Oct 2010 14:15:50 -0400 (EDT) From: John Baldwin To: Attilio Rao Date: Thu, 28 Oct 2010 14:11:40 -0400 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20100819; KDE/4.4.5; amd64; ; ) References: <201010281631.o9SGVdtZ014923@svn.freebsd.org> <201010281257.05481.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201010281411.40423.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Thu, 28 Oct 2010 14:15:50 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.96.3 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.9 required=4.2 tests=BAYES_00 autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on bigwig.baldwin.cx Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r214457 - in head/sys: amd64/amd64 conf i386/i386 x86/x86 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Oct 2010 18:15:52 -0000 On Thursday, October 28, 2010 1:21:34 pm Attilio Rao wrote: > 2010/10/28 John Baldwin : > > On Thursday, October 28, 2010 12:31:39 pm Attilio Rao wrote: > >> Author: attilio > >> Date: Thu Oct 28 16:31:39 2010 > >> New Revision: 214457 > >> URL: http://svn.freebsd.org/changeset/base/214457 > >> > >> Log: > >> Merge nexus.c from amd64 and i386 to x86 subtree. > >> > >> Sponsored by: Sandvine Incorporated > >> Tested by: gianni > >> > > > > It would be better to merge these two routines. The loader now passes the > > smap to i386 kernels as well, so ram_attach() should probably be changed to > > try the amd64 approach first and if that fails fall back to using the > > phys_avail[] array instead. > > What do you think about this patch?: > Index: nexus.c > =================================================================== > --- nexus.c (revision 214457) > +++ nexus.c (working copy) > @@ -52,9 +52,7 @@ > #include > #include > #include > -#ifdef __amd64__ > #include > -#endif > #include > #include > #include > @@ -67,12 +65,10 @@ > #include > #include > > -#ifdef __amd64__ > #include > -#include > -#endif > #include > #include > +#include > > #ifdef DEV_APIC > #include "pcib_if.h" > @@ -89,11 +85,13 @@ > #include > > #ifdef __amd64__ > -#define RMAN_BUS_SPACE_IO AMD64_BUS_SPACE_IO > -#define RMAN_BUS_SPACE_MEM AMD64_BUS_SPACE_MEM > +#define X86_BUS_SPACE_IO AMD64_BUS_SPACE_IO > +#define X86_BUS_SPACE_MEM AMD64_BUS_SPACE_MEM > +#define ELF_KERN_STR "elf64 kernel" > #else > -#define RMAN_BUS_SPACE_IO I386_BUS_SPACE_IO > -#define RMAN_BUS_SPACE_MEM I386_BUS_SPACE_MEM > +#define X86_BUS_SPACE_IO I386_BUS_SPACE_IO > +#define X86_BUS_SPACE_MEM I386_BUS_SPACE_MEM > +#define ELF_KERN_STR "elf32 kernel" > #endif I would not do this. What I meant is that amd64 and i386 should be changed to both use X86_BUS_SPACE_* and {AMD64,I386}_BUS_SPACE_* should be retired. This would involve changing the bus space code itself slightly, but it could perhaps be shared as a result. Alternatively as bde@ notes we could just drop the MD prefix entirely and have BUS_SPACE_* instead. However, I wouldn't make any changes to just nexus.c for the BUS_SPACE_* stuff. > @@ -668,20 +666,20 @@ > return (0); > } > > -#ifdef __amd64__ > static int > ram_attach(device_t dev) > { > struct bios_smap *smapbase, *smap, *smapend; > struct resource *res; > + vm_paddr_t *p; > caddr_t kmdp; > uint32_t smapsize; > - int error, rid; > + int error, i, rid; > > /* Retrieve the system memory map from the loader. */ > kmdp = preload_search_by_type("elf kernel"); > if (kmdp == NULL) > - kmdp = preload_search_by_type("elf64 kernel"); > + kmdp = preload_search_by_type(ELF_KERN_STR); > smapbase = (struct bios_smap *)preload_search_info(kmdp, > MODINFO_METADATA | MODINFOMD_SMAP); > smapsize = *((u_int32_t *)smapbase - 1); It would be nice if ELF_KERN_STR could be autogenerated as something like: "elf ## __ELF_WORD_SIZE ## kernel" instead of needing an #ifdef. > @@ -701,16 +699,11 @@ > panic("ram_attach: resource %d failed to attach", rid); > rid++; > } > - return (0); > -} > -#else > -static int > -ram_attach(device_t dev) > -{ > - struct resource *res; > - vm_paddr_t *p; > - int error, i, rid; > > + /* If at least one smap attached, return. */ > + if (rid != 0) > + return (0); > + Perhaps this instead: /* If we found an SMAP, return. */ if (smapbase != NULL) return (0); > /* > * We use the dump_avail[] array rather than phys_avail[] for > * the memory map as phys_avail[] contains holes for kernel > @@ -724,7 +717,7 @@ > */ > for (i = 0, p = dump_avail; p[1] != 0; i++, p += 2) { > rid = i; > -#ifdef PAE > +#if !defined(__amd64__) && defined(PAE) > /* > * Resources use long's to track resources, so we can't > * include memory regions above 4GB. No need for this bit, PAE is never true on amd64, so it can just be used directly in x86 code. > @@ -743,7 +736,6 @@ > } > return (0); > } > -#endif > > static device_method_t ram_methods[] = { > /* Device interface */ > -- John Baldwin