On Tue, 19 Jul 2022 21:02:48 -0300 Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index a7d2a95796d360..bb1a1677c5c230 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -1226,34 +1226,14 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, > return 0; > } > > -/** > - * vfio_ap_mdev_iommu_notifier - IOMMU notifier callback > - * > - * @nb: The notifier block > - * @action: Action to be taken > - * @data: data associated with the request > - * > - * For an UNMAP request, unpin the guest IOVA (the NIB guest address we > - * pinned before). Other requests are ignored. > - * > - * Return: for an UNMAP request, NOFITY_OK; otherwise NOTIFY_DONE. > - */ > -static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, > - unsigned long action, void *data) > +static void vfio_ap_mdev_dma_unmap(struct vfio_device *vdev, u64 iova, > + u64 length) > { > - struct ap_matrix_mdev *matrix_mdev; > - > - matrix_mdev = container_of(nb, struct ap_matrix_mdev, iommu_notifier); > - > - if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) { > - struct vfio_iommu_type1_dma_unmap *unmap = data; > - unsigned long g_pfn = unmap->iova >> PAGE_SHIFT; > - > - vfio_unpin_pages(&matrix_mdev->vdev, &g_pfn, 1); > - return NOTIFY_OK; > - } > + struct ap_matrix_mdev *matrix_mdev = > + container_of(vdev, struct ap_matrix_mdev, vdev); > + unsigned long g_pfn = iova >> PAGE_SHIFT; > > - return NOTIFY_DONE; > + vfio_unpin_pages(&matrix_mdev->vdev, &g_pfn, 1); > } > > /** I tried to apply this on top of Nicolin's series which results in a conflict that can be resolved as below: diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index e8856a7e151c..d7c38c82f694 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -1219,33 +1219,13 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, return 0; } -/** - * vfio_ap_mdev_iommu_notifier - IOMMU notifier callback - * - * @nb: The notifier block - * @action: Action to be taken - * @data: data associated with the request - * - * For an UNMAP request, unpin the guest IOVA (the NIB guest address we - * pinned before). Other requests are ignored. - * - * Return: for an UNMAP request, NOFITY_OK; otherwise NOTIFY_DONE. - */ -static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, - unsigned long action, void *data) +static void vfio_ap_mdev_dma_unmap(struct vfio_device *vdev, u64 iova, + u64 length) { - struct ap_matrix_mdev *matrix_mdev; - - matrix_mdev = container_of(nb, struct ap_matrix_mdev, iommu_notifier); - - if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) { - struct vfio_iommu_type1_dma_unmap *unmap = data; - - vfio_unpin_pages(&matrix_mdev->vdev, unmap->iova, 1); - return NOTIFY_OK; - } + struct ap_matrix_mdev *matrix_mdev = + container_of(vdev, struct ap_matrix_mdev, vdev); - return NOTIFY_DONE; + vfio_unpin_pages(&matrix_mdev->vdev, iova, 1); } /** ie. we don't need the gfn, we only need the iova. 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. It makes no sense for a driver to assume that the first iova is pinned and is the only pinned page. ccw has the same issue: static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length) { struct vfio_ccw_private *private = container_of(vdev, struct vfio_ccw_private, vdev); /* Drivers MUST unpin pages in response to an invalidation. */ if (!cp_iova_pinned(&private->cp, iova)) return; vfio_ccw_mdev_reset(private); } Entirely ignoring the length arg. It seems only GVT-g has this correct to actually look through the extent of the range being unmapped: static void intel_vgpu_dma_unmap(struct vfio_device *vfio_dev, u64 iova, u64 length) { struct intel_vgpu *vgpu = vfio_dev_to_vgpu(vfio_dev); struct gvt_dma *entry; u64 iov_pfn = iova >> PAGE_SHIFT; u64 end_iov_pfn = iov_pfn + length / PAGE_SIZE; mutex_lock(&vgpu->cache_lock); for (; iov_pfn < end_iov_pfn; iov_pfn++) { entry = __gvt_cache_find_gfn(vgpu, iov_pfn); if (!entry) continue; gvt_dma_unmap_page(vgpu, entry->gfn, entry->dma_addr, entry->size); __gvt_cache_remove_entry(vgpu, entry); } mutex_unlock(&vgpu->cache_lock); } Should ap and ccw implementations of .dma_unmap just be replaced with a BUG_ON(1)? Thanks, Alex