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

[Pierre Morel <pmorel@linux.ibm.com>]

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.