On Mon, 2026-09-21 at 09:31 +0000, Duan, Zhenzhong wrote: > Caution: External email. Do not open attachments or click links, unless this > email comes from a known sender and you know the content is safe. > > > Hi Clement, > > > > -----Original Message----- > > From: Clément MATHIEU--DRIF > > <[[email protected]](mailto:[email protected])> > > Subject: Re: [PATCH v7 6/6] intel_iommu_accel: teardown FAULTQ resources in > > bottom half > > > > > > On Wed, 2026-09-16 at 18:04 +0800, Zhenzhong Duan wrote: > > > > > Caution: External email. Do not open attachments or click links, unless > > > this email > > > > comes from a known sender and you know the content is safe. > > > > > > > > > > > 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 > > > > <[[[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))> > > > > > Signed-off-by: Zhenzhong Duan > > > > <[[[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))> > > > > > Tested-by: Xudong Hao > > > > <[[[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))> > > > > > --- > > > 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..c3f3c74482 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); > > > + > > > + 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(IOMMUFaultQueue)); > > > > > > sizeof(*fq) > > > Sure. > > > > > > > > + 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); > > > > > > are we sure that vtd_pce->vtd_prq_list is not updated by another thread > > concurrently at this point? > > > Yes, vtd_prq_list is updated in below paths: > > 1. poll() exit -> vtd_read_fs_faultq() -> vtd_propagate_recoverable_faults() > -> vtd_prq_list insert > 2. poll() exit -> faultq_teardown_bh() -> vtd_prq_list remove > 3. vtd_process_inv_desc() -> vtd_process_page_group_response_desc() -> > vtd_accel_propagate_page_group_response() -> vtd_prq_handle_pasid_response() > -> vtd_prq_list remove > > 1 and 2 runs in main thread which takes BQL after poll() exit. > 3 runs in vcpu thread, before updating device state, BQL should has been > taken in prepare_mmio_access().
nice, thanks! > > Thanks > Zhenzhong
