Skip site navigation (1)Skip section navigation (2)
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>