Re: [PATCH v6 2/3] uacce: add uacce driver

[Jerome Glisse <jglisse@redhat.com>]

On Wed, Oct 16, 2019 at 07:28:02PM +0200, Jean-Philippe Brucker wrote:
[...]

> > +static struct uacce_qfile_region *
> > +uacce_create_region(struct uacce_queue *q, struct vm_area_struct *vma,
> > +		    enum uacce_qfrt type, unsigned int flags)
> > +{
> > +	struct uacce_qfile_region *qfr;
> > +	struct uacce_device *uacce = q->uacce;
> > +	unsigned long vm_pgoff;
> > +	int ret = -ENOMEM;
> > +
> > +	qfr = kzalloc(sizeof(*qfr), GFP_ATOMIC);
> > +	if (!qfr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	qfr->type = type;
> > +	qfr->flags = flags;
> > +	qfr->iova = vma->vm_start;
> > +	qfr->nr_pages = vma_pages(vma);
> > +
> > +	if (vma->vm_flags & VM_READ)
> > +		qfr->prot |= IOMMU_READ;
> > +
> > +	if (vma->vm_flags & VM_WRITE)
> > +		qfr->prot |= IOMMU_WRITE;
> > +
> > +	if (flags & UACCE_QFRF_SELFMT) {
> > +		if (!uacce->ops->mmap) {
> > +			ret = -EINVAL;
> > +			goto err_with_qfr;
> > +		}
> > +
> > +		ret = uacce->ops->mmap(q, vma, qfr);
> > +		if (ret)
> > +			goto err_with_qfr;
> > +		return qfr;
> > +	}
> 
> I wish the SVA and !SVA paths were less interleaved. Both models are
> fundamentally different:
> 
> * Without SVA you cannot share the device between multiple processes. All
>   DMA mappings are in the "main", non-PASID address space of the device.
> 
>   Note that process isolation without SVA could be achieved with the
>   auxiliary domains IOMMU API (introduced primarily for vfio-mdev) but
>   this is not the model chosen here.
> 
> * With SVA you can share the device between multiple processes. But if the
>   process can somehow program its portion of the device to access the main
>   address space, you loose isolation. Only the kernel must be able to
>   program and access the main address space.
> 
> When interleaving both code paths it's easy to make a mistake and loose
> this isolation. Although I think this code is correct, it took me some
> time to understand that we never end up calling dma_alloc or iommu_map
> when using SVA. Might be worth at least adding a check that if
> UACCE_DEV_SVA, then we never end up in the bottom part of this function.

I would go even further, just remove the DMA path as it is not use.
But yes at bare minimum it needs to be completely separate to avoid
confusion.


[...]


> > +static int uacce_fops_open(struct inode *inode, struct file *filep)
> > +{
> > +	struct uacce_queue *q;
> > +	struct iommu_sva *handle = NULL;
> > +	struct uacce_device *uacce;
> > +	int ret;
> > +	int pasid = 0;
> > +
> > +	uacce = idr_find(&uacce_idr, iminor(inode));
> > +	if (!uacce)
> > +		return -ENODEV;
> > +
> > +	if (!try_module_get(uacce->pdev->driver->owner))
> > +		return -ENODEV;
> > +
> > +	ret = uacce_dev_open_check(uacce);
> > +	if (ret)
> > +		goto out_with_module;
> > +
> > +	if (uacce->flags & UACCE_DEV_SVA) {
> > +		handle = iommu_sva_bind_device(uacce->pdev, current->mm, NULL);
> > +		if (IS_ERR(handle))
> > +			goto out_with_module;
> > +		pasid = iommu_sva_get_pasid(handle);
> 
> We need to register an mm_exit callback. Once we return, userspace will
> start running jobs on the accelerator. If the process is killed while the
> accelerator is running, the mm_exit callback tells the device driver to
> stop using this PASID (stop_queue()), so that it can be reallocated for
> another process.
> 
> Implementing this with the right locking and ordering can be tricky. I'll
> try to implement the callback and test it on the device this week.

It already exist it is call mmu notifier, you can register an mmu notifier
and get callback once the mm exit.

Cheers,
Jérôme