On 11/28/22 20:37, Jason Gunthorpe wrote: > On Mon, Nov 28, 2022 at 06:53:12PM +0100, Eric Auger wrote: > >>> +static int iommufd_vfio_map_dma(struct iommufd_ctx *ictx, unsigned int cmd, >>> + void __user *arg) >>> +{ >>> + u32 supported_flags = VFIO_DMA_MAP_FLAG_READ | VFIO_DMA_MAP_FLAG_WRITE; >>> + size_t minsz = offsetofend(struct vfio_iommu_type1_dma_map, size); >>> + struct vfio_iommu_type1_dma_map map; >>> + int iommu_prot = IOMMU_CACHE; >>> + struct iommufd_ioas *ioas; >>> + unsigned long iova; >>> + int rc; >>> + >>> + if (copy_from_user(&map, arg, minsz)) >>> + return -EFAULT; >>> + >>> + if (map.argsz < minsz || map.flags & ~supported_flags) >>> + return -EINVAL; >>> + >>> + if (map.flags & VFIO_DMA_MAP_FLAG_READ) >>> + iommu_prot |= IOMMU_READ; >>> + if (map.flags & VFIO_DMA_MAP_FLAG_WRITE) >>> + iommu_prot |= IOMMU_WRITE; >>> + >>> + ioas = get_compat_ioas(ictx); >>> + if (IS_ERR(ioas)) >>> + return PTR_ERR(ioas); >>> + >>> + /* >>> + * Maps created through the legacy interface always use VFIO compatible >>> + * rlimit accounting. If the user wishes to use the faster user based >>> + * rlimit accounting then they must use the new interface. >> s/they/he > "they" has become a common neutral singular pronoun in English. Oh OK. > >>> +static int iommufd_vfio_unmap_dma(struct iommufd_ctx *ictx, unsigned int cmd, >>> + void __user *arg) >>> +{ >>> + size_t minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size); >>> + /* >>> + * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP is obsoleted by the new >>> + * dirty tracking direction: >>> + * https://lore.kernel.org/kvm/20220731125503.142683-1-yishaih@xxxxxxxxxx/ >>> + * https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@xxxxxxxxxx/ >>> + */ >>> + u32 supported_flags = VFIO_DMA_UNMAP_FLAG_ALL; >>> + struct vfio_iommu_type1_dma_unmap unmap; >>> + unsigned long unmapped = 0; >>> + struct iommufd_ioas *ioas; >>> + int rc; >>> + >>> + if (copy_from_user(&unmap, arg, minsz)) >>> + return -EFAULT; >>> + >>> + if (unmap.argsz < minsz || unmap.flags & ~supported_flags) >>> + return -EINVAL; >>> + >>> + ioas = get_compat_ioas(ictx); >>> + if (IS_ERR(ioas)) >>> + return PTR_ERR(ioas); >>> + >>> + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL) { >>> + if (unmap.iova != 0 || unmap.size != 0) { >>> + rc = -EINVAL; >>> + goto err_put; >>> + } >>> + rc = iopt_unmap_all(&ioas->iopt, &unmapped); >>> + } else { >>> + if (READ_ONCE(ioas->iopt.disable_large_pages)) { >>> + unsigned long iovas[] = { unmap.iova + unmap.size - 1, >>> + unmap.iova - 1 }; >>> + >>> + rc = iopt_cut_iova(&ioas->iopt, iovas, >>> + unmap.iova ? 2 : 1); >> please can you add a comment to explain what this is supposed to do? > iova -1 when iova == 0 will underflow and becomes garbage > > /* > * Create cuts at the start and last of the requested > * range. If the start IOVA is 0 then it doesn't need to > * be cut. > */ OK thanks > >>> +static int iommufd_vfio_set_iommu(struct iommufd_ctx *ictx, unsigned long type) >>> +{ >>> + struct iommufd_ioas *ioas = NULL; >>> + int rc = 0; >>> + >>> + if (type != VFIO_TYPE1_IOMMU && type != VFIO_TYPE1v2_IOMMU) >>> + return -EINVAL; >>> + >>> + /* VFIO fails the set_iommu if there is no group */ >>> + ioas = get_compat_ioas(ictx); >>> + if (IS_ERR(ioas)) >>> + return PTR_ERR(ioas); >>> + if (type == VFIO_TYPE1_IOMMU) >>> + rc = iopt_disable_large_pages(&ioas->iopt); >> please can you document/explain this setting? > /* > * The difference between TYPE1 and TYPE1v2 is the ability to unmap in > * the middle of mapped ranges. This is complicated by huge page support > * which creates single large IOPTEs that cannot be split by the iommu > * driver. TYPE1 is very old at this point and likely nothing uses it, > * however it is simple enough to emulate by simply disabling the > * problematic large IOPTEs. Then we can safely unmap within any range. OK makes sense. That's helpful with those additions, Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx> Eric > */ > > Thanks, > Jason >