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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx/
>>> +	 *  https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@xxxxxxxxxx/
>>> +	 */
>>> +	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@xxxxxxxxxx>

Eric


> 	 */
>
> Thanks,
> Jason
>




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux