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

[Christian Borntraeger]

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


> that is really confusing, especially think about baselining
> or comparing against "host", where your patch does not apply..
> 
> 
> Whenever users would specify "-cpu gen15", everything would be fine in
> regards to forward migration. Maybe clients really have to be taught
> about upcoming forward migration issues. "Don't use the host model, make
> sure bpa and csske are disabled".


the thing is, I think the host-model is actually the preferred setting so I
would like to have this forward-migrateable as well. I will focus on patches
1-9 first and lets have a side discussion on patch 10.