On Fri, 9 Apr 2021 11:44:15 +0800 Shenming Lu <[email protected]> wrote:
> To avoid pinning pages when they are mapped in IOMMU page tables, we > add an MMU notifier to tell the addresses which are no longer valid > and try to unmap them. > > Signed-off-by: Shenming Lu <[email protected]> > --- > drivers/vfio/vfio_iommu_type1.c | 112 +++++++++++++++++++++++++++++++- > 1 file changed, 109 insertions(+), 3 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index ab0ff60ee207..1cb9d1f2717b 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -40,6 +40,7 @@ > #include <linux/notifier.h> > #include <linux/dma-iommu.h> > #include <linux/irqdomain.h> > +#include <linux/mmu_notifier.h> > > #define DRIVER_VERSION "0.2" > #define DRIVER_AUTHOR "Alex Williamson <[email protected]>" > @@ -69,6 +70,7 @@ struct vfio_iommu { > struct mutex lock; > struct rb_root dma_list; > struct blocking_notifier_head notifier; > + struct mmu_notifier mn; > unsigned int dma_avail; > unsigned int vaddr_invalid_count; > uint64_t pgsize_bitmap; > @@ -1204,6 +1206,72 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, > struct vfio_dma *dma, > return unlocked; > } > > +/* Unmap the IOPF mapped pages in the specified range. */ > +static void vfio_unmap_partial_iopf(struct vfio_iommu *iommu, > + struct vfio_dma *dma, > + dma_addr_t start, dma_addr_t end) > +{ > + struct iommu_iotlb_gather *gathers; > + struct vfio_domain *d; > + int i, num_domains = 0; > + > + list_for_each_entry(d, &iommu->domain_list, next) > + num_domains++; > + > + gathers = kzalloc(sizeof(*gathers) * num_domains, GFP_KERNEL); > + if (gathers) { > + for (i = 0; i < num_domains; i++) > + iommu_iotlb_gather_init(&gathers[i]); > + } If we're always serialized on iommu->lock, would it make sense to have gathers pre-allocated on the vfio_iommu object? > + > + while (start < end) { > + unsigned long bit_offset; > + size_t len; > + > + bit_offset = (start - dma->iova) >> PAGE_SHIFT; > + > + for (len = 0; start + len < end; len += PAGE_SIZE) { > + if (!IOPF_MAPPED_BITMAP_GET(dma, > + bit_offset + (len >> PAGE_SHIFT))) > + break; There are bitmap helpers for this, find_first_bit(), find_next_zero_bit(). > + } > + > + if (len) { > + i = 0; > + list_for_each_entry(d, &iommu->domain_list, next) { > + size_t unmapped; > + > + if (gathers) > + unmapped = iommu_unmap_fast(d->domain, > + start, len, > + > &gathers[i++]); > + else > + unmapped = iommu_unmap(d->domain, > + start, len); > + > + if (WARN_ON(unmapped != len)) The IOMMU API does not guarantee arbitrary unmap sizes, this will trigger and this exit path is wrong. If we've already unmapped the IOMMU, shouldn't we proceed with @unmapped rather than @len so the device can re-fault the extra mappings? Otherwise the IOMMU state doesn't match the iopf bitmap state. > + goto out; > + } > + > + bitmap_clear(dma->iopf_mapped_bitmap, > + bit_offset, len >> PAGE_SHIFT); > + > + cond_resched(); > + } > + > + start += (len + PAGE_SIZE); > + } > + > +out: > + if (gathers) { > + i = 0; > + list_for_each_entry(d, &iommu->domain_list, next) > + iommu_iotlb_sync(d->domain, &gathers[i++]); > + > + kfree(gathers); > + } > +} > + > static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) > { > WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)); > @@ -3197,17 +3265,18 @@ static int vfio_iommu_type1_dma_map_iopf(struct > iommu_fault *fault, void *data) > > vaddr = iova - dma->iova + dma->vaddr; > > - if (vfio_pin_page_external(dma, vaddr, &pfn, true)) > + if (vfio_pin_page_external(dma, vaddr, &pfn, false)) > goto out_invalid; > > if (vfio_iommu_map(iommu, iova, pfn, 1, dma->prot)) { > - if (put_pfn(pfn, dma->prot)) > - vfio_lock_acct(dma, -1, true); > + put_pfn(pfn, dma->prot); > goto out_invalid; > } > > bitmap_set(dma->iopf_mapped_bitmap, bit_offset, 1); > > + put_pfn(pfn, dma->prot); > + > out_success: > status = IOMMU_PAGE_RESP_SUCCESS; > > @@ -3220,6 +3289,43 @@ static int vfio_iommu_type1_dma_map_iopf(struct > iommu_fault *fault, void *data) > return 0; > } > > +static void mn_invalidate_range(struct mmu_notifier *mn, struct mm_struct > *mm, > + unsigned long start, unsigned long end) > +{ > + struct vfio_iommu *iommu = container_of(mn, struct vfio_iommu, mn); > + struct rb_node *n; > + int ret; > + > + mutex_lock(&iommu->lock); > + > + ret = vfio_wait_all_valid(iommu); > + if (WARN_ON(ret < 0)) > + return; Is WARN_ON sufficient for this error condition? We've been told to evacuate a range of mm, the device still has DMA access, we've removed page pinning. This seems like a BUG_ON condition to me, we can't allow the system to continue in any way with pages getting unmapped from under the device. > + > + for (n = rb_first(&iommu->dma_list); n; n = rb_next(n)) { > + struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node); > + unsigned long start_n, end_n; > + > + if (end <= dma->vaddr || start >= dma->vaddr + dma->size) > + continue; > + > + start_n = ALIGN_DOWN(max_t(unsigned long, start, dma->vaddr), > + PAGE_SIZE); > + end_n = ALIGN(min_t(unsigned long, end, dma->vaddr + dma->size), > + PAGE_SIZE); > + > + vfio_unmap_partial_iopf(iommu, dma, > + start_n - dma->vaddr + dma->iova, > + end_n - dma->vaddr + dma->iova); > + } > + > + mutex_unlock(&iommu->lock); > +} > + > +static const struct mmu_notifier_ops vfio_iommu_type1_mn_ops = { > + .invalidate_range = mn_invalidate_range, > +}; > + > static long vfio_iommu_type1_ioctl(void *iommu_data, > unsigned int cmd, unsigned long arg) > { Again, this patch series is difficult to follow because we're introducing dead code until the mmu notifier actually has a path to be registered. We shouldn't be taking any faults until iopf is enabled, so it seems like we can add more of the core support alongside this code. _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
