Re: [PATCH v19 01/21] s390x/cpu topology: add s390 specifics to CPU topology

[Pierre Morel]

On 4/18/23 10:53, Nina Schoetterl-Glausch wrote:
> On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
> > S390 adds two new SMP levels, drawers and books to the CPU
> > topology.
> > The S390 CPU have specific topology features like dedication
> > and entitlement 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 entitlement,
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   MAINTAINERS                     |  5 ++++
> >   qapi/machine-common.json        | 22 ++++++++++++++
> >   qapi/machine-target.json        | 12 ++++++++
> >   qapi/machine.json               | 17 +++++++++--
> >   include/hw/boards.h             | 10 ++++++-
> >   include/hw/s390x/cpu-topology.h | 15 ++++++++++
> Is hw/s390x the right path for cpu-topology?
> I haven't understood the difference between hw/s390x and target/s390x
> but target/s390x feels more correct, I could be mistaken though.

AFAIK target/s390 is for CPU emulation code while hw/s390 is for other 
emulation.

So it depends how we classify the CPU topology, it is related to CPU but 
it is no emulation.

Since Thomas approved this layout I would like to keep it like this.


> >   target/s390x/cpu.h              |  5 ++++
> >   hw/core/machine-smp.c           | 53 ++++++++++++++++++++++++++++-----
> >   hw/core/machine.c               |  4 +++
> >   hw/s390x/s390-virtio-ccw.c      |  2 ++
> >   softmmu/vl.c                    |  6 ++++
> >   target/s390x/cpu.c              |  7 +++++
> >   qapi/meson.build                |  1 +
> >   qemu-options.hx                 |  7 +++--
> >   14 files changed, 152 insertions(+), 14 deletions(-)
> >   create mode 100644 qapi/machine-common.json
> >   create mode 100644 include/hw/s390x/cpu-topology.h
> [...]
>
> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> > new file mode 100644
> > index 0000000000..73ea38d976
> > --- /dev/null
> > +++ b/qapi/machine-common.json
> > @@ -0,0 +1,22 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> > +# See the COPYING file in the top-level directory.
> > +
> > +##
> > +# = Machines S390 data types
> > +##
> > +
> > +##
> > +# @CpuS390Entitlement:
> > +#
> > +# An enumeration of cpu entitlements that can be assumed by a virtual
> > +# S390 CPU
> > +#
> > +# Since: 8.1
> > +##
> > +{ 'enum': 'CpuS390Entitlement',
> > +  'prefix': 'S390_CPU_ENTITLEMENT',
> > +  'data': [ 'horizontal', 'low', 'medium', 'high' ] }
> You can get rid of the horizontal value now that the entitlement is ignored if 
> the
> polarization is vertical.


Right, horizontal is not used, but what would you like?

- replace horizontal with 'none' ?

- add or substract 1 when we do the conversion between enum string and 
value ?

frankly I prefer to keep horizontal here which is exactly what is given 
in the documentation for entitlement = 0



> [...]
>
> > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > index b10a8541ff..57165fa3a0 100644
> > --- a/target/s390x/cpu.c
> > +++ b/target/s390x/cpu.c
> > @@ -37,6 +37,7 @@
> >   #ifndef CONFIG_USER_ONLY
> >   #include "sysemu/reset.h"
> >   #endif
> > +#include "hw/s390x/cpu-topology.h"
> >   
> >   #define CR0_RESET       0xE0UL
> >   #define CR14_RESET      0xC2000000UL;
> > @@ -259,6 +260,12 @@ static gchar *s390_gdb_arch_name(CPUState *cs)
> >   static Property s390x_cpu_properties[] = {
> >   #if !defined(CONFIG_USER_ONLY)
> >       DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0),
> > +    DEFINE_PROP_INT32("socket-id", S390CPU, env.socket_id, -1),
> > +    DEFINE_PROP_INT32("book-id", S390CPU, env.book_id, -1),
> > +    DEFINE_PROP_INT32("drawer-id", S390CPU, env.drawer_id, -1),
> > +    DEFINE_PROP_BOOL("dedicated", S390CPU, env.dedicated, false),
> > +    DEFINE_PROP_UINT8("entitlement", S390CPU, env.entitlement,
> > +                      S390_CPU_ENTITLEMENT__MAX),
> I would define an entitlement PropertyInfo in qdev-properties-system.[ch],
> then one can use e.g.
>
> -device z14-s390x-cpu,core-id=11,entitlement=high


Don't you think it is an enhancement we can do later?


> on the command line and cpu hotplug.
>
> I think setting the default entitlement to medium here should be fine.
>
> [...]

right, I had medium before and should not have change it.

Anyway what ever the default is, it must be changed later depending on 
dedication.