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

[Nina Schoetterl-Glausch]

On Tue, 2023-04-18 at 12:01 +0200, Pierre Morel wrote:
> 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>
> > > ---
[...]
> > 
> > > 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 ?

Yeah, I would completely drop it because it is a meaningless value
and adjust the conversion to the cpu value accordingly.
> 
> frankly I prefer to keep horizontal here which is exactly what is given 
> in the documentation for entitlement = 0

Not sure what you mean with this.
> 
> 
> 
> > 
> > [...]
> > 
> > > 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?

It's a user visible change, so no.
But it's not complicated, should be just:

const PropertyInfo qdev_prop_cpus390entitlement = {
    .name = "CpuS390Entitlement",
    .enum_table = &CpuS390Entitlement_lookup,
    .get   = qdev_propinfo_get_enum,
    .set   = qdev_propinfo_set_enum,
    .set_default_value = qdev_propinfo_set_default_value_enum,
};

Plus a comment & build bug in qdev-properties-system.c

and

extern const PropertyInfo qdev_prop_cpus390entitlement;
#define DEFINE_PROP_CPUS390ENTITLEMENT(_n, _s, _f, _d) \
    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_cpus390entitlement, \
                       CpuS390Entitlement)

in qdev-properties-system.h

You need to change the type of env.entitlement and set the default to 1 for 
medium
and that should be it.
> 
> 
> > 
> > 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.

No, you can just set it to medium and get rid of the adjustment code.
s390_topology_check will reject invalid changes and the default above
is fine since dedication is false.