2023-10-13 11:49:19

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 00/15] arm64: qcom: add and enable SHM Bridge support

From: Bartosz Golaszewski <[email protected]>

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.

v3 -> v4:
- include linux/sizes.h for SZ_X macros
- use dedicated RCU APIs to dereference radix tree slots
- fix kerneldocs
- fix the comment in patch 14/15: it's the hypervisor, not the TrustZone
that creates the SHM bridge

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 | 261 +++++--------
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 | 365 ++++++++++++++++++
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

--
2.39.2


2023-10-13 11:49:25

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 02/15] firmware: qcom: scm: add a missing forward declaration for struct device

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
---
drivers/firmware/qcom/qcom_scm.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index 7b68fa820495..c88e29051d20 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -4,6 +4,8 @@
#ifndef __QCOM_SCM_INT_H
#define __QCOM_SCM_INT_H

+struct device;
+
enum qcom_scm_convention {
SMC_CONVENTION_UNKNOWN,
SMC_CONVENTION_LEGACY,
--
2.39.2

2023-10-13 11:49:43

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 04/15] firmware: qcom: add a dedicated TrustZone buffer allocator

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
---
drivers/firmware/qcom/Kconfig | 19 ++
drivers/firmware/qcom/Makefile | 1 +
drivers/firmware/qcom/qcom_tzmem.c | 302 +++++++++++++++++++++++
drivers/firmware/qcom/qcom_tzmem.h | 13 +
include/linux/firmware/qcom/qcom_tzmem.h | 28 +++
5 files changed, 363 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..68ca59c5598e
--- /dev/null
+++ b/drivers/firmware/qcom/qcom_tzmem.c
@@ -0,0 +1,302 @@
+// 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 __rcu **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 = radix_tree_deref_slot_protected(slot,
+ &qcom_tzmem_chunks_lock);
+
+ 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 <[email protected]>");
+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 */
--
2.39.2

2023-10-13 11:49:45

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 03/15] firmware: qcom: scm: remove unneeded 'extern' specifiers

From: Bartosz Golaszewski <[email protected]>

'extern' specifiers do nothing for function declarations. Remove them
from the private qcom-scm header.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
---
drivers/firmware/qcom/qcom_scm.h | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index c88e29051d20..4532907e8489 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -66,18 +66,17 @@ int qcom_scm_wait_for_wq_completion(u32 wq_ctx);
int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);

#define SCM_SMC_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
-extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
- enum qcom_scm_convention qcom_convention,
- struct qcom_scm_res *res, bool atomic);
+int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
+ enum qcom_scm_convention qcom_convention,
+ struct qcom_scm_res *res, bool atomic);
#define scm_smc_call(dev, desc, res, atomic) \
__scm_smc_call((dev), (desc), qcom_scm_convention, (res), (atomic))

#define SCM_LEGACY_FNID(s, c) (((s) << 10) | ((c) & 0x3ff))
-extern int scm_legacy_call_atomic(struct device *dev,
- const struct qcom_scm_desc *desc,
- struct qcom_scm_res *res);
-extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
+int scm_legacy_call_atomic(struct device *dev, const struct qcom_scm_desc *desc,
struct qcom_scm_res *res);
+int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
+ struct qcom_scm_res *res);

#define QCOM_SCM_SVC_BOOT 0x01
#define QCOM_SCM_BOOT_SET_ADDR 0x01
--
2.39.2

2023-10-13 11:49:51

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 09/15] firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the TZ allocator

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
---
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

2023-10-13 11:50:01

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 06/15] firmware: qcom: scm: smc: switch to using the SCM allocator

From: Bartosz Golaszewski <[email protected]>

We need to allocate, map and pass a buffer to the trustzone if we have
more than 4 arguments for a given SCM calls. Let's use the new TrustZone
allocator for that memory and shrink the code in process.

As this code lives in a different compilation unit than the rest of the
SCM code, we need to provide a helper in the form of
qcom_scm_get_tzmem_pool() that allows the SMC low-level routines to
access the SCM memory pool.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
---
drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++--------------------
drivers/firmware/qcom/qcom_scm.c | 5 +++++
drivers/firmware/qcom/qcom_scm.h | 3 +++
3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 16cf88acfa8e..1a423d1cba6c 100644
--- a/drivers/firmware/qcom/qcom_scm-smc.c
+++ b/drivers/firmware/qcom/qcom_scm-smc.c
@@ -2,6 +2,7 @@
/* Copyright (c) 2015,2019 The Linux Foundation. All rights reserved.
*/

+#include <linux/cleanup.h>
#include <linux/io.h>
#include <linux/errno.h>
#include <linux/delay.h>
@@ -9,6 +10,7 @@
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/firmware/qcom/qcom_tzmem.h>
#include <linux/arm-smccc.h>
#include <linux/dma-mapping.h>

@@ -150,11 +152,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
enum qcom_scm_convention qcom_convention,
struct qcom_scm_res *res, bool atomic)
{
+ struct qcom_tzmem_pool *mempool = qcom_scm_get_tzmem_pool();
int arglen = desc->arginfo & 0xf;
int i, ret;
- dma_addr_t args_phys = 0;
- void *args_virt = NULL;
- size_t alloc_len;
+ void *args_virt __free(qcom_tzmem) = NULL;
gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
@@ -172,9 +173,9 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];

if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
- alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
- args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
-
+ args_virt = qcom_tzmem_alloc(mempool,
+ SCM_SMC_N_EXT_ARGS * sizeof(u64),
+ flag);
if (!args_virt)
return -ENOMEM;

@@ -192,25 +193,12 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
SCM_SMC_FIRST_EXT_IDX]);
}

- args_phys = dma_map_single(dev, args_virt, alloc_len,
- DMA_TO_DEVICE);
-
- if (dma_mapping_error(dev, args_phys)) {
- kfree(args_virt);
- return -ENOMEM;
- }
-
- smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
+ smc.args[SCM_SMC_LAST_REG_IDX] = qcom_tzmem_to_phys(args_virt);
}

/* ret error check follows after args_virt cleanup*/
ret = __scm_smc_do(dev, &smc, &smc_res, atomic);

- if (args_virt) {
- dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
- kfree(args_virt);
- }
-
if (ret)
return ret;

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 0d4c028be0c1..71e98b666391 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -201,6 +201,11 @@ static void qcom_scm_bw_disable(void)
enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
static DEFINE_SPINLOCK(scm_query_lock);

+struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void)
+{
+ return __scm->mempool;
+}
+
static enum qcom_scm_convention __get_convention(void)
{
unsigned long flags;
diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index 4532907e8489..aa7d06939f8e 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -5,6 +5,7 @@
#define __QCOM_SCM_INT_H

struct device;
+struct qcom_tzmem_pool;

enum qcom_scm_convention {
SMC_CONVENTION_UNKNOWN,
@@ -78,6 +79,8 @@ int scm_legacy_call_atomic(struct device *dev, const struct qcom_scm_desc *desc,
int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
struct qcom_scm_res *res);

+struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
+
#define QCOM_SCM_SVC_BOOT 0x01
#define QCOM_SCM_BOOT_SET_ADDR 0x01
#define QCOM_SCM_BOOT_TERMINATE_PC 0x02
--
2.39.2

2023-10-13 11:50:37

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 08/15] firmware: qcom: scm: make qcom_scm_ice_set_key() use the TZ allocator

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 754f6056b99f..31071a714cf1 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1197,32 +1197,21 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
.args[4] = data_unit_size,
.owner = ARM_SMCCC_OWNER_SIP,
};
- void *keybuf;
- dma_addr_t key_phys;
+
int ret;

- /*
- * 'key' may point to vmalloc()'ed memory, but we need to pass a
- * physical address that's been properly flushed. The sanctioned way to
- * do this is by using the DMA API. But as is best practice for crypto
- * keys, we also must wipe the key after use. This makes kmemdup() +
- * dma_map_single() not clearly correct, since the DMA API can use
- * bounce buffers. Instead, just use dma_alloc_coherent(). Programming
- * keys is normally rare and thus not performance-critical.
- */
-
- keybuf = dma_alloc_coherent(__scm->dev, key_size, &key_phys,
- GFP_KERNEL);
+ void *keybuf __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
+ key_size,
+ GFP_KERNEL);
if (!keybuf)
return -ENOMEM;
memcpy(keybuf, key, key_size);
- desc.args[1] = key_phys;
+ desc.args[1] = qcom_tzmem_to_phys(keybuf);

ret = qcom_scm_call(__scm->dev, &desc, NULL);

memzero_explicit(keybuf, key_size);

- dma_free_coherent(__scm->dev, key_size, keybuf, key_phys);
return ret;
}
EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
--
2.39.2

2023-10-13 11:50:49

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 01/15] firmware: qcom: move Qualcomm code into its own directory

From: Bartosz Golaszewski <[email protected]>

We're getting more and more qcom specific .c files in drivers/firmware/
and about to get even more. Create a separate directory for Qualcomm
firmware drivers and move existing sources in there.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Acked-by: Elliot Berman <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
---
MAINTAINERS | 4 +-
drivers/firmware/Kconfig | 48 +---------------
drivers/firmware/Makefile | 5 +-
drivers/firmware/qcom/Kconfig | 56 +++++++++++++++++++
drivers/firmware/qcom/Makefile | 9 +++
drivers/firmware/{ => qcom}/qcom_qseecom.c | 0
.../{ => qcom}/qcom_qseecom_uefisecapp.c | 0
drivers/firmware/{ => qcom}/qcom_scm-legacy.c | 0
drivers/firmware/{ => qcom}/qcom_scm-smc.c | 0
drivers/firmware/{ => qcom}/qcom_scm.c | 0
drivers/firmware/{ => qcom}/qcom_scm.h | 0
11 files changed, 69 insertions(+), 53 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 (100%)
rename drivers/firmware/{ => qcom}/qcom_scm-legacy.c (100%)
rename drivers/firmware/{ => qcom}/qcom_scm-smc.c (100%)
rename drivers/firmware/{ => qcom}/qcom_scm.c (100%)
rename drivers/firmware/{ => qcom}/qcom_scm.h (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0ad7caea2ee2..eac76b8de7e5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17930,13 +17930,13 @@ QUALCOMM QSEECOM DRIVER
M: Maximilian Luz <[email protected]>
L: [email protected]
S: Maintained
-F: drivers/firmware/qcom_qseecom.c
+F: drivers/firmware/qcom/qcom_qseecom.c

QUALCOMM QSEECOM UEFISECAPP DRIVER
M: Maximilian Luz <[email protected]>
L: [email protected]
S: Maintained
-F: drivers/firmware/qcom_qseecom_uefisecapp.c
+F: drivers/firmware/qcom/qcom_qseecom_uefisecapp.c

QUALCOMM RMNET DRIVER
M: Subash Abhinov Kasiviswanathan <[email protected]>
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 817e011a8945..74d00b0c83fe 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -188,53 +188,6 @@ config MTK_ADSP_IPC
ADSP exists on some mtk processors.
Client might use shared memory to exchange information with ADSP.

-config QCOM_SCM
- tristate
-
-config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
- bool "Qualcomm download mode enabled by default"
- depends on QCOM_SCM
- help
- A device with "download mode" enabled will upon an unexpected
- warm-restart enter a special debug mode that allows the user to
- "download" memory content over USB for offline postmortem analysis.
- The feature can be enabled/disabled on the kernel command line.
-
- Say Y here to enable "download mode" by default.
-
-config QCOM_QSEECOM
- bool "Qualcomm QSEECOM interface driver"
- depends on QCOM_SCM=y
- select AUXILIARY_BUS
- help
- Various Qualcomm SoCs have a Secure Execution Environment (SEE) running
- in the Trust Zone. This module provides an interface to that via the
- QSEECOM mechanism, using SCM calls.
-
- The QSEECOM interface allows, among other things, access to applications
- running in the SEE. An example of such an application is 'uefisecapp',
- which is required to access UEFI variables on certain systems. If
- selected, the interface will also attempt to detect and register client
- devices for supported applications.
-
- Select Y here to enable the QSEECOM interface driver.
-
-config QCOM_QSEECOM_UEFISECAPP
- bool "Qualcomm SEE UEFI Secure App client driver"
- depends on QCOM_QSEECOM
- depends on EFI
- help
- Various Qualcomm SoCs do not allow direct access to EFI variables.
- Instead, these need to be accessed via the UEFI Secure Application
- (uefisecapp), residing in the Secure Execution Environment (SEE).
-
- This module provides a client driver for uefisecapp, installing efivar
- operations to allow the kernel accessing EFI variables, and via that also
- provide user-space with access to EFI variables via efivarfs.
-
- Select Y here to provide access to EFI variables on the aforementioned
- platforms.
-
config SYSFB
bool
select BOOT_VESA_SUPPORT
@@ -320,6 +273,7 @@ source "drivers/firmware/efi/Kconfig"
source "drivers/firmware/imx/Kconfig"
source "drivers/firmware/meson/Kconfig"
source "drivers/firmware/psci/Kconfig"
+source "drivers/firmware/qcom/Kconfig"
source "drivers/firmware/smccc/Kconfig"
source "drivers/firmware/tegra/Kconfig"
source "drivers/firmware/xilinx/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index cb18fd8882dc..5f9dab82e1a0 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -17,10 +17,6 @@ obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o
obj-$(CONFIG_MTK_ADSP_IPC) += mtk-adsp-ipc.o
obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
-obj-$(CONFIG_QCOM_SCM) += qcom-scm.o
-qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
-obj-$(CONFIG_QCOM_QSEECOM) += qcom_qseecom.o
-obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o
obj-$(CONFIG_SYSFB) += sysfb.o
obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
@@ -36,6 +32,7 @@ obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
obj-y += efi/
obj-y += imx/
obj-y += psci/
+obj-y += qcom/
obj-y += smccc/
obj-y += tegra/
obj-y += xilinx/
diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
new file mode 100644
index 000000000000..3f05d9854ddf
--- /dev/null
+++ b/drivers/firmware/qcom/Kconfig
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# For a description of the syntax of this configuration file,
+# see Documentation/kbuild/kconfig-language.rst.
+#
+
+menu "Qualcomm firmware drivers"
+
+config QCOM_SCM
+ tristate
+
+config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
+ bool "Qualcomm download mode enabled by default"
+ depends on QCOM_SCM
+ help
+ A device with "download mode" enabled will upon an unexpected
+ warm-restart enter a special debug mode that allows the user to
+ "download" memory content over USB for offline postmortem analysis.
+ The feature can be enabled/disabled on the kernel command line.
+
+ Say Y here to enable "download mode" by default.
+
+config QCOM_QSEECOM
+ bool "Qualcomm QSEECOM interface driver"
+ depends on QCOM_SCM=y
+ select AUXILIARY_BUS
+ help
+ Various Qualcomm SoCs have a Secure Execution Environment (SEE) running
+ in the Trust Zone. This module provides an interface to that via the
+ QSEECOM mechanism, using SCM calls.
+
+ The QSEECOM interface allows, among other things, access to applications
+ running in the SEE. An example of such an application is 'uefisecapp',
+ which is required to access UEFI variables on certain systems. If
+ selected, the interface will also attempt to detect and register client
+ devices for supported applications.
+
+ Select Y here to enable the QSEECOM interface driver.
+
+config QCOM_QSEECOM_UEFISECAPP
+ bool "Qualcomm SEE UEFI Secure App client driver"
+ depends on QCOM_QSEECOM
+ depends on EFI
+ help
+ Various Qualcomm SoCs do not allow direct access to EFI variables.
+ Instead, these need to be accessed via the UEFI Secure Application
+ (uefisecapp), residing in the Secure Execution Environment (SEE).
+
+ This module provides a client driver for uefisecapp, installing efivar
+ operations to allow the kernel accessing EFI variables, and via that also
+ provide user-space with access to EFI variables via efivarfs.
+
+ Select Y here to provide access to EFI variables on the aforementioned
+ platforms.
+
+endmenu
diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile
new file mode 100644
index 000000000000..c9f12ee8224a
--- /dev/null
+++ b/drivers/firmware/qcom/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the linux kernel.
+#
+
+obj-$(CONFIG_QCOM_SCM) += qcom-scm.o
+qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_QSEECOM) += qcom_qseecom.o
+obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o
diff --git a/drivers/firmware/qcom_qseecom.c b/drivers/firmware/qcom/qcom_qseecom.c
similarity index 100%
rename from drivers/firmware/qcom_qseecom.c
rename to drivers/firmware/qcom/qcom_qseecom.c
diff --git a/drivers/firmware/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
similarity index 100%
rename from drivers/firmware/qcom_qseecom_uefisecapp.c
rename to drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
diff --git a/drivers/firmware/qcom_scm-legacy.c b/drivers/firmware/qcom/qcom_scm-legacy.c
similarity index 100%
rename from drivers/firmware/qcom_scm-legacy.c
rename to drivers/firmware/qcom/qcom_scm-legacy.c
diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
similarity index 100%
rename from drivers/firmware/qcom_scm-smc.c
rename to drivers/firmware/qcom/qcom_scm-smc.c
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
similarity index 100%
rename from drivers/firmware/qcom_scm.c
rename to drivers/firmware/qcom/qcom_scm.c
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
similarity index 100%
rename from drivers/firmware/qcom_scm.h
rename to drivers/firmware/qcom/qcom_scm.h
--
2.39.2

2023-10-13 11:51:40

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 12/15] firmware: qcom: scm: add support for SHM bridge operations

From: Bartosz Golaszewski <[email protected]>

Add low-level primitives for enabling SHM bridge support as well as
creating and destroying SHM bridge pools to qcom-scm.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 60 ++++++++++++++++++++++++++
drivers/firmware/qcom/qcom_scm.h | 3 ++
include/linux/firmware/qcom/qcom_scm.h | 6 +++
3 files changed, 69 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 318d7d398e5f..839773270a21 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1323,6 +1323,66 @@ bool qcom_scm_lmh_dcvsh_available(void)
}
EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);

+int qcom_scm_shm_bridge_enable(void)
+{
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_MP,
+ .cmd = QCOM_SCM_MP_SHM_BRIDGE_ENABLE,
+ .owner = ARM_SMCCC_OWNER_SIP
+ };
+
+ struct qcom_scm_res res;
+
+ if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
+ QCOM_SCM_MP_SHM_BRIDGE_ENABLE))
+ return -EOPNOTSUPP;
+
+ return qcom_scm_call(__scm->dev, &desc, &res) ?: res.result[0];
+}
+EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_enable);
+
+int qcom_scm_shm_bridge_create(struct device *dev, u64 pfn_and_ns_perm_flags,
+ u64 ipfn_and_s_perm_flags, u64 size_and_flags,
+ u64 ns_vmids, u64 *handle)
+{
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_MP,
+ .cmd = QCOM_SCM_MP_SHM_BRIDGE_CREATE,
+ .owner = ARM_SMCCC_OWNER_SIP,
+ .args[0] = pfn_and_ns_perm_flags,
+ .args[1] = ipfn_and_s_perm_flags,
+ .args[2] = size_and_flags,
+ .args[3] = ns_vmids,
+ .arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_VAL, QCOM_SCM_VAL,
+ QCOM_SCM_VAL, QCOM_SCM_VAL),
+ };
+
+ struct qcom_scm_res res;
+ int ret;
+
+ ret = qcom_scm_call(__scm->dev, &desc, &res);
+
+ if (handle && !ret)
+ *handle = res.result[1];
+
+ return ret ?: res.result[0];
+}
+EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_create);
+
+int qcom_scm_shm_bridge_delete(struct device *dev, u64 handle)
+{
+ struct qcom_scm_desc desc = {
+ .svc = QCOM_SCM_SVC_MP,
+ .cmd = QCOM_SCM_MP_SHM_BRIDGE_DELETE,
+ .owner = ARM_SMCCC_OWNER_SIP,
+ .args[0] = handle,
+ .arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL),
+ };
+
+ return qcom_scm_call(__scm->dev, &desc, NULL);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_shm_bridge_delete);
+
int qcom_scm_lmh_profile_change(u32 profile_id)
{
struct qcom_scm_desc desc = {
diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index aa7d06939f8e..cb7273aa0a5e 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -116,6 +116,9 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
#define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE 0x05
#define QCOM_SCM_MP_VIDEO_VAR 0x08
#define QCOM_SCM_MP_ASSIGN 0x16
+#define QCOM_SCM_MP_SHM_BRIDGE_ENABLE 0x1c
+#define QCOM_SCM_MP_SHM_BRIDGE_DELETE 0x1d
+#define QCOM_SCM_MP_SHM_BRIDGE_CREATE 0x1e

#define QCOM_SCM_SVC_OCMEM 0x0f
#define QCOM_SCM_OCMEM_LOCK_CMD 0x01
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index ccaf28846054..9b6054813f59 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -115,6 +115,12 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
int qcom_scm_lmh_profile_change(u32 profile_id);
bool qcom_scm_lmh_dcvsh_available(void);

+int qcom_scm_shm_bridge_enable(void);
+int qcom_scm_shm_bridge_create(struct device *dev, u64 pfn_and_ns_perm_flags,
+ u64 ipfn_and_s_perm_flags, u64 size_and_flags,
+ u64 ns_vmids, u64 *handle);
+int qcom_scm_shm_bridge_delete(struct device *dev, u64 handle);
+
#ifdef CONFIG_QCOM_QSEECOM

int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id);
--
2.39.2

2023-10-13 11:51:42

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 13/15] firmware: qcom: tzmem: enable SHM Bridge support

From: Bartosz Golaszewski <[email protected]>

Add a new Kconfig option for selecting the SHM Bridge mode of operation
for the TrustZone memory allocator.

If enabled at build-time, it will still be checked for availability at
run-time. If the architecture doesn't support SHM Bridge, the allocator
will work just like in the default mode.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/firmware/qcom/Kconfig | 10 +++++
drivers/firmware/qcom/qcom_tzmem.c | 65 +++++++++++++++++++++++++++++-
2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
index 237da40de832..e01407e31ae4 100644
--- a/drivers/firmware/qcom/Kconfig
+++ b/drivers/firmware/qcom/Kconfig
@@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT
Use the default allocator mode. The memory is page-aligned, non-cachable
and contiguous.

+config QCOM_TZMEM_MODE_SHMBRIDGE
+ bool "SHM Bridge"
+ help
+ Use Qualcomm Shared Memory Bridge. The memory has the same alignment as
+ in the 'Default' allocator but is also explicitly marked as an SHM Bridge
+ buffer.
+
+ With this selected, all buffers passed to the TrustZone must be allocated
+ using the TZMem allocator or else the TrustZone will refuse to use them.
+
endchoice

config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
index 68ca59c5598e..6c4f29a00f26 100644
--- a/drivers/firmware/qcom/qcom_tzmem.c
+++ b/drivers/firmware/qcom/qcom_tzmem.c
@@ -55,7 +55,70 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)

}

-#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */
+#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE)
+
+#include <linux/firmware/qcom/qcom_scm.h>
+
+#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
+
+static bool qcom_tzmem_using_shm_bridge;
+
+static int qcom_tzmem_init(void)
+{
+ int ret;
+
+ ret = qcom_scm_shm_bridge_enable();
+ if (ret == -EOPNOTSUPP) {
+ dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n");
+ ret = 0;
+ }
+
+ if (!ret)
+ qcom_tzmem_using_shm_bridge = true;
+
+ return ret;
+}
+
+static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool)
+{
+ u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
+ int ret;
+
+ if (!qcom_tzmem_using_shm_bridge)
+ return 0;
+
+ ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
+ pfn_and_ns_perm = (u64)pool->pbase | ns_perms;
+ ipfn_and_s_perm = (u64)pool->pbase | ns_perms;
+ size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
+
+ u64 *handle __free(kfree) = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return -ENOMEM;
+
+ ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm,
+ ipfn_and_s_perm, size_and_flags,
+ QCOM_SCM_VMID_HLOS, handle);
+ if (ret)
+ return ret;
+
+ pool->priv = no_free_ptr(handle);
+
+ return 0;
+}
+
+static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
+{
+ u64 *handle = pool->priv;
+
+ if (!qcom_tzmem_using_shm_bridge)
+ return;
+
+ qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle);
+ kfree(handle);
+}
+
+#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */

/**
* qcom_tzmem_pool_new() - Create a new TZ memory pool.
--
2.39.2

2023-10-13 11:51:50

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 07/15] firmware: qcom: scm: make qcom_scm_assign_mem() use the TZ allocator

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
---
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;
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

2023-10-13 11:51:51

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 10/15] firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the TZ allocator

From: Bartosz Golaszewski <[email protected]>

Let's use the new TZ memory allocator to obtain a buffer for this call
instead of manually kmalloc()ing it and then mapping to physical space.

Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
Tested-by: Maximilian Luz <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 11638daa2fe5..3a6cefb4eb2e 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1525,37 +1525,27 @@ int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
unsigned long app_name_len = strlen(app_name);
struct qcom_scm_desc desc = {};
struct qcom_scm_qseecom_resp res = {};
- dma_addr_t name_buf_phys;
- char *name_buf;
int status;

if (app_name_len >= name_buf_size)
return -EINVAL;

- name_buf = kzalloc(name_buf_size, GFP_KERNEL);
+ char *name_buf __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
+ name_buf_size,
+ GFP_KERNEL);
if (!name_buf)
return -ENOMEM;

memcpy(name_buf, app_name, app_name_len);

- name_buf_phys = dma_map_single(__scm->dev, name_buf, name_buf_size, DMA_TO_DEVICE);
- status = dma_mapping_error(__scm->dev, name_buf_phys);
- if (status) {
- kfree(name_buf);
- dev_err(__scm->dev, "qseecom: failed to map dma address\n");
- return status;
- }
-
desc.owner = QSEECOM_TZ_OWNER_QSEE_OS;
desc.svc = QSEECOM_TZ_SVC_APP_MGR;
desc.cmd = QSEECOM_TZ_CMD_APP_LOOKUP;
desc.arginfo = QCOM_SCM_ARGS(2, QCOM_SCM_RW, QCOM_SCM_VAL);
- desc.args[0] = name_buf_phys;
+ desc.args[0] = qcom_tzmem_to_phys(name_buf);
desc.args[1] = app_name_len;

status = qcom_scm_qseecom_call(&desc, &res);
- dma_unmap_single(__scm->dev, name_buf_phys, name_buf_size, DMA_TO_DEVICE);
- kfree(name_buf);

if (status)
return status;
--
2.39.2

2023-10-13 11:51:53

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 05/15] firmware: qcom: scm: enable the TZ mem allocator

From: Bartosz Golaszewski <[email protected]>

Select the TrustZone memory allocator in Kconfig and create a pool of
memory shareable with the TrustZone when probing the SCM driver.

This will allow a gradual conversion of all relevant SCM calls to using
the dedicated allocator.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/firmware/qcom/Kconfig | 1 +
drivers/firmware/qcom/qcom_scm.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
index b80269a28224..237da40de832 100644
--- a/drivers/firmware/qcom/Kconfig
+++ b/drivers/firmware/qcom/Kconfig
@@ -7,6 +7,7 @@
menu "Qualcomm firmware drivers"

config QCOM_SCM
+ select QCOM_TZMEM
tristate

config QCOM_TZMEM
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 520de9b5633a..0d4c028be0c1 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -8,8 +8,10 @@
#include <linux/completion.h>
#include <linux/cpumask.h>
#include <linux/dma-mapping.h>
+#include <linux/err.h>
#include <linux/export.h>
#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/firmware/qcom/qcom_tzmem.h>
#include <linux/init.h>
#include <linux/interconnect.h>
#include <linux/interrupt.h>
@@ -20,9 +22,11 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/reset-controller.h>
+#include <linux/sizes.h>
#include <linux/types.h>

#include "qcom_scm.h"
+#include "qcom_tzmem.h"

static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
module_param(download_mode, bool, 0);
@@ -41,6 +45,8 @@ struct qcom_scm {
int scm_vote_count;

u64 dload_mode_addr;
+
+ struct qcom_tzmem_pool *mempool;
};

struct qcom_scm_current_perm_info {
@@ -1887,6 +1893,16 @@ static int qcom_scm_probe(struct platform_device *pdev)
if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
qcom_scm_disable_sdi();

+ ret = qcom_tzmem_enable(__scm->dev);
+ if (ret)
+ return dev_err_probe(__scm->dev, ret,
+ "Failed to enable the TrustZone memory allocator\n");
+
+ __scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, SZ_256K);
+ if (IS_ERR(__scm->mempool))
+ return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool),
+ "Failed to create the SCM memory pool\n");
+
/*
* Initialize the QSEECOM interface.
*
--
2.39.2

2023-10-13 11:51:58

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 14/15] firmware: qcom: scm: clarify the comment in qcom_scm_pas_init_image()

From: Bartosz Golaszewski <[email protected]>

The "memory protection" mechanism mentioned in the comment is the SHM
Bridge. This is also the reason why we do not convert this call to using
the TZ memory allocator.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 839773270a21..7ba5cff6e4e7 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -563,9 +563,13 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
struct qcom_scm_res res;

/*
- * During the scm call memory protection will be enabled for the meta
- * data blob, so make sure it's physically contiguous, 4K aligned and
- * non-cachable to avoid XPU violations.
+ * During the SCM call the hypervisor will make the buffer containing
+ * the program data into an SHM Bridge. This is why we exceptionally
+ * must not use the TrustZone memory allocator here as - depending on
+ * Kconfig - it may already use the SHM Bridge mechanism internally.
+ *
+ * If we pass a buffer that is already part of an SHM Bridge to this
+ * call, it will fail.
*/
mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
GFP_KERNEL);
--
2.39.2

2023-10-13 11:51:58

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 15/15] arm64: defconfig: enable SHM Bridge support for the TZ memory allocator

From: Bartosz Golaszewski <[email protected]>

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.

Signed-off-by: Bartosz Golaszewski <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index b4220fff2b44..8cca74242cf6 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

2023-10-13 11:52:02

by Bartosz Golaszewski

[permalink] [raw]
Subject: [PATCH v4 11/15] firmware: qcom: qseecom: convert to using the TZ allocator

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
Reviewed-by: Andrew Halaney <[email protected]>
Tested-by: Maximilian Luz <[email protected]>
---
.../firmware/qcom/qcom_qseecom_uefisecapp.c | 261 +++++++-----------
drivers/firmware/qcom/qcom_scm.c | 30 +-
include/linux/firmware/qcom/qcom_qseecom.h | 4 +-
3 files changed, 104 insertions(+), 191 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
index a33acdaf7b78..3a068f8b6990 100644
--- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
+++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
@@ -7,17 +7,21 @@
* Copyright (C) 2023 Maximilian Luz <[email protected]>
*/

+#include <linux/cleanup.h>
#include <linux/efi.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/sizes.h>
#include <linux/slab.h>
#include <linux/types.h>
#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 +257,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 +277,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 +309,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 +332,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 +353,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 +383,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 +429,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 +459,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 +508,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 +530,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 +552,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 +612,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 +636,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 +765,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
--
2.39.2

2023-10-13 14:14:01

by Andrew Halaney

[permalink] [raw]
Subject: Re: [PATCH v4 13/15] firmware: qcom: tzmem: enable SHM Bridge support

On Fri, Oct 13, 2023 at 01:48:41PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a new Kconfig option for selecting the SHM Bridge mode of operation
> for the TrustZone memory allocator.
>
> If enabled at build-time, it will still be checked for availability at
> run-time. If the architecture doesn't support SHM Bridge, the allocator
> will work just like in the default mode.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/firmware/qcom/Kconfig | 10 +++++
> drivers/firmware/qcom/qcom_tzmem.c | 65 +++++++++++++++++++++++++++++-
> 2 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
> index 237da40de832..e01407e31ae4 100644
> --- a/drivers/firmware/qcom/Kconfig
> +++ b/drivers/firmware/qcom/Kconfig
> @@ -27,6 +27,16 @@ config QCOM_TZMEM_MODE_DEFAULT
> Use the default allocator mode. The memory is page-aligned, non-cachable
> and contiguous.
>
> +config QCOM_TZMEM_MODE_SHMBRIDGE
> + bool "SHM Bridge"
> + help
> + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as
> + in the 'Default' allocator but is also explicitly marked as an SHM Bridge
> + buffer.
> +
> + With this selected, all buffers passed to the TrustZone must be allocated
> + using the TZMem allocator or else the TrustZone will refuse to use them.
> +
> endchoice
>
> config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> index 68ca59c5598e..6c4f29a00f26 100644
> --- a/drivers/firmware/qcom/qcom_tzmem.c
> +++ b/drivers/firmware/qcom/qcom_tzmem.c
> @@ -55,7 +55,70 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
>
> }
>
> -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */
> +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE)
> +
> +#include <linux/firmware/qcom/qcom_scm.h>
> +
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
> +static bool qcom_tzmem_using_shm_bridge;
> +
> +static int qcom_tzmem_init(void)
> +{
> + int ret;
> +
> + ret = qcom_scm_shm_bridge_enable();
> + if (ret == -EOPNOTSUPP) {
> + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n");
> + ret = 0;
> + }
> +
> + if (!ret)
> + qcom_tzmem_using_shm_bridge = true;
> +
> + return ret;
> +}

Restating the comment we just concluded on in v3 (just to make it
obvious for other reviewers), the -EOPNOTSUPP
case here should probably return 0, not ret = 0, otherwise we'll try to
use SHM bridge on memory later even though it is not supported:

https://lore.kernel.org/linux-arm-msm/[email protected]/T/#m175bd35782cb5824820977bb3f799b32e32fb2d7

> +
> +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool)
> +{
> + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> + int ret;
> +
> + if (!qcom_tzmem_using_shm_bridge)
> + return 0;
> +
> + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> + pfn_and_ns_perm = (u64)pool->pbase | ns_perms;
> + ipfn_and_s_perm = (u64)pool->pbase | ns_perms;
> + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> + u64 *handle __free(kfree) = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle)
> + return -ENOMEM;
> +
> + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm,
> + ipfn_and_s_perm, size_and_flags,
> + QCOM_SCM_VMID_HLOS, handle);
> + if (ret)
> + return ret;
> +
> + pool->priv = no_free_ptr(handle);
> +
> + return 0;
> +}
> +
> +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool)
> +{
> + u64 *handle = pool->priv;
> +
> + if (!qcom_tzmem_using_shm_bridge)
> + return;
> +
> + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle);
> + kfree(handle);
> +}
> +
> +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */
>
> /**
> * qcom_tzmem_pool_new() - Create a new TZ memory pool.
> --
> 2.39.2
>

2023-10-13 15:00:12

by Andrew Halaney

[permalink] [raw]
Subject: Re: [PATCH v4 04/15] firmware: qcom: add a dedicated TrustZone buffer allocator

On Fri, Oct 13, 2023 at 01:48:32PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>

I'm still on the fence about introducing the Kconfig for the "just
in case the hypervisor is bad on some boards", but I spent a
while looking at this and it all looks good to me, so that opinion
aside...

Reviewed-by: Andrew Halaney <[email protected]>

2023-10-13 15:03:11

by Andrew Halaney

[permalink] [raw]
Subject: Re: [PATCH v4 05/15] firmware: qcom: scm: enable the TZ mem allocator

On Fri, Oct 13, 2023 at 01:48:33PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Select the TrustZone memory allocator in Kconfig and create a pool of
> memory shareable with the TrustZone when probing the SCM driver.
>
> This will allow a gradual conversion of all relevant SCM calls to using
> the dedicated allocator.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>

Reviewed-by: Andrew Halaney <[email protected]>

2023-10-13 15:05:14

by Andrew Halaney

[permalink] [raw]
Subject: Re: [PATCH v4 12/15] firmware: qcom: scm: add support for SHM bridge operations

On Fri, Oct 13, 2023 at 01:48:40PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add low-level primitives for enabling SHM bridge support as well as
> creating and destroying SHM bridge pools to qcom-scm.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>

Code wise this looks good to me, firmware interface wise I can't
properly review, so:

Acked-by: Andrew Halaney <[email protected]>

2023-10-13 15:30:38

by Andrew Halaney

[permalink] [raw]
Subject: Re: [PATCH v4 00/15] arm64: qcom: add and enable SHM Bridge support

On Fri, Oct 13, 2023 at 01:48:28PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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.

I took this for a spin on my x13s with SHM bridge enabled and don't see
any issues in the usecases that exercises. Also running on a debug
kernel with no warnings, etc.

Tested-by: Andrew Halaney <[email protected]> # sc8280xp-lenovo-thinkpad-x13s
>
> v3 -> v4:
> - include linux/sizes.h for SZ_X macros
> - use dedicated RCU APIs to dereference radix tree slots
> - fix kerneldocs
> - fix the comment in patch 14/15: it's the hypervisor, not the TrustZone
> that creates the SHM bridge
>
> 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 | 261 +++++--------
> 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 | 365 ++++++++++++++++++
> 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
>
> --
> 2.39.2
>

2023-10-13 16:33:13

by Guru Das Srinagesh

[permalink] [raw]
Subject: Re: [PATCH v4 06/15] firmware: qcom: scm: smc: switch to using the SCM allocator

On Oct 13 2023 13:48, Bartosz Golaszewski wrote:

[...]

> @@ -192,25 +193,12 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> SCM_SMC_FIRST_EXT_IDX]);
> }
>
> - args_phys = dma_map_single(dev, args_virt, alloc_len,
> - DMA_TO_DEVICE);
> -
> - if (dma_mapping_error(dev, args_phys)) {
> - kfree(args_virt);
> - return -ENOMEM;
> - }
> -
> - smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
> + smc.args[SCM_SMC_LAST_REG_IDX] = qcom_tzmem_to_phys(args_virt);
> }
>
> /* ret error check follows after args_virt cleanup*/

Since args_virt cleanup is being removed, please remove this comment also.

> ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
>
> - if (args_virt) {
> - dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
> - kfree(args_virt);
> - }
> -
> if (ret)
> return ret;

Guru Das.