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

From: Thomas Huth <thuth@redhat.com>
To: Pierre Morel <pmorel@linux.ibm.com>,
	qemu-s390x@nongnu.org, david@redhat.com
Cc: qemu-devel@nongnu.org, borntraeger@de.ibm.com,
	pasic@linux.ibm.com, richard.henderson@linaro.org,
	cohuck@redhat.com, mst@redhat.com, pbonzini@redhat.com,
	kvm@vger.kernel.org, ehabkost@redhat.com,
	marcel.apfelbaum@gmail.com, eblake@redhat.com, armbru@redhat.com,
	seiden@linux.ibm.com, nrb@linux.ibm.com, scgl@linux.ibm.com,
	frankja@linux.ibm.com, berrange@redhat.com, clg@kaod.org
Subject: Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology
Date: Fri, 2 Dec 2022 15:26:21 +0100	[thread overview]
Message-ID: <2c65f234-688a-796b-b451-e1661b2c07a4@redhat.com> (raw)
In-Reply-To: <59669e8e-6242-9c01-4c2e-5d70b9c31b2b@linux.ibm.com>

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