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

[David Hildenbrand]

On 24.04.19 12:38, Christian Borntraeger wrote:
> 
> 
> 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.

Makes sense, we already have forward migration issues to > gen15 with
guests started in the past when dropping features. No need to rush with
that.

-- 

Thanks,

David / dhildenb