> On 4/18/23 14:38, Nina Schoetterl-Glausch wrote:
> > 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.
>
> I mean: Extract from the PoP:
>
> ----
>
> The following values are used:
> PP Meaning
> 0 The one or more CPUs represented by the TLE are
> horizontally polarized.
> 1 The one or more CPUs represented by the TLE are
> vertically polarized. Entitlement is low.
> 2 The one or more CPUs represented by the TLE are
> vertically polarized. Entitlement is medium.
> 3 The one or more CPUs represented by the TLE are
> vertically polarized. Entitlement is high.
>
> ----
>
> Also I find that using an enum to systematically add/subtract a value is
> for me weird.
It is, I'd do:
+static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
+{
+ struct S390CcwMachineState *s390ms = S390_CCW_MACHINE(current_machine);
+ s390_topology_id topology_id = {0};
+
+ topology_id.drawer = cpu->env.drawer_id;
+ topology_id.book = cpu->env.book_id;
+ topology_id.socket = cpu->env.socket_id;
+ topology_id.origin = cpu->env.core_id / 64;
+ topology_id.type = S390_TOPOLOGY_CPU_IFL;
+ topology_id.dedicated = cpu->env.dedicated;
+
+ if (s390ms->vertical_polarization) {
+ uint8_t to_polarization[] = {
+ [S390_CPU_ENTITLEMENT_LOW] = 1,
+ [S390_CPU_ENTITLEMENT_MEDIUM] = 2,
+ [S390_CPU_ENTITLEMENT_HIGH] = 3,
+ };
+ topology_id.entitlement = to_polarization[cpu->env.entitlement];
+ }
+
+ return topology_id;
+}
You can also use a switch of course.
I'd also rename s390_topology_id.entitlement to polarization.
>
> so I really prefer to keep "horizontal", "low", "medium", "high" event
> "horizontal" will never appear.
>
> A mater of taste, it does not change anything to the functionality or
> the API.
Well, it does change the API a bit, namely which values mean what,
currently there is a value 0 that you're not supposed to use, that would go away.
It also shows up in some meta command to print qapi interfaces.
And dropping it simplifies the implementation IMO --- you don't need
to think about and prevent usage of a nonexistent state.
>
>
> > >
> > >
> > > > [...]
> > > >
> > > > > 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.
>
>
> We could have kept both string and integer.
That sounds harder to do, I guess you'd have to reimplement the PropertyInfo
getters and setters to do that.
>
>
> > 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.
>
>
> OK, it does not change anything to the functionality but is a little bit
> more pretty.
>
>
> > >
> > > > 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.
>
>
> I do not want a default specification for the entitlement to depend on
> the polarization.
I don't see why we cannot just set it to medium.
>
> If we do as you propose, by horizontal polarization a default
> entitlement with dedication will be accepted but will be refused after
> the guest switched for vertical polarization.
No, your check function doesn't look the polarization at all (and shouldn't):
+static void s390_topology_check(uint16_t socket_id, uint16_t book_id,
+ uint16_t drawer_id, uint16_t entitlement,
+ bool dedicated, Error **errp)
+{
[...]
+ if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW ||
+ entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) {
+ error_setg(errp, "A dedicated cpu implies high entitlement");
+ return;
+ }
+}
>
> So we need adjustment before the check in both cases.
I don't see why, just always reject it.
>
> I find it easier and more logical if there is no default value than to
> have a default we need to overwrite.
>
>
>
>
