Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology

[Pierre Morel <pmorel@linux.ibm.com>]

On 12/2/22 15:26, Thomas Huth wrote:
> On 02/12/2022 15.08, Pierre Morel wrote:
>>
>>
>> On 12/2/22 10:05, Thomas Huth wrote:
>>> On 01/12/2022 12.52, Pierre Morel wrote:
>>>>
>>>>
>>>> On 12/1/22 11:15, Thomas Huth wrote:
>>>>> On 29/11/2022 18.42, Pierre Morel wrote:
>>>>>> The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
>>>>>> activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
>>>>>> the topology facility for the guest in the case the topology
>>>>>> is available in QEMU and in KVM.
>>>>>>
>>>>>> The feature is fenced for SE (secure execution).
>>>>>
>>>>> Out of curiosity: Why does it not work yet?
>>>>>
>>>>>> To allow smooth migration with old QEMU the feature is disabled by
>>>>>> default using the CPU flag -disable-topology.
>>>>>
>>>>> I stared at this code for a while now, but I have to admit that I 
>>>>> don't quite get it. Why do we need a new "disable" feature flag 
>>>>> here? I think it is pretty much impossible to set "ctop=on" with an 
>>>>> older version of QEMU, since it would require the QEMU to enable 
>>>>> KVM_CAP_S390_CPU_TOPOLOGY in the kernel for this feature bit - and 
>>>>> older versions of QEMU don't set this capability yet.
>>>>>
>>>>> Which scenario would fail without this disable-topology feature 
>>>>> bit? What do I miss?
>>>>
>>>> The only scenario it provides is that ctop is then disabled by 
>>>> default on newer QEMU allowing migration between old and new QEMU 
>>>> for older machine without changing the CPU flags.
>>>>
>>>> Otherwise, we would need -ctop=off on newer QEMU to disable the 
>>>> topology.
>>>
>>> Ah, it's because you added S390_FEAT_CONFIGURATION_TOPOLOGY to the 
>>> default feature set here:
>>>
>>>   static uint16_t default_GEN10_GA1[] = {
>>>       S390_FEAT_EDAT,
>>>       S390_FEAT_GROUP_MSA_EXT_2,
>>> +    S390_FEAT_DISABLE_CPU_TOPOLOGY,
>>> +    S390_FEAT_CONFIGURATION_TOPOLOGY,
>>>   };
>>>
>>> ?
>>>
>>> But what sense does it make to enable it by default, just to disable 
>>> it by default again with the S390_FEAT_DISABLE_CPU_TOPOLOGY feature? 
>>> ... sorry, I still don't quite get it, but maybe it's because my 
>>> sinuses are quite clogged due to a bad cold ... so if you could 
>>> elaborate again, that would be very appreciated!
>>>
>>> However, looking at this from a distance, I would not rather not add 
>>> this to any default older CPU model at all (since it also depends on 
>>> the kernel to have this feature enabled)? Enabling it in the host 
>>> model is still ok, since the host model is not migration safe anyway.
>>>
>>>   Thomas
>>>
>>
>> I think I did not understand what is exactly the request that was made 
>> about having a CPU flag to disable the topology when we decide to not 
>> have a new machine with new machine property.
>>
>> Let see what we have if the only change to mainline is to activate 
>> S390_FEAT_CONFIGURATION_TOPOLOGY with the KVM capability:
>>
>> In mainline, ctop is enabled in the full GEN10 only.
>>
>> Consequently we have this feature activated by default for the host 
>> model only and deactivated by default if we specify the CPU.
>> It can be activated if we specify the CPU with the flag ctop=on.
>>
>> This is what was in the patch series before the beginning of the 
>> discussion about having a new machine property for new machines.
> 
> Sorry for all the mess ... I'm also not an expert when it comes to CPU 
> model features paired with compatibility and migration, and I'm still in 
> progress of learning ...
> 
>> If this what we want: activating the topology by the CPU flag ctop=on 
>> it is perfect for me and I can take the original patch.
>> We may later make it a default for new machines.
> 
> Given my current understanding, I think it's the best thing to do right 
> now. Not enable it by default, except for the host model where the 
> enablement is fine since migration is not supported any.
> 
> As you said, we could still decide later to change the default for new 
> machines. Though, I recently learnt that features should also not be 
> enable by default at all if they depend on the environment, like a Linux 
> kernel that needs to have support for the feature. So maybe we should 
> keep it off by default forever - or just enable it on new CPU models 
> (>=z17?) that would require a new host kernel anyway.
> 
>   Thomas
> 

OK, thanks, so I let it with a default as off and we change that later 
in a new CPU model or a new machine as we will see what is the best fit.

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen