linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] tee: Add tee_shm_register_fd
@ 2022-08-11 13:56 Olivier Masse
  2022-08-11 13:56 ` [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Olivier Masse
  2022-08-12 14:01 ` [PATCH 0/1] tee: Add tee_shm_register_fd Sumit Garg
  0 siblings, 2 replies; 8+ messages in thread
From: Olivier Masse @ 2022-08-11 13:56 UTC (permalink / raw)
  To: jens.wiklander, sumit.garg, sumit.semwal, christian.koenig,
	op-tee, linaro-mm-sig, linux-kernel, linux-media, dri-devel
  Cc: clement.faure, olivier.masse

Add a new ioctl called TEE_IOC_SHM_REGISTER_FD to register a
shared memory from a dmabuf file descriptor.

Etienne Carriere (1):
  tee: new ioctl to a register tee_shm from a dmabuf file descriptor

 drivers/tee/tee_core.c   | 38 +++++++++++++++
 drivers/tee/tee_shm.c    | 99 +++++++++++++++++++++++++++++++++++++++-
 include/linux/tee_drv.h  | 11 +++++
 include/uapi/linux/tee.h | 29 ++++++++++++
 4 files changed, 175 insertions(+), 2 deletions(-)

-- 
2.25.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
  2022-08-11 13:56 [PATCH 0/1] tee: Add tee_shm_register_fd Olivier Masse
@ 2022-08-11 13:56 ` Olivier Masse
  2022-08-12  5:27   ` kernel test robot
                     ` (2 more replies)
  2022-08-12 14:01 ` [PATCH 0/1] tee: Add tee_shm_register_fd Sumit Garg
  1 sibling, 3 replies; 8+ messages in thread
From: Olivier Masse @ 2022-08-11 13:56 UTC (permalink / raw)
  To: jens.wiklander, sumit.garg, sumit.semwal, christian.koenig,
	op-tee, linaro-mm-sig, linux-kernel, linux-media, dri-devel
  Cc: clement.faure, olivier.masse

From: Etienne Carriere <etienne.carriere@linaro.org>

This change allows userland to create a tee_shm object that refers
to a dmabuf reference.

Userland provides a dmabuf file descriptor as buffer reference.
The created tee_shm object exported as a brand new dmabuf reference
used to provide a clean fd to userland. Userland shall closed this new
fd to release the tee_shm object resources. The initial dmabuf resources
are tracked independently through original dmabuf file descriptor.

Once the buffer is registered and until it is released, TEE driver
keeps a refcount on the registered dmabuf structure.

This change only support dmabuf references that relates to physically
contiguous memory buffers.

New tee_shm flag to identify tee_shm objects built from a registered
dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged with
TEE_SHM_EXT_DMA_BUF.

Co-Developed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
From: https://github.com/linaro-swg/linux.git
(cherry picked from commit 41e21e5c405530590dc2dd10b2a8dbe64589840f)
---
 drivers/tee/tee_core.c   | 38 +++++++++++++++
 drivers/tee/tee_shm.c    | 99 +++++++++++++++++++++++++++++++++++++++-
 include/linux/tee_drv.h  | 11 +++++
 include/uapi/linux/tee.h | 29 ++++++++++++
 4 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 8aa1a4836b92..7c45cbf85eb9 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -355,6 +355,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 	return ret;
 }
 
+static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
+				     struct tee_ioctl_shm_register_fd_data __user *udata)
+{
+	struct tee_ioctl_shm_register_fd_data data;
+	struct tee_shm *shm;
+	long ret;
+
+	if (copy_from_user(&data, udata, sizeof(data)))
+		return -EFAULT;
+
+	/* Currently no input flags are supported */
+	if (data.flags)
+		return -EINVAL;
+
+	shm = tee_shm_register_fd(ctx, data.fd);
+	if (IS_ERR(shm))
+		return -EINVAL;
+
+	data.id = shm->id;
+	data.flags = shm->flags;
+	data.size = shm->size;
+
+	if (copy_to_user(udata, &data, sizeof(data)))
+		ret = -EFAULT;
+	else
+		ret = tee_shm_get_fd(shm);
+
+	/*
+	 * When user space closes the file descriptor the shared memory
+	 * should be freed or if tee_shm_get_fd() failed then it will
+	 * be freed immediately.
+	 */
+	tee_shm_put(shm);
+	return ret;
+}
+
 static int params_from_user(struct tee_context *ctx, struct tee_param *params,
 			    size_t num_params,
 			    struct tee_ioctl_param __user *uparams)
@@ -829,6 +865,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return tee_ioctl_shm_alloc(ctx, uarg);
 	case TEE_IOC_SHM_REGISTER:
 		return tee_ioctl_shm_register(ctx, uarg);
+	case TEE_IOC_SHM_REGISTER_FD:
+		return tee_ioctl_shm_register_fd(ctx, uarg);
 	case TEE_IOC_OPEN_SESSION:
 		return tee_ioctl_open_session(ctx, uarg);
 	case TEE_IOC_INVOKE:
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 836872467dc6..55a3fbbb022e 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -4,6 +4,7 @@
  */
 #include <linux/anon_inodes.h>
 #include <linux/device.h>
+#include <linux/dma-buf.h>
 #include <linux/idr.h>
 #include <linux/mm.h>
 #include <linux/sched.h>
@@ -12,6 +13,14 @@
 #include <linux/uio.h>
 #include "tee_private.h"
 
+/* extra references appended to shm object for registered shared memory */
+struct tee_shm_dmabuf_ref {
+     struct tee_shm shm;
+     struct dma_buf *dmabuf;
+     struct dma_buf_attachment *attach;
+     struct sg_table *sgt;
+};
+
 static void shm_put_kernel_pages(struct page **pages, size_t page_count)
 {
 	size_t n;
@@ -71,7 +80,16 @@ static void release_registered_pages(struct tee_shm *shm)
 
 static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
 {
-	if (shm->flags & TEE_SHM_POOL) {
+	if (shm->flags & TEE_SHM_EXT_DMA_BUF) {
+		struct tee_shm_dmabuf_ref *ref;
+
+		ref = container_of(shm, struct tee_shm_dmabuf_ref, shm);
+		dma_buf_unmap_attachment(ref->attach, ref->sgt,
+					 DMA_BIDIRECTIONAL);
+
+		dma_buf_detach(ref->dmabuf, ref->attach);
+		dma_buf_put(ref->dmabuf);
+	} else if (shm->flags & TEE_SHM_POOL) {
 		teedev->pool->ops->free(teedev->pool, shm);
 	} else if (shm->flags & TEE_SHM_DYNAMIC) {
 		int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
@@ -195,7 +213,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
  * tee_client_invoke_func(). The memory allocated is later freed with a
  * call to tee_shm_free().
  *
- * @returns a pointer to 'struct tee_shm'
+ * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
  */
 struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
 {
@@ -229,6 +247,83 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
 }
 EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
 
+struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd)
+{
+	struct tee_shm_dmabuf_ref *ref;
+	int rc;
+
+	if (!tee_device_get(ctx->teedev))
+		return ERR_PTR(-EINVAL);
+
+	teedev_ctx_get(ctx);
+
+	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
+	if (!ref) {
+		rc = -ENOMEM;
+		goto err_put_tee;
+	}
+
+	refcount_set(&ref->shm.refcount, 1);
+	ref->shm.ctx = ctx;
+	ref->shm.id = -1;
+
+	ref->dmabuf = dma_buf_get(fd);
+	if (IS_ERR(ref->dmabuf)) {
+		rc = PTR_ERR(ref->dmabuf);
+		goto err_put_dmabuf;
+	}
+
+	ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
+	if (IS_ERR(ref->attach)) {
+		rc = PTR_ERR(ref->attach);
+		goto err_detach;
+	}
+
+	ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL);
+	if (IS_ERR(ref->sgt)) {
+		rc = PTR_ERR(ref->sgt);
+		goto err_unmap_attachement;
+	}
+
+	if (sg_nents(ref->sgt->sgl) != 1) {
+		rc = PTR_ERR(ref->sgt->sgl);
+		goto err_unmap_attachement;
+	}
+
+	ref->shm.paddr = sg_dma_address(ref->sgt->sgl);
+	ref->shm.size = sg_dma_len(ref->sgt->sgl);
+	ref->shm.flags = TEE_SHM_EXT_DMA_BUF;
+
+	mutex_lock(&ref->shm.ctx->teedev->mutex);
+	ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm,
+				1, 0, GFP_KERNEL);
+	mutex_unlock(&ref->shm.ctx->teedev->mutex);
+	if (ref->shm.id < 0) {
+		rc = ref->shm.id;
+		goto err_idr_remove;
+	}
+
+	return &ref->shm;
+
+err_idr_remove:
+	mutex_lock(&ctx->teedev->mutex);
+	idr_remove(&ctx->teedev->idr, ref->shm.id);
+	mutex_unlock(&ctx->teedev->mutex);
+err_unmap_attachement:
+	dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
+err_detach:
+	dma_buf_detach(ref->dmabuf, ref->attach);
+err_put_dmabuf:
+	dma_buf_put(ref->dmabuf);
+	kfree(ref);
+err_put_tee:
+	teedev_ctx_put(ctx);
+	tee_device_put(ctx->teedev);
+
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(tee_shm_register_fd);
+
 static struct tee_shm *
 register_shm_helper(struct tee_context *ctx, unsigned long addr,
 		    size_t length, u32 flags, int id)
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 911cad324acc..40ddd5376c2d 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -25,6 +25,7 @@
 #define TEE_SHM_USER_MAPPED	BIT(1)  /* Memory mapped in user space */
 #define TEE_SHM_POOL		BIT(2)  /* Memory allocated from pool */
 #define TEE_SHM_PRIV		BIT(3)  /* Memory private to TEE driver */
+#define TEE_SHM_EXT_DMA_BUF     BIT(4)  /* Memory with dma-buf handle */
 
 struct device;
 struct tee_device;
@@ -276,6 +277,16 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
 struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
 					    void *addr, size_t length);
 
+/**
+ * tee_shm_register_fd() - Register shared memory from file descriptor
+ *
+ * @ctx:	Context that allocates the shared memory
+ * @fd:		Shared memory file descriptor reference
+ *
+ * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
+ */
+struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd);
+
 /**
  * tee_shm_is_dynamic() - Check if shared memory object is of the dynamic kind
  * @shm:	Shared memory handle
diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
index 25a6c534beb1..6f84060ad005 100644
--- a/include/uapi/linux/tee.h
+++ b/include/uapi/linux/tee.h
@@ -121,6 +121,35 @@ struct tee_ioctl_shm_alloc_data {
 #define TEE_IOC_SHM_ALLOC	_IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
 				     struct tee_ioctl_shm_alloc_data)
 
+/**
+ * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
+ * @fd:		[in] File descriptor identifying the shared memory
+ * @size:	[out] Size of shared memory to allocate
+ * @flags:	[in] Flags to/from allocation.
+ * @id:		[out] Identifier of the shared memory
+ *
+ * The flags field should currently be zero as input. Updated by the call
+ * with actual flags as defined by TEE_IOCTL_SHM_* above.
+ * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below.
+ */
+struct tee_ioctl_shm_register_fd_data {
+	__s64 fd;
+	__u64 size;
+	__u32 flags;
+	__s32 id;
+} __aligned(8);
+
+/**
+ * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor
+ *
+ * Returns a file descriptor on success or < 0 on failure
+ *
+ * The returned file descriptor refers to the shared memory object in kernel
+ * land. The shared memory is freed when the descriptor is closed.
+ */
+#define TEE_IOC_SHM_REGISTER_FD	_IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \
+				     struct tee_ioctl_shm_register_fd_data)
+
 /**
  * struct tee_ioctl_buf_data - Variable sized buffer
  * @buf_ptr:	[in] A __user pointer to a buffer
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
  2022-08-11 13:56 ` [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Olivier Masse
@ 2022-08-12  5:27   ` kernel test robot
  2022-08-12 10:18   ` Christian König
  2022-08-12 14:06   ` Sumit Garg
  2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2022-08-12  5:27 UTC (permalink / raw)
  To: Olivier Masse, jens.wiklander, sumit.garg, sumit.semwal,
	christian.koenig, op-tee, linaro-mm-sig, linux-kernel,
	linux-media, dri-devel
  Cc: kbuild-all, clement.faure, olivier.masse

Hi Olivier,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip linus/master v5.19 next-20220811]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Olivier-Masse/tee-Add-tee_shm_register_fd/20220811-220012
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220812/202208121326.FWVAzlch-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/2e8827973f200fdfe64366bec5a57686086f4672
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Olivier-Masse/tee-Add-tee_shm_register_fd/20220811-220012
        git checkout 2e8827973f200fdfe64366bec5a57686086f4672
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:
>> ./usr/include/linux/tee.h:136:13: error: expected declaration specifiers or '...' before numeric constant
     136 | } __aligned(8);
         |             ^

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
  2022-08-11 13:56 ` [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Olivier Masse
  2022-08-12  5:27   ` kernel test robot
@ 2022-08-12 10:18   ` Christian König
  2022-08-12 13:24     ` [EXT] " Olivier Masse
  2022-08-12 14:06   ` Sumit Garg
  2 siblings, 1 reply; 8+ messages in thread
From: Christian König @ 2022-08-12 10:18 UTC (permalink / raw)
  To: Olivier Masse, jens.wiklander, sumit.garg, sumit.semwal, op-tee,
	linaro-mm-sig, linux-kernel, linux-media, dri-devel
  Cc: clement.faure

Hi Etienne,

at least I don't see anything obvious broken.

Just two comments:

1. Dmitry is working on a change which renames some functions and makes 
it mandatory to call them with the dma_resv lock held.

Depending on how you want to upstream this change you will certainly run 
into conflicts with that.

2. Would it be possible to do this dynamically? In other words does the 
tee driver has a concept of buffers moving around?

Am 11.08.22 um 15:56 schrieb Olivier Masse:
> From: Etienne Carriere <etienne.carriere@linaro.org>
>
> This change allows userland to create a tee_shm object that refers
> to a dmabuf reference.
>
> Userland provides a dmabuf file descriptor as buffer reference.
> The created tee_shm object exported as a brand new dmabuf reference
> used to provide a clean fd to userland. Userland shall closed this new
> fd to release the tee_shm object resources. The initial dmabuf resources
> are tracked independently through original dmabuf file descriptor.
>
> Once the buffer is registered and until it is released, TEE driver
> keeps a refcount on the registered dmabuf structure.
>
> This change only support dmabuf references that relates to physically
> contiguous memory buffers.

That sounds like a pretty hard restriction, but I obviously don't see 
how to avoid it either.

Regards,
Christian.

>
> New tee_shm flag to identify tee_shm objects built from a registered
> dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged with
> TEE_SHM_EXT_DMA_BUF.
>
> Co-Developed-by: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> From: https://github.com/linaro-swg/linux.git
> (cherry picked from commit 41e21e5c405530590dc2dd10b2a8dbe64589840f)
> ---
>   drivers/tee/tee_core.c   | 38 +++++++++++++++
>   drivers/tee/tee_shm.c    | 99 +++++++++++++++++++++++++++++++++++++++-
>   include/linux/tee_drv.h  | 11 +++++
>   include/uapi/linux/tee.h | 29 ++++++++++++
>   4 files changed, 175 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 8aa1a4836b92..7c45cbf85eb9 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -355,6 +355,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>   	return ret;
>   }
>   
> +static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
> +				     struct tee_ioctl_shm_register_fd_data __user *udata)
> +{
> +	struct tee_ioctl_shm_register_fd_data data;
> +	struct tee_shm *shm;
> +	long ret;
> +
> +	if (copy_from_user(&data, udata, sizeof(data)))
> +		return -EFAULT;
> +
> +	/* Currently no input flags are supported */
> +	if (data.flags)
> +		return -EINVAL;
> +
> +	shm = tee_shm_register_fd(ctx, data.fd);
> +	if (IS_ERR(shm))
> +		return -EINVAL;
> +
> +	data.id = shm->id;
> +	data.flags = shm->flags;
> +	data.size = shm->size;
> +
> +	if (copy_to_user(udata, &data, sizeof(data)))
> +		ret = -EFAULT;
> +	else
> +		ret = tee_shm_get_fd(shm);
> +
> +	/*
> +	 * When user space closes the file descriptor the shared memory
> +	 * should be freed or if tee_shm_get_fd() failed then it will
> +	 * be freed immediately.
> +	 */
> +	tee_shm_put(shm);
> +	return ret;
> +}
> +
>   static int params_from_user(struct tee_context *ctx, struct tee_param *params,
>   			    size_t num_params,
>   			    struct tee_ioctl_param __user *uparams)
> @@ -829,6 +865,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   		return tee_ioctl_shm_alloc(ctx, uarg);
>   	case TEE_IOC_SHM_REGISTER:
>   		return tee_ioctl_shm_register(ctx, uarg);
> +	case TEE_IOC_SHM_REGISTER_FD:
> +		return tee_ioctl_shm_register_fd(ctx, uarg);
>   	case TEE_IOC_OPEN_SESSION:
>   		return tee_ioctl_open_session(ctx, uarg);
>   	case TEE_IOC_INVOKE:
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 836872467dc6..55a3fbbb022e 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -4,6 +4,7 @@
>    */
>   #include <linux/anon_inodes.h>
>   #include <linux/device.h>
> +#include <linux/dma-buf.h>
>   #include <linux/idr.h>
>   #include <linux/mm.h>
>   #include <linux/sched.h>
> @@ -12,6 +13,14 @@
>   #include <linux/uio.h>
>   #include "tee_private.h"
>   
> +/* extra references appended to shm object for registered shared memory */
> +struct tee_shm_dmabuf_ref {
> +     struct tee_shm shm;
> +     struct dma_buf *dmabuf;
> +     struct dma_buf_attachment *attach;
> +     struct sg_table *sgt;
> +};
> +
>   static void shm_put_kernel_pages(struct page **pages, size_t page_count)
>   {
>   	size_t n;
> @@ -71,7 +80,16 @@ static void release_registered_pages(struct tee_shm *shm)
>   
>   static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
>   {
> -	if (shm->flags & TEE_SHM_POOL) {
> +	if (shm->flags & TEE_SHM_EXT_DMA_BUF) {
> +		struct tee_shm_dmabuf_ref *ref;
> +
> +		ref = container_of(shm, struct tee_shm_dmabuf_ref, shm);
> +		dma_buf_unmap_attachment(ref->attach, ref->sgt,
> +					 DMA_BIDIRECTIONAL);
> +
> +		dma_buf_detach(ref->dmabuf, ref->attach);
> +		dma_buf_put(ref->dmabuf);
> +	} else if (shm->flags & TEE_SHM_POOL) {
>   		teedev->pool->ops->free(teedev->pool, shm);
>   	} else if (shm->flags & TEE_SHM_DYNAMIC) {
>   		int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
> @@ -195,7 +213,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
>    * tee_client_invoke_func(). The memory allocated is later freed with a
>    * call to tee_shm_free().
>    *
> - * @returns a pointer to 'struct tee_shm'
> + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
>    */
>   struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
>   {
> @@ -229,6 +247,83 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
>   }
>   EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
>   
> +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd)
> +{
> +	struct tee_shm_dmabuf_ref *ref;
> +	int rc;
> +
> +	if (!tee_device_get(ctx->teedev))
> +		return ERR_PTR(-EINVAL);
> +
> +	teedev_ctx_get(ctx);
> +
> +	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> +	if (!ref) {
> +		rc = -ENOMEM;
> +		goto err_put_tee;
> +	}
> +
> +	refcount_set(&ref->shm.refcount, 1);
> +	ref->shm.ctx = ctx;
> +	ref->shm.id = -1;
> +
> +	ref->dmabuf = dma_buf_get(fd);
> +	if (IS_ERR(ref->dmabuf)) {
> +		rc = PTR_ERR(ref->dmabuf);
> +		goto err_put_dmabuf;
> +	}
> +
> +	ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
> +	if (IS_ERR(ref->attach)) {
> +		rc = PTR_ERR(ref->attach);
> +		goto err_detach;
> +	}
> +
> +	ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL);
> +	if (IS_ERR(ref->sgt)) {
> +		rc = PTR_ERR(ref->sgt);
> +		goto err_unmap_attachement;
> +	}
> +
> +	if (sg_nents(ref->sgt->sgl) != 1) {
> +		rc = PTR_ERR(ref->sgt->sgl);
> +		goto err_unmap_attachement;
> +	}
> +
> +	ref->shm.paddr = sg_dma_address(ref->sgt->sgl);
> +	ref->shm.size = sg_dma_len(ref->sgt->sgl);
> +	ref->shm.flags = TEE_SHM_EXT_DMA_BUF;
> +
> +	mutex_lock(&ref->shm.ctx->teedev->mutex);
> +	ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm,
> +				1, 0, GFP_KERNEL);
> +	mutex_unlock(&ref->shm.ctx->teedev->mutex);
> +	if (ref->shm.id < 0) {
> +		rc = ref->shm.id;
> +		goto err_idr_remove;
> +	}
> +
> +	return &ref->shm;
> +
> +err_idr_remove:
> +	mutex_lock(&ctx->teedev->mutex);
> +	idr_remove(&ctx->teedev->idr, ref->shm.id);
> +	mutex_unlock(&ctx->teedev->mutex);
> +err_unmap_attachement:
> +	dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
> +err_detach:
> +	dma_buf_detach(ref->dmabuf, ref->attach);
> +err_put_dmabuf:
> +	dma_buf_put(ref->dmabuf);
> +	kfree(ref);
> +err_put_tee:
> +	teedev_ctx_put(ctx);
> +	tee_device_put(ctx->teedev);
> +
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_register_fd);
> +
>   static struct tee_shm *
>   register_shm_helper(struct tee_context *ctx, unsigned long addr,
>   		    size_t length, u32 flags, int id)
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 911cad324acc..40ddd5376c2d 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -25,6 +25,7 @@
>   #define TEE_SHM_USER_MAPPED	BIT(1)  /* Memory mapped in user space */
>   #define TEE_SHM_POOL		BIT(2)  /* Memory allocated from pool */
>   #define TEE_SHM_PRIV		BIT(3)  /* Memory private to TEE driver */
> +#define TEE_SHM_EXT_DMA_BUF     BIT(4)  /* Memory with dma-buf handle */
>   
>   struct device;
>   struct tee_device;
> @@ -276,6 +277,16 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
>   struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
>   					    void *addr, size_t length);
>   
> +/**
> + * tee_shm_register_fd() - Register shared memory from file descriptor
> + *
> + * @ctx:	Context that allocates the shared memory
> + * @fd:		Shared memory file descriptor reference
> + *
> + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
> + */
> +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd);
> +
>   /**
>    * tee_shm_is_dynamic() - Check if shared memory object is of the dynamic kind
>    * @shm:	Shared memory handle
> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> index 25a6c534beb1..6f84060ad005 100644
> --- a/include/uapi/linux/tee.h
> +++ b/include/uapi/linux/tee.h
> @@ -121,6 +121,35 @@ struct tee_ioctl_shm_alloc_data {
>   #define TEE_IOC_SHM_ALLOC	_IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
>   				     struct tee_ioctl_shm_alloc_data)
>   
> +/**
> + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
> + * @fd:		[in] File descriptor identifying the shared memory
> + * @size:	[out] Size of shared memory to allocate
> + * @flags:	[in] Flags to/from allocation.
> + * @id:		[out] Identifier of the shared memory
> + *
> + * The flags field should currently be zero as input. Updated by the call
> + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below.
> + */
> +struct tee_ioctl_shm_register_fd_data {
> +	__s64 fd;
> +	__u64 size;
> +	__u32 flags;
> +	__s32 id;
> +} __aligned(8);
> +
> +/**
> + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor
> + *
> + * Returns a file descriptor on success or < 0 on failure
> + *
> + * The returned file descriptor refers to the shared memory object in kernel
> + * land. The shared memory is freed when the descriptor is closed.
> + */
> +#define TEE_IOC_SHM_REGISTER_FD	_IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \
> +				     struct tee_ioctl_shm_register_fd_data)
> +
>   /**
>    * struct tee_ioctl_buf_data - Variable sized buffer
>    * @buf_ptr:	[in] A __user pointer to a buffer


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXT] Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
  2022-08-12 10:18   ` Christian König
@ 2022-08-12 13:24     ` Olivier Masse
  0 siblings, 0 replies; 8+ messages in thread
From: Olivier Masse @ 2022-08-12 13:24 UTC (permalink / raw)
  To: sumit.garg, linux-kernel, sumit.semwal, linaro-mm-sig,
	christian.koenig, linux-media, dri-devel, jens.wiklander, op-tee
  Cc: Clément Faure, etienne.carriere

Hi Christian,

On ven., 2022-08-12 at 12:18 +0200, Christian König wrote:
> Caution: EXT Email
> 
> Hi Etienne,
> 
> at least I don't see anything obvious broken.
> 
> Just two comments:
> 
> 1. Dmitry is working on a change which renames some functions and
> makes
> it mandatory to call them with the dma_resv lock held.
> 
> Depending on how you want to upstream this change you will certainly
> run
> into conflicts with that.

ok could you send me some link to his changes ?

> 
> 2. Would it be possible to do this dynamically? In other words does
> the
> tee driver has a concept of buffers moving around?

I'm not sure to understand what you mean by a moving buffer ?
For information the TEE need to map the buffer in secure which is done
when calling the invoke operation function.

> 
> Am 11.08.22 um 15:56 schrieb Olivier Masse:
> > From: Etienne Carriere <etienne.carriere@linaro.org>
> > 
> > This change allows userland to create a tee_shm object that refers
> > to a dmabuf reference.
> > 
> > Userland provides a dmabuf file descriptor as buffer reference.
> > The created tee_shm object exported as a brand new dmabuf reference
> > used to provide a clean fd to userland. Userland shall closed this
> > new
> > fd to release the tee_shm object resources. The initial dmabuf
> > resources
> > are tracked independently through original dmabuf file descriptor.
> > 
> > Once the buffer is registered and until it is released, TEE driver
> > keeps a refcount on the registered dmabuf structure.
> > 
> > This change only support dmabuf references that relates to
> > physically
> > contiguous memory buffers.
> 
> That sounds like a pretty hard restriction, but I obviously don't see
> how to avoid it either.
> 
> Regards,
> Christian.
> 
> > 
> > New tee_shm flag to identify tee_shm objects built from a
> > registered
> > dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged
> > with
> > TEE_SHM_EXT_DMA_BUF.
> > 
> > Co-Developed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > From: 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux.git&amp;data=05%7C01%7Colivier.masse%40nxp.com%7C03c2d3cc95b8429693c108da7c4bf7a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637958962989857231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=vz8Y7ZwKXteePpWQVO8EhmZ1oyIX0xOCRd%2BGH7xHjII%3D&amp;reserved=0
> > (cherry picked from commit
> > 41e21e5c405530590dc2dd10b2a8dbe64589840f)
> > ---
> >   drivers/tee/tee_core.c   | 38 +++++++++++++++
> >   drivers/tee/tee_shm.c    | 99
> > +++++++++++++++++++++++++++++++++++++++-
> >   include/linux/tee_drv.h  | 11 +++++
> >   include/uapi/linux/tee.h | 29 ++++++++++++
> >   4 files changed, 175 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 8aa1a4836b92..7c45cbf85eb9 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -355,6 +355,42 @@ tee_ioctl_shm_register(struct tee_context
> > *ctx,
> >       return ret;
> >   }
> > 
> > +static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
> > +                                  struct
> > tee_ioctl_shm_register_fd_data __user *udata)
> > +{
> > +     struct tee_ioctl_shm_register_fd_data data;
> > +     struct tee_shm *shm;
> > +     long ret;
> > +
> > +     if (copy_from_user(&data, udata, sizeof(data)))
> > +             return -EFAULT;
> > +
> > +     /* Currently no input flags are supported */
> > +     if (data.flags)
> > +             return -EINVAL;
> > +
> > +     shm = tee_shm_register_fd(ctx, data.fd);
> > +     if (IS_ERR(shm))
> > +             return -EINVAL;
> > +
> > +     data.id = shm->id;
> > +     data.flags = shm->flags;
> > +     data.size = shm->size;
> > +
> > +     if (copy_to_user(udata, &data, sizeof(data)))
> > +             ret = -EFAULT;
> > +     else
> > +             ret = tee_shm_get_fd(shm);
> > +
> > +     /*
> > +      * When user space closes the file descriptor the shared
> > memory
> > +      * should be freed or if tee_shm_get_fd() failed then it will
> > +      * be freed immediately.
> > +      */
> > +     tee_shm_put(shm);
> > +     return ret;
> > +}
> > +
> >   static int params_from_user(struct tee_context *ctx, struct
> > tee_param *params,
> >                           size_t num_params,
> >                           struct tee_ioctl_param __user *uparams)
> > @@ -829,6 +865,8 @@ static long tee_ioctl(struct file *filp,
> > unsigned int cmd, unsigned long arg)
> >               return tee_ioctl_shm_alloc(ctx, uarg);
> >       case TEE_IOC_SHM_REGISTER:
> >               return tee_ioctl_shm_register(ctx, uarg);
> > +     case TEE_IOC_SHM_REGISTER_FD:
> > +             return tee_ioctl_shm_register_fd(ctx, uarg);
> >       case TEE_IOC_OPEN_SESSION:
> >               return tee_ioctl_open_session(ctx, uarg);
> >       case TEE_IOC_INVOKE:
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 836872467dc6..55a3fbbb022e 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -4,6 +4,7 @@
> >    */
> >   #include <linux/anon_inodes.h>
> >   #include <linux/device.h>
> > +#include <linux/dma-buf.h>
> >   #include <linux/idr.h>
> >   #include <linux/mm.h>
> >   #include <linux/sched.h>
> > @@ -12,6 +13,14 @@
> >   #include <linux/uio.h>
> >   #include "tee_private.h"
> > 
> > +/* extra references appended to shm object for registered shared
> > memory */
> > +struct tee_shm_dmabuf_ref {
> > +     struct tee_shm shm;
> > +     struct dma_buf *dmabuf;
> > +     struct dma_buf_attachment *attach;
> > +     struct sg_table *sgt;
> > +};
> > +
> >   static void shm_put_kernel_pages(struct page **pages, size_t
> > page_count)
> >   {
> >       size_t n;
> > @@ -71,7 +80,16 @@ static void release_registered_pages(struct
> > tee_shm *shm)
> > 
> >   static void tee_shm_release(struct tee_device *teedev, struct
> > tee_shm *shm)
> >   {
> > -     if (shm->flags & TEE_SHM_POOL) {
> > +     if (shm->flags & TEE_SHM_EXT_DMA_BUF) {
> > +             struct tee_shm_dmabuf_ref *ref;
> > +
> > +             ref = container_of(shm, struct tee_shm_dmabuf_ref,
> > shm);
> > +             dma_buf_unmap_attachment(ref->attach, ref->sgt,
> > +                                      DMA_BIDIRECTIONAL);
> > +
> > +             dma_buf_detach(ref->dmabuf, ref->attach);
> > +             dma_buf_put(ref->dmabuf);
> > +     } else if (shm->flags & TEE_SHM_POOL) {
> >               teedev->pool->ops->free(teedev->pool, shm);
> >       } else if (shm->flags & TEE_SHM_DYNAMIC) {
> >               int rc = teedev->desc->ops->shm_unregister(shm->ctx,
> > shm);
> > @@ -195,7 +213,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct
> > tee_context *ctx, size_t size)
> >    * tee_client_invoke_func(). The memory allocated is later freed
> > with a
> >    * call to tee_shm_free().
> >    *
> > - * @returns a pointer to 'struct tee_shm'
> > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR
> > on failure
> >    */
> >   struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx,
> > size_t size)
> >   {
> > @@ -229,6 +247,83 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct
> > tee_context *ctx, size_t size)
> >   }
> >   EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
> > 
> > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int
> > fd)
> > +{
> > +     struct tee_shm_dmabuf_ref *ref;
> > +     int rc;
> > +
> > +     if (!tee_device_get(ctx->teedev))
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     teedev_ctx_get(ctx);
> > +
> > +     ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> > +     if (!ref) {
> > +             rc = -ENOMEM;
> > +             goto err_put_tee;
> > +     }
> > +
> > +     refcount_set(&ref->shm.refcount, 1);
> > +     ref->shm.ctx = ctx;
> > +     ref->shm.id = -1;
> > +
> > +     ref->dmabuf = dma_buf_get(fd);
> > +     if (IS_ERR(ref->dmabuf)) {
> > +             rc = PTR_ERR(ref->dmabuf);
> > +             goto err_put_dmabuf;
> > +     }
> > +
> > +     ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx-
> > >teedev->dev);
> > +     if (IS_ERR(ref->attach)) {
> > +             rc = PTR_ERR(ref->attach);
> > +             goto err_detach;
> > +     }
> > +
> > +     ref->sgt = dma_buf_map_attachment(ref->attach,
> > DMA_BIDIRECTIONAL);
> > +     if (IS_ERR(ref->sgt)) {
> > +             rc = PTR_ERR(ref->sgt);
> > +             goto err_unmap_attachement;
> > +     }
> > +
> > +     if (sg_nents(ref->sgt->sgl) != 1) {
> > +             rc = PTR_ERR(ref->sgt->sgl);
> > +             goto err_unmap_attachement;
> > +     }
> > +
> > +     ref->shm.paddr = sg_dma_address(ref->sgt->sgl);
> > +     ref->shm.size = sg_dma_len(ref->sgt->sgl);
> > +     ref->shm.flags = TEE_SHM_EXT_DMA_BUF;
> > +
> > +     mutex_lock(&ref->shm.ctx->teedev->mutex);
> > +     ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref-
> > >shm,
> > +                             1, 0, GFP_KERNEL);
> > +     mutex_unlock(&ref->shm.ctx->teedev->mutex);
> > +     if (ref->shm.id < 0) {
> > +             rc = ref->shm.id;
> > +             goto err_idr_remove;
> > +     }
> > +
> > +     return &ref->shm;
> > +
> > +err_idr_remove:
> > +     mutex_lock(&ctx->teedev->mutex);
> > +     idr_remove(&ctx->teedev->idr, ref->shm.id);
> > +     mutex_unlock(&ctx->teedev->mutex);
> > +err_unmap_attachement:
> > +     dma_buf_unmap_attachment(ref->attach, ref->sgt,
> > DMA_BIDIRECTIONAL);
> > +err_detach:
> > +     dma_buf_detach(ref->dmabuf, ref->attach);
> > +err_put_dmabuf:
> > +     dma_buf_put(ref->dmabuf);
> > +     kfree(ref);
> > +err_put_tee:
> > +     teedev_ctx_put(ctx);
> > +     tee_device_put(ctx->teedev);
> > +
> > +     return ERR_PTR(rc);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_register_fd);
> > +
> >   static struct tee_shm *
> >   register_shm_helper(struct tee_context *ctx, unsigned long addr,
> >                   size_t length, u32 flags, int id)
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 911cad324acc..40ddd5376c2d 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -25,6 +25,7 @@
> >   #define TEE_SHM_USER_MAPPED BIT(1)  /* Memory mapped in user
> > space */
> >   #define TEE_SHM_POOL                BIT(2)  /* Memory allocated
> > from pool */
> >   #define TEE_SHM_PRIV                BIT(3)  /* Memory private to
> > TEE driver */
> > +#define TEE_SHM_EXT_DMA_BUF     BIT(4)  /* Memory with dma-buf
> > handle */
> > 
> >   struct device;
> >   struct tee_device;
> > @@ -276,6 +277,16 @@ struct tee_shm
> > *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> >   struct tee_shm *tee_shm_register_kernel_buf(struct tee_context
> > *ctx,
> >                                           void *addr, size_t
> > length);
> > 
> > +/**
> > + * tee_shm_register_fd() - Register shared memory from file
> > descriptor
> > + *
> > + * @ctx:     Context that allocates the shared memory
> > + * @fd:              Shared memory file descriptor reference
> > + *
> > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR
> > on failure
> > + */
> > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int
> > fd);
> > +
> >   /**
> >    * tee_shm_is_dynamic() - Check if shared memory object is of the
> > dynamic kind
> >    * @shm:    Shared memory handle
> > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> > index 25a6c534beb1..6f84060ad005 100644
> > --- a/include/uapi/linux/tee.h
> > +++ b/include/uapi/linux/tee.h
> > @@ -121,6 +121,35 @@ struct tee_ioctl_shm_alloc_data {
> >   #define TEE_IOC_SHM_ALLOC   _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE +
> > 1, \
> >                                    struct tee_ioctl_shm_alloc_data)
> > 
> > +/**
> > + * struct tee_ioctl_shm_register_fd_data - Shared memory
> > registering argument
> > + * @fd:              [in] File descriptor identifying the shared
> > memory
> > + * @size:    [out] Size of shared memory to allocate
> > + * @flags:   [in] Flags to/from allocation.
> > + * @id:              [out] Identifier of the shared memory
> > + *
> > + * The flags field should currently be zero as input. Updated by
> > the call
> > + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> > + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD
> > below.
> > + */
> > +struct tee_ioctl_shm_register_fd_data {
> > +     __s64 fd;
> > +     __u64 size;
> > +     __u32 flags;
> > +     __s32 id;
> > +} __aligned(8);
> > +
> > +/**
> > + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file
> > descriptor
> > + *
> > + * Returns a file descriptor on success or < 0 on failure
> > + *
> > + * The returned file descriptor refers to the shared memory object
> > in kernel
> > + * land. The shared memory is freed when the descriptor is closed.
> > + */
> > +#define TEE_IOC_SHM_REGISTER_FD      _IOWR(TEE_IOC_MAGIC,
> > TEE_IOC_BASE + 8, \
> > +                                  struct
> > tee_ioctl_shm_register_fd_data)
> > +
> >   /**
> >    * struct tee_ioctl_buf_data - Variable sized buffer
> >    * @buf_ptr:        [in] A __user pointer to a buffer
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/1] tee: Add tee_shm_register_fd
  2022-08-11 13:56 [PATCH 0/1] tee: Add tee_shm_register_fd Olivier Masse
  2022-08-11 13:56 ` [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Olivier Masse
@ 2022-08-12 14:01 ` Sumit Garg
  1 sibling, 0 replies; 8+ messages in thread
From: Sumit Garg @ 2022-08-12 14:01 UTC (permalink / raw)
  To: Olivier Masse
  Cc: jens.wiklander, sumit.semwal, christian.koenig, op-tee,
	linaro-mm-sig, linux-kernel, linux-media, dri-devel,
	clement.faure

Hi Olivier,

On Thu, 11 Aug 2022 at 19:26, Olivier Masse <olivier.masse@nxp.com> wrote:
>
> Add a new ioctl called TEE_IOC_SHM_REGISTER_FD to register a
> shared memory from a dmabuf file descriptor.
>

IIRC, here you are trying to fill the gap for OP-TEE SDP use-case. But
the use-case should be described here, its current status with OP-TEE
upstream and which platform you have tested this interface with?

-Sumit

> Etienne Carriere (1):
>   tee: new ioctl to a register tee_shm from a dmabuf file descriptor
>
>  drivers/tee/tee_core.c   | 38 +++++++++++++++
>  drivers/tee/tee_shm.c    | 99 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/tee_drv.h  | 11 +++++
>  include/uapi/linux/tee.h | 29 ++++++++++++
>  4 files changed, 175 insertions(+), 2 deletions(-)
>
> --
> 2.25.0
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
  2022-08-11 13:56 ` [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Olivier Masse
  2022-08-12  5:27   ` kernel test robot
  2022-08-12 10:18   ` Christian König
@ 2022-08-12 14:06   ` Sumit Garg
  2022-08-12 14:23     ` [EXT] " Olivier Masse
  2 siblings, 1 reply; 8+ messages in thread
From: Sumit Garg @ 2022-08-12 14:06 UTC (permalink / raw)
  To: Olivier Masse
  Cc: jens.wiklander, sumit.semwal, christian.koenig, op-tee,
	linaro-mm-sig, linux-kernel, linux-media, dri-devel,
	clement.faure

Hi Olivier,

On Thu, 11 Aug 2022 at 19:26, Olivier Masse <olivier.masse@nxp.com> wrote:
>
> From: Etienne Carriere <etienne.carriere@linaro.org>
>
> This change allows userland to create a tee_shm object that refers
> to a dmabuf reference.
>
> Userland provides a dmabuf file descriptor as buffer reference.
> The created tee_shm object exported as a brand new dmabuf reference
> used to provide a clean fd to userland. Userland shall closed this new
> fd to release the tee_shm object resources. The initial dmabuf resources
> are tracked independently through original dmabuf file descriptor.
>
> Once the buffer is registered and until it is released, TEE driver
> keeps a refcount on the registered dmabuf structure.
>
> This change only support dmabuf references that relates to physically
> contiguous memory buffers.

What limits you to extend this feature to non-contiguous memory
buffers? I believe that should be possible with OP-TEE dynamic shared
memory which gives you the granularity to register a list of pages.

-Sumit

>
> New tee_shm flag to identify tee_shm objects built from a registered
> dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged with
> TEE_SHM_EXT_DMA_BUF.
>
> Co-Developed-by: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> From: https://github.com/linaro-swg/linux.git
> (cherry picked from commit 41e21e5c405530590dc2dd10b2a8dbe64589840f)
> ---
>  drivers/tee/tee_core.c   | 38 +++++++++++++++
>  drivers/tee/tee_shm.c    | 99 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/tee_drv.h  | 11 +++++
>  include/uapi/linux/tee.h | 29 ++++++++++++
>  4 files changed, 175 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 8aa1a4836b92..7c45cbf85eb9 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -355,6 +355,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>         return ret;
>  }
>
> +static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
> +                                    struct tee_ioctl_shm_register_fd_data __user *udata)
> +{
> +       struct tee_ioctl_shm_register_fd_data data;
> +       struct tee_shm *shm;
> +       long ret;
> +
> +       if (copy_from_user(&data, udata, sizeof(data)))
> +               return -EFAULT;
> +
> +       /* Currently no input flags are supported */
> +       if (data.flags)
> +               return -EINVAL;
> +
> +       shm = tee_shm_register_fd(ctx, data.fd);
> +       if (IS_ERR(shm))
> +               return -EINVAL;
> +
> +       data.id = shm->id;
> +       data.flags = shm->flags;
> +       data.size = shm->size;
> +
> +       if (copy_to_user(udata, &data, sizeof(data)))
> +               ret = -EFAULT;
> +       else
> +               ret = tee_shm_get_fd(shm);
> +
> +       /*
> +        * When user space closes the file descriptor the shared memory
> +        * should be freed or if tee_shm_get_fd() failed then it will
> +        * be freed immediately.
> +        */
> +       tee_shm_put(shm);
> +       return ret;
> +}
> +
>  static int params_from_user(struct tee_context *ctx, struct tee_param *params,
>                             size_t num_params,
>                             struct tee_ioctl_param __user *uparams)
> @@ -829,6 +865,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                 return tee_ioctl_shm_alloc(ctx, uarg);
>         case TEE_IOC_SHM_REGISTER:
>                 return tee_ioctl_shm_register(ctx, uarg);
> +       case TEE_IOC_SHM_REGISTER_FD:
> +               return tee_ioctl_shm_register_fd(ctx, uarg);
>         case TEE_IOC_OPEN_SESSION:
>                 return tee_ioctl_open_session(ctx, uarg);
>         case TEE_IOC_INVOKE:
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 836872467dc6..55a3fbbb022e 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -4,6 +4,7 @@
>   */
>  #include <linux/anon_inodes.h>
>  #include <linux/device.h>
> +#include <linux/dma-buf.h>
>  #include <linux/idr.h>
>  #include <linux/mm.h>
>  #include <linux/sched.h>
> @@ -12,6 +13,14 @@
>  #include <linux/uio.h>
>  #include "tee_private.h"
>
> +/* extra references appended to shm object for registered shared memory */
> +struct tee_shm_dmabuf_ref {
> +     struct tee_shm shm;
> +     struct dma_buf *dmabuf;
> +     struct dma_buf_attachment *attach;
> +     struct sg_table *sgt;
> +};
> +
>  static void shm_put_kernel_pages(struct page **pages, size_t page_count)
>  {
>         size_t n;
> @@ -71,7 +80,16 @@ static void release_registered_pages(struct tee_shm *shm)
>
>  static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
>  {
> -       if (shm->flags & TEE_SHM_POOL) {
> +       if (shm->flags & TEE_SHM_EXT_DMA_BUF) {
> +               struct tee_shm_dmabuf_ref *ref;
> +
> +               ref = container_of(shm, struct tee_shm_dmabuf_ref, shm);
> +               dma_buf_unmap_attachment(ref->attach, ref->sgt,
> +                                        DMA_BIDIRECTIONAL);
> +
> +               dma_buf_detach(ref->dmabuf, ref->attach);
> +               dma_buf_put(ref->dmabuf);
> +       } else if (shm->flags & TEE_SHM_POOL) {
>                 teedev->pool->ops->free(teedev->pool, shm);
>         } else if (shm->flags & TEE_SHM_DYNAMIC) {
>                 int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
> @@ -195,7 +213,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
>   * tee_client_invoke_func(). The memory allocated is later freed with a
>   * call to tee_shm_free().
>   *
> - * @returns a pointer to 'struct tee_shm'
> + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
>   */
>  struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
>  {
> @@ -229,6 +247,83 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size)
>  }
>  EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
>
> +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd)
> +{
> +       struct tee_shm_dmabuf_ref *ref;
> +       int rc;
> +
> +       if (!tee_device_get(ctx->teedev))
> +               return ERR_PTR(-EINVAL);
> +
> +       teedev_ctx_get(ctx);
> +
> +       ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> +       if (!ref) {
> +               rc = -ENOMEM;
> +               goto err_put_tee;
> +       }
> +
> +       refcount_set(&ref->shm.refcount, 1);
> +       ref->shm.ctx = ctx;
> +       ref->shm.id = -1;
> +
> +       ref->dmabuf = dma_buf_get(fd);
> +       if (IS_ERR(ref->dmabuf)) {
> +               rc = PTR_ERR(ref->dmabuf);
> +               goto err_put_dmabuf;
> +       }
> +
> +       ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
> +       if (IS_ERR(ref->attach)) {
> +               rc = PTR_ERR(ref->attach);
> +               goto err_detach;
> +       }
> +
> +       ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL);
> +       if (IS_ERR(ref->sgt)) {
> +               rc = PTR_ERR(ref->sgt);
> +               goto err_unmap_attachement;
> +       }
> +
> +       if (sg_nents(ref->sgt->sgl) != 1) {
> +               rc = PTR_ERR(ref->sgt->sgl);
> +               goto err_unmap_attachement;
> +       }
> +
> +       ref->shm.paddr = sg_dma_address(ref->sgt->sgl);
> +       ref->shm.size = sg_dma_len(ref->sgt->sgl);
> +       ref->shm.flags = TEE_SHM_EXT_DMA_BUF;
> +
> +       mutex_lock(&ref->shm.ctx->teedev->mutex);
> +       ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm,
> +                               1, 0, GFP_KERNEL);
> +       mutex_unlock(&ref->shm.ctx->teedev->mutex);
> +       if (ref->shm.id < 0) {
> +               rc = ref->shm.id;
> +               goto err_idr_remove;
> +       }
> +
> +       return &ref->shm;
> +
> +err_idr_remove:
> +       mutex_lock(&ctx->teedev->mutex);
> +       idr_remove(&ctx->teedev->idr, ref->shm.id);
> +       mutex_unlock(&ctx->teedev->mutex);
> +err_unmap_attachement:
> +       dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
> +err_detach:
> +       dma_buf_detach(ref->dmabuf, ref->attach);
> +err_put_dmabuf:
> +       dma_buf_put(ref->dmabuf);
> +       kfree(ref);
> +err_put_tee:
> +       teedev_ctx_put(ctx);
> +       tee_device_put(ctx->teedev);
> +
> +       return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_register_fd);
> +
>  static struct tee_shm *
>  register_shm_helper(struct tee_context *ctx, unsigned long addr,
>                     size_t length, u32 flags, int id)
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 911cad324acc..40ddd5376c2d 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -25,6 +25,7 @@
>  #define TEE_SHM_USER_MAPPED    BIT(1)  /* Memory mapped in user space */
>  #define TEE_SHM_POOL           BIT(2)  /* Memory allocated from pool */
>  #define TEE_SHM_PRIV           BIT(3)  /* Memory private to TEE driver */
> +#define TEE_SHM_EXT_DMA_BUF     BIT(4)  /* Memory with dma-buf handle */
>
>  struct device;
>  struct tee_device;
> @@ -276,6 +277,16 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
>  struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
>                                             void *addr, size_t length);
>
> +/**
> + * tee_shm_register_fd() - Register shared memory from file descriptor
> + *
> + * @ctx:       Context that allocates the shared memory
> + * @fd:                Shared memory file descriptor reference
> + *
> + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
> + */
> +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd);
> +
>  /**
>   * tee_shm_is_dynamic() - Check if shared memory object is of the dynamic kind
>   * @shm:       Shared memory handle
> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> index 25a6c534beb1..6f84060ad005 100644
> --- a/include/uapi/linux/tee.h
> +++ b/include/uapi/linux/tee.h
> @@ -121,6 +121,35 @@ struct tee_ioctl_shm_alloc_data {
>  #define TEE_IOC_SHM_ALLOC      _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
>                                      struct tee_ioctl_shm_alloc_data)
>
> +/**
> + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
> + * @fd:                [in] File descriptor identifying the shared memory
> + * @size:      [out] Size of shared memory to allocate
> + * @flags:     [in] Flags to/from allocation.
> + * @id:                [out] Identifier of the shared memory
> + *
> + * The flags field should currently be zero as input. Updated by the call
> + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below.
> + */
> +struct tee_ioctl_shm_register_fd_data {
> +       __s64 fd;
> +       __u64 size;
> +       __u32 flags;
> +       __s32 id;
> +} __aligned(8);
> +
> +/**
> + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor
> + *
> + * Returns a file descriptor on success or < 0 on failure
> + *
> + * The returned file descriptor refers to the shared memory object in kernel
> + * land. The shared memory is freed when the descriptor is closed.
> + */
> +#define TEE_IOC_SHM_REGISTER_FD        _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \
> +                                    struct tee_ioctl_shm_register_fd_data)
> +
>  /**
>   * struct tee_ioctl_buf_data - Variable sized buffer
>   * @buf_ptr:   [in] A __user pointer to a buffer
> --
> 2.25.0
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXT] Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
  2022-08-12 14:06   ` Sumit Garg
@ 2022-08-12 14:23     ` Olivier Masse
  0 siblings, 0 replies; 8+ messages in thread
From: Olivier Masse @ 2022-08-12 14:23 UTC (permalink / raw)
  To: sumit.garg
  Cc: sumit.semwal, linux-kernel, linaro-mm-sig, christian.koenig,
	linux-media, Clément Faure, dri-devel, jens.wiklander,
	op-tee

Hi Sumit,

Thanks for your comments, please find my answer below.

On ven., 2022-08-12 at 19:36 +0530, Sumit Garg wrote:
> Caution: EXT Email
> 
> Hi Olivier,
> 
> On Thu, 11 Aug 2022 at 19:26, Olivier Masse <olivier.masse@nxp.com>
> wrote:
> > 
> > From: Etienne Carriere <etienne.carriere@linaro.org>
> > 
> > This change allows userland to create a tee_shm object that refers
> > to a dmabuf reference.
> > 
> > Userland provides a dmabuf file descriptor as buffer reference.
> > The created tee_shm object exported as a brand new dmabuf reference
> > used to provide a clean fd to userland. Userland shall closed this
> > new
> > fd to release the tee_shm object resources. The initial dmabuf
> > resources
> > are tracked independently through original dmabuf file descriptor.
> > 
> > Once the buffer is registered and until it is released, TEE driver
> > keeps a refcount on the registered dmabuf structure.
> > 
> > This change only support dmabuf references that relates to
> > physically
> > contiguous memory buffers.
> 
> What limits you to extend this feature to non-contiguous memory
> buffers? I believe that should be possible with OP-TEE dynamic shared
> memory which gives you the granularity to register a list of pages.

But it will need more logic in OPTEE OS to verify that all pages are in
the Secure Data Path protected area.
Our solution use a fixed protected reserved memory region and do not
rely on a dynamic protection set in secure.

Best regards,
Olivier

> 
> -Sumit
> 
> > 
> > New tee_shm flag to identify tee_shm objects built from a
> > registered
> > dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged
> > with
> > TEE_SHM_EXT_DMA_BUF.
> > 
> > Co-Developed-by: Etienne Carriere <etienne.carriere@linaro.org>
> > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > From: 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux.git&amp;data=05%7C01%7Colivier.masse%40nxp.com%7C204e0821d1c84355ed4208da7c6be5c6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637959100100176447%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=MRfnQNbwAIj%2Bnd9tSjSb5Nzrv3sQVpVMIdOxxRfu6U4%3D&amp;reserved=0
> > (cherry picked from commit
> > 41e21e5c405530590dc2dd10b2a8dbe64589840f)
> > ---
> >  drivers/tee/tee_core.c   | 38 +++++++++++++++
> >  drivers/tee/tee_shm.c    | 99
> > +++++++++++++++++++++++++++++++++++++++-
> >  include/linux/tee_drv.h  | 11 +++++
> >  include/uapi/linux/tee.h | 29 ++++++++++++
> >  4 files changed, 175 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 8aa1a4836b92..7c45cbf85eb9 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -355,6 +355,42 @@ tee_ioctl_shm_register(struct tee_context
> > *ctx,
> >         return ret;
> >  }
> > 
> > +static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
> > +                                    struct
> > tee_ioctl_shm_register_fd_data __user *udata)
> > +{
> > +       struct tee_ioctl_shm_register_fd_data data;
> > +       struct tee_shm *shm;
> > +       long ret;
> > +
> > +       if (copy_from_user(&data, udata, sizeof(data)))
> > +               return -EFAULT;
> > +
> > +       /* Currently no input flags are supported */
> > +       if (data.flags)
> > +               return -EINVAL;
> > +
> > +       shm = tee_shm_register_fd(ctx, data.fd);
> > +       if (IS_ERR(shm))
> > +               return -EINVAL;
> > +
> > +       data.id = shm->id;
> > +       data.flags = shm->flags;
> > +       data.size = shm->size;
> > +
> > +       if (copy_to_user(udata, &data, sizeof(data)))
> > +               ret = -EFAULT;
> > +       else
> > +               ret = tee_shm_get_fd(shm);
> > +
> > +       /*
> > +        * When user space closes the file descriptor the shared
> > memory
> > +        * should be freed or if tee_shm_get_fd() failed then it
> > will
> > +        * be freed immediately.
> > +        */
> > +       tee_shm_put(shm);
> > +       return ret;
> > +}
> > +
> >  static int params_from_user(struct tee_context *ctx, struct
> > tee_param *params,
> >                             size_t num_params,
> >                             struct tee_ioctl_param __user *uparams)
> > @@ -829,6 +865,8 @@ static long tee_ioctl(struct file *filp,
> > unsigned int cmd, unsigned long arg)
> >                 return tee_ioctl_shm_alloc(ctx, uarg);
> >         case TEE_IOC_SHM_REGISTER:
> >                 return tee_ioctl_shm_register(ctx, uarg);
> > +       case TEE_IOC_SHM_REGISTER_FD:
> > +               return tee_ioctl_shm_register_fd(ctx, uarg);
> >         case TEE_IOC_OPEN_SESSION:
> >                 return tee_ioctl_open_session(ctx, uarg);
> >         case TEE_IOC_INVOKE:
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 836872467dc6..55a3fbbb022e 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -4,6 +4,7 @@
> >   */
> >  #include <linux/anon_inodes.h>
> >  #include <linux/device.h>
> > +#include <linux/dma-buf.h>
> >  #include <linux/idr.h>
> >  #include <linux/mm.h>
> >  #include <linux/sched.h>
> > @@ -12,6 +13,14 @@
> >  #include <linux/uio.h>
> >  #include "tee_private.h"
> > 
> > +/* extra references appended to shm object for registered shared
> > memory */
> > +struct tee_shm_dmabuf_ref {
> > +     struct tee_shm shm;
> > +     struct dma_buf *dmabuf;
> > +     struct dma_buf_attachment *attach;
> > +     struct sg_table *sgt;
> > +};
> > +
> >  static void shm_put_kernel_pages(struct page **pages, size_t
> > page_count)
> >  {
> >         size_t n;
> > @@ -71,7 +80,16 @@ static void release_registered_pages(struct
> > tee_shm *shm)
> > 
> >  static void tee_shm_release(struct tee_device *teedev, struct
> > tee_shm *shm)
> >  {
> > -       if (shm->flags & TEE_SHM_POOL) {
> > +       if (shm->flags & TEE_SHM_EXT_DMA_BUF) {
> > +               struct tee_shm_dmabuf_ref *ref;
> > +
> > +               ref = container_of(shm, struct tee_shm_dmabuf_ref,
> > shm);
> > +               dma_buf_unmap_attachment(ref->attach, ref->sgt,
> > +                                        DMA_BIDIRECTIONAL);
> > +
> > +               dma_buf_detach(ref->dmabuf, ref->attach);
> > +               dma_buf_put(ref->dmabuf);
> > +       } else if (shm->flags & TEE_SHM_POOL) {
> >                 teedev->pool->ops->free(teedev->pool, shm);
> >         } else if (shm->flags & TEE_SHM_DYNAMIC) {
> >                 int rc = teedev->desc->ops->shm_unregister(shm-
> > >ctx, shm);
> > @@ -195,7 +213,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct
> > tee_context *ctx, size_t size)
> >   * tee_client_invoke_func(). The memory allocated is later freed
> > with a
> >   * call to tee_shm_free().
> >   *
> > - * @returns a pointer to 'struct tee_shm'
> > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR
> > on failure
> >   */
> >  struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx,
> > size_t size)
> >  {
> > @@ -229,6 +247,83 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct
> > tee_context *ctx, size_t size)
> >  }
> >  EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf);
> > 
> > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int
> > fd)
> > +{
> > +       struct tee_shm_dmabuf_ref *ref;
> > +       int rc;
> > +
> > +       if (!tee_device_get(ctx->teedev))
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       teedev_ctx_get(ctx);
> > +
> > +       ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> > +       if (!ref) {
> > +               rc = -ENOMEM;
> > +               goto err_put_tee;
> > +       }
> > +
> > +       refcount_set(&ref->shm.refcount, 1);
> > +       ref->shm.ctx = ctx;
> > +       ref->shm.id = -1;
> > +
> > +       ref->dmabuf = dma_buf_get(fd);
> > +       if (IS_ERR(ref->dmabuf)) {
> > +               rc = PTR_ERR(ref->dmabuf);
> > +               goto err_put_dmabuf;
> > +       }
> > +
> > +       ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx-
> > >teedev->dev);
> > +       if (IS_ERR(ref->attach)) {
> > +               rc = PTR_ERR(ref->attach);
> > +               goto err_detach;
> > +       }
> > +
> > +       ref->sgt = dma_buf_map_attachment(ref->attach,
> > DMA_BIDIRECTIONAL);
> > +       if (IS_ERR(ref->sgt)) {
> > +               rc = PTR_ERR(ref->sgt);
> > +               goto err_unmap_attachement;
> > +       }
> > +
> > +       if (sg_nents(ref->sgt->sgl) != 1) {
> > +               rc = PTR_ERR(ref->sgt->sgl);
> > +               goto err_unmap_attachement;
> > +       }
> > +
> > +       ref->shm.paddr = sg_dma_address(ref->sgt->sgl);
> > +       ref->shm.size = sg_dma_len(ref->sgt->sgl);
> > +       ref->shm.flags = TEE_SHM_EXT_DMA_BUF;
> > +
> > +       mutex_lock(&ref->shm.ctx->teedev->mutex);
> > +       ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref-
> > >shm,
> > +                               1, 0, GFP_KERNEL);
> > +       mutex_unlock(&ref->shm.ctx->teedev->mutex);
> > +       if (ref->shm.id < 0) {
> > +               rc = ref->shm.id;
> > +               goto err_idr_remove;
> > +       }
> > +
> > +       return &ref->shm;
> > +
> > +err_idr_remove:
> > +       mutex_lock(&ctx->teedev->mutex);
> > +       idr_remove(&ctx->teedev->idr, ref->shm.id);
> > +       mutex_unlock(&ctx->teedev->mutex);
> > +err_unmap_attachement:
> > +       dma_buf_unmap_attachment(ref->attach, ref->sgt,
> > DMA_BIDIRECTIONAL);
> > +err_detach:
> > +       dma_buf_detach(ref->dmabuf, ref->attach);
> > +err_put_dmabuf:
> > +       dma_buf_put(ref->dmabuf);
> > +       kfree(ref);
> > +err_put_tee:
> > +       teedev_ctx_put(ctx);
> > +       tee_device_put(ctx->teedev);
> > +
> > +       return ERR_PTR(rc);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_register_fd);
> > +
> >  static struct tee_shm *
> >  register_shm_helper(struct tee_context *ctx, unsigned long addr,
> >                     size_t length, u32 flags, int id)
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 911cad324acc..40ddd5376c2d 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -25,6 +25,7 @@
> >  #define TEE_SHM_USER_MAPPED    BIT(1)  /* Memory mapped in user
> > space */
> >  #define TEE_SHM_POOL           BIT(2)  /* Memory allocated from
> > pool */
> >  #define TEE_SHM_PRIV           BIT(3)  /* Memory private to TEE
> > driver */
> > +#define TEE_SHM_EXT_DMA_BUF     BIT(4)  /* Memory with dma-buf
> > handle */
> > 
> >  struct device;
> >  struct tee_device;
> > @@ -276,6 +277,16 @@ struct tee_shm
> > *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> >  struct tee_shm *tee_shm_register_kernel_buf(struct tee_context
> > *ctx,
> >                                             void *addr, size_t
> > length);
> > 
> > +/**
> > + * tee_shm_register_fd() - Register shared memory from file
> > descriptor
> > + *
> > + * @ctx:       Context that allocates the shared memory
> > + * @fd:                Shared memory file descriptor reference
> > + *
> > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR
> > on failure
> > + */
> > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int
> > fd);
> > +
> >  /**
> >   * tee_shm_is_dynamic() - Check if shared memory object is of the
> > dynamic kind
> >   * @shm:       Shared memory handle
> > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> > index 25a6c534beb1..6f84060ad005 100644
> > --- a/include/uapi/linux/tee.h
> > +++ b/include/uapi/linux/tee.h
> > @@ -121,6 +121,35 @@ struct tee_ioctl_shm_alloc_data {
> >  #define TEE_IOC_SHM_ALLOC      _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE +
> > 1, \
> >                                      struct
> > tee_ioctl_shm_alloc_data)
> > 
> > +/**
> > + * struct tee_ioctl_shm_register_fd_data - Shared memory
> > registering argument
> > + * @fd:                [in] File descriptor identifying the shared
> > memory
> > + * @size:      [out] Size of shared memory to allocate
> > + * @flags:     [in] Flags to/from allocation.
> > + * @id:                [out] Identifier of the shared memory
> > + *
> > + * The flags field should currently be zero as input. Updated by
> > the call
> > + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> > + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD
> > below.
> > + */
> > +struct tee_ioctl_shm_register_fd_data {
> > +       __s64 fd;
> > +       __u64 size;
> > +       __u32 flags;
> > +       __s32 id;
> > +} __aligned(8);
> > +
> > +/**
> > + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file
> > descriptor
> > + *
> > + * Returns a file descriptor on success or < 0 on failure
> > + *
> > + * The returned file descriptor refers to the shared memory object
> > in kernel
> > + * land. The shared memory is freed when the descriptor is closed.
> > + */
> > +#define TEE_IOC_SHM_REGISTER_FD        _IOWR(TEE_IOC_MAGIC,
> > TEE_IOC_BASE + 8, \
> > +                                    struct
> > tee_ioctl_shm_register_fd_data)
> > +
> >  /**
> >   * struct tee_ioctl_buf_data - Variable sized buffer
> >   * @buf_ptr:   [in] A __user pointer to a buffer
> > --
> > 2.25.0
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-08-12 14:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-11 13:56 [PATCH 0/1] tee: Add tee_shm_register_fd Olivier Masse
2022-08-11 13:56 ` [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor Olivier Masse
2022-08-12  5:27   ` kernel test robot
2022-08-12 10:18   ` Christian König
2022-08-12 13:24     ` [EXT] " Olivier Masse
2022-08-12 14:06   ` Sumit Garg
2022-08-12 14:23     ` [EXT] " Olivier Masse
2022-08-12 14:01 ` [PATCH 0/1] tee: Add tee_shm_register_fd Sumit Garg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).