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

[Nina Schoetterl-Glausch]

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.
> 
> So yes we can make the horizontal/vertical a machine property.
> However, we do not need to set this tunable as the documentation says 
> that the machine always start with horizontal polarization.
> 
> On the other hand the documentation mixes a lot vertical with different 
> entitlement and horizontal polarization, for TLE order and slacks so I 
> prefer to keep the complete description of the polarization as CPU 
> properties in case we miss something.
> 
> PTF([01]) are no performance bottle neck and the number of CPU is likely 
> to be small, even a maximum of 248 is possible KVM warns above 16 CPU so 
> the loop for setting all CPU inside PTF interception is not very 
> problematic I think.

Yeah, I'm not worried about that.
> 
> Doing like you say should simplify PTF interception (no loop) but 
> complicates (some more if/else) TLE handling and QMP information display 
> on CPU.
> So I will have a look at the implications and answer again on this.
> 
> Thanks,
> 
> Regards,
> Pierre
>