[Qemu-devel] [RFC PATCH V2 0/2] support xen HVM direct kernel boot

[Qemu-devel] [RFC PATCH V2 0/2] support xen HVM direct kernel boot

[Chunyan Liu]

After your valuable suggestions, I'll continue the work to support
stubdom. But since stubdom currently uses qemu-xen-traditional and
will switch to qemu upstream in future, in a period of time, stubdom
HVM direct kernel boot won't be in working status. So, before
continuing the stubdom support work, I updated existing patch series,
adding proper error messages for rombios and stubdom limitation,
adding man page descriptions and other trival updates to make it a
complete working patch series. For those using seabios and non stubdom,
they can use now.

xen side patch: pass kernel/initrd/append parameters to qemu-dm
qemu side patch: reuse load_linux() for xen hvm direct kernel boot.
    Different from pc_memory_init which does lots of ram alloc work
    and rom/bios loading work, for xen, we only need to init a basic
    fw_cfg device used by load_linux() to store ADDRs and
    linuxboot.bin/multiboot.bin to retrive ADDRs, then load_linux(),
    after that, do real add option rom work to add
    linuxboot.bin/multiboot.bin to system option rom. Other things
    would be done by seabios smoothly.

v1 is here:
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06233.html

Chunyan Liu (2):
  xen: pass kernel initrd to qemu
  qemu: support xen hvm direct kernel boot

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

[Chunyan Liu]

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.

[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. For HVM guests, in case of stubdom-dm and old rombios,
+direct kernel boot is not supported.
+
+=over 4
+
+=item B<kernel="PATHNAME">
+
+Load the specified file as the kernel image.
+
+=item B<ramdisk="PATHNAME">
+
+Load the specified file as the ramdisk.
+
+=item B<root="STRING">
+
+Append B<root="STRING"> to the kernel command line (Note: it is guest
+specific what meaning this has).
+
+=item B<extra="STRING">
+
+Append B<STRING> to the kernel command line. (Note: it is guest
+specific what meaning this has).
+
+=back
+
 =head3 Other Options
 
 =over 4
@@ -647,20 +675,12 @@ The following options apply only to Paravirtual guests.
 
 =over 4
 
-=item B<kernel="PATHNAME">
-
-Load the specified file as the kernel image.  Either B<kernel> or
-B<bootloader> must be specified for PV guests.
-
-=item B<ramdisk="PATHNAME">
-
-Load the specified file as the ramdisk.
-
 =item B<bootloader="PROGRAM">
 
 Run C<PROGRAM> to find the kernel image and ramdisk to use.  Normally
 C<PROGRAM> would be C<pygrub>, which is an emulation of
-grub/grub2/syslinux.
+grub/grub2/syslinux. Either B<kernel> or B<bootloader> must be specified
+for PV guests.
 
 =item B<bootloader_args=[ "ARG", "ARG", ...]>
 
@@ -668,16 +688,6 @@ Append B<ARG>s to the arguments to the B<bootloader>
 program. Alternatively if the argument is a simple string then it will
 be split into words at whitespace (this second option is deprecated).
 
-=item B<root="STRING">
-
-Append B<root="STRING"> to the kernel command line (Note: it is guest
-specific what meaning this has).
-
-=item B<extra="STRING">
-
-Append B<STRING> to the kernel command line. Note: it is guest
-specific what meaning this has).
-
 =item B<e820_host=BOOLEAN>
 
 Selects whether to expose the host e820 (memory map) to the guest via
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);
+            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);
+
         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),
                                        ("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);
+
+    if (root) {
+        if (asprintf(&cmdline, "root=%s %s", root, extra) == -1)
+            cmdline = NULL;
+    } else {
+        cmdline = strdup(extra);
+    }
+
+    if ((root || extra) && !cmdline) {
+        fprintf(stderr, "Failed to allocate memory for cmdline\n");
+        exit(1);
+    }
+
+    return cmdline;
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -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");
+            else
+                b_info->u.hvm.kernel = strdup(buf);
+        }
+
+        b_info->u.hvm.cmdline = parse_cmdline(config);
+        xlu_cfg_replace_string (config, "ramdisk", &b_info->u.hvm.ramdisk, 0);
 
         xlu_cfg_replace_string (config, "firmware_override",
                                 &b_info->u.hvm.firmware, 0);
@@ -1061,26 +1091,8 @@ static void parse_config_data(const char *config_source,
         break;
     case LIBXL_DOMAIN_TYPE_PV:
     {
-        char *cmdline = NULL;
-        const char *root = NULL, *extra = "";
-
         xlu_cfg_replace_string (config, "kernel", &b_info->u.pv.kernel, 0);
 
-        xlu_cfg_get_string (config, "root", &root, 0);
-        xlu_cfg_get_string (config, "extra", &extra, 0);
-
-        if (root) {
-            if (asprintf(&cmdline, "root=%s %s", root, extra) == -1)
-                cmdline = NULL;
-        } else {
-            cmdline = strdup(extra);
-        }
-
-        if ((root || extra) && !cmdline) {
-            fprintf(stderr, "Failed to allocate memory for cmdline\n");
-            exit(1);
-        }
-
         xlu_cfg_replace_string (config, "bootloader", &b_info->u.pv.bootloader, 0);
         switch (xlu_cfg_get_list_as_string_list(config, "bootloader_args",
                                       &b_info->u.pv.bootloader_args, 1))
@@ -1108,7 +1120,7 @@ static void parse_config_data(const char *config_source,
             exit(1);
         }
 
-        b_info->u.pv.cmdline = cmdline;
+        b_info->u.pv.cmdline = parse_cmdline(config);
         xlu_cfg_replace_string (config, "ramdisk", &b_info->u.pv.ramdisk, 0);
         break;
     }
-- 
1.8.4.5

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

[Ian Campbell]

On Wed, 2014-06-04 at 15:34 +0800, Chunyan Liu wrote:

Please can you remember to CC Ian Jackson on these patches in the future
as a) he's a toolstack maintainer and b) he's the one doing work on
upstream qemu stubdom so he may have concerns about this stuff.

Thanks,
Ian.

> 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.
> 
> [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. For HVM guests, in case of stubdom-dm and old rombios,
> +direct kernel boot is not supported.
> +
> +=over 4
> +
> +=item B<kernel="PATHNAME">
> +
> +Load the specified file as the kernel image.
> +
> +=item B<ramdisk="PATHNAME">
> +
> +Load the specified file as the ramdisk.
> +
> +=item B<root="STRING">
> +
> +Append B<root="STRING"> to the kernel command line (Note: it is guest
> +specific what meaning this has).
> +
> +=item B<extra="STRING">
> +
> +Append B<STRING> to the kernel command line. (Note: it is guest
> +specific what meaning this has).
> +
> +=back
> +
>  =head3 Other Options
>  
>  =over 4
> @@ -647,20 +675,12 @@ The following options apply only to Paravirtual guests.
>  
>  =over 4
>  
> -=item B<kernel="PATHNAME">
> -
> -Load the specified file as the kernel image.  Either B<kernel> or
> -B<bootloader> must be specified for PV guests.
> -
> -=item B<ramdisk="PATHNAME">
> -
> -Load the specified file as the ramdisk.
> -
>  =item B<bootloader="PROGRAM">
>  
>  Run C<PROGRAM> to find the kernel image and ramdisk to use.  Normally
>  C<PROGRAM> would be C<pygrub>, which is an emulation of
> -grub/grub2/syslinux.
> +grub/grub2/syslinux. Either B<kernel> or B<bootloader> must be specified
> +for PV guests.
>  
>  =item B<bootloader_args=[ "ARG", "ARG", ...]>
>  
> @@ -668,16 +688,6 @@ Append B<ARG>s to the arguments to the B<bootloader>
>  program. Alternatively if the argument is a simple string then it will
>  be split into words at whitespace (this second option is deprecated).
>  
> -=item B<root="STRING">
> -
> -Append B<root="STRING"> to the kernel command line (Note: it is guest
> -specific what meaning this has).
> -
> -=item B<extra="STRING">
> -
> -Append B<STRING> to the kernel command line. Note: it is guest
> -specific what meaning this has).
> -
>  =item B<e820_host=BOOLEAN>
>  
>  Selects whether to expose the host e820 (memory map) to the guest via
> 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);
> +            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);
> +
>          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),
>                                         ("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);
> +
> +    if (root) {
> +        if (asprintf(&cmdline, "root=%s %s", root, extra) == -1)
> +            cmdline = NULL;
> +    } else {
> +        cmdline = strdup(extra);
> +    }
> +
> +    if ((root || extra) && !cmdline) {
> +        fprintf(stderr, "Failed to allocate memory for cmdline\n");
> +        exit(1);
> +    }
> +
> +    return cmdline;
> +}
> +
>  static void parse_config_data(const char *config_source,
>                                const char *config_data,
>                                int config_len,
> @@ -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");
> +            else
> +                b_info->u.hvm.kernel = strdup(buf);
> +        }
> +
> +        b_info->u.hvm.cmdline = parse_cmdline(config);
> +        xlu_cfg_replace_string (config, "ramdisk", &b_info->u.hvm.ramdisk, 0);
>  
>          xlu_cfg_replace_string (config, "firmware_override",
>                                  &b_info->u.hvm.firmware, 0);
> @@ -1061,26 +1091,8 @@ static void parse_config_data(const char *config_source,
>          break;
>      case LIBXL_DOMAIN_TYPE_PV:
>      {
> -        char *cmdline = NULL;
> -        const char *root = NULL, *extra = "";
> -
>          xlu_cfg_replace_string (config, "kernel", &b_info->u.pv.kernel, 0);
>  
> -        xlu_cfg_get_string (config, "root", &root, 0);
> -        xlu_cfg_get_string (config, "extra", &extra, 0);
> -
> -        if (root) {
> -            if (asprintf(&cmdline, "root=%s %s", root, extra) == -1)
> -                cmdline = NULL;
> -        } else {
> -            cmdline = strdup(extra);
> -        }
> -
> -        if ((root || extra) && !cmdline) {
> -            fprintf(stderr, "Failed to allocate memory for cmdline\n");
> -            exit(1);
> -        }
> -
>          xlu_cfg_replace_string (config, "bootloader", &b_info->u.pv.bootloader, 0);
>          switch (xlu_cfg_get_list_as_string_list(config, "bootloader_args",
>                                        &b_info->u.pv.bootloader_args, 1))
> @@ -1108,7 +1120,7 @@ static void parse_config_data(const char *config_source,
>              exit(1);
>          }
>  
> -        b_info->u.pv.cmdline = cmdline;
> +        b_info->u.pv.cmdline = parse_cmdline(config);
>          xlu_cfg_replace_string (config, "ramdisk", &b_info->u.pv.ramdisk, 0);
>          break;
>      }

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

[Ian Campbell]

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.

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

[Chun Yan Liu]

>>> On 6/12/2014 at 05:58 PM, in message
<1402567125.9177.26.camel@kazak.uk.xensource.com>, Ian Campbell
<Ian.Campbell@citrix.com> wrote: 
> 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? 
Not tried.

> If not then you might 
> have to say "qemu-xen when using the default BIOS (seabios)" etc. 

Yes, that's clearer. I'll update.

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

Will update.

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

Will update.

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

Yeah. Will update.

>  
> 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) 
>  
I'll take this way and update code, and update manpage as well.

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

I think it's mainly to handle the old config file format, like:
kernel="/usr/lib/xen/boot/hvmloader"
in fact, in most of our hvm config files, this line still exists.

>  
> 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"); 
> ? 

Guest behavior will change for those using config file including:
kernel="/usr/lib/xen/boot/hvmloader" (as does in xm config file)
Till now, this is allowed and guest can be booted normally.
Without check and set u.hvm.kernel,  guest will fail to boot.
I'm afraid this is too risky?

>  
> Ian. 
>  
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  

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

[Ian Campbell]

On Thu, 2014-06-12 at 23:43 -0600, Chun Yan Liu wrote:
> > > +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) 
> >  
> I'll take this way and update code, and update manpage as well.

Awesome, thank you!

> > > @@ -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. 
> 
> I think it's mainly to handle the old config file format, like:
> kernel="/usr/lib/xen/boot/hvmloader"
> in fact, in most of our hvm config files, this line still exists.

Hrm. I wonder if we could check for that specific string then? Perhaps
also $FOODIR/.../hvmloader (where $FOODIR is whichever bit of that path
comes from autoconf)?
 
> > 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"); 
> > ? 
> 
> Guest behavior will change for those using config file including:
> kernel="/usr/lib/xen/boot/hvmloader" (as does in xm config file)
> Till now, this is allowed and guest can be booted normally.
> Without check and set u.hvm.kernel,  guest will fail to boot.
> I'm afraid this is too risky?

If you guys still have config files with that in around then yes I think
it is a bit risky.

Ian.

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

[Chun Yan Liu]

>>> On 6/13/2014 at 04:28 PM, in message
<1402648083.26678.17.camel@kazak.uk.xensource.com>, Ian Campbell
<Ian.Campbell@citrix.com> wrote: 
> On Thu, 2014-06-12 at 23:43 -0600, Chun Yan Liu wrote: 
> > > > +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)  
> > >   
> > I'll take this way and update code, and update manpage as well. 
>  
> Awesome, thank you! 
>  
> > > > @@ -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.  
> >  
> > I think it's mainly to handle the old config file format, like: 
> > kernel="/usr/lib/xen/boot/hvmloader" 
> > in fact, in most of our hvm config files, this line still exists. 
>  
> Hrm. I wonder if we could check for that specific string then? Perhaps 
> also $FOODIR/.../hvmloader (where $FOODIR is whichever bit of that path 
> comes from autoconf)? 

Or simply check basename="hvmloader".

>   
> > > 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");  
> > > ?  
> >  
> > Guest behavior will change for those using config file including: 
> > kernel="/usr/lib/xen/boot/hvmloader" (as does in xm config file) 
> > Till now, this is allowed and guest can be booted normally. 
> > Without check and set u.hvm.kernel,  guest will fail to boot. 
> > I'm afraid this is too risky? 
>  
> If you guys still have config files with that in around then yes I think 
> it is a bit risky. 
>  
> Ian. 
>  
>  
>  

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

[Ian Campbell]

On Mon, 2014-06-16 at 01:14 -0600, Chun Yan Liu wrote:
> > > > > @@ -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.  
> > >  
> > > I think it's mainly to handle the old config file format, like: 
> > > kernel="/usr/lib/xen/boot/hvmloader" 
> > > in fact, in most of our hvm config files, this line still exists. 
> >  
> > Hrm. I wonder if we could check for that specific string then? Perhaps 
> > also $FOODIR/.../hvmloader (where $FOODIR is whichever bit of that path 
> > comes from autoconf)? 
> 
> Or simply check basename="hvmloader".

I think that would work. Rather than writing a basename function it
might be sufficient to check that the tail of the string is/isn't
"/hvmloader".

Ian.

[Qemu-devel] [RFC PATCH V2 2/2] qemu: support xen hvm direct kernel boot

[Chunyan Liu]

qemu side patch to support xen HVM direct kernel boot:
if -kernel exists, calls xen_load_linux(), which will read kernel/initrd
and add a linuxboot.bin or multiboot.bin option rom. The
linuxboot.bin/multiboot.bin will load kernel/initrd and jump to execute
kernel directly. It's working when xen uses seabios.

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
Changes:
  * remove kvmvpaic.bin from xen option_rom by hacking:
     +    s->vapic_control = 0;
    rather than the way of checking and bypassing it in xen_load_linux().

 hw/i386/pc.c           | 22 ++++++++++++++++++++++
 hw/i386/pc_piix.c      |  7 +++++++
 hw/i386/xen/xen_apic.c |  1 +
 include/hw/i386/pc.h   |  5 +++++
 4 files changed, 35 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e6369d5..bc04fe4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1187,6 +1187,28 @@ void pc_acpi_init(const char *default_dsdt)
     }
 }
 
+FWCfgState *xen_load_linux(const char *kernel_filename,
+                           const char *kernel_cmdline,
+                           const char *initrd_filename,
+                           ram_addr_t below_4g_mem_size,
+                           PcGuestInfo *guest_info)
+{
+    int i;
+    FWCfgState *fw_cfg;
+
+    assert(kernel_filename != NULL);
+
+    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
+    rom_set_fw(fw_cfg);
+
+    load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size);
+    for (i = 0; i < nb_option_roms; i++) {
+        rom_add_option(option_rom[i].name, option_rom[i].bootindex);
+    }
+    guest_info->fw_cfg = fw_cfg;
+    return fw_cfg;
+}
+
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
                            const char *kernel_cmdline,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a48e263..93b8d93 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -158,6 +158,13 @@ static void pc_init1(MachineState *machine,
                        machine->initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
                        rom_memory, &ram_memory, guest_info);
+    } else if (args->kernel_filename != NULL) {
+        /* For xen HVM direct kernel boot, load linux here */
+        fw_cfg = xen_load_linux(args->kernel_filename,
+                                args->kernel_cmdline,
+                                args->initrd_filename,
+                                below_4g_mem_size,
+                                guest_info);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
index 63bb7f7..f5acd6a 100644
--- a/hw/i386/xen/xen_apic.c
+++ b/hw/i386/xen/xen_apic.c
@@ -40,6 +40,7 @@ static void xen_apic_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
 
+    s->vapic_control = 0;
     memory_region_init_io(&s->io_memory, OBJECT(s), &xen_apic_io_ops, s,
                           "xen-apic-msi", APIC_SPACE_SIZE);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 32a7687..e472184 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -134,6 +134,11 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
                             MemoryRegion *pci_address_space);
 
+FWCfgState *xen_load_linux(const char *kernel_filename,
+                           const char *kernel_cmdline,
+                           const char *initrd_filename,
+                           ram_addr_t below_4g_mem_size,
+                           PcGuestInfo *guest_info);
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
                            const char *kernel_cmdline,
-- 
1.8.4.5

Re: [Qemu-devel] [Xen-devel] [RFC PATCH V2 0/2] support xen HVM direct kernel boot

[Chun Yan Liu]

>>> On 6/4/2014 at 03:34 PM, in message
<1401867299-7715-1-git-send-email-cyliu@suse.com>, Chunyan Liu <cyliu@suse.com>
wrote: 
> After your valuable suggestions, I'll continue the work to support 
> stubdom. But since stubdom currently uses qemu-xen-traditional and 
> will switch to qemu upstream in future, in a period of time, stubdom 
> HVM direct kernel boot won't be in working status. So, before 
> continuing the stubdom support work, I updated existing patch series, 
> adding proper error messages for rombios and stubdom limitation, 
> adding man page descriptions and other trival updates to make it a 
> complete working patch series. For those using seabios and non stubdom, 
> they can use now. 

Hi, Stefan, Ian J. & Ian C.,

I don't know if it's proper but could we merge current patch series first and
stubdom compatible work continued next? We do have some customers they
want to use it in guest installation. And considering: a) current patch series and
future stubdom compatible version will keep same user interface. b) stubdom
HVM direct kernel boot won't really work until stubdom-dm switches to qemu
upstream, maybe not soon? (I didn't see it in xen 4.5 development). c) how many
users will actually use stubdom and at the same time using direct kernel boot?

Regards,
Chunyan




> xen side patch: pass kernel/initrd/append parameters to qemu-dm 
> qemu side patch: reuse load_linux() for xen hvm direct kernel boot. 
>     Different from pc_memory_init which does lots of ram alloc work 
>     and rom/bios loading work, for xen, we only need to init a basic 
>     fw_cfg device used by load_linux() to store ADDRs and 
>     linuxboot.bin/multiboot.bin to retrive ADDRs, then load_linux(), 
>     after that, do real add option rom work to add 
>     linuxboot.bin/multiboot.bin to system option rom. Other things 
>     would be done by seabios smoothly. 
>  
> v1 is here: 
> http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06233.html 
>  
> Chunyan Liu (2): 
>   xen: pass kernel initrd to qemu 
>   qemu: support xen hvm direct kernel boot 
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>