Date: Mon, 11 Jun 2012 13:11:17 -0600 From: Ian Lepore <freebsd@damnhippie.dyndns.org> To: Warner Losh <imp@bsdimp.com> Cc: "arm@freebsd.org" <arm@freebsd.org> Subject: Re: Boot parameter parsing Message-ID: <1339441877.36051.351.camel@revolution.hippie.lan> In-Reply-To: <FB34C0E8-2C1D-4758-A8AC-DC090C009B44@bsdimp.com> References: <FB34C0E8-2C1D-4758-A8AC-DC090C009B44@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-Dg8/6bg3FPPx/IU7zj9L Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Thu, 2012-06-07 at 00:26 -0600, Warner Losh wrote: > Greetings, > > Please find enclosed a small patch to the arm port. > > For too long parsing boot args in arm land has suffered from cut and paste code. This is inefficient and inflexible. This patch does something to fix that. First, it modifies all the arm ports to call parse_boot_param passing in the boot parameters from initarm as the first thing in each platform's implementation of that function. This is done really super early, importantly before we start using memory outside of the loaded kernel's text, data, and bss areas. I'd thought of moving this even earlier, into __start just before the call to initarm, but wasn't completely sure was quite right (it would be more code deleted, however, if I do that: please comment). The down side is that initarm was the only function we called in __start apart from mundane things like memcpy and that would change that, but that's kinda a weak argument I think. > > I've created a weak alias to tying this function to fake_preload_metadata. All but one of the ports do this now, and this moves them to a common standard that could be more easily changed. > > For most ports, it replaces the call to fake_preload_metadata. As such, I've modified the signature of fake_preload_metadata to be the same as parse_boot_param and made it a weak alias. So, if you don't define one, you'll get the current behavior. > > In a future patch, I'll likely be moving the mv platform's code into this routine (I'll create a default_parse_boot_param and create a weak alias pointing to that instead). I'll need to modify the mv port to then get the dtb blob via the metadata, or possibly create a new global for it so that other platforms might make use of that also. > > In a patch after that, I may add a kernel option to enable creation of FreeBSD /boot/loader metadata from Linux's standard boot stuff. This will allow platforms to get more data from the Linux boot loader without going through the intermediate /boot/loader. But it should preserve a unified interface by having it behave just like /boot/loader, but without anything setup by its more advanced features like kernel environment variables or loadable modules. > > If I've done things right by this point, then any ARM port can take advantage of these new features, not just the target I'm aiming at. In addition, anybody can use their own boot loader, if they so choose, and be able to write custom code that parses the args from it in whatever appropriate way might arise for their board. I know of at least one FreeBSD/arm user that has a heavily hacked boot2 boot loader that passes things into the kernel in a non-standard way. This will accommodate them, and others like them, while still providing the project with useful functionality. > > Comments? > > Warner > > P.S. You can also find this at http://people.freebsd.org/~imp/parse_boot_param.diff in case the mailing list eats this for lunch. > > I like everything about this except the names. I've always thought fake_preload_metadata() was a bad name. Now having it be the default implementation of parse_boot_param() seems even-more-bad. There's nothing about the name (or the new comment block) that makes it clear that if you supply a parse_boot_param() routine, it must do everything that fake_preload_metadata() does. The attached patch changes just the arm/machdep.c and machdep.h part of your changes. The fake_preload_metadata() is renamed to add_kernel_preload_metadata() and it becomes a helper routine that board-specific implementations of parse_boot_param() can use to generate the preload metadata when it doesn't come in with the boot params data. It also adds a new routine default_parse_boot_param() which is just a thin wrapper around add_kernel_preload_metadata(). This patch applies to -current on top of the recent changes from Andrew Turner, and instead of (not in addition to) your patch. -- Ian --=-Dg8/6bg3FPPx/IU7zj9L Content-Disposition: attachment; filename="parse_boot_param_2.diff" Content-Type: text/x-patch; name="parse_boot_param_2.diff"; charset="us-ascii" Content-Transfer-Encoding: 7bit Index: sys/arm/xscale/i8134x/crb_machdep.c =================================================================== --- sys/arm/xscale/i8134x/crb_machdep.c (revision 236701) +++ sys/arm/xscale/i8134x/crb_machdep.c (working copy) @@ -191,8 +191,8 @@ vm_offset_t lastaddr; uint32_t memsize, memstart; + lastaddr = parse_boot_param(abp); set_cpufuncs(); - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/xscale/i80321/ep80219_machdep.c =================================================================== --- sys/arm/xscale/i80321/ep80219_machdep.c (revision 236701) +++ sys/arm/xscale/i80321/ep80219_machdep.c (working copy) @@ -194,8 +194,8 @@ vm_offset_t lastaddr; uint32_t memsize, memstart; + lastaddr = parse_boot_param(abp); set_cpufuncs(); - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/xscale/i80321/iq31244_machdep.c =================================================================== --- sys/arm/xscale/i80321/iq31244_machdep.c (revision 236701) +++ sys/arm/xscale/i80321/iq31244_machdep.c (working copy) @@ -195,8 +195,8 @@ vm_offset_t lastaddr; uint32_t memsize, memstart; + lastaddr = parse_boot_param(abp); set_cpufuncs(); - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/xscale/pxa/pxa_machdep.c =================================================================== --- sys/arm/xscale/pxa/pxa_machdep.c (revision 236701) +++ sys/arm/xscale/pxa/pxa_machdep.c (working copy) @@ -176,9 +176,8 @@ int i, j; uint32_t memsize[PXA2X0_SDRAM_BANKS], memstart[PXA2X0_SDRAM_BANKS]; + lastaddr = parse_boot_param(abp); set_cpufuncs(); - - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/xscale/ixp425/avila_machdep.c =================================================================== --- sys/arm/xscale/ixp425/avila_machdep.c (revision 236701) +++ sys/arm/xscale/ixp425/avila_machdep.c (working copy) @@ -240,8 +240,8 @@ vm_offset_t lastaddr; uint32_t memsize; + lastaddr = parse_boot_param(abp); set_cpufuncs(); /* NB: sets cputype */ - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/mv/mv_machdep.c =================================================================== --- sys/arm/mv/mv_machdep.c (revision 236701) +++ sys/arm/mv/mv_machdep.c (working copy) @@ -333,7 +333,7 @@ */ mdp = (void *)((uint32_t)mdp & ~PAGE_MASK); - /* Parse metadata and fetch parameters */ + /* Parse metadata and fetch parameters (move to common machdep.c?) */ if (mdp != NULL) { preload_metadata = mdp; kmdp = preload_search_by_type("elf kernel"); @@ -352,7 +352,7 @@ preload_addr_relocate = KERNVIRTADDR - KERNPHYSADDR; } else { /* Fall back to hardcoded metadata. */ - lastaddr = fake_preload_metadata(); + lastaddr = fake_preload_metadata(abp); } #if defined(FDT_DTB_STATIC) Index: sys/arm/econa/econa_machdep.c =================================================================== --- sys/arm/econa/econa_machdep.c (revision 236701) +++ sys/arm/econa/econa_machdep.c (working copy) @@ -197,8 +197,8 @@ boothowto = RB_VERBOSE; + lastaddr = parse_boot_param(abp); set_cpufuncs(); - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/s3c2xx0/s3c24x0_machdep.c =================================================================== --- sys/arm/s3c2xx0/s3c24x0_machdep.c (revision 236701) +++ sys/arm/s3c2xx0/s3c24x0_machdep.c (working copy) @@ -246,10 +246,9 @@ int i; uint32_t memsize; + boothowto = 0; /* Likely not needed */ + lastaddr = parse_boot_param(abp); i = 0; - - boothowto = 0; - set_cpufuncs(); cpufuncs.cf_sleep = s3c24x0_sleep; lastaddr = fake_preload_metadata(); Index: sys/arm/sa11x0/assabet_machdep.c =================================================================== --- sys/arm/sa11x0/assabet_machdep.c (revision 236701) +++ sys/arm/sa11x0/assabet_machdep.c (working copy) @@ -216,10 +216,10 @@ uint32_t memsize = 32 * 1024 * 1024; sa1110_uart_vaddr = SACOM1_VBASE; - boothowto = RB_VERBOSE | RB_SINGLE; + boothowto = RB_VERBOSE | RB_SINGLE; /* Default value */ + lastaddr = parse_boot_param(abp); cninit(); set_cpufuncs(); - lastaddr = fake_preload_metadata(); physmem = memsize / PAGE_SIZE; pc = &__pcpu; pcpu_init(pc, 0, sizeof(struct pcpu)); Index: sys/arm/at91/at91_machdep.c =================================================================== --- sys/arm/at91/at91_machdep.c (revision 236701) +++ sys/arm/at91/at91_machdep.c (working copy) @@ -394,8 +394,8 @@ uint32_t memsize; vm_offset_t lastaddr; + lastaddr = parse_boot_param(abp); set_cpufuncs(); - lastaddr = fake_preload_metadata(); pcpu_init(pcpup, 0, sizeof(struct pcpu)); PCPU_SET(curthread, &thread0); Index: sys/arm/arm/machdep.c =================================================================== --- sys/arm/arm/machdep.c (revision 236888) +++ sys/arm/arm/machdep.c (working copy) @@ -663,40 +663,48 @@ } /* - * Fake up a boot descriptor table + * Add entries to a preloaded-modules metadata table that describe the kernel, + * based on static info available at compile time. Entries are added to the + * given table at index *idx, and upon return *idx points to the MODINFO_END + * entry (allowing the caller to write more entries into the table if + * necessary). + * Returns the last address used by the kernel. + * Can be used by parse_boot_param() routines when the incoming boot parameters + * don't include info about where the kernel was loaded. */ vm_offset_t -fake_preload_metadata(void) +add_kernel_preload_metadata(uint32_t *metadata, size_t *idx) { #ifdef DDB vm_offset_t zstart = 0, zend = 0; #endif vm_offset_t lastaddr; - int i = 0; - static uint32_t fake_preload[35]; + int i; - fake_preload[i++] = MODINFO_NAME; - fake_preload[i++] = strlen("kernel") + 1; - strcpy((char*)&fake_preload[i++], "kernel"); + i = (idx == NULL) ? 0 : *idx; + + metadata[i++] = MODINFO_NAME; + metadata[i++] = strlen("kernel") + 1; + strcpy((char*)&metadata[i++], "kernel"); i += 1; - fake_preload[i++] = MODINFO_TYPE; - fake_preload[i++] = strlen("elf kernel") + 1; - strcpy((char*)&fake_preload[i++], "elf kernel"); + metadata[i++] = MODINFO_TYPE; + metadata[i++] = strlen("elf kernel") + 1; + strcpy((char*)&metadata[i++], "elf kernel"); i += 2; - fake_preload[i++] = MODINFO_ADDR; - fake_preload[i++] = sizeof(vm_offset_t); - fake_preload[i++] = KERNVIRTADDR; - fake_preload[i++] = MODINFO_SIZE; - fake_preload[i++] = sizeof(uint32_t); - fake_preload[i++] = (uint32_t)&end - KERNVIRTADDR; + metadata[i++] = MODINFO_ADDR; + metadata[i++] = sizeof(vm_offset_t); + metadata[i++] = KERNVIRTADDR; + metadata[i++] = MODINFO_SIZE; + metadata[i++] = sizeof(uint32_t); + metadata[i++] = (uint32_t)&end - KERNVIRTADDR; #ifdef DDB if (*(uint32_t *)KERNVIRTADDR == MAGIC_TRAMP_NUMBER) { - fake_preload[i++] = MODINFO_METADATA|MODINFOMD_SSYM; - fake_preload[i++] = sizeof(vm_offset_t); - fake_preload[i++] = *(uint32_t *)(KERNVIRTADDR + 4); - fake_preload[i++] = MODINFO_METADATA|MODINFOMD_ESYM; - fake_preload[i++] = sizeof(vm_offset_t); - fake_preload[i++] = *(uint32_t *)(KERNVIRTADDR + 8); + metadata[i++] = MODINFO_METADATA|MODINFOMD_SSYM; + metadata[i++] = sizeof(vm_offset_t); + metadata[i++] = *(uint32_t *)(KERNVIRTADDR + 4); + metadata[i++] = MODINFO_METADATA|MODINFOMD_ESYM; + metadata[i++] = sizeof(vm_offset_t); + metadata[i++] = *(uint32_t *)(KERNVIRTADDR + 8); lastaddr = *(uint32_t *)(KERNVIRTADDR + 8); zend = lastaddr; zstart = *(uint32_t *)(KERNVIRTADDR + 4); @@ -705,14 +713,45 @@ } else #endif lastaddr = (vm_offset_t)&end; - fake_preload[i++] = 0; - fake_preload[i] = 0; - preload_metadata = (void *)fake_preload; + /* Return the number of entries we added, not counting MODINFO_END. */ + if (idx != NULL) + *idx = i; + metadata[i++] = MODINFO_END; + metadata[i] = 0; return (lastaddr); } /* + * This is the default boot parameter parsing routine. It just provides + * the global preload_metadata describing the kernel, and returns the last + * address used by the kernel. Any board-specific replacement routine must do + * these two things in addition to any board-specific work it does. + * + * This default implementation is provided via weak linkage. A board can + * provide an alternate implementation that uses the arm_boot_params passed in + * from the bootloader and sets up the preload_metadata accordingly. + * + * This routine is called early in initarm, before VM has been initialized. + * It needs to preserve any data that the boot loader has passed in, before the + * kernel starts to grow past the end of the BSS -- traditionally the place + * boot-loaders put this data. Since this is called so early it cannot do + * things that depend on the vm system being setup (including access to some + * SoC's serial ports which means printf() is probably not going to work). + * + */ +static vm_offset_t +default_parse_boot_param(struct arm_boot_params *abp __unused) +{ + static uint32_t kernel_preload_metadata[35]; + + preload_metadata = (void *)kernel_preload_metadata; + return add_kernel_preload_metadata(kernel_preload_metadata, NULL); +} + +__weak_reference(default_parse_boot_param, parse_boot_param); + +/* * Initialize proc0 */ void Index: sys/arm/include/machdep.h =================================================================== --- sys/arm/include/machdep.h (revision 236888) +++ sys/arm/include/machdep.h (working copy) @@ -6,11 +6,13 @@ /* misc prototypes used by the many arm machdeps */ void arm_lock_cache_line(vm_offset_t); -vm_offset_t fake_preload_metadata(void); void init_proc0(vm_offset_t kstack); void halt(void); void data_abort_handler(trapframe_t *); void prefetch_abort_handler(trapframe_t *); void undefinedinstruction_bounce(trapframe_t *); +struct arm_boot_params; +vm_offset_t parse_boot_param(struct arm_boot_params *); +vm_offset_t add_kernel_preload_metadata(uint32_t *, size_t *); #endif /* !_MACHINE_MACHDEP_H_ */ --=-Dg8/6bg3FPPx/IU7zj9L--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1339441877.36051.351.camel>