Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel <pmorel@linux.ibm.com>]

On 10/18/22 18:43, Cédric Le Goater wrote:
> Hello Pierre,
> 
> On 10/12/22 18:20, 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.
>> s390x/cpu topology: core_id sets s390x CPU topology
>>
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the cpu withing the topology.
>>
>> Let's build the topology based on the core_id.
> 
> The commit log is doubled.

Yes, thanks.

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>   hw/s390x/meson.build            |   1 +
>>   4 files changed, 199 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..66c171d0bc
>> --- /dev/null
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -0,0 +1,45 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright 2022 IBM Corp.
>> + *
>> + * 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"
>> +
>> +typedef struct S390TopoContainer {
>> +    int active_count;
>> +} S390TopoContainer;
> 
> This structure does not seem very useful.
> 
>> +
>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +typedef struct S390TopoTLE { 
> 
> The 'Topo' is redundant as TLE stands for 'topology-list entry'. This is 
> minor.
> 
>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> +} S390TopoTLE;
>> +
>> +struct S390Topology {
>> +    SysBusDevice parent_obj;
>> +    int cpus;
>> +    S390TopoContainer *socket;
>> +    S390TopoTLE *tle;
>> +    MachineState *ms;
> 
> hmm, it would be cleaner to introduce the fields and properties needed
> by the S390Topology model and avoid dragging the machine object pointer.
> AFAICT, these properties would be :
> 
>    "nr-cpus"
>    "max-cpus"
>    "nr-sockets"
> 

OK

> 
> 
>> +};
>> +
>> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
>> +
>> +S390Topology *s390_get_topology(void);
>> +void s390_topology_new_cpu(int core_id);
>> +
>> +static inline bool s390_has_topology(void)
>> +{
>> +    return false;
>> +}
>> +
>> +#endif
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..42b22a1831
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
> 
> The Copyright tag is different in the .h file.

OK, I change this to be like in the header file it seems to be the most 
used format.

> 
>> + * 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;
> 
> I am not convinced this routine is useful. The s390Topology pointer
> could be stored under the machine state I think. It wouldn't be a
> problem when CPUs are hot plugged since we have access to the machine
> in the hot plug handler.

OK, I add a pointer to the machine state that will be initialised during 
s390_init_topology()

> 
> For the stsi call, 'struct ArchCPU' probably lacks a back pointer to
> the machine objects with which CPU interact. These are typically
> interrupt controllers or this new s390Topology model. You could add
> the pointer there or, better, under a generic 'void *opaque' attribute.
> 
> That said, what you did works fine. The modeling could be cleaner.

Yes. I think you are right and I add a opaque pointer to the topology.

> 
>> +}
>> +
>> +/*
>> + * 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;
>> +    }
> 
> I would move this test in the caller.

Check will disapear with the new implementation.

> 
>> +
>> +    socket_id = core_id / topo->cpus;
>> +
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * unsigned long 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->tle[socket_id].mask[origin]);
> 
> here, the tle array is indexed with a socket id and ...

It was stupid to keep both structures.
I will keep only the socket structure and incorparate the TLE inside.

> 
>> +}
>> +
>> +/**
>> + * 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());
>> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>> +
>> +    topo->cpus = ms->smp.cores * ms->smp.threads;> +
>> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
>> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> 
> 
> ... here, the tle array is allocated with max_cpus and this looks
> weird. I will dig the specs to try to understand.

ack it looks weird. I keep only the socket structure

> 
>> +
>> +    topo->ms = ms;
> 
> See comment above regarding the properties.
> 
>> +}
>> +
>> +/**
>> + * 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);
>> +}
>> +
>> +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 03855c7231..aa99a62e42 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;
>> @@ -94,6 +95,18 @@ static void s390_init_cpus(MachineState *machine)
>>       }
>>   }
>> +static void s390_init_topology(MachineState *machine)
>> +{
>> +    DeviceState *dev;
>> +
>> +    if (s390_has_topology()) {
>> +        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);
>> +    }
>> +}
>> +
>>   static const char *const reset_dev_types[] = {
>>       TYPE_VIRTUAL_CSS_BRIDGE,
>>       "s390-sclp-event-facility",
>> @@ -244,6 +257,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 intialization */
> 
> initialization

yes, thanks


Thanks,

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen