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

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
orders the list in topological 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

On 05/01/2023 15.53, 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
> orders the list in topological 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.

s/adventage/advantage/

> 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;
> +        uint64_t core:6;    /* byte 7 BE */
> +    };
> +} s390_topology_id;

Bitmasks are OK for code that will definitely only ever work with KVM ... 
but this will certainly fail completely if we ever try to get it running 
with TCG later. Do we care? ... if so, you should certainly avoid a bitfield 
here. Especially since most of the fields are 8-bit anyway and could easily 
be represented by a "uint8_t" variable. Otherwise, just ignore my comment.

> +#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;

So this "uint8_t" basically is a hidden limit of a maximum of 256 sockets 
that can be used for per book? Do we check that limit somewhere? (I looked 
for it, but I didn't spot such a check)

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

Want to update to 2023 now?

> + * 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 */
> +};
> +
> +/**
> + * 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); > +}
I think there might be an off-by-one error in here - you likely need a "- 1" 
at the very end.

For example, assume that we have one socket, one book and one drawer, so 
id.socket, id.book and id.drawer would all be 0. The function then returns 1 ...

> +static void s390_topology_init(MachineState *ms)
> +{
> +    CpuTopology *smp = &ms->smp;
> +
> +    s390_topology.smp = smp;
> +    if (!s390_topology.sockets) {
> +        s390_topology.sockets = g_new0(uint8_t, smp->sockets *
> +                                       smp->books * smp->drawers);

... but here you only allocated one byte. So you later access 
s390_topology.sockets[s390_socket_nb(id)], i.e. s390_topology.sockets[1] 
which is out of bounds.

> +    }
> +}

  Thomas

On Tue, 2023-01-10 at 14:00 +0100, Thomas Huth wrote:
> On 05/01/2023 15.53, 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
> > orders the list in topological 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.
> 
> s/adventage/advantage/
> 
> > 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
> > 
[...]

> > +typedef struct S390Topology {
> > +    QTAILQ_HEAD(, S390TopologyEntry) list;
> > +    uint8_t *sockets;
> 
> So this "uint8_t" basically is a hidden limit of a maximum of 256 sockets 
> that can be used for per book? Do we check that limit somewhere? (I looked 
> for it, but I didn't spot such a check)

S390_MAX_CPUS < 256. Might be a good idea to have a build time assert for that.
And one cannot have more sockets that maxcpus.
> 
> > +    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
> 
> Want to update to 2023 now?

It's the year of first publication, and I'd guess this is a derivative work of what
was published to the mailing list last year.
>

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',

On 1/10/23 14:00, Thomas Huth wrote:
> On 05/01/2023 15.53, 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
>> orders the list in topological 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.
> 
> s/adventage/advantage/
> 
>> 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;
>> +        uint64_t core:6;    /* byte 7 BE */
>> +    };
>> +} s390_topology_id;
> 
> Bitmasks are OK for code that will definitely only ever work with KVM 
> ... but this will certainly fail completely if we ever try to get it 
> running with TCG later. Do we care? ... if so, you should certainly 
> avoid a bitfield here. Especially since most of the fields are 8-bit 
> anyway and could easily be represented by a "uint8_t" variable. 
> Otherwise, just ignore my comment.

The goal of using a bit mask here is not to use it with KVM but to have 
an easy way to order the TLE using the natural order of the placement of 
the fields in the uint64_t
However, if I remove the two unused levels 5 and 6 I can use uint8_t for 
all the entries.

I doubt we use the levels 5 and 6 in a short future.

So I switch on 1 uint8_t for each entry.

...

>> 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
> 
> Want to update to 2023 now?
> 
>> + * 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 */
>> +};
>> +
>> +/**
>> + * 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); > +}
> I think there might be an off-by-one error in here - you likely need a 
> "- 1" at the very end.
> 
> For example, assume that we have one socket, one book and one drawer, so 
> id.socket, id.book and id.drawer would all be 0. The function then 
> returns 1 ...

hum, I fear it is even more false than that but thanks for pointing this 
error.

  /o\

     return (id.drawer * s390_topology.smp.books + id.book) *
            s390_topology.smp.sockets + id.socket;


> 
>> +static void s390_topology_init(MachineState *ms)
>> +{
>> +    CpuTopology *smp = &ms->smp;
>> +
>> +    s390_topology.smp = smp;
>> +    if (!s390_topology.sockets) {
>> +        s390_topology.sockets = g_new0(uint8_t, smp->sockets *
>> +                                       smp->books * smp->drawers);
> 
> ... but here you only allocated one byte. So you later access 
> s390_topology.sockets[s390_socket_nb(id)], i.e. s390_topology.sockets[1] 
> which is out of bounds.

Yes, thanks.

Regards,
Pierre

On 1/13/23 19:15, Nina Schoetterl-Glausch wrote:
> 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/

thx

>> orders the list in topological order.
> s/orders/order/

thx

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

right, must be set so does the core offset in the mask.

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

Yes, I can remove the core:6.
After Thomas comment I will change all the bit field for uint8_t.
I think we should not use the real topology entry here if we want to use 
TGE in the future.

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

OK

> 
>> +};
>> +
>> +/**
>> + * 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.

yes, already changed

> 
>> +}
>> +
>> +/**
>> + * 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?

I forgot to remove this test.
The caller already check this.

> 
>> +        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_add_cpu_to_entry() ?



> 
>> +                                    s390_topology_id id)
>> +{
>> +    set_bit(63 - id.core, &entry->mask);
> 
> You need to subtract the origin first or that might be negative.

yes, origin is not handled correctly

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

looks good, thanks.


> 
>> +}
>> +
>> +/**
>> + * 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 ?

I do not think it is necessary and I find it obfuscating.
So, should I?

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

I did not find a better place, it must be done after the CPU model is 
initialize and before the first CPU is created.
The cpu model is initialized during the early creation of the first cpu.

Any idea?

Thanks.

Regards,
Pierre

On Tue, 2023-01-17 at 14:55 +0100, Pierre Morel wrote:
> 
> On 1/13/23 19:15, Nina Schoetterl-Glausch wrote:
> > 
[...]

> > > +/**
> > > + * 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_add_cpu_to_entry() ?

Yeah, that's better.

[...]
> 
> > > +/**
> > > + * 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 ?
> 
> I do not think it is necessary and I find it obfuscating.
> So, should I?

/*
 * Propagate error object (if any) from @local_err to @dst_errp.
[...]
 * Please use ERRP_GUARD() instead when possible.
 * Please don't error_propagate(&error_fatal, ...), use
 * error_report_err() and exit(), because that's more obvious.
 */
void error_propagate(Error **dst_errp, Error *local_err);

So I'd say yes.
> 
> > 
> > > +    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.
> > 
> 
> I did not find a better place, it must be done after the CPU model is 
> initialize and before the first CPU is created.
> The cpu model is initialized during the early creation of the first cpu.
> 
> Any idea?
> 
> Thanks.
> 
> Regards,
> Pierre
>

On 1/17/23 17:48, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-01-17 at 14:55 +0100, Pierre Morel wrote:
>>
>> On 1/13/23 19:15, Nina Schoetterl-Glausch wrote:
>>>
> [...]
> 
>>>> +/**
>>>> + * 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_add_cpu_to_entry() ?
> 
> Yeah, that's better.
> 
> [...]
>>
>>>> +/**
>>>> + * 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 ?
>>
>> I do not think it is necessary and I find it obfuscating.
>> So, should I?
> 
> /*
>   * Propagate error object (if any) from @local_err to @dst_errp.
> [...]
>   * Please use ERRP_GUARD() instead when possible.
>   * Please don't error_propagate(&error_fatal, ...), use
>   * error_report_err() and exit(), because that's more obvious.
>   */
> void error_propagate(Error **dst_errp, Error *local_err);
> 
> So I'd say yes.

OK, you are right it is better.

Regards,
Pierre