Date: Thu, 28 Oct 2010 14:11:40 -0400 From: John Baldwin <jhb@freebsd.org> To: Attilio Rao <attilio@freebsd.org> 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 Message-ID: <201010281411.40423.jhb@freebsd.org> In-Reply-To: <AANLkTim_znVvoC0DVygFEu8GPOiwiuNs2fB_iMp61GpA@mail.gmail.com> References: <201010281631.o9SGVdtZ014923@svn.freebsd.org> <201010281257.05481.jhb@freebsd.org> <AANLkTim_znVvoC0DVygFEu8GPOiwiuNs2fB_iMp61GpA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, October 28, 2010 1:21:34 pm Attilio Rao wrote: > 2010/10/28 John Baldwin <jhb@freebsd.org>: > > 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 <sys/systm.h> > #include <sys/bus.h> > #include <sys/kernel.h> > -#ifdef __amd64__ > #include <sys/linker.h> > -#endif > #include <sys/malloc.h> > #include <sys/module.h> > #include <machine/bus.h> > @@ -67,12 +65,10 @@ > #include <vm/pmap.h> > #include <machine/pmap.h> > > -#ifdef __amd64__ > #include <machine/metadata.h> > -#include <machine/pc/bios.h> > -#endif > #include <machine/nexusvar.h> > #include <machine/resource.h> > +#include <machine/pc/bios.h> > > #ifdef DEV_APIC > #include "pcib_if.h" > @@ -89,11 +85,13 @@ > #include <sys/rtprio.h> > > #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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201010281411.40423.jhb>