> From: Leon Romanovsky <[email protected]> > Sent: Thursday, January 29, 2026 4:42 PM > > On Thu, Jan 29, 2026 at 08:13:18AM +0000, Tian, Kevin wrote: > > > From: Leon Romanovsky <[email protected]> > > > Sent: Thursday, January 29, 2026 3:34 PM > > > > > > On Thu, Jan 29, 2026 at 07:06:37AM +0000, Tian, Kevin wrote: > > > > > From: Jason Gunthorpe <[email protected]> > > > > > Sent: Wednesday, January 28, 2026 12:28 AM > > > > > > > > > > On Tue, Jan 27, 2026 at 10:58:35AM +0200, Leon Romanovsky wrote: > > > > > > > > @@ -333,7 +359,37 @@ void vfio_pci_dma_buf_move(struct > > > > > vfio_pci_core_device *vdev, bool revoked) > > > > > > > > dma_resv_lock(priv->dmabuf->resv, NULL); > > > > > > > > priv->revoked = revoked; > > > > > > > > dma_buf_invalidate_mappings(priv- > > > >dmabuf); > > > > > > > > + > > > > > > > > dma_resv_wait_timeout(priv->dmabuf->resv, > > > > > > > > + > > > DMA_RESV_USAGE_BOOKKEEP, > > > > > false, > > > > > > > > + > > > MAX_SCHEDULE_TIMEOUT); > > > > > > > > dma_resv_unlock(priv->dmabuf->resv); > > > > > > > > + if (revoked) { > > > > > > > > + kref_put(&priv->kref, > > > > > vfio_pci_dma_buf_done); > > > > > > > > + /* Let's wait till all DMA > > > > > > > > unmap are > > > > > completed. */ > > > > > > > > + wait = > > > > > > > > wait_for_completion_timeout( > > > > > > > > + &priv->comp, > > > secs_to_jiffies(1)); > > > > > > > > > > > > > > Is the 1-second constant sufficient for all hardware, or should > > > > > > > the > > > > > > > invalidate_mappings() contract require the callback to block until > > > > > > > speculative reads are strictly fenced? I'm wondering about a case > > > where > > > > > > > a device's firmware has a high response latency, perhaps due to > > > internal > > > > > > > management tasks like error recovery or thermal and it exceeds > the > > > 1s > > > > > > > timeout. > > > > > > > > > > > > > > If the device is in the middle of a large DMA burst and the > firmware is > > > > > > > slow to flush the internal pipelines to a fully "quiesced" > > > > > > > read-and-discard state, reclaiming the memory at exactly 1.001 > > > seconds > > > > > > > risks triggering platform-level faults.. > > > > > > > > > > > > > > Since the wen explicitly permit these speculative reads until > unmap is > > > > > > > complete, relying on a hardcoded timeout in the exporter seems > to > > > > > > > introduce a hardware-dependent race condition that could > > > compromise > > > > > > > system stability via IOMMU errors or AER faults. > > > > > > > > > > > > > > Should the importer instead be required to guarantee that all > > > > > > > speculative access has ceased before the invalidation call > > > > > > > returns? > > > > > > > > > > > > It is guaranteed by the dma_resv_wait_timeout() call above. That > call > > > > > ensures > > > > > > that the hardware has completed all pending operations. The > 1‑second > > > > > delay is > > > > > > meant to catch cases where an in-kernel DMA unmap call is missing, > > > which > > > > > should > > > > > > not trigger any DMA activity at that point. > > > > > > > > > > Christian may know actual examples, but my general feeling is he was > > > > > worrying about drivers that have pushed the DMABUF to visibility on > > > > > the GPU and the move notify & fences only shoot down some access. > So > > > > > it has to wait until the DMABUF is finally unmapped. > > > > > > > > > > Pranjal's example should be covered by the driver adding a fence and > > > > > then the unbounded fence wait will complete it. > > > > > > > > > > > > > Bear me if it's an ignorant question. > > > > > > > > The commit msg of patch6 says that VFIO doesn't tolerate unbounded > > > > wait, which is the reason behind the 2nd timeout wait here. > > > > > > It is not accurate. A second timeout is present both in the > > > description of patch 6 and in VFIO implementation. The difference is > > > that the timeout is enforced within VFIO. > > > > > > > > > > > Then why is "the unbounded fence wait" not a problem in the same > > > > code path? the use of MAX_SCHEDULE_TIMEOUT imply a worst-case > > > > timeout in hundreds of years... > > > > > > "An unbounded fence wait" is a different class of wait. It indicates > > > broken > > > hardware that continues to issue DMA transactions even after it has been > > > told to > > > stop. > > > > > > The second wait exists to catch software bugs or misuse, where the dma- > buf > > > importer has misrepresented its capabilities. > > > > > > > Okay I see. > > > > > > > > > > and it'd be helpful to put some words in the code based on what's > > > > discussed here. > > > > > > We've documented as much as we can in dma_buf_attach_revocable() > and > > > dma_buf_invalidate_mappings(). Do you have any suggestions on what > else > > > should be added here? > > > > > > > the selection of 1s? > > It is indirectly written in description of WARN_ON(), but let's add > more. What about the following? > > diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c > b/drivers/vfio/pci/vfio_pci_dmabuf.c > index 93795ad2e025..948ba75288c6 100644 > --- a/drivers/vfio/pci/vfio_pci_dmabuf.c > +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c > @@ -357,7 +357,13 @@ void vfio_pci_dma_buf_move(struct > vfio_pci_core_device *vdev, bool revoked) > dma_resv_unlock(priv->dmabuf->resv); > if (revoked) { > kref_put(&priv->kref, vfio_pci_dma_buf_done); > - /* Let's wait till all DMA unmap are > completed. */ > + /* > + * Let's wait for 1 second till all DMA unmap > + * are completed. It is supposed to catch > dma-buf > + * importers which lied about their support > + * of dmabuf revoke. See > dma_buf_invalidate_mappings() > + * for the expected behaviour, > + */ > wait = wait_for_completion_timeout( > &priv->comp, secs_to_jiffies(1)); > /* >
looks good. Just replace the trailing "," with "."
