On 4/27/23 10:04, Thomas Huth wrote:
On 25/04/2023 18.14, 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>
---
...> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 2e267fa458..42a6a40333 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -342,3 +342,15 @@
'TARGET_S390X',
'TARGET_MIPS',
'TARGET_LOONGARCH64' ] } }
+
+##
+# @CpuS390Polarization:
+#
+# An enumeration of cpu polarization that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 8.1
+##
+{ 'enum': 'CpuS390Polarization',
+ 'prefix': 'S390_CPU_POLARIZATION',
+ 'data': [ 'horizontal', 'vertical' ] }
It seems like you don't need this here yet ... I think you likely
could also introduce it in a later patch instead (patch 11 seems the
first one that needs this?)
Also, would a " 'if': 'TARGET_S390X' " be possible here, too?
Yes, I will shift this to patch 11.
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index c3dab007da..77bee06304 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -30,8 +30,19 @@ static char *cpu_hierarchy_to_string(MachineState
*ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
GString *s = g_string_new(NULL);
+ const char *multiply = " * ", *prefix = "";
- g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
+ if (mc->smp_props.drawers_supported) {
+ g_string_append_printf(s, "drawers (%u)", ms->smp.drawers);
+ prefix = multiply;
That "prefix" stuff looks complicated ... why don't you simply add the
"*" at the end of the strings:
g_string_append_printf(s, "drawers (%u) * ",
ms->smp.drawers);
?
Right, I did not think enough when I made this change.
+ }
+
+ if (mc->smp_props.books_supported) {
+ g_string_append_printf(s, "%sbooks (%u)", prefix,
ms->smp.books);
+ prefix = multiply;
+ }
+
+ g_string_append_printf(s, "%ssockets (%u)", prefix,
ms->smp.sockets);
... it's followed by "sockets" here anyway, so adding the " * " at the
end of the strings above should be fine.
yes
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
unsigned cpus = config->has_cpus ? config->cpus : 0;
+ unsigned drawers = config->has_drawers ? config->drawers : 0;
+ unsigned books = config->has_books ? config->books : 0;
unsigned sockets = config->has_sockets ? config->sockets : 0;
unsigned dies = config->has_dies ? config->dies : 0;
unsigned clusters = config->has_clusters ? config->clusters : 0;
@@ -85,6 +98,8 @@ void machine_parse_smp_config(MachineState *ms,
* explicit configuration like "cpus=0" is not allowed.
*/
if ((config->has_cpus && config->cpus == 0) ||
+ (config->has_drawers && config->drawers == 0) ||
+ (config->has_books && config->books == 0) ||
(config->has_sockets && config->sockets == 0) ||
(config->has_dies && config->dies == 0) ||
(config->has_clusters && config->clusters == 0) ||
@@ -111,6 +126,19 @@ void machine_parse_smp_config(MachineState *ms,
dies = dies > 0 ? dies : 1;
clusters = clusters > 0 ? clusters : 1;
+ if (!mc->smp_props.books_supported && books > 1) {
+ error_setg(errp, "books not supported by this machine's CPU
topology");
+ return;
+ }
+ books = books > 0 ? books : 1;
Could be shortened to: book = books ?: 1;
I always forgot that Elvis is not dead
+ if (!mc->smp_props.drawers_supported && drawers > 1) {
+ error_setg(errp,
+ "drawers not supported by this machine's CPU
topology");
+ return;
+ }
+ drawers = drawers > 0 ? drawers : 1;
Could be shortened to: drawers = drawers ?: 1;
yes
/* compute missing values based on the provided ones */
if (cpus == 0 && maxcpus == 0) {
sockets = sockets > 0 ? sockets : 1;
@@ -124,33 +152,41 @@ void machine_parse_smp_config(MachineState *ms,
if (sockets == 0) {
cores = cores > 0 ? cores : 1;
threads = threads > 0 ? threads : 1;
- sockets = maxcpus / (dies * clusters * cores *
threads);
+ sockets = maxcpus /
+ (drawers * books * dies * clusters * cores
* threads);
} else if (cores == 0) {
threads = threads > 0 ? threads : 1;
- cores = maxcpus / (sockets * dies * clusters *
threads);
+ cores = maxcpus /
+ (drawers * books * sockets * dies * clusters
* threads);
}
(not very important, but I wonder whether we should maybe disallow
"prefer_sockets" with drawers and books instead of updating the
calculation here - since prefer_sockets should only occur on old
machine types)
It is OK for me. The contra argument would be that for balancing it is
nicer.
But as you like, it makes less code and we can add it later if we ever
set prefer_socket = true again, which is very uncertain.
} else {
/* prefer cores over sockets since 6.2 */
if (cores == 0) {
sockets = sockets > 0 ? sockets : 1;
threads = threads > 0 ? threads : 1;
- cores = maxcpus / (sockets * dies * clusters *
threads);
+ cores = maxcpus /
+ (drawers * books * sockets * dies * clusters
* threads);
} else if (sockets == 0) {
threads = threads > 0 ? threads : 1;
- sockets = maxcpus / (dies * clusters * cores *
threads);
+ sockets = maxcpus /
+ (drawers * books * dies * clusters * cores
* threads);
}
}
Thomas
Thanks Thomas.
Regards,
Pierre
