Re: [PATCH v15 06/11] s390x/cpu topology: interception of PTF instruction

[Nina Schoetterl-Glausch]

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@linux.ibm.com>
> ---
>  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.

> +{
> +    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?

> +
> +    /*
> +     * Machine polarity is set inside the global s390_topology structure.
> +     * In the case the polarity is set as horizontal set the entitlement
> +     * 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.

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.
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?

> +        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.
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.

> +    }
>  }
>  

[...]