From: Avihai Horon <avihaih@nvidia.com>
To: Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
Cc: "Markus Armbruster" <armbru@redhat.com>,
"Joao Martins" <joao.m.martins@oracle.com>,
"Alex Williamson" <alex.williamson@redhat.com>,
"Cédric Le Goater" <clg@redhat.com>,
"Michael Roth" <michael.roth@amd.com>,
"Eric Blake" <eblake@redhat.com>,
"Maor Gottlieb" <maorg@nvidia.com>,
qemu-devel@nongnu.org
Subject: Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event
Date: Tue, 7 May 2024 10:47:13 +0300 [thread overview]
Message-ID: <23edb44a-7147-443e-b0e3-2a832aff5aa4@nvidia.com> (raw)
In-Reply-To: <ZjjyPESK-YC-XtFO@x1n>
On 06/05/2024 18:07, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> 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.
I agree, so I will keep it a per-device flag.
> 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.
I think vfio_migration_set_state() does exactly that, no?
Thanks.
next prev parent reply other threads:[~2024-05-07 7:48 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 5:16 [PATCH 0/3] qapi/vfio: Add VFIO device migration state change QAPI event Avihai Horon
2024-04-30 5:16 ` [PATCH 1/3] " Avihai Horon
2024-05-01 11:50 ` Joao Martins
2024-05-01 12:08 ` Avihai Horon
2024-05-06 4:52 ` Markus Armbruster
2024-05-06 10:07 ` Avihai Horon
2024-05-06 10:36 ` Markus Armbruster
2024-05-06 10:57 ` Avihai Horon
2024-05-02 11:19 ` Markus Armbruster
2024-05-05 7:48 ` Avihai Horon
2024-05-06 4:35 ` Markus Armbruster
2024-05-06 9:59 ` Avihai Horon
2024-04-30 5:16 ` [PATCH 2/3] vfio/migration: Emit " Avihai Horon
2024-05-01 11:50 ` Joao Martins
2024-05-01 12:28 ` Avihai Horon
2024-05-02 10:22 ` Joao Martins
2024-05-05 7:28 ` Avihai Horon
2024-05-06 4:56 ` Markus Armbruster
2024-05-06 14:38 ` Fabiano Rosas
2024-05-06 15:07 ` Peter Xu
2024-05-07 7:47 ` Avihai Horon [this message]
2024-05-07 15:51 ` Peter Xu
2024-05-07 16:21 ` Avihai Horon
2024-04-30 5:16 ` [PATCH 3/3] vfio/migration: Don't emit STOP_COPY state change event twice Avihai Horon
2024-05-06 14:39 ` Cédric Le Goater
2024-05-06 15:01 ` Avihai Horon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=23edb44a-7147-443e-b0e3-2a832aff5aa4@nvidia.com \
--to=avihaih@nvidia.com \
--cc=alex.williamson@redhat.com \
--cc=armbru@redhat.com \
--cc=clg@redhat.com \
--cc=eblake@redhat.com \
--cc=farosas@suse.de \
--cc=joao.m.martins@oracle.com \
--cc=maorg@nvidia.com \
--cc=michael.roth@amd.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).