[PATCH] vfio: Register/unregister container for CPR only once for each container

[PATCH] vfio: Register/unregister container for CPR only once for each container

[Zhenzhong Duan]

vfio_cpr_register_container and vfio_cpr_unregister_container are container
scoped function. Calling them for each device attaching/detaching would
corrupt CPR reboot notifier list, i.e., when two VFIO devices are attached
to same container and have same notifier registered twice.

Fixes: d9fa4223b30a ("vfio: register container for cpr")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
btw: untested as I have no env with two migration capable devices,
     appreciate if someone have env to help test, thanks

 hw/vfio/iommufd.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index f24054a6a5..b2f72dc8c3 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -416,6 +416,7 @@ static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
     if (!QLIST_EMPTY(&bcontainer->device_list)) {
         return;
     }
+    vfio_cpr_unregister_container(bcontainer);
     vfio_listener_unregister(bcontainer);
     iommufd_backend_free_id(container->be, container->ioas_id);
     object_unref(container);
@@ -561,6 +562,10 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
         goto err_listener_register;
     }
 
+    if (!vfio_cpr_register_container(bcontainer, errp)) {
+        goto err_listener_register;
+    }
+
     bcontainer->initialized = true;
 
 found_container:
@@ -570,13 +575,9 @@ found_container:
         goto err_listener_register;
     }
 
-    if (!vfio_cpr_register_container(bcontainer, errp)) {
-        goto err_listener_register;
-    }
-
     if (!vfio_device_hiod_create_and_realize(vbasedev,
                      TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, errp)) {
-        goto err_hiod_realize;
+        goto err_listener_register;
     }
 
     /*
@@ -600,8 +601,6 @@ found_container:
                                    vbasedev->num_regions, vbasedev->flags);
     return true;
 
-err_hiod_realize:
-    vfio_cpr_unregister_container(bcontainer);
 err_listener_register:
     iommufd_cdev_ram_block_discard_disable(false);
 err_discard_disable:
@@ -632,7 +631,6 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
     }
 
     object_unref(vbasedev->hiod);
-    vfio_cpr_unregister_container(bcontainer);
     iommufd_cdev_detach_container(vbasedev, container);
     iommufd_cdev_container_destroy(container);
     vfio_address_space_put(space);
-- 
2.34.1

Re: [PATCH] vfio: Register/unregister container for CPR only once for each container

[Cédric Le Goater]

On 4/24/25 08:33, Zhenzhong Duan wrote:
> vfio_cpr_register_container and vfio_cpr_unregister_container are container
> scoped function. Calling them for each device attaching/detaching would
> corrupt CPR reboot notifier list, i.e., when two VFIO devices are attached
> to same container and have same notifier registered twice.
> 
> Fixes: d9fa4223b30a ("vfio: register container for cpr")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Looks OK to me, so

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

Some feedback from Steve would be good to have.

> ---
> btw: untested as I have no env with two migration capable devices,
>       appreciate if someone have env to help test, thanks

I did.

Thanks,

C.




> 
>   hw/vfio/iommufd.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index f24054a6a5..b2f72dc8c3 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -416,6 +416,7 @@ static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
>       if (!QLIST_EMPTY(&bcontainer->device_list)) {
>           return;
>       }
> +    vfio_cpr_unregister_container(bcontainer);
>       vfio_listener_unregister(bcontainer);
>       iommufd_backend_free_id(container->be, container->ioas_id);
>       object_unref(container);
> @@ -561,6 +562,10 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>           goto err_listener_register;
>       }
>   
> +    if (!vfio_cpr_register_container(bcontainer, errp)) {
> +        goto err_listener_register;
> +    }
> +
>       bcontainer->initialized = true;
>   
>   found_container:
> @@ -570,13 +575,9 @@ found_container:
>           goto err_listener_register;
>       }
>   
> -    if (!vfio_cpr_register_container(bcontainer, errp)) {
> -        goto err_listener_register;
> -    }
> -
>       if (!vfio_device_hiod_create_and_realize(vbasedev,
>                        TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, errp)) {
> -        goto err_hiod_realize;
> +        goto err_listener_register;
>       }
>   
>       /*
> @@ -600,8 +601,6 @@ found_container:
>                                      vbasedev->num_regions, vbasedev->flags);
>       return true;
>   
> -err_hiod_realize:
> -    vfio_cpr_unregister_container(bcontainer);
>   err_listener_register:
>       iommufd_cdev_ram_block_discard_disable(false);
>   err_discard_disable:
> @@ -632,7 +631,6 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
>       }
>   
>       object_unref(vbasedev->hiod);
> -    vfio_cpr_unregister_container(bcontainer);
>       iommufd_cdev_detach_container(vbasedev, container);
>       iommufd_cdev_container_destroy(container);
>       vfio_address_space_put(space);

RE: [PATCH] vfio: Register/unregister container for CPR only once for each container

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH] vfio: Register/unregister container for CPR only once for
>each container
>
>On 4/24/25 08:33, Zhenzhong Duan wrote:
>> vfio_cpr_register_container and vfio_cpr_unregister_container are container
>> scoped function. Calling them for each device attaching/detaching would
>> corrupt CPR reboot notifier list, i.e., when two VFIO devices are attached
>> to same container and have same notifier registered twice.
>>
>> Fixes: d9fa4223b30a ("vfio: register container for cpr")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Looks OK to me, so
>
>Reviewed-by: Cédric Le Goater <clg@redhat.com>
>
>Some feedback from Steve would be good to have.
>
>> ---
>> btw: untested as I have no env with two migration capable devices,
>>       appreciate if someone have env to help test, thanks
>
>I did.

Thanks Cédric!

BRs,
Zhenzhong

Re: [PATCH] vfio: Register/unregister container for CPR only once for each container

[Cédric Le Goater]

On 4/24/25 08:33, Zhenzhong Duan wrote:
> vfio_cpr_register_container and vfio_cpr_unregister_container are container
> scoped function. Calling them for each device attaching/detaching would
> corrupt CPR reboot notifier list, i.e., when two VFIO devices are attached
> to same container and have same notifier registered twice.
> 
> Fixes: d9fa4223b30a ("vfio: register container for cpr")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> btw: untested as I have no env with two migration capable devices,
>       appreciate if someone have env to help test, thanks
> 
>   hw/vfio/iommufd.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)



Applied to vfio-next.

Thanks,

C.

Re: [PATCH] vfio: Register/unregister container for CPR only once for each container

[Steven Sistare]

On 4/25/2025 3:59 AM, Cédric Le Goater wrote:
> On 4/24/25 08:33, Zhenzhong Duan wrote:
>> vfio_cpr_register_container and vfio_cpr_unregister_container are container
>> scoped function. Calling them for each device attaching/detaching would
>> corrupt CPR reboot notifier list, i.e., when two VFIO devices are attached
>> to same container and have same notifier registered twice.
>>
>> Fixes: d9fa4223b30a ("vfio: register container for cpr")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>> btw: untested as I have no env with two migration capable devices,
>>       appreciate if someone have env to help test, thanks
>>
>>   hw/vfio/iommufd.c | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> Applied to vfio-next.

Looks good - steve