Nina Schoetterl-Glausch <n...@linux.ibm.com> writes:
> On Tue, 2023-09-19 at 14:47 +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,
>> >
>> > 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>
>> > Signed-off-by: Nina Schoetterl-Glausch <n...@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.ca...@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.
[...]
