[v10,1/9] 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 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.

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

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.

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



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

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.

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

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

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

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

> +    s390_init_topology(machine);
> +
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> @@ -306,6 +322,11 @@ 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_has_topology()) {
> +        s390_topology_new_cpu(cpu->env.core_id);
> +    }
> +
>       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',

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

On Wed, 2022-10-19 at 17:39 +0200, Pierre Morel wrote:
> 
> On 10/18/22 18:43, Cédric Le Goater wrote:
[...]
> > 
> > > 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.
> 
No, this form, with the date at the end, is the correct one.
> 

[...]

On 10/19/22 17:39, Pierre Morel wrote:
> 
> 
> 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()

LGTM.

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

As an example, you could look at PPC where the PowerPCCPU CPU model is
shared between two differents machine, a baremetal one PowerNV and the
para-virtual one pSeries/sPAPR. Look for :

    pnv_cpu_state(PowerPCCPU *cpu)
    spapr_cpu_state(PowerPCCPU *cpu)

the machine CPU state is stored under an opaque cpu->machine_data which is
specific to each machine. It doesn't have to be as complex on s390 since
we only have one type of z-machine. An opaque is a good idea still.

Thanks,

C.

On Wed, 2022-10-12 at 18:20 +0200, 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.
> 
> 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;
> +
> +#define S390_TOPOLOGY_CPU_IFL 0x03
> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> +typedef struct S390TopoTLE {
> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> +} S390TopoTLE;

Since this actually represents multiple TLEs, you might want to change the
name of the struct to reflect this. S390TopoTLEList maybe?

> +
> +struct S390Topology {
> +    SysBusDevice parent_obj;
> +    int cpus;
> +    S390TopoContainer *socket;
> +    S390TopoTLE *tle;
> +    MachineState *ms;
> +};
> +
> +#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
> + * 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.

s/aruments/arguments/

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

Not sure if that comment helps if you don't already know how the topology list works.
> +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->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

s/firmware assume/architecture specifies/

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

This sentence is repeated further down.

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

In the last version you had:
+ /*
+ * 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.
+ * 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.
+ */

I would modify it thus (with better line wrapping):
+ /*
+ * At the core level, each CPU is represented by a bit in a 64bit
+ * unsigned long.
+ * The architecture specifies that all CPU in a CPU TLE have the same
+ * type, polarization and are all dedicated or shared.
+ * In the case that a socket contains CPUs 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.
+ * Therefore, the only reason to have several S390TopologyCores inside a socket is
+ * to support CPU id differences > 64.
+ * In that case, the origin field in a container represents the offset of the first CPU
+ * in that CPU container, thereby allowing representation of all CPUs via multiple containers.
+ */

> +    bit = core_id;
> +    origin = bit / 64;
> +    bit %= 64;
> +    bit = 63 - bit;

I'm not convinced that that is more readable than just
 origin = core_id / 64;
 bit = 63 - (core_id % 64);

but that is for you to decide.
> +
> +    topo->socket[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());
> +    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);

As Cédric pointed out, the number of TLE(List)s should be the same as the
sockets.
> +
> +    topo->ms = ms;
> +}
[...]

On Wed, 2022-10-19 at 17:39 +0200, Pierre Morel wrote:
> 
> On 10/18/22 18:43, Cédric Le Goater wrote:
> 
> > > 
[...]
> > > 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
> > > 
[...]
> > > 
> > > +/*
> > > + * 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.

I don't think it's stupid. Both are valid possibilities.
The first one treats sockets and books and drawers exactly the same, since
they are all just containers (once you introduce books and drawers).
The second treats sockets differently, because they're the leaf nodes of the
hierarchy in a certain sense (the leaf nodes of the "regular" hierarchy,
whereas the cpus are the real leaf nodes of the topology but special/not "regular").

I'd say the first is more natural from reading the PoP, but it might indeed be a bit
confusing when reading the code since there's a one to one correspondence between
sockets and TLE(List)s.
> 
> > 
> > > +}
> > > +
> > > +/**
> > > + * 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

[...]

On Wed, 2022-10-12 at 18:20 +0200, 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.
> 
> 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
> 
[...]

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

Currently threads are not supported, effectively increasing the number of cpus,
so this is currently correct. Once the machine version limits the threads to 1,
it is also correct. However, once we support multiple threads, this becomes incorrect.
I wonder if it's ok from a backward compatibility point of view to modify the smp values
by doing cores *= threads, threads = 1 for old machines.
Then you can just use the cores value and it is always correct.
In any case, if you keep it as is, I'd like to see a comment here saying that this
is correct only so long as we don't support threads.
> +
> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> +
> +    topo->ms = ms;
> +}
> +
[...]

On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:20 +0200, 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.
>>
>> 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
>>
> [...]
> 
>> +/**
>> + * 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;
> 
> Currently threads are not supported, effectively increasing the number of cpus,
> so this is currently correct. Once the machine version limits the threads to 1,
> it is also correct. However, once we support multiple threads, this becomes incorrect.
> I wonder if it's ok from a backward compatibility point of view to modify the smp values
> by doing cores *= threads, threads = 1 for old machines.

Right, this will become incorrect with thread support.
What about having a dedicated function:

	topo->cpus = s390_get_cpus(ms);

This function will use the S390CcwMachineClass->max_thread introduced 
later to report the correct number of CPUs.


> Then you can just use the cores value and it is always correct.
> In any case, if you keep it as is, I'd like to see a comment here saying that this
> is correct only so long as we don't support threads.
>> +
>> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
>> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
>> +
>> +    topo->ms = ms;
>> +}
>> +
> [...]

On 24/10/2022 21.25, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:20 +0200, 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.
>>
>> 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;
>> +
>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>> +typedef struct S390TopoTLE {
>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>> +} S390TopoTLE;
> 
> Since this actually represents multiple TLEs, you might want to change the
> name of the struct to reflect this. S390TopoTLEList maybe?

Didn't TLE mean "Topology List Entry"? (by the way, Pierre, please explain 
this three letter acronym somewhere in this header in a comment)...

So expanding the TLE, this would mean S390TopoTopologyListEntryList ? ... 
this is getting weird... Also, this is not a "list" in the sense of a linked 
list, as one might expect at a first glance, so this is all very confusing 
here. Could you please come up with some better naming?

  Thomas

On Thu, 2022-10-27 at 10:05 +0200, Thomas Huth wrote:
> On 24/10/2022 21.25, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:20 +0200, 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.
> > > 
> > > 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;
> > > +
> > > +#define S390_TOPOLOGY_CPU_IFL 0x03
> > > +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
> > > +typedef struct S390TopoTLE {
> > > +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> > > +} S390TopoTLE;
> > 
> > Since this actually represents multiple TLEs, you might want to change the
> > name of the struct to reflect this. S390TopoTLEList maybe?
> 
> Didn't TLE mean "Topology List Entry"? (by the way, Pierre, please explain 

Yes.

> this three letter acronym somewhere in this header in a comment)...
> 
> So expanding the TLE, this would mean S390TopoTopologyListEntryList ? ... 
> this is getting weird...

:D indeed. So the leaves of the topology tree as stored by STSI are lists
of CPU-type TLEs which aren't empty i.e. represent some cpus.
Whereas this struct is used to track which CPU-type TLEs need to be created.
It doesn't represent a TLE and doesn't represent the list of CPU-type TLEs.
So yeah, you're right, not a good name.

Off the top of my head I'd suggest S390TopoCPUSet. It's a bitmap, which is
kind of a set. Maybe S390TopoSocketCPUSet to reflect that it is the set of
CPUs in a socket, although, if we ever support different polarizations, etc.
that wouldn't really be true anymore, since that creates additional levels,
so maybe not. (In that case the leaf list of CPU-types TLEs is a flattened tree.)

> Also, this is not a "list" in the sense of a linked 
> list, as one might expect at a first glance, so this is all very confusing 
> here. Could you please come up with some better naming?
> 
>   Thomas
> 
>

On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
> 
> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:20 +0200, 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.
> > > 
> > > 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
> > > 
> > [...]
> > 
> > > +/**
> > > + * 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;
> > 
> > Currently threads are not supported, effectively increasing the number of cpus,
> > so this is currently correct. Once the machine version limits the threads to 1,
> > it is also correct. However, once we support multiple threads, this becomes incorrect.
> > I wonder if it's ok from a backward compatibility point of view to modify the smp values
> > by doing cores *= threads, threads = 1 for old machines.
> 
> Right, this will become incorrect with thread support.
> What about having a dedicated function:
> 
> 	topo->cpus = s390_get_cpus(ms);
> 
> This function will use the S390CcwMachineClass->max_thread introduced 
> later to report the correct number of CPUs.

I don't think max_threads is exactly what matters here, it's if
threads are supported or not or, if max_threads == 1 it doesn't matter.
The question is how best to do the check. You could check the machine version.
I wonder if you could add a feature bit for the multithreading facility that is
always false and use that.

I don't know if using a function makes a difference, that is if it is obvious on
introduction of multithreading support that the function needs to be updated.
(If it is implemented in a way that requires updating, if you check the machine
version it doesn't)
In any case, the name you suggested isn't very descriptive.
> 
> 
> > Then you can just use the cores value and it is always correct.
> > In any case, if you keep it as is, I'd like to see a comment here saying that this
> > is correct only so long as we don't support threads.
> > > +
> > > +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
> > > +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
> > > +
> > > +    topo->ms = ms;
> > > +}
> > > +
> > [...]
>

On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
>>
>> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
>>> On Wed, 2022-10-12 at 18:20 +0200, 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.
>>>>
>>>> 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
>>>>
>>> [...]
>>>
>>>> +/**
>>>> + * 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;
>>>
>>> Currently threads are not supported, effectively increasing the number of cpus,
>>> so this is currently correct. Once the machine version limits the threads to 1,
>>> it is also correct. However, once we support multiple threads, this becomes incorrect.
>>> I wonder if it's ok from a backward compatibility point of view to modify the smp values
>>> by doing cores *= threads, threads = 1 for old machines.
>>
>> Right, this will become incorrect with thread support.
>> What about having a dedicated function:
>>
>> 	topo->cpus = s390_get_cpus(ms);
>>
>> This function will use the S390CcwMachineClass->max_thread introduced
>> later to report the correct number of CPUs.
> 
> I don't think max_threads is exactly what matters here, it's if
> threads are supported or not or, if max_threads == 1 it doesn't matter.
> The question is how best to do the check. You could check the machine version.
> I wonder if you could add a feature bit for the multithreading facility that is
> always false and use that.
> 
> I don't know if using a function makes a difference, that is if it is obvious on
> introduction of multithreading support that the function needs to be updated.
> (If it is implemented in a way that requires updating, if you check the machine
> version it doesn't)
> In any case, the name you suggested isn't very descriptive.

I think we care about this machine and olders.
Olders do not support topology so this, Multithreading (MT) does not mater.
This machine support topology, if I follow Cedric advise, the 
"max_thread" will/may be introduce before the topology.

This in fact is not an implementation for MT or does not allow the 
implementation of MT it is only a way to get rid of the false 
information given to the user that we accept MT.

So I think that when we introduce MT we will take care of making things 
right at this place as in other places of the code.

What about we keep the original:

     topo->cpus = ms->smp.cores * ms->smp.threads;

Which does not do any arm to machines without MT ?

Regards,
Pierre

On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
> 
> On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
> > > 
> > > On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> > > > On Wed, 2022-10-12 at 18:20 +0200, 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.
> > > > > 
> > > > > 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
> > > > > 
> > > > [...]
> > > > 
> > > > > +/**
> > > > > + * 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;
> > > > 
> > > > Currently threads are not supported, effectively increasing the number of cpus,
> > > > so this is currently correct. Once the machine version limits the threads to 1,
> > > > it is also correct. However, once we support multiple threads, this becomes incorrect.
> > > > I wonder if it's ok from a backward compatibility point of view to modify the smp values
> > > > by doing cores *= threads, threads = 1 for old machines.
> > > 
> > > Right, this will become incorrect with thread support.
> > > What about having a dedicated function:
> > > 
> > > 	topo->cpus = s390_get_cpus(ms);
> > > 
> > > This function will use the S390CcwMachineClass->max_thread introduced
> > > later to report the correct number of CPUs.
> > 
> > I don't think max_threads is exactly what matters here, it's if
> > threads are supported or not or, if max_threads == 1 it doesn't matter.
> > The question is how best to do the check. You could check the machine version.
> > I wonder if you could add a feature bit for the multithreading facility that is
> > always false and use that.
> > 
> > I don't know if using a function makes a difference, that is if it is obvious on
> > introduction of multithreading support that the function needs to be updated.
> > (If it is implemented in a way that requires updating, if you check the machine
> > version it doesn't)
> > In any case, the name you suggested isn't very descriptive.
> 
> I think we care about this machine and olders.
> Olders do not support topology so this, Multithreading (MT) does not mater.
> This machine support topology, if I follow Cedric advise, the 
> "max_thread" will/may be introduce before the topology.
> 
> This in fact is not an implementation for MT or does not allow the 
> implementation of MT it is only a way to get rid of the false 
> information given to the user that we accept MT.
> 
> So I think that when we introduce MT we will take care of making things 
> right at this place as in other places of the code.
> 
> What about we keep the original:
> 
>      topo->cpus = ms->smp.cores * ms->smp.threads;

If topology is only supported for new machines and not the old machines
for which you set max_threads to a compatibility value (max cpus), then
you should just ignore the threads, cpus == cores.
(There might not be any point in keeping a topo->cpus member in this case, I haven't checked)
> 
> Which does not do any arm to machines without MT ?
> 
> Regards,
> Pierre
>

On 11/7/22 19:04, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-10-28 at 11:30 +0200, Pierre Morel wrote:
>>
>> On 10/27/22 22:20, Janis Schoetterl-Glausch wrote:
>>> On Wed, 2022-10-26 at 10:34 +0200, Pierre Morel wrote:
>>>>
>>>> On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
>>>>> On Wed, 2022-10-12 at 18:20 +0200, 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.
>>>>>>
>>>>>> 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
>>>>>>
>>>>> [...]
>>>>>
>>>>>> +/**
>>>>>> + * 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;
>>>>>
>>>>> Currently threads are not supported, effectively increasing the number of cpus,
>>>>> so this is currently correct. Once the machine version limits the threads to 1,
>>>>> it is also correct. However, once we support multiple threads, this becomes incorrect.
>>>>> I wonder if it's ok from a backward compatibility point of view to modify the smp values
>>>>> by doing cores *= threads, threads = 1 for old machines.
>>>>
>>>> Right, this will become incorrect with thread support.
>>>> What about having a dedicated function:
>>>>
>>>> 	topo->cpus = s390_get_cpus(ms);
>>>>
>>>> This function will use the S390CcwMachineClass->max_thread introduced
>>>> later to report the correct number of CPUs.
>>>
>>> I don't think max_threads is exactly what matters here, it's if
>>> threads are supported or not or, if max_threads == 1 it doesn't matter.
>>> The question is how best to do the check. You could check the machine version.
>>> I wonder if you could add a feature bit for the multithreading facility that is
>>> always false and use that.
>>>
>>> I don't know if using a function makes a difference, that is if it is obvious on
>>> introduction of multithreading support that the function needs to be updated.
>>> (If it is implemented in a way that requires updating, if you check the machine
>>> version it doesn't)
>>> In any case, the name you suggested isn't very descriptive.
>>
>> I think we care about this machine and olders.
>> Olders do not support topology so this, Multithreading (MT) does not mater.
>> This machine support topology, if I follow Cedric advise, the
>> "max_thread" will/may be introduce before the topology.
>>
>> This in fact is not an implementation for MT or does not allow the
>> implementation of MT it is only a way to get rid of the false
>> information given to the user that we accept MT.
>>
>> So I think that when we introduce MT we will take care of making things
>> right at this place as in other places of the code.
>>
>> What about we keep the original:
>>
>>       topo->cpus = ms->smp.cores * ms->smp.threads;
> 
> If topology is only supported for new machines and not the old machines
> for which you set max_threads to a compatibility value (max cpus), then
> you should just ignore the threads, cpus == cores.
> (There might not be any point in keeping a topo->cpus member in this case, I haven't checked)

Right but, I need the nr_cpus in the topology so I prefer to keep it.

However, smp.threads has nothing to do there anymore as you pointed.
I think that nr_cpus should may be named nr_cores and should be set to 
smp.cores.

Thanks,
Regards,

Pierre