Re: [PATCH v8 6/6] intel_iommu_accel: teardown FAULTQ resources in bottom half

[Yi Liu <yi.l.liu@intel.com>]

On 10/9/26 17:07, Zhenzhong Duan wrote:
> When a pasid entry becomes invalid, we need to release all resources
> allocated for that entry including FAULTQ object and fault_fd.
> 
> We call qemu_set_fd_handler() to detach fault_fd's io_read handler and
> wakes up main thread from poll(), but there could still be a small
> window we call iommufd_backend_free_id(fault_id) before poll() exit
> and release fault_id file reference. In this rare case, FAULTQ object
> free return -EBUSY because opened fault_id file keeps reference of
> FAULTQ object.
> 
> Teardown FAULTQ resources in bottom half to ensure poll() has released
> fault_id file reference.
> 
> Suggested-by: Shameer Kolothum <skolothumtho@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Tested-by: Xudong Hao <xudong.hao@intel.com>
> ---
>   hw/i386/intel_iommu_accel.c | 40 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
> index cd227315da..c07ffa68e0 100644
> --- a/hw/i386/intel_iommu_accel.c
> +++ b/hw/i386/intel_iommu_accel.c
> @@ -268,17 +268,51 @@ free_faultq:
>       return false;
>   }
>   
> +typedef struct IOMMUFaultQueue {
> +    IOMMUFDBackend *iommufd;
> +    uint32_t id;
> +    int fd;
> +    QLIST_HEAD(, VTDPRQEntry) vtd_prq_list;
> +} IOMMUFaultQueue;
> +
> +static void faultq_teardown_bh(void *opaque)
> +{
> +    IOMMUFaultQueue *fq = opaque;
> +    VTDPRQEntry *prqe, *next;
> +
> +    QLIST_FOREACH_SAFE(prqe, &fq->vtd_prq_list, next, next) {
> +        QLIST_REMOVE(prqe, next);
> +        g_free(prqe);
> +    }
> +
> +    qemu_set_fd_handler(fq->fd, NULL, NULL, NULL);
> +    close(fq->fd);
> +    iommufd_backend_free_id(fq->iommufd, fq->id);
> +

a nit: any reason that you open code instead of reusing
vtd_destroy_fs_faultq() as the existing code? If no special
reason, may be better reusing it.

Other part LGTM.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>


> +    g_free(fq);
> +}
> +
>   static void vtd_destroy_old_fs_faultq(VTDAccelPASIDCacheEntry *vtd_pce)
>   {
> +    HostIOMMUDeviceIOMMUFD *idev =
> +        HOST_IOMMU_DEVICE_IOMMUFD(vtd_pce->vtd_hiod->hiod);
> +    IOMMUFaultQueue *fq;
> +
>       if (vtd_pce->fault_fd < 0) {
>           return;
>       }
>   
> -    qemu_set_fd_handler(vtd_pce->fault_fd, NULL, NULL, NULL);
> -    vtd_destroy_fs_faultq(vtd_pce->vtd_hiod, vtd_pce->fault_id,
> -                          vtd_pce->fault_fd);
> +    fq = g_malloc(sizeof(*fq));
> +    fq->iommufd = idev->iommufd;
> +    fq->fd = vtd_pce->fault_fd;
> +    fq->id = vtd_pce->fault_id;
>       vtd_pce->fault_id = 0;
>       vtd_pce->fault_fd = -1;
> +    fq->vtd_prq_list.lh_first = vtd_pce->vtd_prq_list.lh_first;
> +    QLIST_INIT(&vtd_pce->vtd_prq_list);
> +
> +    aio_bh_schedule_oneshot(iohandler_get_aio_context(),
> +                            faultq_teardown_bh, fq);
>   }
>   
>   static void vtd_setup_fs_faultq(VTDAccelPASIDCacheEntry *vtd_pce,