Re: [PATCH V4 04/10] iommufd: Add an ioctl IOMMU_IOAS_GET_PA to query PA from IOVA

[Jacob Pan <jacob.pan@linux.microsoft.com>]

Hi Kevin,

On Thu, 16 Apr 2026 08:02:33 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jacob Pan <jacob.pan@linux.microsoft.com>
> > Sent: Wednesday, April 15, 2026 5:14 AM
> > 
> > To support no-IOMMU mode where userspace drivers perform unsafe DMA
> > using physical addresses, introduce a new API to retrieve the
> > physical address of a user-allocated DMA buffer that has been
> > mapped to an IOVA via IOAS. The mapping is backed by mock I/O page
> > tables maintained
> > by generic IOMMUPT framework.
> > 
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>  
> 
> Your s-o-b should be the last 
will do.

> 
> > +	/*
> > +	 * Scan the domain for the contiguous physical address
> > length so that
> > +	 * userspace search can be optimized for fewer ioctls.
> > +	 */
> > +	while (iova < iopt_area_last_iova(area)) {
> > +		unsigned long next_iova;
> > +		u64 next_paddr;
> > +
> > +		if (check_add_overflow(iova, PAGE_SIZE,
> > &next_iova))
> > +			break;
> > +
> > +		next_paddr =
> > iommu_iova_to_phys(area->storage_domain, next_iova);
> > +
> > +		if (!next_paddr || next_paddr != tmp_paddr +
> > PAGE_SIZE)
> > +			break;  
> 
> the 2nd condition should be WARN_ON? 
I don't think it should be a WARN_ON since PA non-contiguity is one of
the normal exit conditions.

> 
> > @@ -432,6 +432,7 @@ union ucmd_buffer {
> >  	struct iommu_veventq_alloc veventq;
> >  	struct iommu_vfio_ioas vfio_ioas;
> >  	struct iommu_viommu_alloc viommu;
> > +	struct iommu_ioas_get_pa get_pa;  
> 
> alphabetic order
> 
will do.

> >  #ifdef CONFIG_IOMMUFD_TEST
> >  	struct iommu_test_cmd test;
> >  #endif
> > @@ -484,6 +485,8 @@ static const struct iommufd_ioctl_op
> > iommufd_ioctl_ops[] = {
> >  		 struct iommu_ioas_map_file, iova),
> >  	IOCTL_OP(IOMMU_IOAS_UNMAP, iommufd_ioas_unmap, struct
> > iommu_ioas_unmap,
> >  		 length),
> > +	IOCTL_OP(IOMMU_IOAS_GET_PA, iommufd_ioas_get_pa, struct
> > iommu_ioas_get_pa,
> > +		 out_phys),  
> 
> ditto
will do

> > 
> > +/**
> > + * struct iommu_ioas_get_pa - ioctl(IOMMU_IOAS_GET_PA)
> > + * @size: sizeof(struct iommu_ioas_get_pa)
> > + * @flags: Reserved, must be 0 for now
> > + * @ioas_id: IOAS ID to query IOVA to PA mapping from
> > + * @__reserved: Must be 0
> > + * @iova: IOVA to query
> > + * @out_length: Number of bytes contiguous physical address
> > starting from phys
> > + * @out_phys: Output physical address the IOVA maps to
> > + *
> > + * Query the physical address backing an IOVA range. The entire
> > range must be
> > + * mapped already. For noiommu devices doing unsafe DMA only.
> > + */
> > +struct iommu_ioas_get_pa {
> > +	__u32 size;
> > +	__u32 flags;
> > +	__u32 ioas_id;
> > +	__u32 __reserved;
> > +	__aligned_u64 iova;
> > +	__aligned_u64 out_length;
> > +	__aligned_u64 out_phys;
> > +};
> > +#define IOMMU_IOAS_GET_PA _IO(IOMMUFD_TYPE,
> > IOMMUFD_CMD_IOAS_GET_PA)
> > +  
> 
> Based on its purpose let's make the restriction in the name, e.g.
> 
> IOMMUFD_CMD_IOAS_NOIOMMU_GET_PA

yeah, make sense. will rename all the function names as well.