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 VFIO migration QAPI event, the
STOP_COPY event is undesirably emitted twice.

Prevent this by returning early in vfio_migration_set_state() if
new_state is the same as current device state.

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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 5a359c4c78..14ef9c924e 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -143,6 +143,10 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
         (struct vfio_device_feature_mig_state *)feature->data;
     int ret;
 
+    if (new_state == migration->device_state) {
+        return 0;
+    }
+
     feature->argsz = sizeof(buf);
     feature->flags =
         VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE;
-- 
2.26.3

On 5/9/24 11:09, 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 VFIO migration QAPI event, the
> STOP_COPY event is undesirably emitted twice.
> 
> Prevent this by returning early in vfio_migration_set_state() if
> new_state is the same as current device state.
> 
> 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 | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 5a359c4c78..14ef9c924e 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -143,6 +143,10 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>           (struct vfio_device_feature_mig_state *)feature->data;
>       int ret;
>   

I wonder if we should improve the trace events a little to track better
the state transitions. May be move trace_vfio_migration_set_state()
at the beginning of vfio_migration_set_state() and introduce a new
event for the currently named routine set_state() ?

This can come with followups.


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> +    if (new_state == migration->device_state) {
> +        return 0;
> +    }
> +
>       feature->argsz = sizeof(buf);
>       feature->flags =
>           VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE;

On 13/05/2024 17:13, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/9/24 11:09, 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 VFIO migration QAPI event, the
>> STOP_COPY event is undesirably emitted twice.
>>
>> Prevent this by returning early in vfio_migration_set_state() if
>> new_state is the same as current device state.
>>
>> 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 | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 5a359c4c78..14ef9c924e 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -143,6 +143,10 @@ static int vfio_migration_set_state(VFIODevice 
>> *vbasedev,
>>           (struct vfio_device_feature_mig_state *)feature->data;
>>       int ret;
>>
>
> I wonder if we should improve the trace events a little to track better
> the state transitions. May be move trace_vfio_migration_set_state()
> at the beginning of vfio_migration_set_state() and introduce a new
> event for the currently named routine set_state() ?
>
> This can come with followups.
>
Yes, this sounds good.

Thanks.

>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>
> Thanks,
>
> C.
>
>
>> +    if (new_state == migration->device_state) {
>> +        return 0;
>> +    }
>> +
>>       feature->argsz = sizeof(buf);
>>       feature->flags =
>>           VFIO_DEVICE_FEATURE_SET | 
>> VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE;
>