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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Paul Durrant writes ("[PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter 
to be a feature mask"):
> The following commits introduced the time reference counter MSR and
> TSC/APIC frequency MSRs into the viridian feature set respectively:
...
> The viridian option in xl.cfg(5) has also been changed to a string list so
> that the sets can be individually sepcified. For compatibility, if the
> option is specified as a boolean, then a true (1) value will be translated
> to a string list containing "base" and "freq".

We had a conversation on IRC about the API.  See below.

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)

The feature bits would be provided via a libxl enum either specified
with values 1,2,4 etc. or (better) with a new `shifted' property.

For xl I would suggest:

  Keep the existing config name and do not introduce any new variables.

  Existing values 0 and 1 (or false and true) mean `enable, defaults'.

  Alternatively user can specify comma-separated list of {feature
  name, or `all'} each optionally preceded by `!'.  That means
  `enable, adjust feature set accordingly'.

Ian.

16:25 <Diziet> ijc: Looking at this viridian stuff I'm not particularly 
               enamoured by this integer list API for something that could be a 
               bitmask.  Also it lacks sensible defaulting.
16:25 <Diziet> What do you think ?
16:26 <@ijc> Diziet: IIRC my opinion was there should be a defbool for each 
             individual feature
16:27 <@ijc> a bitmask or a list lacks the necessary defaulting behaviour I 
             think.
16:27 <@ijc> a bitmask is certainly better than a list though, but not my 
             prefeence
16:27 <xadimgnik> isn't an empty list a reasonable default?
16:28 <@ijc> that only allows a global default, not a per feature default, I 
             think
16:28 <Diziet> If we want to add a feature in the future that's on by default, 
               we can't do so, with the current API.
16:28 <Diziet> If we are sure never to want to do that then I guess we can live 
               with it.
16:29 <xadimgnik> why not? you'd simply default it to on in libxl unless an 
                  option turning it off were passed in the list
16:29 <Diziet> For defaulting behaviour with bitmasks we would need to have 
               separate "enable" and "disable" bitmasks.
16:30 <Diziet> xadimgnik: If we do that we can't turn it off by default in the 
               future.
16:30 <Diziet> Also I wonder whether much of the time people would prefer to 
               specify "all available"
16:30 <Diziet> Which the current API doesn't provide.  A bitmask would if you 
               were permitted to pass ~
16:30 <Diziet> 0
16:32 <xadimgnik> hmm, 'all available' may be reasonable... I'd kind of prefer 
                  it if folks actually had to specify what they wanted though
16:32 <Diziet> Why would you prefer that ?
16:33 <xadimgnik> using all available would mean new enlightenments get turned 
                  on by default when VMs are moved to newer versions of Xen
16:34 <xadimgnik> and that may cause some surprise in some cases I guess
16:35 <Diziet> `Moved' = migrated ?  Or just booted on the new setup ?
16:36 <xadimgnik> well the enlightenment would remain off on migrate (because 
                  the save record has the actual viridian HVM param in it), but 
                  would become enabled on reboot when the 'all available' in 
                  the config file takes effect
16:37 <Diziet> That doesn't sound very dangerous.  Perhaps it's not desirable 
               in all configurations but forbidding the user from specifying it 
               seems rather strong.
16:37 <@ijc> I still think an explicit named defbool per enlightenment is 
             better than a bitmap
16:38 <xadimgnik> Not dangerous, but it's the sort of thing that leads to 
                  support tickets being raised against XenServer
16:38 <Diziet> ijc: How would the user specify "all available" ?
16:38 <Diziet> xadimgnik: So you can just not do that for your users.
16:43 <xadimgnik> yes, that's true; if you want an 'all available' then maybe a 
                  bitmask is the way to go
16:44 <@ijc> Diziet: all available wold be leave them all as default, 
             wouldn'tit?
16:45 <@ijc> I'd imageind that the existing field (which must remain) would 
             serve as a global on/off switch
16:46 <xadimgnik> perhaps leave the boolean option in xl.cfg and then have a 
                  list which, if empty, means 'all available'?
16:48 <Diziet> Let's think firstly about API.  The existing API must remain but 
               I thought it was going to be deprecated.
16:49 <Diziet> If we don't deprecate it then we can use it to gate the whole 
               thing.
16:49 <Diziet> In which case perhaps we don't need per-feature defaults.  I'm 
               not sure.
16:49 <Diziet> Personally I think simpler would be to have two bitmaps, enable 
               and disable, and keep the old defbool for compatibility only.
16:50 <Diziet> With a rule that passing ~0 for either enable or disable is 
               permitted.  !!(enable & disable) would be an error.
16:51 <xadimgnik> I'm ok with that if ijc is
16:51 <Diziet> libxl would translate "viridian='on'" to enable==~0
16:52 <Diziet> ijc's proposal seems to be that each individual one would have 
               its own defbool defaulting to on, gated on the existing defbool 
               which defaults to 0.
16:53 <Diziet> But that would make it difficult to specify "exactly these" 
               because there's no comprehensive list of the defbools to set to 
               false.
16:53 <@ijc> own defbool defaulting to a sane defaultm, no necedssarily on
16:53 <Diziet> ijc: Right.
16:53 <@ijc> A helper "set all to false"?
16:53 <Diziet> How ugly.
16:53 <@ijc> With a bitmap what happens when the 33rd enlightment is added? (or 
             the 65th, or the 129th...)
16:53 <Diziet> Are we expecting even 32 of these ?
16:54 <Diziet> If we run out of bitmap we make a new one.
16:54 <xadimgnik> not really
16:54 <Diziet> If it might be that a sane default for `I'm running Windows give 
               me the best thing' is not `all enabled' then my proposal is 
               insufficiently rich.
16:54 <Diziet> In that case I prefer ijc's proposal but with a bitmap instead 
               of the defbool.  Or we could have an array of defbools indexed 
               by enum or something.
16:55 <Diziet> s/a bitmap/two bitmaps/
16:55 <Diziet> s/the defbool/&s
16:56 <Diziet> If we go with bitmaps then the enum values should be called 
               *_shift so that when we fix the API to have the flag values 
               directly we haven't stolen the proper name.
16:57 <Diziet> (Or we could fix the idl compiler right away, I guess, although 
               that's a bit of a can of worms at this stage.)
16:58 -!- sstabellini [~sstabelli@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] has 
          joined #xendevel
16:59 <Diziet> ijc: ^
17:00 <@ijc> what needs changing in teh IDL?
17:00 <Diziet> You should be able to write     .viridian_disable = 
               some_flag|some_other_flag
17:01 <Diziet> when right now if we just use an enum you would have to write    
               .viridian_disable = (1UL<<some_flag)|(1UL<<some_other_flag)
17:02 <@ijc> Or declare the enum with values 1, 2, 4 etc. I suppose adding a 
             "shifted" parameter to the Enumeration class would be reasonably 
             trivial
17:06 <Diziet> ijc: Either would work.  And we could do the former and switch 
               to the latter later.

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