Messages in this thread | | | Subject | Re: [PATCH v7 2/3] uacce: add uacce driver | From | zhangfei <> | Date | Wed, 6 Nov 2019 16:17:40 +0800 |
| |
Hi, Jean
Thanks for the review.
On 2019/11/5 下午7:48, Jean-Philippe Brucker wrote: > Hi Zhangfei, > > Thanks for simplifying this, it's a lot easier to review. I have some > additional comments. > > On Tue, Oct 29, 2019 at 02:40:15PM +0800, Zhangfei Gao wrote: >> +static int uacce_sva_exit(struct device *dev, struct iommu_sva *handle, >> + void *data) >> +{ >> + struct uacce_device *uacce = data; >> + struct uacce_queue *q; >> + >> + mutex_lock(&uacce->q_lock); >> + list_for_each_entry(q, &uacce->qs, list) { >> + if (q->pid == task_pid_nr(current)) >> + uacce_put_queue(q); > This won't work in some cases, because any thread can call __mmput() and > end up here. For example a sibling thread that inherited the queue, or a > workqueue that's executing mmput_async_fn(). In addition I think comparing > PID values is unsafe (see comment in pid.h), we'd need to use the struct > pid if we wanted to do it this way. OK, still in check. > > But I still believe it would be better to create an uacce_mm structure > that tracks all queues bound to this mm, and pass that to uacce_sva_exit > instead of the uacce_device. I am afraid this method may not work. Since currently iommu_sva_bind_device only accept the same drvdata for the same dev, that's the reason we can not directly use "queue" as drvdata. Each time create an uacce_mm structure should be same problem as queue, and fail for same dev. So we use uacce and pick up the right queue inside.
> > The queue isn't bound to a task, but its address space. With clone() the > address space can be shared between tasks. In addition, whoever has a > queue fd also gets access to this address space. So after a fork() the > child may be able to program the queue to DMA into the parent's address > space, even without CLONE_VM. Users must be aware of this and I think it's > important to explain it very clearly in the UAPI. > > [...] >> +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_device *uacce = q->uacce; >> + struct uacce_qfile_region *qfr; >> + int ret = -ENOMEM; >> + >> + qfr = kzalloc(sizeof(*qfr), GFP_KERNEL); >> + if (!qfr) >> + return ERR_PTR(-ENOMEM); >> + >> + qfr->type = type; >> + qfr->flags = flags; >> + >> + if (vma->vm_flags & VM_READ) >> + qfr->prot |= IOMMU_READ; > qfr->prot and qfr->flags aren't used at the moment, you could remove them. Yes, > >> + >> + 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; >> + } >> + >> + return qfr; >> + >> +err_with_qfr: >> + kfree(qfr); >> + return ERR_PTR(ret); >> +} >> + >> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma) >> +{ >> + struct uacce_queue *q = filep->private_data; >> + struct uacce_device *uacce = q->uacce; >> + struct uacce_qfile_region *qfr; >> + enum uacce_qfrt type = 0; >> + unsigned int flags = 0; >> + int ret; >> + >> + if (vma->vm_pgoff < UACCE_QFRT_MAX) >> + type = vma->vm_pgoff; > Otherwise return -EINVAL? type probably shouldn't default to MMIO if it > wasn't explicitly requested by the user. OK > >> + >> + vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK; >> + vma->vm_ops = &uacce_vm_ops; >> + vma->vm_private_data = q; >> + >> + mutex_lock(&uacce_mutex); >> + >> + if (q->qfrs[type]) { >> + ret = -EEXIST; >> + goto out_with_lock; >> + } >> + >> + switch (type) { >> + case UACCE_QFRT_MMIO: >> + flags = UACCE_QFRF_SELFMT; >> + break; >> + >> + case UACCE_QFRT_DUS: >> + if (uacce->flags & UACCE_DEV_SVA) { >> + flags = UACCE_QFRF_SELFMT; > I'd simplify this even further by getting rid of the SELFMT flag. It's the > only possibility at the moment. OK, we can remove this flag for simplicity, may add it back if required in future patch. > >> + break; >> + } >> + break; >> + >> + default: >> + WARN_ON(&uacce->dev); > WARN_ON(uacce->dev). But shouldn't we instead return -EINVAL here? > UACCE_QFRT_MAX is currently 16, so users can easily trigger this WARN by > passing an invalid value. Yes, good idea. > > [...] >> +void uacce_unregister(struct uacce_device *uacce) >> +{ >> + if (!uacce) >> + return; >> + >> + mutex_lock(&uacce->q_lock); >> + if (!list_empty(&uacce->qs)) { >> + struct uacce_queue *q; >> + >> + list_for_each_entry(q, &uacce->qs, list) { >> + uacce_put_queue(q); > The open file descriptor will still exist after this function returns. > Can all fops can be called with a stale queue? To more clear:. Do you mean rmmod without fops_release.
Thanks
| |