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
<address@hidden>, Ian Campbell
<address@hidden> 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 <address@hidden> 
> > --- 
> > 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 
> address@hidden 
> http://lists.xen.org/xen-devel 
>  
>