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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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

> +=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 ?

> +=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 ?

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