Re: [PATCH v14 01/11] s390x/cpu topology: adding s390 specificities to CPU topology

[Pierre Morel]

On 1/16/23 21:34, Nina Schoetterl-Glausch wrote:
> On Mon, 2023-01-16 at 18:28 +0100, Pierre Morel wrote:
> > On 1/13/23 17:58, Nina Schoetterl-Glausch wrote:
> > > On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
> > > > S390 adds two new SMP levels, drawers and books to the CPU
> > > > topology.
> > > > The S390 CPU have specific toplogy features like dedication
> > > > and polarity to give to the guest indications on the host
> > > > vCPUs scheduling and help the guest take the best decisions
> > > > on the scheduling of threads on the vCPUs.
> > > >
> > > > Let us provide the SMP properties with books and drawers levels
> > > > and S390 CPU with dedication and polarity,
> > > >
> > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > ---
> > > >    qapi/machine.json               | 14 ++++++++--
> > > >    include/hw/boards.h             | 10 ++++++-
> > > >    include/hw/s390x/cpu-topology.h | 23 ++++++++++++++++
> > > >    target/s390x/cpu.h              |  6 +++++
> > > >    hw/core/machine-smp.c           | 48 ++++++++++++++++++++++++++++-----
> > > >    hw/core/machine.c               |  4 +++
> > > >    hw/s390x/s390-virtio-ccw.c      |  2 ++
> > > >    softmmu/vl.c                    |  6 +++++
> > > >    target/s390x/cpu.c              | 10 +++++++
> > > >    qemu-options.hx                 |  6 +++--
> > > >    10 files changed, 117 insertions(+), 12 deletions(-)
> > > >    create mode 100644 include/hw/s390x/cpu-topology.h
> > > [...]
> > >
> > > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > > > index 7d6d01325b..39ea63a416 100644
> > > > --- a/target/s390x/cpu.h
> > > > +++ b/target/s390x/cpu.h
> > > > @@ -131,6 +131,12 @@ struct CPUArchState {
> > > >    
> > > >    #if !defined(CONFIG_USER_ONLY)
> > > >        uint32_t core_id; /* PoP "CPU address", same as cpu_index */
> > > > +    int32_t socket_id;
> > > > +    int32_t book_id;
> > > > +    int32_t drawer_id;
> > > > +    int32_t dedicated;
> > > > +    int32_t polarity;
> > >
> > > If I understood the architecture correctly, the polarity is a property of the 
> > > configuration,
> > > not the cpus. So this should be vertical_entitlement, and there should be a 
> > > machine (?) property
> > > specifying if the polarity is horizontal or vertical.
> >
> > You are right, considering PTF only, the documentation says PTF([01])
> > does the following:
> >
> > "... a process is initiated to place all CPUs in the configuration into
> > the polarization specified by the function code, ..."
> >
> > So on one side the polarization property is explicitly set on the CPU,
> > and on the other side all CPU are supposed to be in the same
> > polarization state.
>
> I'm worried about STSI showing both horizontal and vertical CPUs at the same 
> time.
> I don't know if this is allowed.
> If it is not, you need a way to switch between those atomically, which is harder
> if every CPU has this property.

The documentation explicitly provides the order in which to show the TLE 
when both vertical and horizontal TLE are display inside the SYSIB.
So it seems expected by the architecture.
I understand that it is because the documentation says that the 
instruction finished before all the CPU did change polarity.

But I think you are right on the point that:
- It is only transitional
- it does not make sense for my point of view to have both at the same time.

In QEMU, as these are two interceptions we will always finish the PTF 
interception before we start the STSI interception, so we will never see 
this happen.

Question are:

1) Should we do better than the architecture?

2) If yes, are we sure it is better?

Regards,
Pierre



--
Pierre Morel
IBM Lab Boeblingen