Re: [Qemu-devel] [RFC PATCH V2 1/2] xen: pass kernel initrd to qemu

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-06-04 at 15:34 +0800, Chunyan Liu wrote:
> xen side patch to support xen HVM direct kernel boot:
> support 'kernel', 'ramdisk', 'root', 'extra' in HVM config file,
> parse config file, pass -kernel, -initrd, -append parameters to qemu.
> It's working with seabios and non-stubdom. Rombios and stubdom cases
> are currently not supported.

I think everywhere you say "rombios" here and in the patch you actually
mean "qemu-xen-traditional" and likewise when you say "seabios" you
should really say "qemu-xen". The BIOS which happens to be used with the
qemu is rather secondary/incidental.

With that said does this stuff work with OVMF? If not then you might
have to say "qemu-xen when using the default BIOS (seabios)" etc.

> 
> [config example]
> kernel="/mnt/vmlinuz-3.0.13-0.27-default"
> ramdisk="/mnt/initrd-3.0.13-0.27-default"
> root="/dev/hda2"
> extra="console=tty0 console=ttyS0"
> disk=[ 'file:/mnt/images/bjz_04_sles11_sp2/disk0.raw,hda,w', ]
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> Changes:
>   * update man page to document the new parameters for HVM guests (move them
>     from PV special options to general options) and note current limitation 
>   * rombios and stubdom are not working yet, add libxl error messages
>     to inform that.
>   * extract "parse commandline" code to a common helper for both HVM and
>     PV parse_config_data to use.
> 
>  docs/man/xl.cfg.pod.5       | 50 ++++++++++++++++++++++++----------------
>  tools/libxl/libxl_dm.c      | 15 ++++++++++++
>  tools/libxl/libxl_types.idl |  3 +++
>  tools/libxl/xl_cmdimpl.c    | 56 +++++++++++++++++++++++++++------------------
>  4 files changed, 82 insertions(+), 42 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 0ca37bc..c585801 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -304,6 +304,34 @@ Action to take if the domain crashes.  Default is C<destroy>.
>  
>  =back
>  
> +=head3 Direct Kernel Boot
> +
> +Currently, direct kernel boot can be supported by PV guests, and HVM guests
> +in limitation.

"with limitations" or "in some configurations".

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 51ab2bf..c2eaa54 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -196,6 +196,12 @@ static char ** libxl__build_device_model_args_old(libxl__gc *gc,
>          int nr_set_cpus = 0;
>          char *s;
>  
> +        if (b_info->u.hvm.kernel) {
> +            LOG(ERROR, "%s: direct kernel boot is not supported by %s",
> +                __func__, dm);

I know there are existing example in this file, but using __func__ like
this is wrong, the LOG macro already adds it.

Also instead of printing dm (the full path to the binary) I think you
should just print "qemu-xen-traditional" here.
> +            return NULL;
> +        }
> +
>          if (b_info->u.hvm.serial) {
>              flexarray_vappend(dm_args, "-serial", b_info->u.hvm.serial, NULL);
>          }
> @@ -479,6 +485,15 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          int ioemu_nics = 0;
>  
> +        if (b_info->u.hvm.kernel)
> +            flexarray_vappend(dm_args, "-kernel", b_info->u.hvm.kernel, NULL);
> +
> +        if (b_info->u.hvm.ramdisk)
> +            flexarray_vappend(dm_args, "-initrd", b_info->u.hvm.ramdisk, NULL);
> +
> +        if (b_info->u.hvm.cmdline)
> +            flexarray_vappend(dm_args, "-append", b_info->u.hvm.cmdline, NULL);

You could use flexarray_append_pair here, but I appreciate you might
want to go for consistency with the existing code here. I don't mind
either way.

> +
>          if (b_info->u.hvm.serial) {
>              flexarray_vappend(dm_args, "-serial", b_info->u.hvm.serial, NULL);
>          }
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 52f1aa9..a96b228 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -336,6 +336,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("event_channels",   uint32),
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",         string),
> +                                       ("kernel",           string),
> +                                       ("cmdline",          string),
> +                                       ("ramdisk",          string),

You need to add a suitable LIBXL_HAVE_FOO define to libxl.h to indicate
that this new functionality is available, see the comment and existing
examples in libxl.h.

A single one to cover all three would be fine.
LIBXL_HAVE_BUILDINFO_HVM_DIRECT_KERNEL_BOOT perhaps?

>                                         ("bios",             libxl_bios_type),
>                                         ("pae",              libxl_defbool),
>                                         ("apic",             libxl_defbool),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5195914..c3cadbb 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -725,6 +725,29 @@ static void parse_top_level_sdl_options(XLU_Config *config,
>      xlu_cfg_replace_string (config, "xauthority", &sdl->xauthority, 0);
>  }
>  
> +static char *parse_cmdline(XLU_Config *config)
> +{
> +    char *cmdline = NULL;
> +    const char *root = NULL, *extra = "";
> +
> +    xlu_cfg_get_string (config, "root", &root, 0);
> +    xlu_cfg_get_string (config, "extra", &extra, 0);

I sort of hate this root=/extra= stuff which comes from the PV world,
since it is sort of exposing Linux-isms (e.g. root=%s etc).

I'd far rather just have cmdline=. Since for PV this is needed for xm
cfg file compatibility we are sort of stuck with it but for this new HVM
functionality we don't have those backward compat concerns.

If you want to just do xlu_cfg_get_string(..., "cmdline", ...) for the
HVM case I would be OK with that but if you feel like it would you mind
very much going a bit further and implementing cmdline for PV, such that
it takes precedence over root/extra? Something like:

    xlu_cfg_get_string (config, "cmdline", &cmdline, 0);
    xlu_cfg_get_string (config, "root", &root, 0);
    xlu_cfg_get_string (config, "extra", &extra, 0);

    if (cmdline && root)
       fprintf(stderr, "Warning: ignoring deprecated root= in favour of cmdline=\n");
    if (cmdline && extra)
       fprintf(stderr, "Warning: ignoring deprecated extra= in favour of cmdline=\n");
    if (!cmdline) {
        /* The existing code for dealing with extra/root etc */
        ...asprintf(&cmdline, ...)
    }
    return cmdline

? (In doing this I think it would be simpler to allow root=/extra= for
HVM guests too even though they are immediately deprecated rather than
making all of the above conditional)

> @@ -1007,9 +1030,16 @@ static void parse_config_data(const char *config_source,
>  
>      switch(b_info->type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
> -        if (!xlu_cfg_get_string (config, "kernel", &buf, 0))
> -            fprintf(stderr, "WARNING: ignoring \"kernel\" directive for HVM guest. "
> -                    "Use \"firmware_override\" instead if you really want a non-default firmware\n");
> +        if (!xlu_cfg_get_string (config, "kernel", &buf, 0)) {
> +            if (strstr(buf, "hvmloader"))
> +                fprintf(stderr, "WARNING: ignoring \"kernel\" directive for HVM guest. "
> +                        "Use \"firmware_override\" instead if you really want a non-default firmware\n");

I think we've had this for a few releases now, I wonder if it has served
its purpose? Especially since the strstr check could have false
positives and negatives.

IOW perhaps we should just delete this check and handle kernel the
natural way. Trouble is I cannot estimate what sort of support burden
this will make for us. Perhaps keep the warning but continue on having
set u.hvm.kernel? e.g.
    fprintf(stderr,
            "WARNING: You seem to be using the \"kernel\" directive to override the firmware (hvmloader) for an HVM guest.\n"
            "This option will boot the named kernel directly with no firmware present.\n"
            "Use \"firmware_override\" instread if you really want a non-default firmware.\n");
?

Ian.