| Message ID | 20221129174206.84882-7-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
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? > Making the S390_FEAT_CONFIGURATION_TOPOLOGY belonging to the > default features makes the -ctop CPU flag is no more necessary, too many verbs in this sentence ;-) > turning the topology feature on is done with -disable-topology > only. ... Thomas
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. > >> Making the S390_FEAT_CONFIGURATION_TOPOLOGY belonging to the >> default features makes the -ctop CPU flag is no more necessary, > > too many verbs in this sentence ;-) definitively :) > >> turning the topology feature on is done with -disable-topology >> only. > ... > > Thomas > >
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
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. 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. Otherwise or if I misunderstood something, which is greatly possible, I need more advises. Regards, Pierre
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
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
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h index e88059ccec..b2fa24ba93 100644 --- a/include/hw/s390x/cpu-topology.h +++ b/include/hw/s390x/cpu-topology.h @@ -36,9 +36,6 @@ struct S390Topology { #define TYPE_S390_CPU_TOPOLOGY "s390-topology" OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY) -static inline bool s390_has_topology(void) -{ - return false; -} +bool s390_has_topology(void); #endif diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc index e3cfe63735..016a720e38 100644 --- a/target/s390x/cpu_features_def.h.inc +++ b/target/s390x/cpu_features_def.h.inc @@ -147,6 +147,7 @@ DEF_FEAT(SIE_CEI, "cei", SCLP_CPU, 43, "SIE: Conditional-external-interception f DEF_FEAT(DAT_ENH_2, "dateh2", MISC, 0, "DAT-enhancement facility 2") DEF_FEAT(CMM, "cmm", MISC, 0, "Collaborative-memory-management facility") DEF_FEAT(AP, "ap", MISC, 0, "AP instructions installed") +DEF_FEAT(DISABLE_CPU_TOPOLOGY, "disable-topology", MISC, 0, "Disable CPU Topology") /* Features exposed via the PLO instruction. */ DEF_FEAT(PLO_CL, "plo-cl", PLO, 0, "PLO Compare and load (32 bit in general registers)") diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index c3a4f80633..1f5348d6a3 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -253,6 +253,7 @@ bool s390_has_feat(S390Feat feat) case S390_FEAT_SIE_CMMA: case S390_FEAT_SIE_PFMFI: case S390_FEAT_SIE_IBS: + case S390_FEAT_CONFIGURATION_TOPOLOGY: return false; break; default: @@ -422,6 +423,21 @@ void s390_cpu_list(void) } } +static void check_incompatibility(S390CPUModel *model, Error **errp) +{ + static int dep[][2] = { + { S390_FEAT_CONFIGURATION_TOPOLOGY, S390_FEAT_DISABLE_CPU_TOPOLOGY }, + }; + int i; + + for (i = 0; i < ARRAY_SIZE(dep); i++) { + if (test_bit(dep[i][0], model->features) && + test_bit(dep[i][1], model->features)) { + clear_bit(dep[i][0], model->features); + } + } +} + static void check_consistency(const S390CPUModel *model) { static int dep[][2] = { @@ -592,6 +608,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp) cpu->model->cpu_id_format = max_model->cpu_id_format; cpu->model->cpu_ver = max_model->cpu_ver; + check_incompatibility(cpu->model, &err); check_consistency(cpu->model); check_compatibility(max_model, cpu->model, &err); if (err) { diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c index b81f016ba1..8123e6ddf0 100644 --- a/target/s390x/cpu_topology.c +++ b/target/s390x/cpu_topology.c @@ -15,6 +15,11 @@ #include "hw/s390x/cpu-topology.h" #include "hw/s390x/sclp.h" +bool s390_has_topology(void) +{ + return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY); +} + /* * s390_topology_add_cpu: * @topo: pointer to the topology diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index 1e3b7c0dc9..f3acfdd9a5 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -488,6 +488,7 @@ static uint16_t full_GEN9_GA3[] = { static uint16_t full_GEN10_GA1[] = { S390_FEAT_EDAT, S390_FEAT_CONFIGURATION_TOPOLOGY, + S390_FEAT_DISABLE_CPU_TOPOLOGY, S390_FEAT_GROUP_MSA_EXT_2, S390_FEAT_ESOP, S390_FEAT_SIE_PFMFI, @@ -605,6 +606,8 @@ static uint16_t default_GEN9_GA1[] = { static uint16_t default_GEN10_GA1[] = { S390_FEAT_EDAT, S390_FEAT_GROUP_MSA_EXT_2, + S390_FEAT_DISABLE_CPU_TOPOLOGY, + S390_FEAT_CONFIGURATION_TOPOLOGY, }; #define default_GEN10_GA2 EmptyFeat diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index a79fdf1c79..ec2c9fd8fa 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -2471,6 +2471,20 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) set_bit(S390_FEAT_UNPACK, model->features); } + /* + * If we have support for CPU Topology prevent overrule + * S390_FEAT_CONFIGURATION_TOPOLOGY with S390_FEAT_DISABLE_CPU_TOPOLOGY + * implemented in KVM, activate the CPU TOPOLOGY feature. + */ + if (kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) { + if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 0) < 0) { + error_setg(errp, "KVM: Error enabling KVM_CAP_S390_CPU_TOPOLOGY"); + return; + } + set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features); + set_bit(S390_FEAT_DISABLE_CPU_TOPOLOGY, model->features); + } + /* We emulate a zPCI bus and AEN, therefore we don't need HW support */ set_bit(S390_FEAT_ZPCI, model->features); set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
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). To allow smooth migration with old QEMU the feature is disabled by default using the CPU flag -disable-topology. Making the S390_FEAT_CONFIGURATION_TOPOLOGY belonging to the default features makes the -ctop CPU flag is no more necessary, turning the topology feature on is done with -disable-topology only. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- include/hw/s390x/cpu-topology.h | 5 +---- target/s390x/cpu_features_def.h.inc | 1 + target/s390x/cpu_models.c | 17 +++++++++++++++++ target/s390x/cpu_topology.c | 5 +++++ target/s390x/gen-features.c | 3 +++ target/s390x/kvm/kvm.c | 14 ++++++++++++++ 6 files changed, 41 insertions(+), 4 deletions(-)