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
