Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

[Janis Schoetterl-Glausch]

On Fri, 2022-10-28 at 12:00 +0200, Pierre Morel wrote:
> 
> On 10/27/22 22:42, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-10-12 at 18:21 +0200, Pierre Morel wrote:
> > > The guest can use the STSI instruction to get a buffer filled
> > > with the CPU topology description.
> > > 
> > > Let us implement the STSI instruction for the basis CPU topology
> > > level, level 2.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   include/hw/s390x/cpu-topology.h |   3 +
> > >   target/s390x/cpu.h              |  48 ++++++++++++++
> > >   hw/s390x/cpu-topology.c         |   8 ++-
> > >   target/s390x/cpu_topology.c     | 109 ++++++++++++++++++++++++++++++++
> > >   target/s390x/kvm/kvm.c          |   6 +-
> > >   target/s390x/meson.build        |   1 +
> > >   6 files changed, 172 insertions(+), 3 deletions(-)
> > >   create mode 100644 target/s390x/cpu_topology.c
> > > 
> > > diff --git a/include/hw/s390x/cpu-topology.h 
> > > b/include/hw/s390x/cpu-topology.h
> > > index 66c171d0bc..61c11db017 100644
> > > --- a/include/hw/s390x/cpu-topology.h
> > > +++ b/include/hw/s390x/cpu-topology.h
> > > @@ -13,6 +13,8 @@
> > >   #include "hw/qdev-core.h"
> > >   #include "qom/object.h"
> > >   
> > > +#define S390_TOPOLOGY_POLARITY_H  0x00
> > > +
> > >   typedef struct S390TopoContainer {
> > >       int active_count;
> > >   } S390TopoContainer;
> > > @@ -29,6 +31,7 @@ struct S390Topology {
> > >       S390TopoContainer *socket;
> > >       S390TopoTLE *tle;
> > >       MachineState *ms;
> > > +    QemuMutex topo_mutex;
> > >   };
> > >   
> > >   #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > index 7d6d01325b..d604aa9c78 100644
> > > --- a/target/s390x/cpu.h
> > > +++ b/target/s390x/cpu.h
> > > 
> > [...]
> > > +
> > > +/* Maxi size of a SYSIB structure is when all CPU are alone in a 
> > > container */
> > 
> > Max or Maximum.
> > 
> > > +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) +                   
> > >       \
> > > +                                  S390_MAX_CPUS * 
> > > (sizeof(SysIBTl_container) + \
> > > +                                                   sizeof(SysIBTl_cpu)))
> > 
> > Currently this is 16+248*3*8 == 5968 and will grow with books, drawer 
> > support to
> > 16+248*5*8 == 9936 ...
> > 
> > [...]
> > > 
> > > +
> > > +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> > > +{
> > > +    uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};
> > 
> > ... so calling this page is a bit misleading. Also why not make it a char[]?
> > And maybe use a union for type punning.
> 
> OK, what about:
> 
>      union {
>          char place_holder[S390_TOPOLOGY_SYSIB_SIZE];
>          SysIB_151x sysib;
>      } buffer QEMU_ALIGNED(8);
> 
I don't think you need the QEMU_ALIGNED since SysIB_151x already has it. Not 
that it hurts to be
explicit. If you declared the tle member as uint64_t[], you should get the 
correct alignment
automatically and can then drop the explicit one.
Btw, [] seems to be preferred over [0], at least there is a commit doing a 
conversion:
f7795e4096 ("misc: Replace zero-length arrays with flexible array member 
(automatic)")
> 
> > 
> > > +    SysIB_151x *sysib = (SysIB_151x *) page;
> > > +    int len;
> > > +
> > > +    if (s390_is_pv() || !s390_has_topology() ||
> > > +        sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> > > +        setcc(cpu, 3);
> > > +        return;
> > > +    }
> > > +
> > > +    len = setup_stsi(sysib, sel2);
> > 
> > This should now be memory safe, but might be larger than 4k,
> > the maximum size of the SYSIB. I guess you want to set cc code 3
> > in this case and return.
> 
> I do not find why the SYSIB can not be larger than 4k.
> Can you point me to this restriction?

Says so at the top of the description of STSI:

The SYSIB is 4K bytes and must begin at a 4 K-byte
boundary; otherwise, a specification exception may
be recognized.

Also the graphics show that it is 1024 words long.
> 
> 
> Regards,
> Pierre
>