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

The topology information are attributes of the CPU and are
specified during the CPU device creation.

On hot plug we:
- calculate the default values for the topology for drawers,
  books and sockets in the case they are not specified.
- verify the CPU attributes
- check that we have still room on the desired socket

The possibility to insert a CPU in a mask is dependent on the
number of cores allowed in a socket, a book or a drawer, the
checking is done during the hot plug of the CPU to have an
immediate answer.

If the complete topology is not specified, the core is added
in the physical topology based on its core ID and it gets
defaults values for the modifier attributes.

This way, starting QEMU without specifying the topology can
still get some advantage of the CPU topology.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 MAINTAINERS                        |   1 +
 include/hw/s390x/cpu-topology.h    |  44 +++++
 include/hw/s390x/s390-virtio-ccw.h |   1 +
 hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c         |  22 ++-
 hw/s390x/meson.build               |   1 +
 6 files changed, 349 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/cpu-topology.c

On 4/3/23 18:28, Pierre Morel wrote:
> The topology information are attributes of the CPU and are
> specified during the CPU device creation.
> 
> On hot plug we:
> - calculate the default values for the topology for drawers,
>    books and sockets in the case they are not specified.
> - verify the CPU attributes
> - check that we have still room on the desired socket
> 
> The possibility to insert a CPU in a mask is dependent on the
> number of cores allowed in a socket, a book or a drawer, the
> checking is done during the hot plug of the CPU to have an
> immediate answer.
> 
> If the complete topology is not specified, the core is added
> in the physical topology based on its core ID and it gets
> defaults values for the modifier attributes.
> 
> This way, starting QEMU without specifying the topology can
> still get some advantage of the CPU topology.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   MAINTAINERS                        |   1 +
>   include/hw/s390x/cpu-topology.h    |  44 +++++
>   include/hw/s390x/s390-virtio-ccw.h |   1 +
>   hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-ccw.c         |  22 ++-
>   hw/s390x/meson.build               |   1 +
>   6 files changed, 349 insertions(+), 2 deletions(-)
>   create mode 100644 hw/s390x/cpu-topology.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9b1f80739e..bb7b34d0d8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1658,6 +1658,7 @@ S390 CPU topology
>   M: Pierre Morel <pmorel@linux.ibm.com>
>   S: Supported
>   F: include/hw/s390x/cpu-topology.h
> +F: hw/s390x/cpu-topology.c
>   
>   X86 Machines
>   ------------
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 83f31604cc..6326cfeff8 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -10,6 +10,50 @@
>   #ifndef HW_S390X_CPU_TOPOLOGY_H
>   #define HW_S390X_CPU_TOPOLOGY_H
>   
> +#ifndef CONFIG_USER_ONLY
> +
> +#include "qemu/queue.h"
> +#include "hw/boards.h"
> +#include "qapi/qapi-types-machine-target.h"
> +
>   #define S390_TOPOLOGY_CPU_IFL   0x03
>   
> +typedef struct S390Topology {
> +    uint8_t *cores_per_socket;
> +    CpuTopology *smp;
> +} S390Topology;
> +
> +#ifdef CONFIG_KVM
> +bool s390_has_topology(void);
> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
> +#else
> +static inline bool s390_has_topology(void)
> +{
> +       return false;
> +}
> +static inline void s390_topology_setup_cpu(MachineState *ms,
> +                                           S390CPU *cpu,
> +                                           Error **errp) {}
> +#endif
> +
> +extern S390Topology s390_topology;
> +int s390_socket_nb(S390CPU *cpu);
> +
> +static inline int s390_std_socket(int n, CpuTopology *smp)
> +{
> +    return (n / smp->cores) % smp->sockets;
> +}
> +
> +static inline int s390_std_book(int n, CpuTopology *smp)
> +{
> +    return (n / (smp->cores * smp->sockets)) % smp->books;
> +}
> +
> +static inline int s390_std_drawer(int n, CpuTopology *smp)
> +{
> +    return (n / (smp->cores * smp->sockets * smp->books)) % smp->drawers;
> +}
> +
> +#endif /* CONFIG_USER_ONLY */
> +
>   #endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9bba21a916..ea10a6c6e1 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>       bool dea_key_wrap;
>       bool pv;
>       uint8_t loadparm[8];
> +    bool vertical_polarization;
>   };
>   
>   struct S390CcwMachineClass {
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..96da67bd7e
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,282 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> +
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/boards.h"
> +#include "qemu/typedefs.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +/*
> + * s390_topology is used to keep the topology information.
> + * .cores_per_socket: tracks information on the count of cores
> + *                    per socket.
> + * .smp: keeps track of the machine topology.
> + *
> + */
> +S390Topology s390_topology = {
> +    /* will be initialized after the cpu model is realized */
> +    .cores_per_socket = NULL,
> +    .smp = NULL,
> +};
> +
> +/**
> + * s390_socket_nb:
> + * @cpu: s390x CPU
> + *
> + * Returns the socket number used inside the cores_per_socket array
> + * for a cpu.
> + */
> +int s390_socket_nb(S390CPU *cpu)

s390_socket_nb() doesn't seem to be used anywhere else than in
hw/s390x/cpu-topology.c. It should be static.


> +{
> +    return (cpu->env.drawer_id * s390_topology.smp->books + cpu->env.book_id) *
> +           s390_topology.smp->sockets + cpu->env.socket_id;
> +}
> +
> +/**
> + * s390_has_topology:
> + *
> + * Return value: if the topology is supported by the machine.
> + */
> +bool s390_has_topology(void)
> +{
> +    return false;
> +}
> +
> +/**
> + * s390_topology_init:
> + * @ms: the machine state where the machine topology is defined
> + *
> + * Keep track of the machine topology.
> + *
> + * Allocate an array to keep the count of cores per socket.
> + * The index of the array starts at socket 0 from book 0 and
> + * drawer 0 up to the maximum allowed by the machine topology.
> + */
> +static void s390_topology_init(MachineState *ms)
> +{
> +    CpuTopology *smp = &ms->smp;
> +
> +    s390_topology.smp = smp;
> +    s390_topology.cores_per_socket = g_new0(uint8_t, smp->sockets *
> +                                            smp->books * smp->drawers);
> +}
> +
> +/**
> + * s390_topology_cpu_default:
> + * @cpu: pointer to a S390CPU
> + * @errp: Error pointer
> + *
> + * Setup the default topology if no attributes are already set.
> + * Passing a CPU with some, but not all, attributes set is considered
> + * an error.
> + *
> + * The function calculates the (drawer_id, book_id, socket_id)
> + * topology by filling the cores starting from the first socket
> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
> + *
> + * CPU type and dedication have defaults values set in the
> + * s390x_cpu_properties, entitlement must be adjust depending on the
> + * dedication.
> + */
> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    CPUS390XState *env = &cpu->env;
> +
> +    /* All geometry topology attributes must be set or all unset */
> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
> +        error_setg(errp,
> +                   "Please define all or none of the topology geometry attributes");
> +        return;
> +    }
> +
> +    /* Check if one of the geometry topology is unset */
> +    if (env->socket_id < 0) {
> +        /* Calculate default geometry topology attributes */
> +        env->socket_id = s390_std_socket(env->core_id, smp);
> +        env->book_id = s390_std_book(env->core_id, smp);
> +        env->drawer_id = s390_std_drawer(env->core_id, smp);
> +    }
> +
> +    /*
> +     * Even the user can specify the entitlement as horizontal on the
> +     * command line, qemu will only use env->entitlement during vertical
> +     * polarization.
> +     * Medium entitlement is chosen as the default entitlement when the CPU
> +     * 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->dedicated) {
> +            env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
> +        } else {
> +            env->entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
> +        }
> +    }
> +}
> +
> +/**
> + * s390_topology_check:
> + * @socket_id: socket to check
> + * @book_id: book to check
> + * @drawer_id: drawer to check
> + * @entitlement: entitlement to check
> + * @dedicated: dedication to check
> + * @errp: Error pointer
> + *
> + * The function checks if the topology
> + * attributes fits inside the system topology.
> + */
> +static void s390_topology_check(uint16_t socket_id, uint16_t book_id,
> +                                uint16_t drawer_id, uint16_t entitlement,
> +                                bool dedicated, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    ERRP_GUARD();
> +
> +    if (socket_id >= smp->sockets) {
> +        error_setg(errp, "Unavailable socket: %d", socket_id);
> +        return;
> +    }
> +    if (book_id >= smp->books) {
> +        error_setg(errp, "Unavailable book: %d", book_id);
> +        return;
> +    }
> +    if (drawer_id >= smp->drawers) {
> +        error_setg(errp, "Unavailable drawer: %d", drawer_id);
> +        return;
> +    }
> +    if (entitlement >= S390_CPU_ENTITLEMENT__MAX) {
> +        error_setg(errp, "Unknown entitlement: %d", entitlement);
> +        return;
> +    }
> +    if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW ||
> +                      entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) {
> +        error_setg(errp, "A dedicated cpu implies high entitlement");
> +        return;
> +    }
> +}
> +
> +/**
> + * s390_topology_add_core_to_socket:
> + * @cpu: the new S390CPU to insert in the topology structure
> + * @drawer_id: new drawer_id
> + * @book_id: new book_id
> + * @socket_id: new socket_id
> + * @creation: if is true the CPU is a new CPU and there is no old socket
> + *            to handle.
> + *            if is false, this is a moving the CPU and old socket count
> + *            must be decremented.
> + * @errp: the error pointer
> + *
> + */
> +static void s390_topology_add_core_to_socket(S390CPU *cpu, int drawer_id,
> +                                             int book_id, int socket_id,
> +                                             bool creation, Error **errp)
> +{

Since this routine is called twice, in s390_topology_setup_cpu() for
creation, and in s390_change_topology() for socket migration, we could
duplicate the code in two distinct routines.

I think this would simplify a bit each code path and avoid the 'creation'
parameter which is confusing.


> +    int old_socket_entry = s390_socket_nb(cpu);
> +    int new_socket_entry;
> +
> +    if (creation) {
> +        new_socket_entry = old_socket_entry;
> +    } else {
> +        new_socket_entry = (drawer_id * s390_topology.smp->books + book_id) *
> +                            s390_topology.smp->sockets + socket_id;

A helper common routine that s390_socket_nb() could use also would be a plus.

> +    }
> +
> +    /* Check for space on new socket */
> +    if ((new_socket_entry != old_socket_entry) &&
> +        (s390_topology.cores_per_socket[new_socket_entry] >=
> +         s390_topology.smp->cores)) {
> +        error_setg(errp, "No more space on this socket");
> +        return;
> +    }
> +
> +    /* Update the count of cores in sockets */
> +    s390_topology.cores_per_socket[new_socket_entry] += 1;
> +    if (!creation) {
> +        s390_topology.cores_per_socket[old_socket_entry] -= 1;
> +    }
> +}
> +
> +/**
> + * s390_update_cpu_props:
> + * @ms: the machine state
> + * @cpu: the CPU for which to update the properties from the environment.
> + *
> + */
> +static void s390_update_cpu_props(MachineState *ms, S390CPU *cpu)
> +{
> +    CpuInstanceProperties *props;
> +
> +    props = &ms->possible_cpus->cpus[cpu->env.core_id].props;
> +
> +    props->socket_id = cpu->env.socket_id;
> +    props->book_id = cpu->env.book_id;
> +    props->drawer_id = cpu->env.drawer_id;
> +}
> +
> +/**
> + * s390_topology_setup_cpu:
> + * @ms: MachineState used to initialize the topology structure on
> + *      first call.
> + * @cpu: the new S390CPU to insert in the topology structure
> + * @errp: the error pointer
> + *
> + * Called from CPU Hotplug to check and setup the CPU attributes
> + * before to insert the CPU in the topology.

... before the CPU is inserted in the topology.

> + * There is no use to update the MTCR explicitely here because it

... is no need ... sounds better.

> + * will be updated by KVM on creation of the new vCPU.

"CPU" is used everywhere else.

> + */
> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    /*
> +     * We do not want to initialize the topology if the cpu model
> +     * does not support topology, consequently, we have to wait for
> +     * the first CPU to be realized, which realizes the CPU model
> +     * to initialize the topology structures.
> +     *
> +     * s390_topology_setup_cpu() is called from the cpu hotplug.
> +     */
> +    if (!s390_topology.cores_per_socket) {
> +        s390_topology_init(ms);
> +    }
> +
> +    s390_topology_cpu_default(cpu, errp);
> +    if (*errp) {

May be having s390_topology_cpu_default() return a bool would be cleaner.
Same comment for the routines below. This is minor.

> +        return;
> +    }
> +
> +    s390_topology_check(cpu->env.socket_id, cpu->env.book_id,
> +                        cpu->env.drawer_id, cpu->env.entitlement,
> +                        cpu->env.dedicated, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    /* Set the CPU inside the socket */
> +    s390_topology_add_core_to_socket(cpu, 0, 0, 0, true, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    /* topology tree is reflected in props */
> +    s390_update_cpu_props(ms, cpu);
> +}
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1a9bcda8b6..9df60ac447 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -45,6 +45,7 @@
>   #include "hw/s390x/pv.h"
>   #include "migration/blocker.h"
>   #include "qapi/visitor.h"
> +#include "hw/s390x/cpu-topology.h"
>   
>   static Error *pv_mig_blocker;
>   
> @@ -311,10 +312,18 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>   {
>       MachineState *ms = MACHINE(hotplug_dev);
>       S390CPU *cpu = S390_CPU(dev);
> +    ERRP_GUARD();
>   
>       g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
>       ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
>   
> +    if (s390_has_topology()) {
> +        s390_topology_setup_cpu(ms, cpu, errp);
> +        if (*errp) {
> +            return;
> +        }
> +    }
> +
>       if (dev->hotplugged) {
>           raise_irq_cpu_hotplug();
>       }
> @@ -554,11 +563,20 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
>                                     sizeof(CPUArchId) * max_cpus);
>       ms->possible_cpus->len = max_cpus;
>       for (i = 0; i < ms->possible_cpus->len; i++) {
> +        CpuInstanceProperties *props = &ms->possible_cpus->cpus[i].props;
> +
>           ms->possible_cpus->cpus[i].type = ms->cpu_type;
>           ms->possible_cpus->cpus[i].vcpus_count = 1;
>           ms->possible_cpus->cpus[i].arch_id = i;
> -        ms->possible_cpus->cpus[i].props.has_core_id = true;
> -        ms->possible_cpus->cpus[i].props.core_id = i;
> +
> +        props->has_core_id = true;
> +        props->core_id = i;
> +        props->has_socket_id = true;
> +        props->socket_id = s390_std_socket(i, &ms->smp);
> +        props->has_book_id = true;
> +        props->book_id = s390_std_book(i, &ms->smp);
> +        props->has_drawer_id = true;
> +        props->drawer_id = s390_std_drawer(i, &ms->smp);
>       }
>   
>       return ms->possible_cpus;
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..58dfbdff4f 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -24,6 +24,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>     's390-stattrib-kvm.c',
>     'pv.c',
>     's390-pci-kvm.c',
> +  'cpu-topology.c',
>   ))
>   s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>     'tod-tcg.c',

On 4/4/23 09:31, Cédric Le Goater wrote:
> On 4/3/23 18:28, Pierre Morel wrote:
>>

[...]


>> +
>> +/**
>> + * s390_socket_nb:
>> + * @cpu: s390x CPU
>> + *
>> + * Returns the socket number used inside the cores_per_socket array
>> + * for a cpu.
>> + */
>> +int s390_socket_nb(S390CPU *cpu)
>
> s390_socket_nb() doesn't seem to be used anywhere else than in
> hw/s390x/cpu-topology.c. It should be static.


right

[...]
>> +/**
>> + * s390_topology_add_core_to_socket:
>> + * @cpu: the new S390CPU to insert in the topology structure
>> + * @drawer_id: new drawer_id
>> + * @book_id: new book_id
>> + * @socket_id: new socket_id
>> + * @creation: if is true the CPU is a new CPU and there is no old 
>> socket
>> + *            to handle.
>> + *            if is false, this is a moving the CPU and old socket 
>> count
>> + *            must be decremented.
>> + * @errp: the error pointer
>> + *
>> + */
>> +static void s390_topology_add_core_to_socket(S390CPU *cpu, int 
>> drawer_id,
>> +                                             int book_id, int 
>> socket_id,
>> +                                             bool creation, Error 
>> **errp)
>> +{
>
> Since this routine is called twice, in s390_topology_setup_cpu() for
> creation, and in s390_change_topology() for socket migration, we could
> duplicate the code in two distinct routines.
>
> I think this would simplify a bit each code path and avoid the 'creation'
> parameter which is confusing.


right

Thanks.


>
>
>> +    int old_socket_entry = s390_socket_nb(cpu);
>> +    int new_socket_entry;
>> +
>> +    if (creation) {
>> +        new_socket_entry = old_socket_entry;
>> +    } else {
>> +        new_socket_entry = (drawer_id * s390_topology.smp->books + 
>> book_id) *
>> +                            s390_topology.smp->sockets + socket_id;
>
> A helper common routine that s390_socket_nb() could use also would be 
> a plus.


Yes, thanks


>
>> +    }
>> +
>> +    /* Check for space on new socket */
>> +    if ((new_socket_entry != old_socket_entry) &&
>> +        (s390_topology.cores_per_socket[new_socket_entry] >=
>> +         s390_topology.smp->cores)) {
>> +        error_setg(errp, "No more space on this socket");
>> +        return;
>> +    }
>> +
>> +    /* Update the count of cores in sockets */
>> +    s390_topology.cores_per_socket[new_socket_entry] += 1;
>> +    if (!creation) {
>> +        s390_topology.cores_per_socket[old_socket_entry] -= 1;
>> +    }
>> +}
>> +
>> +/**
>> + * s390_update_cpu_props:
>> + * @ms: the machine state
>> + * @cpu: the CPU for which to update the properties from the 
>> environment.
>> + *
>> + */
>> +static void s390_update_cpu_props(MachineState *ms, S390CPU *cpu)
>> +{
>> +    CpuInstanceProperties *props;
>> +
>> +    props = &ms->possible_cpus->cpus[cpu->env.core_id].props;
>> +
>> +    props->socket_id = cpu->env.socket_id;
>> +    props->book_id = cpu->env.book_id;
>> +    props->drawer_id = cpu->env.drawer_id;
>> +}
>> +
>> +/**
>> + * s390_topology_setup_cpu:
>> + * @ms: MachineState used to initialize the topology structure on
>> + *      first call.
>> + * @cpu: the new S390CPU to insert in the topology structure
>> + * @errp: the error pointer
>> + *
>> + * Called from CPU Hotplug to check and setup the CPU attributes
>> + * before to insert the CPU in the topology.
>
> ... before the CPU is inserted in the topology.
OK
>
>> + * There is no use to update the MTCR explicitely here because it
>
> ... is no need ... sounds better.
OK
>
>> + * will be updated by KVM on creation of the new vCPU.
>
> "CPU" is used everywhere else.
OK
>
>> + */
>> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error 
>> **errp)
>> +{
>> +    ERRP_GUARD();
>> +
>> +    /*
>> +     * We do not want to initialize the topology if the cpu model
>> +     * does not support topology, consequently, we have to wait for
>> +     * the first CPU to be realized, which realizes the CPU model
>> +     * to initialize the topology structures.
>> +     *
>> +     * s390_topology_setup_cpu() is called from the cpu hotplug.
>> +     */
>> +    if (!s390_topology.cores_per_socket) {
>> +        s390_topology_init(ms);
>> +    }
>> +
>> +    s390_topology_cpu_default(cpu, errp);
>> +    if (*errp) {
>
> May be having s390_topology_cpu_default() return a bool would be cleaner.
> Same comment for the routines below. This is minor.


Yes and it is more readable. I do it.


Thanks for the comments.

Regards,

Pierre

On Tue, 2023-04-04 at 09:31 +0200, Cédric Le Goater wrote:
> On 4/3/23 18:28, Pierre Morel wrote:
> > The topology information are attributes of the CPU and are
> > specified during the CPU device creation.
> > 
> > On hot plug we:
> > - calculate the default values for the topology for drawers,
> >    books and sockets in the case they are not specified.
> > - verify the CPU attributes
> > - check that we have still room on the desired socket
> > 
> > The possibility to insert a CPU in a mask is dependent on the
> > number of cores allowed in a socket, a book or a drawer, the
> > checking is done during the hot plug of the CPU to have an
> > immediate answer.
> > 
> > If the complete topology is not specified, the core is added
> > in the physical topology based on its core ID and it gets
> > defaults values for the modifier attributes.
> > 
> > This way, starting QEMU without specifying the topology can
> > still get some advantage of the CPU topology.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   MAINTAINERS                        |   1 +
> >   include/hw/s390x/cpu-topology.h    |  44 +++++
> >   include/hw/s390x/s390-virtio-ccw.h |   1 +
> >   hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
> >   hw/s390x/s390-virtio-ccw.c         |  22 ++-
> >   hw/s390x/meson.build               |   1 +
> >   6 files changed, 349 insertions(+), 2 deletions(-)
> >   create mode 100644 hw/s390x/cpu-topology.c

[...]

> > +
> > +/**
> > + * s390_socket_nb:
> > + * @cpu: s390x CPU
> > + *
> > + * Returns the socket number used inside the cores_per_socket array
> > + * for a cpu.
> > + */
> > +int s390_socket_nb(S390CPU *cpu)
> 
> s390_socket_nb() doesn't seem to be used anywhere else than in
> hw/s390x/cpu-topology.c. It should be static.
> 
> 
> > +{
> > +    return (cpu->env.drawer_id * s390_topology.smp->books + cpu->env.book_id) *
> > +           s390_topology.smp->sockets + cpu->env.socket_id;
> > +}

[...]

> > +/**
> > + * s390_topology_add_core_to_socket:
> > + * @cpu: the new S390CPU to insert in the topology structure
> > + * @drawer_id: new drawer_id
> > + * @book_id: new book_id
> > + * @socket_id: new socket_id
> > + * @creation: if is true the CPU is a new CPU and there is no old socket
> > + *            to handle.
> > + *            if is false, this is a moving the CPU and old socket count
> > + *            must be decremented.
> > + * @errp: the error pointer
> > + *
> > + */
> > +static void s390_topology_add_core_to_socket(S390CPU *cpu, int drawer_id,
> > +                                             int book_id, int socket_id,
> > +                                             bool creation, Error **errp)
> > +{
> 
> Since this routine is called twice, in s390_topology_setup_cpu() for
> creation, and in s390_change_topology() for socket migration, we could
> duplicate the code in two distinct routines.
> 
> I think this would simplify a bit each code path and avoid the 'creation'
> parameter which is confusing.
> 
> 
> > +    int old_socket_entry = s390_socket_nb(cpu);
> > +    int new_socket_entry;
> > +
> > +    if (creation) {
> > +        new_socket_entry = old_socket_entry;
> > +    } else {
> > +        new_socket_entry = (drawer_id * s390_topology.smp->books + book_id) *
> > +                            s390_topology.smp->sockets + socket_id;
> 
> A helper common routine that s390_socket_nb() could use also would be a plus.

An alternative to consider would be to define a

struct topology_pos {
    int socket;
    int book;
    int drawer;
};

or similar so you can do

old_socket_entry = s390_socket_nb(cpu->env.topology_pos, smp);

struct topology_pos topology_pos = { socket_id, book_id, drawer_id };
new_socket_entry = s390_socket_nb(topology_pos, smp);

It might also make sense to pass a topology_pos around instead of three ids,
since that is quite common.

> 
> > +    }
> > +
> > +    /* Check for space on new socket */
> > +    if ((new_socket_entry != old_socket_entry) &&
> > +        (s390_topology.cores_per_socket[new_socket_entry] >=
> > +         s390_topology.smp->cores)) {
> > +        error_setg(errp, "No more space on this socket");
> > +        return;
> > +    }
> > +
> > +    /* Update the count of cores in sockets */
> > +    s390_topology.cores_per_socket[new_socket_entry] += 1;
> > +    if (!creation) {
> > +        s390_topology.cores_per_socket[old_socket_entry] -= 1;
> > +    }
> > +}

[...]

On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
> The topology information are attributes of the CPU and are
> specified during the CPU device creation.
> 
> On hot plug we:
> - calculate the default values for the topology for drawers,
>   books and sockets in the case they are not specified.
> - verify the CPU attributes
> - check that we have still room on the desired socket
> 
> The possibility to insert a CPU in a mask is dependent on the
> number of cores allowed in a socket, a book or a drawer, the
> checking is done during the hot plug of the CPU to have an
> immediate answer.
> 
> If the complete topology is not specified, the core is added
> in the physical topology based on its core ID and it gets
> defaults values for the modifier attributes.
> 
> This way, starting QEMU without specifying the topology can
> still get some advantage of the CPU topology.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  MAINTAINERS                        |   1 +
>  include/hw/s390x/cpu-topology.h    |  44 +++++
>  include/hw/s390x/s390-virtio-ccw.h |   1 +
>  hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c         |  22 ++-
>  hw/s390x/meson.build               |   1 +
>  6 files changed, 349 insertions(+), 2 deletions(-)
>  create mode 100644 hw/s390x/cpu-topology.c

[...]

>  #endif
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 9bba21a916..ea10a6c6e1 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>      bool dea_key_wrap;
>      bool pv;
>      uint8_t loadparm[8];
> +    bool vertical_polarization;

Why is this here and not in s390_topology?
This splits up the topology state somewhat.
I don't quite recall, did you use to have s390_topology in S390CcwMachineState at some point?
I think putting everything in S390CcwMachineState might make sense too.

>  };
>  
>  struct S390CcwMachineClass {
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..96da67bd7e
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c

[...]

> +/**
> + * s390_topology_cpu_default:
> + * @cpu: pointer to a S390CPU
> + * @errp: Error pointer
> + *
> + * Setup the default topology if no attributes are already set.
> + * Passing a CPU with some, but not all, attributes set is considered
> + * an error.
> + *
> + * The function calculates the (drawer_id, book_id, socket_id)
> + * topology by filling the cores starting from the first socket
> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
> + *
> + * CPU type and dedication have defaults values set in the
> + * s390x_cpu_properties, entitlement must be adjust depending on the
> + * dedication.
> + */
> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    CPUS390XState *env = &cpu->env;
> +
> +    /* All geometry topology attributes must be set or all unset */
> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
> +        error_setg(errp,
> +                   "Please define all or none of the topology geometry attributes");
> +        return;
> +    }
> +
> +    /* Check if one of the geometry topology is unset */
> +    if (env->socket_id < 0) {
> +        /* Calculate default geometry topology attributes */
> +        env->socket_id = s390_std_socket(env->core_id, smp);
> +        env->book_id = s390_std_book(env->core_id, smp);
> +        env->drawer_id = s390_std_drawer(env->core_id, smp);
> +    }
> +
> +    /*
> +     * Even the user can specify the entitlement as horizontal on the
> +     * command line, qemu will only use env->entitlement during vertical
> +     * polarization.
> +     * Medium entitlement is chosen as the default entitlement when the CPU
> +     * 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->dedicated) {
> +            env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
> +        } else {
> +            env->entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
> +        }
> +    }

As you know, in my opinion there should be not possibility for the user
to set the entitlement to horizontal and dedicated && != HIGH should be
rejected as an error.
If you do this, you can delete this.

> +}
> +
> +/**
> + * s390_topology_check:
> + * @socket_id: socket to check
> + * @book_id: book to check
> + * @drawer_id: drawer to check
> + * @entitlement: entitlement to check
> + * @dedicated: dedication to check
> + * @errp: Error pointer
> + *
> + * The function checks if the topology
> + * attributes fits inside the system topology.

fitS

The function checks the validity of the provided topology arguments,
namely that they're in bounds and non contradictory.

> + */
> +static void s390_topology_check(uint16_t socket_id, uint16_t book_id,

I'd prefer if you stick to one id type. There defined as int32_t in env,
here you use uint16_t and below int.

In env, you want a signed type with sufficient range, int16_t should suffice there,
but bigger is also fine.
You don't want the user to pass a negative id, so by using an unsigned type you
can avoid this without extra code.
But IMO there should be one point where a type conversion occurs.

> +                                uint16_t drawer_id, uint16_t entitlement,
> +                                bool dedicated, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    ERRP_GUARD();
> +
> +    if (socket_id >= smp->sockets) {
> +        error_setg(errp, "Unavailable socket: %d", socket_id);
> +        return;
> +    }
> +    if (book_id >= smp->books) {
> +        error_setg(errp, "Unavailable book: %d", book_id);
> +        return;
> +    }
> +    if (drawer_id >= smp->drawers) {
> +        error_setg(errp, "Unavailable drawer: %d", drawer_id);
> +        return;
> +    }
> +    if (entitlement >= S390_CPU_ENTITLEMENT__MAX) {
> +        error_setg(errp, "Unknown entitlement: %d", entitlement);
> +        return;
> +    }

I think you can delete this, too, there is no way that entitlement is > MAX.

> +    if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW ||
> +                      entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) {

Without HORIZONTAL you can do != HIGH and save one line, but that is
cosmetic only.

> +        error_setg(errp, "A dedicated cpu implies high entitlement");
> +        return;
> +    }
> +}

[...]

On 4/20/23 10:59, Nina Schoetterl-Glausch wrote:
> On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
>> The topology information are attributes of the CPU and are
>> specified during the CPU device creation.
>>
>> On hot plug we:
>> - calculate the default values for the topology for drawers,
>>    books and sockets in the case they are not specified.
>> - verify the CPU attributes
>> - check that we have still room on the desired socket
>>
>> The possibility to insert a CPU in a mask is dependent on the
>> number of cores allowed in a socket, a book or a drawer, the
>> checking is done during the hot plug of the CPU to have an
>> immediate answer.
>>
>> If the complete topology is not specified, the core is added
>> in the physical topology based on its core ID and it gets
>> defaults values for the modifier attributes.
>>
>> This way, starting QEMU without specifying the topology can
>> still get some advantage of the CPU topology.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   MAINTAINERS                        |   1 +
>>   include/hw/s390x/cpu-topology.h    |  44 +++++
>>   include/hw/s390x/s390-virtio-ccw.h |   1 +
>>   hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c         |  22 ++-
>>   hw/s390x/meson.build               |   1 +
>>   6 files changed, 349 insertions(+), 2 deletions(-)
>>   create mode 100644 hw/s390x/cpu-topology.c
> [...]
>
>>   #endif
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 9bba21a916..ea10a6c6e1 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>>       bool dea_key_wrap;
>>       bool pv;
>>       uint8_t loadparm[8];
>> +    bool vertical_polarization;
> Why is this here and not in s390_topology?
> This splits up the topology state somewhat.
> I don't quite recall, did you use to have s390_topology in S390CcwMachineState at some point?
> I think putting everything in S390CcwMachineState might make sense too.

Hum.

This is a left over from an abandoned try.

I put it back where it was, in s390_topology.


>
>>   };
>>   
>>   struct S390CcwMachineClass {
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..96da67bd7e
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
> [...]
>
>> +/**
>> + * s390_topology_cpu_default:
>> + * @cpu: pointer to a S390CPU
>> + * @errp: Error pointer
>> + *
>> + * Setup the default topology if no attributes are already set.
>> + * Passing a CPU with some, but not all, attributes set is considered
>> + * an error.
>> + *
>> + * The function calculates the (drawer_id, book_id, socket_id)
>> + * topology by filling the cores starting from the first socket
>> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
>> + *
>> + * CPU type and dedication have defaults values set in the
>> + * s390x_cpu_properties, entitlement must be adjust depending on the
>> + * dedication.
>> + */
>> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
>> +{
>> +    CpuTopology *smp = s390_topology.smp;
>> +    CPUS390XState *env = &cpu->env;
>> +
>> +    /* All geometry topology attributes must be set or all unset */
>> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
>> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
>> +        error_setg(errp,
>> +                   "Please define all or none of the topology geometry attributes");
>> +        return;
>> +    }
>> +
>> +    /* Check if one of the geometry topology is unset */
>> +    if (env->socket_id < 0) {
>> +        /* Calculate default geometry topology attributes */
>> +        env->socket_id = s390_std_socket(env->core_id, smp);
>> +        env->book_id = s390_std_book(env->core_id, smp);
>> +        env->drawer_id = s390_std_drawer(env->core_id, smp);
>> +    }
>> +
>> +    /*
>> +     * Even the user can specify the entitlement as horizontal on the
>> +     * command line, qemu will only use env->entitlement during vertical
>> +     * polarization.
>> +     * Medium entitlement is chosen as the default entitlement when the CPU
>> +     * 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->dedicated) {
>> +            env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
>> +        } else {
>> +            env->entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
>> +        }
>> +    }
> As you know, in my opinion there should be not possibility for the user
> to set the entitlement to horizontal and dedicated && != HIGH should be
> rejected as an error.
> If you do this, you can delete this.

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?


>
>> +}
>> +
>> +/**
>> + * s390_topology_check:
>> + * @socket_id: socket to check
>> + * @book_id: book to check
>> + * @drawer_id: drawer to check
>> + * @entitlement: entitlement to check
>> + * @dedicated: dedication to check
>> + * @errp: Error pointer
>> + *
>> + * The function checks if the topology
>> + * attributes fits inside the system topology.
> fitS
>
> The function checks the validity of the provided topology arguments,
> namely that they're in bounds and non contradictory.


OK, thanks.


>
>> + */
>> +static void s390_topology_check(uint16_t socket_id, uint16_t book_id,
> I'd prefer if you stick to one id type. There defined as int32_t in env,
> here you use uint16_t and below int.
>
> In env, you want a signed type with sufficient range, int16_t should suffice there,
> but bigger is also fine.
> You don't want the user to pass a negative id, so by using an unsigned type you
> can avoid this without extra code.
> But IMO there should be one point where a type conversion occurs.

OK


>
>> +                                uint16_t drawer_id, uint16_t entitlement,
>> +                                bool dedicated, Error **errp)
>> +{
>> +    CpuTopology *smp = s390_topology.smp;
>> +    ERRP_GUARD();
>> +
>> +    if (socket_id >= smp->sockets) {
>> +        error_setg(errp, "Unavailable socket: %d", socket_id);
>> +        return;
>> +    }
>> +    if (book_id >= smp->books) {
>> +        error_setg(errp, "Unavailable book: %d", book_id);
>> +        return;
>> +    }
>> +    if (drawer_id >= smp->drawers) {
>> +        error_setg(errp, "Unavailable drawer: %d", drawer_id);
>> +        return;
>> +    }
>> +    if (entitlement >= S390_CPU_ENTITLEMENT__MAX) {
>> +        error_setg(errp, "Unknown entitlement: %d", entitlement);
>> +        return;
>> +    }
> I think you can delete this, too, there is no way that entitlement is > MAX.

With entitlement being an enum in CPU properties yes.


>
>> +    if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW ||
>> +                      entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) {
> Without HORIZONTAL you can do != HIGH and save one line, but that is
> cosmetic only.

Right, HORIZONTAL is eliminated during s390_topology_cpu_default()


Regards,

Pierre

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:
> > > > > > The topology information are attributes of the CPU and are
> > > > > > specified during the CPU device creation.
> > > > > > 
> > > > > > On hot plug we:
> > > > > > - calculate the default values for the topology for drawers,
> > > > > >    books and sockets in the case they are not specified.
> > > > > > - verify the CPU attributes
> > > > > > - check that we have still room on the desired socket
> > > > > > 
> > > > > > The possibility to insert a CPU in a mask is dependent on the
> > > > > > number of cores allowed in a socket, a book or a drawer, the
> > > > > > checking is done during the hot plug of the CPU to have an
> > > > > > immediate answer.
> > > > > > 
> > > > > > If the complete topology is not specified, the core is added
> > > > > > in the physical topology based on its core ID and it gets
> > > > > > defaults values for the modifier attributes.
> > > > > > 
> > > > > > This way, starting QEMU without specifying the topology can
> > > > > > still get some advantage of the CPU topology.
> > > > > > 
> > > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > > > ---
> > > > > >   MAINTAINERS                        |   1 +
> > > > > >   include/hw/s390x/cpu-topology.h    |  44 +++++
> > > > > >   include/hw/s390x/s390-virtio-ccw.h |   1 +
> > > > > >   hw/s390x/cpu-topology.c            | 282 +++++++++++++++++++++++++++++
> > > > > >   hw/s390x/s390-virtio-ccw.c         |  22 ++-
> > > > > >   hw/s390x/meson.build               |   1 +
> > > > > >   6 files changed, 349 insertions(+), 2 deletions(-)
> > > > > >   create mode 100644 hw/s390x/cpu-topology.c
> > > > > >   };

[...]

> > > > > >   struct S390CcwMachineClass {
> > > > > > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > > > > > new file mode 100644
> > > > > > index 0000000000..96da67bd7e
> > > > > > --- /dev/null
> > > > > > +++ b/hw/s390x/cpu-topology.c
> > > > [...]
> > > > 
> > > > > > +/**
> > > > > > + * s390_topology_cpu_default:
> > > > > > + * @cpu: pointer to a S390CPU
> > > > > > + * @errp: Error pointer
> > > > > > + *
> > > > > > + * Setup the default topology if no attributes are already set.
> > > > > > + * Passing a CPU with some, but not all, attributes set is considered
> > > > > > + * an error.
> > > > > > + *
> > > > > > + * The function calculates the (drawer_id, book_id, socket_id)
> > > > > > + * topology by filling the cores starting from the first socket
> > > > > > + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
> > > > > > + *
> > > > > > + * CPU type and dedication have defaults values set in the
> > > > > > + * s390x_cpu_properties, entitlement must be adjust depending on the
> > > > > > + * dedication.
> > > > > > + */
> > > > > > +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
> > > > > > +{
> > > > > > +    CpuTopology *smp = s390_topology.smp;
> > > > > > +    CPUS390XState *env = &cpu->env;
> > > > > > +
> > > > > > +    /* All geometry topology attributes must be set or all unset */
> > > > > > +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
> > > > > > +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
> > > > > > +        error_setg(errp,
> > > > > > +                   "Please define all or none of the topology geometry attributes");
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Check if one of the geometry topology is unset */
> > > > > > +    if (env->socket_id < 0) {
> > > > > > +        /* Calculate default geometry topology attributes */
> > > > > > +        env->socket_id = s390_std_socket(env->core_id, smp);
> > > > > > +        env->book_id = s390_std_book(env->core_id, smp);
> > > > > > +        env->drawer_id = s390_std_drawer(env->core_id, smp);
> > > > > > +    }
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Even the user can specify the entitlement as horizontal on the
> > > > > > +     * command line, qemu will only use env->entitlement during vertical
> > > > > > +     * polarization.
> > > > > > +     * Medium entitlement is chosen as the default entitlement when the CPU
> > > > > > +     * 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->dedicated) {
> > > > > > +            env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
> > > > > > +        } else {
> > > > > > +            env->entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
> > > > > > +        }
> > > > > > +    }
> > > > As you know, in my opinion there should be not possibility for the user
> > > > to set the entitlement to horizontal and dedicated && != HIGH should be
> > > > rejected as an error.
> > > > If you do this, you can delete this.
> > 
> > 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",
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:

+static void s390_change_topology(uint16_t core_id,
+ bool has_socket_id, uint16_t socket_id,
+ bool has_book_id, uint16_t book_id,
+ bool has_drawer_id, uint16_t drawer_id,
+ bool has_entitlement, uint16_t entitlement,
+ bool has_dedicated, bool dedicated,
+ Error **errp)
+{

[...]

+ /*
+ * Even user can specify the entitlement as horizontal on the command line,
+ * qemu will only use entitlement during vertical polarization.
+ * Medium entitlement is chosen as the default entitlement when the CPU
+ * is not dedicated.
+ * A dedicated CPU always receives a high entitlement.
+ */
+ if (!has_entitlement || (entitlement == S390_CPU_ENTITLEMENT_HORIZONTAL)) {
+ if (dedicated) {
+ entitlement = S390_CPU_ENTITLEMENT_HIGH;
+ } else {
+ entitlement = S390_CPU_ENTITLEMENT_MEDIUM;
+ }
+ }

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

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 */
     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) {
         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;
     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),
 #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".

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.

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.

On 4/25/23 11:27, Nina Schoetterl-Glausch wrote:
> 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:
[..]
>>>    #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.


This value is only usable but not required on input and is never 
displayed by the qapi.


> Then the only problem is adjusting the entitlement when doing dedicated=on on the command line.
> (If you want that)

Even the exact usage of dedication depends on the administration entity 
it will give the guest the
knowledge of something like: "A real CPU is dedicated to this vCPU".

The people using the qapi interface or using QEMU hotplug can easily 
understand
the concept without going deeper with entitlement which once implemented 
will be,
quite more complex to deal with.


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


I did not have the problem by using 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.

Thank you to accept the solution 3, I understand your objections, but,
I really think that there is no need to bother the user with entitlement
while it will not get used in QEMU/KVM until the Linux scheduler is modified
for the host and for the guest to handle it.


Regards,

Pierre