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: Paul Durrant
> Sent: 15 September 2014 17:56
> To: 'Ian Jackson'
> 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
> 
> > -----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
> 

Thinking about this more overnight, would you be ok with this?

features = (!defbool_val(viridian) ? 0 : default) | (viridian_enable & 
~viridian_disable)

That way compatibility is maintained, but an application not wishing to use the 
default set can then avoid enabling it rather than having to set 
viridian_disable to ~0, which I think is ugly.

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

If you are amenable to the above then we now have xl only use 'viridian' if the 
param is specified as boolean. If the param is specified as a list it just uses 
the enable/disable masks, so we can avoid "!all" . An empty list -> no 
enlightenments, so the user can either explicitly enable groups or use "all" 
and then explicitly disable groups.

That seems better to me and should reduce the size of the patch too.

  Paul

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