Re: [PATCH v23 08/20] qapi/s390x/cpu topology: set-cpu-topology qmp command

[Nina Schoetterl-Glausch]

On Wed, 2023-09-20 at 13:36 +0200, Markus Armbruster wrote:
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
> 
> > From: Pierre Morel <pmorel@linux.ibm.com>
> > 
> > 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>
> > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >  qapi/machine-target.json |  37 +++++++++++
> >  hw/s390x/cpu-topology.c  | 132 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index 0d45a590ce..e47a252bd9 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json

[...]

> > +# 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.
> 
> So, anything absent will not be changed.  Maybe that's a clearer way to
> put it.  What do you think?

Yes.

> 
> > +#
> > +# Returns: Nothing on success, the reason on failure.
> > +#
> > +# Since: 8.2
> > +##
> > +{ 'command': 'set-cpu-topology',
> > +  'data': {
> > +      'core-id': 'uint16',
> > +      '*socket-id': 'uint16',
> > +      '*book-id': 'uint16',
> > +      '*drawer-id': 'uint16',
> 
> CpuInstanceProperties uses 'int' for these.  Any particular reason for
> the difference?

unsigned -> no need to check if >0
16bit is also the width the hardware uses for these values on s390.

> > +      '*entitlement': 'CpuS390Entitlement',
> > +      '*dedicated': 'bool'
> > +  },
> > +  'features': [ 'unstable' ],
> > +  'if': { 'all': [ 'TARGET_S390X' , 'CONFIG_KVM' ] }
> > +}
> 
> [...]
>