| Message ID | 20231009153427.20951-5-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On Mon, Oct 09, 2023 at 05:34:16PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We have several SCM calls that require passing buffers to the TrustZone > on top of the SMC core which allocates memory for calls that require > more than 4 arguments. > > Currently every user does their own thing which leads to code > duplication. Many users call dma_alloc_coherent() for every call which > is terribly unperformant (speed- and size-wise). > > Provide a set of library functions for creating and managing pool of > memory which is suitable for sharing with the TrustZone, that is: > page-aligned, contiguous and non-cachable as well as provides a way of > mapping of kernel virtual addresses to physical space. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/firmware/qcom/Kconfig | 19 ++ > drivers/firmware/qcom/Makefile | 1 + > drivers/firmware/qcom/qcom_tzmem.c | 301 +++++++++++++++++++++++ > drivers/firmware/qcom/qcom_tzmem.h | 13 + > include/linux/firmware/qcom/qcom_tzmem.h | 28 +++ > 5 files changed, 362 insertions(+) > create mode 100644 drivers/firmware/qcom/qcom_tzmem.c > create mode 100644 drivers/firmware/qcom/qcom_tzmem.h > create mode 100644 include/linux/firmware/qcom/qcom_tzmem.h > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > index 3f05d9854ddf..b80269a28224 100644 > --- a/drivers/firmware/qcom/Kconfig > +++ b/drivers/firmware/qcom/Kconfig > @@ -9,6 +9,25 @@ menu "Qualcomm firmware drivers" > config QCOM_SCM > tristate > > +config QCOM_TZMEM > + tristate > + > +choice > + prompt "TrustZone interface memory allocator mode" > + default QCOM_TZMEM_MODE_DEFAULT > + help > + Selects the mode of the memory allocator providing memory buffers of > + suitable format for sharing with the TrustZone. If in doubt, select > + 'Default'. > + > +config QCOM_TZMEM_MODE_DEFAULT > + bool "Default" > + help > + Use the default allocator mode. The memory is page-aligned, non-cachable > + and contiguous. > + > +endchoice > + > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > bool "Qualcomm download mode enabled by default" > depends on QCOM_SCM > diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile > index c9f12ee8224a..0be40a1abc13 100644 > --- a/drivers/firmware/qcom/Makefile > +++ b/drivers/firmware/qcom/Makefile > @@ -5,5 +5,6 @@ > > obj-$(CONFIG_QCOM_SCM) += qcom-scm.o > qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o > +obj-$(CONFIG_QCOM_TZMEM) += qcom_tzmem.o > obj-$(CONFIG_QCOM_QSEECOM) += qcom_qseecom.o > obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > new file mode 100644 > index 000000000000..eee51fed756e > --- /dev/null > +++ b/drivers/firmware/qcom/qcom_tzmem.c > @@ -0,0 +1,301 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Memory allocator for buffers shared with the TrustZone. > + * > + * Copyright (C) 2023 Linaro Ltd. > + */ > + > +#include <linux/bug.h> > +#include <linux/cleanup.h> > +#include <linux/dma-mapping.h> > +#include <linux/err.h> > +#include <linux/firmware/qcom/qcom_tzmem.h> > +#include <linux/genalloc.h> > +#include <linux/gfp.h> > +#include <linux/mm.h> > +#include <linux/radix-tree.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> > + > +#include "qcom_tzmem.h" > + > +struct qcom_tzmem_pool { > + void *vbase; > + phys_addr_t pbase; > + size_t size; > + struct gen_pool *pool; > + void *priv; > +}; > + > +struct qcom_tzmem_chunk { > + phys_addr_t paddr; > + size_t size; > + struct qcom_tzmem_pool *owner; > +}; > + > +static struct device *qcom_tzmem_dev; > +static RADIX_TREE(qcom_tzmem_chunks, GFP_ATOMIC); > +static DEFINE_SPINLOCK(qcom_tzmem_chunks_lock); > + > +#if IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_DEFAULT) > + > +static int qcom_tzmem_init(void) > +{ > + return 0; > +} > + > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > +{ > + return 0; > +} > + > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > +{ > + > +} > + > +#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > + > +/** > + * qcom_tzmem_pool_new() - Create a new TZ memory pool. > + * @size - Size of the new pool in bytes. > + * > + * Create a new pool of memory suitable for sharing with the TrustZone. > + * > + * Must not be used in atomic context. > + * > + * Returns: > + * New memory pool address or ERR_PTR() on error. > + */ > +struct qcom_tzmem_pool *qcom_tzmem_pool_new(size_t size) > +{ > + struct qcom_tzmem_pool *pool; > + int ret = -ENOMEM; > + > + if (!size) > + return ERR_PTR(-EINVAL); > + > + size = PAGE_ALIGN(size); > + > + pool = kzalloc(sizeof(*pool), GFP_KERNEL); > + if (!pool) > + return ERR_PTR(-ENOMEM); > + > + pool->size = size; > + > + pool->vbase = dma_alloc_coherent(qcom_tzmem_dev, size, &pool->pbase, > + GFP_KERNEL); > + if (!pool->vbase) > + goto err_kfree_pool; > + > + pool->pool = gen_pool_create(PAGE_SHIFT, -1); > + if (!pool) > + goto err_dma_free; > + > + gen_pool_set_algo(pool->pool, gen_pool_best_fit, NULL); > + > + ret = gen_pool_add_virt(pool->pool, (unsigned long)pool->vbase, > + pool->pbase, size, -1); > + if (ret) > + goto err_destroy_genpool; > + > + ret = qcom_tzmem_init_pool(pool); > + if (ret) > + goto err_destroy_genpool; > + > + return pool; > + > +err_destroy_genpool: > + gen_pool_destroy(pool->pool); > +err_dma_free: > + dma_free_coherent(qcom_tzmem_dev, size, pool->vbase, pool->pbase); > +err_kfree_pool: > + kfree(pool); > + return ERR_PTR(ret); > +} > +EXPORT_SYMBOL_GPL(qcom_tzmem_pool_new); > + > +/** > + * qcom_tzmem_pool_free() - Destroy a TZ memory pool and free all resources. > + * @pool: Memory pool to free. > + * > + * Must not be called if any of the allocated chunks has not been freed. > + * Must not be used in atomic context. > + */ > +void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > +{ > + struct qcom_tzmem_chunk *chunk; > + struct radix_tree_iter iter; > + bool non_empty = false; > + void **slot; > + > + if (!pool) > + return; > + > + qcom_tzmem_cleanup_pool(pool); > + > + scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { > + radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { > + chunk = *slot; > + > + if (chunk->owner == pool) > + non_empty = true; > + } > + } > + > + WARN(non_empty, "Freeing TZ memory pool with memory still allocated"); > + > + gen_pool_destroy(pool->pool); > + dma_free_coherent(qcom_tzmem_dev, pool->size, pool->vbase, pool->pbase); > + kfree(pool); > +} > +EXPORT_SYMBOL_GPL(qcom_tzmem_pool_free); I got these warnings with this series: ahalaney@fedora ~/git/linux-next (git)-[7204cc6c3d73] % ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make W=1 C=2 drivers/firmware/qcom/ drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter or member 'size' not described in 'qcom_tzmem_pool_new' CHECK drivers/firmware/qcom/qcom_tzmem.c drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in argument 1 (different address spaces) drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void [noderef] __rcu **slot drivers/firmware/qcom/qcom_tzmem.c:204:17: got void **slot drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance in 'qcom_tzmem_to_phys' - wrong count at exit All are confusing me, size seems described, I don't know much about radix tree usage / rcu, and the locking in qcom_tzmem_to_phys seems sane to me but I'm still grappling with the new syntax. For the one address space one, I _think_ maybe a diff like this is in order? diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c index b3137844fe43..5b409615198d 100644 --- a/drivers/firmware/qcom/qcom_tzmem.c +++ b/drivers/firmware/qcom/qcom_tzmem.c @@ -193,7 +193,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) struct qcom_tzmem_chunk *chunk; struct radix_tree_iter iter; bool non_empty = false; - void **slot; + void __rcu **slot; if (!pool) return; @@ -202,7 +202,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { - chunk = *slot; + chunk = radix_tree_deref_slot_protected(slot, &qcom_tzmem_chunks_lock); if (chunk->owner == pool) non_empty = true; Still planning on reviewing/testing the rest, but got tripped up there so thought I'd highlight it before doing the rest. Thanks, Andrew
On Mon, Oct 9, 2023 at 11:28 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Mon, Oct 09, 2023 at 05:34:16PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > We have several SCM calls that require passing buffers to the TrustZone > > on top of the SMC core which allocates memory for calls that require > > more than 4 arguments. > > > > Currently every user does their own thing which leads to code > > duplication. Many users call dma_alloc_coherent() for every call which > > is terribly unperformant (speed- and size-wise). > > > > Provide a set of library functions for creating and managing pool of > > memory which is suitable for sharing with the TrustZone, that is: > > page-aligned, contiguous and non-cachable as well as provides a way of > > mapping of kernel virtual addresses to physical space. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- [snip] > > I got these warnings with this series: > > ahalaney@fedora ~/git/linux-next (git)-[7204cc6c3d73] % ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make W=1 C=2 drivers/firmware/qcom/ > drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter or member 'size' not described in 'qcom_tzmem_pool_new' > CHECK drivers/firmware/qcom/qcom_tzmem.c > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in argument 1 (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void [noderef] __rcu **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance in 'qcom_tzmem_to_phys' - wrong count at exit > > > All are confusing me, size seems described, I don't know much about > radix tree usage / rcu, and the locking in qcom_tzmem_to_phys seems sane > to me but I'm still grappling with the new syntax. > > For the one address space one, I _think_ maybe a diff like this is in > order? > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > index b3137844fe43..5b409615198d 100644 > --- a/drivers/firmware/qcom/qcom_tzmem.c > +++ b/drivers/firmware/qcom/qcom_tzmem.c > @@ -193,7 +193,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > struct qcom_tzmem_chunk *chunk; > struct radix_tree_iter iter; > bool non_empty = false; > - void **slot; > + void __rcu **slot; > > if (!pool) > return; > @@ -202,7 +202,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > > scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { > radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { > - chunk = *slot; > + chunk = radix_tree_deref_slot_protected(slot, &qcom_tzmem_chunks_lock); > > if (chunk->owner == pool) > non_empty = true; > Ah, I was thinking about it but then figured that I already use a spinlock and I didn't see these errors on my side so decided to dereference it normally. I'll check it again. Bart > > Still planning on reviewing/testing the rest, but got tripped up there > so thought I'd highlight it before doing the rest. > > Thanks, > Andrew >
On Mon, Oct 9, 2023 at 11:28 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Mon, Oct 09, 2023 at 05:34:16PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > We have several SCM calls that require passing buffers to the TrustZone > > on top of the SMC core which allocates memory for calls that require > > more than 4 arguments. > > > > Currently every user does their own thing which leads to code > > duplication. Many users call dma_alloc_coherent() for every call which > > is terribly unperformant (speed- and size-wise). > > > > Provide a set of library functions for creating and managing pool of > > memory which is suitable for sharing with the TrustZone, that is: > > page-aligned, contiguous and non-cachable as well as provides a way of > > mapping of kernel virtual addresses to physical space. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- [snip] > > I got these warnings with this series: > > ahalaney@fedora ~/git/linux-next (git)-[7204cc6c3d73] % ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make W=1 C=2 drivers/firmware/qcom/ > drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter or member 'size' not described in 'qcom_tzmem_pool_new' > CHECK drivers/firmware/qcom/qcom_tzmem.c > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in argument 1 (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void [noderef] __rcu **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance in 'qcom_tzmem_to_phys' - wrong count at exit I fixed the other ones but this one I think comes from CHECK not dealing correctly with the spinlock guard. > > > All are confusing me, size seems described, I don't know much about > radix tree usage / rcu, and the locking in qcom_tzmem_to_phys seems sane > to me but I'm still grappling with the new syntax. > > For the one address space one, I _think_ maybe a diff like this is in > order? > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > index b3137844fe43..5b409615198d 100644 > --- a/drivers/firmware/qcom/qcom_tzmem.c > +++ b/drivers/firmware/qcom/qcom_tzmem.c > @@ -193,7 +193,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > struct qcom_tzmem_chunk *chunk; > struct radix_tree_iter iter; > bool non_empty = false; > - void **slot; > + void __rcu **slot; > > if (!pool) > return; > @@ -202,7 +202,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > > scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { > radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { > - chunk = *slot; > + chunk = radix_tree_deref_slot_protected(slot, &qcom_tzmem_chunks_lock); We need to keep the lock taken for the duration of the looping so we can use the regular radix_tree_deref_slot(). Bart > > if (chunk->owner == pool) > non_empty = true; > > > Still planning on reviewing/testing the rest, but got tripped up there > so thought I'd highlight it before doing the rest. > > Thanks, > Andrew >
On Tue, Oct 10, 2023 at 10:26:34AM +0200, Bartosz Golaszewski wrote: > On Mon, Oct 9, 2023 at 11:28 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > On Mon, Oct 09, 2023 at 05:34:16PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > We have several SCM calls that require passing buffers to the TrustZone > > > on top of the SMC core which allocates memory for calls that require > > > more than 4 arguments. > > > > > > Currently every user does their own thing which leads to code > > > duplication. Many users call dma_alloc_coherent() for every call which > > > is terribly unperformant (speed- and size-wise). > > > > > > Provide a set of library functions for creating and managing pool of > > > memory which is suitable for sharing with the TrustZone, that is: > > > page-aligned, contiguous and non-cachable as well as provides a way of > > > mapping of kernel virtual addresses to physical space. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > [snip] > > > > > I got these warnings with this series: > > > > ahalaney@fedora ~/git/linux-next (git)-[7204cc6c3d73] % ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make W=1 C=2 drivers/firmware/qcom/ > > drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter or member 'size' not described in 'qcom_tzmem_pool_new' > > CHECK drivers/firmware/qcom/qcom_tzmem.c > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in argument 1 (different address spaces) > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void [noderef] __rcu **slot > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void **slot > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > > drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance in 'qcom_tzmem_to_phys' - wrong count at exit > > I fixed the other ones but this one I think comes from CHECK not > dealing correctly with the spinlock guard. > > > > > > > All are confusing me, size seems described, I don't know much about > > radix tree usage / rcu, and the locking in qcom_tzmem_to_phys seems sane > > to me but I'm still grappling with the new syntax. > > > > For the one address space one, I _think_ maybe a diff like this is in > > order? > > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > > index b3137844fe43..5b409615198d 100644 > > --- a/drivers/firmware/qcom/qcom_tzmem.c > > +++ b/drivers/firmware/qcom/qcom_tzmem.c > > @@ -193,7 +193,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > > struct qcom_tzmem_chunk *chunk; > > struct radix_tree_iter iter; > > bool non_empty = false; > > - void **slot; > > + void __rcu **slot; > > > > if (!pool) > > return; > > @@ -202,7 +202,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > > > > scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { > > radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { > > - chunk = *slot; > > + chunk = radix_tree_deref_slot_protected(slot, &qcom_tzmem_chunks_lock); > > We need to keep the lock taken for the duration of the looping so we > can use the regular radix_tree_deref_slot(). IIUC, using the protected version is preferable since you already have the lock in hand: https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#id2 Quote: The variant rcu_dereference_protected() can be used outside of an RCU read-side critical section as long as the usage is protected by locks acquired by the update-side code. This variant avoids the lockdep warning that would happen when using (for example) rcu_dereference() without rcu_read_lock() protection. Using rcu_dereference_protected() also has the advantage of permitting compiler optimizations that rcu_dereference() must prohibit. The rcu_dereference_protected() variant takes a lockdep expression to indicate which locks must be acquired by the caller. If the indicated protection is not provided, a lockdep splat is emitted. Thanks, Andrew > > Bart > > > > > if (chunk->owner == pool) > > non_empty = true; > > > > > > Still planning on reviewing/testing the rest, but got tripped up there > > so thought I'd highlight it before doing the rest. > > > > Thanks, > > Andrew > > >
On Tue, Oct 10, 2023 at 10:48 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Tue, Oct 10, 2023 at 10:26:34AM +0200, Bartosz Golaszewski wrote: > > On Mon, Oct 9, 2023 at 11:28 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > On Mon, Oct 09, 2023 at 05:34:16PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > We have several SCM calls that require passing buffers to the TrustZone > > > > on top of the SMC core which allocates memory for calls that require > > > > more than 4 arguments. > > > > > > > > Currently every user does their own thing which leads to code > > > > duplication. Many users call dma_alloc_coherent() for every call which > > > > is terribly unperformant (speed- and size-wise). > > > > > > > > Provide a set of library functions for creating and managing pool of > > > > memory which is suitable for sharing with the TrustZone, that is: > > > > page-aligned, contiguous and non-cachable as well as provides a way of > > > > mapping of kernel virtual addresses to physical space. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > --- > > > > [snip] > > > > > > > > I got these warnings with this series: > > > > > > ahalaney@fedora ~/git/linux-next (git)-[7204cc6c3d73] % ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make W=1 C=2 drivers/firmware/qcom/ > > > drivers/firmware/qcom/qcom_tzmem.c:137: warning: Function parameter or member 'size' not described in 'qcom_tzmem_pool_new' > > > CHECK drivers/firmware/qcom/qcom_tzmem.c > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in argument 1 (different address spaces) > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void [noderef] __rcu **slot > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void **slot > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: warning: incorrect type in assignment (different address spaces) > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: expected void **slot > > > drivers/firmware/qcom/qcom_tzmem.c:204:17: got void [noderef] __rcu ** > > > drivers/firmware/qcom/qcom_tzmem.c:339:13: warning: context imbalance in 'qcom_tzmem_to_phys' - wrong count at exit > > > > I fixed the other ones but this one I think comes from CHECK not > > dealing correctly with the spinlock guard. > > > > > > > > > > > All are confusing me, size seems described, I don't know much about > > > radix tree usage / rcu, and the locking in qcom_tzmem_to_phys seems sane > > > to me but I'm still grappling with the new syntax. > > > > > > For the one address space one, I _think_ maybe a diff like this is in > > > order? > > > > > > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > > > index b3137844fe43..5b409615198d 100644 > > > --- a/drivers/firmware/qcom/qcom_tzmem.c > > > +++ b/drivers/firmware/qcom/qcom_tzmem.c > > > @@ -193,7 +193,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > > > struct qcom_tzmem_chunk *chunk; > > > struct radix_tree_iter iter; > > > bool non_empty = false; > > > - void **slot; > > > + void __rcu **slot; > > > > > > if (!pool) > > > return; > > > @@ -202,7 +202,7 @@ void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) > > > > > > scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { > > > radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { > > > - chunk = *slot; > > > + chunk = radix_tree_deref_slot_protected(slot, &qcom_tzmem_chunks_lock); > > > > We need to keep the lock taken for the duration of the looping so we > > can use the regular radix_tree_deref_slot(). > > IIUC, using the protected version is preferable since you already > have the lock in hand: https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#id2 > > Quote: > The variant rcu_dereference_protected() can be used outside of an RCU > read-side critical section as long as the usage is protected by locks > acquired by the update-side code. This variant avoids the lockdep warning > that would happen when using (for example) rcu_dereference() without > rcu_read_lock() protection. Using rcu_dereference_protected() also has > the advantage of permitting compiler optimizations that rcu_dereference() > must prohibit. The rcu_dereference_protected() variant takes a lockdep > expression to indicate which locks must be acquired by the caller. > If the indicated protection is not provided, a lockdep splat is emitted. > > Thanks, > Andrew I should have RTFM I guess. I assumed that the _protected() variant just takes the indicated lock. Thanks Bart > > > > > > Bart > > > > > > > > if (chunk->owner == pool) > > > non_empty = true; > > > > > > > > > Still planning on reviewing/testing the rest, but got tripped up there > > > so thought I'd highlight it before doing the rest. > > > > > > Thanks, > > > Andrew > > > > > >
diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig index 3f05d9854ddf..b80269a28224 100644 --- a/drivers/firmware/qcom/Kconfig +++ b/drivers/firmware/qcom/Kconfig @@ -9,6 +9,25 @@ menu "Qualcomm firmware drivers" config QCOM_SCM tristate +config QCOM_TZMEM + tristate + +choice + prompt "TrustZone interface memory allocator mode" + default QCOM_TZMEM_MODE_DEFAULT + help + Selects the mode of the memory allocator providing memory buffers of + suitable format for sharing with the TrustZone. If in doubt, select + 'Default'. + +config QCOM_TZMEM_MODE_DEFAULT + bool "Default" + help + Use the default allocator mode. The memory is page-aligned, non-cachable + and contiguous. + +endchoice + config QCOM_SCM_DOWNLOAD_MODE_DEFAULT bool "Qualcomm download mode enabled by default" depends on QCOM_SCM diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile index c9f12ee8224a..0be40a1abc13 100644 --- a/drivers/firmware/qcom/Makefile +++ b/drivers/firmware/qcom/Makefile @@ -5,5 +5,6 @@ obj-$(CONFIG_QCOM_SCM) += qcom-scm.o qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o +obj-$(CONFIG_QCOM_TZMEM) += qcom_tzmem.o obj-$(CONFIG_QCOM_QSEECOM) += qcom_qseecom.o obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c new file mode 100644 index 000000000000..eee51fed756e --- /dev/null +++ b/drivers/firmware/qcom/qcom_tzmem.c @@ -0,0 +1,301 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Memory allocator for buffers shared with the TrustZone. + * + * Copyright (C) 2023 Linaro Ltd. + */ + +#include <linux/bug.h> +#include <linux/cleanup.h> +#include <linux/dma-mapping.h> +#include <linux/err.h> +#include <linux/firmware/qcom/qcom_tzmem.h> +#include <linux/genalloc.h> +#include <linux/gfp.h> +#include <linux/mm.h> +#include <linux/radix-tree.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/types.h> + +#include "qcom_tzmem.h" + +struct qcom_tzmem_pool { + void *vbase; + phys_addr_t pbase; + size_t size; + struct gen_pool *pool; + void *priv; +}; + +struct qcom_tzmem_chunk { + phys_addr_t paddr; + size_t size; + struct qcom_tzmem_pool *owner; +}; + +static struct device *qcom_tzmem_dev; +static RADIX_TREE(qcom_tzmem_chunks, GFP_ATOMIC); +static DEFINE_SPINLOCK(qcom_tzmem_chunks_lock); + +#if IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_DEFAULT) + +static int qcom_tzmem_init(void) +{ + return 0; +} + +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) +{ + return 0; +} + +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) +{ + +} + +#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ + +/** + * qcom_tzmem_pool_new() - Create a new TZ memory pool. + * @size - Size of the new pool in bytes. + * + * Create a new pool of memory suitable for sharing with the TrustZone. + * + * Must not be used in atomic context. + * + * Returns: + * New memory pool address or ERR_PTR() on error. + */ +struct qcom_tzmem_pool *qcom_tzmem_pool_new(size_t size) +{ + struct qcom_tzmem_pool *pool; + int ret = -ENOMEM; + + if (!size) + return ERR_PTR(-EINVAL); + + size = PAGE_ALIGN(size); + + pool = kzalloc(sizeof(*pool), GFP_KERNEL); + if (!pool) + return ERR_PTR(-ENOMEM); + + pool->size = size; + + pool->vbase = dma_alloc_coherent(qcom_tzmem_dev, size, &pool->pbase, + GFP_KERNEL); + if (!pool->vbase) + goto err_kfree_pool; + + pool->pool = gen_pool_create(PAGE_SHIFT, -1); + if (!pool) + goto err_dma_free; + + gen_pool_set_algo(pool->pool, gen_pool_best_fit, NULL); + + ret = gen_pool_add_virt(pool->pool, (unsigned long)pool->vbase, + pool->pbase, size, -1); + if (ret) + goto err_destroy_genpool; + + ret = qcom_tzmem_init_pool(pool); + if (ret) + goto err_destroy_genpool; + + return pool; + +err_destroy_genpool: + gen_pool_destroy(pool->pool); +err_dma_free: + dma_free_coherent(qcom_tzmem_dev, size, pool->vbase, pool->pbase); +err_kfree_pool: + kfree(pool); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(qcom_tzmem_pool_new); + +/** + * qcom_tzmem_pool_free() - Destroy a TZ memory pool and free all resources. + * @pool: Memory pool to free. + * + * Must not be called if any of the allocated chunks has not been freed. + * Must not be used in atomic context. + */ +void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool) +{ + struct qcom_tzmem_chunk *chunk; + struct radix_tree_iter iter; + bool non_empty = false; + void **slot; + + if (!pool) + return; + + qcom_tzmem_cleanup_pool(pool); + + scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { + radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) { + chunk = *slot; + + if (chunk->owner == pool) + non_empty = true; + } + } + + WARN(non_empty, "Freeing TZ memory pool with memory still allocated"); + + gen_pool_destroy(pool->pool); + dma_free_coherent(qcom_tzmem_dev, pool->size, pool->vbase, pool->pbase); + kfree(pool); +} +EXPORT_SYMBOL_GPL(qcom_tzmem_pool_free); + +static void devm_qcom_tzmem_pool_free(void *data) +{ + struct qcom_tzmem_pool *pool = data; + + qcom_tzmem_pool_free(pool); +} + +/** + * devm_qcom_tzmem_pool_new() - Managed variant of qcom_tzmem_pool_new(). + * @dev: Device managing this resource. + * @size: Size of the pool in bytes. + * + * Must not be used in atomic context. + * + * Returns: + * Address of the managed pool or ERR_PTR() on failure. + */ +struct qcom_tzmem_pool * +devm_qcom_tzmem_pool_new(struct device *dev, size_t size) +{ + struct qcom_tzmem_pool *pool; + int ret; + + pool = qcom_tzmem_pool_new(size); + if (IS_ERR(pool)) + return pool; + + ret = devm_add_action_or_reset(dev, devm_qcom_tzmem_pool_free, pool); + if (ret) + return ERR_PTR(ret); + + return pool; +} + +/** + * qcom_tzmem_alloc() - Allocate a memory chunk suitable for sharing with TZ. + * @pool: TZ memory pool from which to allocate memory. + * @size: Number of bytes to allocate. + * @gfp: GFP flags. + * + * Can be used in any context. + * + * Returns: + * Address of the allocated buffer or NULL if no more memory can be allocated. + * The buffer must be released using qcom_tzmem_free(). + */ +void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp) +{ + struct qcom_tzmem_chunk *chunk; + unsigned long vaddr; + int ret; + + if (!size) + return NULL; + + size = PAGE_ALIGN(size); + + chunk = kzalloc(sizeof(*chunk), gfp); + if (!chunk) + return NULL; + + vaddr = gen_pool_alloc(pool->pool, size); + if (!vaddr) { + kfree(chunk); + return NULL; + } + + chunk->paddr = gen_pool_virt_to_phys(pool->pool, vaddr); + chunk->size = size; + chunk->owner = pool; + + scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) { + ret = radix_tree_insert(&qcom_tzmem_chunks, vaddr, chunk); + if (ret) { + gen_pool_free(pool->pool, vaddr, size); + kfree(chunk); + return NULL; + } + } + + return (void *)vaddr; +} +EXPORT_SYMBOL_GPL(qcom_tzmem_alloc); + +/** + * qcom_tzmem_free() - Release a buffer allocated from a TZ memory pool. + * @vaddr: Virtual address of the buffer. + * + * Can be used in any context. + */ +void qcom_tzmem_free(void *vaddr) +{ + struct qcom_tzmem_chunk *chunk; + + scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) + chunk = radix_tree_delete_item(&qcom_tzmem_chunks, + (unsigned long)vaddr, NULL); + + if (!chunk) { + WARN(1, "Virtual address %p not owned by TZ memory allocator", + vaddr); + return; + } + + gen_pool_free(chunk->owner->pool, (unsigned long)vaddr, chunk->size); + kfree(chunk); +} +EXPORT_SYMBOL_GPL(qcom_tzmem_free); + +/** + * qcom_tzmem_to_phys() - Map the virtual address of a TZ buffer to physical. + * @vaddr: Virtual address of the buffer allocated from a TZ memory pool. + * + * Can be used in any context. The address must have been returned by a call + * to qcom_tzmem_alloc(). + * + * Returns: + * Physical address of the buffer. + */ +phys_addr_t qcom_tzmem_to_phys(void *vaddr) +{ + struct qcom_tzmem_chunk *chunk; + + guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock); + + chunk = radix_tree_lookup(&qcom_tzmem_chunks, (unsigned long)vaddr); + if (!chunk) + return 0; + + return chunk->paddr; +} +EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys); + +int qcom_tzmem_enable(struct device *dev) +{ + if (qcom_tzmem_dev) + return -EBUSY; + + qcom_tzmem_dev = dev; + + return qcom_tzmem_init(); +} +EXPORT_SYMBOL_GPL(qcom_tzmem_enable); + +MODULE_DESCRIPTION("TrustZone memory allocator for Qualcomm firmware drivers"); +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/firmware/qcom/qcom_tzmem.h b/drivers/firmware/qcom/qcom_tzmem.h new file mode 100644 index 000000000000..f82f5dc5b7b1 --- /dev/null +++ b/drivers/firmware/qcom/qcom_tzmem.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2023 Linaro Ltd. + */ + +#ifndef __QCOM_TZMEM_PRIV_H +#define __QCOM_TZMEM_PRIV_H + +struct device; + +int qcom_tzmem_enable(struct device *dev); + +#endif /* __QCOM_TZMEM_PRIV_H */ diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h new file mode 100644 index 000000000000..8e7fddab8cb4 --- /dev/null +++ b/include/linux/firmware/qcom/qcom_tzmem.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2023 Linaro Ltd. + */ + +#ifndef __QCOM_TZMEM_H +#define __QCOM_TZMEM_H + +#include <linux/cleanup.h> +#include <linux/gfp.h> +#include <linux/types.h> + +struct device; +struct qcom_tzmem_pool; + +struct qcom_tzmem_pool *qcom_tzmem_pool_new(size_t size); +void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool); +struct qcom_tzmem_pool * +devm_qcom_tzmem_pool_new(struct device *dev, size_t size); + +void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp); +void qcom_tzmem_free(void *ptr); + +DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T)); + +phys_addr_t qcom_tzmem_to_phys(void *ptr); + +#endif /* __QCOM_TZMEM */