On Mon, May 26, 2025 at 10:09 AM Sumit Garg <sumit.garg(a)kernel.org> wrote:
>
> On Tue, May 20, 2025 at 05:16:51PM +0200, Jens Wiklander wrote:
> > Add support in the OP-TEE backend driver dynamic protected memory
> > allocation with FF-A.
> >
> > The protected memory pools for dynamically allocated protected memory
> > are instantiated when requested by user-space. This instantiation can
> > fail if OP-TEE doesn't support the requested use-case of protected
> > 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>
> > ---
[...]
> > +static int optee_ffa_protmem_pool_init(struct optee *optee, u32 sec_caps)
> > +{
> > + enum tee_dma_heap_id id = TEE_DMA_HEAP_SECURE_VIDEO_PLAY;
> > + struct tee_protmem_pool *pool;
> > + int rc = 0;
> > +
> > + if (sec_caps & OPTEE_FFA_SEC_CAP_PROTMEM) {
> > + pool = optee_protmem_alloc_dyn_pool(optee, id);
> > + if (IS_ERR(pool))
> > + return PTR_ERR(pool);
> > +
> > + rc = tee_device_register_dma_heap(optee->teedev, id, pool);
> > + if (rc)
> > + pool->ops->destroy_pool(pool);
> > + }
> > +
> > + return rc;
> > +}
> > +
> > static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > {
> > const struct ffa_notifier_ops *notif_ops;
> > @@ -918,7 +1057,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > optee);
> > if (IS_ERR(teedev)) {
> > rc = PTR_ERR(teedev);
> > - goto err_free_pool;
> > + goto err_free_shm_pool;
> > }
> > optee->teedev = teedev;
> >
> > @@ -965,6 +1104,9 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > rc);
> > }
> >
> > + if (optee_ffa_protmem_pool_init(optee, sec_caps))
>
> Let's add a Kconfig check for DMABUF heaps support here as well.
I prefer complaining in the log if there's something wrong with the
configuration.
>
> > + pr_info("Protected memory service not available\n");
> > +
[...]
> > +static int init_dyn_protmem(struct optee_protmem_dyn_pool *rp)
> > +{
> > + int rc;
> > +
> > + rp->protmem = tee_shm_alloc_dma_mem(rp->optee->ctx, rp->page_count);
> > + if (IS_ERR(rp->protmem)) {
> > + rc = PTR_ERR(rp->protmem);
> > + goto err_null_protmem;
> > + }
> > +
> > + /*
> > + * TODO unmap the memory range since the physical memory will
> > + * become inaccesible after the lend_protmem() call.
>
> Let's ellaborate this comment to also say that unmap isn't strictly
> needed here in case a platform supports hypervisor in EL2 which can
> perform unmapping as part for memory lending to secure world as that
> will avoid any cache pre-fetch of memory lent to secure world.
>
> With that I can live with this as a ToDo in kernel which can be
> implemented once we see platforms requiring this change to happen.
OK, I'll add something.
[...]
> > +
> > +struct tee_protmem_pool *optee_protmem_alloc_dyn_pool(struct optee *optee,
> > + enum tee_dma_heap_id id)
> > +{
> > + struct optee_protmem_dyn_pool *rp;
> > + u32 use_case = id;
>
> Here we can get rid of redundant extra variable with s/id/use_case/.
OK, I'll update.
Cheers,
Jens
Hi,
On Thu, May 22, 2025 at 12:14:18PM -0700, Jared Kangas wrote:
> The CMA heap's name in devtmpfs can vary depending on how the heap is
> defined. Its name defaults to "reserved", but if a CMA area is defined
> in the devicetree, the heap takes on the devicetree node's name, such as
> "default-pool" or "linux,cma". To simplify naming, unconditionally name
> it "default_cma_region", but keep a legacy node in place backed by the
> same underlying allocator for backwards compatibility.
>
> Signed-off-by: Jared Kangas <jkangas(a)redhat.com>
> ---
> Documentation/userspace-api/dma-buf-heaps.rst | 7 +++++--
> drivers/dma-buf/heaps/Kconfig | 10 ++++++++++
> drivers/dma-buf/heaps/cma_heap.c | 20 ++++++++++++++++++-
> 3 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst
> index 23bd0bd7b0654..1dfe5e7acd5a3 100644
> --- a/Documentation/userspace-api/dma-buf-heaps.rst
> +++ b/Documentation/userspace-api/dma-buf-heaps.rst
> @@ -21,5 +21,8 @@ following heaps:
> usually created either through the kernel commandline through the
> ``cma`` parameter, a memory region Device-Tree node with the
> ``linux,cma-default`` property set, or through the ``CMA_SIZE_MBYTES`` or
> - ``CMA_SIZE_PERCENTAGE`` Kconfig options. Depending on the platform, it
> - might be called ``reserved``, ``linux,cma``, or ``default-pool``.
> + ``CMA_SIZE_PERCENTAGE`` Kconfig options. The heap's name in devtmpfs is
> + ``default_cma_region``. For backwards compatibility, when the
> + ``DMABUF_HEAPS_CMA_LEGACY`` Kconfig option is set, a duplicate node is
> + created following legacy naming conventions; the legacy name might be
> + ``reserved``, ``linux,cma``, or ``default-pool``.
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c42264..bb369b38b001a 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -12,3 +12,13 @@ config DMABUF_HEAPS_CMA
> Choose this option to enable dma-buf CMA heap. This heap is backed
> by the Contiguous Memory Allocator (CMA). If your system has these
> regions, you should say Y here.
> +
> +config DMABUF_HEAPS_CMA_LEGACY
> + bool "Legacy DMA-BUF CMA Heap"
> + default y
> + depends on DMABUF_HEAPS_CMA
> + help
> + Add a duplicate CMA-backed dma-buf heap with legacy naming derived
> + from the CMA area's devicetree node, or "reserved" if the area is not
> + defined in the devicetree. This uses the same underlying allocator as
> + CONFIG_DMABUF_HEAPS_CMA.
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index e998d8ccd1dc6..dfeccafc6ae3c 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -9,6 +9,9 @@
> * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> * Andrew F. Davis <afd(a)ti.com>
> */
> +
> +#define pr_fmt(fmt) "cma_heap: " fmt
> +
> #include <linux/cma.h>
> #include <linux/dma-buf.h>
> #include <linux/dma-heap.h>
> @@ -22,6 +25,7 @@
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
>
> +#define DEFAULT_CMA_NAME "default_cma_region"
>
> struct cma_heap {
> struct dma_heap *heap;
> @@ -394,15 +398,29 @@ static int __init __add_cma_heap(struct cma *cma, const char *name)
> static int __init add_default_cma_heap(void)
> {
> struct cma *default_cma = dev_get_cma_area(NULL);
> + const char *legacy_cma_name;
> int ret;
>
> if (!default_cma)
> return 0;
>
> - ret = __add_cma_heap(default_cma, cma_get_name(default_cma));
> + ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME);
> if (ret)
> return ret;
>
> + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY)) {
> + legacy_cma_name = cma_get_name(default_cma);
> + if (!strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) {
> + pr_warn("legacy name and default name are the same, skipping legacy heap\n");
> + return 0;
> + }
> +
> + ret = __add_cma_heap(default_cma, legacy_cma_name);
> + if (ret)
> + pr_warn("failed to add legacy heap: %pe\n",
> + ERR_PTR(-ret));
Are you sure about the -ret? ret should already be a negative number if it failed?
With that fixed,
Reviewed-by: Maxime Ripard <mripard(a)kernel.org>
Maxime
On Thu, 22 May 2025 12:14:17 -0700, Jared Kangas wrote:
> Prepare for the introduction of a fixed-name CMA heap by replacing the
> unused void pointer parameter in __add_cma_heap() with the heap name.
>
> Signed-off-by: Jared Kangas <jkangas(a)redhat.com>
Reviewed-by: Maxime Ripard <mripard(a)kernel.org>
Thanks!
Maxime
On Thu, 22 May 2025 12:14:16 -0700, Jared Kangas wrote:
> Code snippets should be wrapped in double backticks to follow
> reStructuredText semantics; the use of single backticks uses the
> :title-reference: role by default, which isn't quite what we want.
> Add double backticks to code snippets to fix this.
>
>
> [ ... ]
Reviewed-by: Maxime Ripard <mripard(a)kernel.org>
Thanks!
Maxime