Re: [Xen-devel] [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: 26 August 2014 21:29
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org); Jan Beulich; Ian Jackson; Stefano
> Stabellini
> Subject: Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to
> be a feature mask
> 
> On Tue, 2014-08-05 at 14:07 +0100, Paul Durrant wrote:
> 
> Sorry for the delay replying, I've been on vacation and travelling.
> 
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index a412f9c..6c5d7e0 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -363,7 +363,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> >                                         ("acpi_s3",          libxl_defbool),
> >                                         ("acpi_s4",          libxl_defbool),
> >                                         ("nx",               libxl_defbool),
> > -                                       ("viridian",         libxl_defbool),
> > +                                       ("viridian",         
> > libxl_string_list),
> 
> Unfortunately this will break the API. specifically existing  apps which
> call libxl_defbool_set on this field will now get a type error.
>

I wasn't sure what was acceptable breakage; there's precedent for changing 
types with bootloader_args but maybe that came before stabilization of the API.
 
> You need to keep the existing viridian field and add a new one with the
> new semantics and a new name (e.g. viridian_features, or a set of new
> defbools as I suggested in my previous mail) and define the precedence.
> See usbdevice vs. usbdevice_list or the (very new) serial one for
> existing examples of this sort of thing.
> 

Ok, so I could deprecate the viridian defbool but leave it in place, then add a 
new list e.g. viridian_enlightenments. Any app setting the viridian defbool 
then implies an enlightenment list of [base, freq] otherwise, if the defbool is 
not set, libxl uses the actual viridian_enlightenment list.

> For your case though you could continue to treat the viridian defbool as
> a gate on a bunch of viridian_feature_X defbools, IOW if viridian ==
> false the others are ignored, if it is true then the others have the
> usual defbool behaviour of defaults and overrides. I think that would
> fall out fairly naturally in the setdefault function.
> 

I actually implemented that at first but it means that, with every new 
enlightenment, the build info structure has to grow a new defbool. That seems 
messy, which is why I went for the string list. I've now implemented an integer 
list type and enlightenment enum due to Ian J's objection about libxl dealing 
with strings. Hopefully that will be acceptable... Patch coming later today, 
hopefully.

  Paul

> Ian.
> 

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