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

[Christian Borntraeger]

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.

> 
> 
> https://wiki.qemu.org/index.php/Features/CPUModels
> 
> "-cpu host vs -cpu best" is an outdated but interesting read.
> -> "-cpu host will be the "all-you-can-enable" mode"
> -> "We're not going to have a mode for match-host-CPU, probably "
> -> "A "best-predefined-model" mode can be implemented by libvirt. "
> 
> Sounds to me, that at least here, the issue should be sorted out either
> using a new cpu model type or on the libvirt level.
> 
> "Feature deprecated and will be removed soon, remove it from the host
> cpu model list if gen15 is used when setting up a new machine".
> 
>>
>> Regards,
>> Daniel
>>
> 
>