On Mon, May 26, 2025 at 10:09 AM Sumit Garg <sumit.g...@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.wiklan...@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
