Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 9/27/22 14:03, Cédric Le Goater wrote:
> On 9/2/22 09:55, Pierre Morel wrote:
> > In the S390x CPU topology the core_id specifies the CPU address
> > and the position of the core withing the topology.
> >
> > Let's build the topology based on the core_id.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   hw/s390x/cpu-topology.c         | 135 ++++++++++++++++++++++++++++++++
> >   hw/s390x/meson.build            |   1 +
> >   hw/s390x/s390-virtio-ccw.c      |  10 +++
> >   include/hw/s390x/cpu-topology.h |  42 ++++++++++
> >   4 files changed, 188 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..a6ca006ec5
> > --- /dev/null
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -0,0 +1,135 @@
> > +/*
> > + * 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/sysbus.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"
> > +
> > +S390Topology *s390_get_topology(void)
> > +{
> > +    static S390Topology *s390Topology;
> > +
> > +    if (!s390Topology) {
> > +        s390Topology = S390_CPU_TOPOLOGY(
> > +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
> > +    }
> > +
> > +    return s390Topology;
> > +}
> > +
> > +/*
> > + * s390_topology_new_cpu:
> > + * @core_id: the core ID is machine wide
> > + *
> > + * The topology returned by s390_get_topology(), gives us the CPU
> > + * topology established by the -smp QEMU aruments.
> > + * The core-id gives:
> > + *  - the Container TLE (Topology List Entry) containing the CPU TLE.
> > + *  - in the CPU TLE the origin, or offset of the first bit in the 
> > core mask
> > + *  - the bit in the CPU TLE core mask
> > + */
> > +void s390_topology_new_cpu(int core_id)
> > +{
> > +    S390Topology *topo = s390_get_topology();
> > +    int socket_id;
> > +    int bit, origin;
> > +
> > +    /* In the case no Topology is used nothing is to be done here */
> > +    if (!topo) {
> > +        return;
> > +    }
> > +
> > +    socket_id = core_id / topo->cores;
> > +
> > +    bit = core_id;
> > +    origin = bit / 64;
> > +    bit %= 64;
> > +    bit = 63 - bit;
> > +
> > +    /*
> > +     * At the core level, each CPU is represented by a bit in a 64bit
> > +     * unsigned long. Set on plug and clear on unplug of a CPU.
>
> Do we have CPU unplug on s390x ?

not in QEMU.

I will change this sentence.

> > +     * The firmware assume that all CPU in a CPU TLE have the same
> > +     * type, polarization and are all dedicated or shared.
> > +     * In the case a socket contains CPU with different type, 
> > polarization
> > +     * or entitlement then they will be defined in different CPU 
> > containers.
> > +     * Currently we assume all CPU are identical IFL CPUs and that 
> > they are
> > +     * all dedicated CPUs.
> > +     * The only reason to have several S390TopologyCores inside a 
> > socket is
> > +     * to have more than 64 CPUs.
> > +     * In that case the origin field, representing the offset of the 
> > first CPU
> > +     * in the CPU container allows to represent up to the maximal 
> > number of
> > +     * CPU inside several CPU containers inside the socket container.
> > +     */
> > +    topo->socket[socket_id].active_count++;
> > +    topo->tle[socket_id].active_count++;
> > +    set_bit(bit, &topo->tle[socket_id].mask[origin]);
> > +}
> > +
> > +/**
> > + * s390_topology_realize:
> > + * @dev: the device state
> > + * @errp: the error pointer (not used)
> > + *
> > + * During realize the machine CPU topology is initialized with the
> > + * QEMU -smp parameters.
> > + * The maximum count of CPU TLE in the all Topology can not be greater
> > + * than the maximum CPUs.
> > + */
> > +static void s390_topology_realize(DeviceState *dev, Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
>
> Using qdev_get_machine() is suspicious :)

OK

> > +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> > +    int n;
> > +
> > +    topo->sockets = ms->smp.sockets;
> > +    topo->cores = ms->smp.cores;
> > +    topo->tles = ms->smp.max_cpus;
>
> These look like object properties to me.

It is a temporary store to keep them at hand.
I will see if I keep them afterall.
If I keep them I will make them property.

> > +
> > +    n = topo->sockets;
> > +    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> > +    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
> > +}
> > +
> > +/**
> > + * topology_class_init:
> > + * @oc: Object class
> > + * @data: (not used)
> > + *
> > + * A very simple object we will need for reset and migration.
> > + */
> > +static void topology_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    dc->realize = s390_topology_realize;
> > +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > +}
>
> no vmstate ?

it is added in a later patch

> > +static const TypeInfo cpu_topology_info = {
> > +    .name          = TYPE_S390_CPU_TOPOLOGY,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(S390Topology),
> > +    .class_init    = topology_class_init,
> > +};
> > +
> > +static void topology_register(void)
> > +{
> > +    type_register_static(&cpu_topology_info);
> > +}
> > +type_init(topology_register);
> > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> > index de28a90a57..96d7d7d231 100644
> > --- a/hw/s390x/meson.build
> > +++ b/hw/s390x/meson.build
> > @@ -2,6 +2,7 @@ s390x_ss = ss.source_set()
> >   s390x_ss.add(files(
> >     'ap-bridge.c',
> >     'ap-device.c',
> > +  'cpu-topology.c',
> >     'ccw-device.c',
> >     'css-bridge.c',
> >     'css.c',
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index b5ca154e2f..15cefd104b 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -43,6 +43,7 @@
> >   #include "sysemu/sysemu.h"
> >   #include "hw/s390x/pv.h"
> >   #include "migration/blocker.h"
> > +#include "hw/s390x/cpu-topology.h"
> >   static Error *pv_mig_blocker;
> > @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
> >       /* init memory + setup max page size. Required for the CPU model */
> >       s390_memory_init(machine->ram);
> > +    /* Adding the topology must be done before CPU intialization*/
> > +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> > +    object_property_add_child(qdev_get_machine(), 
> > TYPE_S390_CPU_TOPOLOGY,
>
> No need to use qdev_get_machine(), you have 'machine' above.

OK

> > +                              OBJECT(dev));
> > +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>
>
> why not store a TYPE_S390_CPU_TOPOLOGY object pointer under the machine
> state for later use ?

May be, I will think about it.

> > +
> >       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
> >       s390_init_cpus(machine);
> > @@ -309,6 +316,9 @@ static void s390_cpu_plug(HotplugHandler 
> > *hotplug_dev,
> >       g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
> >       ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
> > +    /* Inserting the CPU in the Topology can not fail */
> > +    s390_topology_new_cpu(cpu->env.core_id);
> > +
>
> in which case, we could use the topo object pointer to insert a new CPU
> id and drop s390_get_topology() which looks overkill.
>
> I would add the test :
>
>     if (!S390_CCW_MACHINE(machine)->topology_disable) {
>
> before inserting to be consistent. But I am anticipating some other
> patch.

It belongs here for bisect so I will add it here.
Thanks.

> C.

Thanks,
Pierre

--
Pierre Morel
IBM Lab Boeblingen