On 2/6/23 19:34, Nina Schoetterl-Glausch wrote:
On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
When the host supports the CPU topology facility, the PTF
instruction with function code 2 is interpreted by the SIE,
provided that the userland hypervizor activates the interpretation
by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
The PTF instructions with function code 0 and 1 are intercepted
and must be emulated by the userland hypervizor.
During RESET all CPU of the configuration are placed in
horizontal polarity.
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
include/hw/s390x/s390-virtio-ccw.h | 6 ++
target/s390x/cpu.h | 1 +
hw/s390x/cpu-topology.c | 103 +++++++++++++++++++++++++++++
target/s390x/cpu-sysemu.c | 14 ++++
target/s390x/kvm/kvm.c | 11 +++
5 files changed, 135 insertions(+)
[...]
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index cf63f3dd01..1028bf4476 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -85,16 +85,104 @@ static void s390_topology_init(MachineState *ms)
QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next);
}
+/**
+ * s390_topology_set_cpus_polarity:
+ * @polarity: polarity requested by the caller
+ *
+ * Set all CPU entitlement according to polarity and
+ * dedication.
+ * Default vertical entitlement is POLARITY_VERTICAL_MEDIUM as
+ * it does not require host modification of the CPU provisioning
+ * until the host decide to modify individual CPU provisioning
+ * using QAPI interface.
+ * However a dedicated vCPU will have a POLARITY_VERTICAL_HIGH
+ * entitlement.
+ */
+static void s390_topology_set_cpus_polarity(int polarity)
Since you set the entitlement field I'd prefer _set_cpus_entitlement or similar.
OK if you prefer.
+{
+ CPUState *cs;
+
+ CPU_FOREACH(cs) {
+ if (polarity == POLARITY_HORIZONTAL) {
+ S390_CPU(cs)->env.entitlement = 0;
+ } else if (S390_CPU(cs)->env.dedicated) {
+ S390_CPU(cs)->env.entitlement = POLARITY_VERTICAL_HIGH;
+ } else {
+ S390_CPU(cs)->env.entitlement = POLARITY_VERTICAL_MEDIUM;
+ }
+ }
+}
+
[...]
/**
@@ -137,6 +225,21 @@ static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
(smp->books * smp->sockets * smp->cores)) %
smp->drawers;
}
Why are the changes below in this patch?
Because before thos patch we have only horizontal polarization.
+
+ /*
+ * Machine polarity is set inside the global s390_topology structure.
+ * In the case the polarity is set as horizontal set the entitlement
Sorry here an error in the comment should be :
"In the case the polarity is NOT set as horizontal..."
+ * to POLARITY_VERTICAL_MEDIUM which is the better equivalent when
+ * machine polarity is set to vertical or POLARITY_VERTICAL_HIGH if
+ * the vCPU is dedicated.
+ */
+ if (s390_topology.polarity && !env->entitlement) {
It'd be more readable if you compared against enum values by name.
Right, I will change this to
if (s390_topology.polarity != S390_POLARITY_HORIZONTAL &&
env->entitlement == S390_ENTITLEMENT_UNSET) {
I don't see why you check s390_topology.polarity. If it is horizontal
then the value of the entitlement doesn't matter at all, so you can set it
to whatever.
Right, that is why it is done only for vertical polarization (sorry for
the wrong comment)
All you want to do is enforce dedicated -> VERTICAL_HIGH, right?
So why don't you just add
+ if (cpu->env.dedicated && cpu->env.entitlement != POLARITY_VERTICAL_HIGH) {
+ error_setg(errp, "A dedicated cpu implies high entitlement");
+ return;
+ } >
to s390_topology_check?
Here it is to set the default in the case the values are not provided.
But where you are right is that I should add a verification to the check
function.
+ if (env->dedicated) {
+ env->entitlement = POLARITY_VERTICAL_HIGH;
+ } else {
+ env->entitlement = POLARITY_VERTICAL_MEDIUM;
+ }
If it is horizontal, then setting the entitlement is pointless as it will be
reset to medium on PTF.
That is why the polarity is tested (sorry for the bad comment)
So the current polarization is vertical and a cpu is being hotplugged,
but setting the entitlement of the cpu being added is also pointless, because
it's determined by the dedication. That seems weird.
No it is not determined by the dedication, if there is no dedication the
3 vertical values are possible.
Regards,
Pierre
+ }
}
[...]
--
Pierre Morel
IBM Lab Boeblingen
