Re: [PATCH v16 08/11] qapi/s390x/cpu topology: set-cpu-topology monitor command

[Thomas Huth]

On 27/02/2023 11.57, Pierre Morel wrote:
> On 2/24/23 18:15, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-02-22 at 15:21 +0100, Pierre Morel wrote:
> > > The modification of the CPU attributes are done through a monitor
> > > command.
> > >
> > > It allows to move the core inside the topology tree to optimize
> > > the cache usage in the case the host's hypervisor previously
> > > moved the CPU.
> > >
> > > The same command allows to modify the CPU attributes modifiers
> > > like polarization entitlement and the dedicated attribute to notify
> > > the guest if the host admin modified scheduling or dedication of a vCPU.
> > >
> > > With this knowledge the guest has the possibility to optimize the
> > > usage of the vCPUs.
> > >
> > > The command has a feature unstable for the moment.
> > >
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   qapi/machine-target.json |  35 +++++++++
> > >   include/monitor/hmp.h    |   1 +
> > >   hw/s390x/cpu-topology.c  | 154 +++++++++++++++++++++++++++++++++++++++
> > >   hmp-commands.hx          |  17 +++++
> > >   4 files changed, 207 insertions(+)
> > >
> > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > > index a52cc32f09..baa9d273cf 100644
> > > --- a/qapi/machine-target.json
> > > +++ b/qapi/machine-target.json
> > > @@ -354,3 +354,38 @@
> > >   { 'enum': 'CpuS390Polarization',
> > >     'prefix': 'S390_CPU_POLARIZATION',
> > >     'data': [ 'horizontal', 'vertical' ] }
> > > +
> > > +##
> > > +# @set-cpu-topology:
> > > +#
> > > +# @core-id: the vCPU ID to be moved
> > > +# @socket-id: optional destination socket where to move the vCPU
> > > +# @book-id: optional destination book where to move the vCPU
> > > +# @drawer-id: optional destination drawer where to move the vCPU
> > > +# @entitlement: optional entitlement
> > > +# @dedicated: optional, if the vCPU is dedicated to a real CPU
> > > +#
> > > +# Features:
> > > +# @unstable: This command may still be modified.
> > > +#
> > > +# Modifies the topology by moving the CPU inside the topology
> > > +# tree or by changing a modifier attribute of a CPU.
> > > +# Default value for optional parameter is the current value
> > > +# used by the CPU.
> > > +#
> > > +# Returns: Nothing on success, the reason on failure.
> > > +#
> > > +# Since: 8.0
> > > +##
> > > +{ 'command': 'set-cpu-topology',
> > > +  'data': {
> > > +      'core-id': 'uint16',
> > > +      '*socket-id': 'uint16',
> > > +      '*book-id': 'uint16',
> > > +      '*drawer-id': 'uint16',
> > > +      '*entitlement': 'str',
> > How about you add a machine-common.json and define CpuS390Entitlement there,
> > and then include it from both machine.json and machine-target.json?
> >
> > Then you can declare it as CpuS390Entitlement and don't need to do the 
> > parsing
> > manually.
> >
> > You could also put it in common.json, but that seems a bit too generic.
> >
> > Anyone have objections?
>
> Seems Thomas has questions, I wait until every body agree or not agree.

I'd be fine with such a change if it works ... I just got no clue whether it 
works or not, so you've got to try it, I guess.

But I think I'd rather avoid naming the file "machine-common.json" ... 
"machine.json" is already supposed to be the common code that can be shared 
between all targets, so having a "machine-common.json" file would be super 
confusing, I think.

OTOH, what's the reason again for having CpuS390Entitlement in machine.json? 
Couldn't it be moved to machine-target.json instead?

 Thomas