Re: [Qemu-devel] [PATCH 00/10] s390x: new guest features

[David Hildenbrand]

On 24.04.19 11:30, Daniel P. Berrangé wrote:
> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>
>>>
>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>> Adding generation 15.
>>>>>
>>>>> Some interesting aspects:
>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>   for csske.
>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>   patch)
>>>>>
>>>>> Christian Borntraeger (10):
>>>>>   linux header sync
>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>   s390x/cpumodel: msa9 facility
>>>>>   s390x/cpumodel: vector enhancements
>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>   s390x/cpumodel: deflate
>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>
>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>
>>>>
>>>> I guess to handle deprecation of CSSKE:
>>>>
>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>> Existing base models are not modified.
>>>>
>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>> will work. We can backport this to distros/stable.
>>>
>>> Yes, I have already implemented that, still doing some testing and 
>>> polishinh.
>>>>
>>>>
>>>> CPU model expansion:
>>>>
>>>> cpu_info_from_model() should already properly be based on the base
>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>> when expanding.
>>>>
>>>> CPU model baseline:
>>>>
>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>> when determining maximum model, however it will properly be indicated if
>>>> both models had the feature.
>>>>
>>>> CPU model comparison:
>>>>
>>>> Should work as expected. Availability of CSSKE will be considered when
>>>> calculating the result.
>>>>
>>>> gen14,csske=on and gen15,csske=off will result in
>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>
>>>> gen14,csske=off and gen15,csske=off should result in
>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>
>>>> gen14,csske=on and gen15,csske=on should result in
>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>
>>>> Forward migration:
>>>>
>>>> Now, the only issue is when csske is actually turned of in future
>>>> machines. We would e.g. have
>>>>
>>>> gen15,csske=on and gen16,csske=off
>>>>
>>>> While baselining will work correctly (gen15,csske=off), forward
>>>> migration is broken (comparison will properly indicate
>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>> out features. Same applies to BPB.
>>>>
>>>>
>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>> expanded models in qmp" tried to address this, however I am not really
>>>> happy with this approach. We should not play such tricks when expanding
>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>> different,
>>>
>>> We discussed this some time ago and I think we agreed that for host 
>>> passthrough
>>> it is ok to be different that host-model (e.g. passing through the cpuid, 
>>> passing
>>> through all non-hypervisor managed features etc).
>>
>> I remember the plan was to use the "max" model to do such stuff. E.g.
>> -cpu max / no -cpu
>>
>> Versus
>> -cpu host
>>
>> We can have features in "host" we don't have in "max". But "-cpu host"
>> and it's expansion should look 100% the same.
> 
> I don't think that's the intended semantics of "max" vs "host".
> 
> The "max" CPU model is supposed to enable all features that are possible to
> enable.
> 
> For KVM, thus "max" should be identical to "host".

There once was a mode used by x86-64 to simply pipe through cpuid
features that were not even supported. (I remember something like
passthorugh=true), do you remember if something like that still exists?

> 
> For TCG, "max" should be everything that QEMU currently knows how to emulate.

Yes, and on s390x. "max" contains more features than "qemu".

> 
> Essentially think of "max" as a better name for "host", since "host" as
> a name concept didn't make sense for TCG.

I agree. The main question is, is it acceptable that

"-cpu host" and "-cpu $expanded_host" produce different results, after
expanding "host" via query-cpu-model-expansion?

-- 

Thanks,

David / dhildenb