Re: [PATCH v5 15/19] iommufd: vfio container FD ioctl compatibility

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@nvidia.com/
  

• *  https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@oracle.com/
  

• */
  

• 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@redhat.com

Eric

*/

Thanks, Jason