Messages in this thread | | | Subject | Re: [PATCH v7 2/3] uacce: add uacce driver | From | zhangfei <> | Date | Thu, 7 Nov 2019 21:23:50 +0800 |
| |
On 2019/11/6 下午11:32, Jean-Philippe Brucker wrote: > On Wed, Nov 06, 2019 at 04:17:40PM +0800, zhangfei wrote: >>> 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. > What I had in mind is keep one uacce_mm per mm and per device, and we can > pass that to iommu_sva_bind_device(). It requires some structure changes, > see the attached patch. Cool, thanks Jean How about merge them together. > >>> 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. >>> [...] >>>> +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. > Yes I think so. What happens when userspace starts some queues, and > the device driver suddenly calls uacce_unregister(). We call > cdev_device_del() later in this function, but quoting the documentation: > "any cdevs already open will remain and their fops will still be callable > even after this function returns." So we need to make sure that any of the > fops is safe to run after the uacce device disappears. We can protect stale queue via q->state, since q is released later in fops_release. And uacce_unregister: put_queue will set q->state = UACCE_Q_ZOMBIE. Will add state check in mmap too. > > I noticed a lock dependency inversion on uacce->q_lock: uacce_unregister() > calls iommu_sva_unbind_device() while holding the uacce->q_lock, but > uacce_sva_exit() takes the uacce->q_lock with the SVA lock held. In theory > we could simply avoid calling iommu_sva_unbind_device() here since it will > be done by fops_release(), but then disabling the SVA feature in > uacce_unregister() won't work (because there still are bonds). The > attached patch should fix it, but I haven't tried running uacce_register() > yet. Have tested, it is OK.
Thanks
| |