Re: [PATCH v8 02/12] s390x/cpu_topology: CPU topology objects and structures

[Janosch Frank]

On 6/29/22 17:25, Pierre Morel wrote:
> On 6/27/22 15:31, Janosch Frank wrote:
> > On 6/20/22 16:03, Pierre Morel wrote:
> > > We use new objects to have a dynamic administration of the CPU topology.
> > > The highest level object in this implementation is the s390 book and
> > > in this first implementation of CPU topology for S390 we have a single
> > > book.
> > > The book is built as a SYSBUS bridge during the CPU initialization.
> > > Other objects, sockets and core will be built after the parsing
> > > of the QEMU -smp argument.
> > >
> > > Every object under this single book will be build dynamically
> > > immediately after a CPU has be realized if it is needed.
> > > The CPU will fill the sockets once after the other, according to the
> > > number of core per socket defined during the smp parsing.
> > >
> > > Each CPU inside a socket will be represented by a bit in a 64bit
> > > unsigned long. Set on plug and clear on unplug of a CPU.
> > >
> > > For the S390 CPU topology, thread and cores are merged into
> > > topology cores and the number of topology cores is the multiplication
> > > of cores by the numbers of threads.
> > >
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >
> > [...]
> >
> > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > index 7d6d01325b..216adfde26 100644
> > > --- a/target/s390x/cpu.h
> > > +++ b/target/s390x/cpu.h
> > > @@ -565,6 +565,53 @@ typedef union SysIB {
> > >    } SysIB;
> > >    QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
> > > +/* CPU type Topology List Entry */
> > > +typedef struct SysIBTl_cpu {
> > > +        uint8_t nl;
> > > +        uint8_t reserved0[3];
> > > +        uint8_t reserved1:5;
> > > +        uint8_t dedicated:1;
> > > +        uint8_t polarity:2;
> > > +        uint8_t type;
> > > +        uint16_t origin;
> > > +        uint64_t mask;
> > > +} SysIBTl_cpu;
> > > +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
> > > +
> > > +/* Container type Topology List Entry */
> > > +typedef struct SysIBTl_container {
> > > +        uint8_t nl;
> > > +        uint8_t reserved[6];
> > > +        uint8_t id;
> > > +} QEMU_PACKED SysIBTl_container;
> > > +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> > > +
> > > +/* Generic Topology List Entry */
> > > +typedef union SysIBTl_entry {
> > > +        uint8_t nl;
> >
> > This union member is unused, isn't it?
> >
> > > +        SysIBTl_container container;
> > > +        SysIBTl_cpu cpu;
> > > +} SysIBTl_entry;
> > > +
> > > +#define TOPOLOGY_NR_MAG  6
> >
> > TOPOLOGY_TOTAL_NR_MAGS ?
> >
> > > +#define TOPOLOGY_NR_MAG6 0
> >
> > TOPOLOGY_NR_TLES_MAG6 ?
> >
> > I'm open to other suggestions but we need to differentiate between the
> > number of mag array entries and the number of TLEs in the MAGs.
>
>
> typedef enum {
>           TOPOLOGY_MAG6 = 0,
>           TOPOLOGY_MAG5 = 1,
>           TOPOLOGY_MAG4 = 2,
>           TOPOLOGY_MAG3 = 3,
>           TOPOLOGY_MAG2 = 4,
>           TOPOLOGY_MAG1 = 5,
>           TOPOLOGY_TOTAL_MAGS = 6,
> };
>
>
> oder enum with TOPOLOGY_NR_TLES_MAGx ?

I'd stick with the shorter first variant.

> > > +#define TOPOLOGY_NR_MAG5 1
> > > +#define TOPOLOGY_NR_MAG4 2
> > > +#define TOPOLOGY_NR_MAG3 3
> > > +#define TOPOLOGY_NR_MAG2 4
> > > +#define TOPOLOGY_NR_MAG1 5
> >
> > I'd appreciate a \n here.
>
> OK
>
> > > +/* Configuration topology */
> > > +typedef struct SysIB_151x {
> > > +    uint8_t  res0[2];
> >
> > You're using "reserved" everywhere but now it's "rev"?
>
> OK I will keep reserved
>
> > > +    uint16_t length;
> > > +    uint8_t  mag[TOPOLOGY_NR_MAG];
> > > +    uint8_t  res1;
> > > +    uint8_t  mnest;
> > > +    uint32_t res2;
> > > +    SysIBTl_entry tle[0];
> > > +} SysIB_151x;
> > > +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> > > +
> > >    /* MMU defines */
> > >    #define ASCE_ORIGIN           (~0xfffULL) /* segment table
> > > origin             */
> > >    #define ASCE_SUBSPACE         0x200       /* subspace group
> > > control           */