[gPXE-devel] [PATCH][lkrn] add cmdline and ramdisk support

[Stefan Hajnoczi]

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?  Does this allow
image arguments to contain ';'?

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".

> 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 ';'?

> --- 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.

> @@ -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.

> --- 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?

> --- 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?

Stefan