[gPXE-devel] [PATCH][lkrn] add cmdline and ramdisk support
Wu Fengguang
fengguang.wu at intel.com
Tue Jun 1 02:57:40 EDT 2010
On Mon, May 31, 2010 at 04:57:17PM +0800, Stefan Hajnoczi wrote:
> On Mon, May 31, 2010 at 8:54 AM, Wu Fengguang <fengguang.wu at intel.com> wrote:
> Thanks for your patch Fengguang. Some feedback:
>
> > The command line syntax is "kernel URL [initrd URL]". One may add ';'
> > to truncate extra parameters. For example:
> > gpxe cmdline linux /proc/cmdline
> > -------------------------------------------------------------------
> > kernel URL root=/dev/ram0 initrd URL root=/dev/ram0 initrd URL
> > kernel URL; root=/dev/ram0 initrd URL root=/dev/ram0
>
> Did you mean: kernel URL root=/dev/ram0; initrd URL?
Yes, sorry for the mistake!
> Does this allow image arguments to contain ';'?
For example linux kernel parameters? Maybe there is such a case,
however practically I've never see a linux kernel cmdline that
contains the ';' symbol.
> gPXE supports booting image types other than Linux bzImage. This
> patch should be independent of the image type, preloading multiboot
> images should work, for example. Variable names shouldn't used
> "linux".
You mean the name linux_cmdline and linux_ramdisk*?
Yes it seems reasonable to rename them to bzimage_*.
> > Note that the auto-added BOOT_IMAGE=xxx may be dropped by the simple
> > cmdline parsing.
>
> I checked linux-2.6/Documentation/x86/boot.txt and this seems okay,
> it's optional for the bootloader to pass it. But couldn't the special
> case check for BOOT_IMAGE= be avoided by asking the user to terminate
> their args with ';'?
If remove the strstr("BOOT_IMAGE=") code, the user will have to add
the terminating ';', otherwise the initrd command will turn into
initrd URL BOOT_IMAGE=xxx
when loaded by pxelinux and then fail.
Anyway I come up with a better scheme: we can remove "kernel URL" and
"initrd URL" after consuming them. Now all the ';' or "BOOT_IMAGE="
tricks are no longer necessary:
--- gpxe.orig/src/usr/autoboot.c 2010-05-31 15:40:17.000000000 +0800
+++ gpxe/src/usr/autoboot.c 2010-06-01 14:31:57.000000000 +0800
@@ -122,6 +122,27 @@ int boot_root_path ( const char *root_pa
return -ENOTSUP;
}
+/*
+ * Return 0 if no match, otherwise remove command from buffer and
+ * return command length.
+ */
+static int get_cmdline_cmd ( char *buf, char *cmd ) {
+ char *p, *q;
+
+ buf = strstr ( buf, cmd );
+ if ( ! buf )
+ return 0;
+
+ p = buf + strlen(cmd);
+ q = cmd + strlen(cmd);
+ while ( *p && *p != ' ' )
+ *q++ = *p++;
+ *q = '\0';
+
+ strcpy ( buf, p );
+ return q - cmd;
+}
+
/**
* Boot from a network device
*
@@ -163,6 +184,32 @@ static int netboot ( struct net_device *
return pxe_menu_boot ( netdev );
}
+ if ( linux_cmdline ) {
+ char kernel_cmd[256] = "kernel ";
+ char initrd_cmd[256] = "initrd ";
+ int len;
+
+ DBG ( "Found cmdline at %lx: ", linux_cmdline );
+ copy_from_user ( buf, phys_to_user( linux_cmdline ),
+ 0, sizeof ( buf ) );
+ buf[sizeof ( buf ) - 1] = '\0';
+
+ len = get_cmdline_cmd ( buf, initrd_cmd );
+ len = get_cmdline_cmd ( buf, kernel_cmd );
+ if ( len ) {
+ strcpy ( kernel_cmd + len, buf );
+ rc = system ( kernel_cmd );
+ if ( rc )
+ return rc;
+ if ( initrd_cmd[7] ) {
+ rc = system ( initrd_cmd );
+ if ( rc )
+ return rc;
+ }
+ system( "boot" );
+ }
+ }
+
/* Try to download and boot whatever we are given as a filename */
fetch_ipv4_setting ( NULL, &next_server_setting, &next_server );
fetch_string_setting ( NULL, &filename_setting, buf, sizeof ( buf ) );
> > --- gpxe.orig/src/image/embedded.c 2010-05-31 15:40:17.000000000 +0800
> > +++ gpxe/src/image/embedded.c 2010-05-31 15:40:18.000000000 +0800
> > @@ -50,6 +50,15 @@ static struct image embedded_images[] =
> > EMBED_ALL
> > };
> >
> > +unsigned long __data16 ( linux_cmdline ) = 0;
> > +unsigned long __data16 ( linux_ramdisk ) = 0;
> > +unsigned long __data16 ( linux_ramdisk_size ) = 0;
>
> __data16() is x86-specific and cannot be used in core code. I think
> this part will need to be arch-specific.
Sorry I didn't know this. Shao Miller's implementation is more sane than mine:
http://git.etherboot.org/?p=people/sha0/gpxe.git;a=commitdiff;h=6bada87a533725f6de02c83240519a316bbb88ec
http://git.etherboot.org/?p=people/sha0/gpxe.git;a=commitdiff;h=e9516971775dbb32020fbe7dba93c2ddecd817fc
However I do prefer a simple "kernel URL root=... initrd URL" cmdline
syntax, instead of "foo bar cmd=echo baz". The user could use ramdisk
if he want more. Shao, what do you think?
> > @@ -59,6 +68,16 @@ static void embedded_init ( void ) {
> > void *data;
> > int rc;
> >
> > + if (linux_ramdisk && linux_ramdisk_size) {
> > + DBG ("Found ramdisk at %lx size %ld\n",
> > + linux_ramdisk, linux_ramdisk_size);
> > + preloaded_image.data = phys_to_user( linux_ramdisk );
> > + preloaded_image.len = linux_ramdisk_size;
> > + if ( ! register_image ( &preloaded_image ) &&
> > + ! image_autoload ( &preloaded_image ) )
> > + return;
> > + }
> > +
>
> The initrd images should not be (auto)loaded. The bzimage and
> multiboot code will pick them up without being loaded.
OK.
> > --- gpxe.orig/src/arch/i386/prefix/libprefix.S 2010-05-31 15:40:17.000000000 +0800
> > +++ gpxe/src/arch/i386/prefix/libprefix.S 2010-05-31 15:40:18.000000000 +0800
> > @@ -704,6 +704,13 @@ install_prealloc:
> > /* Set up %ds for access to .data16 */
> > movw %bx, %ds
> >
> > + movl %es:cmd_line_ptr, %ecx
> > + movl %ecx, linux_cmdline
> > + movl %es:ramdisk_image, %ecx
> > + movl %ecx, linux_ramdisk
> > + movl %es:ramdisk_size, %ecx
> > + movl %ecx, linux_ramdisk_size
> > +
> > #ifdef KEEP_IT_REAL
> > /* Initialise libkir */
> > movw %ax, (init_libkir_vector+2)
>
> libprefix is used by all prefix code (.pxe, .hd, .dsk, etc). Is this
> patch going to break all other prefices because these symbols are not
> defined?
I did worry about this. Could use Shao Miller's code instead.
> > --- gpxe.orig/src/arch/i386/core/relocate.c 2010-05-31 15:40:17.000000000 +0800
> > +++ gpxe/src/arch/i386/core/relocate.c 2010-05-31 15:40:18.000000000 +0800
> > @@ -58,6 +61,16 @@ __asmcall void relocate ( struct i386_al
> > "...need %lx bytes for %d-byte alignment\n",
> > start, end, padded_size, max_align );
> >
> > + if (linux_ramdisk && linux_ramdisk_size) {
> > + hide_ramdisk(linux_ramdisk, linux_ramdisk + linux_ramdisk_size);
> > +
> > + /* hide_ramdisk() takes effect in future.
> > + * For now simply lower the top location to hide it.
> > + */
> > + if (top > linux_ramdisk)
> > + top = linux_ramdisk & ~0xfffff;
> > + }
> > +
> > /* Walk through the memory map and find the highest address
> > * below 4GB that etherboot will fit into. Ensure etherboot
> > * lies entirely within a range with A20=0. This means that
>
> This looks like an assumption about where the ramdisk is loaded.
> Could it be loaded low in RAM such that relocate() discards the RAM
> memory region and fails to find space for gPXE?
It's possible _in theory_, however trivial to fix. If I understand it
right, we only need about 1MB space for gPXE text/data. So we can test
whether linux_ramdisk is in the lower half of an e820 region, and use
it as a _low boundary_ if so. Acceptable?
Thanks,
Fengguang
More information about the gPXE-devel
mailing list