On Wed, 20 Jul 2022 17:08:29 -0300 Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > On Wed, Jul 20, 2022 at 01:41:13PM -0600, Alex Williamson wrote: > > > ie. we don't need the gfn, we only need the iova. > > Right, that makes sense > > > However then I start to wonder why we're passing in 1 for the number of > > pages because this previously notifier, now callback is called for the > > entire vfio_dma range when we find any pinned pages. > > Well, it is doing this because it only ever pins one page. Of course that page is not necessarily the page it unpins given the contract misunderstanding below. > The drivers are confused about what the contract is. vfio is calling > the notifier with the entire IOVA range that is being unmapped and the > drivers are expecting to receive notifications only for the IOVA they > have actually pinned. > > > Should ap and ccw implementations of .dma_unmap just be replaced with a > > BUG_ON(1)? > > The point of these callbacks is to halt concurrent DMA, and ccw does > that today. ccw essentially only checks whether the starting iova of the unmap is currently mapped. If not it does nothing, if it is it tries to reset the device and unpin everything. Chances are the first iova is not the one pinned, so we don't end up removing the pinned page and type1 will eventually BUG_ON after a few tries. > It looks like AP is missing a call to ap_aqic(), so it is > probably double wrong. Thankfully the type1 unpinning path can't be tricked into unpinning something that wasn't pinned, so chances are the unpin call does nothing, with a small risk that it unpins another driver's pinned page, which might not yet have been notified and could still be using the page. In the end, if ap did have a page pinned in the range, we'll hit the same BUG_ON as above. > What I'd suggest is adding a WARN_ON that the dma->pfn_list is not > empty and leave these functions alone. The BUG_ON still exists in type1. Eric, Matt, Tony, Halil, JasonH, any quick fixes here? ccw looks like it would be pretty straightforward to test against a range rather than a single iova. > Most likely AP should be fixed to call vfio_ap_irq_disable() and to > check the q->saved_pfn against the IOVA. Right, the q->saved_iova, perhaps calling vfio_ap_irq_disable() on finding a matching queue. > But I'm inclined to leave this as-is for this series given we are at > rc7. On the grounds that it's no worse, maybe, but given the changes around this code hopefully we can submit fixes patches to stable if the backport isn't obvious and the BUG_ON in type1 is reachable. Thanks, Alex
