Messages in this thread | | | Subject | Re: [PATCH v3 2/2] uacce: add uacce driver | From | zhangfei <> | Date | Thu, 12 Sep 2019 22:06:09 +0800 |
| |
Hi, Greg
Thanks for the careful review.
On 2019/9/4 下午8:50, Greg Kroah-Hartman wrote: > On Tue, Sep 03, 2019 at 12:14:47PM +0800, Zhangfei Gao wrote: >> +/** >> + * uacce_wake_up - Wake up the process who is waiting this queue >> + * @q the accelerator queue to wake up >> + */ >> +void uacce_wake_up(struct uacce_queue *q) >> +{ >> + wake_up_interruptible(&q->wait); >> +} >> +EXPORT_SYMBOL_GPL(uacce_wake_up); > You are exporting things that have no in-kernel user, which is not > allowed. > > Can you redo this patch series with an actual user of this new api you > are creating? Otherwise we have no way of properly understanding just > what is going on here. > > Also, do you really need a function to do the above? Why? This function is used by other driver to notify the poll_wait can be started, like when data in fifo is ready. I think we can directly use wake_up_interruptible(&q->wait), then the exporting is not required. >> +static int uacce_dev_open_check(struct uacce_device *uacce) >> +{ >> + /* >> + * The device can be opened once if it dose not support pasid >> + */ >> + if (uacce->flags & UACCE_DEV_PASID) >> + return 0; >> + >> + if (atomic_cmpxchg(&uacce->state, UACCE_ST_INIT, UACCE_ST_OPENED) != >> + UACCE_ST_INIT) { >> + dev_info(&uacce->dev, "this device can be openned only once\n"); > No, NEVER do this, it's wrong, an easy way to spam the syslog for no > good reason, and flat out does not work at all. > > Just let userspace deal with something like this, if they want to open > multiple times, let it deal with the fallout. Just like a tty device > node. Yes, understand now, this print can be triggered by user space. > >> + return -EBUSY; >> + } >> + >> + return 0; >> +} >> + >> +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 (atomic_read(&uacce->state) == UACCE_ST_RST) >> + return -EINVAL; > What happens if the state changes _right_ after this check? > >> + >> + if ((!uacce->ops->get_queue) || (!uacce->ops->start_queue)) >> + return -EINVAL; > How would these two functions ever not be NULL? > > Why check for them in open()? Why not when you register the operations? OK, make sense. > >> + if (!try_module_get(uacce->pdev->driver->owner)) >> + return -ENODEV; >> + >> + ret = uacce_dev_open_check(uacce); >> + if (ret) >> + goto open_err; >> + >> +#ifdef CONFIG_IOMMU_SVA >> + if (uacce->flags & UACCE_DEV_PASID) { >> + handle = iommu_sva_bind_device(uacce->pdev, current->mm, NULL); >> + if (IS_ERR(handle)) >> + goto open_err; >> + pasid = iommu_sva_get_pasid(handle); >> + } >> +#endif >> + ret = uacce->ops->get_queue(uacce, pasid, &q); >> + if (ret < 0) >> + goto open_err; >> + >> + q->pasid = pasid; >> + q->handle = handle; >> + q->uacce = uacce; >> + q->mm = current->mm; >> + memset(q->qfrs, 0, sizeof(q->qfrs)); >> + INIT_LIST_HEAD(&q->list); >> + init_waitqueue_head(&q->wait); >> + filep->private_data = q; >> + mutex_lock(&uacce->q_lock); >> + list_add(&q->q_dev, &uacce->qs); >> + mutex_unlock(&uacce->q_lock); >> + >> + return 0; >> + >> +open_err: >> + module_put(uacce->pdev->driver->owner); >> + return ret; >> +} >> + >> +static int uacce_fops_release(struct inode *inode, struct file *filep) >> +{ >> + struct uacce_queue *q = filep->private_data; >> + struct uacce_qfile_region *qfr; >> + struct uacce_device *uacce = q->uacce; >> + bool is_to_free_region; >> + int free_pages = 0; >> + int i; >> + >> + mutex_lock(&uacce->q_lock); >> + list_del(&q->q_dev); >> + mutex_unlock(&uacce->q_lock); >> + >> + if (atomic_read(&uacce->state) == UACCE_ST_STARTED && >> + uacce->ops->stop_queue) >> + uacce->ops->stop_queue(q); > Same question as before, what happens right after this check? > > Do not use an atomic value thinking it is how to properly test for state > changes. Use a "real" lock for this, otherwise it does not work at all. Will change this atomic, thanks >> + >> + mutex_lock(&uacce_mutex); >> + >> + for (i = 0; i < UACCE_QFRT_MAX; i++) { >> + qfr = q->qfrs[i]; >> + if (!qfr) >> + continue; >> + >> + is_to_free_region = false; >> + uacce_queue_unmap_qfr(q, qfr); >> + if (i == UACCE_QFRT_SS) { >> + list_del(&q->list); >> + if (list_empty(&qfr->qs)) >> + is_to_free_region = true; >> + } else >> + is_to_free_region = true; >> + >> + if (is_to_free_region) { >> + free_pages += qfr->nr_pages; >> + uacce_destroy_region(q, qfr); >> + } >> + >> + qfr = NULL; >> + } >> + >> + mutex_unlock(&uacce_mutex); >> + >> + if (current->mm == q->mm) { >> + down_write(&q->mm->mmap_sem); >> + q->mm->data_vm -= free_pages; >> + up_write(&q->mm->mmap_sem); >> + } >> + >> +#ifdef CONFIG_IOMMU_SVA >> + if (uacce->flags & UACCE_DEV_PASID) >> + iommu_sva_unbind_device(q->handle); >> +#endif >> + >> + if (uacce->ops->put_queue) >> + uacce->ops->put_queue(q); >> + >> + atomic_set(&uacce->state, UACCE_ST_INIT); >> + module_put(uacce->pdev->driver->owner); >> + >> + return 0; >> +} >> + >> +static enum uacce_qfrt uacce_get_region_type(struct uacce_device *uacce, >> + struct vm_area_struct *vma) >> +{ >> + enum uacce_qfrt type = UACCE_QFRT_MAX; >> + size_t next_start = UACCE_QFR_NA; >> + int i; >> + >> + for (i = UACCE_QFRT_MAX - 1; i >= 0; i--) { >> + if (vma->vm_pgoff >= uacce->qf_pg_start[i]) { >> + type = i; >> + break; >> + } >> + } >> + >> + switch (type) { >> + case UACCE_QFRT_MMIO: >> + if (!uacce->ops->mmap) { >> + dev_err(&uacce->dev, "no driver mmap!\n"); >> + return UACCE_QFRT_MAX; > You not return an error? > >> + } >> + break; >> + >> + case UACCE_QFRT_DKO: >> + if (uacce->flags & UACCE_DEV_PASID) >> + return UACCE_QFRT_MAX; > Same here, no error? > >> + break; >> + >> + case UACCE_QFRT_DUS: >> + break; >> + >> + case UACCE_QFRT_SS: >> + /* todo: this can be valid to protect the process space */ >> + if (uacce->flags & UACCE_DEV_FAULT_FROM_DEV) >> + return UACCE_QFRT_MAX; > error? > >> + break; >> + >> + default: >> + dev_err(&uacce->dev, "uacce bug (%d)!\n", type); > Are you now allowing userspace to spam the kernel log if you send it > random crud? Yes, understand, will remove this, The print can be triggered from user space > >> + return UACCE_QFRT_MAX; > Return a proper error here and lots of other places. OK. > >> + } >> + >> + /* make sure the mapping size is exactly the same as the region */ >> + if (type < UACCE_QFRT_SS) { >> + for (i = type + 1; i < UACCE_QFRT_MAX; i++) >> + if (uacce->qf_pg_start[i] != UACCE_QFR_NA) { >> + next_start = uacce->qf_pg_start[i]; >> + break; >> + } >> + >> + if (next_start == UACCE_QFR_NA) { >> + dev_err(&uacce->dev, "uacce config error: SS offset set improperly\n"); >> + return UACCE_QFRT_MAX; >> + } >> + >> + if (vma_pages(vma) != >> + next_start - uacce->qf_pg_start[type]) { >> + dev_err(&uacce->dev, "invalid mmap size (%ld vs %ld pages) for region %s.\n", >> + vma_pages(vma), >> + next_start - uacce->qf_pg_start[type], >> + qfrt_str[type]); >> + return UACCE_QFRT_MAX; >> + } >> + } >> + >> + return type; >> +} >> + >> +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; >> + enum uacce_qfrt type = uacce_get_region_type(uacce, vma); >> + struct uacce_qfile_region *qfr; >> + unsigned int flags = 0; >> + int ret; >> + >> + if (type == UACCE_QFRT_MAX) >> + return -EINVAL; >> + >> + vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND; >> + >> + mutex_lock(&uacce_mutex); >> + >> + /* fixme: if the region need no pages, we don't need to check it */ >> + if (q->mm->data_vm + vma_pages(vma) > >> + rlimit(RLIMIT_DATA) >> PAGE_SHIFT) { >> + ret = -ENOMEM; >> + goto out_with_lock; >> + } >> + >> + if (q->qfrs[type]) { >> + ret = -EBUSY; >> + goto out_with_lock; >> + } >> + >> + switch (type) { >> + case UACCE_QFRT_MMIO: >> + flags = UACCE_QFRF_SELFMT; >> + break; >> + >> + case UACCE_QFRT_SS: >> + if (atomic_read(&uacce->state) != UACCE_ST_STARTED) { >> + ret = -EINVAL; >> + goto out_with_lock; >> + } >> + >> + flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP; >> + >> + break; >> + >> + case UACCE_QFRT_DKO: >> + flags = UACCE_QFRF_MAP | UACCE_QFRF_KMAP; >> + >> + break; >> + >> + case UACCE_QFRT_DUS: >> + if (uacce->flags & UACCE_DEV_PASID) { >> + flags = UACCE_QFRF_SELFMT; >> + break; >> + } >> + >> + flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP; >> + break; >> + >> + default: >> + WARN_ON(&uacce->dev); >> + break; >> + } >> + >> + qfr = uacce_create_region(q, vma, type, flags); >> + if (IS_ERR(qfr)) { >> + ret = PTR_ERR(qfr); >> + goto out_with_lock; >> + } >> + q->qfrs[type] = qfr; >> + >> + if (type == UACCE_QFRT_SS) { >> + INIT_LIST_HEAD(&qfr->qs); >> + list_add(&q->list, &q->qfrs[type]->qs); >> + } >> + >> + mutex_unlock(&uacce_mutex); >> + >> + if (qfr->pages) >> + q->mm->data_vm += qfr->nr_pages; >> + >> + return 0; >> + >> +out_with_lock: >> + mutex_unlock(&uacce_mutex); >> + return ret; >> +} >> + >> +static __poll_t uacce_fops_poll(struct file *file, poll_table *wait) >> +{ >> + struct uacce_queue *q = file->private_data; >> + struct uacce_device *uacce = q->uacce; >> + >> + poll_wait(file, &q->wait, wait); >> + if (uacce->ops->is_q_updated && uacce->ops->is_q_updated(q)) >> + return EPOLLIN | EPOLLRDNORM; >> + >> + return 0; >> +} >> + >> +static const struct file_operations uacce_fops = { >> + .owner = THIS_MODULE, >> + .open = uacce_fops_open, >> + .release = uacce_fops_release, >> + .unlocked_ioctl = uacce_fops_unl_ioctl, >> +#ifdef CONFIG_COMPAT >> + .compat_ioctl = uacce_fops_compat_ioctl, >> +#endif >> + .mmap = uacce_fops_mmap, >> + .poll = uacce_fops_poll, >> +}; >> + >> +#define UACCE_FROM_CDEV_ATTR(dev) container_of(dev, struct uacce_device, dev) >> + >> +static ssize_t id_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); > FROM_CDEV? But that's not a cdev. I'm confused. Pick a better name > for your macro please. Will use to_uacce_device instead. #define to_uacce_device(dev) container_of(dev, struct uacce_device, dev) > >> + >> + return sprintf(buf, "%d\n", uacce->dev_id); >> +} >> + >> +static ssize_t api_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + >> + return sprintf(buf, "%s\n", uacce->api_ver); >> +} >> + >> +static ssize_t numa_distance_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + int distance; >> + >> + distance = node_distance(smp_processor_id(), uacce->pdev->numa_node); >> + >> + return sprintf(buf, "%d\n", abs(distance)); >> +} >> + >> +static ssize_t node_id_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + int node_id; >> + >> + node_id = dev_to_node(uacce->pdev); >> + >> + return sprintf(buf, "%d\n", node_id); >> +} >> + >> +static ssize_t flags_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + >> + return sprintf(buf, "%d\n", uacce->flags); >> +} >> + >> +static ssize_t available_instances_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + int val = 0; >> + >> + if (uacce->ops->get_available_instances) >> + val = uacce->ops->get_available_instances(uacce); >> + >> + return sprintf(buf, "%d\n", val); >> +} >> + >> +static ssize_t algorithms_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + >> + return sprintf(buf, "%s", uacce->algs); >> +} >> + >> +static ssize_t qfrs_offset_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + int i, ret; >> + unsigned long offset; >> + >> + for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) { >> + offset = uacce->qf_pg_start[i]; >> + if (offset != UACCE_QFR_NA) >> + offset = offset << PAGE_SHIFT; >> + if (i == UACCE_QFRT_SS) >> + break; >> + ret += sprintf(buf + ret, "%lu\t", offset); >> + } >> + ret += sprintf(buf + ret, "%lu\n", offset); >> + >> + return ret; >> +} >> + >> +static DEVICE_ATTR_RO(id); >> +static DEVICE_ATTR_RO(api); >> +static DEVICE_ATTR_RO(numa_distance); >> +static DEVICE_ATTR_RO(node_id); >> +static DEVICE_ATTR_RO(flags); >> +static DEVICE_ATTR_RO(available_instances); >> +static DEVICE_ATTR_RO(algorithms); >> +static DEVICE_ATTR_RO(qfrs_offset); >> + >> +static struct attribute *uacce_dev_attrs[] = { >> + &dev_attr_id.attr, >> + &dev_attr_api.attr, >> + &dev_attr_node_id.attr, >> + &dev_attr_numa_distance.attr, >> + &dev_attr_flags.attr, >> + &dev_attr_available_instances.attr, >> + &dev_attr_algorithms.attr, >> + &dev_attr_qfrs_offset.attr, >> + NULL, >> +}; >> + >> +static const struct attribute_group uacce_dev_attr_group = { >> + .attrs = uacce_dev_attrs, >> +}; >> + >> +static const struct attribute_group *uacce_dev_attr_groups[] = { >> + &uacce_dev_attr_group, >> + NULL >> +}; > ATTRIBUTE_GROUPS()? Yes, good idea. > >> + >> +static void uacce_release(struct device *dev) >> +{ >> + struct uacce_device *uacce = UACCE_FROM_CDEV_ATTR(dev); >> + >> + idr_remove(&uacce_idr, uacce->dev_id); >> + kfree(uacce); >> +} >> + >> +static int uacce_create_chrdev(struct uacce_device *uacce) > You do more than just that here, you should rename the function. OK > >> +{ >> + int ret; >> + >> + ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL); >> + if (ret < 0) >> + return ret; >> + >> + uacce->cdev = cdev_alloc(); >> + uacce->cdev->ops = &uacce_fops; >> + uacce->dev_id = ret; >> + uacce->cdev->owner = THIS_MODULE; >> + device_initialize(&uacce->dev); >> + uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id); >> + uacce->dev.class = uacce_class; >> + uacce->dev.groups = uacce_dev_attr_groups; >> + uacce->dev.parent = uacce->pdev; >> + uacce->dev.release = uacce_release; >> + dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id); >> + ret = cdev_device_add(uacce->cdev, &uacce->dev); >> + if (ret) >> + goto err_with_idr; >> + >> + dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id); >> + >> + return 0; >> + >> +err_with_idr: >> + idr_remove(&uacce_idr, uacce->dev_id); >> + return ret; >> +} >> + >> +static void uacce_destroy_chrdev(struct uacce_device *uacce) >> +{ >> + cdev_device_del(uacce->cdev, &uacce->dev); >> + put_device(&uacce->dev); > Again, drop the _chrdev in the function name. OK. >> +} >> + >> +static int uacce_dev_match(struct device *dev, void *data) >> +{ >> + if (dev->parent == data) >> + return -EBUSY; >> + >> + return 0; > Do you really need this? This is preventing multi uacce_register from the same dev, which is not supported in non sva case now. Though we do not have such case now: same device have different algorithm and do multi-register. I think we can remove this, since we have no such case now. >> +} >> + >> +/* Borrowed from VFIO to fix msi translation */ >> +static bool uacce_iommu_has_sw_msi(struct iommu_group *group, >> + phys_addr_t *base) >> +{ >> + struct list_head group_resv_regions; >> + struct iommu_resv_region *region, *next; >> + bool ret = false; >> + >> + INIT_LIST_HEAD(&group_resv_regions); >> + iommu_get_group_resv_regions(group, &group_resv_regions); >> + list_for_each_entry(region, &group_resv_regions, list) { >> + pr_debug("uacce: find a resv region (%d) on %llx\n", >> + region->type, region->start); >> + >> + /* >> + * The presence of any 'real' MSI regions should take >> + * precedence over the software-managed one if the >> + * IOMMU driver happens to advertise both types. >> + */ >> + if (region->type == IOMMU_RESV_MSI) { >> + ret = false; >> + break; >> + } >> + >> + if (region->type == IOMMU_RESV_SW_MSI) { >> + *base = region->start; >> + ret = true; >> + } >> + } >> + list_for_each_entry_safe(region, next, &group_resv_regions, list) >> + kfree(region); >> + >> + return ret; >> +} >> + >> +static int uacce_set_iommu_domain(struct uacce_device *uacce) >> +{ >> + struct iommu_domain *domain; >> + struct iommu_group *group; >> + struct device *dev = uacce->pdev; >> + bool resv_msi; >> + phys_addr_t resv_msi_base = 0; >> + int ret; >> + >> + if (uacce->flags & UACCE_DEV_PASID) >> + return 0; >> + >> + /* >> + * We don't support multiple register for the same dev if no pasid >> + */ >> + ret = class_for_each_device(uacce_class, NULL, uacce->pdev, >> + uacce_dev_match); >> + if (ret) >> + return ret; >> + >> + /* allocate and attach a unmanged domain */ >> + domain = iommu_domain_alloc(uacce->pdev->bus); >> + if (!domain) { >> + dev_err(&uacce->dev, "cannot get domain for iommu\n"); >> + return -ENODEV; >> + } >> + >> + ret = iommu_attach_device(domain, uacce->pdev); >> + if (ret) >> + goto err_with_domain; >> + >> + if (iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) >> + uacce->prot |= IOMMU_CACHE; >> + >> + group = iommu_group_get(dev); >> + if (!group) { >> + ret = -EINVAL; >> + goto err_with_domain; >> + } >> + >> + resv_msi = uacce_iommu_has_sw_msi(group, &resv_msi_base); >> + iommu_group_put(group); >> + >> + if (resv_msi) { >> + if (!irq_domain_check_msi_remap() && >> + !iommu_capable(dev->bus, IOMMU_CAP_INTR_REMAP)) { >> + dev_warn(dev, "No interrupt remapping support!"); >> + ret = -EPERM; >> + goto err_with_domain; >> + } >> + >> + ret = iommu_get_msi_cookie(domain, resv_msi_base); >> + if (ret) >> + goto err_with_domain; >> + } >> + >> + return 0; >> + >> +err_with_domain: >> + iommu_domain_free(domain); >> + return ret; >> +} >> + >> +static void uacce_unset_iommu_domain(struct uacce_device *uacce) >> +{ >> + struct iommu_domain *domain; >> + >> + if (uacce->flags & UACCE_DEV_PASID) >> + return; >> + >> + domain = iommu_get_domain_for_dev(uacce->pdev); >> + if (!domain) { >> + dev_err(&uacce->dev, "bug: no domain attached to device\n"); >> + return; >> + } >> + >> + iommu_detach_device(domain, uacce->pdev); >> + iommu_domain_free(domain); >> +} >> + >> +/** >> + * uacce_register - register an accelerator >> + * @uacce: the accelerator structure >> + */ >> +struct uacce_device *uacce_register(struct device *parent, >> + struct uacce_interface *interface) >> +{ >> + int ret, i; >> + struct uacce_device *uacce; >> + unsigned int flags = interface->flags; >> + >> + /* if dev support fault-from-dev, it should support pasid */ >> + if ((flags & UACCE_DEV_FAULT_FROM_DEV) && !(flags & UACCE_DEV_PASID)) { >> + dev_warn(parent, "SVM/SAV device should support PASID\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> +#ifdef CONFIG_IOMMU_SVA >> + if (flags & UACCE_DEV_PASID) { >> + ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA); >> + if (ret) >> + flags &= ~(UACCE_DEV_FAULT_FROM_DEV | >> + UACCE_DEV_PASID); >> + } >> +#endif >> + uacce = kzalloc(sizeof(struct uacce_device), GFP_KERNEL); >> + if (!uacce) >> + return ERR_PTR(-ENOMEM); >> + >> + uacce->pdev = parent; >> + uacce->flags = flags; >> + uacce->ops = interface->ops; >> + uacce->drv_name = interface->name; >> + >> + for (i = 0; i < UACCE_QFRT_MAX; i++) >> + uacce->qf_pg_start[i] = UACCE_QFR_NA; >> + >> + ret = uacce_set_iommu_domain(uacce); >> + if (ret) >> + goto err_free; >> + >> + mutex_lock(&uacce_mutex); >> + >> + ret = uacce_create_chrdev(uacce); >> + if (ret) >> + goto err_with_lock; >> + >> + atomic_set(&uacce->state, UACCE_ST_INIT); >> + INIT_LIST_HEAD(&uacce->qs); >> + mutex_init(&uacce->q_lock); >> + >> + mutex_unlock(&uacce_mutex); >> + >> + return uacce; >> + >> +err_with_lock: >> + mutex_unlock(&uacce_mutex); >> +err_free: >> + kfree(uacce); >> + return ERR_PTR(ret); >> +} >> +EXPORT_SYMBOL_GPL(uacce_register); >> + >> +/** >> + * uacce_unregister - unregisters a uacce >> + * @uacce: the accelerator to unregister >> + * >> + * Unregister an accelerator that wat previously successully registered with >> + * uacce_register(). >> + */ >> +void uacce_unregister(struct uacce_device *uacce) >> +{ >> + mutex_lock(&uacce_mutex); >> + >> +#ifdef CONFIG_IOMMU_SVA >> + if (uacce->flags & UACCE_DEV_PASID) >> + iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA); >> +#endif >> + uacce_unset_iommu_domain(uacce); >> + uacce_destroy_chrdev(uacce); >> + >> + mutex_unlock(&uacce_mutex); >> +} >> +EXPORT_SYMBOL_GPL(uacce_unregister); >> + >> +static int __init uacce_init(void) >> +{ >> + int ret; >> + >> + uacce_class = class_create(THIS_MODULE, UACCE_NAME); >> + if (IS_ERR(uacce_class)) { >> + ret = PTR_ERR(uacce_class); >> + goto err; >> + } >> + >> + ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, UACCE_NAME); >> + if (ret) >> + goto err_with_class; >> + >> + pr_info("uacce init with major number:%d\n", MAJOR(uacce_devt)); > When your code works properly, it is totally silent. Do not spam the > kernel log for no good reason. Stuff like this is debugging only, if > that as you can see it in the proper /proc/ file. OK, understand. > >> + >> + return 0; >> + >> +err_with_class: >> + class_destroy(uacce_class); >> +err: >> + return ret; >> +} >> + >> +static __exit void uacce_exit(void) >> +{ >> + unregister_chrdev_region(uacce_devt, MINORMASK); >> + class_destroy(uacce_class); >> + idr_destroy(&uacce_idr); >> +} >> + >> +subsys_initcall(uacce_init); >> +module_exit(uacce_exit); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_AUTHOR("Hisilicon Tech. Co., Ltd."); >> +MODULE_DESCRIPTION("Accelerator interface for Userland applications"); >> diff --git a/include/linux/uacce.h b/include/linux/uacce.h >> new file mode 100644 >> index 0000000..b23a28b >> --- /dev/null >> +++ b/include/linux/uacce.h >> @@ -0,0 +1,172 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +#ifndef __UACCE_H >> +#define __UACCE_H >> + >> +#include <linux/cdev.h> >> +#include <linux/device.h> >> +#include <linux/fs.h> >> +#include <linux/list.h> >> +#include <linux/iommu.h> >> +#include <uapi/misc/uacce.h> > Why are all of these needed in this .h file? Are you sure? Sorry, only two of them are required. > >> + >> +#define UACCE_NAME "uacce" >> + >> +struct uacce_queue; >> +struct uacce_device; >> + >> +/* uacce mode of the driver */ >> +#define UACCE_MODE_NOUACCE 0 /* don't use uacce */ >> +#define UACCE_MODE_UACCE 1 /* use uacce exclusively */ >> + >> +/* uacce queue file flag, requires different operation */ >> +#define UACCE_QFRF_MAP BIT(0) /* map to current queue */ >> +#define UACCE_QFRF_MMAP BIT(1) /* map to user space */ >> +#define UACCE_QFRF_KMAP BIT(2) /* map to kernel space */ >> +#define UACCE_QFRF_DMA BIT(3) /* use dma api for the region */ >> +#define UACCE_QFRF_SELFMT BIT(4) /* self maintained qfr */ >> + >> +/** >> + * struct uacce_qfile_region - structure of queue file region >> + * @type: type of the qfr >> + * @iova: iova share between user and device space >> + * @pages: pages pointer of the qfr memory >> + * @nr_pages: page numbers of the qfr memory >> + * @prot: qfr protection flag >> + * @flags: flags of qfr >> + * @qs: list sharing the same region, for ss region >> + * @kaddr: kernel addr of the qfr >> + * @dma: dma address, if created by dma api >> + */ >> +struct uacce_qfile_region { >> + enum uacce_qfrt type; >> + unsigned long iova; >> + struct page **pages; >> + int nr_pages; >> + int prot; >> + unsigned int flags; >> + struct list_head qs; >> + void *kaddr; >> + dma_addr_t dma; >> +}; >> + >> +/** >> + * struct uacce_ops - uacce device operations >> + * @get_available_instances: get available instances left of the device >> + * @get_queue: get a queue from the device >> + * @put_queue: free a queue to the device >> + * @start_queue: make the queue start work after get_queue >> + * @stop_queue: make the queue stop work before put_queue >> + * @is_q_updated: check whether the task is finished >> + * @mask_notify: mask the task irq of queue >> + * @mmap: mmap addresses of queue to user space >> + * @reset: reset the uacce device >> + * @reset_queue: reset the queue >> + * @ioctl: ioctl for user space users of the queue >> + */ >> +struct uacce_ops { >> + int (*get_available_instances)(struct uacce_device *uacce); >> + int (*get_queue)(struct uacce_device *uacce, unsigned long arg, >> + struct uacce_queue **q); >> + void (*put_queue)(struct uacce_queue *q); >> + int (*start_queue)(struct uacce_queue *q); >> + void (*stop_queue)(struct uacce_queue *q); >> + int (*is_q_updated)(struct uacce_queue *q); >> + void (*mask_notify)(struct uacce_queue *q, int event_mask); >> + int (*mmap)(struct uacce_queue *q, struct vm_area_struct *vma, >> + struct uacce_qfile_region *qfr); >> + int (*reset)(struct uacce_device *uacce); >> + int (*reset_queue)(struct uacce_queue *q); >> + long (*ioctl)(struct uacce_queue *q, unsigned int cmd, >> + unsigned long arg); >> +}; >> + >> +/** >> + * struct uacce_interface >> + * @name: the uacce device name. Will show up in sysfs >> + * @flags: uacce device attributes >> + * @ops: pointer to the struct uacce_ops >> + * >> + * This structure is used for the uacce_register() >> + */ >> +struct uacce_interface { >> + char name[32]; >> + unsigned int flags; >> + struct uacce_ops *ops; >> +}; >> + >> +/** >> + * struct uacce_queue >> + * @uacce: pointer to uacce >> + * @priv: private pointer >> + * @wait: wait queue head >> + * @pasid: pasid of the queue >> + * @handle: iommu_sva handle return from iommu_sva_bind_device >> + * @list: share list for qfr->qs >> + * @mm: current->mm >> + * @qfrs: pointer of qfr regions >> + * @q_dev: list for uacce->qs >> + */ >> +struct uacce_queue { >> + struct uacce_device *uacce; >> + void *priv; >> + wait_queue_head_t wait; >> + int pasid; >> + struct iommu_sva *handle; >> + struct list_head list; >> + struct mm_struct *mm; >> + struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX]; >> + struct list_head q_dev; >> +}; >> + >> +/* uacce state */ >> +enum { >> + UACCE_ST_INIT = 0, >> + UACCE_ST_OPENED, >> + UACCE_ST_STARTED, >> + UACCE_ST_RST, >> +}; >> + >> +/** >> + * struct uacce_device >> + * @drv_name: the uacce driver name. Will show up in sysfs >> + * @algs: supported algorithms >> + * @api_ver: api version >> + * @flags: uacce attributes >> + * @qf_pg_start: page start of the queue file regions >> + * @ops: pointer to the struct uacce_ops >> + * @pdev: pointer to the parent device >> + * @is_vf: whether virtual function >> + * @dev_id: id of the uacce device >> + * @cdev: cdev of the uacce >> + * @dev: dev of the uacce >> + * @priv: private pointer of the uacce >> + * @state: uacce state >> + * @prot: uacce protection flag >> + * @q_lock: uacce mutex lock for queue >> + * @qs: uacce list head for share >> + */ >> +struct uacce_device { >> + const char *drv_name; > A driver is bound to a device, and you can get the name there. No need > for it to be here. > >> + const char *algs; >> + const char *api_ver; >> + unsigned int flags; > u64? > >> + unsigned long qf_pg_start[UACCE_QFRT_MAX]; >> + struct uacce_ops *ops; >> + struct device *pdev; >> + bool is_vf; >> + u32 dev_id; >> + struct cdev *cdev; >> + struct device dev; >> + void *priv; >> + atomic_t state; >> + int prot; > u32? OK. > > >> + struct mutex q_lock; >> + struct list_head qs; > What's wrong with the list in the device itself? Double checked, will remove this. > >> +}; >> + >> +struct uacce_device *uacce_register(struct device *parent, >> + struct uacce_interface *interface); >> +void uacce_unregister(struct uacce_device *uacce); >> +void uacce_wake_up(struct uacce_queue *q); >> + >> +#endif >> diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h >> new file mode 100644 >> index 0000000..685b4a1 >> --- /dev/null >> +++ b/include/uapi/misc/uacce.h >> @@ -0,0 +1,39 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +#ifndef _UAPIUUACCE_H >> +#define _UAPIUUACCE_H >> + >> +#include <linux/types.h> >> +#include <linux/ioctl.h> >> + >> +#define UACCE_CMD_SHARE_SVAS _IO('W', 0) >> +#define UACCE_CMD_START _IO('W', 1) >> + >> +/** >> + * UACCE Device Attributes: >> + * >> + * SHARE_DOMAIN: no PASID, can shae sva only for one process and the kernel >> + * PASID: the device has IOMMU which support PASID setting >> + * can do share sva, mapped to dev per process >> + * FAULT_FROM_DEV: the device has IOMMU which can do page fault request >> + * no need for share sva, should be used with PASID >> + * SVA: full function device >> + */ >> + >> +enum { >> + UACCE_DEV_SHARE_DOMAIN = 0x0, >> + UACCE_DEV_PASID = 0x1, >> + UACCE_DEV_FAULT_FROM_DEV = 0x2, >> + UACCE_DEV_SVA = UACCE_DEV_PASID | UACCE_DEV_FAULT_FROM_DEV, >> +}; >> + >> +#define UACCE_QFR_NA ((unsigned long)-1) >> + >> +enum uacce_qfrt { >> + UACCE_QFRT_MMIO = 0, /* device mmio region */ >> + UACCE_QFRT_DKO = 1, /* device kernel-only */ >> + UACCE_QFRT_DUS = 2, /* device user share */ >> + UACCE_QFRT_SS = 3, /* static share memory */ >> + UACCE_QFRT_MAX, > You should set this value too, and probably to a big number, otherwise > you just created an api that can never add a value of '4' to its list. > > Why is this UACCE_QFRT_MAX even needed? > Good idea, will use UACCE_QFRT_MAX = 16 instead. UACCE_QFRT_MAX is required in struct uacce_device and uacce_queue. Like struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX] to record different region.
Thanks
| |