Re: [PATCH v11 03/11] s390x/cpu topology: core_id sets s390x CPU topology

[Cédric Le Goater]

Hello Pierre,

On 11/3/22 18:01, 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>
> ---
>   include/hw/s390x/cpu-topology.h    |  41 ++++++++++
>   include/hw/s390x/s390-virtio-ccw.h |   1 +
>   target/s390x/cpu.h                 |   2 +
>   hw/s390x/cpu-topology.c            | 125 +++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c         |  23 ++++++
>   hw/s390x/meson.build               |   1 +
>   6 files changed, 193 insertions(+)
>   create mode 100644 include/hw/s390x/cpu-topology.h
>   create mode 100644 hw/s390x/cpu-topology.c
>
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> new file mode 100644
> index 0000000000..4e16a2153d
> --- /dev/null
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -0,0 +1,41 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * 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.
> + */
> +#ifndef HW_S390X_CPU_TOPOLOGY_H
> +#define HW_S390X_CPU_TOPOLOGY_H
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +#define S390_TOPOLOGY_CPU_IFL 0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> +
> +typedef struct S390TopoSocket {
> +    int active_count;
> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoSocket;
> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    uint32_t nr_cpus;
> +    uint32_t nr_sockets;
> +    S390TopoSocket *socket;
> +};
> +
> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
> +
> +void s390_topology_new_cpu(S390CPU *cpu);
> +
> +static inline bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +#endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> b/include/hw/s390x/s390-virtio-ccw.h
> index 4f8a39abda..23b472708d 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -29,6 +29,7 @@ struct S390CcwMachineState {
>       bool pv;
>       bool zpcii_disable;
>       uint8_t loadparm[8];
> +    void *topology;

I think it is safe to use 'DeviceState *' or 'Object *' for the pointer
under machine. What is most practical.

>   };
>   
>   struct S390CcwMachineClass {
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..c9066b2496 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -175,6 +175,8 @@ struct ArchCPU {
>       /* needed for live migration */
>       void *irqstate;
>       uint32_t irqstate_saved_size;
> +    /* Topology this CPU belongs too */
> +    void *topology;

However, under the CPU, we don't know what the future changes reserve
for us and I would call the attribute 'opaque' or 'machine_data'.

For now, it only holds a reference to the S390Topology device model
but it could become a struct with time.
 

>   };
>   
>   
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..6af41d3d7b
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,125 @@
> +/*
> + * 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"
> +
> +/*
> + * s390_topology_new_cpu:
> + * @cpu: a pointer to the new CPU
> + *
> + * The topology pointed by S390CPU, gives us the CPU topology
> + * established by the -smp QEMU aruments.
> + * The core-id is used to calculate the position of the CPU inside
> + * the topology:
> + *  - the socket, container TLE, containing the CPU, we have one socket
> + *    for every nr_cpus (nr_cpus = smp.cores * smp.threads)
> + *  - the CPU TLE inside the socket, we have potentionly up to 4 CPU TLE
> + *    in a container TLE with the assumption that all CPU are identical
> + *    with the same polarity and entitlement because we have maximum 256
> + *    CPUs and each TLE can hold up to 64 identical CPUs.
> + *  - the bit in the 64 bit CPU TLE core mask
> + */
> +void s390_topology_new_cpu(S390CPU *cpu)
> +{
> +    S390Topology *topo = (S390Topology *)cpu->topology;

where is cpu->topology set ?

> +    int core_id = cpu->env.core_id;
> +    int bit, origin;
> +    int socket_id;
> +
> +    socket_id = core_id / topo->nr_cpus;
> +
> +    /*
> +     * At the core level, each CPU is represented by a bit in a 64bit
> +     * uint64_t which represent the presence of a CPU.
> +     * The firmware assume that all CPU in a CPU TLE have the same
> +     * type, polarization and are all dedicated or shared.
> +     * In that case the origin variable represents the offset of the first
> +     * CPU in the CPU container.
> +     * More than 64 CPUs per socket are represented in several CPU containers
> +     * inside the socket container.
> +     * The only reason to have several S390TopologyCores inside a socket is
> +     * to have more than 64 CPUs.
> +     * In that case the origin variable represents the offset of the first CPU
> +     * in the CPU container. More than 64 CPUs per socket are represented in
> +     * several CPU containers inside the socket container.
> +     */
> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;
> +
> +    topo->socket[socket_id].active_count++;
> +    set_bit(bit, &topo->socket[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());

hmm,

> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> +
> +    topo->nr_cpus = ms->smp.cores * ms->smp.threads;
> +    topo->nr_sockets = ms->smp.sockets;

These properties should be set in s390_init_topology() with :

  object_property_set_int(OBJECT(dev), "num-cpus",
                          ms->smp.cores * ms->smp.threads, errp);

  object_property_set_int(OBJECT(dev), "num-sockets",
                          ms->smp.sockets, errp);

before calling sysbus_realize_and_unref()


> +    topo->socket = g_new0(S390TopoSocket, topo->nr_sockets);

For consistency, you could add an unrealize handler to free the array.

> +}
> +
> +static Property s390_topology_properties[] = {
> +    DEFINE_PROP_UINT32("nr_cpus", S390Topology, nr_cpus, 1),
> +    DEFINE_PROP_UINT32("nr_sockets", S390Topology, nr_sockets, 1),

A quick audit of the property names in QEMU code shows that a "num-" prefix
is usually preferred.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +/**
> + * 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;
> +    device_class_set_props(dc, s390_topology_properties);
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 9ab91df5b1..5776d3e58f 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;
>   
> @@ -102,6 +103,19 @@ static void s390_init_cpus(MachineState *machine)
>       }
>   }
>   
> +static void s390_init_topology(MachineState *machine)
> +{
> +    DeviceState *dev;
> +
> +    if (s390_has_topology()) {

I would move the s390_has_topology() check in the caller.

> +        dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +        object_property_add_child(&machine->parent_obj,
> +                                  TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +        S390_CCW_MACHINE(machine)->topology = dev;

and I would move the assignment in the caller also.

> +    }
> +}
> +
>   static const char *const reset_dev_types[] = {
>       TYPE_VIRTUAL_CSS_BRIDGE,
>       "s390-sclp-event-facility",
> @@ -252,6 +266,9 @@ 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 initialization */
> +    s390_init_topology(machine);
> +
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> @@ -314,6 +331,12 @@ 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 */
> +    if (S390_CCW_MACHINE(ms)->topology) {
> +        cpu->topology = S390_CCW_MACHINE(ms)->topology;

Two QOM cast. One should be enough. Please introduce a local variable.

> +        s390_topology_new_cpu(cpu);

I would pass the 'topology' object as a parameter of s390_topology_new_cpu()
and do the cpu->topology assignment in the same routine.

May be rename it also to :

  void s390_topology_add_cpu(S390Topology *topo, S390CPU *cpu)


Thanks,

C.
 
> +    }
> +
>       if (dev->hotplugged) {
>           raise_irq_cpu_hotplug();
>       }
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..653f6ab488 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',