Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
> The topology information are attributes of the CPU and are
> specified during the CPU device creation.
> 
> On hot plug we:
> - calculate the default values for the topology for drawers,
>   books and sockets in the case they are not specified.
> - verify the CPU attributes
> - check that we have still room on the desired socket
> 
> The possibility to insert a CPU in a mask is dependent on the
> number of cores allowed in a socket, a book or a drawer, the
> checking is done during the hot plug of the CPU to have an
> immediate answer.
> 
> If the complete topology is not specified, the core is added
> in the physical topology based on its core ID and it gets
> defaults values for the modifier attributes.
> 
> This way, starting QEMU without specifying the topology can
> still get some advantage of the CPU topology.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  MAINTAINERS                        |   1 +
>  include/hw/s390x/cpu-topology.h    |  44 +++++
>  include/hw/s390x/s390-virtio-ccw.h |   1 +
>  hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c         |  22 ++-
>  hw/s390x/meson.build               |   1 +
>  6 files changed, 349 insertions(+), 2 deletions(-)
>  create mode 100644 hw/s390x/cpu-topology.c

[...]

>  #endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9bba21a916..ea10a6c6e1 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>      bool dea_key_wrap;
>      bool pv;
>      uint8_t loadparm[8];
> +    bool vertical_polarization;

Why is this here and not in s390_topology?
This splits up the topology state somewhat.
I don't quite recall, did you use to have s390_topology in S390CcwMachineState at some point?
I think putting everything in S390CcwMachineState might make sense too.

>  };
>  
>  struct S390CcwMachineClass {
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..96da67bd7e
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c

[...]

> +/**
> + * s390_topology_cpu_default:
> + * @cpu: pointer to a S390CPU
> + * @errp: Error pointer
> + *
> + * Setup the default topology if no attributes are already set.
> + * Passing a CPU with some, but not all, attributes set is considered
> + * an error.
> + *
> + * The function calculates the (drawer_id, book_id, socket_id)
> + * topology by filling the cores starting from the first socket
> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
> + *
> + * CPU type and dedication have defaults values set in the
> + * s390x_cpu_properties, entitlement must be adjust depending on the
> + * dedication.
> + */
> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    CPUS390XState *env = &cpu->env;
> +
> +    /* All geometry topology attributes must be set or all unset */
> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
> +        error_setg(errp,
> +                   "Please define all or none of the topology geometry attributes");
> +        return;
> +    }
> +
> +    /* Check if one of the geometry topology is unset */
> +    if (env->socket_id < 0) {
> +        /* Calculate default geometry topology attributes */
> +        env->socket_id = s390_std_socket(env->core_id, smp);
> +        env->book_id = s390_std_book(env->core_id, smp);
> +        env->drawer_id = s390_std_drawer(env->core_id, smp);
> +    }
> +
> +    /*
> +     * Even the user can specify the entitlement as horizontal on the
> +     * command line, qemu will only use env->entitlement during vertical
> +     * polarization.
> +     * Medium entitlement is chosen as the default entitlement when the CPU
> +     * is not dedicated.
> +     * A dedicated CPU always receives a high entitlement.
> +     */
> +    if (env->entitlement >= S390_CPU_ENTITLEMENT__MAX ||
> +        env->entitlement == S390_CPU_ENTITLEMENT_HORIZONTAL) {
> +        if (env->dedicated) {
> +            env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
> +        } else {
> +            env->entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
> +        }
> +    }

As you know, in my opinion there should be not possibility for the user
to set the entitlement to horizontal and dedicated && != HIGH should be
rejected as an error.
If you do this, you can delete this.

> +}
> +
> +/**
> + * s390_topology_check:
> + * @socket_id: socket to check
> + * @book_id: book to check
> + * @drawer_id: drawer to check
> + * @entitlement: entitlement to check
> + * @dedicated: dedication to check
> + * @errp: Error pointer
> + *
> + * The function checks if the topology
> + * attributes fits inside the system topology.

fitS

The function checks the validity of the provided topology arguments,
namely that they're in bounds and non contradictory.

> + */
> +static void s390_topology_check(uint16_t socket_id, uint16_t book_id,

I'd prefer if you stick to one id type. There defined as int32_t in env,
here you use uint16_t and below int.

In env, you want a signed type with sufficient range, int16_t should suffice there,
but bigger is also fine.
You don't want the user to pass a negative id, so by using an unsigned type you
can avoid this without extra code.
But IMO there should be one point where a type conversion occurs.

> +                                uint16_t drawer_id, uint16_t entitlement,
> +                                bool dedicated, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    ERRP_GUARD();
> +
> +    if (socket_id >= smp->sockets) {
> +        error_setg(errp, "Unavailable socket: %d", socket_id);
> +        return;
> +    }
> +    if (book_id >= smp->books) {
> +        error_setg(errp, "Unavailable book: %d", book_id);
> +        return;
> +    }
> +    if (drawer_id >= smp->drawers) {
> +        error_setg(errp, "Unavailable drawer: %d", drawer_id);
> +        return;
> +    }
> +    if (entitlement >= S390_CPU_ENTITLEMENT__MAX) {
> +        error_setg(errp, "Unknown entitlement: %d", entitlement);
> +        return;
> +    }

I think you can delete this, too, there is no way that entitlement is > MAX.

> +    if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW ||
> +                      entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) {

Without HORIZONTAL you can do != HIGH and save one line, but that is
cosmetic only.

> +        error_setg(errp, "A dedicated cpu implies high entitlement");
> +        return;
> +    }
> +}

[...]