| Message ID | 20230914120650.1318932-2-nsg@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > From: Pierre Morel <pmorel@linux.ibm.com> > > S390 adds two new SMP levels, drawers and books to the CPU > topology. > S390 CPUs have specific topology features like dedication and > entitlement. These indicate to the guest information on host > vCPU scheduling and help the guest make better scheduling decisions. > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > --- > qapi/machine-common.json | 21 +++++++++++++ > qapi/machine.json | 19 ++++++++++-- > include/hw/boards.h | 10 +++++- > include/hw/qdev-properties-system.h | 4 +++ > target/s390x/cpu.h | 6 ++++ > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- > hw/core/machine.c | 4 +++ > hw/core/qdev-properties-system.c | 13 ++++++++ > hw/s390x/s390-virtio-ccw.c | 4 +++ > softmmu/vl.c | 6 ++++ > target/s390x/cpu.c | 7 +++++ > qapi/meson.build | 1 + > qemu-options.hx | 7 +++-- > 13 files changed, 137 insertions(+), 13 deletions(-) > create mode 100644 qapi/machine-common.json > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > new file mode 100644 > index 0000000000..e40421bb37 > --- /dev/null > +++ b/qapi/machine-common.json Why do you need a separate QAPI sub-module? > @@ -0,0 +1,21 @@ > +# -*- 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.2 > +## > +{ 'enum': 'CpuS390Entitlement', > + 'prefix': 'S390_CPU_ENTITLEMENT', > + 'data': [ 'auto', 'low', 'medium', 'high' ] } > diff --git a/qapi/machine.json b/qapi/machine.json > index a08b6576ca..a63cb951d2 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -9,6 +9,7 @@ ## # = Machines > ## > > { 'include': 'common.json' } > +{ 'include': 'machine-common.json' } Section structure is borked :) Existing section "Machine" now ends at the new "Machines S390 data types" you pull in here. The contents of below moves from "Machines" to "Machines S390 data types". Before I explain how to avoid this, I'd like to understand why we need a new sub-module. > > ## > # @SysEmuTarget: > @@ -71,7 +72,7 @@ ## # @CpuInfoFast: # # Information about a virtual CPU # # @cpu-index: index of the virtual CPU # # @qom-path: path to the CPU object in the QOM tree > # > # @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 Is this description accurate? @props is of type CpuInstanceProperties, shown below. Its documentation describes it as "properties to be used for hotplugging a CPU instance, it should be passed by management with device_add command when a CPU is being hotplugged." Hmm. I figure details ("node/drawer/book/socket/core/thread") are better left to CpuInstanceProperties. The "provided if supported by board" part makes no sense to me. If @props is there, it lists the properties we need to provide with device_add. What if it's not there? Same as empty list, i.e. we don't need to provide properties with device_add? Not your patch's fault, but let's get this in shape if we can. > # > # @target: the QEMU system emulation target, which determines which > @@ -901,7 +902,11 @@ > # > # @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.2) > +# > +# @book-id: book number within drawer/node/board the CPU belongs to (since 8.2) Long lines, please wrap: # @drawer-id: drawer number within node/board the CPU belongs to # (since 8.2) # # @book-id: book number within drawer/node/board the CPU belongs to # (since 8.2) > +# > +# @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) > # > @@ -912,7 +917,7 @@ ## # @CpuInstanceProperties: # # List of properties to be used for hotplugging a CPU instance, it # should be passed by management with device_add command when 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 # # @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 but > +# 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 sync with the # properties passed to -device/device_add. The last sentence is for developers, not for users, which means it doesn't belong here. Suggest to move it to a non-doc comment, and rephrase the note like # Note: management should be prepared to pass through additional # properties with device_add. > @@ -922,6 +927,8 @@ > ## > { 'struct': 'CpuInstanceProperties', Non-doc comment could go here: # Keep these in sync with the properties device_add accepts Again, not your patch's fault, but your help improving this stuff would be appreciated. > 'data': { '*node-id': 'int', > + '*drawer-id': 'int', > + '*book-id': 'int', > '*socket-id': 'int', > '*die-id': 'int', > '*cluster-id': 'int', '*core-id': 'int', '*thread-id': 'int' } } > @@ -1480,6 +1487,10 @@ > # > # @cpus: number of virtual CPUs in the virtual machine > # > +# @drawers: number of drawers in the CPU topology (since 8.2) > +# > +# @books: number of books in the CPU topology (since 8.2) > +# > # @sockets: number of sockets in the CPU topology > # > # @dies: number of dies per socket in the CPU topology > @@ -1498,6 +1509,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 6c67af196a..6dcfc879eb 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -134,12 +134,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; > > /** > @@ -310,7 +314,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 > @@ -319,6 +325,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/qapi/meson.build b/qapi/meson.build > index 60a668b343..f81a37565c 100644 > --- a/qapi/meson.build > +++ b/qapi/meson.build > @@ -36,6 +36,7 @@ qapi_all_modules = [ > 'error', > 'introspect', > 'job', > + 'machine-common', > 'machine', > 'machine-target', > 'migration', [...]
On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > > > From: Pierre Morel <pmorel@linux.ibm.com> > > > > S390 adds two new SMP levels, drawers and books to the CPU > > topology. > > S390 CPUs have specific topology features like dedication and > > entitlement. These indicate to the guest information on host > > vCPU scheduling and help the guest make better scheduling decisions. > > > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > --- > > qapi/machine-common.json | 21 +++++++++++++ > > qapi/machine.json | 19 ++++++++++-- > > include/hw/boards.h | 10 +++++- > > include/hw/qdev-properties-system.h | 4 +++ > > target/s390x/cpu.h | 6 ++++ > > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- > > hw/core/machine.c | 4 +++ > > hw/core/qdev-properties-system.c | 13 ++++++++ > > hw/s390x/s390-virtio-ccw.c | 4 +++ > > softmmu/vl.c | 6 ++++ > > target/s390x/cpu.c | 7 +++++ > > qapi/meson.build | 1 + > > qemu-options.hx | 7 +++-- > > 13 files changed, 137 insertions(+), 13 deletions(-) > > create mode 100644 qapi/machine-common.json > > > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > > new file mode 100644 > > index 0000000000..e40421bb37 > > --- /dev/null > > +++ b/qapi/machine-common.json > > Why do you need a separate QAPI sub-module? See here https://lore.kernel.org/qemu-devel/d8da6f7d1e3addcb63614f548ed77ac1b8895e63.camel@linux.ibm.com/ > > > @@ -0,0 +1,21 @@ > > +# -*- 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.2 > > +## > > +{ 'enum': 'CpuS390Entitlement', > > + 'prefix': 'S390_CPU_ENTITLEMENT', > > + 'data': [ 'auto', 'low', 'medium', 'high' ] } > > diff --git a/qapi/machine.json b/qapi/machine.json > > index a08b6576ca..a63cb951d2 100644 > > --- a/qapi/machine.json > > +++ b/qapi/machine.json > > @@ -9,6 +9,7 @@ > ## > # = Machines > > ## > > > > { 'include': 'common.json' } > > +{ 'include': 'machine-common.json' } > > Section structure is borked :) > > Existing section "Machine" now ends at the new "Machines S390 data > types" you pull in here. The contents of below moves from "Machines" to > "Machines S390 data types". > > Before I explain how to avoid this, I'd like to understand why we need a > new sub-module. > > > > > ## > > # @SysEmuTarget: > > @@ -71,7 +72,7 @@ > ## > # @CpuInfoFast: > # > # Information about a virtual CPU > # > # @cpu-index: index of the virtual CPU > # > # @qom-path: path to the CPU object in the QOM tree > > # > > # @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 > > Is this description accurate? Kinda, although the wording might not be the best. All the CpuInstanceProperties fields are optional, it's like a superset of possible properties across architectures. Only a subset might be returned by query-cpus-fast. Also die and cluster are missing. > > @props is of type CpuInstanceProperties, shown below. Its documentation > describes it as "properties to be used for hotplugging a CPU instance, > it should be passed by management with device_add command when a CPU is > being hotplugged." Hmm. > > I figure details ("node/drawer/book/socket/core/thread") are better left > to CpuInstanceProperties. > > The "provided if supported by board" part makes no sense to me. If > @props is there, it lists the properties we need to provide with > device_add. What if it's not there? Same as empty list, i.e. we don't > need to provide properties with device_add? There are default values/default logic. For s390x, socket, book, drawer are calculated from the core id if not provided with device_add. Partial specifications are rejected. > > Not your patch's fault, but let's get this in shape if we can. > > > # > > # @target: the QEMU system emulation target, which determines which > > @@ -901,7 +902,11 @@ > > # > > # @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.2) > > +# > > +# @book-id: book number within drawer/node/board the CPU belongs to (since 8.2) > > Long lines, please wrap: > > # @drawer-id: drawer number within node/board the CPU belongs to > # (since 8.2) > # > # @book-id: book number within drawer/node/board the CPU belongs to > # (since 8.2) Ok. > > > +# > > +# @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) > > # > > @@ -912,7 +917,7 @@ > ## > # @CpuInstanceProperties: > # > # List of properties to be used for hotplugging a CPU instance, it > # should be passed by management with device_add command when 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 > # > # @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 but > > +# 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 sync with the > # properties passed to -device/device_add. > > The last sentence is for developers, not for users, which means it > doesn't belong here. Suggest to move it to a non-doc comment, and > rephrase the note like > > # Note: management should be prepared to pass through additional > # properties with device_add. > > > @@ -922,6 +927,8 @@ > > ## > > { 'struct': 'CpuInstanceProperties', > > Non-doc comment could go here: > > # Keep these in sync with the properties device_add accepts > > Again, not your patch's fault, but your help improving this stuff would > be appreciated. > > > 'data': { '*node-id': 'int', > > + '*drawer-id': 'int', > > + '*book-id': 'int', > > '*socket-id': 'int', > > '*die-id': 'int', > > '*cluster-id': 'int', > '*core-id': 'int', > '*thread-id': 'int' > } > } > [...]
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: >> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: >> >> > From: Pierre Morel <pmorel@linux.ibm.com> >> > >> > S390 adds two new SMP levels, drawers and books to the CPU >> > topology. >> > S390 CPUs have specific topology features like dedication and >> > entitlement. These indicate to the guest information on host >> > vCPU scheduling and help the guest make better scheduling decisions. >> > >> > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > --- >> > qapi/machine-common.json | 21 +++++++++++++ >> > qapi/machine.json | 19 ++++++++++-- >> > include/hw/boards.h | 10 +++++- >> > include/hw/qdev-properties-system.h | 4 +++ >> > target/s390x/cpu.h | 6 ++++ >> > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- >> > hw/core/machine.c | 4 +++ >> > hw/core/qdev-properties-system.c | 13 ++++++++ >> > hw/s390x/s390-virtio-ccw.c | 4 +++ >> > softmmu/vl.c | 6 ++++ >> > target/s390x/cpu.c | 7 +++++ >> > qapi/meson.build | 1 + >> > qemu-options.hx | 7 +++-- >> > 13 files changed, 137 insertions(+), 13 deletions(-) >> > create mode 100644 qapi/machine-common.json >> > >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json >> > new file mode 100644 >> > index 0000000000..e40421bb37 >> > --- /dev/null >> > +++ b/qapi/machine-common.json >> >> Why do you need a separate QAPI sub-module? > > See here https://lore.kernel.org/qemu-devel/d8da6f7d1e3addcb63614f548ed77ac1b8895e63.camel@linux.ibm.com/ Quote: CpuS390Entitlement would be useful in both machine.json and machine-target.json This is not obvious from this patch. I figure this patch could add it to machine.json just fine. The use in machine-target.json in appears only in PATCH 08. because query-cpu-fast is defined in machine.json and set-cpu-topology is defined in machine-target.json. So then the question is where best to define CpuS390Entitlement. In machine.json and include machine.json in machine-target.json? Or define it in another file and include it from both? You do the latter in this patch. I figure the former would be tolerable, too. That said, having target-specific stuff in machine.json feels... odd. Before this series, we have CpuInfoS390 and CpuS390State there, for query-cpus-fast. That command returns a list of objects where common members are target-independent, and the variant members are target-dependent. qmp_query_cpus_fast() uses a CPU method to populate the target-dependent members. I'm not sure splitting query-cpus-fast into a target-dependent and a target-independent part is worth the bother. In this patch, you work with the structure you found. Can't fault you for that :) >> > @@ -0,0 +1,21 @@ >> > +# -*- 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.2 >> > +## >> > +{ 'enum': 'CpuS390Entitlement', >> > + 'prefix': 'S390_CPU_ENTITLEMENT', >> > + 'data': [ 'auto', 'low', 'medium', 'high' ] } >> > diff --git a/qapi/machine.json b/qapi/machine.json >> > index a08b6576ca..a63cb951d2 100644 >> > --- a/qapi/machine.json >> > +++ b/qapi/machine.json >> > @@ -9,6 +9,7 @@ >> ## >> # = Machines >> > ## >> > >> > { 'include': 'common.json' } >> > +{ 'include': 'machine-common.json' } >> >> Section structure is borked :) >> >> Existing section "Machine" now ends at the new "Machines S390 data >> types" you pull in here. The contents of below moves from "Machines" to >> "Machines S390 data types". >> >> Before I explain how to avoid this, I'd like to understand why we need a >> new sub-module. >> >> > >> > ## >> > # @SysEmuTarget: >> > @@ -71,7 +72,7 @@ >> ## >> # @CpuInfoFast: >> # >> # Information about a virtual CPU >> # >> # @cpu-index: index of the virtual CPU >> # >> # @qom-path: path to the CPU object in the QOM tree >> > # >> > # @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 >> >> Is this description accurate? > > Kinda, although the wording might not be the best. > All the CpuInstanceProperties fields are optional, it's like a superset of possible > properties across architectures. > Only a subset might be returned by query-cpus-fast. Let's see whether I got this right... The members of CpuInstanceProperties are properties you can pass to device_add for some targets. The members present in a response from query-cpus-fast are properties you must pass to device_add in this VM. Or is that a "may pass"? On what exactly does the set of present members depend? Just the target? The machine type? The CPU? Anything else? > Also die and cluster are missing. Does this need fixing? >> @props is of type CpuInstanceProperties, shown below. Its documentation >> describes it as "properties to be used for hotplugging a CPU instance, >> it should be passed by management with device_add command when a CPU is >> being hotplugged." Hmm. >> >> I figure details ("node/drawer/book/socket/core/thread") are better left >> to CpuInstanceProperties. >> >> The "provided if supported by board" part makes no sense to me. If >> @props is there, it lists the properties we need to provide with >> device_add. What if it's not there? Same as empty list, i.e. we don't >> need to provide properties with device_add? > > There are default values/default logic. > For s390x, socket, book, drawer are calculated from the core id > if not provided with device_add. > Partial specifications are rejected. Undocumented magic? >> Not your patch's fault, but let's get this in shape if we can. [...]
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > From: Pierre Morel <pmorel@linux.ibm.com> > > S390 adds two new SMP levels, drawers and books to the CPU > topology. > S390 CPUs have specific topology features like dedication and > entitlement. These indicate to the guest information on host > vCPU scheduling and help the guest make better scheduling decisions. > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > new file mode 100644 > index 0000000000..e40421bb37 > --- /dev/null > +++ b/qapi/machine-common.json > @@ -0,0 +1,21 @@ > +# -*- 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 CPU entitlements Would someone reasonably familiar with S390 understand this? Because I'm not and I don't; I wonder what "a virtual CPU assuming an entitlement" means. > +# > +# Since: 8.2 > +## > +{ 'enum': 'CpuS390Entitlement', > + 'prefix': 'S390_CPU_ENTITLEMENT', > + 'data': [ 'auto', 'low', 'medium', 'high' ] } [...]
On Wed, 2023-09-20 at 12:57 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > > > On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: > > > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > > > > > > > From: Pierre Morel <pmorel@linux.ibm.com> > > > > > > > > S390 adds two new SMP levels, drawers and books to the CPU > > > > topology. > > > > S390 CPUs have specific topology features like dedication and > > > > entitlement. These indicate to the guest information on host > > > > vCPU scheduling and help the guest make better scheduling decisions. > > > > > > > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > > > --- > > > > qapi/machine-common.json | 21 +++++++++++++ > > > > qapi/machine.json | 19 ++++++++++-- > > > > include/hw/boards.h | 10 +++++- > > > > include/hw/qdev-properties-system.h | 4 +++ > > > > target/s390x/cpu.h | 6 ++++ > > > > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- > > > > hw/core/machine.c | 4 +++ > > > > hw/core/qdev-properties-system.c | 13 ++++++++ > > > > hw/s390x/s390-virtio-ccw.c | 4 +++ > > > > softmmu/vl.c | 6 ++++ > > > > target/s390x/cpu.c | 7 +++++ > > > > qapi/meson.build | 1 + > > > > qemu-options.hx | 7 +++-- > > > > 13 files changed, 137 insertions(+), 13 deletions(-) > > > > create mode 100644 qapi/machine-common.json > > > > > > > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > > > > new file mode 100644 > > > > index 0000000000..e40421bb37 > > > > --- /dev/null > > > > +++ b/qapi/machine-common.json > > > > > > Why do you need a separate QAPI sub-module? > > > > See here https://lore.kernel.org/qemu-devel/d8da6f7d1e3addcb63614f548ed77ac1b8895e63.camel@linux.ibm.com/ > > Quote: > > CpuS390Entitlement would be useful in both machine.json and machine-target.json > > This is not obvious from this patch. I figure this patch could add it > to machine.json just fine. The use in machine-target.json in appears > only in PATCH 08. Want me to add the rational to the commit message? > > because query-cpu-fast is defined in machine.json and set-cpu-topology is defined > in machine-target.json. > > So then the question is where best to define CpuS390Entitlement. > In machine.json and include machine.json in machine-target.json? > Or define it in another file and include it from both? > > You do the latter in this patch. > > I figure the former would be tolerable, too. > > That said, having target-specific stuff in machine.json feels... odd. > Before this series, we have CpuInfoS390 and CpuS390State there, for > query-cpus-fast. That command returns a list of objects where common > members are target-independent, and the variant members are > target-dependent. qmp_query_cpus_fast() uses a CPU method to populate > the target-dependent members. > > I'm not sure splitting query-cpus-fast into a target-dependent and a > target-independent part is worth the bother. > > In this patch, you work with the structure you found. Can't fault you > for that :) > > > > > @@ -0,0 +1,21 @@ > > > > +# -*- 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.2 > > > > +## > > > > +{ 'enum': 'CpuS390Entitlement', > > > > + 'prefix': 'S390_CPU_ENTITLEMENT', > > > > + 'data': [ 'auto', 'low', 'medium', 'high' ] } > > > > diff --git a/qapi/machine.json b/qapi/machine.json > > > > index a08b6576ca..a63cb951d2 100644 > > > > --- a/qapi/machine.json > > > > +++ b/qapi/machine.json > > > > @@ -9,6 +9,7 @@ > > > ## > > > # = Machines > > > > ## > > > > > > > > { 'include': 'common.json' } > > > > +{ 'include': 'machine-common.json' } > > > > > > Section structure is borked :) > > > > > > Existing section "Machine" now ends at the new "Machines S390 data > > > types" you pull in here. The contents of below moves from "Machines" to > > > "Machines S390 data types". > > > > > > Before I explain how to avoid this, I'd like to understand why we need a > > > new sub-module. > > > > > > > > > > > ## > > > > # @SysEmuTarget: > > > > @@ -71,7 +72,7 @@ > > > ## > > > # @CpuInfoFast: > > > # > > > # Information about a virtual CPU > > > # > > > # @cpu-index: index of the virtual CPU > > > # > > > # @qom-path: path to the CPU object in the QOM tree > > > > # > > > > # @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 > > > > > > Is this description accurate? > > > > Kinda, although the wording might not be the best. > > All the CpuInstanceProperties fields are optional, it's like a superset of possible > > properties across architectures. > > Only a subset might be returned by query-cpus-fast. > > Let's see whether I got this right... > > The members of CpuInstanceProperties are properties you can pass to > device_add for some targets. Yes. > > The members present in a response from query-cpus-fast are properties > you must pass to device_add in this VM. Or is that a "may pass"? On x86 must pass, s390x may pass, I haven't checked other architectures. s390x shows the defaults calculated. > > On what exactly does the set of present members depend? Just the > target? The machine type? The CPU? Anything else? The target and the machine I'd say. On x86 if you have one die per socket you don't need to provide a die_id on device_add. > > > Also die and cluster are missing. > > Does this need fixing? Only if we keep the list of properties here. > > > @props is of type CpuInstanceProperties, shown below. Its documentation > > > describes it as "properties to be used for hotplugging a CPU instance, > > > it should be passed by management with device_add command when a CPU is > > > being hotplugged." Hmm. > > > > > > I figure details ("node/drawer/book/socket/core/thread") are better left > > > to CpuInstanceProperties. > > > > > > The "provided if supported by board" part makes no sense to me. If > > > @props is there, it lists the properties we need to provide with > > > device_add. What if it's not there? Same as empty list, i.e. we don't > > > need to provide properties with device_add? > > > > There are default values/default logic. > > For s390x, socket, book, drawer are calculated from the core id > > if not provided with device_add. > > Partial specifications are rejected. > > Undocumented magic? Patch 13 documents it: Default topology usage ---------------------- [...] When a CPU is defined by the ``-device`` command argument, the tree topology attributes must all be defined or all not defined. .. code-block:: bash -device gen16b-s390x-cpu,drawer-id=1,book-id=1,socket-id=2,core-id=1 or .. code-block:: bash -device gen16b-s390x-cpu,core-id=1,dedicated=true If none of the tree attributes (drawer, book, sockets), are specified for the ``-device`` argument, like for all CPUs defined with the ``-smp`` command argument the topology tree attributes will be set by simply adding the CPUs to the topology based on the core-id. QEMU will not try to resolve collisions and will report an error if the CPU topology defined explicitly or implicitly on a ``-device`` argument collides with the definition of a CPU implicitly defined on the ``-smp`` argument. When the topology modifier attributes are not defined for the ``-device`` command argument they takes following default values: - dedicated: ``false`` - entitlement: ``medium`` Hot plug ++++++++ New CPUs can be plugged using the device_add hmp command as in: .. code-block:: bash (qemu) device_add gen16b-s390x-cpu,core-id=9 The placement of the CPU is derived from the core-id as described above. The topology can of course also be fully defined: .. code-block:: bash (qemu) device_add gen16b-s390x-cpu,drawer-id=1,book-id=1,socket-id=2,core-id=1 > > > > Not your patch's fault, but let's get this in shape if we can. > > [...] >
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > On Wed, 2023-09-20 at 12:57 +0200, Markus Armbruster wrote: >> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: >> >> > On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: >> > > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: >> > > >> > > > From: Pierre Morel <pmorel@linux.ibm.com> >> > > > >> > > > S390 adds two new SMP levels, drawers and books to the CPU >> > > > topology. >> > > > S390 CPUs have specific topology features like dedication and >> > > > entitlement. These indicate to the guest information on host >> > > > vCPU scheduling and help the guest make better scheduling decisions. >> > > > >> > > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > > > --- >> > > > qapi/machine-common.json | 21 +++++++++++++ >> > > > qapi/machine.json | 19 ++++++++++-- >> > > > include/hw/boards.h | 10 +++++- >> > > > include/hw/qdev-properties-system.h | 4 +++ >> > > > target/s390x/cpu.h | 6 ++++ >> > > > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- >> > > > hw/core/machine.c | 4 +++ >> > > > hw/core/qdev-properties-system.c | 13 ++++++++ >> > > > hw/s390x/s390-virtio-ccw.c | 4 +++ >> > > > softmmu/vl.c | 6 ++++ >> > > > target/s390x/cpu.c | 7 +++++ >> > > > qapi/meson.build | 1 + >> > > > qemu-options.hx | 7 +++-- >> > > > 13 files changed, 137 insertions(+), 13 deletions(-) >> > > > create mode 100644 qapi/machine-common.json >> > > > >> > > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json >> > > > new file mode 100644 >> > > > index 0000000000..e40421bb37 >> > > > --- /dev/null >> > > > +++ b/qapi/machine-common.json >> > > >> > > Why do you need a separate QAPI sub-module? >> > >> > See here https://lore.kernel.org/qemu-devel/d8da6f7d1e3addcb63614f548ed77ac1b8895e63.camel@linux.ibm.com/ >> >> Quote: >> >> CpuS390Entitlement would be useful in both machine.json and machine-target.json >> >> This is not obvious from this patch. I figure this patch could add it >> to machine.json just fine. The use in machine-target.json in appears >> only in PATCH 08. > > Want me to add the rational to the commit message? Would work for me. If the target-specific stuff in machine.json (discussed below) bothers us, we can clean up on top. >> because query-cpu-fast is defined in machine.json and set-cpu-topology is defined >> in machine-target.json. >> >> So then the question is where best to define CpuS390Entitlement. >> In machine.json and include machine.json in machine-target.json? >> Or define it in another file and include it from both? >> >> You do the latter in this patch. >> >> I figure the former would be tolerable, too. >> >> That said, having target-specific stuff in machine.json feels... odd. >> Before this series, we have CpuInfoS390 and CpuS390State there, for >> query-cpus-fast. That command returns a list of objects where common >> members are target-independent, and the variant members are >> target-dependent. qmp_query_cpus_fast() uses a CPU method to populate >> the target-dependent members. >> >> I'm not sure splitting query-cpus-fast into a target-dependent and a >> target-independent part is worth the bother. >> >> In this patch, you work with the structure you found. Can't fault you >> for that :) >> >> > > > @@ -0,0 +1,21 @@ >> > > > +# -*- 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.2 >> > > > +## >> > > > +{ 'enum': 'CpuS390Entitlement', >> > > > + 'prefix': 'S390_CPU_ENTITLEMENT', >> > > > + 'data': [ 'auto', 'low', 'medium', 'high' ] } >> > > > diff --git a/qapi/machine.json b/qapi/machine.json >> > > > index a08b6576ca..a63cb951d2 100644 >> > > > --- a/qapi/machine.json >> > > > +++ b/qapi/machine.json >> > > > @@ -9,6 +9,7 @@ >> > > ## >> > > # = Machines >> > > > ## >> > > > >> > > > { 'include': 'common.json' } >> > > > +{ 'include': 'machine-common.json' } >> > > >> > > Section structure is borked :) >> > > >> > > Existing section "Machine" now ends at the new "Machines S390 data >> > > types" you pull in here. The contents of below moves from "Machines" to >> > > "Machines S390 data types". >> > > >> > > Before I explain how to avoid this, I'd like to understand why we need a >> > > new sub-module. >> > > >> > > > >> > > > ## >> > > > # @SysEmuTarget: >> > > > @@ -71,7 +72,7 @@ >> > > ## >> > > # @CpuInfoFast: >> > > # >> > > # Information about a virtual CPU >> > > # >> > > # @cpu-index: index of the virtual CPU >> > > # >> > > # @qom-path: path to the CPU object in the QOM tree >> > > > # >> > > > # @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 >> > > >> > > Is this description accurate? >> > >> > Kinda, although the wording might not be the best. >> > All the CpuInstanceProperties fields are optional, it's like a superset of possible >> > properties across architectures. >> > Only a subset might be returned by query-cpus-fast. >> >> Let's see whether I got this right... >> >> The members of CpuInstanceProperties are properties you can pass to >> device_add for some targets. > > Yes. > >> >> The members present in a response from query-cpus-fast are properties >> you must pass to device_add in this VM. Or is that a "may pass"? > > On x86 must pass, s390x may pass, I haven't checked other architectures. > s390x shows the defaults calculated. Asking you to figure this out for all the targets wouldn't be fair, so I won't. Perhaps the documentation should state explicitly that properties may or may not be optional, and ideally point to documentation that tells you more, like the stuff you show below. This might also address my "undocumented magic" remark below. Again, I'm not asking you to create documentation for the other targets. >> On what exactly does the set of present members depend? Just the >> target? The machine type? The CPU? Anything else? > > The target and the machine I'd say. > On x86 if you have one die per socket you don't need to provide a die_id on device_add. >> >> > Also die and cluster are missing. >> >> Does this need fixing? > > Only if we keep the list of properties here. Makes sense. Let's replace it. >> > > @props is of type CpuInstanceProperties, shown below. Its documentation >> > > describes it as "properties to be used for hotplugging a CPU instance, >> > > it should be passed by management with device_add command when a CPU is >> > > being hotplugged." Hmm. >> > > >> > > I figure details ("node/drawer/book/socket/core/thread") are better left >> > > to CpuInstanceProperties. >> > > >> > > The "provided if supported by board" part makes no sense to me. If >> > > @props is there, it lists the properties we need to provide with >> > > device_add. What if it's not there? Same as empty list, i.e. we don't >> > > need to provide properties with device_add? >> > >> > There are default values/default logic. >> > For s390x, socket, book, drawer are calculated from the core id >> > if not provided with device_add. >> > Partial specifications are rejected. >> >> Undocumented magic? > > Patch 13 documents it: > > Default topology usage > ---------------------- > > [...] > > When a CPU is defined by the ``-device`` command argument, the > tree topology attributes must all be defined or all not defined. > > .. code-block:: bash > > -device gen16b-s390x-cpu,drawer-id=1,book-id=1,socket-id=2,core-id=1 > > or > > .. code-block:: bash > > -device gen16b-s390x-cpu,core-id=1,dedicated=true > > If none of the tree attributes (drawer, book, sockets), are specified > for the ``-device`` argument, like for all CPUs defined with the ``-smp`` > command argument the topology tree attributes will be set by simply > adding the CPUs to the topology based on the core-id. > > QEMU will not try to resolve collisions and will report an error if the > CPU topology defined explicitly or implicitly on a ``-device`` > argument collides with the definition of a CPU implicitly defined > on the ``-smp`` argument. > > When the topology modifier attributes are not defined for the > ``-device`` command argument they takes following default values: > > - dedicated: ``false`` > - entitlement: ``medium`` > > > Hot plug > ++++++++ > > New CPUs can be plugged using the device_add hmp command as in: > > .. code-block:: bash > > (qemu) device_add gen16b-s390x-cpu,core-id=9 > > The placement of the CPU is derived from the core-id as described above. > > The topology can of course also be fully defined: > > .. code-block:: bash > > (qemu) device_add gen16b-s390x-cpu,drawer-id=1,book-id=1,socket-id=2,core-id=1 >> >> > > Not your patch's fault, but let's get this in shape if we can. >> >> [...] >>
On Wed, 2023-09-20 at 13:11 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > > > From: Pierre Morel <pmorel@linux.ibm.com> > > > > S390 adds two new SMP levels, drawers and books to the CPU > > topology. > > S390 CPUs have specific topology features like dedication and > > entitlement. These indicate to the guest information on host > > vCPU scheduling and help the guest make better scheduling decisions. > > > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > > new file mode 100644 > > index 0000000000..e40421bb37 > > --- /dev/null > > +++ b/qapi/machine-common.json > > @@ -0,0 +1,21 @@ > > +# -*- 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 > > CPU entitlements > > Would someone reasonably familiar with S390 understand this? Because Well, someone familiar with s390 topology would, otherwise probably not tbh. > I'm not and I don't; I wonder what "a virtual CPU assuming an > entitlement" means. Basically, on s390x the OS is always running on some hypervisor. Even without KVM or z/VM you can slice up the machine, namely into logical partitions (LPARs). Therefore, there is a scheduling of virtual CPUs to the actual physical ones. "Entitlement" is a statement about how that scheduling works for a virtual CPU. The same concepts can then also be applied to KVM. > > > +# > > +# Since: 8.2 > > +## > > +{ 'enum': 'CpuS390Entitlement', > > + 'prefix': 'S390_CPU_ENTITLEMENT', > > + 'data': [ 'auto', 'low', 'medium', 'high' ] } > > [...] >
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > On Wed, 2023-09-20 at 13:11 +0200, Markus Armbruster wrote: >> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: >> >> > From: Pierre Morel <pmorel@linux.ibm.com> >> > >> > S390 adds two new SMP levels, drawers and books to the CPU >> > topology. >> > S390 CPUs have specific topology features like dedication and >> > entitlement. These indicate to the guest information on host >> > vCPU scheduling and help the guest make better scheduling decisions. >> > >> > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json >> > new file mode 100644 >> > index 0000000000..e40421bb37 >> > --- /dev/null >> > +++ b/qapi/machine-common.json >> > @@ -0,0 +1,21 @@ >> > +# -*- 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 >> >> CPU entitlements >> >> Would someone reasonably familiar with S390 understand this? Because > > Well, someone familiar with s390 topology would, otherwise probably not tbh. Good enough, I guess. >> I'm not and I don't; I wonder what "a virtual CPU assuming an >> entitlement" means. > > Basically, on s390x the OS is always running on some hypervisor. > Even without KVM or z/VM you can slice up the machine, namely into logical > partitions (LPARs). Therefore, there is a scheduling of virtual CPUs to the > actual physical ones. "Entitlement" is a statement about how that scheduling > works for a virtual CPU. The same concepts can then also be applied to KVM. Thanks! [...]
On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > > > From: Pierre Morel <pmorel@linux.ibm.com> > > > > S390 adds two new SMP levels, drawers and books to the CPU > > topology. > > S390 CPUs have specific topology features like dedication and > > entitlement. These indicate to the guest information on host > > vCPU scheduling and help the guest make better scheduling decisions. > > > > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > --- > > qapi/machine-common.json | 21 +++++++++++++ > > qapi/machine.json | 19 ++++++++++-- > > include/hw/boards.h | 10 +++++- > > include/hw/qdev-properties-system.h | 4 +++ > > target/s390x/cpu.h | 6 ++++ > > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- > > hw/core/machine.c | 4 +++ > > hw/core/qdev-properties-system.c | 13 ++++++++ > > hw/s390x/s390-virtio-ccw.c | 4 +++ > > softmmu/vl.c | 6 ++++ > > target/s390x/cpu.c | 7 +++++ > > qapi/meson.build | 1 + > > qemu-options.hx | 7 +++-- > > 13 files changed, 137 insertions(+), 13 deletions(-) > > create mode 100644 qapi/machine-common.json > > > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > > new file mode 100644 > > index 0000000000..e40421bb37 > > --- /dev/null > > +++ b/qapi/machine-common.json > > Why do you need a separate QAPI sub-module? > > > @@ -0,0 +1,21 @@ > > +# -*- 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.2 > > +## > > +{ 'enum': 'CpuS390Entitlement', > > + 'prefix': 'S390_CPU_ENTITLEMENT', > > + 'data': [ 'auto', 'low', 'medium', 'high' ] } > > diff --git a/qapi/machine.json b/qapi/machine.json > > index a08b6576ca..a63cb951d2 100644 > > --- a/qapi/machine.json > > +++ b/qapi/machine.json > > @@ -9,6 +9,7 @@ > ## > # = Machines > > ## > > > > { 'include': 'common.json' } > > +{ 'include': 'machine-common.json' } > > Section structure is borked :) > > Existing section "Machine" now ends at the new "Machines S390 data > types" you pull in here. The contents of below moves from "Machines" to > "Machines S390 data types". > > Before I explain how to avoid this, I'd like to understand why we need a > new sub-module. Should I just move the include statements above the section header? I assume I could also include it in qapi-schema.json before the machine.json include and that that is the reason we don't have the same problem with e.g. migration.json. But just moving the includes seems cleaner. [...]
Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: > On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: >> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes: >> >> > From: Pierre Morel <pmorel@linux.ibm.com> >> > >> > S390 adds two new SMP levels, drawers and books to the CPU >> > topology. >> > S390 CPUs have specific topology features like dedication and >> > entitlement. These indicate to the guest information on host >> > vCPU scheduling and help the guest make better scheduling decisions. >> > >> > 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: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >> > --- >> > qapi/machine-common.json | 21 +++++++++++++ >> > qapi/machine.json | 19 ++++++++++-- >> > include/hw/boards.h | 10 +++++- >> > include/hw/qdev-properties-system.h | 4 +++ >> > target/s390x/cpu.h | 6 ++++ >> > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++----- >> > hw/core/machine.c | 4 +++ >> > hw/core/qdev-properties-system.c | 13 ++++++++ >> > hw/s390x/s390-virtio-ccw.c | 4 +++ >> > softmmu/vl.c | 6 ++++ >> > target/s390x/cpu.c | 7 +++++ >> > qapi/meson.build | 1 + >> > qemu-options.hx | 7 +++-- >> > 13 files changed, 137 insertions(+), 13 deletions(-) >> > create mode 100644 qapi/machine-common.json >> > >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json >> > new file mode 100644 >> > index 0000000000..e40421bb37 >> > --- /dev/null >> > +++ b/qapi/machine-common.json >> >> Why do you need a separate QAPI sub-module? >> >> > @@ -0,0 +1,21 @@ >> > +# -*- 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.2 >> > +## >> > +{ 'enum': 'CpuS390Entitlement', >> > + 'prefix': 'S390_CPU_ENTITLEMENT', >> > + 'data': [ 'auto', 'low', 'medium', 'high' ] } >> > diff --git a/qapi/machine.json b/qapi/machine.json >> > index a08b6576ca..a63cb951d2 100644 >> > --- a/qapi/machine.json >> > +++ b/qapi/machine.json >> > @@ -9,6 +9,7 @@ >> ## >> # = Machines >> > ## >> > >> > { 'include': 'common.json' } >> > +{ 'include': 'machine-common.json' } >> >> Section structure is borked :) >> >> Existing section "Machine" now ends at the new "Machines S390 data >> types" you pull in here. The contents of below moves from "Machines" to >> "Machines S390 data types". >> >> Before I explain how to avoid this, I'd like to understand why we need a >> new sub-module. > > Should I just move the include statements above the section header? > I assume I could also include it in qapi-schema.json before the machine.json > include and that that is the reason we don't have > the same problem with e.g. migration.json. > But just moving the includes seems cleaner. qapi-schema.json should include all sub-modules in the order desired for the manual. Do double-check the generated manual's section structure when adding sections or moving include directives.
diff --git a/qapi/machine-common.json b/qapi/machine-common.json new file mode 100644 index 0000000000..e40421bb37 --- /dev/null +++ b/qapi/machine-common.json @@ -0,0 +1,21 @@ +# -*- 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.2 +## +{ 'enum': 'CpuS390Entitlement', + 'prefix': 'S390_CPU_ENTITLEMENT', + 'data': [ 'auto', 'low', 'medium', 'high' ] } diff --git a/qapi/machine.json b/qapi/machine.json index a08b6576ca..a63cb951d2 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -9,6 +9,7 @@ ## { 'include': 'common.json' } +{ 'include': 'machine-common.json' } ## # @SysEmuTarget: @@ -71,7 +72,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 @@ -901,7 +902,11 @@ # # @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.2) +# +# @book-id: book number within drawer/node/board the CPU belongs to (since 8.2) +# +# @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) # @@ -912,7 +917,7 @@ # # @thread-id: thread number within core the CPU belongs to # -# Note: currently there are 6 properties that could be present but +# 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 sync with the @@ -922,6 +927,8 @@ ## { 'struct': 'CpuInstanceProperties', 'data': { '*node-id': 'int', + '*drawer-id': 'int', + '*book-id': 'int', '*socket-id': 'int', '*die-id': 'int', '*cluster-id': 'int', @@ -1480,6 +1487,10 @@ # # @cpus: number of virtual CPUs in the virtual machine # +# @drawers: number of drawers in the CPU topology (since 8.2) +# +# @books: number of books in the CPU topology (since 8.2) +# # @sockets: number of sockets in the CPU topology # # @dies: number of dies per socket in the CPU topology @@ -1498,6 +1509,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 6c67af196a..6dcfc879eb 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -134,12 +134,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; /** @@ -310,7 +314,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 @@ -319,6 +325,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/qdev-properties-system.h b/include/hw/qdev-properties-system.h index 0ac327ae60..e4f8a13afc 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, _d) \ + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_cpus390entitlement, \ + CpuS390Entitlement) #endif diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 304029e57c..dfcc1aa1fc 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -30,6 +30,7 @@ #include "exec/cpu-defs.h" #include "qemu/cpu-float.h" #include "tcg/tcg_s390x.h" +#include "qapi/qapi-types-machine-common.h" #define ELF_MACHINE_UNAME "S390X" @@ -132,6 +133,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; + CpuS390Entitlement 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 0f4d9b6f7a..25019c91ee 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -33,6 +33,14 @@ static char *cpu_hierarchy_to_string(MachineState *ms) MachineClass *mc = MACHINE_GET_CLASS(ms); GString *s = g_string_new(NULL); + if (mc->smp_props.drawers_supported) { + g_string_append_printf(s, "drawers (%u) * ", ms->smp.drawers); + } + + if (mc->smp_props.books_supported) { + g_string_append_printf(s, "books (%u) * ", ms->smp.books); + } + g_string_append_printf(s, "sockets (%u)", ms->smp.sockets); if (mc->smp_props.dies_supported) { @@ -75,6 +83,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; @@ -87,6 +97,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) || @@ -113,6 +125,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; @@ -126,33 +151,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; @@ -163,7 +196,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 da699cf4e1..363ddafc89 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -843,6 +843,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, @@ -1108,6 +1110,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/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 6d5d43eda2..7f50ce9746 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -1147,3 +1147,16 @@ const PropertyInfo qdev_prop_uuid = { .set = set_uuid, .set_default_value = set_default_uuid_auto, }; + +/* --- s390 cpu entitlement policy --- */ + +QEMU_BUILD_BUG_ON(sizeof(CpuS390Entitlement) != sizeof(int)); + +const PropertyInfo qdev_prop_cpus390entitlement = { + .name = "CpuS390Entitlement", + .description = "low/medium (default)/high", + .enum_table = &CpuS390Entitlement_lookup, + .get = qdev_propinfo_get_enum, + .set = qdev_propinfo_set_enum, + .set_default_value = qdev_propinfo_set_default_value_enum, +}; diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index bfcf64d007..bbb001cb9e 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -733,6 +733,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; @@ -842,6 +844,8 @@ static void ccw_machine_8_1_class_options(MachineClass *mc) { ccw_machine_8_2_class_options(mc); compat_props_add(mc->compat_props, hw_compat_8_1, hw_compat_8_1_len); + mc->smp_props.drawers_supported = false; + mc->smp_props.books_supported = false; } DEFINE_CCW_MACHINE(8_1, "8.1", false); diff --git a/softmmu/vl.c b/softmmu/vl.c index 3db4fd2680..eb2450ac24 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 df167493c3..74405beb51 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" @@ -292,6 +293,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_CPUS390ENTITLEMENT("entitlement", S390CPU, env.entitlement, + S390_CPU_ENTITLEMENT_AUTO), #endif DEFINE_PROP_END_OF_LIST() }; diff --git a/qapi/meson.build b/qapi/meson.build index 60a668b343..f81a37565c 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -36,6 +36,7 @@ qapi_all_modules = [ 'error', 'introspect', 'job', + 'machine-common', 'machine', 'machine-target', 'migration', diff --git a/qemu-options.hx b/qemu-options.hx index 6be621c232..09889913c3 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -272,11 +272,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"