Re: [PATCH v2 3/5] s390x: topology: CPU topology objects and structures

[Pierre Morel]

On 9/7/21 9:32 AM, Thomas Huth wrote:
> On 22/07/2021 19.42, Pierre Morel wrote:
> > We use new objects to have a dynamic administration of the CPU topology.
> > The highier level object is the S390 book. In a first implementation
>
> s/highier/higher/ ... or highest ?

thx I chose highest

And I modify to
"
The highest level object in this implementation is the s390 book and in 
this first implementation of CPU topology for S390 we have a single book.
"

> > we will have only a single S390 book.
> > The book is built as a SYSBUS bridge during the CPU initialisation. > 
> > Every object under this single book will be build dynamically
> > immediately after a CPU has be realized if it is needed.
> > The CPU will fill the sockets once after the other, according to the
> > number of core per socket defined during the smp parsing.
> >
> > Each CPU inside a socket will be represented by a bit in a 64bit
> > unsigned long. Set on plug and clear on unplug of a CPU.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   hw/s390x/cpu-topology.c         | 334 ++++++++++++++++++++++++++++++++
> >   hw/s390x/meson.build            |   1 +
> >   hw/s390x/s390-virtio-ccw.c      |   4 +
> >   include/hw/s390x/cpu-topology.h |  67 +++++++
> >   4 files changed, 406 insertions(+)
> >   create mode 100644 hw/s390x/cpu-topology.c
> >   create mode 100644 include/hw/s390x/cpu-topology.h
> >
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > new file mode 100644
> > index 0000000000..73f91d5334
> > --- /dev/null
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -0,0 +1,334 @@
> > +/*
> > + * CPU Topology
> > + *
> > + * Copyright 2021 IBM Corp.
> > + * 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/sysbus.h"
> > +#include "hw/s390x/cpu-topology.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"
> > +
> > +static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
> > +                                            int origin)
> > +{
> > +    DeviceState *dev;
> > +    S390TopologyCores *cores;
> > +    const MachineState *ms = MACHINE(qdev_get_machine());
> > +
> > +    if (socket->bus->num_children >= ms->smp.cores) {
> > +        return NULL;
> > +    }
> > +
> > +    dev = qdev_new(TYPE_S390_TOPOLOGY_CORES);
> > +    qdev_realize_and_unref(dev, socket->bus, &error_fatal);
> > +
> > +    cores = S390_TOPOLOGY_CORES(dev);
> > +    cores->origin = origin;
> > +    socket->cnt += 1;
> > +
> > +    return cores;
> > +}
> > +
> > +static S390TopologySocket *s390_create_socket(S390TopologyBook *book, 
> > int id)
> > +{
> > +    DeviceState *dev;
> > +    S390TopologySocket *socket;
> > +    const MachineState *ms = MACHINE(qdev_get_machine());
> > +
> > +    if (book->bus->num_children >= ms->smp.sockets) {
> > +        return NULL;
> > +    }
> > +
> > +    dev = qdev_new(TYPE_S390_TOPOLOGY_SOCKET);
> > +    qdev_realize_and_unref(dev, book->bus, &error_fatal);
> > +
> > +    socket = S390_TOPOLOGY_SOCKET(dev);
> > +    socket->socket_id = id;
> > +    book->cnt++;
> > +
> > +    return socket;
> > +}
> > +
>
> Could you add a short comment in front of s390_get_cores, describing 
> what it does? From the name, I would not expect that this function 
> creates something automatically, so a comment explaining this would be 
> nice.

Yes, I will do.


> > +static S390TopologyCores *s390_get_cores(S390TopologySocket *socket, 
> > int origin)
> > +{
> > +    S390TopologyCores *cores;
> > +    BusChild *kid;
> > +
> > +    QTAILQ_FOREACH(kid, &socket->bus->children, sibling) {
> > +        cores = S390_TOPOLOGY_CORES(kid->child);
> > +        if (cores->origin == origin) {
> > +            return cores;
> > +        }
> > +    }
> > +    return s390_create_cores(socket, origin);
> > +}
>
> dito.

too

> > +static S390TopologySocket *s390_get_socket(S390TopologyBook *book,
> > +                                           int socket_id)
> > +{
> > +    S390TopologySocket *socket;
> > +    BusChild *kid;
> > +
> > +    QTAILQ_FOREACH(kid, &book->bus->children, sibling) {
> > +        socket = S390_TOPOLOGY_SOCKET(kid->child);
> > +        if (socket->socket_id == socket_id) {
> > +            return socket;
> > +        }
> > +    }
> > +    return s390_create_socket(book, socket_id);
> > +}
> > +
> > +/*
> > + * s390_topology_new_cpu:
> > + * @core_id: the core ID is machine wide
> > + *
> > + * We have a single book returned by s390_get_topology(),
> > + * then we build the hierarchy on demand.
> > + * Note that we do not destroy the hierarchy on error creating
> > + * an entry in the topology, we just keept it empty.
>
> s/keept/keep/
yes.

> > + * We do not need to worry about not finding a topology level
> > + * entry this would have been catched during smp parsing.
>
> s/catched/caught/
hum, yes

> > + */
> > +void s390_topology_new_cpu(int core_id)
> > +{
> > +    const MachineState *ms = MACHINE(qdev_get_machine());
> > +    S390TopologyBook *book;
> > +    S390TopologySocket *socket;
> > +    S390TopologyCores *cores;
> > +    int cores_per_socket, sock_idx;
> > +    int origin, bit;
> > +
> > +    book = s390_get_topology();
> > +
> > +    cores_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> > +
> > +    sock_idx = (core_id / cores_per_socket);
> > +    socket = s390_get_socket(book, sock_idx);
> > +
> > +    /*
> > +     * We assert that all CPU are identical IFL, shared and
>
> assert? Or did you mean assume? When reading something like "assert" I'd 
> expect some error checking here...?

no the right word is "assume"

> > +     * horizontal topology, the only reason to have several
> > +     * S390TopologyCores is to have more than 64 CPUs.
> > +     */
> > +    origin = 64 * (core_id / 64);
> > +
> > +    cores = s390_get_cores(socket, origin);
> > +

...snip...

> > +static char *socket_bus_get_dev_path(DeviceState *dev)
> > +{
> > +    S390TopologySocket *socket = S390_TOPOLOGY_SOCKET(dev);
> > +    DeviceState *book = dev->parent_bus->parent;
> > +    char *id = qdev_get_dev_path(book);
> > +    char *ret;
> > +
> > +    if (id) {
> > +        ret = g_strdup_printf("%s:%02d", id, socket->socket_id);
> > +        g_free(id);
> > +    } else {
> > +        ret = g_malloc(6);
> > +        snprintf(ret, 6, "_:%02d", socket->socket_id);
>
> Why don't you use g_strdup_printf() here?

I wonder too...


> > +    }
> > +
> > +    return ret;
> > +}
...snip...
> > +    }
> > +};
>
> I didn't spot any migration related code in here ... is this already 
> migration-safe?

You are right, I must think about this,
I think we must keep the same topology, even false, during the migration 
and set the topology_change_report_pending when the migration is done.

I will add this in the next round.

Thanks a lot for the reviewing
Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen