Re: [PATCH v1 11/13] intel_iommu_accel: drop _lock suffix in vtd_flush_host_piotlb_all_locked()

[CLEMENT MATHIEU--DRIF <clement.mathieu--drif@bull.com>]

On Wed, 2026-04-01 at 09:23 +0000, Duan, Zhenzhong wrote:
> Hi Clement,
> 
> 
> > -----Original Message-----
> > From: CLEMENT MATHIEU--DRIF <[clement.mathieu--drif@bull.com](mailto:clement.mathieu--drif@bull.com)>
> > Subject: Re: [PATCH v1 11/13] intel_iommu_accel: drop _lock suffix in
> > vtd_flush_host_piotlb_all_locked()
> > 
> > 
> > On Tue, 2026-03-31 at 07:18 +0000, Duan, Zhenzhong wrote:
> > 
> > > 
> > > 
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: CLEMENT MATHIEU--DRIF <[clement.mathieu--
> > > 
> > 
> > [drif@bull.com](mailto:drif@bull.com)](mailto:[clement.mathieu--drif@bull.com](mailto:clement.mathieu--drif@bull.com))>
> > 
> > > 
> > > > Subject: Re: [PATCH v1 11/13] intel_iommu_accel: drop _lock suffix in
> > > > vtd_flush_host_piotlb_all_locked()
> > > > 
> > > > 
> > > > On Fri, 2026-03-20 at 04:04 +0000, Duan, Zhenzhong wrote:
> > > > 
> > > > 
> > > > > Hi Clement,
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Duan, Zhenzhong
> > > > > > Subject: RE: [PATCH v1 11/13] intel_iommu_accel: drop _lock suffix in
> > > > > > vtd_flush_host_piotlb_all_locked()
> > > > > > 
> > > > > > Hi Clement,
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > -----Original Message-----
> > > > > > > From: CLEMENT MATHIEU--DRIF <[clement.mathieu--
> > > > > > 
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > > [[drif@bull.com](mailto:drif@bull.com)](mailto:[drif@bull.com](mailto:drif@bull.com))](mailto:[clement.mathieu--
> > > 
> > 
> > [drif@bull.com](mailto:drif@bull.com)](mailto:[clement.mathieu--drif@bull.com](mailto:clement.mathieu--drif@bull.com)))>
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > Subject: Re: [PATCH v1 11/13] intel_iommu_accel: drop _lock suffix in
> > > > > > > vtd_flush_host_piotlb_all_locked()
> > > > > > > 
> > > > > > > vtd_piotlb_page_invalidate can be called from
> > > > > > > vtd_pri_perform_implicit_invalidation without any lock. Thus we can call
> > > > > > > vtd_flush_host_piotlb_all_accel unlocked. Is there a risk in case of
> > > > > > 
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > > concurrent hot
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > plugging of another device?
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Good catch, hotplug is protected by BQL, but if an iothread could send PRI
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > > request,
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > we suffer from the race.
> > > > > > 
> > > > > > I think we can bypass vtd_pri_perform_implicit_invalidation() for
> > > > > 
> > > > 
> > > 
> > 
> > passthrough
> > 
> > > 
> > > > 
> > > > > 
> > > > > > device
> > > > > > which doesn't cache iotlb entry in QEMU but in host, like:
> > > > > > 
> > > > > > @@ -5401,7 +5401,8 @@ static int vtd_pri_request_page(PCIBus *bus,
> > > > > 
> > > > 
> > > 
> > 
> > void
> > 
> > > 
> > > > 
> > > > > 
> > > > > > *opaque, int devfn,
> > > > > >         return -ENOSPC;
> > > > > >     }
> > > > > > 
> > > > > > -    if (vtd_pri_perform_implicit_invalidation(vtd_as, addr)) {
> > > > > > +    if (!vtd_find_hiod_iommufd(vtd_as) &&
> > > > > > +        vtd_pri_perform_implicit_invalidation(vtd_as, addr)) {
> > > > > >         return -EINVAL;
> > > > > >     }
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > After further thinking, I found above change doesn’t resolve the race you
> > > > 
> > > > 
> > > > mentioned.
> > > > 
> > > > 
> > > > > 
> > > > > But I think the iothread should take BQL before calling ::pri_request_page(),
> > > > > because it is a function changing vtd device context, e.g., PQT and PRS
> > > > 
> > > 
> > 
> > register,
> > 
> > > 
> > > > 
> > > > > page invalidation queue and iotlb entries. We should always take BQL before
> > > > 
> > > > 
> > > > changing
> > > > 
> > > > 
> > > > > context of any device. Let me know if you have different opinions.
> > > > 
> > > > 
> > > > 
> > > > Do you mean that every emulated device that triggers a PRI request should
> > > 
> > 
> > hold
> > 
> > > 
> > > > the BQL?
> > > > I'm not sure this is a viable pattern as the initiator probably already holds an
> > > > internal lock
> > > > and thus cannot take the BQL without risking deadlocks with concurrent
> > > 
> > 
> > MMIO :/
> > 
> > > 
> > > 
> > > IIUC, qemu doesn't support concurrent MMIO emulations yet, they are serialized
> > 
> > by BQL. If we don't take BQL in call site, then we need it in callee, like:
> > 
> > > 
> > > @@ -5351,6 +5351,8 @@ static int vtd_pri_request_page(PCIBus *bus, void
> > 
> > *opaque, int devfn,
> > 
> > >      IntelIOMMUState *s = opaque;
> > >      VTDAddressSpace *vtd_as;
> > > 
> > > +    BQL_LOCK_GUARD();
> > > +
> > >      vtd_as = vtd_find_add_as(s, bus, devfn, pasid);
> > > 
> > >      uint64_t queue_addr_reg = vtd_get_quad(s, DMAR_PQA_REG);
> > > 
> > > 
> > > 
> > > 
> > > > Otherwise, it means that the device always takes the BQL before taking  it's
> > > 
> > 
> > own
> > 
> > > 
> > > > lock.
> > > 
> > > 
> > > 
> > > I think it's true because we always take BQL before emulation start.
> > 
> > 
> > This is pretty dangerous IMHO.
> > 
> > The emulated device that ends up calling this function probably has a lock on its
> > side already, let's call it dev_lock.
> > 
> > I the device does:
> > 	- lock(&dev_lock)
> > 	- start a PRI request
> > 	- lock(&bql)
> 
> 
> I have two questions:
> 
> 1. which thread may have above sequence?  
> 2. do we have to protect PRI request with dev_lock?
> 
> PRI state is protected by BQL, I think no need to be guarded by dev_lock. Maybe unlock dev_lock during PRI?

Devices that perform svm operations instead of regular dma end up performing this kind of sequence.  
The issue is that device state is guarded by the device lock. Thus, taking the bql in the pri callback  
of the iommu requires the device to unlock its own lock before starting the PRI operation or to rely on  
the bql all the time instead of its own lock.

I'm just trying to make sure svm devices will not become impossible to implement just because they  
have to break their flow and unlock every time they trigger a memory operation.

> 
> Thanks  
> Zhenzhong
> 
> 
> 
> > and if the host triggers an MMIO operation on the device:
> > 	- lock(&bql)
> > 	- lock(&dev_lock)
> > 
> > We can take dev_lock and bql in reverse order, leading to potential deadlocks o.O
> > 
> > 
> > > 
> > > Thanks
> > > Zhenzhong
> >
>