|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask
> -----Original Message----- > From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx] > Sent: 03 September 2014 17:26 > To: Paul Durrant > Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org); Ian Campbell; Stefano Stabellini; > Dave Scott > Subject: Re: [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to > be a feature mask > > 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. > Ok. > 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'. > Thanks for the summary. It was (mostly) the xl part I was not sure about. V10 patches coming shortly... Cheers, Paul > 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |