Re: [PATCH v20 03/21] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Pierre Morel]

On 4/27/23 19:01, Thomas Huth wrote:
> On 25/04/2023 18.14, Pierre Morel wrote:
> > On interception of STSI(15.1.x) the System Information Block
> > (SYSIB) is built from the list of pre-ordered topology entries.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   MAINTAINERS                     |   1 +
> >   include/hw/s390x/cpu-topology.h |  24 +++
> >   include/hw/s390x/sclp.h         |   1 +
> >   target/s390x/cpu.h              |  72 ++++++++
> >   hw/s390x/cpu-topology.c         |  13 +-
> >   target/s390x/kvm/cpu_topology.c | 308 ++++++++++++++++++++++++++++++++
> >   target/s390x/kvm/kvm.c          |   5 +-
> >   target/s390x/kvm/meson.build    |   3 +-
> >   8 files changed, 424 insertions(+), 3 deletions(-)
> >   create mode 100644 target/s390x/kvm/cpu_topology.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index bb7b34d0d8..de9052f753 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1659,6 +1659,7 @@ M: Pierre Morel <pmorel@linux.ibm.com>
> >   S: Supported
> >   F: include/hw/s390x/cpu-topology.h
> >   F: hw/s390x/cpu-topology.c
> > +F: target/s390x/kvm/cpu_topology.c
>
> It's somewhat weird to have one file "cpu-topology.c" (in hw/s390x, 
> with a dash), and one file cpu_topology.c (in target/s390x, with an 
> underscore) ... could you come up with a better naming? Maybe call the 
> new file stsi-topology.c or so?


If I keep the two files (see answer to your comments on patch 2) then 
yes I can change it to stsi-topology.c,


> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index bb7cfb0cab..9f97989bd7 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -561,6 +561,25 @@ typedef struct SysIB_322 {
> >   } SysIB_322;
> >   QEMU_BUILD_BUG_ON(sizeof(SysIB_322) != 4096);
>
>
> Maybe add a short comment here what MAG stands for (magnitude fields?)?


Yes, in fact Magnitude bytes.


> > +#define S390_TOPOLOGY_MAG  6
> > +#define S390_TOPOLOGY_MAG6 0
> > +#define S390_TOPOLOGY_MAG5 1
> > +#define S390_TOPOLOGY_MAG4 2
> > +#define S390_TOPOLOGY_MAG3 3
> > +#define S390_TOPOLOGY_MAG2 4
> > +#define S390_TOPOLOGY_MAG1 5
> > +/* Configuration topology */
> > +typedef struct SysIB_151x {
> > +    uint8_t  reserved0[2];
> > +    uint16_t length;
> > +    uint8_t  mag[S390_TOPOLOGY_MAG];
> > +    uint8_t  reserved1;
> > +    uint8_t  mnest;
> > +    uint32_t reserved2;
> > +    char tle[];
> > +} SysIB_151x;
> > +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> ...
>
> > diff --git a/target/s390x/kvm/cpu_topology.c 
> > b/target/s390x/kvm/cpu_topology.c
> > new file mode 100644
> > index 0000000000..86a286afe2
> > --- /dev/null
> > +++ b/target/s390x/kvm/cpu_topology.c
> > @@ -0,0 +1,308 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * QEMU S390x CPU Topology
> > + *
> > + * Copyright IBM Corp. 2022,2023
> > + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> > + *
> > + */
> > +#include "qemu/osdep.h"
> > +#include "cpu.h"
> > +#include "hw/s390x/pv.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/s390x/sclp.h"
> > +#include "hw/s390x/cpu-topology.h"
> > +
> > +/**
> > + * fill_container:
> > + * @p: The address of the container TLE to fill
> > + * @level: The level of nesting for this container
> > + * @id: The container receives a uniq ID inside its own container
>
> s/uniq/unique/


yes, thanks,

regards,

Pierre


>  Thomas