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

[Nina Schoetterl-Glausch]

On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
> The topology information are attributes of the CPU and are
> specified during the CPU device creation.
> 
> On hot plug, we gather the topology information on the core,
> creates a list of topology entries, each entry contains a single
> core mask of each core with identical topology and finaly we
s/finaly/finally/
> orders the list in topological order.
s/orders/order/
> The topological order is, from higher to lower priority:
> - physical topology
>     - drawer
>     - book
>     - socket
>     - core origin, offset in 64bit increment from core 0.
> - modifier attributes
>     - CPU type
>     - polarization entitlement
>     - dedication
> 
> 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 adventage of the CPU topology.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/s390x/cpu-topology.h |  48 ++++++
>  hw/s390x/cpu-topology.c         | 293 ++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c      |  10 ++
>  hw/s390x/meson.build            |   1 +
>  4 files changed, 352 insertions(+)
>  create mode 100644 hw/s390x/cpu-topology.c
> 
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index d945b57fc3..b3fd752d8d 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -10,7 +10,11 @@
>  #ifndef HW_S390X_CPU_TOPOLOGY_H
>  #define HW_S390X_CPU_TOPOLOGY_H
>  
> +#include "qemu/queue.h"
> +#include "hw/boards.h"
> +
>  #define S390_TOPOLOGY_CPU_IFL   0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>  
>  #define S390_TOPOLOGY_POLARITY_HORIZONTAL      0x00
>  #define S390_TOPOLOGY_POLARITY_VERTICAL_LOW    0x01
> @@ -20,4 +24,48 @@
>  #define S390_TOPOLOGY_SHARED    0x00
>  #define S390_TOPOLOGY_DEDICATED 0x01
>  
> +typedef union s390_topology_id {
> +    uint64_t id;
> +    struct {
> +        uint64_t level_6:8; /* byte 0 BE */
> +        uint64_t level_5:8; /* byte 1 BE */
> +        uint64_t drawer:8;  /* byte 2 BE */
> +        uint64_t book:8;    /* byte 3 BE */
> +        uint64_t socket:8;  /* byte 4 BE */
> +        uint64_t rsrv:5;
> +        uint64_t d:1;
> +        uint64_t p:2;       /* byte 5 BE */
> +        uint64_t type:8;    /* byte 6 BE */
> +        uint64_t origin:2;

This is two bits because it's the core divided by 64, and we have 248 cores at 
most?
Where is this set?

> +        uint64_t core:6;    /* byte 7 BE */
> +    };
> +} s390_topology_id;

This struct seems to do double duty, 1. it represents a cpu and 2. a topology 
entry.
You also use it for sorting.
I would suggest to just use a cpu object when referring to a specific cpu and
put the relevant fields directly into the topology entry.
You get rid of the bit field that way.
You'd then need a comparison function for a cpu object and a topology entry.
As long as that isn't the only type pair that shouldn't be too ugly.

> +#define TOPO_CPU_MASK       0x000000000000003fUL
> +
> +typedef struct S390TopologyEntry {
> +    s390_topology_id id;
> +    QTAILQ_ENTRY(S390TopologyEntry) next;
> +    uint64_t mask;
> +} S390TopologyEntry;
> +
> +typedef struct S390Topology {
> +    QTAILQ_HEAD(, S390TopologyEntry) list;
> +    uint8_t *sockets;
> +    CpuTopology *smp;
> +} S390Topology;
> +
> +#ifdef CONFIG_KVM
> +bool s390_has_topology(void);
> +void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
> +#else
> +static inline bool s390_has_topology(void)
> +{
> +       return false;
> +}
> +static inline void s390_topology_set_cpu(MachineState *ms,
> +                                         S390CPU *cpu,
> +                                         Error **errp) {}
> +#endif
> +extern S390Topology s390_topology;
> +
>  #endif
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..438055c612
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,293 @@
> +/*
> + * 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.
> + * .list: queue the topology entries inside which
> + *        we keep the information on the CPU topology.
> + *
> + * .smp: keeps track of the machine topology.
> + *
> + * .socket: tracks information on the count of cores per socket.
> + *
> + */
> +S390Topology s390_topology = {
> +    .list = QTAILQ_HEAD_INITIALIZER(s390_topology.list),
> +    .sockets = NULL, /* will be initialized after the cpu model is realized 
> */

I guess you should do the same for .smp then also.

> +};
> +
> +/**
> + * s390_socket_nb:
> + * @id: s390_topology_id
> + *
> + * Returns the socket number used inside the socket array.
> + */
> +static int s390_socket_nb(s390_topology_id id)
> +{
> +    return (id.socket + 1) * (id.book + 1) * (id.drawer + 1);

This calculation doesn't make a whole lot of sense to me.
It's symmetric with regards to the variables, so (s=0 b=1 d=1)
will have the same result as (s=1 b=0 d=1).
You want the "global" socket number right?
So that would be (drawer * books_per_drawer + book) * sockets_per_book + socket.

> +}
> +
> +/**
> + * 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;
> +    if (!s390_topology.sockets) {

Is this function being called multiple times, or why the if?
Use an assert instead?

> +        s390_topology.sockets = g_new0(uint8_t, smp->sockets *
> +                                       smp->books * smp->drawers);
> +    }
> +}
> +
> +/**
> + * s390_topology_from_cpu:
> + * @cpu: The S390CPU
> + *
> + * Initialize the topology id from the CPU environment.
> + */
> +static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
> +{
> +    s390_topology_id topology_id;
> +
> +    topology_id.core = cpu->env.core_id;
> +    topology_id.type = cpu->env.cpu_type;
> +    topology_id.p = cpu->env.polarity;
> +    topology_id.d = cpu->env.dedicated;
> +    topology_id.socket = cpu->env.socket_id;
> +    topology_id.book = cpu->env.book_id;
> +    topology_id.drawer = cpu->env.drawer_id;
> +
> +    return topology_id;
> +}
> +
> +/**
> + * s390_topology_set_entry:
> + * @entry: Topology entry to setup
> + * @id: topology id to use for the setup
> + *
> + * Set the core bit inside the topology mask and
> + * increments the number of cores for the socket.
> + */
> +static void s390_topology_set_entry(S390TopologyEntry *entry,

Not sure if I like the name, what it does is to add a cpu to the entry.

> +                                    s390_topology_id id)
> +{
> +    set_bit(63 - id.core, &entry->mask);

You need to subtract the origin first or that might be negative.

> +    s390_topology.sockets[s390_socket_nb(id)]++;
> +}
> +
> +/**
> + * s390_topology_new_entry:
> + * @id: s390_topology_id to add
> + *
> + * Allocate a new entry and initialize it.
> + *
> + * returns the newly allocated entry.
> + */
> +static S390TopologyEntry *s390_topology_new_entry(s390_topology_id id)
> +{
> +    S390TopologyEntry *entry;
> +
> +    entry = g_malloc0(sizeof(S390TopologyEntry));
> +    entry->id.id = id.id & ~TOPO_CPU_MASK;
> +    s390_topology_set_entry(entry, id);
> +
> +    return entry;
> +}
> +
> +/**
> + * s390_topology_insert:
> + *
> + * @id: s390_topology_id to insert.
> + *
> + * Parse the topology list to find if the entry already
> + * exist and add the core in it.
> + * If it does not exist, allocate a new entry and insert
> + * it in the queue from lower id to greater id.
> + */
> +static void s390_topology_insert(s390_topology_id id)
> +{
> +    S390TopologyEntry *entry;
> +    S390TopologyEntry *tmp = NULL;
> +    uint64_t new_id;
> +
> +    new_id = id.id & ~TOPO_CPU_MASK;
> +
> +    /* First CPU to add to an entry */
> +    if (QTAILQ_EMPTY(&s390_topology.list)) {
> +        entry = s390_topology_new_entry(id);
> +        QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next);
> +        return;
> +    }
> +
> +    QTAILQ_FOREACH(tmp, &s390_topology.list, next) {
> +        if (new_id == tmp->id.id) {
> +            s390_topology_set_entry(tmp, id);
> +            return;
> +        } else if (new_id < tmp->id.id) {
> +            entry = s390_topology_new_entry(id);
> +            QTAILQ_INSERT_BEFORE(tmp, entry, next);
> +            return;
> +        }
> +    }
> +
> +    entry = s390_topology_new_entry(id);
> +    QTAILQ_INSERT_TAIL(&s390_topology.list, entry, next);

Consider adding a sentinel entry "at infinity", then that whole code
would simplify.

> +}
> +
> +/**
> + * s390_topology_check:
> + * @errp: Error pointer
> + * id: s390_topology_id to be verified
> + *
> + * The function checks if the topology id fits inside the
> + * system topology.
> + */
> +static void s390_topology_check(Error **errp, s390_topology_id id)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +
> +    if (id.socket > smp->sockets) {
> +            error_setg(errp, "Unavailable socket: %d", id.socket);
> +            return;
> +    }
> +    if (id.book > smp->books) {
> +            error_setg(errp, "Unavailable book: %d", id.book);
> +            return;
> +    }
> +    if (id.drawer > smp->drawers) {
> +            error_setg(errp, "Unavailable drawer: %d", id.drawer);
> +            return;
> +    }
> +    if (id.type != S390_TOPOLOGY_CPU_IFL) {
> +            error_setg(errp, "Unknown cpu type: %d", id.type);
> +            return;
> +    }
> +    /* Polarity and dedication can never be wrong */
> +}
> +
> +/**
> + * s390_topology_cpu_default:
> + * @errp: Error pointer
> + * @cpu: pointer to a S390CPU
> + *
> + * Setup the default topology for unset attributes.
> + *
> + * The function accept only all all default values or all set values
> + * for the geometry topology.
> + *
> + * 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).
> + *
> + */
> +static void s390_topology_cpu_default(Error **errp, S390CPU *cpu)
> +{
> +    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 = (env->core_id / smp->cores) % smp->sockets;
> +        env->book_id = (env->core_id / (smp->sockets * smp->cores)) %
> +                       smp->books;
> +        env->drawer_id = (env->core_id /
> +                          (smp->books * smp->sockets * smp->cores)) %
> +                         smp->drawers;
> +    }
> +}
> +
> +/**
> + * s390_topology_set_cpu:
> + * @ms: MachineState used to initialize the topology structure on
> + *      first call.
> + * @cpu: the new S390CPU to insert in the topology structure
> + * @errp: the error pointer
> + *
> + * Called from CPU Hotplug to check and setup the CPU attributes
> + * before to insert the CPU in the topology.
> + */
> +void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp)
> +{
> +    Error *local_error = NULL;

Can't you just use ERRP_GUARD ?

> +    s390_topology_id id;
> +
> +    /*
> +     * We do not want to initialize the topology if the cpu model
> +     * does not support topology consequently, we have to wait for

", consequently," I think. Could you do the initialization some where else,
after you know what the cpu model is? Not that I object to doing it this way.

> +     * the first CPU to be realized, which realizes the CPU model
> +     * to initialize the topology structures.
> +     *
> +     * s390_topology_set_cpu() is called from the cpu hotplug.
> +     */
> +    if (!s390_topology.sockets) {
> +        s390_topology_init(ms);
> +    }
> +
> +    s390_topology_cpu_default(&local_error, cpu);
> +    if (local_error) {
> +        error_propagate(errp, local_error);
> +        return;
> +    }
> +
> +    id = s390_topology_from_cpu(cpu);
> +
> +    /* Check for space on the socket */
> +    if (s390_topology.sockets[s390_socket_nb(id)] >=
> +        s390_topology.smp->sockets) {
> +        error_setg(&local_error, "No more space on socket");
> +        return;
> +    }
> +
> +    s390_topology_check(&local_error, id);
> +    if (local_error) {
> +        error_propagate(errp, local_error);
> +        return;
> +    }
> +
> +    s390_topology_insert(id);
> +}
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f3cc845d3b..c98b93a15f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -44,6 +44,7 @@
>  #include "hw/s390x/pv.h"
>  #include "migration/blocker.h"
>  #include "qapi/visitor.h"
> +#include "hw/s390x/cpu-topology.h"
>  
>  static Error *pv_mig_blocker;
>  
> @@ -310,10 +311,19 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>  {
>      MachineState *ms = MACHINE(hotplug_dev);
>      S390CPU *cpu = S390_CPU(dev);
> +    Error *local_err = NULL;
>  
>      g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
>      ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
>  
> +    if (s390_has_topology()) {
> +        s390_topology_set_cpu(ms, cpu, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
>      if (dev->hotplugged) {
>          raise_irq_cpu_hotplug();
>      }
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..58dfbdff4f 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -24,6 +24,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>    's390-stattrib-kvm.c',
>    'pv.c',
>    's390-pci-kvm.c',
> +  'cpu-topology.c',
>  ))
>  s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>    'tod-tcg.c',