RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)

[Liu, Yi L]

> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Sent: Wednesday, April 1, 2020 1:43 PM
> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>; alex.williamson@xxxxxxxxxx;
> Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> 
> > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > Sent: Tuesday, March 31, 2020 9:22 PM
> >
> > > From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> > > Sent: Tuesday, March 31, 2020 1:41 PM
> > > To: Liu, Yi L <yi.l.liu@xxxxxxxxx>; alex.williamson@xxxxxxxxxx;
> > > eric.auger@xxxxxxxxxx
> > > Subject: RE: [PATCH v1 1/8] vfio: Add
> > VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > >
> > > > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > > > Sent: Monday, March 30, 2020 10:37 PM
> > > >
> > > > > From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> > > > > Sent: Monday, March 30, 2020 4:32 PM
> > > > > To: Liu, Yi L <yi.l.liu@xxxxxxxxx>; alex.williamson@xxxxxxxxxx;
> > > > > Subject: RE: [PATCH v1 1/8] vfio: Add
> > > > VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > > > >
> > > > > > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > > > > > Sent: Sunday, March 22, 2020 8:32 PM
> > > > > >
> > > > > > From: Liu Yi L <yi.l.liu@xxxxxxxxx>
> > > > > >
> > > > > > For a long time, devices have only one DMA address space from
> > > > > > platform IOMMU's point of view. This is true for both bare
> > > > > > metal and directed- access in virtualization environment.
> > > > > > Reason is the source ID of DMA in PCIe are BDF (bus/dev/fnc
> > > > > > ID), which results in only device granularity
[...]
> > >
> > > >
> > > > > > +
> > > > > > +		switch (req.flags & VFIO_PASID_REQUEST_MASK) {
> > > > > > +		case VFIO_IOMMU_PASID_ALLOC:
> > > > > > +		{
> > > > > > +			int ret = 0, result;
> > > > > > +
> > > > > > +			result =
> > vfio_iommu_type1_pasid_alloc(iommu,
> > > > > > +
> > 	req.alloc_pasid.min,
> > > > > > +
> > 	req.alloc_pasid.max);
> > > > > > +			if (result > 0) {
> > > > > > +				offset = offsetof(
> > > > > > +					struct
> > > > > > vfio_iommu_type1_pasid_request,
> > > > > > +					alloc_pasid.result);
> > > > > > +				ret = copy_to_user(
> > > > > > +					      (void __user *) (arg +
> > offset),
> > > > > > +					      &result, sizeof(result));
> > > > > > +			} else {
> > > > > > +				pr_debug("%s: PASID alloc failed\n",
> > > > > > __func__);
> > > > > > +				ret = -EFAULT;
> > > > >
> > > > > no, this branch is not for copy_to_user error. it is about pasid
> > > > > alloc failure. you should handle both.
> > > >
> > > > Emmm, I just want to fail the IOCTL in such case, so the @result
> > > > field is meaningless in the user side. How about using another
> > > > return value (e.g. ENOSPC) to indicate the IOCTL failure?
> > >
> > > If pasid_alloc fails, you return its result to userspace if
> > > copy_to_user fails, then return -EFAULT.
> > >
> > > however, above you return -EFAULT for pasid_alloc failure, and then
> > > the number of not-copied bytes for copy_to_user.
> >
> > not quite get. Let me re-paste the code. :-)
> >
> > +		case VFIO_IOMMU_PASID_ALLOC:
> > +		{
> > +			int ret = 0, result;
> > +
> > +			result = vfio_iommu_type1_pasid_alloc(iommu,
> > +							req.alloc_pasid.min,
> > +							req.alloc_pasid.max);
> > +			if (result > 0) {
> > +				offset = offsetof(
> > +					struct
> > vfio_iommu_type1_pasid_request,
> > +					alloc_pasid.result);
> > +				ret = copy_to_user(
> > +					      (void __user *) (arg + offset),
> > +					      &result, sizeof(result));
> > if copy_to_user failed, ret is the number of uncopied bytes and will
> > be returned to userspace to indicate failure. userspace will not use
> > the data in result field. perhaps, I should check the ret here and
> > return -EFAULT or another errno, instead of return the number of
> > un-copied bytes.
> 
> here should return -EFAULT.

got it. so if any failure, the return value of this ioctl is a -ERROR_VAL.

> 
> > +			} else {
> > +				pr_debug("%s: PASID alloc failed\n",
> > __func__);
> > +				ret = -EFAULT;
> > if vfio_iommu_type1_pasid_alloc() failed, no doubt, return -EFAULT to
> > userspace to indicate failure.
> 
> pasid_alloc has its own error types returned. why blindly replace it with -EFAULT?

right, should use its own error types.

Thanks,
Yi Liu