On Thu, 2023-10-12 at 13:02 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <n...@linux.ibm.com> writes: > > > From: Pierre Morel <pmo...@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, > > This is vague. Peeking at the patch, I can see it adds properties > "socket-id", "book-id", "drawer-id", "dedicated", and "entitlement" to > "s390x-cpu" objects. Suggest to spell that out here. > > > Add machine-common.json so we can later include it in > > machine-target.json also. > > > > Signed-off-by: Pierre Morel <pmo...@linux.ibm.com> > > Reviewed-by: Nina Schoetterl-Glausch <n...@linux.ibm.com> > > Co-developed-by: Nina Schoetterl-Glausch <n...@linux.ibm.com> > > Reviewed-by: Thomas Huth <th...@redhat.com> > > Signed-off-by: Nina Schoetterl-Glausch <n...@linux.ibm.com> > > --- > > MAINTAINERS | 1 + > > qapi/machine-common.json | 21 +++++++++++++ > > qapi/machine.json | 17 +++++++++- > > qapi/qapi-schema.json | 1 + > > 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 +++-- > > 15 files changed, 139 insertions(+), 11 deletions(-) > > create mode 100644 qapi/machine-common.json > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 81625f036b..3f6888aa86 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1775,6 +1775,7 @@ F: hw/core/null-machine.c > > F: hw/core/numa.c > > F: hw/cpu/cluster.c > > F: qapi/machine.json > > +F: qapi/machine-common.json > > F: qapi/machine-target.json > > F: include/hw/boards.h > > F: include/hw/core/cpu.h > > diff --git a/qapi/machine-common.json b/qapi/machine-common.json > > new file mode 100644 > > index 0000000000..fa6bd71d12
[...] > > ## > > # @SysEmuTarget: > > @@ -904,7 +905,13 @@ > > # > > # @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) > > # > # @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 > > So... > > * A thread can only be within a core > > * A core can only be within a cluster > > * A cluster can only be within a die > > * A die can only be within a socket > > * A socket can be within a book, node, or board > > * A book can be within a drawer, node, or board > > * A drawer can be within a node, or board > > * A node is a NUMA node > > * A board is what exactly? can we have more than one? is node always > within a/the board? Yeah, the description is confusing. > > Asked differently: what are the possible hierarchies of things? The way I understand things is: * Different architectures have different hierarchies, say 1. (thread, core, cluster, die, socket) 2. (thread, core, socket, book, drawer) We define a qemu artificial ordered super set (thread, core, cluster, die, socket, book, drawer) where architectures can choose a subset of, specifying that they support a certain level or not. Now if for example x86 wanted to support a book level between thread and core, we'd need to change a bunch of code and make things more complicated. The NUMA node-id maps a hierarchy tuple to a node, I don't think it's part of hierarchy itself. Now the question is how to document this. On s390x there is no cluster, so what does @core-id: core number within cluster the CPU belongs to mean? We could say, that within the qemu super set there is a virtual cluster of which there is one per die (and one die per socket). Or we rewrite the documentation to say @x-id: x number within the upper hierarchy container to account for the fact that the upper container is different on different architectures. > > > @@ -923,6 +930,8 @@ > > { 'struct': 'CpuInstanceProperties', > > # Keep these in sync with the properties device_add accepts > > 'data': { '*node-id': 'int', > > + '*drawer-id': 'int', > > + '*book-id': 'int', > > '*socket-id': 'int', > > '*die-id': 'int', > > '*cluster-id': 'int', > > @@ -1481,6 +1490,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 > > Total numer of sockets? Or number of sockets per whatever thing > contains sockets? The latter. I'll change this > > Same question for @books, @drawers, and @cpus. Same for the first two, total for @cpus. > > The documentation is less than clear before your patch; your patch > merely makes me look at it. We may decide that addressing the lack of > clarity is not your patch's job, and leave it for later. Yeah, same problem here around different architectures using different sub sets. > > > # > > # @dies: number of dies per socket in the CPU topology > > @@ -1499,6 +1512,8 @@ > > ## > > { 'struct': 'SMPConfiguration', 'data': { > > '*cpus': 'int', > > + '*drawers': 'int', > > + '*books': 'int', > > '*sockets': 'int', > > '*dies': 'int', > > '*clusters': 'int', [...]