Nina Schoetterl-Glausch <n...@linux.ibm.com> writes:
> 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.
Right.
> 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.
Perhaps a structure like this could work.
1. Define the most general hierarchy.
2. Explain the actual hierarchy is a subset.
3. Explain the members as "number within container", as you proposed.
Only works if a superset of all the actual hierarchies exists, and is
sufficiently easy explain.
>> > @@ -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',
>
> [...]
Thanks!
