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

[Markus Armbruster <armbru@redhat.com>]

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?

>>> +
>>
>> 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.

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