Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug

[Nina Schoetterl-Glausch]

On Tue, 2023-04-25 at 10:45 +0200, Pierre Morel wrote:
> On 4/24/23 17:32, Nina Schoetterl-Glausch wrote:
> > On Fri, 2023-04-21 at 12:20 +0200, Pierre Morel wrote:
> > > > On 4/20/23 10:59, Nina Schoetterl-Glausch wrote:
> > > > > > On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
> [..]
> > > > In the next version with entitlement being an enum it is right.
> > > > 
> > > > However, deleting this means that the default value for entitlement
> > > > depends on dedication.
> > > > 
> > > > If we have only low, medium, high and default for entitlement is medium.
> > > > 
> > > > If the user specifies the dedication true without specifying entitlement
> > > > we could force entitlement to high.
> > > > 
> > > > But we can not distinguish this from the user specifying dedication true
> > > > with a medium entitlement, which is wrong.
> > > > 
> > > > So three solution:
> > > > 
> > > > 1) We ignore what the user say if dedication is specified as true
> > > > 
> > > > 2) We specify that both dedication and entitlement must be specified if
> > > > dedication is true
> > > > 
> > > > 3) We set an impossible default to distinguish default from medium
> > > > entitlement
> > > > 
> > > > 
> > > > For me the solution 3 is the best one, it is more flexible for the user.
> > > > 
> > > > Solution 1 is obviously bad.
> > > > 
> > > > Solution 2 forces the user to specify entitlement high and only high if
> > > > it specifies dedication true.
> > > > 
> > > > AFAIU, you prefer the solution 2, forcing user to specify both
> > > > dedication and entitlement to suppress a default value in the enum.
> > > > Why is it bad to have a default value in the enum that we do not use to
> > > > specify that the value must be calculated?
> > Yes, I'd prefer solution 2. I don't like adapting the internal state where 
> > only
> > the three values make sense for the user interface.
> > It also keeps things simple and requires less code.
> > I also don't think it's a bad thing for the user, as it's not a thing done 
> > manually often.
> > I'm also not a fan of a value being implicitly being changed even though it 
> > doesn't look
> > like it from the command.
> > 
> > However, what I really don't like is the additional state and naming it 
> > "horizontal",
> 
> 
> No problem to use another name like "auto" as you propose later.
> 
> 
> > not so much the adjustment if dedication is switched to true without an 
> > entitlement given.
> > For the monitor command there is no problem, you currently have:
> 
> 
> That is clear, the has_xxx does the job.
> 
> [..]
> 
> 
> > So you can just set it if (!has_entitlement).
> > There is also ways to set the value for cpus defined on the command line, 
> > e.g.:
> 
> 
> Yes, thanks, I already said I find your proposition to use a 
> DEFINE_PROP_CPUS390ENTITLEMENT good and will use it.
> 
> 
> > 
> > diff --git a/include/hw/qdev-properties-system.h 
> > b/include/hw/qdev-properties-system.h
> > index 0ac327ae60..41a605c5a7 100644
> > --- a/include/hw/qdev-properties-system.h
> > +++ b/include/hw/qdev-properties-system.h
> > @@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev;
> >   extern const PropertyInfo qdev_prop_off_auto_pcibar;
> >   extern const PropertyInfo qdev_prop_pcie_link_speed;
> >   extern const PropertyInfo qdev_prop_pcie_link_width;
> > +extern const PropertyInfo qdev_prop_cpus390entitlement;
> >   
> >   #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
> >       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
> > @@ -73,5 +74,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
> >   #define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) \
> >       DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID)
> >   
> > +#define DEFINE_PROP_CPUS390ENTITLEMENT(_n, _s, _f) \
> > +    DEFINE_PROP(_n, _s, _f, qdev_prop_cpus390entitlement, int)
> > +
> >   
> >   #endif
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index 54541d2230..01308e0b94 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -135,7 +135,7 @@ struct CPUArchState {
> >       int32_t book_id;
> >       int32_t drawer_id;
> >       bool dedicated;
> > -    uint8_t entitlement;        /* Used only for vertical polarization */
> > +    int entitlement;        /* Used only for vertical polarization */
> 
> 
> Isn't it better to use:
> 
> +    CpuS390Entitlement entitlement; /* Used only for vertical 
> polarization */
> 
> 
> >       uint64_t cpuid;
> >   #endif
> >   
> > diff --git a/hw/core/qdev-properties-system.c 
> > b/hw/core/qdev-properties-system.c
> > index d42493f630..db5c3d4fe6 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -1143,3 +1143,14 @@ const PropertyInfo qdev_prop_uuid = {
> >       .set   = set_uuid,
> >       .set_default_value = set_default_uuid_auto,
> >   };
> > +
> > +/* --- s390x cpu topology entitlement --- */
> > +
> > +QEMU_BUILD_BUG_ON(sizeof(CpuS390Entitlement) != sizeof(int));
> > +
> > +const PropertyInfo qdev_prop_cpus390entitlement = {
> > +    .name = "CpuS390Entitlement",
> > +    .enum_table = &CpuS390Entitlement_lookup,
> > +    .get   = qdev_propinfo_get_enum,
> > +    .set   = qdev_propinfo_set_enum,
> > +};
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index b8a292340c..1b3f5c61ae 100644
> > --- a/hw/s390x/cpu-topology.c
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -199,8 +199,7 @@ static void s390_topology_cpu_default(S390CPU *cpu, 
> > Error **errp)
> >        * is not dedicated.
> >        * A dedicated CPU always receives a high entitlement.
> >        */
> > -    if (env->entitlement >= S390_CPU_ENTITLEMENT__MAX ||
> > -        env->entitlement == S390_CPU_ENTITLEMENT_HORIZONTAL) {
> > +    if (env->entitlement < 0) {
> 
> 
> Here we can have:
> 
> +    if (env->entitlement == S390_CPU_ENTITLEMENT_AUTO) {
> ...
> 
> >           if (env->dedicated) {
> >               env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
> >           } else {
> > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > index 57165fa3a0..dea50a3e06 100644
> > --- a/target/s390x/cpu.c
> > +++ b/target/s390x/cpu.c
> > @@ -31,6 +31,7 @@
> >   #include "qapi/qapi-types-machine.h"
> >   #include "sysemu/hw_accel.h"
> >   #include "hw/qdev-properties.h"
> > +#include "hw/qdev-properties-system.h"
> >   #include "fpu/softfloat-helpers.h"
> >   #include "disas/capstone.h"
> >   #include "sysemu/tcg.h"
> > @@ -248,6 +249,7 @@ static void s390_cpu_initfn(Object *obj)
> >       cs->exception_index = EXCP_HLT;
> >   
> >   #if !defined(CONFIG_USER_ONLY)
> > +    cpu->env.entitlement = -1;
> 
> 
> Then we do not need this initialization if here under we define 
> DEFINE_PROP_CPUS390ENTITLEMENT differently
> 
> 
> >       s390_cpu_init_sysemu(obj);
> >   #endif
> >   }
> > @@ -264,8 +266,7 @@ static Property s390x_cpu_properties[] = {
> >       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),
> > +    DEFINE_PROP_CPUS390ENTITLEMENT("entitlement", S390CPU, 
> > env.entitlement),
> 
> 
> +    DEFINE_PROP_CPUS390ENTITLEMENT("entitlement", S390CPU, env.entitlement,
>                                     S390_CPU_ENTITLEMENT_AUTO),
> 
> >   #endif
> >       DEFINE_PROP_END_OF_LIST()
> >   };
> > 
> > There are other ways to achieve the same, you could also
> > implement get, set and set_default_value so that there is an additional
> > "auto"/"uninitialized" value that is not in the enum.
> > If you insist on having an additional state in the enum, name it "auto".
> 
> Yes, I think it is a better name.

IMO using entitlement=auto doesn't make too much sense with the 
set-cpu-topology command,
because you can just leave if off or specify the entitlement you want directly.
So there is no actual need to have a user visible auto value and no need to 
have it in the enum.
Then the only problem is adjusting the entitlement when doing dedicated=on on 
the command line.
(If you want that)
So with my proposal there are only the low, medium and high values in the enum.
In order to set the entitlement automatically when using the command line I 
initialize
the entitlement to -1, so we later know if it has been set via the command line 
or not.
But you cannot set -1 via the property because qdev_propinfo_get_enum expects a 
string,
which is why I do it in s390_cpu_initfn.

I'm not sure if you can define entitlement as CpuS390Entitlement.
I think I changed it to int when I was exploring different solutions
and had to change it because of a type check. But what I proposed above doesn't 
cause the same issue.
DEFINE_PROP_CPUS390ENTITLEMENT could then also use CpuS390Entitlement.

So there are three possible solutions now:
1. My proposal above, which as automatic adjustment, but only the three 
required values in the enum.
2. Don't do automatic adjustment, three enum values.
3. Automatic adjustment with auto value in the enum.

I still favor 2. but the other ones aren't terrible.