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

2023-09-25 Thread Nina Schoetterl-Glausch
On Wed, 2023-09-20 at 13:36 +0200, Markus Armbruster wrote:
> Nina Schoetterl-Glausch  writes:
> 
> > From: Pierre Morel 
> > 
> > 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 
> > Reviewed-by: Nina Schoetterl-Glausch 
> > Co-developed-by: Nina Schoetterl-Glausch 
> > Signed-off-by: Nina Schoetterl-Glausch 
> > ---
> >  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' ] }
> > +}
> 
> [...]
> 




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

2023-09-20 Thread Markus Armbruster
Nina Schoetterl-Glausch  writes:

> From: Pierre Morel 
>
> 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 
> Reviewed-by: Nina Schoetterl-Glausch 
> Co-developed-by: Nina Schoetterl-Glausch 
> Signed-off-by: Nina Schoetterl-Glausch 
> ---
>  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
> @@ -4,6 +4,8 @@
>  # This work is licensed under the terms of the GNU GPL, version 2 or later.
>  # See the COPYING file in the top-level directory.
>  
> +{ 'include': 'machine-common.json' }
> +
>  ##
>  # @CpuModelInfo:
>  #
> @@ -375,3 +377,38 @@
>'data': [ 'horizontal', 'vertical' ],
>  'if': 'TARGET_S390X'
>  }
> +
> +##
> +# @set-cpu-topology:

Move the overview here from [*] below.

> +#
> +# @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

Separate the members with blank lines, please.

The doc generator adds optional automatically, so this will come out
like

"socket-id": "int" (optional)
   optional destination socket where to move the vCPU

"book-id": "int" (optional)
   optional destination book where to move the vCPU

"drawer-id": "int" (optional)
   optional destination drawer where to move the vCPU

"entitlement": "CpuS390Entitlement" (optional)
   optional entitlement

"dedicated": "boolean" (optional)
   optional, if the vCPU is dedicated to a real CPU

Drop the optional from the text.

Suggest

   # @dedicated: whether the vCPU is dedicated to a real CPU

(whatever that may mean; I'm an S390 noob)

> +#
> +# Features:

Another blank line, please.

> +# @unstable: This command may still be modified.
> +#

[*] The overview:

> +# 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?

> +#
> +# 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?

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

[...]