RE: [PATCH 12/13] vfio: Add ioctls for device cdev iommufd

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

 



> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Sent: Friday, January 20, 2023 4:03 PM
> 
> > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > Sent: Tuesday, January 17, 2023 9:50 PM
> >
> > This adds two vfio device ioctls for userspace using iommufd on vfio
> > devices.
> >
> >     VFIO_DEVICE_BIND_IOMMUFD: bind device to an iommufd, hence gain
> > DMA
> > 			      control provided by the iommufd. VFIO no
> > 			      iommu is indicated by passing a minus
> > 			      fd value.
> 
> Can't this be a flag bit for better readability than using a special value?
> 
> >     VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach device to ioas, page
> tables
> > 				   managed by iommufd. Attach can be
> > 				   undo by passing IOMMUFD_INVALID_ID
> > 				   to kernel.
> 
> With Alex' remark we need a separate DETACH cmd now.

Yes.

> >
> > +	/*
> > +	 * For group path, iommufd pointer is NULL when comes into this
> > +	 * helper. Its noiommu support is in container.c.
> > +	 *
> > +	 * For iommufd compat mode, iommufd pointer here is a valid value.
> > +	 * Its noiommu support is supposed to be in vfio_iommufd_bind().
> > +	 *
> > +	 * For device cdev path, iommufd pointer here is a valid value for
> > +	 * normal cases, but it is NULL if it's noiommu. The reason is
> > +	 * that userspace uses iommufd==-1 to indicate noiommu mode in
> > this
> > +	 * path. So caller of this helper will pass in a NULL iommufd
> > +	 * pointer. To differentiate it from the group path which also
> > +	 * passes NULL iommufd pointer in, df->noiommu is used. For cdev
> > +	 * noiommu, df->noiommu would be set to mark noiommu case for
> > cdev
> > +	 * path.
> > +	 *
> > +	 * So if df->noiommu is set then this helper just goes ahead to
> > +	 * open device. If not, it depends on if iommufd pointer is NULL
> > +	 * to handle the group path, iommufd compat mode, normal cases
> in
> > +	 * the cdev path.
> > +	 */
> >  	if (iommufd)
> >  		ret = vfio_iommufd_bind(device, iommufd, dev_id, pt_id);
> > -	else
> > +	else if (!df->noiommu)
> >  		ret = vfio_device_group_use_iommu(device);
> >  	if (ret)
> >  		goto err_module_put;
> 
> Isn't 'ret' uninitialized when df->noiommu is true?

Done.

> > +static int vfio_ioctl_device_attach(struct vfio_device *device,
> > +				    struct vfio_device_feature __user *arg)
> > +{
> > +	struct vfio_device_attach_iommufd_pt attach;
> > +	int ret;
> > +	bool is_attach;
> > +
> > +	if (copy_from_user(&attach, (void __user *)arg, sizeof(attach)))
> > +		return -EFAULT;
> > +
> > +	if (attach.flags)
> > +		return -EINVAL;
> > +
> > +	if (!device->ops->bind_iommufd)
> > +		return -ENODEV;
> > +
> 
> this should fail if noiommu is true.

Yes.

Regards,
Yi Liu




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux