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

[Christian Borntraeger]

On 24.04.19 12:27, David Hildenbrand wrote:
> On 24.04.19 12:26, Christian Borntraeger wrote:
>>
>>
>> On 24.04.19 11:56, David Hildenbrand wrote:
>>> On 24.04.19 11:41, Daniel P. Berrangé wrote:
>>>> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
>>>>> 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?
>>>>
>>>> I don't recall such a feature existing even in the past !
>>>
>>> Maybe my mind is tricking me, or maybe that has long been removed :)
>>>
>>>>
>>>>>> 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
>>>>
>>>> Hmm, maybe I misinterpreted when you wrote
>>>>
>>>>       We can have features in "host" we don't have in "max"
>>>>
>>>> I read that as meaning that "-cpu host" and "-cpu max" would be
>>>> different.
>>>
>>> No you didn't misinterpret it, I agreed after you spelled it out :)
>>>
>>>>
>>>>> "-cpu host" and "-cpu $expanded_host" produce different results, after
>>>>> expanding "host" via query-cpu-model-expansion?
>>>>
>>>> That has always been the case on x86-64, since it is not possible to set
>>>> the level, xlevel, vendor, family, model & stepping properties via -cpu,
>>>> only the feature flags.
>>>
>>> Fair enough, but the question is if we should mess with feature flags we
>>> can indicate on that level.
>>>
>>> It would mean that on a specific host e.g.
>>>
>>> "-cpu gen15,csske=on" and "-cpu gen15,csske=off"
>>>
>>> Would work. However, "host" model expansion would give us
>>>
>>> "-cpu gen15,csske=off"
>>>
>>> So trying to e.g. do a query-cpu-model-comparison or
>>> query-cpu-model-baseline against "host" and against the expanded host
>>> model will produce different results.
>>>
>>> Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
>>> because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
>>> be "incompatible". But running "-cpu gen14,csske=on" on the host would
>>> work perfectly fine.
>>
>> I would like to avoid special knowledge in libvirt (since we moved to have
>> everything in qemu). 
>>
>> A more complex idea would be to extend the qmp query with a list of 
>> deprecated
>> features and libvirt could then disable that for expansion but allow it for
>> baselining.
> 
> That would fit in nicely into query-cpu-model-expansion. Nice idea.

Hmm, I think that should also be possible as an add-on patch series later on
(e.g. in 6 month or so). We would have instances of gen15 machines with 
host-model
that start with csske but sooner or later they will be restarted and then they 
no 
longer have csske.
As we do not change anything for baselining this would allow us to separate 
patch10
from the other patches and deal with it later with a proper series on its on?
Need to think about that.