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

[Pierre Morel]

On 9/5/22 20:11, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +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.
> >
> > 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
> [...]
>
> > +/**
> > + * 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);
> > +    int n;
> > +
> > +    topo->sockets = ms->smp.sockets;
> > +    topo->cores = ms->smp.cores;
> > +    topo->tles = ms->smp.max_cpus;
> > +
> > +    n = topo->sockets;
> > +    topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> > +    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
>
> Seems like a good use case for g_new0.

Yes


> [...]
> > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> > new file mode 100644
> > index 0000000000..6911f975f4
> > --- /dev/null
> > +++ b/include/hw/s390x/cpu-topology.h
> > @@ -0,0 +1,42 @@
> > +/*
> > + * 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
>
> Is there a reason this is before the includes?
> > +
> > +typedef struct S390TopoContainer {
> > +    int active_count;
> > +} S390TopoContainer;
> > +
> > +#define S390_TOPOLOGY_MAX_ORIGIN (1 + S390_MAX_CPUS / 64)
>
> This is correct because cpu_id == core_id for s390, right?
> So the cpu limit also applies to the core id.
> You could do ((S390_MAX_CPUS + 63) / 64) instead.
> But if you chose this for simplicity's sake, I'm fine with it.

hum, I think your proposition is right and mine is false.
If S390_MAX_CPUS is 64 we have only 1 origin.
I count 2 but you count 1.

So I change this.

> > +typedef struct S390TopoTLE {
> > +    int active_count;
>
> Do you use (read) this field somewhere?
> Is this in anticipation of there being multiple TLE arrays, for
> different polarizations, etc? If so I would defer this for later.

OK

> > +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
> > +} S390TopoTLE;
> > +
> > +#include "hw/qdev-core.h"
> > +#include "qom/object.h"
> > +
> > +struct S390Topology {
> > +    SysBusDevice parent_obj;
> > +    int sockets;
> > +    int cores;
>
> These are just cached values from machine_state.smp, right?
> Not sure if I like the redundancy, it doesn't aid in comprehension.
>
> > +    int tles;
> > +    S390TopoContainer *socket;
> > +    S390TopoTLE *tle;
> > +};
> > +typedef struct S390Topology S390Topology;
>
> The DECLARE macro takes care of this typedef.

right

> > +
> > +#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);
> > +
> > +#endif

--
Pierre Morel
IBM Lab Boeblingen