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

[Thomas Huth <thuth@redhat.com>]

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