| Message ID | 20240430051621.19597-4-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi/vfio: Add VFIO device migration state change QAPI event | expand |
Hello Avihai, On 4/30/24 07:16, Avihai Horon wrote: > When migrating a VFIO device that supports pre-copy, it is transitioned > to STOP_COPY twice: once in vfio_vmstate_change() and second time in > vfio_save_complete_precopy(). > > The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op > transition. However, with the newly added migration state change QAPI > event, the STOP_COPY state change event is undesirably emitted twice. > > Prevent this by conditionally transitioning to STOP_COPY state in > vfio_save_complete_precopy(). > > Note that the STOP_COPY transition in vfio_save_complete_precopy() is > essential for VFIO devices that don't support pre-copy, for migrating an > already stopped guest and for snapshots. > > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > --- > hw/vfio/migration.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 6bbccf6545..30a2b2ea74 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -591,14 +591,17 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) > static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) > { > VFIODevice *vbasedev = opaque; > + VFIOMigration *migration = vbasedev->migration; > ssize_t data_size; > int ret; > > /* We reach here with device state STOP or STOP_COPY only */ > - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY, > - VFIO_DEVICE_STATE_STOP); > - if (ret) { > - return ret; > + if (migration->device_state == VFIO_DEVICE_STATE_STOP) { Shouldn't we handle no-op transitions in vfio_migration_set_state() instead ? Thanks, C. > + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY, > + VFIO_DEVICE_STATE_STOP); > + if (ret) { > + return ret; > + } > } > > do {
On 06/05/2024 17:39, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > Hello Avihai, > > On 4/30/24 07:16, Avihai Horon wrote: >> When migrating a VFIO device that supports pre-copy, it is transitioned >> to STOP_COPY twice: once in vfio_vmstate_change() and second time in >> vfio_save_complete_precopy(). >> >> The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op >> transition. However, with the newly added migration state change QAPI >> event, the STOP_COPY state change event is undesirably emitted twice. >> >> Prevent this by conditionally transitioning to STOP_COPY state in >> vfio_save_complete_precopy(). >> >> Note that the STOP_COPY transition in vfio_save_complete_precopy() is >> essential for VFIO devices that don't support pre-copy, for migrating an >> already stopped guest and for snapshots. >> >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >> --- >> hw/vfio/migration.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index 6bbccf6545..30a2b2ea74 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -591,14 +591,17 @@ static int vfio_save_iterate(QEMUFile *f, void >> *opaque) >> static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> + VFIOMigration *migration = vbasedev->migration; >> ssize_t data_size; >> int ret; >> >> /* We reach here with device state STOP or STOP_COPY only */ >> - ret = vfio_migration_set_state(vbasedev, >> VFIO_DEVICE_STATE_STOP_COPY, >> - VFIO_DEVICE_STATE_STOP); >> - if (ret) { >> - return ret; >> + if (migration->device_state == VFIO_DEVICE_STATE_STOP) { > > Shouldn't we handle no-op transitions in vfio_migration_set_state() > instead ? > Yes, you are right. It's better to handle the general case in vfio_migration_set_state(). Will change it. Thanks. > Thanks, > > C. > > > >> + ret = vfio_migration_set_state(vbasedev, >> VFIO_DEVICE_STATE_STOP_COPY, >> + VFIO_DEVICE_STATE_STOP); >> + if (ret) { >> + return ret; >> + } >> } >> >> do { >
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 6bbccf6545..30a2b2ea74 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -591,14 +591,17 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) { VFIODevice *vbasedev = opaque; + VFIOMigration *migration = vbasedev->migration; ssize_t data_size; int ret; /* We reach here with device state STOP or STOP_COPY only */ - ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY, - VFIO_DEVICE_STATE_STOP); - if (ret) { - return ret; + if (migration->device_state == VFIO_DEVICE_STATE_STOP) { + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY, + VFIO_DEVICE_STATE_STOP); + if (ret) { + return ret; + } } do {
When migrating a VFIO device that supports pre-copy, it is transitioned to STOP_COPY twice: once in vfio_vmstate_change() and second time in vfio_save_complete_precopy(). The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op transition. However, with the newly added migration state change QAPI event, the STOP_COPY state change event is undesirably emitted twice. Prevent this by conditionally transitioning to STOP_COPY state in vfio_save_complete_precopy(). Note that the STOP_COPY transition in vfio_save_complete_precopy() is essential for VFIO devices that don't support pre-copy, for migrating an already stopped guest and for snapshots. Signed-off-by: Avihai Horon <avihaih@nvidia.com> --- hw/vfio/migration.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)