Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Peter, Fabiano, I'd like to hear your opinion on the issue discussed
> below.
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> On 02/05/2024 13:22, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 01/05/2024 13:28, Avihai Horon wrote:
>>>> 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:
>>>>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>>>>>    hw/vfio/pci.c                 |  2 ++
>>>>>>    3 files changed, 47 insertions(+)
>>>>>>
>>>>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
>>>>>> vfio_device_mig_state state)
>>>>>>        }
>>>>>>    }
>>>>>>
>>>>>> +static VFIODeviceMigState
>>>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>>>>> +{
>>>>>> +    switch (state) {
>>>>>> +    case VFIO_DEVICE_STATE_STOP:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>>>>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>>>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>>>>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>>>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>>>>>> +    default:
>>>>>> +        g_assert_not_reached();
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>>>>>> +{
>>>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>>>> +    const char *id;
>>>>>> +    Object *obj;
>>>>>> +
>>>>>> +    if (!vbasedev->migration_events) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>> Shouldn't this leap frog migrate_events() capability instead of 
>>>>> introducing its
>>>>> vfio equivalent i.e.
>>>>>
>>>>>           if (!migrate_events()) {
>>>>>               return;
>>>>>           }
>>>>>
>>>>> ?
>>>>
>>>> I used a per VFIO device cap so the events can be fine tuned for each 
>>>> device
>>>> (maybe one device should send events while the other not).
>>>> This gives the most flexibility and I don't think it complicates the
>>>> configuration (one downside, though, is that it can't be enabled/disabled
>>>> dynamically during runtime).
>>>>
>>> Right.
>>>
>>>> I don't think events add much overhead, so if you prefer a global cap, I 
>>>> can
>>>> change it.
>>>> However, I'm not sure overloading the existing migrate_events() is valid?
>>>>
>>> migration 'events' capability just means we will have some migration events
>>> emited via QAPI monitor for: 1) general global status and 2) for each 
>>> migration
>>> pass (both with different event names=.
>>
>> Yes, it's already overloaded.
>>
>> In migration QAPI it says: "@events: generate events for each migration 
>> state change (since 2.4)".
>> This only refers to the MIGRATION event AFAIU.
>>
>> Later on (in QEMU 2.6), MIGRATION_PASS event was added and the events cap 
>> was overloaded for the first time (without changing @events description).
>>
>> Now we want to add yet another use for events capability, the VFIO migration 
>> state change events.
>>
>> I think what bothers me is the @events description, which is not accurate.
>> Maybe it should be changed to "@events: generate migration related events 
>> (since 2.4)"? However, I'm not sure if it's OK to do this.
>>
>>>   So the suggestion was just what feels a
>>> natural extension of that (...)
>>>
>>>>> Applications that don't understand the event string (migration related or 
>>>>> not)
>>>>> will just discard it (AIUI)
>>>
>>> (...) specially because of this as all these events have a different name.
>>>
>>> But overloading might not make sense for others IDK ... it was just a 
>>> suggestion
>>> :) not a strong preference
>>
>> Yes, I get your rationale.
>> I don't have a strong opinion either, so maybe let's see what other people 
>> think.

I don't see the need to tie this to the migration events
capability. Although there's overlap in the terms used, this seems more
like exposing a device feature via QEMU, then a migration event
per-se. The state changes also happen during moments unrelated to
migration (cover letter mentions start/stopping guest), so I assume we'd
like to keep those even if the management layer doesn't want to see
migration events.