Re: [PATCH v2 2/3] vfio/migration: Emit VFIO migration QAPI event

["Cédric Le Goater" <clg@redhat.com>]

On 5/13/24 16:34, Avihai Horon wrote:
> 
> On 13/05/2024 17:01, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/9/24 11:09, Avihai Horon wrote:
>>> Emit VFIO migration QAPI event when a VFIO device changes its migration
>>> state. This can be used by management applications to get updates on the
>>> current state of the VFIO device for their own purposes.
>>>
>>> A new per VFIO device capability, "migration-events", is added so events
>>> can be enabled only for the required devices. It is disabled by default.
>>>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> ---
>>>   include/hw/vfio/vfio-common.h |  1 +
>>>   hw/vfio/migration.c           | 56 +++++++++++++++++++++++++++++++++--
>>>   hw/vfio/pci.c                 |  2 ++
>>>   3 files changed, 56 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index b9da6c08ef..3ec5f2425e 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -115,6 +115,7 @@ typedef struct VFIODevice {
>>>       bool no_mmap;
>>>       bool ram_block_discard_allowed;
>>>       OnOffAuto enable_migration;
>>> +    bool migration_events;
>>>       VFIODeviceOps *ops;
>>>       unsigned int num_irqs;
>>>       unsigned int num_regions;
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 06ae40969b..5a359c4c78 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -24,6 +24,7 @@
>>>   #include "migration/register.h"
>>>   #include "migration/blocker.h"
>>>   #include "qapi/error.h"
>>> +#include "qapi/qapi-events-vfio.h"
>>>   #include "exec/ramlist.h"
>>>   #include "exec/ram_addr.h"
>>>   #include "pci.h"
>>> @@ -80,6 +81,55 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>>       }
>>>   }
>>>
>>> +static VfioMigrationState
>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>> +{
>>> +    switch (state) {
>>> +    case VFIO_DEVICE_STATE_STOP:
>>> +        return QAPI_VFIO_MIGRATION_STATE_STOP;
>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING;
>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>> +        return QAPI_VFIO_MIGRATION_STATE_STOP_COPY;
>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>> +        return QAPI_VFIO_MIGRATION_STATE_RESUMING;
>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P;
>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY;
>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P;
>>> +    default:
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
>>> +
>>> +static void vfio_migration_send_event(VFIODevice *vbasedev)
>>> +{
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +    DeviceState *dev = vbasedev->dev;
>>> +    g_autofree char *qom_path = NULL;
>>> +    Object *obj;
>>> +
>>> +    if (!vbasedev->migration_events) {
>>> +        return;
>>> +    }
>>
>> I would add an assert on vbasedev->ops->vfio_get_object
>>
>>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>>
>> and another assert on obj.
> 
> vfio_migration_init() already checks these:
> 
>      if (!vbasedev->ops->vfio_get_object) {
>          return -EINVAL;
>      }
> 
>      obj = vbasedev->ops->vfio_get_object(vbasedev);
>      if (!obj) {
>          return -EINVAL;
>      }
> 
> Do you think these checks in migration init are enough?

I am sure they are today. These extra asserts are to avoid issues if
the code is moved around or if anyone finds inspiration by reading
vfio_migration_send_event().

Thanks,

C.



> 
>>
>>> +    qom_path = object_get_canonical_path(obj);
>>> +
>>> +    qapi_event_send_vfio_migration(
>>> +        dev->id, qom_path, mig_state_to_qapi_state(migration->device_state));
>>> +}
>>> +
>>> +static void set_state(VFIODevice *vbasedev, enum vfio_device_mig_state state)
>>
>> to avoid the conflict with vfio_migration_set_state(), let's call it :
>> vfio_migration_set_device_state() ? We want a 'vfio_migration_' prefix.
> 
> Sure, I will rename to that.
> 
> Thanks.
> 
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>
>>> +{
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +
>>> +    migration->device_state = state;
>>> +    vfio_migration_send_event(vbasedev);
>>> +}
>>> +
>>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>                                       enum vfio_device_mig_state new_state,
>>>                                       enum vfio_device_mig_state recover_state)
>>> @@ -125,12 +175,12 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>               goto reset_device;
>>>           }
>>>
>>> -        migration->device_state = recover_state;
>>> +        set_state(vbasedev, recover_state);
>>>
>>>           return ret;
>>>       }
>>>
>>> -    migration->device_state = new_state;
>>> +    set_state(vbasedev, new_state);
>>>       if (mig_state->data_fd != -1) {
>>>           if (migration->data_fd != -1) {
>>>               /*
>>> @@ -156,7 +206,7 @@ reset_device:
>>>                    strerror(errno));
>>>       }
>>>
>>> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>>> +    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>>>
>>>       return ret;
>>>   }
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 64780d1b79..8840602c50 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3362,6 +3362,8 @@ static Property vfio_pci_dev_properties[] = {
>>>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>>>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>>                               vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>>> +    DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>>> +                     vbasedev.migration_events, false),
>>>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
>>>       DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>>>                        vbasedev.ram_block_discard_allowed, false),
>>
>