Re: [Xen-devel] [PATCH 2 of 2] Add the ability to specify the option "bios_override" in the guest

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-02-23 at 10:31 +0000, Attilio Rao wrote:
> On 23/02/12 10:13, Ian Campbell wrote:
> > On Wed, 2012-02-22 at 19:17 +0000, Attilio Rao wrote:
> >    
> >> configuration file.
> >> An example is given by:
> >> bios_override="ovmf-x64"
> >>      
> > I'm not sure I would use the override suffix here -- that is generally
> > for settings where we recommend that the user accepts the default. In
> > this case though a user has perfectly valid reasons for selecting
> > ovmf-{32,64} or seabios etc.
> >
> > Also please update docs/man/xl.cfg.pod.5 to document the new setting.
> >    
> 
> It makes sense, thanks.
> 
> >> diff -r 032fea10f8d1 -r 520ba1527b7a tools/libxl/libxl_dm.c
> >> --- a/tools/libxl/libxl_dm.c       Wed Feb 22 18:54:03 2012 +0000
> >> +++ b/tools/libxl/libxl_dm.c       Wed Feb 22 18:56:22 2012 +0000
> >> @@ -66,6 +66,8 @@ const char *libxl__domain_device_model(l
> >>   static const char *libxl__domain_bios(libxl__gc *gc,
> >>                                   const libxl_domain_build_info *info)
> >>   {
> >> +    if (info->u.hvm.bios)
> >> +       return info->u.hvm.bios;
> >>       switch (info->device_model_version) {
> >>       case 1: return "rombios";
> >>       case 2: return "seabios";
> >>      
> > Should integrate this function into libxl__domain_build_info_setdefaults
> > from my "libxl: improved handling for default values in API" series.
> >
> >    
> 
> I really wanted to do this type of integration in a separate step.

Sure, either you or I can take a look after both series are in.

> > Also it would be good to use that same infrastructure to enforce that
> > qemu-xen-traditional can only use rombios and that seabios and ovmf-*
> > are only valid with qemu-xen.
> >
> >    
> >> diff -r 032fea10f8d1 -r 520ba1527b7a tools/libxl/libxl_types.idl
> >> --- a/tools/libxl/libxl_types.idl  Wed Feb 22 18:54:03 2012 +0000
> >> +++ b/tools/libxl/libxl_types.idl  Wed Feb 22 18:56:22 2012 +0000
> >> @@ -228,6 +228,7 @@ libxl_domain_build_info = Struct("domain
> >>
> >>       ("u", KeyedUnion(None, libxl_domain_type, "type",
> >>                   [("hvm", Struct(None, [("firmware", string),
> >> +                                       ("bios", string),
> >>      
> > I think an Enumeration would be better here.
> > libxl__domain_bios
> 
> I just found it quicker/cleaner to just store the string and pass it 
> down, rather than translating to an Enumeration and then translate it 
> back to a string into libxl__domain_bios(), don't you think?

The IDL provides helpers for doing this (..._{to,from}_string). You
would end up just calling that instead of libxl__domain_bios(). i.e.
libxl__domain_bios goes away because the logic for setting
info->u.hvm.bios (in the future in setdefaults) ensures it is always a
specific valid value.

> However I'm open to do what you feel is better.
> 
> Thanks,
> Attilio



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