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

[Nina Schoetterl-Glausch]

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