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

[Nina Schoetterl-Glausch]

> 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@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.
> 
> 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.
> 
> 
> 
>