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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Eric Auger <eric.auger@redhat.com>
Cc: bpf@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
	David Woodhouse <dwmw2@infradead.org>,
	iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	Kevin Tian <kevin.tian@intel.com>,
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org,
	llvm@lists.linux.dev, Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Shuah Khan <shuah@kernel.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Tom Rix <trix@redhat.com>, Will Deacon <will@kernel.org>,
	Anthony Krowiak <akrowiak@linux.ibm.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Chaitanya Kulkarni <chaitanyak@nvidia.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Daniel Jordan <daniel.m.jordan@oracle.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Eric Farman <farman@linux.ibm.com>,
	Jason Wang <jasowang@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Jason Herne <jjherne@linux.ibm.com>,
	Joao Martins <joao.m.martins@oracle.com>,
	kvm@vger.kernel.org, Lixiao Yang <lixiao.yang@intel.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	Yi Liu <yi.l.liu@intel.com>, Keqian Zhu <zhukeqian1@huawei.com>
Subject: Re: [PATCH v5 15/19] iommufd: vfio container FD ioctl compatibility
Date: Mon, 28 Nov 2022 15:37:06 -0400	[thread overview]
Message-ID: <Y4UN4qVpNqTP/JEF@nvidia.com> (raw)
In-Reply-To: <81f898b6-e40f-be72-78d4-b5d836981d8f@redhat.com>

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.

> > +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.
			 */

> > +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.
	 */

Thanks,
Jason