RE: [PATCH v1 10/16] iommufd: Add mmap interface

Tian, Kevin
2025-04-21T08:17:04Z

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, April 11, 2025 2:38 PM
>
> For vIOMMU passing through HW resources to user space (VMs), add an
> mmap
> infrastructure to map a region of hardware MMIO pages. The addr and size
> should be given previously via a prior IOMMU_VIOMMU_ALLOC ioctl in some
> output fields of the structure.

According to the code the addr must be the immap_id given by previous
alloc but size can be any as long as it doesn't exceed the physical length.

>
> +/* Entry for iommufd_ctx::mt_mmap */
> +struct iommufd_mmap {
> + unsigned long pfn_start;
> + unsigned long pfn_end;
> + bool is_io;
> +};

what is the point of 'is_io' here? Do you intend to allow userspace to
mmap anonymous memory via iommufd?

anyway for now the only user in this series always sets it to true.

I'd suggest to remove it until there is a real need.

>
> +/*
> + * The pfn and size carried in @vma from the user space mmap call should
> be

there is no 'pfn' carried in the mmap call. It's vm_pgoff.

> + * previously given to user space via a prior ioctl output.
> + */
> +static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> + struct iommufd_ctx *ictx = filp->private_data;
> + size_t size = vma->vm_end - vma->vm_start;
> + struct iommufd_mmap *immap;
> +
> + if (size & ~PAGE_MASK)
> + return -EINVAL;
> + if (!(vma->vm_flags & VM_SHARED))
> + return -EINVAL;
> + if (vma->vm_flags & VM_EXEC)
> + return -EPERM;
> +
> + /* vm_pgoff carries an index of an mtree entry/immap */
> + immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff);
> + if (!immap)
> + return -EINVAL;
> + if (size >> PAGE_SHIFT > immap->pfn_end - immap->pfn_start + 1)
> + return -EINVAL;

Do we want to document in uAPI that iommufd mmap allows to map
a sub-region (starting from offset zero) of the reported size from earlier
alloc ioctl, but not from random offset (of course impossible by forcing
vm_pgoff to be a mtree index)?