qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pierre Morel
Subject: Re: [PATCH v12 6/7] s390x/cpu_topology: activating CPU topology
Date: Mon, 5 Dec 2022 14:29:52 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]