diff mbox

[RFC,V3,1/2] xen: pass kernel initrd to qemu

Message ID 1403244196-19630-2-git-send-email-cyliu@suse.com
State New
Headers show

Commit Message

Chunyan Liu June 20, 2014, 6:03 a.m. UTC
xen side patch to support xen HVM direct kernel boot:
support 'kernel', 'ramdisk', 'cmdline' (and 'root', 'extra' as well
which would be deprecated later) in HVM config file, parse config file,
pass -kernel, -initrd, -append parameters to qemu.

It's working with qemu-xen when using the default BIOS (seabios).

[config example]
kernel="/mnt/vmlinuz-3.0.13-0.27-default"
ramdisk="/mnt/initrd-3.0.13-0.27-default"
cmdline="/dev/hda2 console=tty0 console=ttyS0"
or:
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"

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 docs/man/xl.cfg.pod.5       | 57 +++++++++++++++++++++++++--------------
 tools/libxl/libxl.h         | 13 +++++++++
 tools/libxl/libxl_dm.c      | 15 +++++++++++
 tools/libxl/libxl_types.idl |  3 +++
 tools/libxl/xl_cmdimpl.c    | 66 ++++++++++++++++++++++++++++++---------------
 5 files changed, 112 insertions(+), 42 deletions(-)

Comments

Ian Jackson June 23, 2014, 2:22 p.m. UTC | #1
Chunyan Liu writes ("[RFC PATCH V3 1/2] xen: pass kernel initrd to qemu"):
> xen side patch to support xen HVM direct kernel boot:
> support 'kernel', 'ramdisk', 'cmdline' (and 'root', 'extra' as well
> which would be deprecated later) in HVM config file, parse config file,
> pass -kernel, -initrd, -append parameters to qemu.
...
> +Currently, direct kernel boot can be supported by PV guests, and HVM guests
> +with limitations. For HVM guests, in case of stubdom-dm and old rombios,
> +direct kernel boot is not supported.

What does "PV guests" mean here ?  Do you mean guest kernels which
support the Xen PV environment ?

If so, what is the benefit of booting them "direct" but HVM, rather
than simply booting them PV ?

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

Surely using this should be possible for direct kernel boot too.

> +/*
> + * LIBXL_HAVE_BUILDINFO_HVM_DIRECT_KERNEL_BOOT
> + *
> + * If this is defined, then the libxl_domain_build_info structure will
> + * contain hvm.kernel, hvm.cmdline and hvm.ramdisk. hvm.kernel is a string
> + * to indicate kernel image location, hvm.ramdisk is a string to indicate
> + * ramdisk location, hvm.cmdline is a string to indicate the paramters which
> + * would be appened to kernel image.

If we are going to do this then I think the kernel, cmdline and
ramdisk (and bootloader) parameters shoudl be moved into the main part
of the domain_build_info struct.  This will involve a compatibility
layer: temporarily (for at least one release) both will be supported
so the IDL will have to have both and code inside libxl will have to
honour either.

The #define should then be
  LIBXL_HAVE_BUILD_INFO_CMDLINE
or some such - because all libxl callers will want to update to the
  new API eventually.

> +static char *parse_cmdline(XLU_Config *config)
> +{
> +    char *cmdline = NULL;
> +    const char *root = NULL, *extra = NULL, *buf = NULL;
> +
> +    xlu_cfg_get_string (config, "cmdline", &buf, 0);
> +    xlu_cfg_get_string (config, "root", &root, 0);
> +    xlu_cfg_get_string (config, "extra", &extra, 0);
> +
> +    if (buf) {
> +        cmdline = strdup(buf);
> +        if (root || extra)
> +            fprintf(stderr, "Warning: ignoring deprecated root= and extra= "
> +                    "in favour of cmdline=\n");

Why are you deprecating root= and extra= ?

They seem likely to be a useful distinction if we decide to add
further default options in the future.

If you do want to deprecate them, you should make this very clear -
probably by putting that change in a separate patch in your series.

Thanks,
Ian.
Chunyan Liu June 24, 2014, 3:24 a.m. UTC | #2
>>> On 6/23/2014 at 10:22 PM, in message
<21416.14398.695327.278606@mariner.uk.xensource.com>, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote: 
> Chunyan Liu writes ("[RFC PATCH V3 1/2] xen: pass kernel initrd to qemu"): 
> > xen side patch to support xen HVM direct kernel boot: 
> > support 'kernel', 'ramdisk', 'cmdline' (and 'root', 'extra' as well 
> > which would be deprecated later) in HVM config file, parse config file, 
> > pass -kernel, -initrd, -append parameters to qemu. 
> ... 
> > +Currently, direct kernel boot can be supported by PV guests, and HVM  
> guests 
> > +with limitations. For HVM guests, in case of stubdom-dm and old rombios, 
> > +direct kernel boot is not supported. 
>  
> What does "PV guests" mean here ?  Do you mean guest kernels which 
> support the Xen PV environment ? 

Yes. I mean Paravirtualized Guest.
Since before this patch these options are supported by PV guest only (to do
direct kernel boot), now HVM guest also supports them, so I move them to the
general options. What I want to say here is:
PV guests can support direct kernel boot (as always), now HVM guests can
support direct kernel boot too but with limitations (qemu-xen with default
seabios).

>  
> If so, what is the benefit of booting them "direct" but HVM, rather 
> than simply booting them PV ? 
>  
> >  =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. 
>  
> Surely using this should be possible for direct kernel boot too. 
>  
> > +/* 
> > + * LIBXL_HAVE_BUILDINFO_HVM_DIRECT_KERNEL_BOOT 
> > + * 
> > + * If this is defined, then the libxl_domain_build_info structure will 
> > + * contain hvm.kernel, hvm.cmdline and hvm.ramdisk. hvm.kernel is a string 
> > + * to indicate kernel image location, hvm.ramdisk is a string to indicate 
> > + * ramdisk location, hvm.cmdline is a string to indicate the paramters  
> which 
> > + * would be appened to kernel image. 
>  
> If we are going to do this then I think the kernel, cmdline and 
> ramdisk (and bootloader) parameters shoudl be moved into the main part 
> of the domain_build_info struct.  This will involve a compatibility 
> layer: temporarily (for at least one release) both will be supported 
> so the IDL will have to have both and code inside libxl will have to 
> honour either. 

I see.

>  
> The #define should then be 
>   LIBXL_HAVE_BUILD_INFO_CMDLINE 
> or some such - because all libxl callers will want to update to the 
>   new API eventually. 
>  
> > +static char *parse_cmdline(XLU_Config *config) 
> > +{ 
> > +    char *cmdline = NULL; 
> > +    const char *root = NULL, *extra = NULL, *buf = NULL; 
> > + 
> > +    xlu_cfg_get_string (config, "cmdline", &buf, 0); 
> > +    xlu_cfg_get_string (config, "root", &root, 0); 
> > +    xlu_cfg_get_string (config, "extra", &extra, 0); 
> > + 
> > +    if (buf) { 
> > +        cmdline = strdup(buf); 
> > +        if (root || extra) 
> > +            fprintf(stderr, "Warning: ignoring deprecated root= and extra=  
> " 
> > +                    "in favour of cmdline=\n"); 
>  
> Why are you deprecating root= and extra= ? 

Just following Ian Campbell's suggestion:
http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg02909.html
Now, cmdline is actually 'root' plus 'extra', if there is no special future usage,
one 'cmdline' seems to be better.

> 
> They seem likely to be a useful distinction if we decide to add 
> further default options in the future. 

Don't know how they will be used. But it we do need them, sure we can keep
the current way.

Thanks,
Chunyan

>  
> If you do want to deprecate them, you should make this very clear - 
> probably by putting that change in a separate patch in your series. 
>  
> Thanks, 
> Ian. 
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>
Ian Campbell July 2, 2014, 2:40 p.m. UTC | #3
On Mon, 2014-06-23 at 15:22 +0100, Ian Jackson wrote:
> > +/*
> > + * LIBXL_HAVE_BUILDINFO_HVM_DIRECT_KERNEL_BOOT
> > + *
> > + * If this is defined, then the libxl_domain_build_info structure will
> > + * contain hvm.kernel, hvm.cmdline and hvm.ramdisk. hvm.kernel is a string
> > + * to indicate kernel image location, hvm.ramdisk is a string to indicate
> > + * ramdisk location, hvm.cmdline is a string to indicate the paramters which
> > + * would be appened to kernel image.
> 
> If we are going to do this then I think the kernel, cmdline and
> ramdisk (and bootloader) parameters shoudl be moved into the main part
> of the domain_build_info struct.  This will involve a compatibility
> layer: temporarily (for at least one release)

I don't think so -- we would need to retain it forever or at least until
some sort of "API break" event. We still guarantee that applications
using the 4.2 API will be supported.

>  both will be supported
> so the IDL will have to have both and code inside libxl will have to
> honour either.
> 
> The #define should then be
>   LIBXL_HAVE_BUILD_INFO_CMDLINE
> or some such - because all libxl callers will want to update to the
>   new API eventually.
> 
> > +static char *parse_cmdline(XLU_Config *config)
> > +{
> > +    char *cmdline = NULL;
> > +    const char *root = NULL, *extra = NULL, *buf = NULL;
> > +
> > +    xlu_cfg_get_string (config, "cmdline", &buf, 0);
> > +    xlu_cfg_get_string (config, "root", &root, 0);
> > +    xlu_cfg_get_string (config, "extra", &extra, 0);
> > +
> > +    if (buf) {
> > +        cmdline = strdup(buf);
> > +        if (root || extra)
> > +            fprintf(stderr, "Warning: ignoring deprecated root= and extra= "
> > +                    "in favour of cmdline=\n");
> 
> Why are you deprecating root= and extra= ?

I suggested this. They are suckful interfaces which expose Linux
specifics (e.g. the root= syntax) in our guest cfg files. cmdline is the
generic equivalent.

However, I didn't mean for them to not work, just that cmdline would be
preferred.

> They seem likely to be a useful distinction if we decide to add
> further default options in the future.
> 
> If you do want to deprecate them, you should make this very clear -
> probably by putting that change in a separate patch in your series.
> 
> Thanks,
> Ian.
Ian Jackson July 2, 2014, 3:17 p.m. UTC | #4
Ian Campbell writes ("Re: [RFC PATCH V3 1/2] xen: pass kernel initrd to qemu"):
> On Mon, 2014-06-23 at 15:22 +0100, Ian Jackson wrote:
> > If we are going to do this then I think the kernel, cmdline and
> > ramdisk (and bootloader) parameters shoudl be moved into the main part
> > of the domain_build_info struct.  This will involve a compatibility
> > layer: temporarily (for at least one release)
> 
> I don't think so -- we would need to retain it forever or at least until
> some sort of "API break" event. We still guarantee that applications
> using the 4.2 API will be supported.

Yes.  Sorry, I meant that the compatibility should be retained for
some considerable time.  So for now we should honour all the existing
config struct members plus also the new cmdline member which should
IMO be in the main part of the struct and not inside pv.

> > Why are you deprecating root= and extra= ?
> 
> I suggested this. They are suckful interfaces which expose Linux
> specifics (e.g. the root= syntax) in our guest cfg files. cmdline is the
> generic equivalent.

I have spoken to Ian C about this and he has convinced me that I was
wrong to object to deprecating root= and extra=.  So please do what
Ian C says, not what I say.  Sorry.

Thanks,
Ian.
Chunyan Liu July 3, 2014, 5:56 a.m. UTC | #5
>>> On 7/2/2014 at 11:17 PM, in message
<21428.8829.273127.394928@mariner.uk.xensource.com>, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote: 
> Ian Campbell writes ("Re: [RFC PATCH V3 1/2] xen: pass kernel initrd to  
> qemu"): 
> > On Mon, 2014-06-23 at 15:22 +0100, Ian Jackson wrote: 
> > > If we are going to do this then I think the kernel, cmdline and 
> > > ramdisk (and bootloader) parameters shoudl be moved into the main part 
> > > of the domain_build_info struct.  This will involve a compatibility 
> > > layer: temporarily (for at least one release) 
> >  
> > I don't think so -- we would need to retain it forever or at least until 
> > some sort of "API break" event. We still guarantee that applications 
> > using the 4.2 API will be supported. 
>  
> Yes.  Sorry, I meant that the compatibility should be retained for 
> some considerable time.  So for now we should honour all the existing 
> config struct members plus also the new cmdline member which should 
> IMO be in the main part of the struct and not inside pv. 

No new member created, it's always 'cmdline' in libxl_domain_build_info.
'root' and 'extra' and new 'cmdline' are only words to config file.

Before, in libxl_domain_build_info, there are only u.pv.kernel|cmdline|ramdisk,
now since both PV and HVM support them, in theory we should move them
to main part, but considering the compatibility issue, I'm not sure which one
is better?
1. add u.hvm.kernel|cmdline|ramdisk and add hvm processing only (as in V2)
2. add u.kernel|cmdline|ramdisk (since now both PV and HVM have these) but
    keep u.pv.kernel|cmdline|ramdisk, add hvm processing, add pv processing
    u.kernel|cmdline|ramdisk too so that new users could use new APIs. (as in V3)

Your suggestions?

- Chunyan



>  
> > > Why are you deprecating root= and extra= ? 
> >  
> > I suggested this. They are suckful interfaces which expose Linux 
> > specifics (e.g. the root= syntax) in our guest cfg files. cmdline is the 
> > generic equivalent. 
>  
> I have spoken to Ian C about this and he has convinced me that I was 
> wrong to object to deprecating root= and extra=.  So please do what 
> Ian C says, not what I say.  Sorry. 
>  
> Thanks, 
> Ian. 
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>
Ian Campbell July 3, 2014, 9:08 a.m. UTC | #6
On Wed, 2014-07-02 at 23:56 -0600, Chun Yan Liu wrote:
> 
> >>> On 7/2/2014 at 11:17 PM, in message
> <21428.8829.273127.394928@mariner.uk.xensource.com>, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote: 
> > Ian Campbell writes ("Re: [RFC PATCH V3 1/2] xen: pass kernel initrd to  
> > qemu"): 
> > > On Mon, 2014-06-23 at 15:22 +0100, Ian Jackson wrote: 
> > > > If we are going to do this then I think the kernel, cmdline and 
> > > > ramdisk (and bootloader) parameters shoudl be moved into the main part 
> > > > of the domain_build_info struct.  This will involve a compatibility 
> > > > layer: temporarily (for at least one release) 
> > >  
> > > I don't think so -- we would need to retain it forever or at least until 
> > > some sort of "API break" event. We still guarantee that applications 
> > > using the 4.2 API will be supported. 
> >  
> > Yes.  Sorry, I meant that the compatibility should be retained for 
> > some considerable time.  So for now we should honour all the existing 
> > config struct members plus also the new cmdline member which should 
> > IMO be in the main part of the struct and not inside pv. 
> 
> No new member created, it's always 'cmdline' in libxl_domain_build_info.
> 'root' and 'extra' and new 'cmdline' are only words to config file.
> 
> Before, in libxl_domain_build_info, there are only u.pv.kernel|cmdline|ramdisk,
> now since both PV and HVM support them, in theory we should move them
> to main part, but considering the compatibility issue, I'm not sure which one
> is better?
> 1. add u.hvm.kernel|cmdline|ramdisk and add hvm processing only (as in V2)
> 2. add u.kernel|cmdline|ramdisk (since now both PV and HVM have these) but
>     keep u.pv.kernel|cmdline|ramdisk, add hvm processing, add pv processing
>     u.kernel|cmdline|ramdisk too so that new users could use new APIs. (as in V3)
> 
> Your suggestions?

Something like 2 is the correct thing.

The final b_info struct should have the new fields:
   b_info->{kernel,cmdline,ramdisk}
and these existing ones:
   b_info->u.pv.{kernel,cmdline,ramdisk}

In libxl__domain_build_info_setdefaults you should then do, iff type ==
PV:
   if (!b_info->kernel)
       b_info->kernel = b_info->u.pv.kernel;
   b_info->u.pv.kernel = NULL;

likewise for ramdisk and cmdline.

and then all of the remaining library code should use b_info->kernel
exclusively.

Ian.
diff mbox

Patch

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a94d037..c460dd4 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -304,6 +304,41 @@  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
+with limitations. 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<cmdline="STRING">
+
+Append B<cmdline="STRING"> to the kernel command line. (Note: it is
+guest specific what meaning this has). It will replace
+B<root="STRING"> plus B<extra="STRING">. When B<cmdline="STRING"> is
+set, B<root="STRING"> and B<extra="STRING"> will be ignored.
+
+=item B<root="STRING">
+
+Append B<root="STRING"> to the kernel command line (Note: it is guest
+specific what meaning this has). It will be deprecated.
+
+=item B<extra="STRING">
+
+Append B<STRING> to the kernel command line. (Note: it is guest
+specific what meaning this has). It will be deprecated.
+
+=back
+
 =head3 Other Options
 
 =over 4
@@ -646,20 +681,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", ...]>
 
@@ -667,16 +694,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.h b/tools/libxl/libxl.h
index 80947c3..0ef393e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -482,6 +482,19 @@ 
  */
 #define LIBXL_HAVE_DEVICE_PCI_SEIZE 1
 
+/*
+ * LIBXL_HAVE_BUILDINFO_HVM_DIRECT_KERNEL_BOOT
+ *
+ * If this is defined, then the libxl_domain_build_info structure will
+ * contain hvm.kernel, hvm.cmdline and hvm.ramdisk. hvm.kernel is a string
+ * to indicate kernel image location, hvm.ramdisk is a string to indicate
+ * ramdisk location, hvm.cmdline is a string to indicate the paramters which
+ * would be appened to kernel image.
+ *
+ * If it is set, guest will be booted from the indicated kernel and ramdisk.
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_DIRECT_KERNEL_BOOT 1
+
 /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
  * called from within libxl itself. Callers outside libxl, who
  * do not #include libxl_internal.h, are fine. */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 51ab2bf..fe53441 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, "direct kernel boot is not supported by "
+                "qemu-xen-traditional");
+            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..a31e8c6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -725,6 +725,37 @@  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 = NULL, *buf = NULL;
+
+    xlu_cfg_get_string (config, "cmdline", &buf, 0);
+    xlu_cfg_get_string (config, "root", &root, 0);
+    xlu_cfg_get_string (config, "extra", &extra, 0);
+
+    if (buf) {
+        cmdline = strdup(buf);
+        if (root || extra)
+            fprintf(stderr, "Warning: ignoring deprecated root= and extra= "
+                    "in favour of cmdline=\n");
+    } else {
+        if (root) {
+            if (asprintf(&cmdline, "root=%s %s", root, extra) == -1)
+                cmdline = NULL;
+        } else if (extra) {
+            cmdline = strdup(extra);
+        }
+    }
+
+    if ((buf || 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 +1038,18 @@  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 (!strcmp(libxl_basename(buf), "hvmloader"))
+                fprintf(stderr, "WARNING: you seem to be using \"kernel\" "
+                        "directive to override HVM guest firmware. Ignore "
+                        "that. 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 +1101,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 +1130,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;
     }