Re: [PATCH v25 02/21] CPU topology: extend with s390 specifics

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Thu, 2023-10-12 at 13:02 +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,
> 
> 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 <pmorel@linux.ibm.com>
> > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@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',

[...]