Re: [PATCH v2 13/22] iommufd: Add mmap interface

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Apr 28, 2025 at 10:50:32AM +0800, Baolu Lu wrote:
> On 4/26/25 13:58, Nicolin Chen wrote:
> > For vIOMMU passing through HW resources to user space (VMs), add an mmap
> > infrastructure to map a region of hardware MMIO pages.
> > 
> > Maintain an mt_mmap per ictx for validations. To allow IOMMU drivers to
> > add and delete mmappable regions to/from the mt_mmap, add a pair of new
> > helpers: iommufd_ctx_alloc_mmap() and iommufd_ctx_free_mmap().
> 
> I am wondering why the dma_buf mechanism isn't used here, considering
> that this also involves an export and import pattern.

The use case here is to expose one small MMIO page for user space
to directly HW control, so mmap seems to be a good fit. What would
it benefit from using dma_buf here?

> > @@ -55,6 +57,12 @@ struct iommufd_ctx {
> >   	struct iommufd_ioas *vfio_ioas;
> >   };
> > +/* Entry for iommufd_ctx::mt_mmap */
> > +struct iommufd_mmap {
> > +	unsigned long pfn_start;
> > +	unsigned long pfn_end;
> > +};
> 
> This structure is introduced to represent a mappable/mapped region,
> right? It would be better to add comments specifying whether the start
> and end are inclusive or exclusive.

Yes. Sure I can add that pfn_start/pfn_end are inclusive.

> > diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
> > index fb7f8fe40f95..c55336c580dc 100644
> > --- a/drivers/iommu/iommufd/driver.c
> > +++ b/drivers/iommu/iommufd/driver.c
> > @@ -78,6 +78,45 @@ void iommufd_object_undepend(struct iommufd_object *obj_dependent,
> >   }
> >   EXPORT_SYMBOL_NS_GPL(iommufd_object_undepend, "IOMMUFD");
> > +/* Driver should report the output @immap_id to user space for mmap() syscall */
> > +int iommufd_ctx_alloc_mmap(struct iommufd_ctx *ictx, phys_addr_t base,
> > +			   size_t size, unsigned long *immap_id)
> > +{
> > +	struct iommufd_mmap *immap;
> > +	int rc;
> > +
> > +	if (WARN_ON_ONCE(!immap_id))
> > +		return -EINVAL;
> > +	if (base & ~PAGE_MASK)
> > +		return -EINVAL;
> 
> Is it equal to PAGE_ALIGNED()?

Yes. Will change.

> > +void iommufd_ctx_free_mmap(struct iommufd_ctx *ictx, unsigned long immap_id)
> > +{
> > +	kfree(mtree_erase(&ictx->mt_mmap, immap_id >> PAGE_SHIFT));
> 
> MMIO lifecycle question: what happens if a region is removed from the
> maple tree (and is therefore no longer mappable), but is still mapped
> and in use by userspace?

That's a good point!

Yea, mmap() should refcount an object to prevent its destroy call
where this iommufd_ctx_free_mmap gets called. So, these two could
probably be iommufd_object_alloc_mmap/unmmap().

And I need to find some callback in the munmap path to release the
reference..

Thanks
Nicolin