Re: [PATCH v12 2/4] uacce: add uacce driver
On Mon, Feb 24, 2020 at 10:22:02AM -0800, Raj, Ashok wrote:
> Hi Kenneth,
>
> sorry for waking up late on this patchset.
>
>
> On Wed, Jan 15, 2020 at 10:12:46PM +0800, Zhangfei Gao wrote:
> [... trimmed]
>
> > +
> > +static int uacce_fops_open(struct inode *inode, struct file *filep)
> > +{
> > + struct uacce_mm *uacce_mm = NULL;
> > + struct uacce_device *uacce;
> > + struct uacce_queue *q;
> > + int ret = 0;
> > +
> > + uacce = xa_load(_xa, iminor(inode));
> > + if (!uacce)
> > + return -ENODEV;
> > +
> > + q = kzalloc(sizeof(struct uacce_queue), GFP_KERNEL);
> > + if (!q)
> > + return -ENOMEM;
> > +
> > + mutex_lock(>mm_lock);
> > + uacce_mm = uacce_mm_get(uacce, q, current->mm);
>
> I think having this at open time is a bit unnatural. Since when a process
> does fork, we do not inherit the PASID. Although it inherits the fd
> but cannot use the mmaped address in the child.
Both the queue and the PASID are tied to a single address space. When it
disappears, the queue is stopped (zombie state) and the PASID is freed.
The fd is not usable nor recoverable at this point, it's just waiting to
be released.
> If you move this to the mmap time, its more natural. The child could
> do a mmap() get a new PASID + mmio space to work with the hardware.
I like the idea, as it ties the lifetime of the bond to that of the queue
mapping, but I have two small concerns:
* It adds a lot of side-effect to mmap(). In addition to mapping the MMIO
region it would now create both the bond and the queue. For userspace,
figuring out why the mmap() fails would be more difficult.
* It forces uacce drivers to implement an mmap() interface, and have MMIO
regions to share. I suspect it's going to be the norm but at the moment
it's not mandatory, drivers could just implement ioctls ops.
I guess the main benefit would be reusing an fd after the original address
space dies, but is it a use-case?
I'd rather go one step further in the other direction, declare that an fd
is a queue and is exclusive to an address space, by preventing any
operation (ioctl and mmap) from an mm other than the one that opened the
fd. It's not natural but it'd keep the kernel driver simple as we wouldn't
have to reconfigure the queue during the lifetime of the fd.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
Hi, Raj
On 2020/2/25 上午2:22, Raj, Ashok wrote:
Hi Kenneth,
sorry for waking up late on this patchset.
+
+static int uacce_fops_open(struct inode *inode, struct file *filep)
+{
+ struct uacce_mm *uacce_mm = NULL;
+ struct uacce_device *uacce;
+ struct uacce_queue *q;
+ int ret = 0;
+
+ uacce = xa_load(_xa, iminor(inode));
+ if (!uacce)
+ return -ENODEV;
+
+ q = kzalloc(sizeof(struct uacce_queue), GFP_KERNEL);
+ if (!q)
+ return -ENOMEM;
+
+ mutex_lock(>mm_lock);
+ uacce_mm = uacce_mm_get(uacce, q, current->mm);
I think having this at open time is a bit unnatural. Since when a process
does fork, we do not inherit the PASID. Although it inherits the fd
but cannot use the mmaped address in the child.
If you move this to the mmap time, its more natural. The child could
do a mmap() get a new PASID + mmio space to work with the hardware.
Thanks for the suggestion.
We will consider fork in the next step, may need some time.
Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
Hi Kenneth,
sorry for waking up late on this patchset.
On Wed, Jan 15, 2020 at 10:12:46PM +0800, Zhangfei Gao wrote:
[... trimmed]
> +
> +static int uacce_fops_open(struct inode *inode, struct file *filep)
> +{
> + struct uacce_mm *uacce_mm = NULL;
> + struct uacce_device *uacce;
> + struct uacce_queue *q;
> + int ret = 0;
> +
> + uacce = xa_load(_xa, iminor(inode));
> + if (!uacce)
> + return -ENODEV;
> +
> + q = kzalloc(sizeof(struct uacce_queue), GFP_KERNEL);
> + if (!q)
> + return -ENOMEM;
> +
> + mutex_lock(>mm_lock);
> + uacce_mm = uacce_mm_get(uacce, q, current->mm);
I think having this at open time is a bit unnatural. Since when a process
does fork, we do not inherit the PASID. Although it inherits the fd
but cannot use the mmaped address in the child.
If you move this to the mmap time, its more natural. The child could
do a mmap() get a new PASID + mmio space to work with the hardware.
> + mutex_unlock(>mm_lock);
> + if (!uacce_mm) {
> + ret = -ENOMEM;
> + goto out_with_mem;
> + }
> +
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On Thu, Feb 13, 2020 at 05:15:10PM +0800, Herbert Xu wrote: > On Mon, Feb 10, 2020 at 03:37:11PM -0800, Greg Kroah-Hartman wrote: > > > > Looks much saner now, thanks for all of the work on this: > > > > Reviewed-by: Greg Kroah-Hartman > > > > Or am I supposed to take this in my tree? If so, I can, but I need an > > ack for the crypto parts. > > I can take this series through the crypto tree if that's fine with > you. Please do, thanks! greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On 2020/2/13 下午5:15, Herbert Xu wrote: On Mon, Feb 10, 2020 at 03:37:11PM -0800, Greg Kroah-Hartman wrote: Looks much saner now, thanks for all of the work on this: Reviewed-by: Greg Kroah-Hartman Or am I supposed to take this in my tree? If so, I can, but I need an ack for the crypto parts. I can take this series through the crypto tree if that's fine with you. Thanks Herbert That's a good idea, otherwise there may be build issue if taken separately. By the way, the latest v13 is on v5.6-rc1 https://lkml.org/lkml/2020/2/11/54 Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On Mon, Feb 10, 2020 at 03:37:11PM -0800, Greg Kroah-Hartman wrote: > > Looks much saner now, thanks for all of the work on this: > > Reviewed-by: Greg Kroah-Hartman > > Or am I supposed to take this in my tree? If so, I can, but I need an > ack for the crypto parts. I can take this series through the crypto tree if that's fine with you. Thank, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On 2020/2/11 上午7:37, Greg Kroah-Hartman wrote: On Wed, Jan 15, 2020 at 10:12:46PM +0800, Zhangfei Gao wrote: From: Kenneth Lee 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 Reviewed-by: Jonathan Cameron Signed-off-by: Kenneth Lee Signed-off-by: Zaibo Xu Signed-off-by: Zhou Wang Signed-off-by: Jean-Philippe Brucker Signed-off-by: Zhangfei Gao Looks much saner now, thanks for all of the work on this: Reviewed-by: Greg Kroah-Hartman Or am I supposed to take this in my tree? If so, I can, but I need an ack for the crypto parts. That's Great, thanks Greg. For the convenience, I rebase the patchset on 5.6-rc1. Not sure is there any conflict to crypto tree. How about just pick the uacce part, patch 1 , 2. We can resend the crypto part to crypto tree. Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On Wed, Jan 15, 2020 at 10:12:46PM +0800, Zhangfei Gao wrote: > From: Kenneth Lee > > 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 > > Reviewed-by: Jonathan Cameron > Signed-off-by: Kenneth Lee > Signed-off-by: Zaibo Xu > Signed-off-by: Zhou Wang > Signed-off-by: Jean-Philippe Brucker > Signed-off-by: Zhangfei Gao Looks much saner now, thanks for all of the work on this: Reviewed-by: Greg Kroah-Hartman Or am I supposed to take this in my tree? If so, I can, but I need an ack for the crypto parts. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v12 2/4] uacce: add uacce driver
From: Kenneth Lee 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 Reviewed-by: Jonathan Cameron Signed-off-by: Kenneth Lee Signed-off-by: Zaibo Xu Signed-off-by: Zhou Wang Signed-off-by: Jean-Philippe Brucker Signed-off-by: Zhangfei Gao --- Documentation/ABI/testing/sysfs-driver-uacce | 39 ++ drivers/misc/Kconfig | 1 + drivers/misc/Makefile| 1 + drivers/misc/uacce/Kconfig | 13 + drivers/misc/uacce/Makefile | 2 + drivers/misc/uacce/uacce.c | 617 +++ include/linux/uacce.h| 161 +++ include/uapi/misc/uacce/uacce.h | 38 ++ 8 files changed, 872 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 diff --git a/Documentation/ABI/testing/sysfs-driver-uacce b/Documentation/ABI/testing/sysfs-driver-uacce new file mode 100644 index 000..ef4003a --- /dev/null +++ b/Documentation/ABI/testing/sysfs-driver-uacce @@ -0,0 +1,39 @@ +What: /sys/class/uacce//api +Date: Jan 2020 +KernelVersion: 5.6 +Contact:linux-accelerat...@lists.ozlabs.org +Description:Api of the device +Can be any string and up to userspace to parse. +Application use the api to match the correct driver + +What: /sys/class/uacce//flags +Date: Jan 2020 +KernelVersion: 5.6 +Contact:linux-accelerat...@lists.ozlabs.org +Description:Attributes of the device, see UACCE_DEV_xxx flag defined in uacce.h + +What: /sys/class/uacce//available_instances +Date: Jan 2020 +KernelVersion: 5.6 +Contact:linux-accelerat...@lists.ozlabs.org +Description:Available instances left of the device +Return -ENODEV if uacce_ops get_available_instances is not provided + +What: /sys/class/uacce//algorithms +Date: Jan 2020 +KernelVersion: 5.6 +Contact:linux-accelerat...@lists.ozlabs.org +Description:Algorithms supported by this accelerator, separated by new line. +Can be any string and up to userspace to parse. + +What: /sys/class/uacce//region_mmio_size +Date: Jan 2020 +KernelVersion: 5.6 +Contact:linux-accelerat...@lists.ozlabs.org +Description:Size (bytes) of mmio region queue file + +What: /sys/class/uacce//region_dus_size +Date: Jan 2020 +KernelVersion: 5.6 +Contact:linux-accelerat...@lists.ozlabs.org +Description:Size (bytes) of dus region queue file diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 7f0d48f..99e1514 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -480,4 +480,5 @@ source "drivers/misc/cxl/Kconfig" source "drivers/misc/ocxl/Kconfig" source "drivers/misc/cardreader/Kconfig" source "drivers/misc/habanalabs/Kconfig" +source "drivers/misc/uacce/Kconfig" endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index c1860d3..9abf292 100644 ---
