| Message ID | 20230403162905.17703-2-pmorel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: CPU Topology | expand |
On 4/3/23 18:28, 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@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> Some minor comments below, > --- > MAINTAINERS | 5 ++++ > qapi/machine-common.json | 22 ++++++++++++++ > qapi/machine-target.json | 12 ++++++++ > qapi/machine.json | 17 +++++++++-- > include/hw/boards.h | 10 ++++++- > include/hw/s390x/cpu-topology.h | 15 ++++++++++ > target/s390x/cpu.h | 5 ++++ > hw/core/machine-smp.c | 53 ++++++++++++++++++++++++++++----- > hw/core/machine.c | 4 +++ > hw/s390x/s390-virtio-ccw.c | 2 ++ > softmmu/vl.c | 6 ++++ > target/s390x/cpu.c | 7 +++++ > qapi/meson.build | 1 + > qemu-options.hx | 7 +++-- > 14 files changed, 152 insertions(+), 14 deletions(-) > create mode 100644 qapi/machine-common.json > create mode 100644 include/hw/s390x/cpu-topology.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5340de0515..9b1f80739e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1654,6 +1654,11 @@ F: hw/s390x/event-facility.c > F: hw/s390x/sclp*.c > L: qemu-s390x@nongnu.org > > +S390 CPU topology > +M: Pierre Morel <pmorel@linux.ibm.com> > +S: Supported > +F: include/hw/s390x/cpu-topology.h > + > X86 Machines > ------------ > PC > 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' ] } > + > 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' ] } > diff --git a/qapi/machine.json b/qapi/machine.json > index 604b686e59..1cdd83f3fd 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -9,6 +9,7 @@ > ## > > { 'include': 'common.json' } > +{ 'include': 'machine-common.json' } > > ## > # @SysEmuTarget: > @@ -70,7 +71,7 @@ > # > # @thread-id: ID of the underlying host thread > # > -# @props: properties describing to which node/socket/core/thread > +# @props: properties describing to which node/drawer/book/socket/core/thread > # virtual CPU belongs to, provided if supported by board > # > # @target: the QEMU system emulation target, which determines which > @@ -902,13 +903,15 @@ > # a CPU is being hotplugged. > # > # @node-id: NUMA node ID the CPU belongs to > -# @socket-id: socket number within node/board the CPU belongs to > +# @drawer-id: drawer number within node/board the CPU belongs to (since 8.1) > +# @book-id: book number within drawer/node/board the CPU belongs to (since 8.1) > +# @socket-id: socket number within book/node/board the CPU belongs to > # @die-id: die number within socket the CPU belongs to (since 4.1) > # @cluster-id: cluster number within die the CPU belongs to (since 7.1) > # @core-id: core number within cluster the CPU belongs to > # @thread-id: thread number within core the CPU belongs to > # > -# Note: currently there are 6 properties that could be present > +# Note: currently there are 8 properties that could be present > # but management should be prepared to pass through other > # properties with device_add command to allow for future > # interface extension. This also requires the filed names to be kept in > @@ -918,6 +921,8 @@ > ## > { 'struct': 'CpuInstanceProperties', > 'data': { '*node-id': 'int', > + '*drawer-id': 'int', > + '*book-id': 'int', > '*socket-id': 'int', > '*die-id': 'int', > '*cluster-id': 'int', > @@ -1467,6 +1472,10 @@ > # > # @cpus: number of virtual CPUs in the virtual machine > # > +# @drawers: number of drawers in the CPU topology (since 8.1) > +# > +# @books: number of books in the CPU topology (since 8.1) > +# > # @sockets: number of sockets in the CPU topology > # > # @dies: number of dies per socket in the CPU topology > @@ -1483,6 +1492,8 @@ > ## > { 'struct': 'SMPConfiguration', 'data': { > '*cpus': 'int', > + '*drawers': 'int', > + '*books': 'int', > '*sockets': 'int', > '*dies': 'int', > '*clusters': 'int', > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 6fbbfd56c8..9ef0bb76cf 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -131,12 +131,16 @@ typedef struct { > * @clusters_supported - whether clusters are supported by the machine > * @has_clusters - whether clusters are explicitly specified in the user > * provided SMP configuration > + * @books_supported - whether books are supported by the machine > + * @drawers_supported - whether drawers are supported by the machine > */ > typedef struct { > bool prefer_sockets; > bool dies_supported; > bool clusters_supported; > bool has_clusters; > + bool books_supported; > + bool drawers_supported; > } SMPCompatProps; > > /** > @@ -301,7 +305,9 @@ typedef struct DeviceMemoryState { > /** > * CpuTopology: > * @cpus: the number of present logical processors on the machine > - * @sockets: the number of sockets on the machine > + * @drawers: the number of drawers on the machine > + * @books: the number of books in one drawer > + * @sockets: the number of sockets in one book > * @dies: the number of dies in one socket > * @clusters: the number of clusters in one die > * @cores: the number of cores in one cluster > @@ -310,6 +316,8 @@ typedef struct DeviceMemoryState { > */ > typedef struct CpuTopology { > unsigned int cpus; > + unsigned int drawers; > + unsigned int books; > unsigned int sockets; > unsigned int dies; > unsigned int clusters; > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h > new file mode 100644 > index 0000000000..83f31604cc > --- /dev/null > +++ b/include/hw/s390x/cpu-topology.h > @@ -0,0 +1,15 @@ > +/* > + * CPU Topology > + * > + * Copyright IBM Corp. 2022 Shouldn't we have some range : 2022-2023 ? > + * > + * 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. > + */ QEMU uses a SPDX tag like the kernel now : /* SPDX-License-Identifier: GPL-2.0-or-later */ > +#ifndef HW_S390X_CPU_TOPOLOGY_H > +#define HW_S390X_CPU_TOPOLOGY_H > + > +#define S390_TOPOLOGY_CPU_IFL 0x03 This definition is only used in patch 3. May be introduce it then, it would cleaner. > + > +#endif > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 7d6d01325b..f2b2a38fe7 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -131,6 +131,11 @@ struct CPUArchState { > > #if !defined(CONFIG_USER_ONLY) > uint32_t core_id; /* PoP "CPU address", same as cpu_index */ > + int32_t socket_id; > + int32_t book_id; > + int32_t drawer_id; > + bool dedicated; > + uint8_t entitlement; /* Used only for vertical polarization */ > uint64_t cpuid; > #endif > > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c > index c3dab007da..6f5622a024 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; indent issue. > + } > + > + if (mc->smp_props.books_supported) { > + g_string_append_printf(s, "%sbooks (%u)", prefix, ms->smp.books); > + prefix = multiply; ditto. > + } > + > + g_string_append_printf(s, "%ssockets (%u)", prefix, ms->smp.sockets); > > if (mc->smp_props.dies_supported) { > g_string_append_printf(s, " * dies (%u)", ms->smp.dies); > @@ -73,6 +84,8 @@ void machine_parse_smp_config(MachineState *ms, > { > 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; > + > + 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; > + > /* 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); > } > } 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); > } > } > > /* try to calculate omitted threads at last */ > if (threads == 0) { > - threads = maxcpus / (sockets * dies * clusters * cores); > + threads = maxcpus / > + (drawers * books * sockets * dies * clusters * cores); > } > } > > - maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * clusters * cores * threads; > + maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies * > + clusters * cores * threads; > cpus = cpus > 0 ? cpus : maxcpus; > > ms->smp.cpus = cpus; > + ms->smp.drawers = drawers; > + ms->smp.books = books; > ms->smp.sockets = sockets; > ms->smp.dies = dies; > ms->smp.clusters = clusters; > @@ -161,7 +197,8 @@ void machine_parse_smp_config(MachineState *ms, > mc->smp_props.has_clusters = config->has_clusters; > > /* sanity-check of the computed topology */ > - if (sockets * dies * clusters * cores * threads != maxcpus) { > + if (drawers * books * sockets * dies * clusters * cores * threads != > + maxcpus) { > g_autofree char *topo_msg = cpu_hierarchy_to_string(ms); > error_setg(errp, "Invalid CPU topology: " > "product of the hierarchy must match maxcpus: " > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 1cf6822e06..41c7ba7027 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -831,6 +831,8 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name, > MachineState *ms = MACHINE(obj); > SMPConfiguration *config = &(SMPConfiguration){ > .has_cpus = true, .cpus = ms->smp.cpus, > + .has_drawers = true, .drawers = ms->smp.drawers, > + .has_books = true, .books = ms->smp.books, > .has_sockets = true, .sockets = ms->smp.sockets, > .has_dies = true, .dies = ms->smp.dies, > .has_clusters = true, .clusters = ms->smp.clusters, > @@ -1096,6 +1098,8 @@ static void machine_initfn(Object *obj) > /* default to mc->default_cpus */ > ms->smp.cpus = mc->default_cpus; > ms->smp.max_cpus = mc->default_cpus; > + ms->smp.drawers = 1; > + ms->smp.books = 1; > ms->smp.sockets = 1; > ms->smp.dies = 1; > ms->smp.clusters = 1; > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 503f212a31..1a9bcda8b6 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -736,6 +736,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) > mc->no_sdcard = 1; > mc->max_cpus = S390_MAX_CPUS; > mc->has_hotpluggable_cpus = true; > + mc->smp_props.books_supported = true; > + mc->smp_props.drawers_supported = true; > assert(!mc->get_hotplug_handler); > mc->get_hotplug_handler = s390_get_hotplug_handler; > mc->cpu_index_to_instance_props = s390_cpu_index_to_props; > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 3340f63c37..bc293f8100 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -724,6 +724,12 @@ static QemuOptsList qemu_smp_opts = { > { > .name = "cpus", > .type = QEMU_OPT_NUMBER, > + }, { > + .name = "drawers", > + .type = QEMU_OPT_NUMBER, > + }, { > + .name = "books", > + .type = QEMU_OPT_NUMBER, > }, { > .name = "sockets", > .type = QEMU_OPT_NUMBER, > 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), > #endif > DEFINE_PROP_END_OF_LIST() > }; > diff --git a/qapi/meson.build b/qapi/meson.build > index fbdb442fdf..b5b01fb7b5 100644 > --- a/qapi/meson.build > +++ b/qapi/meson.build > @@ -35,6 +35,7 @@ qapi_all_modules = [ > 'error', > 'introspect', > 'job', > + 'machine-common', > 'machine', > 'machine-target', > 'migration', > diff --git a/qemu-options.hx b/qemu-options.hx > index d42f60fb91..4c79e153fb 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -250,11 +250,14 @@ SRST > ERST > > DEF("smp", HAS_ARG, QEMU_OPTION_smp, > - "-smp [[cpus=]n][,maxcpus=maxcpus][,sockets=sockets][,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]\n" > + "-smp [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets]\n" > + " [,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]\n" > " set the number of initial CPUs to 'n' [default=1]\n" > " maxcpus= maximum number of total CPUs, including\n" > " offline CPUs for hotplug, etc\n" > - " sockets= number of sockets on the machine board\n" > + " drawers= number of drawers on the machine board\n" > + " books= number of books in one drawer\n" > + " sockets= number of sockets in one book\n" > " dies= number of dies in one socket\n" > " clusters= number of clusters in one die\n" > " cores= number of cores in one cluster\n"
On 4/4/23 09:03, Cédric Le Goater wrote: > On 4/3/23 18:28, 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@linux.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> > > Some minor comments below, > >> --- >> MAINTAINERS | 5 ++++ >> qapi/machine-common.json | 22 ++++++++++++++ >> qapi/machine-target.json | 12 ++++++++ >> qapi/machine.json | 17 +++++++++-- >> include/hw/boards.h | 10 ++++++- >> include/hw/s390x/cpu-topology.h | 15 ++++++++++ >> target/s390x/cpu.h | 5 ++++ >> hw/core/machine-smp.c | 53 ++++++++++++++++++++++++++++----- >> hw/core/machine.c | 4 +++ >> hw/s390x/s390-virtio-ccw.c | 2 ++ >> softmmu/vl.c | 6 ++++ >> target/s390x/cpu.c | 7 +++++ >> qapi/meson.build | 1 + >> qemu-options.hx | 7 +++-- >> 14 files changed, 152 insertions(+), 14 deletions(-) >> create mode 100644 qapi/machine-common.json >> create mode 100644 include/hw/s390x/cpu-topology.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 5340de0515..9b1f80739e 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -1654,6 +1654,11 @@ F: hw/s390x/event-facility.c >> F: hw/s390x/sclp*.c >> L: qemu-s390x@nongnu.org >> +S390 CPU topology >> +M: Pierre Morel <pmorel@linux.ibm.com> >> +S: Supported >> +F: include/hw/s390x/cpu-topology.h >> + >> X86 Machines >> ------------ >> PC >> 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' ] } >> + >> 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' ] } >> diff --git a/qapi/machine.json b/qapi/machine.json >> index 604b686e59..1cdd83f3fd 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -9,6 +9,7 @@ >> ## >> { 'include': 'common.json' } >> +{ 'include': 'machine-common.json' } >> ## >> # @SysEmuTarget: >> @@ -70,7 +71,7 @@ >> # >> # @thread-id: ID of the underlying host thread >> # >> -# @props: properties describing to which node/socket/core/thread >> +# @props: properties describing to which >> node/drawer/book/socket/core/thread >> # virtual CPU belongs to, provided if supported by board >> # >> # @target: the QEMU system emulation target, which determines which >> @@ -902,13 +903,15 @@ >> # a CPU is being hotplugged. >> # >> # @node-id: NUMA node ID the CPU belongs to >> -# @socket-id: socket number within node/board the CPU belongs to >> +# @drawer-id: drawer number within node/board the CPU belongs to >> (since 8.1) >> +# @book-id: book number within drawer/node/board the CPU belongs to >> (since 8.1) >> +# @socket-id: socket number within book/node/board the CPU belongs to >> # @die-id: die number within socket the CPU belongs to (since 4.1) >> # @cluster-id: cluster number within die the CPU belongs to (since >> 7.1) >> # @core-id: core number within cluster the CPU belongs to >> # @thread-id: thread number within core the CPU belongs to >> # >> -# Note: currently there are 6 properties that could be present >> +# Note: currently there are 8 properties that could be present >> # but management should be prepared to pass through other >> # properties with device_add command to allow for future >> # interface extension. This also requires the filed names to >> be kept in >> @@ -918,6 +921,8 @@ >> ## >> { 'struct': 'CpuInstanceProperties', >> 'data': { '*node-id': 'int', >> + '*drawer-id': 'int', >> + '*book-id': 'int', >> '*socket-id': 'int', >> '*die-id': 'int', >> '*cluster-id': 'int', >> @@ -1467,6 +1472,10 @@ >> # >> # @cpus: number of virtual CPUs in the virtual machine >> # >> +# @drawers: number of drawers in the CPU topology (since 8.1) >> +# >> +# @books: number of books in the CPU topology (since 8.1) >> +# >> # @sockets: number of sockets in the CPU topology >> # >> # @dies: number of dies per socket in the CPU topology >> @@ -1483,6 +1492,8 @@ >> ## >> { 'struct': 'SMPConfiguration', 'data': { >> '*cpus': 'int', >> + '*drawers': 'int', >> + '*books': 'int', >> '*sockets': 'int', >> '*dies': 'int', >> '*clusters': 'int', >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 6fbbfd56c8..9ef0bb76cf 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -131,12 +131,16 @@ typedef struct { >> * @clusters_supported - whether clusters are supported by the machine >> * @has_clusters - whether clusters are explicitly specified in the >> user >> * provided SMP configuration >> + * @books_supported - whether books are supported by the machine >> + * @drawers_supported - whether drawers are supported by the machine >> */ >> typedef struct { >> bool prefer_sockets; >> bool dies_supported; >> bool clusters_supported; >> bool has_clusters; >> + bool books_supported; >> + bool drawers_supported; >> } SMPCompatProps; >> /** >> @@ -301,7 +305,9 @@ typedef struct DeviceMemoryState { >> /** >> * CpuTopology: >> * @cpus: the number of present logical processors on the machine >> - * @sockets: the number of sockets on the machine >> + * @drawers: the number of drawers on the machine >> + * @books: the number of books in one drawer >> + * @sockets: the number of sockets in one book >> * @dies: the number of dies in one socket >> * @clusters: the number of clusters in one die >> * @cores: the number of cores in one cluster >> @@ -310,6 +316,8 @@ typedef struct DeviceMemoryState { >> */ >> typedef struct CpuTopology { >> unsigned int cpus; >> + unsigned int drawers; >> + unsigned int books; >> unsigned int sockets; >> unsigned int dies; >> unsigned int clusters; >> diff --git a/include/hw/s390x/cpu-topology.h >> b/include/hw/s390x/cpu-topology.h >> new file mode 100644 >> index 0000000000..83f31604cc >> --- /dev/null >> +++ b/include/hw/s390x/cpu-topology.h >> @@ -0,0 +1,15 @@ >> +/* >> + * CPU Topology >> + * >> + * Copyright IBM Corp. 2022 > > Shouldn't we have some range : 2022-2023 ? There was a discussion on this in the first spins, I think to remember that Nina wanted 22 and Thomas 23, now we have a third opinion :) . I must say that all three have their reasons and I take what the majority wants. A vote? > >> + * >> + * 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. >> + */ > > QEMU uses a SPDX tag like the kernel now : > > /* SPDX-License-Identifier: GPL-2.0-or-later */ OK, so I will add it on all .c and .h new files > >> +#ifndef HW_S390X_CPU_TOPOLOGY_H >> +#define HW_S390X_CPU_TOPOLOGY_H >> + >> +#define S390_TOPOLOGY_CPU_IFL 0x03 > > This definition is only used in patch 3. May be introduce it then, > it would cleaner. yes. > >> + >> +#endif >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index 7d6d01325b..f2b2a38fe7 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -131,6 +131,11 @@ struct CPUArchState { >> #if !defined(CONFIG_USER_ONLY) >> uint32_t core_id; /* PoP "CPU address", same as cpu_index */ >> + int32_t socket_id; >> + int32_t book_id; >> + int32_t drawer_id; >> + bool dedicated; >> + uint8_t entitlement; /* Used only for vertical >> polarization */ >> uint64_t cpuid; >> #endif >> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c >> index c3dab007da..6f5622a024 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; > > indent issue. right, seems I forgot to update the patch set after the checkpatch. > >> + } >> + >> + if (mc->smp_props.books_supported) { >> + g_string_append_printf(s, "%sbooks (%u)", prefix, >> ms->smp.books); >> + prefix = multiply; > > ditto. > >> + } >> + [...] Thanks Regards, Pierre
>>> @@ -0,0 +1,15 @@ >>> +/* >>> + * CPU Topology >>> + * >>> + * Copyright IBM Corp. 2022 >> >> Shouldn't we have some range : 2022-2023 ? > > There was a discussion on this in the first spins, I think to remember that Nina wanted 22 and Thomas 23, > > now we have a third opinion :) . > > I must say that all three have their reasons and I take what the majority wants. There is an internal IBM document describing the copyright tags. If I recall well, first date is the first year the code was officially published, second year is the last year it was modified (so last commit of the year). Or something like that and it's theory, because we tend to forget. For an example, see the OPAL FW https://github.com/open-power/skiboot/, and run : "grep Copyright.*IBM" in the OPAL FW [ ...] >>> @@ -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; >> >> indent issue. > > right, seems I forgot to update the patch set after the checkpatch. nope, you didn't. checkpatch doesn't report it. It's not perfect :/ C.
On 4/4/23 14:35, Cédric Le Goater wrote: >>>> @@ -0,0 +1,15 @@ >>>> +/* >>>> + * CPU Topology >>>> + * >>>> + * Copyright IBM Corp. 2022 >>> >>> Shouldn't we have some range : 2022-2023 ? >> >> There was a discussion on this in the first spins, I think to >> remember that Nina wanted 22 and Thomas 23, >> >> now we have a third opinion :) . >> >> I must say that all three have their reasons and I take what the >> majority wants. > > There is an internal IBM document describing the copyright tags. If I > recall > well, first date is the first year the code was officially published, > second > year is the last year it was modified (so last commit of the year). Or > something like that and it's theory, because we tend to forget. > > For an example, see the OPAL FW https://github.com/open-power/skiboot/, > and run : > > "grep Copyright.*IBM" in the OPAL FW OK for me, it looks logical, and all three of you are right then. So I will use Copyright IBM Corp. 2022-2023 in the next spin if nobody is against. Thanks, Pierre > [ ...] > >>>> @@ -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; >>> >>> indent issue. >> >> right, seems I forgot to update the patch set after the checkpatch. > > nope, you didn't. checkpatch doesn't report it. It's not perfect :/ > > C.
On Tue, 2023-04-04 at 16:04 +0200, Pierre Morel wrote: > On 4/4/23 14:35, Cédric Le Goater wrote: > > > > > @@ -0,0 +1,15 @@ > > > > > +/* > > > > > + * CPU Topology > > > > > + * > > > > > + * Copyright IBM Corp. 2022 > > > > > > > > Shouldn't we have some range : 2022-2023 ? > > > > > > There was a discussion on this in the first spins, I think to > > > remember that Nina wanted 22 and Thomas 23, > > > > > > now we have a third opinion :) . > > > > > > I must say that all three have their reasons and I take what the > > > majority wants. > > > > There is an internal IBM document describing the copyright tags. If I > > recall > > well, first date is the first year the code was officially published, > > second > > year is the last year it was modified (so last commit of the year). Or > > something like that and it's theory, because we tend to forget. > > > > For an example, see the OPAL FW https://github.com/open-power/skiboot/, > > and run : > > > > "grep Copyright.*IBM" in the OPAL FW > > > OK for me, it looks logical, and all three of you are right then. > > So I will use > > Copyright IBM Corp. 2022-2023 You should use a comma instead of a hyphen as per IBM policy. I.e. 2022, 2023 > > in the next spin if nobody is against. > > Thanks, > > Pierre > > > > [ ...] > > > > > > > @@ -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; > > > > > > > > indent issue. > > > > > > right, seems I forgot to update the patch set after the checkpatch. > > > > nope, you didn't. checkpatch doesn't report it. It's not perfect :/ > > > > C.
On 4/11/23 14:27, Nina Schoetterl-Glausch wrote: > On Tue, 2023-04-04 at 16:04 +0200, Pierre Morel wrote: >> On 4/4/23 14:35, Cédric Le Goater wrote: >>>>>> @@ -0,0 +1,15 @@ >>>>>> +/* >>>>>> + * CPU Topology >>>>>> + * >>>>>> + * Copyright IBM Corp. 2022 >>>>> Shouldn't we have some range : 2022-2023 ? >>>> There was a discussion on this in the first spins, I think to >>>> remember that Nina wanted 22 and Thomas 23, >>>> >>>> now we have a third opinion :) . >>>> >>>> I must say that all three have their reasons and I take what the >>>> majority wants. >>> There is an internal IBM document describing the copyright tags. If I >>> recall >>> well, first date is the first year the code was officially published, >>> second >>> year is the last year it was modified (so last commit of the year). Or >>> something like that and it's theory, because we tend to forget. >>> >>> For an example, see the OPAL FW https://github.com/open-power/skiboot/, >>> and run : >>> >>> "grep Copyright.*IBM" in the OPAL FW >> >> OK for me, it looks logical, and all three of you are right then. >> >> So I will use >> >> Copyright IBM Corp. 2022-2023 > You should use a comma instead of a hyphen as per IBM policy. > I.e. 2022, 2023 OK, thanks. Regards, Pierre
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@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > --- > MAINTAINERS | 5 ++++ > qapi/machine-common.json | 22 ++++++++++++++ > qapi/machine-target.json | 12 ++++++++ > qapi/machine.json | 17 +++++++++-- > include/hw/boards.h | 10 ++++++- > include/hw/s390x/cpu-topology.h | 15 ++++++++++ Is hw/s390x the right path for cpu-topology? I haven't understood the difference between hw/s390x and target/s390x but target/s390x feels more correct, I could be mistaken though. > target/s390x/cpu.h | 5 ++++ > hw/core/machine-smp.c | 53 ++++++++++++++++++++++++++++----- > hw/core/machine.c | 4 +++ > hw/s390x/s390-virtio-ccw.c | 2 ++ > softmmu/vl.c | 6 ++++ > target/s390x/cpu.c | 7 +++++ > qapi/meson.build | 1 + > qemu-options.hx | 7 +++-- > 14 files changed, 152 insertions(+), 14 deletions(-) > create mode 100644 qapi/machine-common.json > create mode 100644 include/hw/s390x/cpu-topology.h > [...] > 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. [...] > 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 on the command line and cpu hotplug. I think setting the default entitlement to medium here should be fine. [...]
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@linux.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> --- >> MAINTAINERS | 5 ++++ >> qapi/machine-common.json | 22 ++++++++++++++ >> qapi/machine-target.json | 12 ++++++++ >> qapi/machine.json | 17 +++++++++-- >> include/hw/boards.h | 10 ++++++- >> include/hw/s390x/cpu-topology.h | 15 ++++++++++ > Is hw/s390x the right path for cpu-topology? > I haven't understood the difference between hw/s390x and target/s390x > but target/s390x feels more correct, I could be mistaken though. AFAIK target/s390 is for CPU emulation code while hw/s390 is for other emulation. So it depends how we classify the CPU topology, it is related to CPU but it is no emulation. Since Thomas approved this layout I would like to keep it like this. > >> target/s390x/cpu.h | 5 ++++ >> hw/core/machine-smp.c | 53 ++++++++++++++++++++++++++++----- >> hw/core/machine.c | 4 +++ >> hw/s390x/s390-virtio-ccw.c | 2 ++ >> softmmu/vl.c | 6 ++++ >> target/s390x/cpu.c | 7 +++++ >> qapi/meson.build | 1 + >> qemu-options.hx | 7 +++-- >> 14 files changed, 152 insertions(+), 14 deletions(-) >> create mode 100644 qapi/machine-common.json >> create mode 100644 include/hw/s390x/cpu-topology.h >> > [...] > >> 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 ? frankly I prefer to keep horizontal here which is exactly what is given in the documentation for entitlement = 0 > > [...] > >> 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? > > 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.
On 18/04/2023 12.01, 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@linux.ibm.com> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> --- >>> MAINTAINERS | 5 ++++ >>> qapi/machine-common.json | 22 ++++++++++++++ >>> qapi/machine-target.json | 12 ++++++++ >>> qapi/machine.json | 17 +++++++++-- >>> include/hw/boards.h | 10 ++++++- >>> include/hw/s390x/cpu-topology.h | 15 ++++++++++ >> Is hw/s390x the right path for cpu-topology? >> I haven't understood the difference between hw/s390x and target/s390x >> but target/s390x feels more correct, I could be mistaken though. > > AFAIK target/s390 is for CPU emulation code while hw/s390 is for other > emulation. > > So it depends how we classify the CPU topology, it is related to CPU but it > is no emulation. Normally I'd say target/ is for everything what happens within a CPU chip, and hw/ is for everything that happens outside of a CPU chip, i.e. machine definitions and other devices. Now CPU topology is borderline - drawers and books are rather a concept of the machine and not of the CPU, but things like dies and threads rather happen within a CPU chip. So I don't mind too much either way, but I think it's certainly ok to keep it in hw/s390x/ if you prefer that. Thomas
On 4/18/23 12:15, Thomas Huth wrote: > On 18/04/2023 12.01, 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@linux.ibm.com> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> MAINTAINERS | 5 ++++ >>>> qapi/machine-common.json | 22 ++++++++++++++ >>>> qapi/machine-target.json | 12 ++++++++ >>>> qapi/machine.json | 17 +++++++++-- >>>> include/hw/boards.h | 10 ++++++- >>>> include/hw/s390x/cpu-topology.h | 15 ++++++++++ >>> Is hw/s390x the right path for cpu-topology? >>> I haven't understood the difference between hw/s390x and target/s390x >>> but target/s390x feels more correct, I could be mistaken though. >> >> AFAIK target/s390 is for CPU emulation code while hw/s390 is for >> other emulation. >> >> So it depends how we classify the CPU topology, it is related to CPU >> but it is no emulation. > > Normally I'd say target/ is for everything what happens within a CPU > chip, and hw/ is for everything that happens outside of a CPU chip, > i.e. machine definitions and other devices. > Now CPU topology is borderline - drawers and books are rather a > concept of the machine and not of the CPU, but things like dies and > threads rather happen within a CPU chip. > So I don't mind too much either way, but I think it's certainly ok to > keep it in hw/s390x/ if you prefer that. > > Thomas > Thanks for the clarification Thomas. Pierre
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@linux.ibm.com> > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > --- [...] > > > > > 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. > > > > > > > [...] > > > > > 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. 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. > > > > > > 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.
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@linux.ibm.com> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> --- > [...] >>>> 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.
> 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@linux.ibm.com> > > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > > > --- > > [...] > > > > > 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. It is, I'd do: +static s390_topology_id s390_topology_from_cpu(S390CPU *cpu) +{ + struct S390CcwMachineState *s390ms = S390_CCW_MACHINE(current_machine); + s390_topology_id topology_id = {0}; + + topology_id.drawer = cpu->env.drawer_id; + topology_id.book = cpu->env.book_id; + topology_id.socket = cpu->env.socket_id; + topology_id.origin = cpu->env.core_id / 64; + topology_id.type = S390_TOPOLOGY_CPU_IFL; + topology_id.dedicated = cpu->env.dedicated; + + if (s390ms->vertical_polarization) { + uint8_t to_polarization[] = { + [S390_CPU_ENTITLEMENT_LOW] = 1, + [S390_CPU_ENTITLEMENT_MEDIUM] = 2, + [S390_CPU_ENTITLEMENT_HIGH] = 3, + }; + topology_id.entitlement = to_polarization[cpu->env.entitlement]; + } + + return topology_id; +} You can also use a switch of course. I'd also rename s390_topology_id.entitlement to polarization. > > 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. Well, it does change the API a bit, namely which values mean what, currently there is a value 0 that you're not supposed to use, that would go away. It also shows up in some meta command to print qapi interfaces. And dropping it simplifies the implementation IMO --- you don't need to think about and prevent usage of a nonexistent state. > > > > > > > > > > > > [...] > > > > > > > > > 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. That sounds harder to do, I guess you'd have to reimplement the PropertyInfo getters and setters to do that. > > > > 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. I don't see why we cannot just set it to medium. > > 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. No, your check function doesn't look the polarization at all (and shouldn't): +static void s390_topology_check(uint16_t socket_id, uint16_t book_id, + uint16_t drawer_id, uint16_t entitlement, + bool dedicated, Error **errp) +{ [...] + if (dedicated && (entitlement == S390_CPU_ENTITLEMENT_LOW || + entitlement == S390_CPU_ENTITLEMENT_MEDIUM)) { + error_setg(errp, "A dedicated cpu implies high entitlement"); + return; + } +} > > So we need adjustment before the check in both cases. I don't see why, just always reject it. > > I find it easier and more logical if there is no default value than to > have a default we need to overwrite. > > > >
On Tue, Apr 04, 2023 at 02:26:05PM +0200, Pierre Morel wrote: > > On 4/4/23 09:03, Cédric Le Goater wrote: > > On 4/3/23 18:28, Pierre Morel wrote: > > > diff --git a/include/hw/s390x/cpu-topology.h > > > b/include/hw/s390x/cpu-topology.h > > > new file mode 100644 > > > index 0000000000..83f31604cc > > > --- /dev/null > > > +++ b/include/hw/s390x/cpu-topology.h > > > @@ -0,0 +1,15 @@ > > > +/* > > > + * CPU Topology > > > + * > > > + * Copyright IBM Corp. 2022 > > > > Shouldn't we have some range : 2022-2023 ? > > There was a discussion on this in the first spins, I think to remember that > Nina wanted 22 and Thomas 23, > > now we have a third opinion :) . > > I must say that all three have their reasons and I take what the majority > wants. > > A vote? Whether or not to include a single year, or range of years in the copyright statement is ultimately a policy decision for the copyright holder to take (IBM in this case I presume), and not subject to community vote/preferences. I will note that some (possibly even many) organizations consider the year to be largely redundant and devoid of legal benefit, so are happy with basically any usage of dates (first year, most recent year, a range of years, or none at all). With this in mind, QEMU is willing to accept any usage wrt dates in the copyright statement. It is possible that IBM have a specific policy their employees are expected to follow. If so, follow that. With regards, Daniel
On 4/18/23 17:57, Daniel P. Berrangé wrote: > On Tue, Apr 04, 2023 at 02:26:05PM +0200, Pierre Morel wrote: >> On 4/4/23 09:03, Cédric Le Goater wrote: >>> On 4/3/23 18:28, Pierre Morel wrote: >>>> diff --git a/include/hw/s390x/cpu-topology.h >>>> b/include/hw/s390x/cpu-topology.h >>>> new file mode 100644 >>>> index 0000000000..83f31604cc >>>> --- /dev/null >>>> +++ b/include/hw/s390x/cpu-topology.h >>>> @@ -0,0 +1,15 @@ >>>> +/* >>>> + * CPU Topology >>>> + * >>>> + * Copyright IBM Corp. 2022 >>> Shouldn't we have some range : 2022-2023 ? >> There was a discussion on this in the first spins, I think to remember that >> Nina wanted 22 and Thomas 23, >> >> now we have a third opinion :) . >> >> I must say that all three have their reasons and I take what the majority >> wants. >> >> A vote? > Whether or not to include a single year, or range of years in > the copyright statement is ultimately a policy decision for the > copyright holder to take (IBM in this case I presume), and not > subject to community vote/preferences. > > I will note that some (possibly even many) organizations consider > the year to be largely redundant and devoid of legal benefit, so > are happy with basically any usage of dates (first year, most recent > year, a range of years, or none at all). With this in mind, QEMU is > willing to accept any usage wrt dates in the copyright statement. > > It is possible that IBM have a specific policy their employees are > expected to follow. If so, follow that. > > With regards, > Daniel OK, thanks, Regards, Pierre
diff --git a/MAINTAINERS b/MAINTAINERS index 5340de0515..9b1f80739e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1654,6 +1654,11 @@ F: hw/s390x/event-facility.c F: hw/s390x/sclp*.c L: qemu-s390x@nongnu.org +S390 CPU topology +M: Pierre Morel <pmorel@linux.ibm.com> +S: Supported +F: include/hw/s390x/cpu-topology.h + X86 Machines ------------ PC 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' ] } + 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' ] } diff --git a/qapi/machine.json b/qapi/machine.json index 604b686e59..1cdd83f3fd 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -9,6 +9,7 @@ ## { 'include': 'common.json' } +{ 'include': 'machine-common.json' } ## # @SysEmuTarget: @@ -70,7 +71,7 @@ # # @thread-id: ID of the underlying host thread # -# @props: properties describing to which node/socket/core/thread +# @props: properties describing to which node/drawer/book/socket/core/thread # virtual CPU belongs to, provided if supported by board # # @target: the QEMU system emulation target, which determines which @@ -902,13 +903,15 @@ # a CPU is being hotplugged. # # @node-id: NUMA node ID the CPU belongs to -# @socket-id: socket number within node/board the CPU belongs to +# @drawer-id: drawer number within node/board the CPU belongs to (since 8.1) +# @book-id: book number within drawer/node/board the CPU belongs to (since 8.1) +# @socket-id: socket number within book/node/board the CPU belongs to # @die-id: die number within socket the CPU belongs to (since 4.1) # @cluster-id: cluster number within die the CPU belongs to (since 7.1) # @core-id: core number within cluster the CPU belongs to # @thread-id: thread number within core the CPU belongs to # -# Note: currently there are 6 properties that could be present +# Note: currently there are 8 properties that could be present # but management should be prepared to pass through other # properties with device_add command to allow for future # interface extension. This also requires the filed names to be kept in @@ -918,6 +921,8 @@ ## { 'struct': 'CpuInstanceProperties', 'data': { '*node-id': 'int', + '*drawer-id': 'int', + '*book-id': 'int', '*socket-id': 'int', '*die-id': 'int', '*cluster-id': 'int', @@ -1467,6 +1472,10 @@ # # @cpus: number of virtual CPUs in the virtual machine # +# @drawers: number of drawers in the CPU topology (since 8.1) +# +# @books: number of books in the CPU topology (since 8.1) +# # @sockets: number of sockets in the CPU topology # # @dies: number of dies per socket in the CPU topology @@ -1483,6 +1492,8 @@ ## { 'struct': 'SMPConfiguration', 'data': { '*cpus': 'int', + '*drawers': 'int', + '*books': 'int', '*sockets': 'int', '*dies': 'int', '*clusters': 'int', diff --git a/include/hw/boards.h b/include/hw/boards.h index 6fbbfd56c8..9ef0bb76cf 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -131,12 +131,16 @@ typedef struct { * @clusters_supported - whether clusters are supported by the machine * @has_clusters - whether clusters are explicitly specified in the user * provided SMP configuration + * @books_supported - whether books are supported by the machine + * @drawers_supported - whether drawers are supported by the machine */ typedef struct { bool prefer_sockets; bool dies_supported; bool clusters_supported; bool has_clusters; + bool books_supported; + bool drawers_supported; } SMPCompatProps; /** @@ -301,7 +305,9 @@ typedef struct DeviceMemoryState { /** * CpuTopology: * @cpus: the number of present logical processors on the machine - * @sockets: the number of sockets on the machine + * @drawers: the number of drawers on the machine + * @books: the number of books in one drawer + * @sockets: the number of sockets in one book * @dies: the number of dies in one socket * @clusters: the number of clusters in one die * @cores: the number of cores in one cluster @@ -310,6 +316,8 @@ typedef struct DeviceMemoryState { */ typedef struct CpuTopology { unsigned int cpus; + unsigned int drawers; + unsigned int books; unsigned int sockets; unsigned int dies; unsigned int clusters; diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h new file mode 100644 index 0000000000..83f31604cc --- /dev/null +++ b/include/hw/s390x/cpu-topology.h @@ -0,0 +1,15 @@ +/* + * CPU Topology + * + * Copyright IBM Corp. 2022 + * + * 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. + */ +#ifndef HW_S390X_CPU_TOPOLOGY_H +#define HW_S390X_CPU_TOPOLOGY_H + +#define S390_TOPOLOGY_CPU_IFL 0x03 + +#endif diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 7d6d01325b..f2b2a38fe7 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -131,6 +131,11 @@ struct CPUArchState { #if !defined(CONFIG_USER_ONLY) uint32_t core_id; /* PoP "CPU address", same as cpu_index */ + int32_t socket_id; + int32_t book_id; + int32_t drawer_id; + bool dedicated; + uint8_t entitlement; /* Used only for vertical polarization */ uint64_t cpuid; #endif diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index c3dab007da..6f5622a024 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; + } + + 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); if (mc->smp_props.dies_supported) { g_string_append_printf(s, " * dies (%u)", ms->smp.dies); @@ -73,6 +84,8 @@ void machine_parse_smp_config(MachineState *ms, { 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; + + 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; + /* 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); } } 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); } } /* try to calculate omitted threads at last */ if (threads == 0) { - threads = maxcpus / (sockets * dies * clusters * cores); + threads = maxcpus / + (drawers * books * sockets * dies * clusters * cores); } } - maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * clusters * cores * threads; + maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies * + clusters * cores * threads; cpus = cpus > 0 ? cpus : maxcpus; ms->smp.cpus = cpus; + ms->smp.drawers = drawers; + ms->smp.books = books; ms->smp.sockets = sockets; ms->smp.dies = dies; ms->smp.clusters = clusters; @@ -161,7 +197,8 @@ void machine_parse_smp_config(MachineState *ms, mc->smp_props.has_clusters = config->has_clusters; /* sanity-check of the computed topology */ - if (sockets * dies * clusters * cores * threads != maxcpus) { + if (drawers * books * sockets * dies * clusters * cores * threads != + maxcpus) { g_autofree char *topo_msg = cpu_hierarchy_to_string(ms); error_setg(errp, "Invalid CPU topology: " "product of the hierarchy must match maxcpus: " diff --git a/hw/core/machine.c b/hw/core/machine.c index 1cf6822e06..41c7ba7027 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -831,6 +831,8 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name, MachineState *ms = MACHINE(obj); SMPConfiguration *config = &(SMPConfiguration){ .has_cpus = true, .cpus = ms->smp.cpus, + .has_drawers = true, .drawers = ms->smp.drawers, + .has_books = true, .books = ms->smp.books, .has_sockets = true, .sockets = ms->smp.sockets, .has_dies = true, .dies = ms->smp.dies, .has_clusters = true, .clusters = ms->smp.clusters, @@ -1096,6 +1098,8 @@ static void machine_initfn(Object *obj) /* default to mc->default_cpus */ ms->smp.cpus = mc->default_cpus; ms->smp.max_cpus = mc->default_cpus; + ms->smp.drawers = 1; + ms->smp.books = 1; ms->smp.sockets = 1; ms->smp.dies = 1; ms->smp.clusters = 1; diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 503f212a31..1a9bcda8b6 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -736,6 +736,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) mc->no_sdcard = 1; mc->max_cpus = S390_MAX_CPUS; mc->has_hotpluggable_cpus = true; + mc->smp_props.books_supported = true; + mc->smp_props.drawers_supported = true; assert(!mc->get_hotplug_handler); mc->get_hotplug_handler = s390_get_hotplug_handler; mc->cpu_index_to_instance_props = s390_cpu_index_to_props; diff --git a/softmmu/vl.c b/softmmu/vl.c index 3340f63c37..bc293f8100 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -724,6 +724,12 @@ static QemuOptsList qemu_smp_opts = { { .name = "cpus", .type = QEMU_OPT_NUMBER, + }, { + .name = "drawers", + .type = QEMU_OPT_NUMBER, + }, { + .name = "books", + .type = QEMU_OPT_NUMBER, }, { .name = "sockets", .type = QEMU_OPT_NUMBER, 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), #endif DEFINE_PROP_END_OF_LIST() }; diff --git a/qapi/meson.build b/qapi/meson.build index fbdb442fdf..b5b01fb7b5 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -35,6 +35,7 @@ qapi_all_modules = [ 'error', 'introspect', 'job', + 'machine-common', 'machine', 'machine-target', 'migration', diff --git a/qemu-options.hx b/qemu-options.hx index d42f60fb91..4c79e153fb 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -250,11 +250,14 @@ SRST ERST DEF("smp", HAS_ARG, QEMU_OPTION_smp, - "-smp [[cpus=]n][,maxcpus=maxcpus][,sockets=sockets][,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]\n" + "-smp [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets]\n" + " [,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]\n" " set the number of initial CPUs to 'n' [default=1]\n" " maxcpus= maximum number of total CPUs, including\n" " offline CPUs for hotplug, etc\n" - " sockets= number of sockets on the machine board\n" + " drawers= number of drawers on the machine board\n" + " books= number of books in one drawer\n" + " sockets= number of sockets in one book\n" " dies= number of dies in one socket\n" " clusters= number of clusters in one die\n" " cores= number of cores in one cluster\n"