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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx]
> Sent: 15 September 2014 17:11
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org); Ian Campbell; Stefano Stabellini;
> Dave Scott
> Subject: Re: [PATCH v10 1/2] x86/viridian: Re-purpose the HVM parameter to
> be a feature mask
> 
> Paul Durrant writes ("[PATCH v10 1/2] x86/viridian: Re-purpose the HVM
> parameter to be a feature mask"):
> > The viridian option in xl.cfg(5) has also been changed to a list so
> > that the sets can be individually enabled or disabled. For compatibility,
> > if the option is specified as a boolean, then a true (1) value will enable
> > the base and freq sets and a false (0) value will not enable any
> > enlightenments.
> 
> Thanks.
> 
> 
> I like what you have done with the enum and libxl_bitmap.
> 
> 
> I disagree with the semantics of the interface you propose at the
> libxl level.  The new API syntax would permit the specification not
> only of `none', `defaults', `exactly this set' but also of `like the
> defaults but with these additions or omissions'.  But your
> interpretation of that syntax does not.
> 
> I would suggest the following semantics for these fields:
> 
> (from my mail of Wed, 3 Sep 2014 17:26:29 +0100)
> ]  For libxl I think this means:
> ]
> ]    - Keep the existing flag
> ]
> ]    - Introduce two new bitmaps called something like
> ]       viridian_features_{enable,disable}
> ]
> ]    - Features actually enabled are
> ]        !defbool_value(viridian) ? 0 :
> ]        ((viridian_features_enable | default) & ~viridian_features_disable)
> 
> Your implementation has
> 
>   - Features actually enabled are
>       !defbool_value(viridian) ? 0 :
>       (!viridian_features_enable && !viridian_features_disable) ? default :
>       (viridian_features_enable &~ viridian_features_disable)
> 
> which I think is a bit daft - especially since it renders
> viridian_features_disable rather pointless.  (default seems to be
> BASE|FREQ, and I have no quibble with that.)
> 

The problem was that, using your suggestion, you could do something like set 
viridian_features_enable to FREQ and that (because BASE is also in defaults) 
would be apparently fine... but essentially it's not, because BASE is a 
pre-requisite for everything else. This is why I opted for:

viridian = 0 -> nothing
viridian = 1 and no enable/disable masks -> defaults (for compatibility)
viridian = 1 and masks specified -> whatever the masks say

I agree that the logic looks weird but because of BASE being a pre-requisite it 
seemed more sensible to me to just have a 'defaults' purely for compatibility.

> (I'm using bitmask-in-integer notation here for clarity even though
> these are actually libxl bitmaps.)
> 
> 
> At the xl level the semantics are OK, but it would be quite easy to
> allow the user to modify, rather than simply replace, the default set.
> 
> To do that I would suggest:
> 
>  - have "all" do                enable  =  ~0,  disable =   0;
>  - have "!all" do               enable  =   0,  disable =  ~0;
>  - have each other keyword do   enable  |= bit, disable &= ~bit;
>  - have each other !keyword do  disable |= bit, enable  &= ~bit;
> 
> So you can say
>    viridian=["!all","base"]
> to turn on exactly base, or
>    viridian=["!freq"]
> to turn on the defaults but disable freq.
>
> What do you think ?
>

I'd say it seems a bit weird, from a user perspective, that you'd have to say 
["!all", "base"] rather than just ["base"] if that's the only set you wanted. I 
still favour a list that includes specific sets, or contains "all" and then 
excludes specific sets.
 
> 
> I have some minor comments about the xl interface and its docs:
> 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index 1e04eed..4090464 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> ...
> > +=item B<viridian=[ "SET", "SET", ...]>
> > +
> > +The sets of Microsoft Hyper-V (AKA viridian) compatible enlightenments
> > +exposed to the guest. The following sets of enlightenments may be
> > +specified:
> 
> This whole thing might be less confusing if you used the word `group'
> rather than `set'.  Then the bitmaps etc. specify a `set of groups'
> rather than a `set of sets'.
>

Ok.
 
> > +=over 4
> > +
> > +=item B<base>
> > +
> > +This set incorporates the Hypercall MSRs, Virtual processor index MSR,
> > +and APIC access MSRs. These enlightenments can improve performance
> of
> > +Windows Vista and Windows Server 2008 onwards and setting this option
> > +for such guests is strongly recommended.
> > +This set is also a pre-requisite for all other sets. If it is
> > +disabled then all enlightenments will be disabled.
> 
> Wouldn't it be better to reject, rather than silently ignore, an
> erroneous configuration ?
> 

Ok. 

> > +=item B<all>
> > +
> > +This is a special value that enables all of the above sets
> 
> That's not technically true.  It enables all of the available
> enlightenments, not specifically all of `base' and `freq'.  The
> difference is that future versions of xl will interpret `all' more
> widely.
> 
> 
> > +Specifying the viridian option as a boolean is deprecated. However, for
> > +backwards compatibility, if it is specified as a boolean a value of
> > +true (1) will cause both the B<base> and B<freq> sets to be exposed to
> > +the guest, and a value of false (0) will disable all enlightenments.
> 
> Why do we now need to deprecate this ?
> 

I thought that it would be better to deprecate use of the boolean and use a 
list of groups going forward. Use of a boolean may, I guess, mean that users 
may believe they are somehow getting a magically optimal set of enlightenments 
for their OS. I would prefer that users either opted for 'all' or said what 
they wanted. Maybe not the boolean and have it mean 'all'? That does mean more 
enlightenments may get turned on at reboot if a VM is moved from one host to a 
newer host though.

  Paul

> It would be IMO better to formally document that specifying it as a
> boolean gets you the `default' set of groups, which is currently
> `base' and `freq'.
> 
> 
> Thanks,
> Ian.

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