Re: [PATCH 08/17] s390x/cpumodel: Fix UI to CPU features pcc-cmac-{aes, eaes}-256

[Markus Armbruster]

David Hildenbrand <address@hidden> writes:

> [...]
>
>>           1                 "msa4-base": true,
>>           1                 "pcc-cmac-aes-256": false,
>>           1                 "pcc-cmac-eaes-256": false,
>> 
>> The grouping and masking you described appears to apply to
>> query-cpu-model-expansion with type "static".  With type "full", I can
>> see the grouping, but not the masking.  With query-cpu-definitions, I
>> can't see either.
>> 
>> I haven't played with query-cpu-model-comparison and
>> query-cpu-model-baseline.
>
> Grouping will be done whenever all features part of a group are to be
> reported. Please note that "msa4-base" is part of "msa4".
>
> You chose the only model where we do have msa4-base but none of the
> other msa4 features - the qemu model.

Ah, "lucky" random pick.

> So when you do a "query-cpu-definitions" under TCG (which basically maps
> to the qemu model and only has the msa4-base feature), then you do have
> msa4-base of all relevant models, but none of the other sub-features
> they define. That's why you don't see msa4 explicitly, but instead the
> subfeatures.
>
> query-cpu-model-expansion and the others behave similar the same way
> with the "qemu" model. Note that the qemu model is not really used under
> KVM on s390x. There, we use "host" as default.

Next random pick: z13.2 shows msa4 before and after.

>>>> But "'-cpu' setup" doesn't seem to be about reporting features.  Am I
>>>> confused?
>>>>
>>>
>>> Let me clarify. Any user input would be broken if the two sub-features
>>> would be specified explicitly, instead of the whole "msa4" group. This
>>> applies to any user input, also the user input for query-cpu-model-.
>>>
>>> In the usual cases, libvirt will expand a cpu model (e.g., "host",
>>> "z15") and start QEMU with that (-cpu ...). We will only have the
>>> complete msa4 group here in practice.
>>>
>>> Yes, if some user would pick and chose such features manually, it would
>>> be broken - it's just not the common on s390x with the huge amount of
>>> features. But that's why I thing stable backports still make sense.
>> 
>> The commit message should be accurate and sufficiently precise.  The
>> "sufficiently" gives me some wiggle room to avoid inaccuracy due to my
>> ignorance.  Would the following be good enough?
>> 
>>     Impact:
>>     
>>     * s390_feat_bitmap_to_ascii() misidentifies S390_FEAT_PCC_CMAC_AES_256
>>       as "pcc-cmac-eaes-256".  Affects QMP commands query-cpu-definitions,
>>       query-cpu-model-expansion, query-cpu-model-baseline,
>>       query-cpu-model-comparison, and the error message when
>>       s390_realize_cpu_model() fails in check_compatibility().
>>     
>>     * s390_cpu_list() also misidentifies it.  Affects -cpu help.
>>     
>>     * s390_cpu_model_register_props() creates CPU property
>>       "pcc-cmac-eaes-256" twice.  The second one fails, but the error is
>>       ignored (a later commit will change that).  Results in a single
>>       property "pcc-cmac-eaes-256" with the description for
>>       S390_FEAT_PCC_CMAC_AES_256, and no property for
>>       S390_FEAT_PCC_CMAC_EAES_256.  CPU properties are visible in CLI -cpu
>>       and -device, QMP & HMP device_add, QMP device-list-properties, and
>>       QOM introspection.
>> 
>>     The two features are almost always used via their group msa4.  Such
>>     use is not affected by this bug.
>
> Yeah, sounds good, thanks.

Excellent.