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

[Pierre Morel]

On 4/18/23 16:58, Nina Schoetterl-Glausch wrote:
> > 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:
[...]
> > 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;
> +}


If it is to hide it, we can use a macro:

#define entitlement_to_topology_id(x) (x + 1)

shorter and does the same.


> You can also use a switch of course.
> I'd also rename s390_topology_id.entitlement to polarization.

I do not like that, polarization is a machine concept, not a CPU concept.


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


OK, I will do like 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.
> >
> > 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.


It is not so complicated and as said would be an extension and not a 
change, so it would reduce changes to be done for the series.

But OK, as above, I change it this for the next spin.


> > > 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;
> +    }
> +}

Yes but before the code you show here there is the 
s390_topology_cpu_default() which prepares for this check.


> > So we need adjustment before the check in both cases.
> I don't see why, just always reject it.

Specifying a medium entitlement with dedication on the command line is 
wrong so I think we should have the following result when specifying the 
CPU:

"default + shared" -> "medium"

"default + dedicated" -> "high + dedicated"

but

"medium + dedicated" -> ERROR

If a CPU is specified from its creation as "medium" how do we know if it 
is the user's specification or the default set in the case the user did 
not specify anything?


Regards,

Pierre