[PATCH RFC] vfio: Set the priority of VFIO VM state change handler explicitly

[PATCH RFC] vfio: Set the priority of VFIO VM state change handler explicitly

[Shenming Lu]

In VFIO VM state change handler, VFIO devices are transitioned in
_SAVING state, which should keep them from sending interrupts. Then
we can save the pending states of all interrupts in GIC VM state
change handler (on ARM).

So we have to set the priority of VFIO VM state change handler
explicitly (like virtio devices) to ensure it is called before GIC's
in saving.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 hw/vfio/migration.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 55261562d4..d0d30864ba 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -857,7 +857,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
     register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
                          vbasedev);
 
-    migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
+    migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev,
+                                                           vfio_vmstate_change,
                                                            vbasedev);
     migration->migration_state.notify = vfio_migration_state_notifier;
     add_migration_state_change_notifier(&migration->migration_state);
-- 
2.19.1

Re: [PATCH RFC] vfio: Set the priority of VFIO VM state change handler explicitly

[Kirti Wankhede]

On 11/17/2020 7:10 AM, Shenming Lu wrote:
> In VFIO VM state change handler, VFIO devices are transitioned in
> _SAVING state, which should keep them from sending interrupts. Then
> we can save the pending states of all interrupts in GIC VM state
> change handler (on ARM).
> 
> So we have to set the priority of VFIO VM state change handler
> explicitly (like virtio devices) to ensure it is called before GIC's
> in saving.
> 
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>   hw/vfio/migration.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 55261562d4..d0d30864ba 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -857,7 +857,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>       register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
>                            vbasedev);
>   
> -    migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
> +    migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev,
> +                                                           vfio_vmstate_change,
>                                                              vbasedev);
>       migration->migration_state.notify = vfio_migration_state_notifier;
>       add_migration_state_change_notifier(&migration->migration_state);
> 

Looks good to me.
Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>

Re: [PATCH RFC] vfio: Set the priority of VFIO VM state change handler explicitly

[Cornelia Huck]

On Tue, 17 Nov 2020 09:40:50 +0800
Shenming Lu <lushenming@huawei.com> wrote:

> In VFIO VM state change handler, VFIO devices are transitioned in
> _SAVING state, which should keep them from sending interrupts. Then
> we can save the pending states of all interrupts in GIC VM state
> change handler (on ARM).
> 
> So we have to set the priority of VFIO VM state change handler
> explicitly (like virtio devices) to ensure it is called before GIC's
> in saving.

I'm wondering whether this is the case for other irq chips as well, and
the GIC is just the first one where it was noticed.

> 
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>  hw/vfio/migration.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 55261562d4..d0d30864ba 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -857,7 +857,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>      register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
>                           vbasedev);
>  
> -    migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
> +    migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev,
> +                                                           vfio_vmstate_change,
>                                                             vbasedev);
>      migration->migration_state.notify = vfio_migration_state_notifier;
>      add_migration_state_change_notifier(&migration->migration_state);

I think this patch does what the patch description says, but I'm
wondering whether we might miss other interdependencies as well (which
would also be fixed by pushing the vm state handling of vfio devices to
that point in the sequence?)