On 4/18/23 14:38, Nina Schoetterl-Glausch wrote:
On Tue, 2023-04-18 at 12:01 +0200, Pierre Morel wrote:
On 4/18/23 10:53, Nina Schoetterl-Glausch wrote:
On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
S390 adds two new SMP levels, drawers and books to the CPU
topology.
The S390 CPU have specific topology features like dedication
and entitlement to give to the guest indications on the host
vCPUs scheduling and help the guest take the best decisions
on the scheduling of threads on the vCPUs.
Let us provide the SMP properties with books and drawers levels
and S390 CPU with dedication and entitlement,
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
---
[...]
diff --git a/qapi/machine-common.json b/qapi/machine-common.json
new file mode 100644
index 0000000000..73ea38d976
--- /dev/null
+++ b/qapi/machine-common.json
@@ -0,0 +1,22 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+##
+# = Machines S390 data types
+##
+
+##
+# @CpuS390Entitlement:
+#
+# An enumeration of cpu entitlements that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 8.1
+##
+{ 'enum': 'CpuS390Entitlement',
+ 'prefix': 'S390_CPU_ENTITLEMENT',
+ 'data': [ 'horizontal', 'low', 'medium', 'high' ] }
You can get rid of the horizontal value now that the entitlement is ignored if the
polarization is vertical.
Right, horizontal is not used, but what would you like?
- replace horizontal with 'none' ?
- add or substract 1 when we do the conversion between enum string and
value ?
Yeah, I would completely drop it because it is a meaningless value
and adjust the conversion to the cpu value accordingly.
frankly I prefer to keep horizontal here which is exactly what is given
in the documentation for entitlement = 0
Not sure what you mean with this.
I mean: Extract from the PoP:
----
The following values are used:
PP Meaning
0 The one or more CPUs represented by the TLE are
horizontally polarized.
1 The one or more CPUs represented by the TLE are
vertically polarized. Entitlement is low.
2 The one or more CPUs represented by the TLE are
vertically polarized. Entitlement is medium.
3 The one or more CPUs represented by the TLE are
vertically polarized. Entitlement is high.
----
Also I find that using an enum to systematically add/subtract a value is
for me weird.
so I really prefer to keep "horizontal", "low", "medium", "high" event
"horizontal" will never appear.
A mater of taste, it does not change anything to the functionality or
the API.
[...]
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index b10a8541ff..57165fa3a0 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -37,6 +37,7 @@
#ifndef CONFIG_USER_ONLY
#include "sysemu/reset.h"
#endif
+#include "hw/s390x/cpu-topology.h"
#define CR0_RESET 0xE0UL
#define CR14_RESET 0xC2000000UL;
@@ -259,6 +260,12 @@ static gchar *s390_gdb_arch_name(CPUState *cs)
static Property s390x_cpu_properties[] = {
#if !defined(CONFIG_USER_ONLY)
DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0),
+ DEFINE_PROP_INT32("socket-id", S390CPU, env.socket_id, -1),
+ DEFINE_PROP_INT32("book-id", S390CPU, env.book_id, -1),
+ DEFINE_PROP_INT32("drawer-id", S390CPU, env.drawer_id, -1),
+ DEFINE_PROP_BOOL("dedicated", S390CPU, env.dedicated, false),
+ DEFINE_PROP_UINT8("entitlement", S390CPU, env.entitlement,
+ S390_CPU_ENTITLEMENT__MAX),
I would define an entitlement PropertyInfo in qdev-properties-system.[ch],
then one can use e.g.
-device z14-s390x-cpu,core-id=11,entitlement=high
Don't you think it is an enhancement we can do later?
It's a user visible change, so no.
We could have kept both string and integer.
But it's not complicated, should be just:
const PropertyInfo qdev_prop_cpus390entitlement = {
.name = "CpuS390Entitlement",
.enum_table = &CpuS390Entitlement_lookup,
.get = qdev_propinfo_get_enum,
.set = qdev_propinfo_set_enum,
.set_default_value = qdev_propinfo_set_default_value_enum,
};
Plus a comment & build bug in qdev-properties-system.c
and
extern const PropertyInfo qdev_prop_cpus390entitlement;
#define DEFINE_PROP_CPUS390ENTITLEMENT(_n, _s, _f, _d) \
DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_cpus390entitlement, \
CpuS390Entitlement)
in qdev-properties-system.h
You need to change the type of env.entitlement and set the default to 1 for medium
and that should be it.
OK, it does not change anything to the functionality but is a little bit
more pretty.
on the command line and cpu hotplug.
I think setting the default entitlement to medium here should be fine.
[...]
right, I had medium before and should not have change it.
Anyway what ever the default is, it must be changed later depending on
dedication.
No, you can just set it to medium and get rid of the adjustment code.
s390_topology_check will reject invalid changes and the default above
is fine since dedication is false.
I do not want a default specification for the entitlement to depend on
the polarization.
If we do as you propose, by horizontal polarization a default
entitlement with dedication will be accepted but will be refused after
the guest switched for vertical polarization.
So we need adjustment before the check in both cases.
I find it easier and more logical if there is no default value than to
have a default we need to overwrite.
