Re: [PATCH v15 10/11] qapi/s390x/cpu topology: CPU_POLARITY_CHANGE qapi event

[Markus Armbruster]

Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:

> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
>> When the guest asks to change the polarity this change
>> is forwarded to the admin using QAPI.
>> The admin is supposed to take according decisions concerning
>> CPU provisioning.
>> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  qapi/machine-target.json | 30 ++++++++++++++++++++++++++++++
>>  hw/s390x/cpu-topology.c  |  2 ++
>>  2 files changed, 32 insertions(+)
>> 
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index 58df0f5061..5883c3b020 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -371,3 +371,33 @@
>>    },
>>    'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
>>  }
>> +
>> +##
>> +# @CPU_POLARITY_CHANGE:
>> +#
>> +# Emitted when the guest asks to change the polarity.
>> +#
>> +# @polarity: polarity specified by the guest
>> +#
>> +# The guest can tell the host (via the PTF instruction) whether the
>> +# CPUs should be provisioned using horizontal or vertical polarity.
>> +#
>> +# On horizontal polarity the host is expected to provision all vCPUs
>> +# equally.
>> +# On vertical polarity the host can provision each vCPU differently.
>> +# The guest will get information on the details of the provisioning
>> +# the next time it uses the STSI(15) instruction.
>> +#
>> +# Since: 8.0
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "CPU_POLARITY_CHANGE",
>> +#      "data": { "polarity": 0 },
>> +#      "timestamp": { "seconds": 1401385907, "microseconds": 422329 } }
>> +#
>> +##
>> +{ 'event': 'CPU_POLARITY_CHANGE',
>> +  'data': { 'polarity': 'int' },
>> +  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM'] }
>
> I wonder if you should depend on CONFIG_KVM or not. If tcg gets topology
> support it will use the same event and right now it would just never be 
> emitted.
> On the other hand it's more conservative this way.

TCG vs. KVM should be as transparent as we can make it.

If only KVM can get into the state where the event is emitted, say
because the state is only possible with features only KVM supports, then
making the event conditional on KVM makes sense.  Of course, when
another accelerator acquires these features, we need to emit the event
there as well, which will involve adjusting the condition.

> I also wonder if you should add 'feature' : [ 'unstable' ].
> On the upside, it would mark the event as unstable, but I don't know what the
> consequences are exactly.

docs/devel/qapi-code-gen.rst:

    Special features
    ~~~~~~~~~~~~~~~~

    Feature "deprecated" marks a command, event, enum value, or struct
    member as deprecated.  It is not supported elsewhere so far.
    Interfaces so marked may be withdrawn in future releases in accordance
    with QEMU's deprecation policy.

    Feature "unstable" marks a command, event, enum value, or struct
    member as unstable.  It is not supported elsewhere so far.  Interfaces
    so marked may be withdrawn or changed incompatibly in future releases.

See also -compat parameters unstable-input, unstable-output, both
intended for "testing the future".

> Also I guess one can remove qemu events without breaking backwards 
> compatibility,
> since they just won't be emitted? Unless I guess you specify that a event must
> occur under certain situations and the client waits on it?

Events are part of the interface just like command returns are.  Not
emitting an event in a situation where it was emitted before can easily
break things.  Only when the situation is no longer possible, the event
can be removed safely.

Questions?

[...]