Re: [PATCH v16 02/11] s390x/cpu topology: add topology entries on CPU hotplug

[Thomas Huth <thuth@redhat.com>]

On 22/02/2023 15.20, Pierre Morel wrote:
> The topology information are attributes of the CPU and are
> specified during the CPU device creation.
...
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 83f31604cc..fa7f885a9f 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -10,6 +10,47 @@
>   #ifndef HW_S390X_CPU_TOPOLOGY_H
>   #define HW_S390X_CPU_TOPOLOGY_H
>   
> +#include "qemu/queue.h"
> +#include "hw/boards.h"
> +#include "qapi/qapi-types-machine-target.h"
> +
>   #define S390_TOPOLOGY_CPU_IFL   0x03
>   
> +typedef struct S390Topology {
> +    uint8_t *cores_per_socket;
> +    CpuTopology *smp;
> +    CpuS390Polarization polarization;
> +} S390Topology;
> +
> +#ifdef CONFIG_KVM
> +bool s390_has_topology(void);
> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
> +#else
> +static inline bool s390_has_topology(void)
> +{
> +       return false;
> +}
> +static inline void s390_topology_setup_cpu(MachineState *ms,
> +                                           S390CPU *cpu,
> +                                           Error **errp) {}
> +#endif
> +
> +extern S390Topology s390_topology;
> +int s390_socket_nb(S390CPU *cpu);
> +
> +static inline int s390_std_socket(int n, CpuTopology *smp)
> +{
> +    return (n / smp->cores) % smp->sockets;
> +}
> +
> +static inline int s390_std_book(int n, CpuTopology *smp)
> +{
> +    return (n / (smp->cores * smp->sockets)) % smp->books;
> +}
> +
> +static inline int s390_std_drawer(int n, CpuTopology *smp)
> +{
> +    return (n / (smp->cores * smp->sockets * smp->books)) % smp->books;

Shouldn't that be " % smp->drawers" instead?

> +}
> +
>   #endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..59f2cc15c7
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,270 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> +
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/boards.h"
> +#include "qemu/typedefs.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +/*
> + * s390_topology is used to keep the topology information.
> + * .cores_per_socket: tracks information on the count of cores
> + *                    per socket.
> + * .smp: keeps track of the machine topology.
> + *
> + */
> +S390Topology s390_topology = {
> +    /* will be initialized after the cpu model is realized */
> +    .cores_per_socket = NULL,
> +    .smp = NULL,
> +    .polarization = S390_CPU_POLARIZATION_HORIZONTAL,
> +};
> +
> +/**
> + * s390_socket_nb:
> + * @cpu: s390x CPU
> + *
> + * Returns the socket number used inside the cores_per_socket array
> + * for a cpu.
> + */
> +int s390_socket_nb(S390CPU *cpu)
> +{
> +    return (cpu->env.drawer_id * s390_topology.smp->books + cpu->env.book_id) *
> +           s390_topology.smp->sockets + cpu->env.socket_id;
> +}
> +
> +/**
> + * s390_has_topology:
> + *
> + * Return value: if the topology is supported by the machine.
> + */
> +bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +/**
> + * s390_topology_init:
> + * @ms: the machine state where the machine topology is defined
> + *
> + * Keep track of the machine topology.
> + *
> + * Allocate an array to keep the count of cores per socket.
> + * The index of the array starts at socket 0 from book 0 and
> + * drawer 0 up to the maximum allowed by the machine topology.
> + */
> +static void s390_topology_init(MachineState *ms)
> +{
> +    CpuTopology *smp = &ms->smp;
> +
> +    s390_topology.smp = smp;
> +    s390_topology.cores_per_socket = g_new0(uint8_t, smp->sockets *
> +                                            smp->books * smp->drawers);
> +}
> +
> +/**
> + * 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, entitlement and dedication have defaults values set in the
> + * s390x_cpu_properties, however entitlement is forced to 0 'none' when
> + * the polarization is horizontale.
> + */
> +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);
> +    }
> +
> +    if (s390_topology.polarization == S390_CPU_POLARIZATION_HORIZONTAL) {
> +        env->entitlement = 0;

Should this be S390_CPU_ENTITLEMENT_HORIZONTAL instead of 0 ?

> +    }
> +}

  Thomas