On 01.04.25 12:13, Sumit Garg wrote:
> + MM folks to seek guidance here.
>
> On Thu, Mar 27, 2025 at 09:07:34AM +0100, Jens Wiklander wrote:
>> Hi Sumit,
>>
>> On Tue, Mar 25, 2025 at 8:42 AM Sumit Garg <sumit.garg(a)kernel.org> wrote:
>>>
>>> On Wed, Mar 05, 2025 at 02:04:15PM +0100, Jens Wiklander wrote:
>>>> Add support in the OP-TEE backend driver dynamic restricted memory
>>>> allocation with FF-A.
>>>>
>>>> The restricted memory pools for dynamically allocated restrict memory
>>>> are instantiated when requested by user-space. This instantiation can
>>>> fail if OP-TEE doesn't support the requested use-case of restricted
>>>> memory.
>>>>
>>>> Restricted memory pools based on a static carveout or dynamic allocation
>>>> can coexist for different use-cases. We use only dynamic allocation with
>>>> FF-A.
>>>>
>>>> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org>
>>>> ---
>>>> drivers/tee/optee/Makefile | 1 +
>>>> drivers/tee/optee/ffa_abi.c | 143 ++++++++++++-
>>>> drivers/tee/optee/optee_private.h | 13 +-
>>>> drivers/tee/optee/rstmem.c | 329 ++++++++++++++++++++++++++++++
>>>> 4 files changed, 483 insertions(+), 3 deletions(-)
>>>> create mode 100644 drivers/tee/optee/rstmem.c
>>>>
>
> <snip>
>
>>>> diff --git a/drivers/tee/optee/rstmem.c b/drivers/tee/optee/rstmem.c
>>>> new file mode 100644
>>>> index 000000000000..ea27769934d4
>>>> --- /dev/null
>>>> +++ b/drivers/tee/optee/rstmem.c
>>>> @@ -0,0 +1,329 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2025, Linaro Limited
>>>> + */
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#include <linux/errno.h>
>>>> +#include <linux/genalloc.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/string.h>
>>>> +#include <linux/tee_core.h>
>>>> +#include <linux/types.h>
>>>> +#include "optee_private.h"
>>>> +
>>>> +struct optee_rstmem_cma_pool {
>>>> + struct tee_rstmem_pool pool;
>>>> + struct gen_pool *gen_pool;
>>>> + struct optee *optee;
>>>> + size_t page_count;
>>>> + u16 *end_points;
>>>> + u_int end_point_count;
>>>> + u_int align;
>>>> + refcount_t refcount;
>>>> + u32 use_case;
>>>> + struct tee_shm *rstmem;
>>>> + /* Protects when initializing and tearing down this struct */
>>>> + struct mutex mutex;
>>>> +};
>>>> +
>>>> +static struct optee_rstmem_cma_pool *
>>>> +to_rstmem_cma_pool(struct tee_rstmem_pool *pool)
>>>> +{
>>>> + return container_of(pool, struct optee_rstmem_cma_pool, pool);
>>>> +}
>>>> +
>>>> +static int init_cma_rstmem(struct optee_rstmem_cma_pool *rp)
>>>> +{
>>>> + int rc;
>>>> +
>>>> + rp->rstmem = tee_shm_alloc_cma_phys_mem(rp->optee->ctx, rp->page_count,
>>>> + rp->align);
>>>> + if (IS_ERR(rp->rstmem)) {
>>>> + rc = PTR_ERR(rp->rstmem);
>>>> + goto err_null_rstmem;
>>>> + }
>>>> +
>>>> + /*
>>>> + * TODO unmap the memory range since the physical memory will
>>>> + * become inaccesible after the lend_rstmem() call.
>>>> + */
>>>
>>> What's your plan for this TODO? I think we need a CMA allocator here
>>> which can allocate un-mapped memory such that any cache speculation
>>> won't lead to CPU hangs once the memory restriction comes into picture.
>>
>> What happens is platform-specific. For some platforms, it might be
>> enough to avoid explicit access. Yes, a CMA allocator with unmapped
>> memory or where memory can be unmapped is one option.
>
> Did you get a chance to enable real memory protection on RockPi board?
> This will atleast ensure that mapped restricted memory without explicit
> access works fine. Since otherwise once people start to enable real
> memory restriction in OP-TEE, there can be chances of random hang ups
> due to cache speculation.
>
> MM folks,
>
> Basically what we are trying to achieve here is a "no-map" DT behaviour
> [1] which is rather dynamic in nature. The use-case here is that a memory
> block allocated from CMA can be marked restricted at runtime where we
> would like the Linux not being able to directly or indirectly (cache
> speculation) access it. Once memory restriction use-case has been
> completed, the memory block can be marked as normal and freed for
> further CMA allocation.
>
> It will be apprciated if you can guide us regarding the appropriate APIs
> to use for un-mapping/mamping CMA allocations for this use-case.
Can we get some more information why that is even required, so we can
decide if that is even the right thing to do? :)
Who would mark the memory block as restricted and for which purpose?
In arch/powerpc/platforms/powernv/memtrace.c we have some arch-specific
code to remove the directmap after alloc_contig_pages(). See
memtrace_alloc_node(). But it's very arch-specific ...
--
Cheers,
David / dhildenb
On Wed, Apr 9, 2025 at 9:20 AM Amirreza Zarrabi
<amirreza.zarrabi(a)oss.qualcomm.com> wrote:
>
>
>
> On 4/9/2025 4:41 PM, Jens Wiklander wrote:
> > Hi Amirreza,
> >
> > On Wed, Apr 9, 2025 at 2:28 AM Amirreza Zarrabi
> > <amirreza.zarrabi(a)oss.qualcomm.com> wrote:
> >>
> >> Hi jens,
> >>
> >> On 4/8/2025 10:19 PM, Jens Wiklander wrote:
> >>
> >> Hi Amirreza,
> >>
> >> On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi
> >> <amirreza.zarrabi(a)oss.qualcomm.com> wrote:
> >>
> >> For drivers that can transfer data to the TEE without using shared
> >> memory from client, it is necessary to receive the user address
> >> directly, bypassing any processing by the TEE subsystem. Introduce
> >> TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT/OUTPUT/INOUT to represent
> >> userspace buffers.
> >>
> >> Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com>
> >> ---
> >> drivers/tee/tee_core.c | 33 +++++++++++++++++++++++++++++++++
> >> include/linux/tee_drv.h | 6 ++++++
> >> include/uapi/linux/tee.h | 22 ++++++++++++++++------
> >> 3 files changed, 55 insertions(+), 6 deletions(-)
> >>
> >> Is this patch needed now that the QCOMTEE driver supports shared
> >> memory? I prefer keeping changes to the ABI to a minimum.
> >>
> >> Cheers,
> >> Jens
> >>
> >> Unfortunately, this is still required. QTEE supports two types of data transfer:
> >> (1) using UBUF and (2) memory objects. Even with memory object support, some APIs still
> >> expect to receive data using UBUF. For instance, to load a TA, QTEE offers two interfaces:
> >> one where the TA binary is in UBUF and another where the TA binary is in a memory object.
> >
> > Is this a limitation in the QTEE backend driver or on the secure side?
> > Can it be fixed? I don't ask for changes in the ABI to the secure
> > world since I assume you haven't made such changes while this patch
> > set has evolved.
> >
> > Cheers,
> > Jens
>
> The secure-side ABI supports passing data using memcpy to the same
> buffer that contains the message for QTEE, rather than using a memory
> object. Some services tend to use this approach for small data instead
> of allocating a memory object. I have no choice but to expose this support.
Got it, thanks! It's needed.
>
> Throughout the patchset, I have not made any change to the ABI but
> tried to provide support for the memory object in a separate,
> independent commit, distinct from the UBUF.
OK
Cheers,
Jens
>
> Best regards,
> Amir
>
> >
> >>
> >> Best Regards,
> >> Amir
> >>
> >> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> >> index 22cc7d624b0c..bc862a11d437 100644
> >> --- a/drivers/tee/tee_core.c
> >> +++ b/drivers/tee/tee_core.c
> >> @@ -404,6 +404,17 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params,
> >> params[n].u.value.b = ip.b;
> >> params[n].u.value.c = ip.c;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a);
> >> + params[n].u.ubuf.size = ip.b;
> >> +
> >> + if (!access_ok(params[n].u.ubuf.uaddr,
> >> + params[n].u.ubuf.size))
> >> + return -EFAULT;
> >> +
> >> + break;
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> >> @@ -472,6 +483,11 @@ static int params_to_user(struct tee_ioctl_param __user *uparams,
> >> put_user(p->u.value.c, &up->c))
> >> return -EFAULT;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + if (put_user((u64)p->u.ubuf.size, &up->b))
> >> + return -EFAULT;
> >> + break;
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> >> if (put_user((u64)p->u.memref.size, &up->b))
> >> @@ -672,6 +688,13 @@ static int params_to_supp(struct tee_context *ctx,
> >> ip.b = p->u.value.b;
> >> ip.c = p->u.value.c;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + ip.a = (u64)p->u.ubuf.uaddr;
> >> + ip.b = p->u.ubuf.size;
> >> + ip.c = 0;
> >> + break;
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> >> @@ -774,6 +797,16 @@ static int params_from_supp(struct tee_param *params, size_t num_params,
> >> p->u.value.b = ip.b;
> >> p->u.value.c = ip.c;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + p->u.ubuf.uaddr = u64_to_user_ptr(ip.a);
> >> + p->u.ubuf.size = ip.b;
> >> +
> >> + if (!access_ok(params[n].u.ubuf.uaddr,
> >> + params[n].u.ubuf.size))
> >> + return -EFAULT;
> >> +
> >> + break;
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> >> /*
> >> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> >> index ce23fd42c5d4..d773f91c6bdd 100644
> >> --- a/include/linux/tee_drv.h
> >> +++ b/include/linux/tee_drv.h
> >> @@ -82,6 +82,11 @@ struct tee_param_memref {
> >> struct tee_shm *shm;
> >> };
> >>
> >> +struct tee_param_ubuf {
> >> + void * __user uaddr;
> >> + size_t size;
> >> +};
> >> +
> >> struct tee_param_value {
> >> u64 a;
> >> u64 b;
> >> @@ -92,6 +97,7 @@ struct tee_param {
> >> u64 attr;
> >> union {
> >> struct tee_param_memref memref;
> >> + struct tee_param_ubuf ubuf;
> >> struct tee_param_value value;
> >> } u;
> >> };
> >> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> >> index d0430bee8292..3e9b1ec5dfde 100644
> >> --- a/include/uapi/linux/tee.h
> >> +++ b/include/uapi/linux/tee.h
> >> @@ -151,6 +151,13 @@ struct tee_ioctl_buf_data {
> >> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6
> >> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */
> >>
> >> +/*
> >> + * These defines userspace buffer parameters.
> >> + */
> >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT 8
> >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT 9
> >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT 10 /* input and output */
> >> +
> >> /*
> >> * Mask for the type part of the attribute, leaves room for more types
> >> */
> >> @@ -186,14 +193,17 @@ struct tee_ioctl_buf_data {
> >> /**
> >> * struct tee_ioctl_param - parameter
> >> * @attr: attributes
> >> - * @a: if a memref, offset into the shared memory object, else a value parameter
> >> - * @b: if a memref, size of the buffer, else a value parameter
> >> + * @a: if a memref, offset into the shared memory object,
> >> + * else if a ubuf, address of the user buffer,
> >> + * else a value parameter
> >> + * @b: if a memref or ubuf, size of the buffer, else a value parameter
> >> * @c: if a memref, shared memory identifier, else a value parameter
> >> *
> >> - * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in
> >> - * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and
> >> - * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE
> >> - * indicates that none of the members are used.
> >> + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref, ubuf, or value is
> >> + * used in the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value,
> >> + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref, and TEE_PARAM_ATTR_TYPE_UBUF_*
> >> + * indicates ubuf. TEE_PARAM_ATTR_TYPE_NONE indicates that none of the members
> >> + * are used.
> >> *
> >> * Shared memory is allocated with TEE_IOC_SHM_ALLOC which returns an
> >> * identifier representing the shared memory object. A memref can reference
> >>
> >> --
> >> 2.34.1
> >>
>
Hi Amirreza,
On Wed, Apr 9, 2025 at 2:28 AM Amirreza Zarrabi
<amirreza.zarrabi(a)oss.qualcomm.com> wrote:
>
> Hi jens,
>
> On 4/8/2025 10:19 PM, Jens Wiklander wrote:
>
> Hi Amirreza,
>
> On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi
> <amirreza.zarrabi(a)oss.qualcomm.com> wrote:
>
> For drivers that can transfer data to the TEE without using shared
> memory from client, it is necessary to receive the user address
> directly, bypassing any processing by the TEE subsystem. Introduce
> TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT/OUTPUT/INOUT to represent
> userspace buffers.
>
> Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com>
> ---
> drivers/tee/tee_core.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/tee_drv.h | 6 ++++++
> include/uapi/linux/tee.h | 22 ++++++++++++++++------
> 3 files changed, 55 insertions(+), 6 deletions(-)
>
> Is this patch needed now that the QCOMTEE driver supports shared
> memory? I prefer keeping changes to the ABI to a minimum.
>
> Cheers,
> Jens
>
> Unfortunately, this is still required. QTEE supports two types of data transfer:
> (1) using UBUF and (2) memory objects. Even with memory object support, some APIs still
> expect to receive data using UBUF. For instance, to load a TA, QTEE offers two interfaces:
> one where the TA binary is in UBUF and another where the TA binary is in a memory object.
Is this a limitation in the QTEE backend driver or on the secure side?
Can it be fixed? I don't ask for changes in the ABI to the secure
world since I assume you haven't made such changes while this patch
set has evolved.
Cheers,
Jens
>
> Best Regards,
> Amir
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 22cc7d624b0c..bc862a11d437 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -404,6 +404,17 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params,
> params[n].u.value.b = ip.b;
> params[n].u.value.c = ip.c;
> break;
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT:
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> + params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a);
> + params[n].u.ubuf.size = ip.b;
> +
> + if (!access_ok(params[n].u.ubuf.uaddr,
> + params[n].u.ubuf.size))
> + return -EFAULT;
> +
> + break;
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> @@ -472,6 +483,11 @@ static int params_to_user(struct tee_ioctl_param __user *uparams,
> put_user(p->u.value.c, &up->c))
> return -EFAULT;
> break;
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> + if (put_user((u64)p->u.ubuf.size, &up->b))
> + return -EFAULT;
> + break;
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> if (put_user((u64)p->u.memref.size, &up->b))
> @@ -672,6 +688,13 @@ static int params_to_supp(struct tee_context *ctx,
> ip.b = p->u.value.b;
> ip.c = p->u.value.c;
> break;
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT:
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> + ip.a = (u64)p->u.ubuf.uaddr;
> + ip.b = p->u.ubuf.size;
> + ip.c = 0;
> + break;
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> @@ -774,6 +797,16 @@ static int params_from_supp(struct tee_param *params, size_t num_params,
> p->u.value.b = ip.b;
> p->u.value.c = ip.c;
> break;
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> + p->u.ubuf.uaddr = u64_to_user_ptr(ip.a);
> + p->u.ubuf.size = ip.b;
> +
> + if (!access_ok(params[n].u.ubuf.uaddr,
> + params[n].u.ubuf.size))
> + return -EFAULT;
> +
> + break;
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> /*
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index ce23fd42c5d4..d773f91c6bdd 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -82,6 +82,11 @@ struct tee_param_memref {
> struct tee_shm *shm;
> };
>
> +struct tee_param_ubuf {
> + void * __user uaddr;
> + size_t size;
> +};
> +
> struct tee_param_value {
> u64 a;
> u64 b;
> @@ -92,6 +97,7 @@ struct tee_param {
> u64 attr;
> union {
> struct tee_param_memref memref;
> + struct tee_param_ubuf ubuf;
> struct tee_param_value value;
> } u;
> };
> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> index d0430bee8292..3e9b1ec5dfde 100644
> --- a/include/uapi/linux/tee.h
> +++ b/include/uapi/linux/tee.h
> @@ -151,6 +151,13 @@ struct tee_ioctl_buf_data {
> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6
> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */
>
> +/*
> + * These defines userspace buffer parameters.
> + */
> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT 8
> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT 9
> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT 10 /* input and output */
> +
> /*
> * Mask for the type part of the attribute, leaves room for more types
> */
> @@ -186,14 +193,17 @@ struct tee_ioctl_buf_data {
> /**
> * struct tee_ioctl_param - parameter
> * @attr: attributes
> - * @a: if a memref, offset into the shared memory object, else a value parameter
> - * @b: if a memref, size of the buffer, else a value parameter
> + * @a: if a memref, offset into the shared memory object,
> + * else if a ubuf, address of the user buffer,
> + * else a value parameter
> + * @b: if a memref or ubuf, size of the buffer, else a value parameter
> * @c: if a memref, shared memory identifier, else a value parameter
> *
> - * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in
> - * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and
> - * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE
> - * indicates that none of the members are used.
> + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref, ubuf, or value is
> + * used in the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value,
> + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref, and TEE_PARAM_ATTR_TYPE_UBUF_*
> + * indicates ubuf. TEE_PARAM_ATTR_TYPE_NONE indicates that none of the members
> + * are used.
> *
> * Shared memory is allocated with TEE_IOC_SHM_ALLOC which returns an
> * identifier representing the shared memory object. A memref can reference
>
> --
> 2.34.1
>
Hi,
Here's preliminary work to enable dmem tracking for heavy users of DMA
allocations on behalf of userspace: v4l2, DRM, and dma-buf heaps.
It's not really meant for inclusion at the moment, because I really
don't like it that much, and would like to discuss solutions on how to
make it nicer.
In particular, the dma dmem region accessors don't feel that great to
me. It duplicates the logic to select the proper accessor in
dma_alloc_attrs(), and it looks fragile and potentially buggy to me.
One solution I tried is to do the accounting in dma_alloc_attrs()
directly, depending on a flag being set, similar to what __GFP_ACCOUNT
is doing.
It didn't work because dmem initialises a state pointer when charging an
allocation to a region, and expects that state pointer to be passed back
when uncharging. Since dma_alloc_attrs() returns a void pointer to the
allocated buffer, we need to put that state into a higher-level
structure, such as drm_gem_object, or dma_buf.
Since we can't share the region selection logic, we need to get the
region through some other mean. Another thing I consider was to return
the region as part of the allocated buffer (through struct page or
folio), but those are lost across the calls and dma_alloc_attrs() will
only get a void pointer. So that's not doable without some heavy
rework, if it's a good idea at all.
So yeah, I went for the dumbest possible solution with the accessors,
hoping you could suggest a much smarter idea :)
Thanks,
Maxime
Signed-off-by: Maxime Ripard <mripard(a)kernel.org>
---
Maxime Ripard (12):
cma: Register dmem region for each cma region
cma: Provide accessor to cma dmem region
dma: coherent: Register dmem region for each coherent region
dma: coherent: Provide accessor to dmem region
dma: contiguous: Provide accessor to dmem region
dma: direct: Provide accessor to dmem region
dma: Create default dmem region for DMA allocations
dma: Provide accessor to dmem region
dma-buf: Clear cgroup accounting on release
dma-buf: cma: Account for allocations in dmem cgroup
drm/gem: Add cgroup memory accounting
media: videobuf2: Track buffer allocations through the dmem cgroup
drivers/dma-buf/dma-buf.c | 7 ++++
drivers/dma-buf/heaps/cma_heap.c | 18 ++++++++--
drivers/gpu/drm/drm_gem.c | 5 +++
drivers/gpu/drm/drm_gem_dma_helper.c | 6 ++++
.../media/common/videobuf2/videobuf2-dma-contig.c | 19 +++++++++++
include/drm/drm_device.h | 1 +
include/drm/drm_gem.h | 2 ++
include/linux/cma.h | 9 +++++
include/linux/dma-buf.h | 5 +++
include/linux/dma-direct.h | 2 ++
include/linux/dma-map-ops.h | 32 ++++++++++++++++++
include/linux/dma-mapping.h | 11 ++++++
kernel/dma/coherent.c | 26 +++++++++++++++
kernel/dma/direct.c | 8 +++++
kernel/dma/mapping.c | 39 ++++++++++++++++++++++
mm/cma.c | 21 +++++++++++-
mm/cma.h | 3 ++
17 files changed, 211 insertions(+), 3 deletions(-)
---
base-commit: 55a2aa61ba59c138bd956afe0376ec412a7004cf
change-id: 20250307-dmem-cgroups-73febced0989
Best regards,
--
Maxime Ripard <mripard(a)kernel.org>
Hi Amir,
On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi
<amirreza.zarrabi(a)oss.qualcomm.com> wrote:
>
> The tee_context can be used to manage TEE user resources, including
> those allocated by the driver for the TEE on behalf of the user.
> The release() callback is invoked only when all resources, such as
> tee_shm, are released and there are no references to the tee_context.
>
> When a user closes the device file, the driver should notify the
> TEE to release any resources it may hold and drop the context
> references. To achieve this, a close_context() callback is
> introduced to initiate resource release in the TEE driver when
> the device file is closed.
>
> Relocate teedev_ctx_get, teedev_ctx_put, tee_device_get, and
> tee_device_get functions to tee_drv.h to make them accessible
> outside the TEE subsystem.
>
> Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi(a)oss.qualcomm.com>
> ---
> drivers/tee/tee_core.c | 39 +++++++++++++++++++++++++++++++++++++++
> drivers/tee/tee_private.h | 6 ------
> include/linux/tee_core.h | 11 +++++++++--
> include/linux/tee_drv.h | 40 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 88 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 24edce4cdbaa..22cc7d624b0c 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -72,6 +72,20 @@ struct tee_context *teedev_open(struct tee_device *teedev)
> }
> EXPORT_SYMBOL_GPL(teedev_open);
>
> +/**
> + * teedev_ctx_get() - Increment the reference count of a context
> + *
> + * This function increases the refcount of the context, which is tied to
> + * resources shared by the same tee_device. During the unregistration process,
> + * the context may remain valid even after tee_device_unregister() has returned.
> + *
> + * Users should ensure that the context's refcount is properly decreased before
> + * calling tee_device_put(), typically within the context's release() function.
> + * Alternatively, users can call tee_device_get() and teedev_ctx_get() together
> + * and release them simultaneously (see shm_alloc_helper()).
> + *
> + * @ctx: Pointer to the context
Please move this @ctx line to before the verbose description of the function.
Cheers,
Jens
> + */
> void teedev_ctx_get(struct tee_context *ctx)
> {
> if (ctx->releasing)
> @@ -79,6 +93,7 @@ void teedev_ctx_get(struct tee_context *ctx)
>
> kref_get(&ctx->refcount);
> }
> +EXPORT_SYMBOL_GPL(teedev_ctx_get);
>
> static void teedev_ctx_release(struct kref *ref)
> {
> @@ -89,6 +104,10 @@ static void teedev_ctx_release(struct kref *ref)
> kfree(ctx);
> }
>
> +/**
> + * teedev_ctx_put() - Decrease reference count on a context
> + * @ctx: pointer to the context
> + */
> void teedev_ctx_put(struct tee_context *ctx)
> {
> if (ctx->releasing)
> @@ -96,11 +115,15 @@ void teedev_ctx_put(struct tee_context *ctx)
>
> kref_put(&ctx->refcount, teedev_ctx_release);
> }
> +EXPORT_SYMBOL_GPL(teedev_ctx_put);
>
> void teedev_close_context(struct tee_context *ctx)
> {
> struct tee_device *teedev = ctx->teedev;
>
> + if (teedev->desc->ops->close_context)
> + teedev->desc->ops->close_context(ctx);
> +
> teedev_ctx_put(ctx);
> tee_device_put(teedev);
> }
> @@ -1024,6 +1047,10 @@ int tee_device_register(struct tee_device *teedev)
> }
> EXPORT_SYMBOL_GPL(tee_device_register);
>
> +/**
> + * tee_device_put() - Decrease the user count for a tee_device
> + * @teedev: pointer to the tee_device
> + */
> void tee_device_put(struct tee_device *teedev)
> {
> mutex_lock(&teedev->mutex);
> @@ -1037,7 +1064,18 @@ void tee_device_put(struct tee_device *teedev)
> }
> mutex_unlock(&teedev->mutex);
> }
> +EXPORT_SYMBOL_GPL(tee_device_put);
>
> +/**
> + * tee_device_get() - Increment the user count for a tee_device
> + * @teedev: Pointer to the tee_device
> + *
> + * If tee_device_unregister() has been called and the final user of @teedev
> + * has already released the device, this function will fail to prevent new users
> + * from accessing the device during the unregistration process.
> + *
> + * Returns: true if @teedev remains valid, otherwise false
> + */
> bool tee_device_get(struct tee_device *teedev)
> {
> mutex_lock(&teedev->mutex);
> @@ -1049,6 +1087,7 @@ bool tee_device_get(struct tee_device *teedev)
> mutex_unlock(&teedev->mutex);
> return true;
> }
> +EXPORT_SYMBOL_GPL(tee_device_get);
>
> /**
> * tee_device_unregister() - Removes a TEE device
> diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
> index 9bc50605227c..d3f40a03de36 100644
> --- a/drivers/tee/tee_private.h
> +++ b/drivers/tee/tee_private.h
> @@ -14,12 +14,6 @@
>
> int tee_shm_get_fd(struct tee_shm *shm);
>
> -bool tee_device_get(struct tee_device *teedev);
> -void tee_device_put(struct tee_device *teedev);
> -
> -void teedev_ctx_get(struct tee_context *ctx);
> -void teedev_ctx_put(struct tee_context *ctx);
> -
> struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size);
> struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx,
> unsigned long addr, size_t length);
> diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h
> index a38494d6b5f4..8a4c9e30b652 100644
> --- a/include/linux/tee_core.h
> +++ b/include/linux/tee_core.h
> @@ -65,8 +65,9 @@ struct tee_device {
> /**
> * struct tee_driver_ops - driver operations vtable
> * @get_version: returns version of driver
> - * @open: called when the device file is opened
> - * @release: release this open file
> + * @open: called for a context when the device file is opened
> + * @close_context: called when the device file is closed
> + * @release: called to release the context
> * @open_session: open a new session
> * @close_session: close a session
> * @system_session: declare session as a system session
> @@ -76,11 +77,17 @@ struct tee_device {
> * @supp_send: called for supplicant to send a response
> * @shm_register: register shared memory buffer in TEE
> * @shm_unregister: unregister shared memory buffer in TEE
> + *
> + * The context given to @open might last longer than the device file if it is
> + * tied to other resources in the TEE driver. @close_context is called when the
> + * client closes the device file, even if there are existing references to the
> + * context. The TEE driver can use @close_context to start cleaning up.
> */
> struct tee_driver_ops {
> void (*get_version)(struct tee_device *teedev,
> struct tee_ioctl_version_data *vers);
> int (*open)(struct tee_context *ctx);
> + void (*close_context)(struct tee_context *ctx);
> void (*release)(struct tee_context *ctx);
> int (*open_session)(struct tee_context *ctx,
> struct tee_ioctl_open_session_arg *arg,
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index a54c203000ed..ce23fd42c5d4 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -96,6 +96,46 @@ struct tee_param {
> } u;
> };
>
> +/**
> + * tee_device_get() - Increment the user count for a tee_device
> + * @teedev: Pointer to the tee_device
> + *
> + * If tee_device_unregister() has been called and the final user of @teedev
> + * has already released the device, this function will fail to prevent new users
> + * from accessing the device during the unregistration process.
> + *
> + * Returns: true if @teedev remains valid, otherwise false
> + */
> +bool tee_device_get(struct tee_device *teedev);
> +
> +/**
> + * tee_device_put() - Decrease the user count for a tee_device
> + * @teedev: pointer to the tee_device
> + */
> +void tee_device_put(struct tee_device *teedev);
> +
> +/**
> + * teedev_ctx_get() - Increment the reference count of a context
> + *
> + * This function increases the refcount of the context, which is tied to
> + * resources shared by the same tee_device. During the unregistration process,
> + * the context may remain valid even after tee_device_unregister() has returned.
> + *
> + * Users should ensure that the context's refcount is properly decreased before
> + * calling tee_device_put(), typically within the context's release() function.
> + * Alternatively, users can call tee_device_get() and teedev_ctx_get() together
> + * and release them simultaneously (see shm_alloc_helper()).
> + *
> + * @ctx: Pointer to the context
> + */
> +void teedev_ctx_get(struct tee_context *ctx);
> +
> +/**
> + * teedev_ctx_put() - Decrease reference count on a context
> + * @ctx: pointer to the context
> + */
> +void teedev_ctx_put(struct tee_context *ctx);
> +
> /**
> * tee_shm_alloc_kernel_buf() - Allocate kernel shared memory for a
> * particular TEE client driver
>
> --
> 2.34.1
>