[PATCH v2 00/11] arm64: qcom: add and enable SHM Bridge support

[PATCH v2 00/11] arm64: qcom: add and enable SHM Bridge support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This is technically the second iteration of the SHM Bridge enablement on
QCom platforms but in practice it's a complete rewrite.

During the internal discussion with QCom the requirement has been established
as an SHM Bridge pool manager with the assumption that there will be multiple
users, each with their own pool. The problem with this is that we don't have
many potential users of SHM pools upstream at the moment which was rightfully
pointed out in the reviews under v1 (which even had some unused symbols etc.).

Moreover: after some investigating, it turns out that there simply isn't any
need for multiple pools as the core SCM only allocates a buffer if given call
requires more than 4 arguments and there are only a handful of SCM calls that
need to pass a physical address to a buffer as argument to the trustzone.

Additionally all but one SCM call allocate buffers passed to the TZ only for
the duration of the call and then free it right aftr it returns. The one
exception is called once and the buffer it uses can remain in memory until
released by the user.

This all makes using multiple pools wasteful as most of that memory will be
reserved but remain unused 99% of the time. What we need is a single pool of
SCM memory that deals out chunks of suitable format (coherent and
page-aligned) that fulfills the requirements of all calls.

As not all architectures support SHM bridge, it makes sense to first unify the
memory operations in SCM calls. All users do some kind of DMA mapping to obtain
buffers, most using dma_alloc_coherent() which impacts performance.

Genalloc pools are very fast so let's use them instead. Create a custom
allocator that also deals with the mapping and use it across all SCM calls.

Then once this is done, let's extend the allocator to use the SHM bridge
functionality if available on the given platform.

While at it: let's create a qcom specific directory in drivers/firmware/ and
move relevant code in there.

I couldn't test all SCM calls but tested with the inline crypto engine on
sm8550 and sa8775p that runs most of new code paths (with and without SHM
bridge). At least qseecom would need some Tested-by.

v1 -> v2:
- too many changes to list, it's a complete rewrite as explained above

Bartosz Golaszewski (11):
  firmware: qcom: move Qualcomm code into its own directory
  firmware: qcom: scm: add a dedicated SCM memory allocator
  firmware: qcom: scm: switch to using the SCM allocator
  firmware: qcom: scm: make qcom_scm_assign_mem() use the SCM allocator
  firmware: qcom: scm: make qcom_scm_ice_set_key() use the SCM allocator
  firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM
    allocator
  firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the SCM allocator
  firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the SCM
    allocator
  firmware: qcom: qseecom: convert to using the SCM allocator
  firmware: qcom-scm: add support for SHM bridge operations
  firmware: qcom: scm: enable SHM bridge

 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      | 251 ++++++------------
 drivers/firmware/{ => qcom}/qcom_scm-legacy.c |   0
 drivers/firmware/qcom/qcom_scm-mem.c          | 170 ++++++++++++
 drivers/firmware/{ => qcom}/qcom_scm-smc.c    |  21 +-
 drivers/firmware/{ => qcom}/qcom_scm.c        | 149 ++++++-----
 drivers/firmware/{ => qcom}/qcom_scm.h        |   9 +
 include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
 include/linux/firmware/qcom/qcom_scm.h        |  13 +
 14 files changed, 427 insertions(+), 312 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%)
 create mode 100644 drivers/firmware/qcom/qcom_scm-mem.c
 rename drivers/firmware/{ => qcom}/qcom_scm-smc.c (92%)
 rename drivers/firmware/{ => qcom}/qcom_scm.c (94%)
 rename drivers/firmware/{ => qcom}/qcom_scm.h (95%)

-- 
2.39.2

[PATCH v2 01/11] firmware: qcom: move Qualcomm code into its own directory

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 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 861a16b4586c..88c2186b4975 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17887,13 +17887,13 @@ QUALCOMM QSEECOM DRIVER
 M:	Maximilian Luz <luzmaximilian@gmail.com>
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
-F:	drivers/firmware/qcom_qseecom.c
+F:	drivers/firmware/qcom/qcom_qseecom.c
 
 QUALCOMM QSEECOM UEFISECAPP DRIVER
 M:	Maximilian Luz <luzmaximilian@gmail.com>
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
-F:	drivers/firmware/qcom_qseecom_uefisecapp.c
+F:	drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
 
 QUALCOMM RMNET DRIVER
 M:	Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
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

[PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory allocator

[Bartosz Golaszewski]

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 allocated 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).

As all but one calls allocate memory just for the duration of the call,
we don't need a lot of memory. A single pool for that purpose is enough.
Let's create a genalloc pool dealing out chunks of coherent, page-aligned
memory suitable for SCM calls that also provides a function for mapping
virtual to physical addresses.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/Makefile         |   2 +-
 drivers/firmware/qcom/qcom_scm-mem.c   | 134 +++++++++++++++++++++++++
 drivers/firmware/qcom/qcom_scm.c       |   5 +
 drivers/firmware/qcom/qcom_scm.h       |   7 ++
 include/linux/firmware/qcom/qcom_scm.h |   7 ++
 5 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/qcom/qcom_scm-mem.c

diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile
index c9f12ee8224a..b9b117f22e9f 100644
--- a/drivers/firmware/qcom/Makefile
+++ b/drivers/firmware/qcom/Makefile
@@ -4,6 +4,6 @@
 #
 
 obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
-qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o qcom_scm-mem.o
 obj-$(CONFIG_QCOM_QSEECOM)	+= qcom_qseecom.o
 obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o
diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
new file mode 100644
index 000000000000..eafecbe23770
--- /dev/null
+++ b/drivers/firmware/qcom/qcom_scm-mem.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Linaro Ltd.
+ */
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/genalloc.h>
+#include <linux/gfp.h>
+#include <linux/moduleparam.h>
+#include <linux/radix-tree.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include "qcom_scm.h"
+
+static size_t qcom_scm_mem_pool_size = SZ_2M;
+module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
+		   ulong, 0400);
+
+struct {
+	struct device *dev;
+	void *vbase;
+	phys_addr_t pbase;
+	size_t size;
+	struct gen_pool *pool;
+	struct radix_tree_root chunks;
+	spinlock_t lock;
+} qcom_scm_mem;
+
+struct qcom_scm_mem_chunk {
+	phys_addr_t paddr;
+	size_t size;
+};
+
+void *qcom_scm_mem_alloc(size_t size, gfp_t gfp)
+{
+	struct qcom_scm_mem_chunk *chunk;
+	unsigned long vaddr;
+	int ret;
+
+	if (!size)
+		return ZERO_SIZE_PTR;
+
+	size = roundup(size, 1 << PAGE_SHIFT);
+
+	chunk = kzalloc(sizeof(*chunk), gfp);
+	if (!chunk)
+		return NULL;
+
+	vaddr = gen_pool_alloc(qcom_scm_mem.pool, size);
+	if (!vaddr) {
+		kfree(chunk);
+		return NULL;
+	}
+
+	chunk->paddr = gen_pool_virt_to_phys(qcom_scm_mem.pool,
+					     (unsigned long)vaddr);
+	chunk->size = size;
+
+	scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock) {
+		ret = radix_tree_insert(&qcom_scm_mem.chunks, vaddr, chunk);
+		if (ret) {
+			gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr,
+				      chunk->size);
+			kfree(chunk);
+			return NULL;
+		}
+	}
+
+	return (void *)vaddr;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_mem_alloc);
+
+void qcom_scm_mem_free(void *vaddr)
+{
+	struct qcom_scm_mem_chunk *chunk;
+
+	if (!vaddr)
+		return;
+
+	scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock)
+		chunk = radix_tree_delete_item(&qcom_scm_mem.chunks,
+					       (unsigned long)vaddr, NULL);
+
+	if (!chunk) {
+		WARN(1, "Virtual address %p not allocated for SCM", vaddr);
+		return;
+	}
+
+	gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr, chunk->size);
+	kfree(chunk);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_mem_free);
+
+phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
+{
+	struct qcom_scm_mem_chunk *chunk;
+
+	guard(spinlock_irqsave)(&qcom_scm_mem.lock);
+
+	chunk = radix_tree_lookup(&qcom_scm_mem.chunks, (unsigned long)vaddr);
+	if (!chunk)
+		return 0;
+
+	return chunk->paddr;
+}
+
+int qcom_scm_mem_enable(struct device *dev)
+{
+	INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
+	spin_lock_init(&qcom_scm_mem.lock);
+	qcom_scm_mem.dev = dev;
+	qcom_scm_mem.size = qcom_scm_mem_pool_size;
+
+	qcom_scm_mem.vbase = dmam_alloc_coherent(dev, qcom_scm_mem.size,
+						 &qcom_scm_mem.pbase,
+						 GFP_KERNEL);
+	if (!qcom_scm_mem.vbase)
+		return -ENOMEM;
+
+	qcom_scm_mem.pool = devm_gen_pool_create(dev, PAGE_SHIFT, -1,
+						 "qcom-scm-mem");
+	if (!qcom_scm_mem.pool)
+		return -ENOMEM;
+
+	gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
+
+	return gen_pool_add_virt(qcom_scm_mem.pool,
+				 (unsigned long)qcom_scm_mem.vbase,
+				 qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
+}
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index c2c7fafef34b..258aa0782754 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1880,6 +1880,11 @@ 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_scm_mem_enable(scm->dev);
+	if (ret)
+		return dev_err_probe(scm->dev, ret,
+				     "Failed to enable SCM memory\n");
+
 	/*
 	 * Initialize the QSEECOM interface.
 	 *
diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index 7b68fa820495..8c97e3906afa 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -4,6 +4,10 @@
 #ifndef __QCOM_SCM_INT_H
 #define __QCOM_SCM_INT_H
 
+#include <linux/types.h>
+
+struct device;
+
 enum qcom_scm_convention {
 	SMC_CONVENTION_UNKNOWN,
 	SMC_CONVENTION_LEGACY,
@@ -165,4 +169,7 @@ static inline int qcom_scm_remap_error(int err)
 	return -EINVAL;
 }
 
+int qcom_scm_mem_enable(struct device *dev);
+phys_addr_t qcom_scm_mem_to_phys(void *vaddr);
+
 #endif
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index ccaf28846054..291ef8fd21b0 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -5,7 +5,9 @@
 #ifndef __QCOM_SCM_H
 #define __QCOM_SCM_H
 
+#include <linux/cleanup.h>
 #include <linux/err.h>
+#include <linux/gfp.h>
 #include <linux/types.h>
 #include <linux/cpumask.h>
 
@@ -61,6 +63,11 @@ enum qcom_scm_ice_cipher {
 
 bool qcom_scm_is_available(void);
 
+void *qcom_scm_mem_alloc(size_t size, gfp_t gfp);
+void qcom_scm_mem_free(void *vaddr);
+
+DEFINE_FREE(qcom_scm_mem, void *, if (_T) qcom_scm_mem_free(_T));
+
 int qcom_scm_set_cold_boot_addr(void *entry);
 int qcom_scm_set_warm_boot_addr(void *entry);
 void qcom_scm_cpu_power_down(u32 flags);
-- 
2.39.2

[PATCH v2 03/11] firmware: qcom: scm: switch to using the SCM allocator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 SCM
allocator for that memory and shrink the code in process.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm-smc.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 16cf88acfa8e..0d5554df1321 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>
@@ -152,8 +153,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 {
 	int arglen = desc->arginfo & 0xf;
 	int i, ret;
-	dma_addr_t args_phys = 0;
-	void *args_virt = NULL;
+	void *args_virt __free(qcom_scm_mem) = NULL;
 	size_t alloc_len;
 	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
 	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
@@ -173,7 +173,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 
 	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_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
 
 		if (!args_virt)
 			return -ENOMEM;
@@ -192,25 +192,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_scm_mem_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;
 
-- 
2.39.2

[PATCH v2 04/11] firmware: qcom: scm: make qcom_scm_assign_mem() use the SCM allocator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Let's use the new SCM 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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 258aa0782754..0c532e794b92 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>
@@ -980,14 +981,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;
 
@@ -997,10 +997,12 @@ 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_scm_mem) = qcom_scm_mem_alloc(ptr_sz, GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
 
+	ptr_phys = qcom_scm_mem_to_phys(ptr);
+
 	/* Fill source vmid detail */
 	src = ptr;
 	i = 0;
@@ -1029,7 +1031,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

[PATCH v2 05/11] firmware: qcom: scm: make qcom_scm_ice_set_key() use the SCM allocator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Let's use the new SCM 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 | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 0c532e794b92..02a773ba1383 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1178,32 +1178,20 @@ 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_scm_mem) = qcom_scm_mem_alloc(key_size,
+							       GFP_KERNEL);
 	if (!keybuf)
 		return -ENOMEM;
 	memcpy(keybuf, key, key_size);
-	desc.args[1] = key_phys;
+	desc.args[1] = qcom_scm_mem_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

[PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Let's use the new SCM 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 | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 02a773ba1383..c0eb81069847 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
 int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 			    struct qcom_scm_pas_metadata *ctx)
 {
-	dma_addr_t mdata_phys;
+	phys_addr_t mdata_phys;
 	void *mdata_buf;
 	int ret;
 	struct qcom_scm_desc desc = {
@@ -544,13 +544,7 @@ 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.
-	 */
-	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
-				       GFP_KERNEL);
+	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
 	if (!mdata_buf) {
 		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
 		return -ENOMEM;
@@ -574,10 +568,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 
 out:
 	if (ret < 0 || !ctx) {
-		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
+		qcom_scm_mem_free(mdata_buf);
 	} else if (ctx) {
 		ctx->ptr = mdata_buf;
-		ctx->phys = mdata_phys;
+		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
 		ctx->size = size;
 	}
 
@@ -594,7 +588,7 @@ void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
 	if (!ctx->ptr)
 		return;
 
-	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
+	qcom_scm_mem_free(ctx->ptr);
 
 	ctx->ptr = NULL;
 	ctx->phys = 0;
-- 
2.39.2

[PATCH v2 07/11] firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the SCM allocator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Let's use the new SCM 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 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index c0eb81069847..41095bf1d4d7 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1314,8 +1314,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 = {
@@ -1330,7 +1328,8 @@ 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_scm_mem) =
+				qcom_scm_mem_alloc(payload_size, GFP_KERNEL);
 	if (!payload_buf)
 		return -ENOMEM;
 
@@ -1340,11 +1339,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_scm_mem_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

[RFT PATCH v2 08/11] firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the SCM allocator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Let's use the new SCM 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 <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 41095bf1d4d7..37e876437a3e 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1498,37 +1498,26 @@ 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_scm_mem) = qcom_scm_mem_alloc(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_scm_mem_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

[RFT PATCH v2 09/11] firmware: qcom: qseecom: convert to using the SCM allocator

[Bartosz Golaszewski]

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 SCM allocator
for creating SCM call buffers. Together with using the cleanup macros,
it has the added benefit of a significant code shrink

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../firmware/qcom/qcom_qseecom_uefisecapp.c   | 251 ++++++------------
 drivers/firmware/qcom/qcom_scm.c              |  31 +--
 include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
 3 files changed, 94 insertions(+), 192 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
index a33acdaf7b78..59193716161d 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,7 @@
 #include <linux/ucs2_string.h>
 
 #include <linux/firmware/qcom/qcom_qseecom.h>
+#include <linux/firmware/qcom/qcom_scm.h>
 
 /* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */
 
@@ -272,11 +274,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_scm_mem) = NULL;
+	struct qsee_rsp_uefi_get_variable *rsp_data __free(qcom_scm_mem) = 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 +306,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_scm_mem_alloc(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_scm_mem_alloc(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 +329,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 +350,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 +380,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_scm_mem) = NULL;
+	struct qsee_rsp_uefi_set_variable *rsp_data __free(qcom_scm_mem) = NULL;
 	unsigned long name_length;
 	size_t name_offs;
 	size_t guid_offs;
@@ -448,17 +426,13 @@ 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_scm_mem_alloc(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_scm_mem_alloc(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 +455,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_scm_mem) = NULL;
+	struct qsee_rsp_uefi_get_next_variable *rsp_data __free(qcom_scm_mem) = NULL;
+	efi_status_t efi_status;
 	size_t guid_offs;
 	size_t name_offs;
 	size_t req_size;
@@ -541,17 +504,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_scm_mem_alloc(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_scm_mem_alloc(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 +526,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 +548,57 @@ 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_scm_mem) = NULL;
+	struct qsee_rsp_uefi_query_variable_info *rsp_data __free(qcom_scm_mem) = 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_scm_mem_alloc(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_scm_mem_alloc(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 +606,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 +630,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. ---------------------------------------------- */
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 37e876437a3e..1fa27c44f472 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -8,7 +8,6 @@
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/cpumask.h>
-#include <linux/dma-mapping.h>
 #include <linux/export.h>
 #include <linux/firmware/qcom/qcom_scm.h>
 #include <linux/init.h>
@@ -1539,9 +1538,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 SCM 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 SCM memory)
  * @rsp_size: Size of the response buffer.
  *
  * Sends a request to the QSEE app associated with the given ID and read back
@@ -1557,26 +1556,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_scm_mem_to_phys(req);
+	rsp_phys = qcom_scm_mem_to_phys(rsp);
 
 	/* Set up SCM call data */
 	desc.owner = QSEECOM_TZ_OWNER_TZ_APPS;
@@ -1594,10 +1579,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..9b9803f78f03 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 SCM 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 SCM 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

[PATCH v2 10/11] firmware: qcom-scm: add support for SHM bridge operations

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add low-level primitives for enabling SHM bridge support, creating SHM
bridge pools and testing the availability of SHM bridges to qcom-scm. We
don't yet provide a way to destroy the bridges as the first user will
not require it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c       | 43 ++++++++++++++++++++++++++
 drivers/firmware/qcom/qcom_scm.h       |  2 ++
 include/linux/firmware/qcom/qcom_scm.h |  6 ++++
 3 files changed, 51 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 1fa27c44f472..5969ff0c0beb 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1296,6 +1296,49 @@ bool qcom_scm_lmh_dcvsh_available(void)
 }
 EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
 
+int qcom_scm_enable_shm_bridge(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_enable_shm_bridge);
+
+int qcom_scm_create_shm_bridge(struct device *dev, u64 pfn_and_ns_perm_flags,
+			       u64 ipfn_and_s_perm_flags, u64 size_and_flags,
+			       u64 ns_vmids)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_MP,
+		.cmd = QCOM_SCM_MP_SHM_BRDIGE_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);
+
+	return ret ?: res.result[0];
+}
+EXPORT_SYMBOL_GPL(qcom_scm_create_shm_bridge);
+
 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 8c97e3906afa..f5a29bc0f549 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -116,6 +116,8 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #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_BRDIGE_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 291ef8fd21b0..dc26cfd6d011 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -6,6 +6,7 @@
 #define __QCOM_SCM_H
 
 #include <linux/cleanup.h>
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gfp.h>
 #include <linux/types.h>
@@ -122,6 +123,11 @@ 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_enable_shm_bridge(void);
+int qcom_scm_create_shm_bridge(struct device *dev, u64 pfn_and_ns_perm_flags,
+			       u64 ipfn_and_s_perm_flags, u64 size_and_flags,
+			       u64 ns_vmids);
+
 #ifdef CONFIG_QCOM_QSEECOM
 
 int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id);
-- 
2.39.2

[PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Extens the SCM memory allocator with using the SHM Bridge feature if
available on the platform. This makes the trustzone only use dedicated
buffers for SCM calls. We map the entire SCM genpool as a bridge.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
index eafecbe23770..12b12b15f46f 100644
--- a/drivers/firmware/qcom/qcom_scm-mem.c
+++ b/drivers/firmware/qcom/qcom_scm-mem.c
@@ -16,6 +16,8 @@
 
 #include "qcom_scm.h"
 
+#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
+
 static size_t qcom_scm_mem_pool_size = SZ_2M;
 module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
 		   ulong, 0400);
@@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
 	return chunk->paddr;
 }
 
+static int qcom_scm_mem_shm_bridge_create(void)
+{
+	uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
+
+	ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
+	pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
+	ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
+	size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
+
+	return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
+					  ipfn_and_s_perm, size_and_flags,
+					  QCOM_SCM_VMID_HLOS);
+}
+
 int qcom_scm_mem_enable(struct device *dev)
 {
+	int ret;
+
 	INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
 	spin_lock_init(&qcom_scm_mem.lock);
 	qcom_scm_mem.dev = dev;
@@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
 
 	gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
 
-	return gen_pool_add_virt(qcom_scm_mem.pool,
-				 (unsigned long)qcom_scm_mem.vbase,
-				 qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
+	ret = gen_pool_add_virt(qcom_scm_mem.pool,
+				(unsigned long)qcom_scm_mem.vbase,
+				qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
+	if (ret)
+		return ret;
+
+	ret = qcom_scm_enable_shm_bridge();
+	if (ret) {
+		if (ret == EOPNOTSUPP)
+			dev_info(dev, "SHM Bridge not supported\n");
+		else
+			return ret;
+	} else {
+		ret = qcom_scm_mem_shm_bridge_create();
+		if (ret)
+			return ret;
+
+		dev_info(dev, "SHM Bridge enabled\n");
+	}
+
+	return 0;
 }
-- 
2.39.2

Re: [PATCH v2 01/11] firmware: qcom: move Qualcomm code into its own directory

[Elliot Berman]

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>

Acked-by: Elliot Berman <quic_eberman@quicinc.com>

> ---
>  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 861a16b4586c..88c2186b4975 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17887,13 +17887,13 @@ QUALCOMM QSEECOM DRIVER
>  M:	Maximilian Luz <luzmaximilian@gmail.com>
>  L:	linux-arm-msm@vger.kernel.org
>  S:	Maintained
> -F:	drivers/firmware/qcom_qseecom.c
> +F:	drivers/firmware/qcom/qcom_qseecom.c
>  
>  QUALCOMM QSEECOM UEFISECAPP DRIVER
>  M:	Maximilian Luz <luzmaximilian@gmail.com>
>  L:	linux-arm-msm@vger.kernel.org
>  S:	Maintained
> -F:	drivers/firmware/qcom_qseecom_uefisecapp.c
> +F:	drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>  
>  QUALCOMM RMNET DRIVER
>  M:	Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
> 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

Re: [PATCH v2 10/11] firmware: qcom-scm: add support for SHM bridge operations

[Elliot Berman]

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add low-level primitives for enabling SHM bridge support, creating SHM
> bridge pools and testing the availability of SHM bridges to qcom-scm. We
> don't yet provide a way to destroy the bridges as the first user will
> not require it.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

After fixing the typo:

Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>

> ---
>  drivers/firmware/qcom/qcom_scm.c       | 43 ++++++++++++++++++++++++++
>  drivers/firmware/qcom/qcom_scm.h       |  2 ++
>  include/linux/firmware/qcom/qcom_scm.h |  6 ++++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 1fa27c44f472..5969ff0c0beb 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1296,6 +1296,49 @@ bool qcom_scm_lmh_dcvsh_available(void)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_lmh_dcvsh_available);
>  
> +int qcom_scm_enable_shm_bridge(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_enable_shm_bridge);
> +
> +int qcom_scm_create_shm_bridge(struct device *dev, u64 pfn_and_ns_perm_flags,
> +			       u64 ipfn_and_s_perm_flags, u64 size_and_flags,
> +			       u64 ns_vmids)
> +{
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_MP,
> +		.cmd = QCOM_SCM_MP_SHM_BRDIGE_CREATE,

s/BRDIGE/BRIDGE/g

> +		.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);
> +
> +	return ret ?: res.result[0];
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_create_shm_bridge);
> +
>  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 8c97e3906afa..f5a29bc0f549 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -116,6 +116,8 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  #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_BRDIGE_CREATE		0x1e

s/BRDIGE/BRIDGE/g

>  
>  #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 291ef8fd21b0..dc26cfd6d011 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -6,6 +6,7 @@
>  #define __QCOM_SCM_H
>  
>  #include <linux/cleanup.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/gfp.h>
>  #include <linux/types.h>
> @@ -122,6 +123,11 @@ 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_enable_shm_bridge(void);
> +int qcom_scm_create_shm_bridge(struct device *dev, u64 pfn_and_ns_perm_flags,
> +			       u64 ipfn_and_s_perm_flags, u64 size_and_flags,
> +			       u64 ns_vmids);
> +
>  #ifdef CONFIG_QCOM_QSEECOM
>  
>  int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id);

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Elliot Berman]

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Extens the SCM memory allocator with using the SHM Bridge feature if
> available on the platform. This makes the trustzone only use dedicated
> buffers for SCM calls. We map the entire SCM genpool as a bridge.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> index eafecbe23770..12b12b15f46f 100644
> --- a/drivers/firmware/qcom/qcom_scm-mem.c
> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> @@ -16,6 +16,8 @@
>  
>  #include "qcom_scm.h"
>  
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
>  static size_t qcom_scm_mem_pool_size = SZ_2M;
>  module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
>  		   ulong, 0400);
> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
>  	return chunk->paddr;
>  }
>  
> +static int qcom_scm_mem_shm_bridge_create(void)
> +{
> +	uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> +
> +	ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> +	pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> +	ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> +	size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> +	return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> +					  ipfn_and_s_perm, size_and_flags,
> +					  QCOM_SCM_VMID_HLOS);
> +}
> +
>  int qcom_scm_mem_enable(struct device *dev)
>  {
> +	int ret;
> +
>  	INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
>  	spin_lock_init(&qcom_scm_mem.lock);
>  	qcom_scm_mem.dev = dev;
> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>  
>  	gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>  
> -	return gen_pool_add_virt(qcom_scm_mem.pool,
> -				 (unsigned long)qcom_scm_mem.vbase,
> -				 qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> +	ret = gen_pool_add_virt(qcom_scm_mem.pool,
> +				(unsigned long)qcom_scm_mem.vbase,
> +				qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_scm_enable_shm_bridge();
> +	if (ret) {
> +		if (ret == EOPNOTSUPP)
> +			dev_info(dev, "SHM Bridge not supported\n");
> +		else
> +			return ret;
> +	} else {
> +		ret = qcom_scm_mem_shm_bridge_create();
> +		if (ret)
> +			return ret;
> +
> +		dev_info(dev, "SHM Bridge enabled\n");

Do you need to add clean up (deletion) of the SHM bridge on driver remove?

One easy approach I could think: implemnet devm_qcom_scm_mem_shm_bridge_create
which calls qcom_scm_delete_shm_bridge on the clean up 
(qcom_scm_delete_shm_bridge implemented in downstream, not in this series).

> +	}
> +
> +	return 0;
>  }

Re: [PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory allocator

[Jeff Johnson]

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> +void *qcom_scm_mem_alloc(size_t size, gfp_t gfp)
> +{
> +	struct qcom_scm_mem_chunk *chunk;
> +	unsigned long vaddr;

there are places below where you unnecessarily typecast this to its 
given type

> +	int ret;
> +
> +	if (!size)
> +		return ZERO_SIZE_PTR;
> +
> +	size = roundup(size, 1 << PAGE_SHIFT);
> +
> +	chunk = kzalloc(sizeof(*chunk), gfp);
> +	if (!chunk)
> +		return NULL;
> +
> +	vaddr = gen_pool_alloc(qcom_scm_mem.pool, size);
> +	if (!vaddr) {
> +		kfree(chunk);
> +		return NULL;
> +	}
> +
> +	chunk->paddr = gen_pool_virt_to_phys(qcom_scm_mem.pool,
> +					     (unsigned long)vaddr);

unnecessary typecast?

> +	chunk->size = size;
> +
> +	scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock) {

my first exposure to this infrastructure..very cool now that I've 
wrapped my head around it! This helped for those also new to this:
https://lwn.net/Articles/934679/

> +		ret = radix_tree_insert(&qcom_scm_mem.chunks, vaddr, chunk);
> +		if (ret) {
> +			gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr,

unnecessary typecast?

> +				      chunk->size);
> +			kfree(chunk);
> +			return NULL;
> +		}
> +	}
> +
> +	return (void *)vaddr;
> +}

Re: [PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory allocator

[Bartosz Golaszewski]

On Thu, Sep 28, 2023 at 8:19 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> > +void *qcom_scm_mem_alloc(size_t size, gfp_t gfp)
> > +{
> > +     struct qcom_scm_mem_chunk *chunk;
> > +     unsigned long vaddr;
>
> there are places below where you unnecessarily typecast this to its
> given type
>

Ah, it's a leftover from when this variable was of type void *. Thanks.

> > +     int ret;
> > +
> > +     if (!size)
> > +             return ZERO_SIZE_PTR;
> > +
> > +     size = roundup(size, 1 << PAGE_SHIFT);
> > +
> > +     chunk = kzalloc(sizeof(*chunk), gfp);
> > +     if (!chunk)
> > +             return NULL;
> > +
> > +     vaddr = gen_pool_alloc(qcom_scm_mem.pool, size);
> > +     if (!vaddr) {
> > +             kfree(chunk);
> > +             return NULL;
> > +     }
> > +
> > +     chunk->paddr = gen_pool_virt_to_phys(qcom_scm_mem.pool,
> > +                                          (unsigned long)vaddr);
>
> unnecessary typecast?
>
> > +     chunk->size = size;
> > +
> > +     scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock) {
>
> my first exposure to this infrastructure..very cool now that I've
> wrapped my head around it! This helped for those also new to this:
> https://lwn.net/Articles/934679/
>

It's amazing but be careful with it. I used it wrong in one place and
got yelled at by Linus Torvalds personally already. :)

Bartosz

> > +             ret = radix_tree_insert(&qcom_scm_mem.chunks, vaddr, chunk);
> > +             if (ret) {
> > +                     gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr,
>
> unnecessary typecast?
>
> > +                                   chunk->size);
> > +                     kfree(chunk);
> > +                     return NULL;
> > +             }
> > +     }
> > +
> > +     return (void *)vaddr;
> > +}
>

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Bartosz Golaszewski]

On Thu, Sep 28, 2023 at 7:10 PM Elliot Berman <quic_eberman@quicinc.com> wrote:
>
>
>
> On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Extens the SCM memory allocator with using the SHM Bridge feature if
> > available on the platform. This makes the trustzone only use dedicated
> > buffers for SCM calls. We map the entire SCM genpool as a bridge.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> > index eafecbe23770..12b12b15f46f 100644
> > --- a/drivers/firmware/qcom/qcom_scm-mem.c
> > +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> > @@ -16,6 +16,8 @@
> >
> >  #include "qcom_scm.h"
> >
> > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> > +
> >  static size_t qcom_scm_mem_pool_size = SZ_2M;
> >  module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> >                  ulong, 0400);
> > @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> >       return chunk->paddr;
> >  }
> >
> > +static int qcom_scm_mem_shm_bridge_create(void)
> > +{
> > +     uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> > +
> > +     ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> > +     pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > +     ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > +     size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> > +
> > +     return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> > +                                       ipfn_and_s_perm, size_and_flags,
> > +                                       QCOM_SCM_VMID_HLOS);
> > +}
> > +
> >  int qcom_scm_mem_enable(struct device *dev)
> >  {
> > +     int ret;
> > +
> >       INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> >       spin_lock_init(&qcom_scm_mem.lock);
> >       qcom_scm_mem.dev = dev;
> > @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
> >
> >       gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
> >
> > -     return gen_pool_add_virt(qcom_scm_mem.pool,
> > -                              (unsigned long)qcom_scm_mem.vbase,
> > -                              qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > +     ret = gen_pool_add_virt(qcom_scm_mem.pool,
> > +                             (unsigned long)qcom_scm_mem.vbase,
> > +                             qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = qcom_scm_enable_shm_bridge();
> > +     if (ret) {
> > +             if (ret == EOPNOTSUPP)
> > +                     dev_info(dev, "SHM Bridge not supported\n");
> > +             else
> > +                     return ret;
> > +     } else {
> > +             ret = qcom_scm_mem_shm_bridge_create();
> > +             if (ret)
> > +                     return ret;
> > +
> > +             dev_info(dev, "SHM Bridge enabled\n");
>
> Do you need to add clean up (deletion) of the SHM bridge on driver remove?
>
> One easy approach I could think: implemnet devm_qcom_scm_mem_shm_bridge_create
> which calls qcom_scm_delete_shm_bridge on the clean up
> (qcom_scm_delete_shm_bridge implemented in downstream, not in this series).
>

There wouldn't be any user of these symbols yet so let's think about
it when there's a valid use-case upstream.

Bart

> > +     }
> > +
> > +     return 0;
> >  }

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Jeff Johnson]

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Extens the SCM memory allocator with using the SHM Bridge feature if

nit: s/Extens/Extend/

Re: [PATCH v2 03/11] firmware: qcom: scm: switch to using the SCM allocator

[Elliot Berman]

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 SCM
> allocator for that memory and shrink the code in process.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>

> ---
>  drivers/firmware/qcom/qcom_scm-smc.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 16cf88acfa8e..0d5554df1321 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>
> @@ -152,8 +153,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  {
>  	int arglen = desc->arginfo & 0xf;
>  	int i, ret;
> -	dma_addr_t args_phys = 0;
> -	void *args_virt = NULL;
> +	void *args_virt __free(qcom_scm_mem) = NULL;
>  	size_t alloc_len;
>  	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>  	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
> @@ -173,7 +173,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  
>  	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_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
>  
>  		if (!args_virt)
>  			return -ENOMEM;
> @@ -192,25 +192,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_scm_mem_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;
>  

Re: [PATCH v2 03/11] firmware: qcom: scm: switch to using the SCM allocator

[Bartosz Golaszewski]

On Thu, Sep 28, 2023 at 11:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 SCM
> allocator for that memory and shrink the code in process.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm-smc.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 16cf88acfa8e..0d5554df1321 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>
> @@ -152,8 +153,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  {
>         int arglen = desc->arginfo & 0xf;
>         int i, ret;
> -       dma_addr_t args_phys = 0;
> -       void *args_virt = NULL;
> +       void *args_virt __free(qcom_scm_mem) = NULL;
>         size_t alloc_len;
>         gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>         u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
> @@ -173,7 +173,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>
>         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_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);

Ah, I realized the PAGE_ALIGN() here can be dropped when switching to
the new allocator.

There'll be a v3 anyway so I'll drop it then.

Bart

>
>                 if (!args_virt)
>                         return -ENOMEM;
> @@ -192,25 +192,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_scm_mem_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;
>
> --
> 2.39.2
>

Re: [PATCH v2 00/11] arm64: qcom: add and enable SHM Bridge support

[Andrew Halaney]

On Thu, Sep 28, 2023 at 11:20:29AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This is technically the second iteration of the SHM Bridge enablement on
> QCom platforms but in practice it's a complete rewrite.
>
> During the internal discussion with QCom the requirement has been established
> as an SHM Bridge pool manager with the assumption that there will be multiple
> users, each with their own pool. The problem with this is that we don't have
> many potential users of SHM pools upstream at the moment which was rightfully
> pointed out in the reviews under v1 (which even had some unused symbols etc.).
>
> Moreover: after some investigating, it turns out that there simply isn't any
> need for multiple pools as the core SCM only allocates a buffer if given call
> requires more than 4 arguments and there are only a handful of SCM calls that
> need to pass a physical address to a buffer as argument to the trustzone.
>
> Additionally all but one SCM call allocate buffers passed to the TZ only for
> the duration of the call and then free it right aftr it returns. The one
> exception is called once and the buffer it uses can remain in memory until
> released by the user.
>
> This all makes using multiple pools wasteful as most of that memory will be
> reserved but remain unused 99% of the time. What we need is a single pool of
> SCM memory that deals out chunks of suitable format (coherent and
> page-aligned) that fulfills the requirements of all calls.
>
> As not all architectures support SHM bridge, it makes sense to first unify the
> memory operations in SCM calls. All users do some kind of DMA mapping to obtain
> buffers, most using dma_alloc_coherent() which impacts performance.
>
> Genalloc pools are very fast so let's use them instead. Create a custom
> allocator that also deals with the mapping and use it across all SCM calls.
>
> Then once this is done, let's extend the allocator to use the SHM bridge
> functionality if available on the given platform.
>
> While at it: let's create a qcom specific directory in drivers/firmware/ and
> move relevant code in there.
>
> I couldn't test all SCM calls but tested with the inline crypto engine on
> sm8550 and sa8775p that runs most of new code paths (with and without SHM
> bridge). At least qseecom would need some Tested-by.

I have been playing around with this on my x13s (sc8280xp). It seems
that efivars works ok (and therefore the qseecom stuff I believe), but
with these patches applied I'm getting -22 when loading any firmware.

I'll try to dig a little more, but thought I'd report that before I
context switch for a little bit.

    dmesg | grep "firmware\|-22"
    [    0.000000] psci: PSCIv1.1 detected in firmware.
    [    2.351999] qcom_scm firmware:scm: SHM Bridge enabled
    [    2.353002] qcom_scm firmware:scm: qseecom: found qseecom with version 0x1402000
    [    6.727604] systemd[1]: systemd-pcrmachine.service - TPM2 PCR Machine ID Measurement was skipped because of an unmet condition check (ConditionPathExists=/sys/firmware/efi/efivars/StubPcrKernelImage-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f).
    [    8.198381] qcom_q6v5_pas 1b300000.remoteproc: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn
    [    8.198418] remoteproc remoteproc1: can't start rproc 1b300000.remoteproc: -22
    [    8.407641] qcom_q6v5_pas 3000000.remoteproc: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcadsp8280.mbn
    [    8.407742] remoteproc remoteproc0: can't start rproc 3000000.remoteproc: -22
    [    8.588496] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [    8.588509] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [    9.392185] ath11k_pci 0006:01:00.0: fw_version 0x110b196e fw_build_timestamp 2022-12-22 12:54 fw_build_id WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
    [   10.229367] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   10.229383] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.041385] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.041399] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.070144] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.070160] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.321999] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.322015] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.340989] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.341002] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.576180] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.576198] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.593647] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.593661] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.854212] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.854226] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   11.879925] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   11.879937] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   12.157090] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   12.157106] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
    [   12.183074] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
    [   12.183088] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22

Thanks,
Andrew
>
> v1 -> v2:
> - too many changes to list, it's a complete rewrite as explained above
>
> Bartosz Golaszewski (11):
>   firmware: qcom: move Qualcomm code into its own directory
>   firmware: qcom: scm: add a dedicated SCM memory allocator
>   firmware: qcom: scm: switch to using the SCM allocator
>   firmware: qcom: scm: make qcom_scm_assign_mem() use the SCM allocator
>   firmware: qcom: scm: make qcom_scm_ice_set_key() use the SCM allocator
>   firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM
>     allocator
>   firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the SCM allocator
>   firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the SCM
>     allocator
>   firmware: qcom: qseecom: convert to using the SCM allocator
>   firmware: qcom-scm: add support for SHM bridge operations
>   firmware: qcom: scm: enable SHM bridge
>
>  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      | 251 ++++++------------
>  drivers/firmware/{ => qcom}/qcom_scm-legacy.c |   0
>  drivers/firmware/qcom/qcom_scm-mem.c          | 170 ++++++++++++
>  drivers/firmware/{ => qcom}/qcom_scm-smc.c    |  21 +-
>  drivers/firmware/{ => qcom}/qcom_scm.c        | 149 ++++++-----
>  drivers/firmware/{ => qcom}/qcom_scm.h        |   9 +
>  include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
>  include/linux/firmware/qcom/qcom_scm.h        |  13 +
>  14 files changed, 427 insertions(+), 312 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%)
>  create mode 100644 drivers/firmware/qcom/qcom_scm-mem.c
>  rename drivers/firmware/{ => qcom}/qcom_scm-smc.c (92%)
>  rename drivers/firmware/{ => qcom}/qcom_scm.c (94%)
>  rename drivers/firmware/{ => qcom}/qcom_scm.h (95%)
>
> --
> 2.39.2
>

Re: [PATCH v2 00/11] arm64: qcom: add and enable SHM Bridge support

[Bartosz Golaszewski]

On Fri, Sep 29, 2023 at 5:29 PM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Thu, Sep 28, 2023 at 11:20:29AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > This is technically the second iteration of the SHM Bridge enablement on
> > QCom platforms but in practice it's a complete rewrite.
> >
> > During the internal discussion with QCom the requirement has been established
> > as an SHM Bridge pool manager with the assumption that there will be multiple
> > users, each with their own pool. The problem with this is that we don't have
> > many potential users of SHM pools upstream at the moment which was rightfully
> > pointed out in the reviews under v1 (which even had some unused symbols etc.).
> >
> > Moreover: after some investigating, it turns out that there simply isn't any
> > need for multiple pools as the core SCM only allocates a buffer if given call
> > requires more than 4 arguments and there are only a handful of SCM calls that
> > need to pass a physical address to a buffer as argument to the trustzone.
> >
> > Additionally all but one SCM call allocate buffers passed to the TZ only for
> > the duration of the call and then free it right aftr it returns. The one
> > exception is called once and the buffer it uses can remain in memory until
> > released by the user.
> >
> > This all makes using multiple pools wasteful as most of that memory will be
> > reserved but remain unused 99% of the time. What we need is a single pool of
> > SCM memory that deals out chunks of suitable format (coherent and
> > page-aligned) that fulfills the requirements of all calls.
> >
> > As not all architectures support SHM bridge, it makes sense to first unify the
> > memory operations in SCM calls. All users do some kind of DMA mapping to obtain
> > buffers, most using dma_alloc_coherent() which impacts performance.
> >
> > Genalloc pools are very fast so let's use them instead. Create a custom
> > allocator that also deals with the mapping and use it across all SCM calls.
> >
> > Then once this is done, let's extend the allocator to use the SHM bridge
> > functionality if available on the given platform.
> >
> > While at it: let's create a qcom specific directory in drivers/firmware/ and
> > move relevant code in there.
> >
> > I couldn't test all SCM calls but tested with the inline crypto engine on
> > sm8550 and sa8775p that runs most of new code paths (with and without SHM
> > bridge). At least qseecom would need some Tested-by.
>
> I have been playing around with this on my x13s (sc8280xp). It seems
> that efivars works ok (and therefore the qseecom stuff I believe), but
> with these patches applied I'm getting -22 when loading any firmware.
>
> I'll try to dig a little more, but thought I'd report that before I
> context switch for a little bit.
>
>     dmesg | grep "firmware\|-22"
>     [    0.000000] psci: PSCIv1.1 detected in firmware.
>     [    2.351999] qcom_scm firmware:scm: SHM Bridge enabled
>     [    2.353002] qcom_scm firmware:scm: qseecom: found qseecom with version 0x1402000
>     [    6.727604] systemd[1]: systemd-pcrmachine.service - TPM2 PCR Machine ID Measurement was skipped because of an unmet condition check (ConditionPathExists=/sys/firmware/efi/efivars/StubPcrKernelImage-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f).
>     [    8.198381] qcom_q6v5_pas 1b300000.remoteproc: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn
>     [    8.198418] remoteproc remoteproc1: can't start rproc 1b300000.remoteproc: -22
>     [    8.407641] qcom_q6v5_pas 3000000.remoteproc: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcadsp8280.mbn
>     [    8.407742] remoteproc remoteproc0: can't start rproc 3000000.remoteproc: -22
>     [    8.588496] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [    8.588509] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [    9.392185] ath11k_pci 0006:01:00.0: fw_version 0x110b196e fw_build_timestamp 2022-12-22 12:54 fw_build_id WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
>     [   10.229367] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   10.229383] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.041385] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.041399] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.070144] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.070160] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.321999] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.322015] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.340989] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.341002] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.576180] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.576198] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.593647] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.593661] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.854212] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.854226] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   11.879925] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   11.879937] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   12.157090] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   12.157106] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>     [   12.183074] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
>     [   12.183088] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
>
> Thanks,
> Andrew

Huh remoteproc seems to work fine on sm8550. Can you post the full
kernel log? Do you know which SCM calls fails?

Bart

> >
> > v1 -> v2:
> > - too many changes to list, it's a complete rewrite as explained above
> >
> > Bartosz Golaszewski (11):
> >   firmware: qcom: move Qualcomm code into its own directory
> >   firmware: qcom: scm: add a dedicated SCM memory allocator
> >   firmware: qcom: scm: switch to using the SCM allocator
> >   firmware: qcom: scm: make qcom_scm_assign_mem() use the SCM allocator
> >   firmware: qcom: scm: make qcom_scm_ice_set_key() use the SCM allocator
> >   firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM
> >     allocator
> >   firmware: qcom: scm: make qcom_scm_lmh_dcvsh() use the SCM allocator
> >   firmware: qcom: scm: make qcom_scm_qseecom_app_get_id() use the SCM
> >     allocator
> >   firmware: qcom: qseecom: convert to using the SCM allocator
> >   firmware: qcom-scm: add support for SHM bridge operations
> >   firmware: qcom: scm: enable SHM bridge
> >
> >  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      | 251 ++++++------------
> >  drivers/firmware/{ => qcom}/qcom_scm-legacy.c |   0
> >  drivers/firmware/qcom/qcom_scm-mem.c          | 170 ++++++++++++
> >  drivers/firmware/{ => qcom}/qcom_scm-smc.c    |  21 +-
> >  drivers/firmware/{ => qcom}/qcom_scm.c        | 149 ++++++-----
> >  drivers/firmware/{ => qcom}/qcom_scm.h        |   9 +
> >  include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
> >  include/linux/firmware/qcom/qcom_scm.h        |  13 +
> >  14 files changed, 427 insertions(+), 312 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%)
> >  create mode 100644 drivers/firmware/qcom/qcom_scm-mem.c
> >  rename drivers/firmware/{ => qcom}/qcom_scm-smc.c (92%)
> >  rename drivers/firmware/{ => qcom}/qcom_scm.c (94%)
> >  rename drivers/firmware/{ => qcom}/qcom_scm.h (95%)
> >
> > --
> > 2.39.2
> >
>

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Bartosz Golaszewski]

On Thu, Sep 28, 2023 at 11:21 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Extens the SCM memory allocator with using the SHM Bridge feature if
> available on the platform. This makes the trustzone only use dedicated
> buffers for SCM calls. We map the entire SCM genpool as a bridge.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> index eafecbe23770..12b12b15f46f 100644
> --- a/drivers/firmware/qcom/qcom_scm-mem.c
> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> @@ -16,6 +16,8 @@
>
>  #include "qcom_scm.h"
>
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
>  static size_t qcom_scm_mem_pool_size = SZ_2M;
>  module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
>                    ulong, 0400);
> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
>         return chunk->paddr;
>  }
>
> +static int qcom_scm_mem_shm_bridge_create(void)
> +{
> +       uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> +
> +       ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> +       pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> +       ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> +       size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> +       return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> +                                         ipfn_and_s_perm, size_and_flags,
> +                                         QCOM_SCM_VMID_HLOS);
> +}
> +
>  int qcom_scm_mem_enable(struct device *dev)
>  {
> +       int ret;
> +
>         INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
>         spin_lock_init(&qcom_scm_mem.lock);
>         qcom_scm_mem.dev = dev;
> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>
>         gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>
> -       return gen_pool_add_virt(qcom_scm_mem.pool,
> -                                (unsigned long)qcom_scm_mem.vbase,
> -                                qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> +       ret = gen_pool_add_virt(qcom_scm_mem.pool,
> +                               (unsigned long)qcom_scm_mem.vbase,
> +                               qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> +       if (ret)
> +               return ret;
> +
> +       ret = qcom_scm_enable_shm_bridge();
> +       if (ret) {
> +               if (ret == EOPNOTSUPP)

FYI I noticed this is wrong, I will fix it in v3.

Bart

> +                       dev_info(dev, "SHM Bridge not supported\n");
> +               else
> +                       return ret;
> +       } else {
> +               ret = qcom_scm_mem_shm_bridge_create();
> +               if (ret)
> +                       return ret;
> +
> +               dev_info(dev, "SHM Bridge enabled\n");
> +       }
> +
> +       return 0;
>  }
> --
> 2.39.2
>

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Andrew Halaney]

On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Let's use the new SCM 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 | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 02a773ba1383..c0eb81069847 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>  			    struct qcom_scm_pas_metadata *ctx)
>  {
> -	dma_addr_t mdata_phys;
> +	phys_addr_t mdata_phys;

>  	void *mdata_buf;
>  	int ret;
>  	struct qcom_scm_desc desc = {
> @@ -544,13 +544,7 @@ 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.
> -	 */
> -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> -				       GFP_KERNEL);
> +	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);

mdata_phys is never initialized now, and its what's being shoved into
desc.args[1] later, which I believe is what triggered the -EINVAL
with qcom_scm_call() that I reported in my cover letter reply this
morning.

Prior with the DMA API that would have been the device view of the buffer.

>  	if (!mdata_buf) {
>  		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
>  		return -ENOMEM;
> @@ -574,10 +568,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>  
>  out:
>  	if (ret < 0 || !ctx) {
> -		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> +		qcom_scm_mem_free(mdata_buf);
>  	} else if (ctx) {
>  		ctx->ptr = mdata_buf;
> -		ctx->phys = mdata_phys;
> +		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
>  		ctx->size = size;
>  	}
>  
> @@ -594,7 +588,7 @@ void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
>  	if (!ctx->ptr)
>  		return;
>  
> -	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> +	qcom_scm_mem_free(ctx->ptr);
>  
>  	ctx->ptr = NULL;
>  	ctx->phys = 0;
> -- 
> 2.39.2
> 

Re: [PATCH v2 00/11] arm64: qcom: add and enable SHM Bridge support

[Andrew Halaney]

On Fri, Sep 29, 2023 at 08:56:57PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 29, 2023 at 5:29 PM Andrew Halaney <ahalaney@redhat.com> wrote:
> >
> > On Thu, Sep 28, 2023 at 11:20:29AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > This is technically the second iteration of the SHM Bridge enablement on
> > > QCom platforms but in practice it's a complete rewrite.
> > >
> > > During the internal discussion with QCom the requirement has been established
> > > as an SHM Bridge pool manager with the assumption that there will be multiple
> > > users, each with their own pool. The problem with this is that we don't have
> > > many potential users of SHM pools upstream at the moment which was rightfully
> > > pointed out in the reviews under v1 (which even had some unused symbols etc.).
> > >
> > > Moreover: after some investigating, it turns out that there simply isn't any
> > > need for multiple pools as the core SCM only allocates a buffer if given call
> > > requires more than 4 arguments and there are only a handful of SCM calls that
> > > need to pass a physical address to a buffer as argument to the trustzone.
> > >
> > > Additionally all but one SCM call allocate buffers passed to the TZ only for
> > > the duration of the call and then free it right aftr it returns. The one
> > > exception is called once and the buffer it uses can remain in memory until
> > > released by the user.
> > >
> > > This all makes using multiple pools wasteful as most of that memory will be
> > > reserved but remain unused 99% of the time. What we need is a single pool of
> > > SCM memory that deals out chunks of suitable format (coherent and
> > > page-aligned) that fulfills the requirements of all calls.
> > >
> > > As not all architectures support SHM bridge, it makes sense to first unify the
> > > memory operations in SCM calls. All users do some kind of DMA mapping to obtain
> > > buffers, most using dma_alloc_coherent() which impacts performance.
> > >
> > > Genalloc pools are very fast so let's use them instead. Create a custom
> > > allocator that also deals with the mapping and use it across all SCM calls.
> > >
> > > Then once this is done, let's extend the allocator to use the SHM bridge
> > > functionality if available on the given platform.
> > >
> > > While at it: let's create a qcom specific directory in drivers/firmware/ and
> > > move relevant code in there.
> > >
> > > I couldn't test all SCM calls but tested with the inline crypto engine on
> > > sm8550 and sa8775p that runs most of new code paths (with and without SHM
> > > bridge). At least qseecom would need some Tested-by.
> >
> > I have been playing around with this on my x13s (sc8280xp). It seems
> > that efivars works ok (and therefore the qseecom stuff I believe), but
> > with these patches applied I'm getting -22 when loading any firmware.
> >
> > I'll try to dig a little more, but thought I'd report that before I
> > context switch for a little bit.
> >
> >     dmesg | grep "firmware\|-22"
> >     [    0.000000] psci: PSCIv1.1 detected in firmware.
> >     [    2.351999] qcom_scm firmware:scm: SHM Bridge enabled
> >     [    2.353002] qcom_scm firmware:scm: qseecom: found qseecom with version 0x1402000
> >     [    6.727604] systemd[1]: systemd-pcrmachine.service - TPM2 PCR Machine ID Measurement was skipped because of an unmet condition check (ConditionPathExists=/sys/firmware/efi/efivars/StubPcrKernelImage-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f).
> >     [    8.198381] qcom_q6v5_pas 1b300000.remoteproc: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn
> >     [    8.198418] remoteproc remoteproc1: can't start rproc 1b300000.remoteproc: -22
> >     [    8.407641] qcom_q6v5_pas 3000000.remoteproc: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcadsp8280.mbn
> >     [    8.407742] remoteproc remoteproc0: can't start rproc 3000000.remoteproc: -22
> >     [    8.588496] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [    8.588509] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [    9.392185] ath11k_pci 0006:01:00.0: fw_version 0x110b196e fw_build_timestamp 2022-12-22 12:54 fw_build_id WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23
> >     [   10.229367] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   10.229383] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.041385] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.041399] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.070144] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.070160] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.321999] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.322015] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.340989] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.341002] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.576180] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.576198] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.593647] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.593661] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.854212] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.854226] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   11.879925] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   11.879937] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   12.157090] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   12.157106] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >     [   12.183074] adreno 3d00000.gpu: error -22 initializing firmware qcom/sc8280xp/LENOVO/21BX/qcdxkmsuc8280.mbn
> >     [   12.183088] msm_dpu ae01000.display-controller: [drm:adreno_load_gpu [msm]] *ERROR* gpu hw init failed: -22
> >
> > Thanks,
> > Andrew
> 
> Huh remoteproc seems to work fine on sm8550. Can you post the full
> kernel log? Do you know which SCM calls fails?
> 
> Bart

See my response on patch 6, and please let me know if you I messed up
the logic there and need to dig a little more. The log didn't have
anything useful outside what is shown here unfortunately, but I can
gather more if you refute that comment on patch 6.

Thanks,
Andrew

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Bartosz Golaszewski]

On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said:
> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> Let's use the new SCM 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 | 16 +++++-----------
>>  1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 02a773ba1383..c0eb81069847 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>>  			    struct qcom_scm_pas_metadata *ctx)
>>  {
>> -	dma_addr_t mdata_phys;
>> +	phys_addr_t mdata_phys;
>
>>  	void *mdata_buf;
>>  	int ret;
>>  	struct qcom_scm_desc desc = {
>> @@ -544,13 +544,7 @@ 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.
>> -	 */
>> -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
>> -				       GFP_KERNEL);
>> +	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
>
> mdata_phys is never initialized now, and its what's being shoved into
> desc.args[1] later, which I believe is what triggered the -EINVAL
> with qcom_scm_call() that I reported in my cover letter reply this
> morning.
>
> Prior with the DMA API that would have been the device view of the buffer.
>

Gah! Thanks for finding this.

Can you try the following diff?

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 794388c3212f..b0d4ea237034 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
void *metadata, size_t size,
 		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
 		return -ENOMEM;
 	}
+	mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
 	memcpy(mdata_buf, metadata, size);

 	ret = qcom_scm_clk_enable();
@@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
void *metadata, size_t size,
 		qcom_scm_mem_free(mdata_buf);
 	} else if (ctx) {
 		ctx->ptr = mdata_buf;
-		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
+		ctx->phys = mdata_phys;
 		ctx->size = size;
 	}

Bart

>>  	if (!mdata_buf) {
>>  		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
>>  		return -ENOMEM;
>> @@ -574,10 +568,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>>
>>  out:
>>  	if (ret < 0 || !ctx) {
>> -		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
>> +		qcom_scm_mem_free(mdata_buf);
>>  	} else if (ctx) {
>>  		ctx->ptr = mdata_buf;
>> -		ctx->phys = mdata_phys;
>> +		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
>>  		ctx->size = size;
>>  	}
>>
>> @@ -594,7 +588,7 @@ void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
>>  	if (!ctx->ptr)
>>  		return;
>>
>> -	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
>> +	qcom_scm_mem_free(ctx->ptr);
>>
>>  	ctx->ptr = NULL;
>>  	ctx->phys = 0;
>> --
>> 2.39.2
>>
>
>

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Andrew Halaney]

On Fri, Sep 29, 2023 at 12:22:16PM -0700, Bartosz Golaszewski wrote:
> On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said:
> > On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
> >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>
> >> Let's use the new SCM 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 | 16 +++++-----------
> >>  1 file changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> >> index 02a773ba1383..c0eb81069847 100644
> >> --- a/drivers/firmware/qcom/qcom_scm.c
> >> +++ b/drivers/firmware/qcom/qcom_scm.c
> >> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
> >>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> >>  			    struct qcom_scm_pas_metadata *ctx)
> >>  {
> >> -	dma_addr_t mdata_phys;
> >> +	phys_addr_t mdata_phys;
> >
> >>  	void *mdata_buf;
> >>  	int ret;
> >>  	struct qcom_scm_desc desc = {
> >> @@ -544,13 +544,7 @@ 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.
> >> -	 */
> >> -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> >> -				       GFP_KERNEL);
> >> +	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
> >
> > mdata_phys is never initialized now, and its what's being shoved into
> > desc.args[1] later, which I believe is what triggered the -EINVAL
> > with qcom_scm_call() that I reported in my cover letter reply this
> > morning.
> >
> > Prior with the DMA API that would have been the device view of the buffer.
> >
> 
> Gah! Thanks for finding this.
> 
> Can you try the following diff?
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 794388c3212f..b0d4ea237034 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> void *metadata, size_t size,
>  		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
>  		return -ENOMEM;
>  	}
> +	mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
>  	memcpy(mdata_buf, metadata, size);
> 
>  	ret = qcom_scm_clk_enable();
> @@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> void *metadata, size_t size,
>  		qcom_scm_mem_free(mdata_buf);
>  	} else if (ctx) {
>  		ctx->ptr = mdata_buf;
> -		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
> +		ctx->phys = mdata_phys;
>  		ctx->size = size;
>  	}
> 
> Bart
> 

For some reason that I can't explain that is still not working. It seems
the SMC call is returning !0 and then we return -EINVAL from there
with qcom_scm_remap_error().

Here's a really crummy diff of what I hacked in during lunch to debug (don't
judge my primitive debug skills):

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 0d5554df1321..56eab0ae5f3a 100644
--- a/drivers/firmware/qcom/qcom_scm-smc.c
+++ b/drivers/firmware/qcom/qcom_scm-smc.c
@@ -162,6 +162,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
        struct arm_smccc_res smc_res;
        struct arm_smccc_args smc = {0};
 
+       dev_err(dev, "%s: %d: We are in this function\n", __func__, __LINE__);
+
        smc.args[0] = ARM_SMCCC_CALL_VAL(
                smccc_call_type,
                qcom_smccc_convention,
@@ -174,6 +176,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
        if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
                alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
                args_virt = qcom_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
+               dev_err(dev, "%s: %d: Hit the unlikely case!\n", __func__, __LINE__);
 
                if (!args_virt)
                        return -ENOMEM;
@@ -197,6 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 
        /* ret error check follows after args_virt cleanup*/
        ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
+       dev_err(dev, "%s: %d: ret: %d\n", __func__, __LINE__, ret);
 
        if (ret)
                return ret;
@@ -205,8 +209,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
                res->result[0] = smc_res.a1;
                res->result[1] = smc_res.a2;
                res->result[2] = smc_res.a3;
+               dev_err(dev, "%s: %d: 0: %llu, 1: %llu: 2: %llu\n", __func__, __LINE__, res->result[0], res->result[1], res->result[2]);
        }
 
+       dev_err(dev, "%s: %d: smc_res.a0: %lu\n", __func__, __LINE__, smc_res.a0);
        return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;


And that all spams dmesg successfully for most cases, but the
pas_init_image calls log this out:

[   16.362965] remoteproc remoteproc1: powering up 1b300000.remoteproc
[   16.364897] remoteproc remoteproc1: Booting fw image qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn, size 3575808
[   16.365009] qcom_scm firmware:scm: __scm_smc_call: 165: We are in this function
[   16.365251] qcom_scm firmware:scm: __scm_smc_call: 203: ret: 0
[   16.365256] qcom_scm firmware:scm: __scm_smc_call: 212: 0: 0, 1: 0: 2: 0
[   16.365261] qcom_scm firmware:scm: __scm_smc_call: 215: smc_res.a0: 4291821558

At the moment I am unsure why...

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Elliot Berman]

Hi Andrew,

On 9/29/2023 1:44 PM, Andrew Halaney wrote:
> On Fri, Sep 29, 2023 at 12:22:16PM -0700, Bartosz Golaszewski wrote:
>> On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said:
>>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> Let's use the new SCM 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 | 16 +++++-----------
>>>>  1 file changed, 5 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>>>> index 02a773ba1383..c0eb81069847 100644
>>>> --- a/drivers/firmware/qcom/qcom_scm.c
>>>> +++ b/drivers/firmware/qcom/qcom_scm.c
>>>> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>>>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>>>>  			    struct qcom_scm_pas_metadata *ctx)
>>>>  {
>>>> -	dma_addr_t mdata_phys;
>>>> +	phys_addr_t mdata_phys;
>>>
>>>>  	void *mdata_buf;
>>>>  	int ret;
>>>>  	struct qcom_scm_desc desc = {
>>>> @@ -544,13 +544,7 @@ 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.
>>>> -	 */
>>>> -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
>>>> -				       GFP_KERNEL);
>>>> +	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
>>>
>>> mdata_phys is never initialized now, and its what's being shoved into
>>> desc.args[1] later, which I believe is what triggered the -EINVAL
>>> with qcom_scm_call() that I reported in my cover letter reply this
>>> morning.
>>>
>>> Prior with the DMA API that would have been the device view of the buffer.
>>>
>>
>> Gah! Thanks for finding this.
>>
>> Can you try the following diff?
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 794388c3212f..b0d4ea237034 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
>> void *metadata, size_t size,
>>  		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
>>  		return -ENOMEM;
>>  	}
>> +	mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
>>  	memcpy(mdata_buf, metadata, size);
>>
>>  	ret = qcom_scm_clk_enable();
>> @@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
>> void *metadata, size_t size,
>>  		qcom_scm_mem_free(mdata_buf);
>>  	} else if (ctx) {
>>  		ctx->ptr = mdata_buf;
>> -		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
>> +		ctx->phys = mdata_phys;
>>  		ctx->size = size;
>>  	}
>>
>> Bart
>>
> 
> For some reason that I can't explain that is still not working. It seems
> the SMC call is returning !0 and then we return -EINVAL from there
> with qcom_scm_remap_error().
> 
> Here's a really crummy diff of what I hacked in during lunch to debug (don't
> judge my primitive debug skills):
> 

I don't know what you're talking about :-)

> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 0d5554df1321..56eab0ae5f3a 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> @@ -162,6 +162,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>         struct arm_smccc_res smc_res;
>         struct arm_smccc_args smc = {0};
>  
> +       dev_err(dev, "%s: %d: We are in this function\n", __func__, __LINE__);
> +
>         smc.args[0] = ARM_SMCCC_CALL_VAL(
>                 smccc_call_type,
>                 qcom_smccc_convention,
> @@ -174,6 +176,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>         if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
>                 alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
>                 args_virt = qcom_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
> +               dev_err(dev, "%s: %d: Hit the unlikely case!\n", __func__, __LINE__);
>  
>                 if (!args_virt)
>                         return -ENOMEM;
> @@ -197,6 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  
>         /* ret error check follows after args_virt cleanup*/
>         ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> +       dev_err(dev, "%s: %d: ret: %d\n", __func__, __LINE__, ret);
>  
>         if (ret)
>                 return ret;
> @@ -205,8 +209,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>                 res->result[0] = smc_res.a1;
>                 res->result[1] = smc_res.a2;
>                 res->result[2] = smc_res.a3;
> +               dev_err(dev, "%s: %d: 0: %llu, 1: %llu: 2: %llu\n", __func__, __LINE__, res->result[0], res->result[1], res->result[2]);
>         }
>  
> +       dev_err(dev, "%s: %d: smc_res.a0: %lu\n", __func__, __LINE__, smc_res.a0);
>         return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> 
> 
> And that all spams dmesg successfully for most cases, but the
> pas_init_image calls log this out:
> 
> [   16.362965] remoteproc remoteproc1: powering up 1b300000.remoteproc
> [   16.364897] remoteproc remoteproc1: Booting fw image qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn, size 3575808
> [   16.365009] qcom_scm firmware:scm: __scm_smc_call: 165: We are in this function
> [   16.365251] qcom_scm firmware:scm: __scm_smc_call: 203: ret: 0
> [   16.365256] qcom_scm firmware:scm: __scm_smc_call: 212: 0: 0, 1: 0: 2: 0
> [   16.365261] qcom_scm firmware:scm: __scm_smc_call: 215: smc_res.a0: 4291821558
> 
> At the moment I am unsure why...
> 
Does the issue appear right after taking patch 6 or does it only appear after taking
the whole series? If it's just to this patch, then maybe something wrong with
the refactor: shm bridge isn't enabled at this point in the series.

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Andrew Halaney]

On Fri, Sep 29, 2023 at 03:48:37PM -0700, Elliot Berman wrote:
> Hi Andrew,
> 
> On 9/29/2023 1:44 PM, Andrew Halaney wrote:
> > On Fri, Sep 29, 2023 at 12:22:16PM -0700, Bartosz Golaszewski wrote:
> >> On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said:
> >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>
> >>>> Let's use the new SCM 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 | 16 +++++-----------
> >>>>  1 file changed, 5 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> >>>> index 02a773ba1383..c0eb81069847 100644
> >>>> --- a/drivers/firmware/qcom/qcom_scm.c
> >>>> +++ b/drivers/firmware/qcom/qcom_scm.c
> >>>> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
> >>>>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> >>>>  			    struct qcom_scm_pas_metadata *ctx)
> >>>>  {
> >>>> -	dma_addr_t mdata_phys;
> >>>> +	phys_addr_t mdata_phys;
> >>>
> >>>>  	void *mdata_buf;
> >>>>  	int ret;
> >>>>  	struct qcom_scm_desc desc = {
> >>>> @@ -544,13 +544,7 @@ 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.
> >>>> -	 */
> >>>> -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> >>>> -				       GFP_KERNEL);
> >>>> +	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
> >>>
> >>> mdata_phys is never initialized now, and its what's being shoved into
> >>> desc.args[1] later, which I believe is what triggered the -EINVAL
> >>> with qcom_scm_call() that I reported in my cover letter reply this
> >>> morning.
> >>>
> >>> Prior with the DMA API that would have been the device view of the buffer.
> >>>
> >>
> >> Gah! Thanks for finding this.
> >>
> >> Can you try the following diff?
> >>
> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> >> index 794388c3212f..b0d4ea237034 100644
> >> --- a/drivers/firmware/qcom/qcom_scm.c
> >> +++ b/drivers/firmware/qcom/qcom_scm.c
> >> @@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> >> void *metadata, size_t size,
> >>  		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> >>  		return -ENOMEM;
> >>  	}
> >> +	mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
> >>  	memcpy(mdata_buf, metadata, size);
> >>
> >>  	ret = qcom_scm_clk_enable();
> >> @@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> >> void *metadata, size_t size,
> >>  		qcom_scm_mem_free(mdata_buf);
> >>  	} else if (ctx) {
> >>  		ctx->ptr = mdata_buf;
> >> -		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
> >> +		ctx->phys = mdata_phys;
> >>  		ctx->size = size;
> >>  	}
> >>
> >> Bart
> >>
> > 
> > For some reason that I can't explain that is still not working. It seems
> > the SMC call is returning !0 and then we return -EINVAL from there
> > with qcom_scm_remap_error().
> > 
> > Here's a really crummy diff of what I hacked in during lunch to debug (don't
> > judge my primitive debug skills):
> > 
> 
> I don't know what you're talking about :-)
> 
> > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > index 0d5554df1321..56eab0ae5f3a 100644
> > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > @@ -162,6 +162,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> >         struct arm_smccc_res smc_res;
> >         struct arm_smccc_args smc = {0};
> >  
> > +       dev_err(dev, "%s: %d: We are in this function\n", __func__, __LINE__);
> > +
> >         smc.args[0] = ARM_SMCCC_CALL_VAL(
> >                 smccc_call_type,
> >                 qcom_smccc_convention,
> > @@ -174,6 +176,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> >         if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> >                 alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
> >                 args_virt = qcom_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
> > +               dev_err(dev, "%s: %d: Hit the unlikely case!\n", __func__, __LINE__);
> >  
> >                 if (!args_virt)
> >                         return -ENOMEM;
> > @@ -197,6 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> >  
> >         /* ret error check follows after args_virt cleanup*/
> >         ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> > +       dev_err(dev, "%s: %d: ret: %d\n", __func__, __LINE__, ret);
> >  
> >         if (ret)
> >                 return ret;
> > @@ -205,8 +209,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> >                 res->result[0] = smc_res.a1;
> >                 res->result[1] = smc_res.a2;
> >                 res->result[2] = smc_res.a3;
> > +               dev_err(dev, "%s: %d: 0: %llu, 1: %llu: 2: %llu\n", __func__, __LINE__, res->result[0], res->result[1], res->result[2]);
> >         }
> >  
> > +       dev_err(dev, "%s: %d: smc_res.a0: %lu\n", __func__, __LINE__, smc_res.a0);
> >         return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> > 
> > 
> > And that all spams dmesg successfully for most cases, but the
> > pas_init_image calls log this out:
> > 
> > [   16.362965] remoteproc remoteproc1: powering up 1b300000.remoteproc
> > [   16.364897] remoteproc remoteproc1: Booting fw image qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn, size 3575808
> > [   16.365009] qcom_scm firmware:scm: __scm_smc_call: 165: We are in this function
> > [   16.365251] qcom_scm firmware:scm: __scm_smc_call: 203: ret: 0
> > [   16.365256] qcom_scm firmware:scm: __scm_smc_call: 212: 0: 0, 1: 0: 2: 0
> > [   16.365261] qcom_scm firmware:scm: __scm_smc_call: 215: smc_res.a0: 4291821558
> > 
> > At the moment I am unsure why...
> > 
> Does the issue appear right after taking patch 6 or does it only appear after taking
> the whole series? If it's just to this patch, then maybe something wrong with
> the refactor: shm bridge isn't enabled at this point in the series.
> 

I've only been testing the series as a whole on top of a 6.6 based
branch, I'm going to try and test some more today to see if just the
allocator bits (but not the SHM bridge enablement) works alright for
me.

Thanks,
Andrew

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Andrew Halaney]

On Mon, Oct 02, 2023 at 08:24:09AM -0500, Andrew Halaney wrote:
> On Fri, Sep 29, 2023 at 03:48:37PM -0700, Elliot Berman wrote:
> > Hi Andrew,
> > 
> > On 9/29/2023 1:44 PM, Andrew Halaney wrote:
> > > On Fri, Sep 29, 2023 at 12:22:16PM -0700, Bartosz Golaszewski wrote:
> > >> On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said:
> > >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
> > >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >>>>
> > >>>> Let's use the new SCM 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 | 16 +++++-----------
> > >>>>  1 file changed, 5 insertions(+), 11 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > >>>> index 02a773ba1383..c0eb81069847 100644
> > >>>> --- a/drivers/firmware/qcom/qcom_scm.c
> > >>>> +++ b/drivers/firmware/qcom/qcom_scm.c
> > >>>> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
> > >>>>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > >>>>  			    struct qcom_scm_pas_metadata *ctx)
> > >>>>  {
> > >>>> -	dma_addr_t mdata_phys;
> > >>>> +	phys_addr_t mdata_phys;
> > >>>
> > >>>>  	void *mdata_buf;
> > >>>>  	int ret;
> > >>>>  	struct qcom_scm_desc desc = {
> > >>>> @@ -544,13 +544,7 @@ 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.
> > >>>> -	 */
> > >>>> -	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> > >>>> -				       GFP_KERNEL);
> > >>>> +	mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
> > >>>
> > >>> mdata_phys is never initialized now, and its what's being shoved into
> > >>> desc.args[1] later, which I believe is what triggered the -EINVAL
> > >>> with qcom_scm_call() that I reported in my cover letter reply this
> > >>> morning.
> > >>>
> > >>> Prior with the DMA API that would have been the device view of the buffer.
> > >>>
> > >>
> > >> Gah! Thanks for finding this.
> > >>
> > >> Can you try the following diff?
> > >>
> > >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > >> index 794388c3212f..b0d4ea237034 100644
> > >> --- a/drivers/firmware/qcom/qcom_scm.c
> > >> +++ b/drivers/firmware/qcom/qcom_scm.c
> > >> @@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> > >> void *metadata, size_t size,
> > >>  		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> > >>  		return -ENOMEM;
> > >>  	}
> > >> +	mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
> > >>  	memcpy(mdata_buf, metadata, size);
> > >>
> > >>  	ret = qcom_scm_clk_enable();
> > >> @@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> > >> void *metadata, size_t size,
> > >>  		qcom_scm_mem_free(mdata_buf);
> > >>  	} else if (ctx) {
> > >>  		ctx->ptr = mdata_buf;
> > >> -		ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
> > >> +		ctx->phys = mdata_phys;
> > >>  		ctx->size = size;
> > >>  	}
> > >>
> > >> Bart
> > >>
> > > 
> > > For some reason that I can't explain that is still not working. It seems
> > > the SMC call is returning !0 and then we return -EINVAL from there
> > > with qcom_scm_remap_error().
> > > 
> > > Here's a really crummy diff of what I hacked in during lunch to debug (don't
> > > judge my primitive debug skills):
> > > 
> > 
> > I don't know what you're talking about :-)
> > 
> > > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > > index 0d5554df1321..56eab0ae5f3a 100644
> > > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > > @@ -162,6 +162,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > >         struct arm_smccc_res smc_res;
> > >         struct arm_smccc_args smc = {0};
> > >  
> > > +       dev_err(dev, "%s: %d: We are in this function\n", __func__, __LINE__);
> > > +
> > >         smc.args[0] = ARM_SMCCC_CALL_VAL(
> > >                 smccc_call_type,
> > >                 qcom_smccc_convention,
> > > @@ -174,6 +176,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > >         if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> > >                 alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
> > >                 args_virt = qcom_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
> > > +               dev_err(dev, "%s: %d: Hit the unlikely case!\n", __func__, __LINE__);
> > >  
> > >                 if (!args_virt)
> > >                         return -ENOMEM;
> > > @@ -197,6 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > >  
> > >         /* ret error check follows after args_virt cleanup*/
> > >         ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> > > +       dev_err(dev, "%s: %d: ret: %d\n", __func__, __LINE__, ret);
> > >  
> > >         if (ret)
> > >                 return ret;
> > > @@ -205,8 +209,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > >                 res->result[0] = smc_res.a1;
> > >                 res->result[1] = smc_res.a2;
> > >                 res->result[2] = smc_res.a3;
> > > +               dev_err(dev, "%s: %d: 0: %llu, 1: %llu: 2: %llu\n", __func__, __LINE__, res->result[0], res->result[1], res->result[2]);
> > >         }
> > >  
> > > +       dev_err(dev, "%s: %d: smc_res.a0: %lu\n", __func__, __LINE__, smc_res.a0);
> > >         return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> > > 
> > > 
> > > And that all spams dmesg successfully for most cases, but the
> > > pas_init_image calls log this out:
> > > 
> > > [   16.362965] remoteproc remoteproc1: powering up 1b300000.remoteproc
> > > [   16.364897] remoteproc remoteproc1: Booting fw image qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn, size 3575808
> > > [   16.365009] qcom_scm firmware:scm: __scm_smc_call: 165: We are in this function
> > > [   16.365251] qcom_scm firmware:scm: __scm_smc_call: 203: ret: 0
> > > [   16.365256] qcom_scm firmware:scm: __scm_smc_call: 212: 0: 0, 1: 0: 2: 0
> > > [   16.365261] qcom_scm firmware:scm: __scm_smc_call: 215: smc_res.a0: 4291821558
> > > 
> > > At the moment I am unsure why...
> > > 
> > Does the issue appear right after taking patch 6 or does it only appear after taking
> > the whole series? If it's just to this patch, then maybe something wrong with
> > the refactor: shm bridge isn't enabled at this point in the series.
> > 
> 
> I've only been testing the series as a whole on top of a 6.6 based
> branch, I'm going to try and test some more today to see if just the
> allocator bits (but not the SHM bridge enablement) works alright for
> me.
> 

After testing a little more with the fix Bart sent above,
the allocator refactor seems to work well.
With "firmware: qcom: scm: enable SHM bridge" applied is when I see the
errors I pointed out above. All prior patches cause no problems on boot
for me.

For patches 1-9 (with the fix sent here for patch 6) please feel free
to add:

    Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s

If anyone has suggestions for debugging why the switch to using
SHM bridge is breaking firmware loading for me, I'm willing to give
that a try.

Thanks,
Andrew

Re: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator

[Bartosz Golaszewski]

On Mon, Oct 2, 2023 at 4:15 PM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Mon, Oct 02, 2023 at 08:24:09AM -0500, Andrew Halaney wrote:
> > On Fri, Sep 29, 2023 at 03:48:37PM -0700, Elliot Berman wrote:
> > > Hi Andrew,
> > >
> > > On 9/29/2023 1:44 PM, Andrew Halaney wrote:
> > > > On Fri, Sep 29, 2023 at 12:22:16PM -0700, Bartosz Golaszewski wrote:
> > > >> On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said:
> > > >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
> > > >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >>>>
> > > >>>> Let's use the new SCM 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 | 16 +++++-----------
> > > >>>>  1 file changed, 5 insertions(+), 11 deletions(-)
> > > >>>>
> > > >>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > >>>> index 02a773ba1383..c0eb81069847 100644
> > > >>>> --- a/drivers/firmware/qcom/qcom_scm.c
> > > >>>> +++ b/drivers/firmware/qcom/qcom_scm.c
> > > >>>> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
> > > >>>>  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > > >>>>                            struct qcom_scm_pas_metadata *ctx)
> > > >>>>  {
> > > >>>> -      dma_addr_t mdata_phys;
> > > >>>> +      phys_addr_t mdata_phys;
> > > >>>
> > > >>>>        void *mdata_buf;
> > > >>>>        int ret;
> > > >>>>        struct qcom_scm_desc desc = {
> > > >>>> @@ -544,13 +544,7 @@ 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.
> > > >>>> -       */
> > > >>>> -      mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> > > >>>> -                                     GFP_KERNEL);
> > > >>>> +      mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
> > > >>>
> > > >>> mdata_phys is never initialized now, and its what's being shoved into
> > > >>> desc.args[1] later, which I believe is what triggered the -EINVAL
> > > >>> with qcom_scm_call() that I reported in my cover letter reply this
> > > >>> morning.
> > > >>>
> > > >>> Prior with the DMA API that would have been the device view of the buffer.
> > > >>>
> > > >>
> > > >> Gah! Thanks for finding this.
> > > >>
> > > >> Can you try the following diff?
> > > >>
> > > >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > >> index 794388c3212f..b0d4ea237034 100644
> > > >> --- a/drivers/firmware/qcom/qcom_scm.c
> > > >> +++ b/drivers/firmware/qcom/qcom_scm.c
> > > >> @@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> > > >> void *metadata, size_t size,
> > > >>                  dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> > > >>                  return -ENOMEM;
> > > >>          }
> > > >> +        mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
> > > >>          memcpy(mdata_buf, metadata, size);
> > > >>
> > > >>          ret = qcom_scm_clk_enable();
> > > >> @@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> > > >> void *metadata, size_t size,
> > > >>                  qcom_scm_mem_free(mdata_buf);
> > > >>          } else if (ctx) {
> > > >>                  ctx->ptr = mdata_buf;
> > > >> -                ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
> > > >> +                ctx->phys = mdata_phys;
> > > >>                  ctx->size = size;
> > > >>          }
> > > >>
> > > >> Bart
> > > >>
> > > >
> > > > For some reason that I can't explain that is still not working. It seems
> > > > the SMC call is returning !0 and then we return -EINVAL from there
> > > > with qcom_scm_remap_error().
> > > >
> > > > Here's a really crummy diff of what I hacked in during lunch to debug (don't
> > > > judge my primitive debug skills):
> > > >
> > >
> > > I don't know what you're talking about :-)
> > >
> > > > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > > > index 0d5554df1321..56eab0ae5f3a 100644
> > > > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > > > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > > > @@ -162,6 +162,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > >         struct arm_smccc_res smc_res;
> > > >         struct arm_smccc_args smc = {0};
> > > >
> > > > +       dev_err(dev, "%s: %d: We are in this function\n", __func__, __LINE__);
> > > > +
> > > >         smc.args[0] = ARM_SMCCC_CALL_VAL(
> > > >                 smccc_call_type,
> > > >                 qcom_smccc_convention,
> > > > @@ -174,6 +176,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > >         if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> > > >                 alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
> > > >                 args_virt = qcom_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
> > > > +               dev_err(dev, "%s: %d: Hit the unlikely case!\n", __func__, __LINE__);
> > > >
> > > >                 if (!args_virt)
> > > >                         return -ENOMEM;
> > > > @@ -197,6 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > >
> > > >         /* ret error check follows after args_virt cleanup*/
> > > >         ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> > > > +       dev_err(dev, "%s: %d: ret: %d\n", __func__, __LINE__, ret);
> > > >
> > > >         if (ret)
> > > >                 return ret;
> > > > @@ -205,8 +209,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > >                 res->result[0] = smc_res.a1;
> > > >                 res->result[1] = smc_res.a2;
> > > >                 res->result[2] = smc_res.a3;
> > > > +               dev_err(dev, "%s: %d: 0: %llu, 1: %llu: 2: %llu\n", __func__, __LINE__, res->result[0], res->result[1], res->result[2]);
> > > >         }
> > > >
> > > > +       dev_err(dev, "%s: %d: smc_res.a0: %lu\n", __func__, __LINE__, smc_res.a0);
> > > >         return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> > > >
> > > >
> > > > And that all spams dmesg successfully for most cases, but the
> > > > pas_init_image calls log this out:
> > > >
> > > > [   16.362965] remoteproc remoteproc1: powering up 1b300000.remoteproc
> > > > [   16.364897] remoteproc remoteproc1: Booting fw image qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn, size 3575808
> > > > [   16.365009] qcom_scm firmware:scm: __scm_smc_call: 165: We are in this function
> > > > [   16.365251] qcom_scm firmware:scm: __scm_smc_call: 203: ret: 0
> > > > [   16.365256] qcom_scm firmware:scm: __scm_smc_call: 212: 0: 0, 1: 0: 2: 0
> > > > [   16.365261] qcom_scm firmware:scm: __scm_smc_call: 215: smc_res.a0: 4291821558
> > > >
> > > > At the moment I am unsure why...
> > > >
> > > Does the issue appear right after taking patch 6 or does it only appear after taking
> > > the whole series? If it's just to this patch, then maybe something wrong with
> > > the refactor: shm bridge isn't enabled at this point in the series.
> > >
> >
> > I've only been testing the series as a whole on top of a 6.6 based
> > branch, I'm going to try and test some more today to see if just the
> > allocator bits (but not the SHM bridge enablement) works alright for
> > me.
> >
>
> After testing a little more with the fix Bart sent above,
> the allocator refactor seems to work well.
> With "firmware: qcom: scm: enable SHM bridge" applied is when I see the
> errors I pointed out above. All prior patches cause no problems on boot
> for me.
>
> For patches 1-9 (with the fix sent here for patch 6) please feel free
> to add:
>
>     Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s
>
> If anyone has suggestions for debugging why the switch to using
> SHM bridge is breaking firmware loading for me, I'm willing to give
> that a try.
>

Is it possible that sc8280xp doesn't support SHM bridge but for some
reason __qcom_scm_is_call_available() returns true when checking if it
does?

That's the only thing that comes to my mind ATM.

Bart

Re: [PATCH v2 01/11] firmware: qcom: move Qualcomm code into its own directory

[Krzysztof Kozlowski]

On 28/09/2023 11:20, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <bartosz.golaszewski@linaro.org>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Maximilian Luz]

On 9/28/23 11:20, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Extens the SCM memory allocator with using the SHM Bridge feature if
> available on the platform. This makes the trustzone only use dedicated
> buffers for SCM calls. We map the entire SCM genpool as a bridge.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This patch breaks something in early boot on my Surface Pro X (sc8180x).
Unfortunately I can't provide many details at the moment because the
only thing I can see are RCU stalls, and the traces from them are quite
useless.

Without this patch, the rest of the series (with the fix you posted on
patch 6 applied) seems to work fine. Including both RFT qseecom patches.

I plan to have a closer look at this once I have some more time though.

Regards,
Max

> ---
>   drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
>   1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> index eafecbe23770..12b12b15f46f 100644
> --- a/drivers/firmware/qcom/qcom_scm-mem.c
> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> @@ -16,6 +16,8 @@
>   
>   #include "qcom_scm.h"
>   
> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> +
>   static size_t qcom_scm_mem_pool_size = SZ_2M;
>   module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
>   		   ulong, 0400);
> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
>   	return chunk->paddr;
>   }
>   
> +static int qcom_scm_mem_shm_bridge_create(void)
> +{
> +	uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> +
> +	ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> +	pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> +	ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> +	size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> +
> +	return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> +					  ipfn_and_s_perm, size_and_flags,
> +					  QCOM_SCM_VMID_HLOS);
> +}
> +
>   int qcom_scm_mem_enable(struct device *dev)
>   {
> +	int ret;
> +
>   	INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
>   	spin_lock_init(&qcom_scm_mem.lock);
>   	qcom_scm_mem.dev = dev;
> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>   
>   	gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>   
> -	return gen_pool_add_virt(qcom_scm_mem.pool,
> -				 (unsigned long)qcom_scm_mem.vbase,
> -				 qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> +	ret = gen_pool_add_virt(qcom_scm_mem.pool,
> +				(unsigned long)qcom_scm_mem.vbase,
> +				qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_scm_enable_shm_bridge();
> +	if (ret) {
> +		if (ret == EOPNOTSUPP)
> +			dev_info(dev, "SHM Bridge not supported\n");
> +		else
> +			return ret;
> +	} else {
> +		ret = qcom_scm_mem_shm_bridge_create();
> +		if (ret)
> +			return ret;
> +
> +		dev_info(dev, "SHM Bridge enabled\n");
> +	}
> +
> +	return 0;
>   }

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Bartosz Golaszewski]

On Thu, Oct 5, 2023 at 12:24 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> On 9/28/23 11:20, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Extens the SCM memory allocator with using the SHM Bridge feature if
> > available on the platform. This makes the trustzone only use dedicated
> > buffers for SCM calls. We map the entire SCM genpool as a bridge.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This patch breaks something in early boot on my Surface Pro X (sc8180x).
> Unfortunately I can't provide many details at the moment because the
> only thing I can see are RCU stalls, and the traces from them are quite
> useless.
>
> Without this patch, the rest of the series (with the fix you posted on
> patch 6 applied) seems to work fine. Including both RFT qseecom patches.
>
> I plan to have a closer look at this once I have some more time though.
>

Can it be the PAS image loading? This is something Andrew reported and
I have it fixed for v3.

Bart

> Regards,
> Max
>
> > ---
> >   drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
> >   1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
> > index eafecbe23770..12b12b15f46f 100644
> > --- a/drivers/firmware/qcom/qcom_scm-mem.c
> > +++ b/drivers/firmware/qcom/qcom_scm-mem.c
> > @@ -16,6 +16,8 @@
> >
> >   #include "qcom_scm.h"
> >
> > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
> > +
> >   static size_t qcom_scm_mem_pool_size = SZ_2M;
> >   module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
> >                  ulong, 0400);
> > @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
> >       return chunk->paddr;
> >   }
> >
> > +static int qcom_scm_mem_shm_bridge_create(void)
> > +{
> > +     uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
> > +
> > +     ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
> > +     pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > +     ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
> > +     size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
> > +
> > +     return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
> > +                                       ipfn_and_s_perm, size_and_flags,
> > +                                       QCOM_SCM_VMID_HLOS);
> > +}
> > +
> >   int qcom_scm_mem_enable(struct device *dev)
> >   {
> > +     int ret;
> > +
> >       INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
> >       spin_lock_init(&qcom_scm_mem.lock);
> >       qcom_scm_mem.dev = dev;
> > @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
> >
> >       gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
> >
> > -     return gen_pool_add_virt(qcom_scm_mem.pool,
> > -                              (unsigned long)qcom_scm_mem.vbase,
> > -                              qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > +     ret = gen_pool_add_virt(qcom_scm_mem.pool,
> > +                             (unsigned long)qcom_scm_mem.vbase,
> > +                             qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = qcom_scm_enable_shm_bridge();
> > +     if (ret) {
> > +             if (ret == EOPNOTSUPP)
> > +                     dev_info(dev, "SHM Bridge not supported\n");
> > +             else
> > +                     return ret;
> > +     } else {
> > +             ret = qcom_scm_mem_shm_bridge_create();
> > +             if (ret)
> > +                     return ret;
> > +
> > +             dev_info(dev, "SHM Bridge enabled\n");
> > +     }
> > +
> > +     return 0;
> >   }

Re: [PATCH v2 11/11] firmware: qcom: scm: enable SHM bridge

[Maximilian Luz]

Am 05/10/2023 um 09:12 schrieb Bartosz Golaszewski:
> On Thu, Oct 5, 2023 at 12:24 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
>> On 9/28/23 11:20, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> Extens the SCM memory allocator with using the SHM Bridge feature if
>>> available on the platform. This makes the trustzone only use dedicated
>>> buffers for SCM calls. We map the entire SCM genpool as a bridge.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> This patch breaks something in early boot on my Surface Pro X (sc8180x).
>> Unfortunately I can't provide many details at the moment because the
>> only thing I can see are RCU stalls, and the traces from them are quite
>> useless.
>>
>> Without this patch, the rest of the series (with the fix you posted on
>> patch 6 applied) seems to work fine. Including both RFT qseecom patches.
>>
>> I plan to have a closer look at this once I have some more time though.
>>
> 
> Can it be the PAS image loading? This is something Andrew reported and
> I have it fixed for v3.

That is my current suspicion, but I haven't had the time to properly
check it yet.

Regards,
Max

> Bart
> 
>> Regards,
>> Max
>>
>>> ---
>>>    drivers/firmware/qcom/qcom_scm-mem.c | 42 ++++++++++++++++++++++++++--
>>>    1 file changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/firmware/qcom/qcom_scm-mem.c b/drivers/firmware/qcom/qcom_scm-mem.c
>>> index eafecbe23770..12b12b15f46f 100644
>>> --- a/drivers/firmware/qcom/qcom_scm-mem.c
>>> +++ b/drivers/firmware/qcom/qcom_scm-mem.c
>>> @@ -16,6 +16,8 @@
>>>
>>>    #include "qcom_scm.h"
>>>
>>> +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9
>>> +
>>>    static size_t qcom_scm_mem_pool_size = SZ_2M;
>>>    module_param_named(qcom_scm_mem_pool_size, qcom_scm_mem_pool_size,
>>>                   ulong, 0400);
>>> @@ -108,8 +110,24 @@ phys_addr_t qcom_scm_mem_to_phys(void *vaddr)
>>>        return chunk->paddr;
>>>    }
>>>
>>> +static int qcom_scm_mem_shm_bridge_create(void)
>>> +{
>>> +     uint64_t pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms;
>>> +
>>> +     ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ);
>>> +     pfn_and_ns_perm = (u64)qcom_scm_mem.pbase | ns_perms;
>>> +     ipfn_and_s_perm = (u64)qcom_scm_mem.pbase | ns_perms;
>>> +     size_and_flags = qcom_scm_mem.size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT);
>>> +
>>> +     return qcom_scm_create_shm_bridge(qcom_scm_mem.dev, pfn_and_ns_perm,
>>> +                                       ipfn_and_s_perm, size_and_flags,
>>> +                                       QCOM_SCM_VMID_HLOS);
>>> +}
>>> +
>>>    int qcom_scm_mem_enable(struct device *dev)
>>>    {
>>> +     int ret;
>>> +
>>>        INIT_RADIX_TREE(&qcom_scm_mem.chunks, GFP_ATOMIC);
>>>        spin_lock_init(&qcom_scm_mem.lock);
>>>        qcom_scm_mem.dev = dev;
>>> @@ -128,7 +146,25 @@ int qcom_scm_mem_enable(struct device *dev)
>>>
>>>        gen_pool_set_algo(qcom_scm_mem.pool, gen_pool_best_fit, NULL);
>>>
>>> -     return gen_pool_add_virt(qcom_scm_mem.pool,
>>> -                              (unsigned long)qcom_scm_mem.vbase,
>>> -                              qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
>>> +     ret = gen_pool_add_virt(qcom_scm_mem.pool,
>>> +                             (unsigned long)qcom_scm_mem.vbase,
>>> +                             qcom_scm_mem.pbase, qcom_scm_mem.size, -1);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = qcom_scm_enable_shm_bridge();
>>> +     if (ret) {
>>> +             if (ret == EOPNOTSUPP)
>>> +                     dev_info(dev, "SHM Bridge not supported\n");
>>> +             else
>>> +                     return ret;
>>> +     } else {
>>> +             ret = qcom_scm_mem_shm_bridge_create();
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             dev_info(dev, "SHM Bridge enabled\n");
>>> +     }
>>> +
>>> +     return 0;
>>>    }