Date: Mon, 11 Jun 2012 14:57:37 -0600 From: Warner Losh <imp@bsdimp.com> To: Ian Lepore <freebsd@damnhippie.dyndns.org> Cc: "arm@freebsd.org" <arm@FreeBSD.org> Subject: Re: Boot parameter parsing Message-ID: <9EC29453-F3DD-43C9-850F-0174EAE9BAD8@bsdimp.com> In-Reply-To: <1339441877.36051.351.camel@revolution.hippie.lan> References: <FB34C0E8-2C1D-4758-A8AC-DC090C009B44@bsdimp.com> <1339441877.36051.351.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jun 11, 2012, at 1:11 PM, Ian Lepore wrote: > On Thu, 2012-06-07 at 00:26 -0600, Warner Losh wrote: >> Greetings, >>=20 >> Please find enclosed a small patch to the arm port. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> Comments? >>=20 >> Warner >>=20 >> 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. >>=20 >>=20 >=20 > 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 next commit in my tree actually creates a new = default_parse_boot_param() that will check to see if the FreeBSD data is = present and parse the usual suspects and fall back to the = fake_preload_metadata() routine that's used everywhere. Right now only = mv uses it, and we really should have everybody use it. After that, there is an option to parse the Linux boot loader stuff into = the metadata and then have the normal metadata code do the right thing. = this would be an optional thing, but the good news here is that we can = tell the difference between a FreeBSD /boot/loader and a Linux/Uboot = load automatically. > 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(). Yea, I have that in a new patch in my tree. I don't really want to = change fake_preload_metadata() just yet, since there are out of tree = ports that use it and I don't want to cut that cord yet. > This patch applies to -current on top of the recent changes from = Andrew > Turner, and instead of (not in addition to) your patch. Thanks. Warner=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9EC29453-F3DD-43C9-850F-0174EAE9BAD8>