Re: [PATCH v2 2/2] s390x/cpumodel: Introduce dynamic feature groups

[Eduardo Habkost]

On Thu, Dec 05, 2019 at 03:48:47PM +0100, David Hildenbrand wrote:
> On 05.12.19 15:35, Eduardo Habkost wrote:
> > On Mon, Dec 02, 2019 at 10:15:12AM +0100, David Hildenbrand wrote:
> >>
> >>>> Say the user has the option to select a model (zEC12, z13, z14), upper
> >>>> layers always want to have a model that includes all backported security
> >>>> features. While the host model can do that, CPU definitions can't. You
> >>>> can't change default models within a QEMU release, or for older releases
> >>>> (e.g., a z13).
> >>>>
> >>>
> >>> This is a good description of the main use case we're worried
> >>> about in x86 too, and the main reason we have added versioned CPU
> >>> models.
> >>>
> >>> I remember I was planning to use `query-cpu-model-expansion` for
> >>> "please give me the best configuration for this specific CPU
> >>> model" (which would be very similar to the approach used in this
> >>> series).  Now, I need to refresh my memory and try to remember
> >>> why I concluded this approach wouldn't work for x86.
> >>
> >> I would be interested in that - I don't really think exposing CPU
> >> versions to the user is necessary here.
> >>
> >> E.g., you can maintain the versions internally and enable the stored
> >> features of the fitting one with "recommended-features=on...".
> > 
> > I was re-reading some code and threads, and now I remember: the
> > main obstacle for using query-cpu-model-expansion for CPU model
> > version resolution in x86 is the fact that the x86 CPU models
> > aren't static yet.  (type=full expansion isn't useful for CPU the
> > use case above; type=static expansion requires static CPU models
> > to be useful)
> 
> 
> I think, you could if you would expand "best X" to something like
> 
> -cpu X,all-features=off,featX=on,featY=on ...
> 
> The "all-features" part would need a better name as discussed. Such a
> model would always have a defined feature set (all listed features) ==
> static. The list could get a little longer, which is why s390x has these
> static "base" features. But that's not a road blocker.
> 
> > 
> > I was planning to make x86 CPU models static, then I noticed we
> > do have lots of feature flags that depend on the current
> > accelerator (set by kvm_default_props) or current machine (set
> > by compat_props).  This breaks the rules for static CPU models.
> 
> The static models we have (e.g., z13-base) contain a minimum set of
> features we expect to be around in every environment (but doesn't have
> to). It's just a way to make the featX=on,featY=on ... list shorter.
> 
> X would be expanded to e.g.,
> 
> -cpu X-base,featX=on,featY=on ...
> 
> But nothing speaks against having
> 
> -cpu X-base,featX=off,featY=on ...
> 
> A very simplistic base model would be a model without any features.
> (like -cpu X,all-features=off), but then it would be set in stone.

x86 has only one static CPU model, called "base", just to make
type=static expansion work.  Having multiple "<model>-base" CPU
models would help make the extra feature list shorter, yes.

But we would still need to decide how to handle the
accel-specific code in x86_cpu_load_model(), including:
* kvm_default_props/tcg_default_props;
* x2apic special case for !kvm_irqchip_in_kernel();
* host vendor ID special case for KVM.

If we include that in static expansion, it would be a large
number of user-visible side effects for something that was
supposed to just add/remove a tiny set of CPU features to an
existing configuration.  If we don't, we are breaking the rules
of static expansion (aren't we?).

We can still try to address this and make
"query-cpu-model-expansion type=static ...,recommended-features=on"
work on x86, and see it is usable by libvirt in x86.  I'm just
worried that the interface may become complex, easy to get wrong,
and hard to validate until full libvirt support is implemented.
query-cpu-model-expansion is very extensible and flexible, but
hard to explain and reason about.


> 
> > 
> > We can still try to provide useful static CPU models in x86 in
> > the future (I want to).  But I don't want to make this an
> > obstacle for providing a CPU model update mechanism that works
> > for x86 (which is more urgent).
> > 
> >>
> >>>
> >>>
> >>>>>
> >>>>> Maybe its just the interface or the name. But I find this very 
> >>>>> non-intuitive
> >>>>
> >>>> I'm open for suggestions.
> >>>>
> >>>>>
> >>>>> e.g. you wrote
> >>>>>
> >>>>>     Get the maximum possible feature set (e.g., including deprecated
> >>>>>     features) for a CPU definition in the configuration ("everything 
> >>>>> that
> >>>>>     could be enabled"):
> >>>>>         -cpu z14,all-features=off,available-features=on
> >>>>>
> >>>>>     Get all valid features for a CPU definition:
> >>>>>         -cpu z14,all-features=on
> >>>>>
> >>>>> What is the point of this? It is either the same as the one before, or 
> >>>>> it wont
> >>>>> be able to start. 
> >>>>
> >>>> valid != available, all != available. Yes, the model won't run unless
> >>>> you are on pretty good HW :)
> >>>>
> >>>> Maybe I should just have dropped the last example, as it seems to
> >>>> confuse people - it's mostly only relevant for introspection via CPU
> >>>> model expansion.
> >>>>
> >>>> I am open for better names. e.g. all-features -> valid-features.
> >>>
> >>> "all" is not a meaningful name to me.  It surely doesn't mean
> >>> "all features in the universe", so it means a more specific set
> >>> of features.  How is that set defined?
> >>>
> >>> "valid" seems clearer, but we still need a description of what
> >>> "valid" means exactly.
> >>>
> >>
> >> So, we have
> >>
> >> +static S390DynFeatGroupDef s390_dyn_feature_groups[] = {
> >> +    /* "all" corresponds to our "full" definitions */
> >> +    DYN_FEAT_GROUP_INIT("all-features", ALL, "Features valid for a CPU
> >> definition"),
> >> [...]
> >> +};
> >>
> >> it includes features that are not available - all features that could
> >> theoretically be enabled for that CPU definition.
> >>
> >> (e.g., "vx" was introduced with z13 and cannot be enabled for the z12.
> >> It's part of the full model of a z13, but not of a z12)
> > 
> > Isn't this something already returned by device-list-properties?
> > 
> 
> We do register all feature properties for all models. So, yes, it would
> have been possible if we (I) would have implemented that differently. We
> could (and maybe should) still change that - only register the features
> that are part of the "full" model.

Understood.  When exactly would all-features=on be useful for
management software?

-- 
Eduardo