On Fri, 10 Jan 2020 14:55:39 +0800 "[email protected]" <[email protected]> wrote:
> On 2020/1/10 上午1:38, Jonathan Cameron wrote: > > On Mon, 16 Dec 2019 11:08:15 +0800 > > Zhangfei Gao <[email protected]> wrote: > > > >> From: Kenneth Lee <[email protected]> > >> > >> Uacce (Unified/User-space-access-intended Accelerator Framework) targets to > >> provide Shared Virtual Addressing (SVA) between accelerators and processes. > >> So accelerator can access any data structure of the main cpu. > >> This differs from the data sharing between cpu and io device, which share > >> only data content rather than address. > >> Since unified address, hardware and user space of process can share the > >> same virtual address in the communication. > >> > >> Uacce create a chrdev for every registration, the queue is allocated to > >> the process when the chrdev is opened. Then the process can access the > >> hardware resource by interact with the queue file. By mmap the queue > >> file space to user space, the process can directly put requests to the > >> hardware without syscall to the kernel space. > >> > >> The IOMMU core only tracks mm<->device bonds at the moment, because it > >> only needs to handle IOTLB invalidation and PASID table entries. However > >> uacce needs a finer granularity since multiple queues from the same > >> device can be bound to an mm. When the mm exits, all bound queues must > >> be stopped so that the IOMMU can safely clear the PASID table entry and > >> reallocate the PASID. > >> > >> An intermediate struct uacce_mm links uacce devices and queues. > >> Note that an mm may be bound to multiple devices but an uacce_mm > >> structure only ever belongs to a single device, because we don't need > >> anything more complex (if multiple devices are bound to one mm, then > >> we'll create one uacce_mm for each bond). > >> > >> uacce_device --+-- uacce_mm --+-- uacce_queue > >> | '-- uacce_queue > >> | > >> '-- uacce_mm --+-- uacce_queue > >> +-- uacce_queue > >> '-- uacce_queue > >> > >> Signed-off-by: Kenneth Lee <[email protected]> > >> Signed-off-by: Zaibo Xu <[email protected]> > >> Signed-off-by: Zhou Wang <[email protected]> > >> Signed-off-by: Jean-Philippe Brucker <[email protected]> > >> Signed-off-by: Zhangfei Gao <[email protected]> > > Hi, > > > > Two small things I'd missed previously. Fix those and for > > what it's worth > > > > Reviewed-by: Jonathan Cameron <[email protected]> > Thanks Jonathan > > > >> --- > >> Documentation/ABI/testing/sysfs-driver-uacce | 37 ++ > >> drivers/misc/Kconfig | 1 + > >> drivers/misc/Makefile | 1 + > >> drivers/misc/uacce/Kconfig | 13 + > >> drivers/misc/uacce/Makefile | 2 + > >> drivers/misc/uacce/uacce.c | 628 > >> +++++++++++++++++++++++++++ > >> include/linux/uacce.h | 161 +++++++ > >> include/uapi/misc/uacce/uacce.h | 38 ++ > >> 8 files changed, 881 insertions(+) > >> create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce > >> create mode 100644 drivers/misc/uacce/Kconfig > >> create mode 100644 drivers/misc/uacce/Makefile > >> create mode 100644 drivers/misc/uacce/uacce.c > >> create mode 100644 include/linux/uacce.h > >> create mode 100644 include/uapi/misc/uacce/uacce.h > >> > > ... > >> + > >> +What: /sys/class/uacce/<dev_name>/available_instances > >> +Date: Dec 2019 > >> +KernelVersion: 5.6 > >> +Contact: [email protected] > >> +Description: Available instances left of the device > >> + Return -ENODEV if uacce_ops get_available_instances is > >> not provided > >> + > > See below. It doesn't "return" it prints it currently. > Will update to > 'unknown' if uacce_ops get_available_instances is not provided > > > > ... > > > >> +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 = UACCE_MAX_REGION; > >> + int ret = 0; > >> + > >> + if (vma->vm_pgoff < UACCE_MAX_REGION) > >> + type = vma->vm_pgoff; > >> + else > >> + return -EINVAL; > >> + > >> + qfr = kzalloc(sizeof(*qfr), GFP_KERNEL); > >> + if (!qfr) > >> + return -ENOMEM; > >> + > >> + vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK; > >> + vma->vm_ops = &uacce_vm_ops; > >> + vma->vm_private_data = q; > >> + qfr->type = type; > >> + > >> + mutex_lock(&uacce_mutex); > >> + > >> + if (q->state != UACCE_Q_INIT && q->state != UACCE_Q_STARTED) { > >> + ret = -EINVAL; > >> + goto out_with_lock; > >> + } > >> + > >> + if (q->qfrs[type]) { > >> + ret = -EEXIST; > >> + goto out_with_lock; > >> + } > >> + > >> + switch (type) { > >> + case UACCE_QFRT_MMIO: > >> + if (!uacce->ops->mmap) { > >> + ret = -EINVAL; > >> + goto out_with_lock; > >> + } > >> + > >> + ret = uacce->ops->mmap(q, vma, qfr); > >> + if (ret) > >> + goto out_with_lock; > >> + > >> + break; > >> + > >> + case UACCE_QFRT_DUS: > >> + if (uacce->flags & UACCE_DEV_SVA) { > >> + if (!uacce->ops->mmap) { > >> + ret = -EINVAL; > >> + goto out_with_lock; > >> + } > >> + > >> + ret = uacce->ops->mmap(q, vma, qfr); > >> + if (ret) > >> + goto out_with_lock; > >> + } > > Slightly odd corner case, but what stops us getting here with > > the UACCE_DEV_SVA flag not set? If that happened I'd expect to > > return an error but looks like we return 0. > The check with flag UACCE_DEV_SVA can be removed here, non-sva also has > dus region. > We have removed the check when we add non-sva support. > > ... > > > >> +static ssize_t available_instances_show(struct device *dev, > >> + struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + struct uacce_device *uacce = to_uacce_device(dev); > >> + int val = -ENODEV; > >> + > >> + if (uacce->ops->get_available_instances) > >> + val = uacce->ops->get_available_instances(uacce); > >> + > >> + return sprintf(buf, "%d\n", val); > > It's unusual to pass an error value back as a string. > > I'd expect some logic like.. > > > > if (val < 0) > > return val; > > > > return sprintf(buf, "%d\n", val); > > > > Note this is the documented behavior "returns -ENODEV". > If return -ENODEV, > cat /sys/class/uacce/hisi_zip-0/available_instances > cat: /sys/class/uacce/hisi_zip-0/available_instances: No such device > > I think print "unknown" maybe better, like cpufreq.c > > if (uacce->ops->get_available_instances) > return sprintf(buf, "%d\n", > uacce->ops->get_available_instances(uacce)); > > return sprintf(buf, "unknown\n"); From userspace code point a simple error code return is better than a 'magic' string in the file. You'll find people just try to read an integer without checking for unknown and hence get a very odd result. Much better to throw them an error code. Jonathan > > Thanks > > > _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
