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

[Peter Xu <peterx@redhat.com>]

On Mon, May 06, 2024 at 11:38:00AM -0300, Fabiano Rosas wrote:
> 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.

Yeh makes sense to me to keep that per-device flag, as it's not a generic
migration state change.

  @events: generate events for each migration state change (since 2.4)

The other option is to emit event only if "migrate_events() &&
vfio_dev_send_event", but that sounds like an overkill to overload "events"
cap, and doesn't look like bring anything better than just keeping them
separate.

While at it, another trivial comment is maybe it's nice to have a helper to
both update the vfio migration state, plus emitting events when necessary.

Thanks,

-- 
Peter Xu