Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon <avihaih@nvidia.com>]

On 06/05/2024 7:52, Markus Armbruster wrote:
> External email: Use caution opening links or attachments
>
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> On 01/05/2024 14:50, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>> Add a new QAPI event for VFIO device migration state change. This event
>>>> will be emitted when a VFIO device changes its migration state, for
>>>> example, during migration or when stopping/starting the guest.
>>>>
>>>> This event can be used by management applications to get updates on the
>>>> current state of the VFIO device for their own purposes.
>>>>
>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> ---
>>>>    MAINTAINERS           |  1 +
>>>>    qapi/qapi-schema.json |  1 +
>>>>    qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>>>>    qapi/meson.build      |  1 +
>>>>    4 files changed, 64 insertions(+)
>>>>    create mode 100644 qapi/vfio.json
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 302b6fd00c..ef58a39d36 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -2159,6 +2159,7 @@ F: hw/vfio/*
>>>>   F: include/hw/vfio/
>>>>   F: docs/igd-assign.txt
>>>>   F: docs/devel/migration/vfio.rst
>>>> +F: qapi/vfio.json
>>>>
>>>>    vfio-ccw
>>>>    M: Eric Farman <farman@linux.ibm.com>
>>>> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>>>> index 5e33da7228..b1581988e4 100644
>>>> --- a/qapi/qapi-schema.json
>>>> +++ b/qapi/qapi-schema.json
>>>> @@ -78,5 +78,6 @@
>>>>   { 'include': 'pci.json' }
>>>>   { 'include': 'stats.json' }
>>>>   { 'include': 'virtio.json' }
>>>> +{ 'include': 'vfio.json' }
>>>>   { 'include': 'cryptodev.json' }
>>>>   { 'include': 'cxl.json' }
>>>> diff --git a/qapi/vfio.json b/qapi/vfio.json
>>>> new file mode 100644
>>>> index 0000000000..a38f26bccd
>>>> --- /dev/null
>>>> +++ b/qapi/vfio.json
>>>> @@ -0,0 +1,61 @@
>>>> +# -*- Mode: Python -*-
>>>> +# vim: filetype=python
>>>> +#
>>>> +
>>>> +##
>>>> +# = VFIO devices
>>>> +##
>>>> +
>>>> +##
>>>> +# @VFIODeviceMigState:
>>>> +#
>>>> +# An enumeration of the VFIO device migration states.
>>>> +#
>>>> +# @stop: The device is stopped.
>>>> +#
>>>> +# @running: The device is running.
>>>> +#
>>>> +# @stop-copy: The device is stopped and its internal state is available
>>>> +#     for reading.
>>>> +#
>>>> +# @resuming: The device is stopped and its internal state is available
>>>> +#     for writing.
>>>> +#
>>>> +# @running-p2p: The device is running in the P2P quiescent state.
>>>> +#
>>>> +# @pre-copy: The device is running, tracking its internal state and its
>>>> +#     internal state is available for reading.
>>>> +#
>>>> +# @pre-copy-p2p: The device is running in the P2P quiescent state,
>>>> +#     tracking its internal state and its internal state is available
>>>> +#     for reading.
>>>> +#
>>>> +# Since: 9.1
>>>> +##
>>>> +{ 'enum': 'VFIODeviceMigState',
>>>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>>>> +            'pre-copy', 'pre-copy-p2p' ],
>>>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
> Without 'prefix', you get VFIO_DEVICE_MIG_STATE_STOP and so forth.  Why
> do you need a QAPI_ prefix?

VFIO uAPI already defines enum vfio_device_mig_state and its values 
VFIO_DEVICE_STATE_STOP, VFIO_DEVICE_STATE_RUNNING, etc.

I wanted to emphasize these are QAPI entities.

>
>>>> +
>>> Considering MIG can also be interpreted as Multi Instance GPU elsewhere
>>> unrelated to this shouldn't we be explicit here? i.e.
>>>
>>>           VFIO_DEVICE_MIGRATION_STATE
>>>
>>> ... to avoid ambiguiosity.
>> I used mig to avoid long names, but I don't mind changing it to migration if that's clearer.
>>
>> Thanks.
> We generally avoid abbreviations in QAPI/QMP.
>
> The event that reports general migration state change is called
> MIGRATION, and its data type MigrationStatus.
>
> We could call this one VFIO_MIGRATION, and its data type
> VfioMigrationStatus.

Sounds good, but how about VFIOMigrationState (I'd like it to relate to 
the VFIO state)?

>
>>>> +##
>>>> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
>>>> +#
>>>> +# This event is emitted when a VFIO device migration state is changed.
>>>> +#
>>>> +# @device-id: The id of the VFIO device (final component of QOM path).
>>>> +#
>>>> +# @device-state: The new changed device migration state.
>>>> +#
>>>> +# Since: 9.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
>>>> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
>>>> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
>>>> +##
>>>> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
>>>> +  'data': {
>>>> +      'device-id': 'str',
>>>> +      'device-state': 'VFIODeviceMigState'
>>>> +  } }
>>>> diff --git a/qapi/meson.build b/qapi/meson.build
>>>> index c92af6e063..e7bc54e5d0 100644
>>>> --- a/qapi/meson.build
>>>> +++ b/qapi/meson.build
>>>> @@ -52,6 +52,7 @@ qapi_all_modules = [
>>>>     'stats',
>>>>     'trace',
>>>>     'transaction',
>>>> +  'vfio',
>>>>     'virtio',
>>>>     'yank',
>>>>   ]
> The new code is in the intersection of VFIO and migration.  Putting it
> into new vfio.json instead of existing migration.json lets you add it to
> MAINTAINERS section VFIO instead of Migration.  Up to the maintainers
> involved.

Sure. I will keep it as is unless someone thinks otherwise.

Thanks.