| Message ID | 20231009153427.20951-1-brgl@bgdev.pl |
|---|---|
| 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 Mon, Oct 09, 2023 at 05:34:14PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We reference struct device in the private scm header but we neither > include linux/device.h nor forward declare it. Fix it. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
On Mon, Oct 09, 2023 at 05:34:15PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 'extern' specifiers do nothing for function declarations. Remove them > from the private qcom-scm header. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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 Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Let's use the new TZ memory allocator to obtain a buffer for this call > instead of using dma_alloc_coherent(). > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 71e98b666391..754f6056b99f 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/arm-smccc.h> > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/completion.h> > #include <linux/cpumask.h> > @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > struct qcom_scm_mem_map_info *mem_to_map; > phys_addr_t mem_to_map_phys; > phys_addr_t dest_phys; > - dma_addr_t ptr_phys; > + phys_addr_t ptr_phys; > size_t mem_to_map_sz; > size_t dest_sz; > size_t src_sz; > size_t ptr_sz; > int next_vm; > __le32 *src; > - void *ptr; nit: couldn't you keep this up here? Otherwise, Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > int ret, i, b; > u64 srcvm_bits = *srcvm; > > @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + > ALIGN(dest_sz, SZ_64); > > - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > + ptr_sz, GFP_KERNEL); > if (!ptr) > return -ENOMEM; > > + ptr_phys = qcom_tzmem_to_phys(ptr); > + > /* Fill source vmid detail */ > src = ptr; > i = 0; > @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, > ptr_phys, src_sz, dest_phys, dest_sz); > - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); > if (ret) { > dev_err(__scm->dev, > "Assign memory protection call failed %d\n", ret); > -- > 2.39.2 >
On Mon, Oct 09, 2023 at 05:34:21PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Let's use the new TZ memory allocator to obtain a buffer for this call > instead of using dma_alloc_coherent(). > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > --- > drivers/firmware/qcom/qcom_scm.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 31071a714cf1..11638daa2fe5 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -1340,8 +1340,6 @@ EXPORT_SYMBOL_GPL(qcom_scm_lmh_profile_change); > int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > u64 limit_node, u32 node_id, u64 version) > { > - dma_addr_t payload_phys; > - u32 *payload_buf; > int ret, payload_size = 5 * sizeof(u32); > > struct qcom_scm_desc desc = { > @@ -1356,7 +1354,9 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > .owner = ARM_SMCCC_OWNER_SIP, > }; > > - payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL); > + u32 *payload_buf __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > + payload_size, > + GFP_KERNEL); > if (!payload_buf) > return -ENOMEM; > > @@ -1366,11 +1366,10 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > payload_buf[3] = 1; > payload_buf[4] = payload_val; > > - desc.args[0] = payload_phys; > + desc.args[0] = qcom_tzmem_to_phys(payload_buf); > > ret = qcom_scm_call(__scm->dev, &desc, NULL); > > - dma_free_coherent(__scm->dev, payload_size, payload_buf, payload_phys); > return ret; > } > EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh); > -- > 2.39.2 >
On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and > convert all users of it in the qseecom module to using the TZ allocator > for creating SCM call buffers. Together with using the cleanup macros, > it has the added benefit of a significant code shrink. As this is > largely a module separate from the SCM driver, let's use a separate > memory pool. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> <snip> > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > return EFI_INVALID_PARAMETER; > > status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); > - if (status) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (status) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->length < sizeof(*rsp_data)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length < sizeof(*rsp_data)) > + return EFI_DEVICE_ERROR; > > if (rsp_data->status) { > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > if (efi_status == EFI_BUFFER_TOO_SMALL) > *name_size = rsp_data->name_size; > > - goto out_free; > + return efi_status; > } > > - if (rsp_data->length > rsp_size) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length > rsp_size) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) > + return EFI_DEVICE_ERROR; > > if (rsp_data->name_size > *name_size) { > *name_size = rsp_data->name_size; > - efi_status = EFI_BUFFER_TOO_SMALL; > - goto out_free; > + return EFI_BUFFER_TOO_SMALL; > } > > - if (rsp_data->guid_size != sizeof(*guid)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->guid_size != sizeof(*guid)) > + return EFI_DEVICE_ERROR; > > memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size); > status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset, > rsp_data->name_size / sizeof(*name)); > *name_size = rsp_data->name_size; > > - if (status < 0) { > + if (status < 0) > /* > * Return EFI_DEVICE_ERROR here because the buffer size should > * have already been validated above, causing this function to > * bail with EFI_BUFFER_TOO_SMALL. > */ > return EFI_DEVICE_ERROR; > - } Personally (no idea what the actual style guide says) leaving braces around the multiline if statement would be nice.... that being said, that's my opinion :) <snip> > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, > if (max_variable_size) > *max_variable_size = rsp_data->max_variable_size; > > -out_free: > - kfree(rsp_data); > -out_free_req: > - kfree(req_data); > -out: > - return efi_status; > + return EFI_SUCCESS; > } > > /* -- Global efivar interface. ---------------------------------------------- */ > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, > if (status) > qcuefi_set_reference(NULL); > > + qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K); Any particular reason for this size? Just curious, it was (one) of the reasons I had not marked patch 4 yet (it looks good, but I wanted to get through the series to digest the Kconfig as well). Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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 > > > > > >
On Wed, Oct 11, 2023 at 09:44:54AM +0200, Bartosz Golaszewski wrote: > On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and > > > convert all users of it in the qseecom module to using the TZ allocator > > > for creating SCM call buffers. Together with using the cleanup macros, > > > it has the added benefit of a significant code shrink. As this is > > > largely a module separate from the SCM driver, let's use a separate > > > memory pool. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > <snip> > > > > > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > > > return EFI_INVALID_PARAMETER; > > > > > > status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); > > > - if (status) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (status) > > > + return EFI_DEVICE_ERROR; > > > > > > - if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) > > > + return EFI_DEVICE_ERROR; > > > > > > - if (rsp_data->length < sizeof(*rsp_data)) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (rsp_data->length < sizeof(*rsp_data)) > > > + return EFI_DEVICE_ERROR; > > > > > > if (rsp_data->status) { > > > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > > > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > > > if (efi_status == EFI_BUFFER_TOO_SMALL) > > > *name_size = rsp_data->name_size; > > > > > > - goto out_free; > > > + return efi_status; > > > } > > > > > > - if (rsp_data->length > rsp_size) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (rsp_data->length > rsp_size) > > > + return EFI_DEVICE_ERROR; > > > > > > - if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) > > > + return EFI_DEVICE_ERROR; > > > > > > - if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) > > > + return EFI_DEVICE_ERROR; > > > > > > if (rsp_data->name_size > *name_size) { > > > *name_size = rsp_data->name_size; > > > - efi_status = EFI_BUFFER_TOO_SMALL; > > > - goto out_free; > > > + return EFI_BUFFER_TOO_SMALL; > > > } > > > > > > - if (rsp_data->guid_size != sizeof(*guid)) { > > > - efi_status = EFI_DEVICE_ERROR; > > > - goto out_free; > > > - } > > > + if (rsp_data->guid_size != sizeof(*guid)) > > > + return EFI_DEVICE_ERROR; > > > > > > memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size); > > > status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset, > > > rsp_data->name_size / sizeof(*name)); > > > *name_size = rsp_data->name_size; > > > > > > - if (status < 0) { > > > + if (status < 0) > > > /* > > > * Return EFI_DEVICE_ERROR here because the buffer size should > > > * have already been validated above, causing this function to > > > * bail with EFI_BUFFER_TOO_SMALL. > > > */ > > > return EFI_DEVICE_ERROR; > > > - } > > > > Personally (no idea what the actual style guide says) leaving braces > > around the multiline if statement would be nice.... that being said, > > that's my opinion :) > > > > <snip> > > > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, > > > if (max_variable_size) > > > *max_variable_size = rsp_data->max_variable_size; > > > > > > -out_free: > > > - kfree(rsp_data); > > > -out_free_req: > > > - kfree(req_data); > > > -out: > > > - return efi_status; > > > + return EFI_SUCCESS; > > > } > > > > > > /* -- Global efivar interface. ---------------------------------------------- */ > > > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, > > > if (status) > > > qcuefi_set_reference(NULL); > > > > > > + qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K); > > > > Any particular reason for this size? Just curious, it was (one) of the > > reasons I had not marked patch 4 yet (it looks good, but I wanted to get > > through the series to digest the Kconfig as well). > > > > I cannot test this. Do you know what the minimum correct size would be? I've got no insight into these firmware interfaces unfortunately. Was mostly curious if Qualcomm had provided a suggestion behind the scenes or if this was picked as a "sufficiently large" pool size. > > Bart > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > >
On Wed, Oct 11, 2023 at 3:56 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Wed, Oct 11, 2023 at 09:44:54AM +0200, Bartosz Golaszewski wrote: > > On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and > > > > convert all users of it in the qseecom module to using the TZ allocator > > > > for creating SCM call buffers. Together with using the cleanup macros, > > > > it has the added benefit of a significant code shrink. As this is > > > > largely a module separate from the SCM driver, let's use a separate > > > > memory pool. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > <snip> > > > > > > > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > > > > return EFI_INVALID_PARAMETER; > > > > > > > > status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); > > > > - if (status) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (status) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > - if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > - if (rsp_data->length < sizeof(*rsp_data)) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (rsp_data->length < sizeof(*rsp_data)) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > if (rsp_data->status) { > > > > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > > > > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > > > > if (efi_status == EFI_BUFFER_TOO_SMALL) > > > > *name_size = rsp_data->name_size; > > > > > > > > - goto out_free; > > > > + return efi_status; > > > > } > > > > > > > > - if (rsp_data->length > rsp_size) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (rsp_data->length > rsp_size) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > - if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > - if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > if (rsp_data->name_size > *name_size) { > > > > *name_size = rsp_data->name_size; > > > > - efi_status = EFI_BUFFER_TOO_SMALL; > > > > - goto out_free; > > > > + return EFI_BUFFER_TOO_SMALL; > > > > } > > > > > > > > - if (rsp_data->guid_size != sizeof(*guid)) { > > > > - efi_status = EFI_DEVICE_ERROR; > > > > - goto out_free; > > > > - } > > > > + if (rsp_data->guid_size != sizeof(*guid)) > > > > + return EFI_DEVICE_ERROR; > > > > > > > > memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size); > > > > status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset, > > > > rsp_data->name_size / sizeof(*name)); > > > > *name_size = rsp_data->name_size; > > > > > > > > - if (status < 0) { > > > > + if (status < 0) > > > > /* > > > > * Return EFI_DEVICE_ERROR here because the buffer size should > > > > * have already been validated above, causing this function to > > > > * bail with EFI_BUFFER_TOO_SMALL. > > > > */ > > > > return EFI_DEVICE_ERROR; > > > > - } > > > > > > Personally (no idea what the actual style guide says) leaving braces > > > around the multiline if statement would be nice.... that being said, > > > that's my opinion :) > > > > > > <snip> > > > > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, > > > > if (max_variable_size) > > > > *max_variable_size = rsp_data->max_variable_size; > > > > > > > > -out_free: > > > > - kfree(rsp_data); > > > > -out_free_req: > > > > - kfree(req_data); > > > > -out: > > > > - return efi_status; > > > > + return EFI_SUCCESS; > > > > } > > > > > > > > /* -- Global efivar interface. ---------------------------------------------- */ > > > > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, > > > > if (status) > > > > qcuefi_set_reference(NULL); > > > > > > > > + qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K); > > > > > > Any particular reason for this size? Just curious, it was (one) of the > > > reasons I had not marked patch 4 yet (it looks good, but I wanted to get > > > through the series to digest the Kconfig as well). > > > > > > > I cannot test this. Do you know what the minimum correct size would be? > > I've got no insight into these firmware interfaces unfortunately. Was > mostly curious if Qualcomm had provided a suggestion behind the scenes > or if this was picked as a "sufficiently large" pool size. > No, I chose a small but reasonable value and intend to see if it breaks anything. :) But if anyone from QCom reading knows a better value - be it smaller or larger, please let me know. Bartosz > > > > Bart > > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > > > >
On 10/9/23 17:34, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and > convert all users of it in the qseecom module to using the TZ allocator > for creating SCM call buffers. Together with using the cleanup macros, > it has the added benefit of a significant code shrink. As this is > largely a module separate from the SCM driver, let's use a separate > memory pool. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Tested-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > .../firmware/qcom/qcom_qseecom_uefisecapp.c | 260 +++++++----------- > drivers/firmware/qcom/qcom_scm.c | 30 +- > include/linux/firmware/qcom/qcom_qseecom.h | 4 +- > 3 files changed, 103 insertions(+), 191 deletions(-) > > diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c > index a33acdaf7b78..720cddd7c8c7 100644 > --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c > +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c > @@ -7,6 +7,7 @@ > * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com> > */ > > +#include <linux/cleanup.h> > #include <linux/efi.h> > #include <linux/kernel.h> > #include <linux/module.h> > @@ -18,6 +19,8 @@ > #include <linux/ucs2_string.h> > > #include <linux/firmware/qcom/qcom_qseecom.h> > +#include <linux/firmware/qcom/qcom_scm.h> > +#include <linux/firmware/qcom/qcom_tzmem.h> > > /* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */ > > @@ -253,6 +256,7 @@ struct qsee_rsp_uefi_query_variable_info { > struct qcuefi_client { > struct qseecom_client *client; > struct efivars efivars; > + struct qcom_tzmem_pool *mempool; > }; > > static struct device *qcuefi_dev(struct qcuefi_client *qcuefi) > @@ -272,11 +276,11 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e > const efi_guid_t *guid, u32 *attributes, > unsigned long *data_size, void *data) > { > - struct qsee_req_uefi_get_variable *req_data; > - struct qsee_rsp_uefi_get_variable *rsp_data; > + struct qsee_req_uefi_get_variable *req_data __free(qcom_tzmem) = NULL; > + struct qsee_rsp_uefi_get_variable *rsp_data __free(qcom_tzmem) = NULL; > unsigned long buffer_size = *data_size; > - efi_status_t efi_status = EFI_SUCCESS; > unsigned long name_length; > + efi_status_t efi_status; > size_t guid_offs; > size_t name_offs; > size_t req_size; > @@ -304,17 +308,13 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e > __array(u8, buffer_size) > ); > > - req_data = kzalloc(req_size, GFP_KERNEL); > - if (!req_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out; > - } > + req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL); > + if (!req_data) > + return EFI_OUT_OF_RESOURCES; > > - rsp_data = kzalloc(rsp_size, GFP_KERNEL); > - if (!rsp_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out_free_req; > - } > + rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL); > + if (!rsp_data) > + return EFI_OUT_OF_RESOURCES; > > req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE; > req_data->data_size = buffer_size; > @@ -331,20 +331,14 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e > memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size); > > status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); > - if (status) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (status) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->length < sizeof(*rsp_data)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length < sizeof(*rsp_data)) > + return EFI_DEVICE_ERROR; > > if (rsp_data->status) { > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > @@ -358,18 +352,14 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e > *attributes = rsp_data->attributes; > } > > - goto out_free; > + return efi_status; > } > > - if (rsp_data->length > rsp_size) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length > rsp_size) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) > + return EFI_DEVICE_ERROR; > > /* > * Note: We need to set attributes and data size even if the buffer is > @@ -392,33 +382,23 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e > if (attributes) > *attributes = rsp_data->attributes; > > - if (buffer_size == 0 && !data) { > - efi_status = EFI_SUCCESS; > - goto out_free; > - } > + if (buffer_size == 0 && !data) > + return EFI_SUCCESS; > > - if (buffer_size < rsp_data->data_size) { > - efi_status = EFI_BUFFER_TOO_SMALL; > - goto out_free; > - } > + if (buffer_size < rsp_data->data_size) > + return EFI_BUFFER_TOO_SMALL; > > memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size); > > -out_free: > - kfree(rsp_data); > -out_free_req: > - kfree(req_data); > -out: > - return efi_status; > + return EFI_SUCCESS; > } > > static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name, > const efi_guid_t *guid, u32 attributes, > unsigned long data_size, const void *data) > { > - struct qsee_req_uefi_set_variable *req_data; > - struct qsee_rsp_uefi_set_variable *rsp_data; > - efi_status_t efi_status = EFI_SUCCESS; > + struct qsee_req_uefi_set_variable *req_data __free(qcom_tzmem) = NULL; > + struct qsee_rsp_uefi_set_variable *rsp_data __free(qcom_tzmem) = NULL; > unsigned long name_length; > size_t name_offs; > size_t guid_offs; > @@ -448,17 +428,14 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e > __array_offs(u8, data_size, &data_offs) > ); > > - req_data = kzalloc(req_size, GFP_KERNEL); > - if (!req_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out; > - } > + req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL); > + if (!req_data) > + return EFI_OUT_OF_RESOURCES; > > - rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL); > - if (!rsp_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out_free_req; > - } > + rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data), > + GFP_KERNEL); > + if (!rsp_data) > + return EFI_OUT_OF_RESOURCES; > > req_data->command_id = QSEE_CMD_UEFI_SET_VARIABLE; > req_data->attributes = attributes; > @@ -481,42 +458,31 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e > > status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, > sizeof(*rsp_data)); > - if (status) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (status) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->length != sizeof(*rsp_data)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length != sizeof(*rsp_data)) > + return EFI_DEVICE_ERROR; > > if (rsp_data->status) { > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > __func__, rsp_data->status); > - efi_status = qsee_uefi_status_to_efi(rsp_data->status); > + return qsee_uefi_status_to_efi(rsp_data->status); > } > > -out_free: > - kfree(rsp_data); > -out_free_req: > - kfree(req_data); > -out: > - return efi_status; > + return EFI_SUCCESS; > } > > static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > unsigned long *name_size, efi_char16_t *name, > efi_guid_t *guid) > { > - struct qsee_req_uefi_get_next_variable *req_data; > - struct qsee_rsp_uefi_get_next_variable *rsp_data; > - efi_status_t efi_status = EFI_SUCCESS; > + struct qsee_req_uefi_get_next_variable *req_data __free(qcom_tzmem) = NULL; > + struct qsee_rsp_uefi_get_next_variable *rsp_data __free(qcom_tzmem) = NULL; > + efi_status_t efi_status; > size_t guid_offs; > size_t name_offs; > size_t req_size; > @@ -541,17 +507,13 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > __array(*name, *name_size / sizeof(*name)) > ); > > - req_data = kzalloc(req_size, GFP_KERNEL); > - if (!req_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out; > - } > + req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL); > + if (!req_data) > + return EFI_OUT_OF_RESOURCES; > > - rsp_data = kzalloc(rsp_size, GFP_KERNEL); > - if (!rsp_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out_free_req; > - } > + rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL); > + if (!rsp_data) > + return EFI_OUT_OF_RESOURCES; > > req_data->command_id = QSEE_CMD_UEFI_GET_NEXT_VARIABLE; > req_data->guid_offset = guid_offs; > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > return EFI_INVALID_PARAMETER; > > status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size); > - if (status) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (status) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->length < sizeof(*rsp_data)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length < sizeof(*rsp_data)) > + return EFI_DEVICE_ERROR; > > if (rsp_data->status) { > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi, > if (efi_status == EFI_BUFFER_TOO_SMALL) > *name_size = rsp_data->name_size; > > - goto out_free; > + return efi_status; > } > > - if (rsp_data->length > rsp_size) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length > rsp_size) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) > + return EFI_DEVICE_ERROR; > > if (rsp_data->name_size > *name_size) { > *name_size = rsp_data->name_size; > - efi_status = EFI_BUFFER_TOO_SMALL; > - goto out_free; > + return EFI_BUFFER_TOO_SMALL; > } > > - if (rsp_data->guid_size != sizeof(*guid)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->guid_size != sizeof(*guid)) > + return EFI_DEVICE_ERROR; > > memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size); > status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset, > rsp_data->name_size / sizeof(*name)); > *name_size = rsp_data->name_size; > > - if (status < 0) { > + if (status < 0) > /* > * Return EFI_DEVICE_ERROR here because the buffer size should > * have already been validated above, causing this function to > * bail with EFI_BUFFER_TOO_SMALL. > */ > return EFI_DEVICE_ERROR; > - } > > -out_free: > - kfree(rsp_data); > -out_free_req: > - kfree(req_data); > -out: > - return efi_status; > + return EFI_SUCCESS; > } > > static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, u32 attr, > u64 *storage_space, u64 *remaining_space, > u64 *max_variable_size) > { > - struct qsee_req_uefi_query_variable_info *req_data; > - struct qsee_rsp_uefi_query_variable_info *rsp_data; > - efi_status_t efi_status = EFI_SUCCESS; > + struct qsee_req_uefi_query_variable_info *req_data __free(qcom_tzmem) = NULL; > + struct qsee_rsp_uefi_query_variable_info *rsp_data __free(qcom_tzmem) = NULL; > int status; > > - req_data = kzalloc(sizeof(*req_data), GFP_KERNEL); > - if (!req_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out; > - } > + req_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*req_data), > + GFP_KERNEL); > + if (!req_data) > + return EFI_OUT_OF_RESOURCES; > > - rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL); > - if (!rsp_data) { > - efi_status = EFI_OUT_OF_RESOURCES; > - goto out_free_req; > - } > + rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data), > + GFP_KERNEL); > + if (!rsp_data) > + return EFI_OUT_OF_RESOURCES; > > req_data->command_id = QSEE_CMD_UEFI_QUERY_VARIABLE_INFO; > req_data->attributes = attr; > @@ -673,26 +611,19 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, > > status = qcom_qseecom_app_send(qcuefi->client, req_data, sizeof(*req_data), rsp_data, > sizeof(*rsp_data)); > - if (status) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (status) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO) > + return EFI_DEVICE_ERROR; > > - if (rsp_data->length != sizeof(*rsp_data)) { > - efi_status = EFI_DEVICE_ERROR; > - goto out_free; > - } > + if (rsp_data->length != sizeof(*rsp_data)) > + return EFI_DEVICE_ERROR; > > if (rsp_data->status) { > dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n", > __func__, rsp_data->status); > - efi_status = qsee_uefi_status_to_efi(rsp_data->status); > - goto out_free; > + return qsee_uefi_status_to_efi(rsp_data->status); > } > > if (storage_space) > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, > if (max_variable_size) > *max_variable_size = rsp_data->max_variable_size; > > -out_free: > - kfree(rsp_data); > -out_free_req: > - kfree(req_data); > -out: > - return efi_status; > + return EFI_SUCCESS; > } > > /* -- Global efivar interface. ---------------------------------------------- */ > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, > if (status) > qcuefi_set_reference(NULL); > > + qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K); > + if (IS_ERR(qcuefi->mempool)) > + return PTR_ERR(qcuefi->mempool); > + > return status; > } > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 3a6cefb4eb2e..318d7d398e5f 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -1567,9 +1567,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_get_id); > /** > * qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app. > * @app_id: The ID of the target app. > - * @req: Request buffer sent to the app (must be DMA-mappable). > + * @req: Request buffer sent to the app (must be TZ memory) > * @req_size: Size of the request buffer. > - * @rsp: Response buffer, written to by the app (must be DMA-mappable). > + * @rsp: Response buffer, written to by the app (must be TZ memory) > * @rsp_size: Size of the response buffer. > * > * Sends a request to the QSEE app associated with the given ID and read back > @@ -1585,26 +1585,12 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp, > { > struct qcom_scm_qseecom_resp res = {}; > struct qcom_scm_desc desc = {}; > - dma_addr_t req_phys; > - dma_addr_t rsp_phys; > + phys_addr_t req_phys; > + phys_addr_t rsp_phys; > int status; > > - /* Map request buffer */ > - req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE); > - status = dma_mapping_error(__scm->dev, req_phys); > - if (status) { > - dev_err(__scm->dev, "qseecom: failed to map request buffer\n"); > - return status; > - } > - > - /* Map response buffer */ > - rsp_phys = dma_map_single(__scm->dev, rsp, rsp_size, DMA_FROM_DEVICE); > - status = dma_mapping_error(__scm->dev, rsp_phys); > - if (status) { > - dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE); > - dev_err(__scm->dev, "qseecom: failed to map response buffer\n"); > - return status; > - } > + req_phys = qcom_tzmem_to_phys(req); > + rsp_phys = qcom_tzmem_to_phys(rsp); > > /* Set up SCM call data */ > desc.owner = QSEECOM_TZ_OWNER_TZ_APPS; > @@ -1622,10 +1608,6 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp, > /* Perform call */ > status = qcom_scm_qseecom_call(&desc, &res); > > - /* Unmap buffers */ > - dma_unmap_single(__scm->dev, rsp_phys, rsp_size, DMA_FROM_DEVICE); > - dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE); > - > if (status) > return status; > > diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h > index b531547e1dc9..26af1e778f00 100644 > --- a/include/linux/firmware/qcom/qcom_qseecom.h > +++ b/include/linux/firmware/qcom/qcom_qseecom.h > @@ -23,9 +23,9 @@ struct qseecom_client { > /** > * qcom_qseecom_app_send() - Send to and receive data from a given QSEE app. > * @client: The QSEECOM client associated with the target app. > - * @req: Request buffer sent to the app (must be DMA-mappable). > + * @req: Request buffer sent to the app (must be TZ memory). > * @req_size: Size of the request buffer. > - * @rsp: Response buffer, written to by the app (must be DMA-mappable). > + * @rsp: Response buffer, written to by the app (must be TZ memory). > * @rsp_size: Size of the response buffer. > * > * Sends a request to the QSEE app associated with the given client and read
On 10/11/23 09:44, Bartosz Golaszewski wrote: > On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote: >> >> On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and >>> convert all users of it in the qseecom module to using the TZ allocator >>> for creating SCM call buffers. Together with using the cleanup macros, >>> it has the added benefit of a significant code shrink. As this is >>> largely a module separate from the SCM driver, let's use a separate >>> memory pool. >>> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> [...] >>> @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, >>> if (max_variable_size) >>> *max_variable_size = rsp_data->max_variable_size; >>> >>> -out_free: >>> - kfree(rsp_data); >>> -out_free_req: >>> - kfree(req_data); >>> -out: >>> - return efi_status; >>> + return EFI_SUCCESS; >>> } >>> >>> /* -- Global efivar interface. ---------------------------------------------- */ >>> @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev, >>> if (status) >>> qcuefi_set_reference(NULL); >>> >>> + qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K); >> >> Any particular reason for this size? Just curious, it was (one) of the >> reasons I had not marked patch 4 yet (it looks good, but I wanted to get >> through the series to digest the Kconfig as well). >> > > I cannot test this. Do you know what the minimum correct size would be? Unfortunately, I don't know a specific size either. We can try to roughly estimate that though: At most, we have some rather negligible overhead for the argument struct and GUID, the name buffer, and the data buffer (get/set variable). The name is limited to 1024 utf-16 characters (although that's a limit that we set in our driver, not necessarily of the firmware). The thing that's more difficult to gauge is the maximum data size. Also I think we can reach the alloc code with multiple threads (unless the EFI subsystem is doing some locking). Only the actual SCM call is locked on the qseecom side. The efivar_query_variable_info() call could help with the data size part (it can return the maximum valid size of a single variable). Unfortunately it's not directly exposed, but I could code something up to read it out. The next best thing is `df -h` (which uses the same call under the hood) to get the total storage space available for EFI variables. On my Surface Pro X, that's 60K. So I guess overall, 64K should be enough for a single call. That being said, the biggest variable stored right now is about 4K in size. Given that that's a sample-size of one device though and that we might want to future-proof things, I think 256K is a good choice. But we could probably go with less if we really want to save some memory. Regards, Max
On Mon, Oct 09, 2023 at 05:34:27PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Enable SHM Bridge support in the Qualcomm TrustZone allocator by default > as even on architectures that don't support it, we automatically fall > back to the default behavior. Can you give some motivation for the Kconfig? It seems like what you've wrote should just fallback to the non SHM bridge allocated memory, so I don't see what having the option to exclude that at build time gives us. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > arch/arm64/configs/defconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 07011114eef8..ebe97fec6e33 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -255,6 +255,7 @@ CONFIG_INTEL_STRATIX10_RSU=m > CONFIG_EFI_CAPSULE_LOADER=y > CONFIG_IMX_SCU=y > CONFIG_IMX_SCU_PD=y > +CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE=y > CONFIG_GNSS=m > CONFIG_GNSS_MTK_SERIAL=m > CONFIG_MTD=y > -- > 2.39.2 >
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> This is pretty much another full rewrite of the SHM Bridge support series. After more on- and off-list discussions I think this time it will be close to the final thing though. We've established the need for using separate pools for SCM and QSEECOM as well as the upcoming scminvoke driver. It's also become clear that in order to be future-proof, the new allocator must be an abstraction layer of a higher level as the SHM Bridge will not be the only memory protection mechanism that we'll see upstream. Hence the rename to TrustZone Memory rather than SCM Memory allocator. Also to that end: the new allocator is its own module now and provides a Kconfig choice menu for selecting the mode of operation (currently default and SHM Bridge). Due to a high divergence from v2, I dropped all tags except for patch 1/15 which didn't change. Tested on sm8550 and sa8775p with the Inline Crypto Engine and remoteproc. v2 -> v3: - restore pool management and use separate pools for different users - don't use the new allocator in qcom_scm_pas_init_image() as the TrustZone will create an SHM bridge for us here - rewrite the entire series again for most part v1 -> v2: - too many changes to list, it's a complete rewrite as explained above Bartosz Golaszewski (15): firmware: qcom: move Qualcomm code into its own directory firmware: qcom: scm: add a missing forward declaration for struct device firmware: qcom: scm: remove unneeded 'extern' specifiers firmware: qcom: add a dedicated TrustZone buffer allocator firmware: qcom: scm: enable the TZ mem allocator firmware: qcom: scm: smc: switch to using the SCM allocator firmware: qcom: scm: make qcom_scm_assign_mem() use the TZ allocator firmware: qcom: scm: make qcom_scm_ice_set_key() use the TZ allocator firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the TZ allocator firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the TZ allocator firmware: qcom: qseecom: convert to using the TZ allocator firmware: qcom: scm: add support for SHM bridge operations firmware: qcom: tzmem: enable SHM Bridge support firmware: qcom: scm: clarify the comment in qcom_scm_pas_init_image() arm64: defconfig: enable SHM Bridge support for the TZ memory allocator MAINTAINERS | 4 +- arch/arm64/configs/defconfig | 1 + drivers/firmware/Kconfig | 48 +-- drivers/firmware/Makefile | 5 +- drivers/firmware/qcom/Kconfig | 86 ++++ drivers/firmware/qcom/Makefile | 10 + drivers/firmware/{ => qcom}/qcom_qseecom.c | 0 .../{ => qcom}/qcom_qseecom_uefisecapp.c | 260 +++++-------- drivers/firmware/{ => qcom}/qcom_scm-legacy.c | 0 drivers/firmware/{ => qcom}/qcom_scm-smc.c | 28 +- drivers/firmware/{ => qcom}/qcom_scm.c | 179 +++++---- drivers/firmware/{ => qcom}/qcom_scm.h | 21 +- drivers/firmware/qcom/qcom_tzmem.c | 366 ++++++++++++++++++ drivers/firmware/qcom/qcom_tzmem.h | 13 + include/linux/firmware/qcom/qcom_qseecom.h | 4 +- include/linux/firmware/qcom/qcom_scm.h | 6 + include/linux/firmware/qcom/qcom_tzmem.h | 28 ++ 17 files changed, 746 insertions(+), 313 deletions(-) create mode 100644 drivers/firmware/qcom/Kconfig create mode 100644 drivers/firmware/qcom/Makefile rename drivers/firmware/{ => qcom}/qcom_qseecom.c (100%) rename drivers/firmware/{ => qcom}/qcom_qseecom_uefisecapp.c (84%) rename drivers/firmware/{ => qcom}/qcom_scm-legacy.c (100%) rename drivers/firmware/{ => qcom}/qcom_scm-smc.c (91%) rename drivers/firmware/{ => qcom}/qcom_scm.c (93%) rename drivers/firmware/{ => qcom}/qcom_scm.h (88%) 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