Date: Thu, 28 Oct 2010 16:38:13 -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: <201010281638.14043.jhb@freebsd.org> In-Reply-To: <AANLkTi=5oMOtVVVtnuG0BFEnLyzHnParq7QixgMaPJ1m@mail.gmail.com> References: <201010281631.o9SGVdtZ014923@svn.freebsd.org> <201010281411.40423.jhb@freebsd.org> <AANLkTi=5oMOtVVVtnuG0BFEnLyzHnParq7QixgMaPJ1m@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, October 28, 2010 4:28:12 pm Attilio Rao wrote: > 2010/10/28 John Baldwin <jhb@freebsd.org>: > > 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 > >> @@ -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); > > No, I don't think this check is right, smapbase will always be != NULL > (otherwise the code panics). Oh, that needs to be fixed then. It can be NULL on i386 with an old loader (or on a really old machine without an SMAP). The amd64 nexus code could assume it would never be NULL, but i386 cannot. It should probably more closely match what i386 does during the memory probe which is: kmdp = search("elf kernel"); if (kmdp == NULL) kmdp = search("elfXX kernel"); if (kmdp != NULL) smapbase = preload_search(...); else smapbase = NULL; if (smapbase != NULL) { for (smap = ...) { } return (0); } /* fall through to old i386 code using phys_avail[] */ -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201010281638.14043.jhb>