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@xxxxxxxxxxxxx>
> > > Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
> > > ---
[...]
> >
> > > 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.
