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

[Markus Armbruster]

David Hildenbrand <address@hidden> writes:

> On 02.05.20 08:26, Markus Armbruster wrote:
>> David Hildenbrand <address@hidden> writes:
>> 
>>> On 30.04.20 20:22, Markus Armbruster wrote:
>>>> David Hildenbrand <address@hidden> writes:
>>>>
>>>>> On 28.04.20 18:34, Markus Armbruster wrote:
>>>>>> Both s390_features[S390_FEAT_PCC_CMAC_AES_256].name and
>>>>>> s390_features[S390_FEAT_PCC_CMAC_EAES_256].name is
>>>>>> "pcc-cmac-eaes-256".  The former is obviously a pasto.
>>>>>>
>>>>>> 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_realize_cpu_model() misidentifies it in check_consistency()
>>>>>>   warnings.

Actually not, because the feature isn't in check_consistency()'s dep[].
I'll drop this item.

>>>>>> * s390_cpu_list() likewise.  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.
>>>>>>
>>>>>> Fix by deleting the wayward 'e'.
>>>>>
>>>>> Very nice catch - thanks!
>>>>
>>>> :)
>>>>
>>>>> While this sounds very bad, it's luckily not that bad in practice
>>>>> (currently).
>>>>>
>>>>> The feature (or rather, both features) is part of the feature group
>>>>> "msa4". As long as we have all sub-features part of that group (which is
>>>>> usually the case), we will always indicate "msa4" to the user, instead
>>>>> of all the separate sub-features. So, expansion, baseline, comparison
>>>>> will usually only work with "msa4".
>>>>>
>>>>> (in addition, current KVM is not capable of actually masking off these
>>>>> sub-features, so it will still, always see the feature, even if not
>>>>> explicitly specified via "-cpu X,pcc-cmac-aes-256=on)
>>>>
>>>> Would you like to propose an commit message improvements?
>>>
>>> Maybe something like
>>>
>>> "Both affected features are part of the feature group msa4. In current
>>> setups, we will always see the msa4 feature instead of the separate
>>> contained sub-features (because all sub-features are around). Therefore,
>>> both features are currently never passed from/to the user explicitly
>>> (e.g., via cpu model expansion, comparison, baseline and '-cpu' setup)."
>>>
>>> Thanks!
>> 
>> I think I can guess how this could work for reporting features (I
>> haven't checked my guess against the code), which is what the
>> query-cpu-model-* do: suppress individual features when their group is
>> complete.
>
> Yes. Expand the group to single features on user input, expand the
> single features to the group on user output (if all features are enabled).

Okay, let's play.

Here's query-cpu-definitions:

    $ echo -e '{"execute": "qmp_capabilities"}\n{"execute": 
"query-cpu-definitions"}\n{"execute": "quit"}' | 
../qemu/bld/s390x-softmmu/qemu-system-s390x -qmp-pretty stdio -S -display none 
| egrep 'msa4|pcc-cmac-e?aes-256'

Before this patch:

         28                 "pcc-cmac-eaes-256",

After:

         14                 "pcc-cmac-aes-256",
         14                 "pcc-cmac-eaes-256",

No msa4.

Here's query-cpu-model-expansion for model "qemu" (arbitrarily chosen),
type "static":

    $ echo -e '{"execute": "qmp_capabilities"}\n{"execute": 
"query-cpu-model-expansion", "arguments": {"type": "full", "model": {"name": 
"qemu"}}}\n{"execute": "quit"}' | $i -qmp-pretty stdio -S -display none | egrep 
'msa4|pcc-cmac-e?aes-256' | sort | uniq -c

Before and after:

          1                 "msa4-base": true,

Same with type "full":

    $ echo -e '{"execute": "qmp_capabilities"}\n{"execute": 
"query-cpu-model-expansion", "arguments": {"type": "full", "model": {"name": 
"qemu"}}}\n{"execute": "quit"}' | $i -qmp-pretty stdio -S -display none | egrep 
'msa4|pcc-cmac-e?aes-256' | sort | uniq -c

Before:

          1                 "msa4-base": true,
          1                 "pcc-cmac-eaes-256": false,

After

          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.

>> 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.

>> While testing, I noticed that
>> 
>>     $ s390x-softmmu/qemu-system-s390x
>> 
>> flashes a window at me, then terminates successfully, without printing
>> anything.  With -S, it behaves like other targets.  Bug?
>> 
>
> Think this is expected.
>
> t480s: ~  $ qemu-system-s390x --nographic
> LOADPARM=[        ]
> Could not find a suitable boot device (none specified)
>
> The s390-ccw bios will come up, detect that there is nothing to boot and
> quit. The bios can only print to the sclp console, not to a graphical
> output.
>
> What the others do (e.g., ppc64, x86_64) is boot the bios/firmware and
> then halt there.

Thanks!