[libvirt] [PATCH v4 0/8] CPU Model Baseline and Comparison for s390x

[Collin Walling]

On 9/10/19 11:38 AM, David Hildenbrand wrote:
> On 10.09.19 17:22, Jiri Denemark wrote:
>> On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:
>>> On 8/20/19 10:06 AM, Jiri Denemark wrote:
>>>> First, let me apologize for such a late review. I'll try my best to
>>>> review your series earlier next time.
>>>>
>>>
>>> Your review is greatly appreciated! I haven't replied to your other
>>> posts on this series as my comments were mostly acknowledgements rather
>>> than discussion pieces. I'm working through them.
>>>
>>>> On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
>>>>> When baselining CPU models and the user appends the --features argument
>>>>> to the command, s390x will only report back features that supersede the
>>>>> base model definition.
>>>>>
>>>>> *NOTE* if the --features flag is intended to expand /ALL/ features
>>>>> available to a CPU model (such as the huge list of features reported
>>>>> by a full CPU model expansion), please let me know and I can resolve
>>>>> this.
>>>>
>>>> I'm not sure how well this fits s390 world, but the semantics of
>>>> VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
>>>> features which are hidden behind the CPU model. That is, all feature
>>>> which you'd get when starting QEMU with just the CPU model name and no
>>>> additional features. The extra features should not be touched at all.
>>>> Specifically, removing them when the flag is not used is wrong.
>>>>
>>>> To me this looks like the flag should really result in running
>>>> query-cpu-model-expansion (but likely the "static" one rather then
>>>> "full" expansion) on the baseline CPU model and reporting the enabled
>>>> features along with those already included in the baseline feature set.
>>>>
>>>
>>> Actually, query-cpu-model-baseline will return a CPU model along with a
>>> feature set. The features returned are the same as those produced from a
>>> static expansion on the model.
>>>
>>> Correct me if I am wrong here: virsh should report features *only* if the
>>> --features flag is present. Otherwise, we only report the model name (which
>>> can be accomplished by stripping the result of all reported features).
>>
>> No, this is wrong in general (although it maybe correct for s390, I
>> don't know).
> 
> David here: Not correct for s390x
> 
>>
>> Let me explain with some hypothetical CPU models.
>>
>> Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
>> Model2 will enable f1, f2, f3, f4, f5.
>> Model3 will enable f1, f2, f3, f4, f6.
>>
>> Running baseline command for [Model1, Model2, Model3] should return
>> Model1 with no additional features as [f1, f2, f3] are in only ones
>> common to all three models and they are all enabled by Model1.
>>
>> However, baseline of [Model2, Model3] should return Model1 + f4. The
>> common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
>> f2, f3] only, which means we need to add f4 explicitly.
>>
>> With --features, even the features enabled implicitly by a specific CPU
>> model should be returned. That is, the first result would be Model1 +
>> [f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].
>>
>> In other words, stripping all features if no --features option is used
>> is wrong. Only features implicitly enabled by the CPU model should be
>> stripped.
>>
>> Specifically, baseline without --features on the following CPU
>> definition
>>
>>      <cpu mode='custom' match='exact'>
>>        <model>Model1</model>
>>        <feature name='f4' policy='require'/>
>>      </cpu>
>>
>> should be the same definition (i.e., Model1 + f4).
> 
> s390x will fallback to base models when baselining/expanding ("minimum
> feature set we expect to be around for a certain cpu generation", which
> is independent of the QEMU version and will never change). But if you're
> already using base models, it works as you would expect it.
> 
> E.g.
> 
> Model1 is [f1, f2, f3]
> Model1-base is [f1, f2]
> 
> Model2 is [f1, f2, f3, f4]
> Model2-base is [f1, f2, f3]
> 
> Baselining Model1 with Model2 would result in
> Model1-base + f3
> 
> For this, QMP "query-cpu-model-baseline" can be used.
> 
> Note: When expending cpu models (e.g. host), one would already always
> get base models. So there, it would work completely as you expect it.
> 
> Now, to achieve the "--features" part, simply throw the result of
> "query-cpu-model-baseline" into "query-cpu-model-expansion" with
> "CpuModelExpansionType" == "full"
> 
>>
>> The question is whether you can enabled extra features on s390 or you
>> can only use plain CPU models with no additional features. If additional
>> features are allowed, I'd guess baseline without features should return
>> the result of the QMP command minus the features a static expansion of
>> the CPU model itself would return (this is assuming f4 will be included
>> in the result we get from QEMU for Model1 + f4). When --features is
>> used, the result from QEMU should be returned.
> 
> We do have plenty of extra cpu features on s390x. *Plenty* :)
> 

Indeed, there's quite a few.

>>
>> I hope it's clear now. However, I'm not sure how CPU models work on
>> s390 and I'd be happy if you could explain it to me. Especially, whether
>> something similar to what I described can happen there.
>>
>> Jirka
>>
> 
> 

Yes, both you and David have provided an excellent explanation of what's
expected (from both perspectives). Thank you both. I'll make the
appropriate changes.