[PATCH 0/6] Peripheral Image Loader support for Qualcomm SoCs

[PATCH 0/6] Peripheral Image Loader support for Qualcomm SoCs

[Mukesh Ojha]

Qualcomm is looking to enable remote processors on the SA8775p SoC
running KVM Linux host and is currently trying to figure out an
upstream-compatible solution for the IOMMU translation scheme problem it
is facing when SoCs running with KVM. This issue arises due to
differences in how IOMMU translation is currently handled on SoCs
running Qualcomm EL2 hypervisor(QHEE) where IOMMU translation for any
device is completely owned by it and the other issue is that remote
processors firmware does not contain resource table where these IOMMU
configuration setting will be present.

Qualcomm SoCs running with the QHEE(EL2) have been utilizing the
Peripheral Authentication Service (PAS) from its TrustZone (TZ) firmware
to securely authenticate and reset via a single SMC call
_auth_and_reset_.  This call first gets trapped to QHEE, which then
makes a call to TZ for authentication. Once it is done, the call returns
to QHEE, which sets up the IOMMU translation scheme for these remote
processors and later brings them out of reset. The design of the
Qualcomm EL2 hypervisor dictates that the Linux host OS running at EL1
is not allowed to set up IOMMU translation for remote processors,
and only a single stage is being configured for them.

To make the remote processors’ bring-up (PAS) sequence
hypervisor-independent, the auth_and_reset SMC call is now entirely
handled by TZ. However, the problem of IOMMU handling still remains with
the KVM host, which has no knowledge of the remote processors’ IOMMU
configuration.

We have looked up one approach where SoC remoteproc device tree could
contain resources like iommus for remoteproc carveout and qcom,devmem
specific binding for device memory needed for remoteproc and these
properties are optional and will only be overlaid by the firmware if it
is running with non-QHEE based hypervisor like KVM.

- Patch 1/6 adds the iommus and qcom,devmem binding for PAS common yaml.
- Patch 2/6 and 3/6 add helper function to IOMMU map and unmap carveout
  and device memory region.
- Patch 4/6 adds a function to parse individual field of qcom,devmem property.
- Patch 5/6 add helpers to create/destroy SHM bridge for remoteproc
  carveout and to get memory from tzmem SHM bridge pool for remoteproc
  firmware metadata.
- Patch 6/6 enable all the required support to enable remoteproc for
  non-QHEE hypervisor based systems like KVM host via parsing the iommus
  properties and mapping/unmapping carveout and device memory based on
  it.

Komal Bajaj (1):
  remoteproc: qcom: Add iommu map_unmap helper function

Mukesh Ojha (2):
  remoteproc: qcom: Add support of SHM bridge to enable memory
    protection
  remoteproc: qcom: Enable map/unmap and SHM bridge support

Shiraz Hashim (3):
  dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and
    qcom,devmem property
  remoteproc: qcom: Add helper function to support IOMMU devmem
    translation
  remoteproc: qcom: Add support to parse qcom,devmem property

 .../bindings/remoteproc/qcom,pas-common.yaml  |  42 +++++
 .../bindings/remoteproc/qcom,sa8775p-pas.yaml |  20 +++
 drivers/firmware/qcom/qcom_scm.c              |  29 +++-
 drivers/firmware/qcom/qcom_tzmem.c            |  14 +-
 drivers/remoteproc/qcom_common.c              | 148 ++++++++++++++++++
 drivers/remoteproc/qcom_common.h              |  38 +++++
 drivers/remoteproc/qcom_q6v5_pas.c            | 140 ++++++++++++++++-
 include/linux/firmware/qcom/qcom_scm.h        |   1 +
 include/linux/firmware/qcom/qcom_tzmem.h      |  10 ++
 9 files changed, 423 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Mukesh Ojha]

From: Shiraz Hashim <quic_shashim@quicinc.com>

Qualcomm’s PAS implementation for remote processors only supports a
single stage of IOMMU translation and is presently managed by the
Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
such as with a KVM hypervisor, IOMMU translations need to be set up by
the KVM host. Remoteproc needs carveout memory region and its resource
(device memory) permissions to be set before it comes up, and this
information is presently available statically with QHEE.

In the absence of QHEE, the boot firmware needs to overlay this
information based on SoCs running with either QHEE or a KVM hypervisor
(CPUs booted in EL2).

The qcom,devmem property provides IOMMU devmem translation information
intended for non-QHEE based systems.

Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
 .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
 2 files changed, 62 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
index 63a82e7a8bf8..068e177ad934 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
@@ -52,6 +52,48 @@ properties:
     minItems: 1
     maxItems: 3
 
+  iommus:
+    maxItems: 1
+
+  qcom,devmem:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    description:
+      Qualcomm’s PAS implementation for remote processors only supports a
+      single stage of IOMMU translation and is presently managed by the
+      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
+      such as with a KVM hypervisor, IOMMU translations need to be set up by
+      the KVM host. Remoteproc might need some device resources and related
+      access permissions to be set before it comes up, and this information is
+      presently available statically with QHEE.
+
+      In the absence of QHEE, the boot firmware needs to overlay this
+      information based on SoCs running with either QHEE or a KVM hypervisor
+      (CPUs booted in EL2).
+
+      The qcom,devmem property provides IOMMU devmem translation information
+      intended for non-QHEE based systems. It is an array of u32 values
+      describing the device memory regions for which IOMMU translations need to
+      be set up before bringing up Remoteproc. This array consists of 4-tuples
+      defining the device address, physical address, size, and attribute flags
+      with which it has to be mapped.
+
+      remoteproc@3000000 {
+          ...
+
+          qcom,devmem = <0x82000 0x82000 0x2000 0x3>,
+                        <0x92000 0x92000 0x1000 0x1>;
+      }
+
+    items:
+      items:
+        - description: device address
+        - description: physical address
+        - description: size of mapping
+        - description: |
+            iommu attributes - IOMMU_READ, IOMMU_WRITE, IOMMU_CACHE, IOMMU_NOEXEC, IOMMU_MMIO
+          enum: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
+                  25, 26, 27, 28, 29, 30, 31 ]
+
   qcom,smem-states:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     description: States used by the AP to signal the Hexagon core
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
index 7fe401a06805..503c5c9d8ea7 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
@@ -139,6 +139,26 @@ examples:
         power-domains = <&rpmhpd RPMHPD_LCX>, <&rpmhpd RPMHPD_LMX>;
         power-domain-names = "lcx", "lmx";
 
+        iommus = <&apps_smmu 0x3000 0x0>;
+        qcom,devmem = <0x00110000 0x00110000 0x4000 0x1>,
+                      <0x00123000 0x00123000 0x1000 0x3>,
+                      <0x00124000 0x00124000 0x3000 0x3>,
+                      <0x00127000 0x00127000 0x2000 0x3>,
+                      <0x0012a000 0x0012a000 0x3000 0x3>,
+                      <0x0012e000 0x0012e000 0x1000 0x3>,
+                      <0x0012f000 0x0012f000 0x1000 0x1>,
+                      <0x00144000 0x00144000 0x1000 0x1>,
+                      <0x00148000 0x00148000 0x1000 0x1>,
+                      <0x00149000 0x00149000 0xe000 0x3>,
+                      <0x00157000 0x00157000 0x1000 0x3>,
+                      <0x00158000 0x00158000 0xd000 0x3>,
+                      <0x00165000 0x00165000 0x1000 0x3>,
+                      <0x00172000 0x00172000 0x1000 0x3>,
+                      <0x00173000 0x00173000 0x8000 0x3>,
+                      <0x0017b000 0x0017b000 0x2000 0x3>,
+                      <0x0017f000 0x0017f000 0x1000 0x3>,
+                      <0x00184000 0x00184000 0x1000 0x1>;
+
         interconnects = <&lpass_ag_noc MASTER_LPASS_PROC 0 &mc_virt SLAVE_EBI1 0>;
 
         memory-region = <&pil_adsp_mem>;
-- 
2.34.1

[PATCH 2/6] remoteproc: qcom: Add iommu map_unmap helper function

[Mukesh Ojha]

From: Komal Bajaj <quic_kbajaj@quicinc.com>

Qualcomm remote processor's IOMMU translation running on Linux KVM host
should be managed by PAS driver and to do this PAS driver need to do map
and unmap remoteproc carveout memory region. Similar map and unmap
private functions for the similar purpose are already available in
qcom_q6v5_adsp.c. So, in motivation to reuse code, introduce common
exported functions like qcom_map_unmap_carveout() such that it can be
used by both qcom_q6v5_adsp and qcom_q6v5_pas.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_common.c | 52 ++++++++++++++++++++++++++++++++
 drivers/remoteproc/qcom_common.h |  3 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 8c8688f99f0a..1c7887dc65b4 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/firmware.h>
+#include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
@@ -35,6 +36,8 @@
 #define MINIDUMP_SS_ENCR_DONE		('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
 #define MINIDUMP_SS_ENABLED		('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
 
+#define SID_MASK_DEFAULT	0xfUL
+
 /**
  * struct minidump_region - Minidump region
  * @name		: Name of the region to be dumped
@@ -606,5 +609,54 @@ void qcom_remove_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm)
 }
 EXPORT_SYMBOL_GPL(qcom_remove_pdm_subdev);
 
+/**
+ * qcom_map_unmap_carveout() - iommu map and unmap carveout region
+ *
+ * @rproc:	rproc handle
+ * @mem_phys:	starting physical address of carveout region
+ * @mem_size:	size of carveout region
+ * @map:	if true, map otherwise, unmap
+ * @use_sid:	decision to append sid to iova
+ * @sid:	SID value
+ */
+int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t mem_size,
+			    bool map, bool use_sid, unsigned long sid)
+{
+	unsigned long iova = mem_phys;
+	unsigned long sid_def_val;
+	int ret;
+
+	if (!rproc->has_iommu)
+		return 0;
+
+	if (!rproc->domain)
+		return -EINVAL;
+
+	/*
+	 * Remote processor like ADSP supports upto 36 bit device
+	 * address space and some of its clients like fastrpc uses
+	 * upper 32-35 bits to keep lower 4 bits of its SID to use
+	 * larger address space. To keep this consistent across other
+	 * use cases add remoteproc SID configuration for firmware
+	 * to IOMMU for carveouts.
+	 */
+	if (use_sid && sid) {
+		sid_def_val = sid & SID_MASK_DEFAULT;
+		iova |= (sid_def_val << 32);
+	}
+
+	if (map)
+		ret = iommu_map(rproc->domain, iova, mem_phys, mem_size, IOMMU_READ | IOMMU_WRITE, GFP_KERNEL);
+	else
+		ret = iommu_unmap(rproc->domain, iova, mem_size);
+
+	if (ret)
+		dev_err(&rproc->dev, "Unable to %s IOVA Memory, ret: %d\n",
+			map ? "map" : "unmap", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_map_unmap_carveout);
+
 MODULE_DESCRIPTION("Qualcomm Remoteproc helper driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index b07fbaa091a0..bbc41054e1ea 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -59,6 +59,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 			 const char *ssr_name);
 void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr);
 
+int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t mem_size,
+			    bool map, bool use_sid, unsigned long sid);
+
 void qcom_add_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm);
 void qcom_remove_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm);
 
-- 
2.34.1

[PATCH 3/6] remoteproc: qcom: Add helper function to support IOMMU devmem translation

[Mukesh Ojha]

From: Shiraz Hashim <quic_shashim@quicinc.com>

Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
translation set up for remote processors is managed by QHEE itself
however, for a case when these remote processors has to run under KVM
hypervisor, IOMMU translation need to setup from Linux remoteproc driver
before it is brought up.

Add qcom_devmem_info and qcom_devmem_table data structure and make a
common helper functions which caller can call if these translation need
to be taken care by the driver to enable iommu devmem access for
remoteproc processors.

Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_common.c | 96 ++++++++++++++++++++++++++++++++
 drivers/remoteproc/qcom_common.h | 35 ++++++++++++
 2 files changed, 131 insertions(+)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 1c7887dc65b4..644920972b58 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -658,5 +658,101 @@ int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t me
 }
 EXPORT_SYMBOL_GPL(qcom_map_unmap_carveout);
 
+/**
+ * qcom_map_devmem() - Map the device memories needed by Remoteproc using IOMMU
+ *
+ * When Qualcomm EL2 hypervisor(QHEE) present, device memories needed for remoteproc
+ * processors is managed by it and Linux remoteproc drivers should not call
+ * this and its respective unmap function in such scenario. This function
+ * should only be called if remoteproc IOMMU translation need to be managed
+ * from Linux side.
+ *
+ * @rproc: rproc handle
+ * @devmem_table: list of devmem regions to map
+ * @use_sid: decision to append sid to iova
+ * @sid: SID value
+ */
+int qcom_map_devmem(struct rproc *rproc, struct qcom_devmem_table *devmem_table,
+		    bool use_sid, unsigned long sid)
+{
+	struct qcom_devmem_info *info;
+	unsigned long sid_def_val;
+	int ret;
+	int i;
+
+	if (!rproc->has_iommu)
+		return 0;
+
+	if (!rproc->domain)
+		return -EINVAL;
+
+	/* remoteproc may not have devmem data */
+	if (!devmem_table)
+		return 0;
+
+	if (use_sid && sid)
+		sid_def_val = sid & SID_MASK_DEFAULT;
+
+	info = &devmem_table->entries[0];
+	for (i = 0; i < devmem_table->num_entries; i++, info++) {
+		/*
+		 * Remote processor like ADSP supports upto 36 bit device
+		 * address space and some of its clients like fastrpc uses
+		 * upper 32-35 bits to keep lower 4 bits of its SID to use
+		 * larger address space. To keep this consistent across other
+		 * use cases add remoteproc SID configuration for firmware
+		 * to IOMMU for carveouts.
+		 */
+		if (use_sid)
+			info->da |= (sid_def_val << 32);
+
+		ret = iommu_map(rproc->domain, info->da, info->pa, info->len, info->flags, GFP_KERNEL);
+		if (ret) {
+			dev_err(&rproc->dev, "Unable to map devmem, ret: %d\n", ret);
+			if (use_sid)
+				info->da &= ~(SID_MASK_DEFAULT << 32);
+			goto undo_mapping;
+		}
+	}
+
+	return 0;
+
+undo_mapping:
+	for (i = i - 1; i >= 0; i--, info--) {
+		iommu_unmap(rproc->domain, info->da, info->len);
+		if (use_sid)
+			info->da &= ~(SID_MASK_DEFAULT << 32);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_map_devmem);
+
+/**
+ * qcom_unmap_devmem() -  unmap the device memories needed by Remoteproc using IOMMU
+ *
+ * @rproc:		rproc handle
+ * @devmem_table:	list of devmem regions to unmap
+ * @use_sid:		decision to append sid to iova
+ */
+void qcom_unmap_devmem(struct rproc *rproc, struct qcom_devmem_table *devmem_table, bool use_sid)
+{
+	struct qcom_devmem_info *info;
+	int i;
+
+	if (!rproc->has_iommu || !rproc->domain || !devmem_table)
+		return;
+
+	info = &devmem_table->entries[0];
+	for (i = 0; i < devmem_table->num_entries; i++, info++) {
+		iommu_unmap(rproc->domain, info->da, info->len);
+		if (use_sid)
+			info->da &= ~(SID_MASK_DEFAULT << 32);
+	}
+
+	return;
+}
+EXPORT_SYMBOL_GPL(qcom_unmap_devmem);
+
 MODULE_DESCRIPTION("Qualcomm Remoteproc helper driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index bbc41054e1ea..bbc684e1df01 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -41,6 +41,36 @@ struct qcom_rproc_pdm {
 	struct auxiliary_device *adev;
 };
 
+/**
+ * struct qcom_devmem_info - iommu devmem region
+ * @da: device address
+ * @pa: physical address
+ * @len: length (in bytes)
+ * @flags: iommu protection flags
+ *
+ * The resource entry carries the device address to which a physical address is
+ * to be mapped with required permissions in flag. The pa, len is expected to
+ * be a physically contiguous memory region.
+ */
+struct qcom_devmem_info {
+	u64 da;
+	u64 pa;
+	u32 len;
+	u32 flags;
+};
+
+/**
+ * struct qcom_devmem_table - iommu devmem entries
+ * @num_entries: number of devmem entries
+ * @entries: devmem entries
+ *
+ * The table that carries each devmem resource entry.
+ */
+struct qcom_devmem_table {
+	int num_entries;
+	struct qcom_devmem_info entries[0];
+};
+
 void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
 			void (*rproc_dumpfn_t)(struct rproc *rproc,
 				struct rproc_dump_segment *segment, void *dest, size_t offset,
@@ -65,6 +95,11 @@ int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t me
 void qcom_add_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm);
 void qcom_remove_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm);
 
+int qcom_map_devmem(struct rproc *rproc, struct qcom_devmem_table *table,
+		    bool use_sid, unsigned long sid);
+void qcom_unmap_devmem(struct rproc *rproc, struct qcom_devmem_table *table,
+		       bool use_sid);
+
 #if IS_ENABLED(CONFIG_QCOM_SYSMON)
 struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
 					   const char *name,
-- 
2.34.1

[PATCH 4/6] remoteproc: qcom: Add support to parse qcom,devmem property

[Mukesh Ojha]

From: Shiraz Hashim <quic_shashim@quicinc.com>

Qualcomm remote processors firmware does not contain resource table data
where devmem setting information should actually be present and for its
SoC running with Qualcomm EL2 hypervisor(QHEE), IOMMU translation for
remoteproc is managed by QHEE and it has all the IOMMU resource
information available to it to apply the settings however, when the same
SoCs run with KVM hypervisor these translation needs to setup by
remoteproc PAS driver and required setting information is being overlaid
from boot firmware to remoteproc device tree node.

Add helper function which parses qcom,devmem information.

Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 55 ++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index ef82835e98a4..bdb071ab5938 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -34,6 +34,7 @@
 #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS	100
 
 #define MAX_ASSIGN_COUNT 3
+#define DEVMEM_ENTRY_SIZE 4
 
 struct adsp_data {
 	int crash_reason_smem;
@@ -117,6 +118,8 @@ struct qcom_adsp {
 
 	struct qcom_scm_pas_metadata pas_metadata;
 	struct qcom_scm_pas_metadata dtb_pas_metadata;
+
+	struct qcom_devmem_table *devmem;
 };
 
 static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
@@ -683,6 +686,58 @@ static void adsp_unassign_memory_region(struct qcom_adsp *adsp)
 	}
 }
 
+static int adsp_devmem_init(struct qcom_adsp *adsp)
+{
+	unsigned int entry_size = DEVMEM_ENTRY_SIZE;
+	struct qcom_devmem_table *devmem_table;
+	struct rproc *rproc = adsp->rproc;
+	struct device *dev = adsp->dev;
+	struct qcom_devmem_info *info;
+	char *pname = "qcom,devmem";
+	size_t table_size;
+	int num_entries;
+	u32 i;
+
+	if (!rproc->has_iommu)
+		return 0;
+
+	/* devmem property is a set of n-tuple */
+	num_entries = of_property_count_u32_elems(dev->of_node, pname);
+	if (num_entries < 0) {
+		dev_err(adsp->dev, "No '%s' property present\n", pname);
+		return num_entries;
+	}
+
+	if (!num_entries || (num_entries % entry_size)) {
+		dev_err(adsp->dev, "All '%s' list entries need %d vals\n", pname,
+			entry_size);
+		return -EINVAL;
+	}
+
+	num_entries /= entry_size;
+	table_size = sizeof(*devmem_table) + sizeof(*info) * num_entries;
+	devmem_table = devm_kzalloc(dev, table_size, GFP_KERNEL);
+	if (!devmem_table)
+		return -ENOMEM;
+
+	devmem_table->num_entries = num_entries;
+	info = &devmem_table->entries[0];
+	for (i = 0; i < num_entries; i++, info++) {
+		of_property_read_u32_index(dev->of_node, pname,
+					   i * entry_size, (u32 *)&info->da);
+		of_property_read_u32_index(dev->of_node, pname,
+					   i * entry_size + 1, (u32 *)&info->pa);
+		of_property_read_u32_index(dev->of_node, pname,
+					   i * entry_size + 2, &info->len);
+		of_property_read_u32_index(dev->of_node, pname,
+					   i * entry_size + 3, &info->flags);
+	}
+
+	adsp->devmem = devmem_table;
+
+	return 0;
+}
+
 static int adsp_probe(struct platform_device *pdev)
 {
 	const struct adsp_data *desc;
-- 
2.34.1

[PATCH 5/6] remoteproc: qcom: Add support of SHM bridge to enable memory protection

[Mukesh Ojha]

Qualcomm SoCs running with the Qualcomm EL2 hypervisor(QHEE) have been
utilizing the Peripheral Authentication Service (PAS) from its TrustZone
(TZ) firmware to securely authenticate and reset via sequence of SMC
calls like qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(), and
qcom_scm_pas_auth_and_reset().

Memory protection need to be enabled for both meta data memory region and
remoteproc carveout memory region.

For memory passed to Qualcomm TrustZone, the memory should be part of
SHM bridge memory. However, when QHEE is present, PAS SMC calls are
getting trapped in QHEE, which create or gets memory from SHM bridge for
both meta data memory and for remoteproc carve out regions before it get
passed to TZ.  However, in absence of QHEE hypervisor, Linux need to
create SHM bridge for both meta data in qcom_scm_pas_init_image() and
for remoteproc memory before the call being made to
qcom_scm_pas_auth_and_reset().

For qcom_scm_pas_init_image() call, metadata content need to be copied
to the buffer allocated from SHM bridge before making the SMC call.

For qcom_scm_pas_auth_and_reset(), remoteproc memory region need to be
protected and for that SHM bridge need to be created. Make
qcom_tzmem_init_area() and qcom_tzmem_cleanup_area() exported symbol so
that it could be used to create SHM bridge for remoteproc region.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c         | 29 +++++++++++-----
 drivers/firmware/qcom/qcom_tzmem.c       | 14 +++-----
 drivers/remoteproc/qcom_q6v5_pas.c       | 44 ++++++++++++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h   |  1 +
 include/linux/firmware/qcom/qcom_tzmem.h | 10 ++++++
 5 files changed, 80 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 10986cb11ec0..dafc07dc181f 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -591,15 +591,19 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	 * data blob, so make sure it's physically contiguous, 4K aligned and
 	 * non-cachable to avoid XPU violations.
 	 *
-	 * For PIL calls the hypervisor creates SHM Bridges for the blob
-	 * buffers on behalf of Linux so we must not do it ourselves hence
-	 * not using the TZMem allocator here.
+	 * For PIL calls the hypervisor like Gunyah or older QHEE creates SHM
+	 * Bridges for the blob buffers on behalf of Linux so we must not do it
+	 * ourselves hence use TZMem allocator only when these hypervisors are
+	 * not present.
 	 *
 	 * If we pass a buffer that is already part of an SHM Bridge to this
 	 * call, it will fail.
 	 */
-	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
-				       GFP_KERNEL);
+	if (ctx && ctx->shm_bridge_needed)
+		mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
+	else
+		mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys, GFP_KERNEL);
+
 	if (!mdata_buf)
 		return -ENOMEM;
 
@@ -613,7 +617,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	if (ret)
 		goto disable_clk;
 
-	desc.args[1] = mdata_phys;
+	if (ctx && ctx->shm_bridge_needed)
+		desc.args[1] = qcom_tzmem_to_phys(mdata_buf);
+	else
+		desc.args[1] = mdata_phys;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
@@ -625,8 +632,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	if (ret < 0 || !ctx) {
 		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
 	} else if (ctx) {
+		if (ctx->shm_bridge_needed)
+			ctx->phys = qcom_tzmem_to_phys(mdata_buf);
+		else
+			ctx->phys = mdata_phys;
 		ctx->ptr = mdata_buf;
-		ctx->phys = mdata_phys;
 		ctx->size = size;
 	}
 
@@ -643,7 +653,10 @@ 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);
+	if (ctx->shm_bridge_needed)
+		qcom_tzmem_free(ctx->ptr);
+	else
+		dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
 
 	ctx->ptr = NULL;
 	ctx->phys = 0;
diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
index 92b365178235..66aba2fc979d 100644
--- a/drivers/firmware/qcom/qcom_tzmem.c
+++ b/drivers/firmware/qcom/qcom_tzmem.c
@@ -22,14 +22,6 @@
 
 #include "qcom_tzmem.h"
 
-struct qcom_tzmem_area {
-	struct list_head list;
-	void *vaddr;
-	dma_addr_t paddr;
-	size_t size;
-	void *priv;
-};
-
 struct qcom_tzmem_pool {
 	struct gen_pool *genpool;
 	struct list_head areas;
@@ -107,7 +99,7 @@ static int qcom_tzmem_init(void)
 	return 0;
 }
 
-static int qcom_tzmem_init_area(struct qcom_tzmem_area *area)
+int qcom_tzmem_init_area(struct qcom_tzmem_area *area)
 {
 	u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags;
 	int ret;
@@ -133,8 +125,9 @@ static int qcom_tzmem_init_area(struct qcom_tzmem_area *area)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(qcom_tzmem_init_area);
 
-static void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area)
+void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area)
 {
 	u64 *handle = area->priv;
 
@@ -144,6 +137,7 @@ static void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area)
 	qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle);
 	kfree(handle);
 }
+EXPORT_SYMBOL_GPL(qcom_tzmem_cleanup_area);
 
 #endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */
 
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index bdb071ab5938..ac339145e072 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -20,6 +20,7 @@
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/firmware/qcom/qcom_tzmem.h>
 #include <linux/regulator/consumer.h>
 #include <linux/remoteproc.h>
 #include <linux/soc/qcom/mdt_loader.h>
@@ -120,6 +121,7 @@ struct qcom_adsp {
 	struct qcom_scm_pas_metadata dtb_pas_metadata;
 
 	struct qcom_devmem_table *devmem;
+	struct qcom_tzmem_area *tzmem;
 };
 
 static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
@@ -262,6 +264,43 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
+static int adsp_create_shmbridge(struct qcom_adsp *adsp)
+{
+	struct qcom_tzmem_area *rproc_tzmem;
+	struct rproc *rproc = adsp->rproc;
+	int ret;
+
+	if (!rproc->has_iommu)
+		return 0;
+
+	rproc_tzmem = devm_kzalloc(adsp->dev, sizeof(*rproc_tzmem), GFP_KERNEL);
+	if (!rproc_tzmem)
+		return -ENOMEM;
+
+	rproc_tzmem->size = PAGE_ALIGN(adsp->mem_size);
+	rproc_tzmem->paddr = adsp->mem_phys;
+	ret = qcom_tzmem_init_area(rproc_tzmem);
+	if (ret) {
+		dev_err(adsp->dev,
+			"failed to create shmbridge for carveout: %d\n", ret);
+		return ret;
+	}
+
+	adsp->tzmem = rproc_tzmem;
+
+	return ret;
+}
+
+static void adsp_delete_shmbridge(struct qcom_adsp *adsp)
+{
+	struct rproc *rproc = adsp->rproc;
+
+	if (!rproc->has_iommu)
+		return;
+
+	qcom_tzmem_cleanup_area(adsp->tzmem);
+}
+
 static int adsp_start(struct rproc *rproc)
 {
 	struct qcom_adsp *adsp = rproc->priv;
@@ -317,6 +356,10 @@ static int adsp_start(struct rproc *rproc)
 
 	qcom_pil_info_store(adsp->info_name, adsp->mem_phys, adsp->mem_size);
 
+	ret = adsp_create_shmbridge(adsp);
+	if (ret)
+		goto release_pas_metadata;
+
 	ret = qcom_scm_pas_auth_and_reset(adsp->pas_id);
 	if (ret) {
 		dev_err(adsp->dev,
@@ -324,6 +367,7 @@ static int adsp_start(struct rproc *rproc)
 		goto release_pas_metadata;
 	}
 
+	adsp_delete_shmbridge(adsp);
 	ret = qcom_q6v5_wait_for_start(&adsp->q6v5, msecs_to_jiffies(5000));
 	if (ret == -ETIMEDOUT) {
 		dev_err(adsp->dev, "start timed out\n");
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 9f14976399ab..25243cd889bb 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -70,6 +70,7 @@ struct qcom_scm_pas_metadata {
 	void *ptr;
 	dma_addr_t phys;
 	ssize_t size;
+	bool shm_bridge_needed;
 };
 
 int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h
index b83b63a0c049..e0a57cc8f74b 100644
--- a/include/linux/firmware/qcom/qcom_tzmem.h
+++ b/include/linux/firmware/qcom/qcom_tzmem.h
@@ -39,6 +39,14 @@ struct qcom_tzmem_pool_config {
 	size_t max_size;
 };
 
+struct qcom_tzmem_area {
+	struct list_head list;
+	void *vaddr;
+	dma_addr_t paddr;
+	size_t size;
+	void *priv;
+};
+
 struct qcom_tzmem_pool *
 qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config);
 void qcom_tzmem_pool_free(struct qcom_tzmem_pool *pool);
@@ -48,6 +56,8 @@ devm_qcom_tzmem_pool_new(struct device *dev,
 
 void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp);
 void qcom_tzmem_free(void *ptr);
+int qcom_tzmem_init_area(struct qcom_tzmem_area *area);
+void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area);
 
 DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T))
 
-- 
2.34.1

[PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Mukesh Ojha]

For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
translation for remote processors is managed by QHEE and if the same SoC
run under KVM, remoteproc carveout and devmem region should be IOMMU
mapped from Linux PAS driver before remoteproc is brought up and
unmapped once it is tear down and apart from this, SHM bridge also need
to set up to enable memory protection on both remoteproc meta data
memory as well as for the carveout region.

Enable the support required to run Qualcomm remoteprocs on non-QHEE
hypervisors.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index ac339145e072..13bd13f1b989 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -122,6 +122,7 @@ struct qcom_adsp {
 
 	struct qcom_devmem_table *devmem;
 	struct qcom_tzmem_area *tzmem;
+	unsigned long sid;
 };
 
 static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
@@ -310,9 +311,21 @@ static int adsp_start(struct rproc *rproc)
 	if (ret)
 		return ret;
 
+	ret = qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, true, true, adsp->sid);
+	if (ret) {
+		dev_err(adsp->dev, "iommu mapping failed, ret: %d\n", ret);
+		goto disable_irqs;
+	}
+
+	ret = qcom_map_devmem(rproc, adsp->devmem, true, adsp->sid);
+	if (ret) {
+		dev_err(adsp->dev, "devmem iommu mapping failed, ret: %d\n", ret);
+		goto unmap_carveout;
+	}
+
 	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
 	if (ret < 0)
-		goto disable_irqs;
+		goto unmap_devmem;
 
 	ret = clk_prepare_enable(adsp->xo);
 	if (ret)
@@ -400,6 +413,10 @@ static int adsp_start(struct rproc *rproc)
 	clk_disable_unprepare(adsp->xo);
 disable_proxy_pds:
 	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+unmap_devmem:
+	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
+unmap_carveout:
+	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
 disable_irqs:
 	qcom_q6v5_unprepare(&adsp->q6v5);
 
@@ -445,6 +462,9 @@ static int adsp_stop(struct rproc *rproc)
 			dev_err(adsp->dev, "failed to shutdown dtb: %d\n", ret);
 	}
 
+	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
+	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
+
 	handover = qcom_q6v5_unprepare(&adsp->q6v5);
 	if (handover)
 		qcom_pas_handover(&adsp->q6v5);
@@ -844,6 +864,25 @@ static int adsp_probe(struct platform_device *pdev)
 	}
 	platform_set_drvdata(pdev, adsp);
 
+	if (of_property_present(pdev->dev.of_node, "iommus")) {
+		struct of_phandle_args args;
+
+		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
+		if (ret < 0)
+			return ret;
+
+		rproc->has_iommu = true;
+		adsp->sid = args.args[0];
+		of_node_put(args.np);
+		ret = adsp_devmem_init(adsp);
+		if (ret)
+			return ret;
+
+		adsp->pas_metadata.shm_bridge_needed = true;
+	} else {
+		rproc->has_iommu = false;
+	}
+
 	ret = device_init_wakeup(adsp->dev, true);
 	if (ret)
 		goto free_rproc;
-- 
2.34.1

Re: [PATCH 5/6] remoteproc: qcom: Add support of SHM bridge to enable memory protection

[kernel test robot]

Hi Mukesh,

kernel test robot noticed the following build errors:

[auto build test ERROR on remoteproc/rproc-next]
[also build test ERROR on robh/for-next linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mukesh-Ojha/dt-bindings-remoteproc-qcom-pas-common-Introduce-iommus-and-qcom-devmem-property/20241005-052733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link:    https://lore.kernel.org/r/20241004212359.2263502-6-quic_mojha%40quicinc.com
patch subject: [PATCH 5/6] remoteproc: qcom: Add support of SHM bridge to enable memory protection
config: arc-randconfig-001-20241006 (https://download.01.org/0day-ci/archive/20241006/202410060641.ZedzhoKd-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241006/202410060641.ZedzhoKd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410060641.ZedzhoKd-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/qcom/qcom_tzmem.c:50:12: error: static declaration of 'qcom_tzmem_init_area' follows non-static declaration
      50 | static int qcom_tzmem_init_area(struct qcom_tzmem_area *area)
         |            ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/firmware/qcom/qcom_tzmem.c:12:
   include/linux/firmware/qcom/qcom_tzmem.h:59:5: note: previous declaration of 'qcom_tzmem_init_area' with type 'int(struct qcom_tzmem_area *)'
      59 | int qcom_tzmem_init_area(struct qcom_tzmem_area *area);
         |     ^~~~~~~~~~~~~~~~~~~~
>> drivers/firmware/qcom/qcom_tzmem.c:55:13: error: static declaration of 'qcom_tzmem_cleanup_area' follows non-static declaration
      55 | static void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area)
         |             ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/firmware/qcom/qcom_tzmem.h:60:6: note: previous declaration of 'qcom_tzmem_cleanup_area' with type 'void(struct qcom_tzmem_area *)'
      60 | void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area);
         |      ^~~~~~~~~~~~~~~~~~~~~~~


vim +/qcom_tzmem_init_area +50 drivers/firmware/qcom/qcom_tzmem.c

84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  49  
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27 @50  static int qcom_tzmem_init_area(struct qcom_tzmem_area *area)
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  51  {
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  52  	return 0;
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  53  }
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  54  
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27 @55  static void qcom_tzmem_cleanup_area(struct qcom_tzmem_area *area)
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  56  {
84f5a7b67b61bf Bartosz Golaszewski 2024-05-27  57  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/6] remoteproc: qcom: Add iommu map_unmap helper function

[kernel test robot]

Hi Mukesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on robh/for-next linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mukesh-Ojha/dt-bindings-remoteproc-qcom-pas-common-Introduce-iommus-and-qcom-devmem-property/20241005-052733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link:    https://lore.kernel.org/r/20241004212359.2263502-3-quic_mojha%40quicinc.com
patch subject: [PATCH 2/6] remoteproc: qcom: Add iommu map_unmap helper function
config: arm-defconfig (https://download.01.org/0day-ci/archive/20241006/202410060943.UAdM2uKn-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241006/202410060943.UAdM2uKn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410060943.UAdM2uKn-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/remoteproc/qcom_common.c:645:24: warning: shift count >= width of type [-Wshift-count-overflow]
                   iova |= (sid_def_val << 32);
                                        ^  ~~
   1 warning generated.


vim +645 drivers/remoteproc/qcom_common.c

   611	
   612	/**
   613	 * qcom_map_unmap_carveout() - iommu map and unmap carveout region
   614	 *
   615	 * @rproc:	rproc handle
   616	 * @mem_phys:	starting physical address of carveout region
   617	 * @mem_size:	size of carveout region
   618	 * @map:	if true, map otherwise, unmap
   619	 * @use_sid:	decision to append sid to iova
   620	 * @sid:	SID value
   621	 */
   622	int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t mem_size,
   623				    bool map, bool use_sid, unsigned long sid)
   624	{
   625		unsigned long iova = mem_phys;
   626		unsigned long sid_def_val;
   627		int ret;
   628	
   629		if (!rproc->has_iommu)
   630			return 0;
   631	
   632		if (!rproc->domain)
   633			return -EINVAL;
   634	
   635		/*
   636		 * Remote processor like ADSP supports upto 36 bit device
   637		 * address space and some of its clients like fastrpc uses
   638		 * upper 32-35 bits to keep lower 4 bits of its SID to use
   639		 * larger address space. To keep this consistent across other
   640		 * use cases add remoteproc SID configuration for firmware
   641		 * to IOMMU for carveouts.
   642		 */
   643		if (use_sid && sid) {
   644			sid_def_val = sid & SID_MASK_DEFAULT;
 > 645			iova |= (sid_def_val << 32);
   646		}
   647	
   648		if (map)
   649			ret = iommu_map(rproc->domain, iova, mem_phys, mem_size, IOMMU_READ | IOMMU_WRITE, GFP_KERNEL);
   650		else
   651			ret = iommu_unmap(rproc->domain, iova, mem_size);
   652	
   653		if (ret)
   654			dev_err(&rproc->dev, "Unable to %s IOVA Memory, ret: %d\n",
   655				map ? "map" : "unmap", ret);
   656	
   657		return ret;
   658	}
   659	EXPORT_SYMBOL_GPL(qcom_map_unmap_carveout);
   660	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/6] remoteproc: qcom: Add iommu map_unmap helper function

[kernel test robot]

Hi Mukesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on robh/for-next linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mukesh-Ojha/dt-bindings-remoteproc-qcom-pas-common-Introduce-iommus-and-qcom-devmem-property/20241005-052733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link:    https://lore.kernel.org/r/20241004212359.2263502-3-quic_mojha%40quicinc.com
patch subject: [PATCH 2/6] remoteproc: qcom: Add iommu map_unmap helper function
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20241006/202410061256.KV3EbD7H-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241006/202410061256.KV3EbD7H-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410061256.KV3EbD7H-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/remoteproc/qcom_common.c: In function 'qcom_map_unmap_carveout':
>> drivers/remoteproc/qcom_common.c:645:38: warning: left shift count >= width of type [-Wshift-count-overflow]
     645 |                 iova |= (sid_def_val << 32);
         |                                      ^~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +645 drivers/remoteproc/qcom_common.c

   611	
   612	/**
   613	 * qcom_map_unmap_carveout() - iommu map and unmap carveout region
   614	 *
   615	 * @rproc:	rproc handle
   616	 * @mem_phys:	starting physical address of carveout region
   617	 * @mem_size:	size of carveout region
   618	 * @map:	if true, map otherwise, unmap
   619	 * @use_sid:	decision to append sid to iova
   620	 * @sid:	SID value
   621	 */
   622	int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t mem_size,
   623				    bool map, bool use_sid, unsigned long sid)
   624	{
   625		unsigned long iova = mem_phys;
   626		unsigned long sid_def_val;
   627		int ret;
   628	
   629		if (!rproc->has_iommu)
   630			return 0;
   631	
   632		if (!rproc->domain)
   633			return -EINVAL;
   634	
   635		/*
   636		 * Remote processor like ADSP supports upto 36 bit device
   637		 * address space and some of its clients like fastrpc uses
   638		 * upper 32-35 bits to keep lower 4 bits of its SID to use
   639		 * larger address space. To keep this consistent across other
   640		 * use cases add remoteproc SID configuration for firmware
   641		 * to IOMMU for carveouts.
   642		 */
   643		if (use_sid && sid) {
   644			sid_def_val = sid & SID_MASK_DEFAULT;
 > 645			iova |= (sid_def_val << 32);
   646		}
   647	
   648		if (map)
   649			ret = iommu_map(rproc->domain, iova, mem_phys, mem_size, IOMMU_READ | IOMMU_WRITE, GFP_KERNEL);
   650		else
   651			ret = iommu_unmap(rproc->domain, iova, mem_size);
   652	
   653		if (ret)
   654			dev_err(&rproc->dev, "Unable to %s IOVA Memory, ret: %d\n",
   655				map ? "map" : "unmap", ret);
   656	
   657		return ret;
   658	}
   659	EXPORT_SYMBOL_GPL(qcom_map_unmap_carveout);
   660	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/6] Peripheral Image Loader support for Qualcomm SoCs

[Dmitry Baryshkov]

On Sat, Oct 05, 2024 at 02:53:53AM GMT, Mukesh Ojha wrote:
> Qualcomm is looking to enable remote processors on the SA8775p SoC
> running KVM Linux host and is currently trying to figure out an
> upstream-compatible solution for the IOMMU translation scheme problem it
> is facing when SoCs running with KVM. This issue arises due to
> differences in how IOMMU translation is currently handled on SoCs
> running Qualcomm EL2 hypervisor(QHEE) where IOMMU translation for any
> device is completely owned by it and the other issue is that remote
> processors firmware does not contain resource table where these IOMMU
> configuration setting will be present.
> 
> Qualcomm SoCs running with the QHEE(EL2) have been utilizing the
> Peripheral Authentication Service (PAS) from its TrustZone (TZ) firmware
> to securely authenticate and reset via a single SMC call
> _auth_and_reset_.  This call first gets trapped to QHEE, which then
> makes a call to TZ for authentication. Once it is done, the call returns
> to QHEE, which sets up the IOMMU translation scheme for these remote
> processors and later brings them out of reset. The design of the
> Qualcomm EL2 hypervisor dictates that the Linux host OS running at EL1
> is not allowed to set up IOMMU translation for remote processors,
> and only a single stage is being configured for them.
> 
> To make the remote processors’ bring-up (PAS) sequence
> hypervisor-independent, the auth_and_reset SMC call is now entirely
> handled by TZ. However, the problem of IOMMU handling still remains with
> the KVM host, which has no knowledge of the remote processors’ IOMMU
> configuration.
> 
> We have looked up one approach where SoC remoteproc device tree could
> contain resources like iommus for remoteproc carveout and qcom,devmem
> specific binding for device memory needed for remoteproc and these
> properties are optional and will only be overlaid by the firmware if it
> is running with non-QHEE based hypervisor like KVM.

Can you follow the approach that has been implemented for existing
systems (ChromeOS) not using QHEE? See drivers/remoteproc/qcom_q6v5_adsp.c
If this approach can not be used, please describe why.

> 
> - Patch 1/6 adds the iommus and qcom,devmem binding for PAS common yaml.
> - Patch 2/6 and 3/6 add helper function to IOMMU map and unmap carveout
>   and device memory region.
> - Patch 4/6 adds a function to parse individual field of qcom,devmem property.
> - Patch 5/6 add helpers to create/destroy SHM bridge for remoteproc
>   carveout and to get memory from tzmem SHM bridge pool for remoteproc
>   firmware metadata.
> - Patch 6/6 enable all the required support to enable remoteproc for
>   non-QHEE hypervisor based systems like KVM host via parsing the iommus
>   properties and mapping/unmapping carveout and device memory based on
>   it.
> 
> Komal Bajaj (1):
>   remoteproc: qcom: Add iommu map_unmap helper function
> 
> Mukesh Ojha (2):
>   remoteproc: qcom: Add support of SHM bridge to enable memory
>     protection
>   remoteproc: qcom: Enable map/unmap and SHM bridge support
> 
> Shiraz Hashim (3):
>   dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and
>     qcom,devmem property
>   remoteproc: qcom: Add helper function to support IOMMU devmem
>     translation
>   remoteproc: qcom: Add support to parse qcom,devmem property
> 
>  .../bindings/remoteproc/qcom,pas-common.yaml  |  42 +++++
>  .../bindings/remoteproc/qcom,sa8775p-pas.yaml |  20 +++
>  drivers/firmware/qcom/qcom_scm.c              |  29 +++-
>  drivers/firmware/qcom/qcom_tzmem.c            |  14 +-
>  drivers/remoteproc/qcom_common.c              | 148 ++++++++++++++++++
>  drivers/remoteproc/qcom_common.h              |  38 +++++
>  drivers/remoteproc/qcom_q6v5_pas.c            | 140 ++++++++++++++++-
>  include/linux/firmware/qcom/qcom_scm.h        |   1 +
>  include/linux/firmware/qcom/qcom_tzmem.h      |  10 ++
>  9 files changed, 423 insertions(+), 19 deletions(-)
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Dmitry Baryshkov]

On Sat, Oct 05, 2024 at 02:53:54AM GMT, Mukesh Ojha wrote:
> From: Shiraz Hashim <quic_shashim@quicinc.com>
> 
> Qualcomm’s PAS implementation for remote processors only supports a
> single stage of IOMMU translation and is presently managed by the
> Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> such as with a KVM hypervisor, IOMMU translations need to be set up by
> the KVM host. Remoteproc needs carveout memory region and its resource
> (device memory) permissions to be set before it comes up, and this
> information is presently available statically with QHEE.
> 
> In the absence of QHEE, the boot firmware needs to overlay this
> information based on SoCs running with either QHEE or a KVM hypervisor
> (CPUs booted in EL2).
> 
> The qcom,devmem property provides IOMMU devmem translation information
> intended for non-QHEE based systems.
> 
> Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
> Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
>  .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> index 63a82e7a8bf8..068e177ad934 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> @@ -52,6 +52,48 @@ properties:
>      minItems: 1
>      maxItems: 3
>  
> +  iommus:
> +    maxItems: 1
> +
> +  qcom,devmem:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    description:
> +      Qualcomm’s PAS implementation for remote processors only supports a
> +      single stage of IOMMU translation and is presently managed by the
> +      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> +      such as with a KVM hypervisor, IOMMU translations need to be set up by
> +      the KVM host. Remoteproc might need some device resources and related
> +      access permissions to be set before it comes up, and this information is
> +      presently available statically with QHEE.
> +
> +      In the absence of QHEE, the boot firmware needs to overlay this
> +      information based on SoCs running with either QHEE or a KVM hypervisor
> +      (CPUs booted in EL2).
> +
> +      The qcom,devmem property provides IOMMU devmem translation information
> +      intended for non-QHEE based systems. It is an array of u32 values
> +      describing the device memory regions for which IOMMU translations need to
> +      be set up before bringing up Remoteproc. This array consists of 4-tuples
> +      defining the device address, physical address, size, and attribute flags
> +      with which it has to be mapped.

I'd expect that this kind of information is hardware-dependent. As such
it can go to the driver itself, rather than the device tree. The driver
can use compatible string to select the correct table.

> +
> +      remoteproc@3000000 {
> +          ...
> +
> +          qcom,devmem = <0x82000 0x82000 0x2000 0x3>,
> +                        <0x92000 0x92000 0x1000 0x1>;
> +      }
> +
> +    items:
> +      items:
> +        - description: device address
> +        - description: physical address
> +        - description: size of mapping
> +        - description: |
> +            iommu attributes - IOMMU_READ, IOMMU_WRITE, IOMMU_CACHE, IOMMU_NOEXEC, IOMMU_MMIO
> +          enum: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
> +                  25, 26, 27, 28, 29, 30, 31 ]

Attributes should definitely be defined and then the DT should use
defines rather than the raw values.

> +
>    qcom,smem-states:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      description: States used by the AP to signal the Hexagon core
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
> index 7fe401a06805..503c5c9d8ea7 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
> @@ -139,6 +139,26 @@ examples:
>          power-domains = <&rpmhpd RPMHPD_LCX>, <&rpmhpd RPMHPD_LMX>;
>          power-domain-names = "lcx", "lmx";
>  
> +        iommus = <&apps_smmu 0x3000 0x0>;
> +        qcom,devmem = <0x00110000 0x00110000 0x4000 0x1>,
> +                      <0x00123000 0x00123000 0x1000 0x3>,
> +                      <0x00124000 0x00124000 0x3000 0x3>,
> +                      <0x00127000 0x00127000 0x2000 0x3>,
> +                      <0x0012a000 0x0012a000 0x3000 0x3>,
> +                      <0x0012e000 0x0012e000 0x1000 0x3>,
> +                      <0x0012f000 0x0012f000 0x1000 0x1>,
> +                      <0x00144000 0x00144000 0x1000 0x1>,
> +                      <0x00148000 0x00148000 0x1000 0x1>,
> +                      <0x00149000 0x00149000 0xe000 0x3>,
> +                      <0x00157000 0x00157000 0x1000 0x3>,
> +                      <0x00158000 0x00158000 0xd000 0x3>,
> +                      <0x00165000 0x00165000 0x1000 0x3>,
> +                      <0x00172000 0x00172000 0x1000 0x3>,
> +                      <0x00173000 0x00173000 0x8000 0x3>,
> +                      <0x0017b000 0x0017b000 0x2000 0x3>,
> +                      <0x0017f000 0x0017f000 0x1000 0x3>,
> +                      <0x00184000 0x00184000 0x1000 0x1>;
> +
>          interconnects = <&lpass_ag_noc MASTER_LPASS_PROC 0 &mc_virt SLAVE_EBI1 0>;
>  
>          memory-region = <&pil_adsp_mem>;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[neil.armstrong]

On 04/10/2024 23:23, Mukesh Ojha wrote:
> For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> translation for remote processors is managed by QHEE and if the same SoC
> run under KVM, remoteproc carveout and devmem region should be IOMMU
> mapped from Linux PAS driver before remoteproc is brought up and
> unmapped once it is tear down and apart from this, SHM bridge also need
> to set up to enable memory protection on both remoteproc meta data
> memory as well as for the carveout region.
> 
> Enable the support required to run Qualcomm remoteprocs on non-QHEE
> hypervisors.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index ac339145e072..13bd13f1b989 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -122,6 +122,7 @@ struct qcom_adsp {
>   
>   	struct qcom_devmem_table *devmem;
>   	struct qcom_tzmem_area *tzmem;
> +	unsigned long sid;
>   };
>   
>   static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
> @@ -310,9 +311,21 @@ static int adsp_start(struct rproc *rproc)
>   	if (ret)
>   		return ret;
>   
> +	ret = qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, true, true, adsp->sid);
> +	if (ret) {
> +		dev_err(adsp->dev, "iommu mapping failed, ret: %d\n", ret);
> +		goto disable_irqs;
> +	}
> +
> +	ret = qcom_map_devmem(rproc, adsp->devmem, true, adsp->sid);
> +	if (ret) {
> +		dev_err(adsp->dev, "devmem iommu mapping failed, ret: %d\n", ret);
> +		goto unmap_carveout;
> +	}
> +
>   	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>   	if (ret < 0)
> -		goto disable_irqs;
> +		goto unmap_devmem;
>   
>   	ret = clk_prepare_enable(adsp->xo);
>   	if (ret)
> @@ -400,6 +413,10 @@ static int adsp_start(struct rproc *rproc)
>   	clk_disable_unprepare(adsp->xo);
>   disable_proxy_pds:
>   	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> +unmap_devmem:
> +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
> +unmap_carveout:
> +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
>   disable_irqs:
>   	qcom_q6v5_unprepare(&adsp->q6v5);
>   
> @@ -445,6 +462,9 @@ static int adsp_stop(struct rproc *rproc)
>   			dev_err(adsp->dev, "failed to shutdown dtb: %d\n", ret);
>   	}
>   
> +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
> +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
> +
>   	handover = qcom_q6v5_unprepare(&adsp->q6v5);
>   	if (handover)
>   		qcom_pas_handover(&adsp->q6v5);
> @@ -844,6 +864,25 @@ static int adsp_probe(struct platform_device *pdev)
>   	}
>   	platform_set_drvdata(pdev, adsp);
>   
> +	if (of_property_present(pdev->dev.of_node, "iommus")) {
> +		struct of_phandle_args args;
> +
> +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> +		if (ret < 0)
> +			return ret;
> +
> +		rproc->has_iommu = true;
> +		adsp->sid = args.args[0];
> +		of_node_put(args.np);
> +		ret = adsp_devmem_init(adsp);
> +		if (ret)
> +			return ret;

Why don't you get this table from the firmware like presumably QHEE does ?

Neil

> +
> +		adsp->pas_metadata.shm_bridge_needed = true;
> +	} else {
> +		rproc->has_iommu = false;
> +	}
> +
>   	ret = device_init_wakeup(adsp->dev, true);
>   	if (ret)
>   		goto free_rproc;

Re: [PATCH 3/6] remoteproc: qcom: Add helper function to support IOMMU devmem translation

[neil.armstrong]

On 04/10/2024 23:23, Mukesh Ojha wrote:
> From: Shiraz Hashim <quic_shashim@quicinc.com>
> 
> Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> translation set up for remote processors is managed by QHEE itself
> however, for a case when these remote processors has to run under KVM

This is not true, KVM is a Linux hypervisor, remote processors have
nothing to do with KVM, please rephrase.

> hypervisor, IOMMU translation need to setup from Linux remoteproc driver
> before it is brought up.
> 
> Add qcom_devmem_info and qcom_devmem_table data structure and make a
> common helper functions which caller can call if these translation need
> to be taken care by the driver to enable iommu devmem access for
> remoteproc processors.
> 
> Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
> Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/remoteproc/qcom_common.c | 96 ++++++++++++++++++++++++++++++++
>   drivers/remoteproc/qcom_common.h | 35 ++++++++++++
>   2 files changed, 131 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 1c7887dc65b4..644920972b58 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -658,5 +658,101 @@ int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t me
>   }
>   EXPORT_SYMBOL_GPL(qcom_map_unmap_carveout);
>   
> +/**
> + * qcom_map_devmem() - Map the device memories needed by Remoteproc using IOMMU
> + *
> + * When Qualcomm EL2 hypervisor(QHEE) present, device memories needed for remoteproc
> + * processors is managed by it and Linux remoteproc drivers should not call
> + * this and its respective unmap function in such scenario. This function
> + * should only be called if remoteproc IOMMU translation need to be managed
> + * from Linux side.
> + *
> + * @rproc: rproc handle
> + * @devmem_table: list of devmem regions to map
> + * @use_sid: decision to append sid to iova
> + * @sid: SID value
> + */
> +int qcom_map_devmem(struct rproc *rproc, struct qcom_devmem_table *devmem_table,
> +		    bool use_sid, unsigned long sid)
> +{
> +	struct qcom_devmem_info *info;
> +	unsigned long sid_def_val;
> +	int ret;
> +	int i;
> +
> +	if (!rproc->has_iommu)
> +		return 0;
> +
> +	if (!rproc->domain)
> +		return -EINVAL;
> +
> +	/* remoteproc may not have devmem data */
> +	if (!devmem_table)
> +		return 0;
> +
> +	if (use_sid && sid)
> +		sid_def_val = sid & SID_MASK_DEFAULT;
> +
> +	info = &devmem_table->entries[0];
> +	for (i = 0; i < devmem_table->num_entries; i++, info++) {
> +		/*
> +		 * Remote processor like ADSP supports upto 36 bit device
> +		 * address space and some of its clients like fastrpc uses
> +		 * upper 32-35 bits to keep lower 4 bits of its SID to use
> +		 * larger address space. To keep this consistent across other
> +		 * use cases add remoteproc SID configuration for firmware
> +		 * to IOMMU for carveouts.
> +		 */
> +		if (use_sid)
> +			info->da |= (sid_def_val << 32);
> +
> +		ret = iommu_map(rproc->domain, info->da, info->pa, info->len, info->flags, GFP_KERNEL);
> +		if (ret) {
> +			dev_err(&rproc->dev, "Unable to map devmem, ret: %d\n", ret);
> +			if (use_sid)
> +				info->da &= ~(SID_MASK_DEFAULT << 32);
> +			goto undo_mapping;
> +		}
> +	}
> +
> +	return 0;
> +
> +undo_mapping:
> +	for (i = i - 1; i >= 0; i--, info--) {
> +		iommu_unmap(rproc->domain, info->da, info->len);
> +		if (use_sid)
> +			info->da &= ~(SID_MASK_DEFAULT << 32);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_map_devmem);
> +
> +/**
> + * qcom_unmap_devmem() -  unmap the device memories needed by Remoteproc using IOMMU
> + *
> + * @rproc:		rproc handle
> + * @devmem_table:	list of devmem regions to unmap
> + * @use_sid:		decision to append sid to iova
> + */
> +void qcom_unmap_devmem(struct rproc *rproc, struct qcom_devmem_table *devmem_table, bool use_sid)
> +{
> +	struct qcom_devmem_info *info;
> +	int i;
> +
> +	if (!rproc->has_iommu || !rproc->domain || !devmem_table)
> +		return;
> +
> +	info = &devmem_table->entries[0];
> +	for (i = 0; i < devmem_table->num_entries; i++, info++) {
> +		iommu_unmap(rproc->domain, info->da, info->len);
> +		if (use_sid)
> +			info->da &= ~(SID_MASK_DEFAULT << 32);
> +	}
> +
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(qcom_unmap_devmem);
> +
>   MODULE_DESCRIPTION("Qualcomm Remoteproc helper driver");
>   MODULE_LICENSE("GPL v2");
> diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> index bbc41054e1ea..bbc684e1df01 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -41,6 +41,36 @@ struct qcom_rproc_pdm {
>   	struct auxiliary_device *adev;
>   };
>   
> +/**
> + * struct qcom_devmem_info - iommu devmem region
> + * @da: device address
> + * @pa: physical address
> + * @len: length (in bytes)
> + * @flags: iommu protection flags
> + *
> + * The resource entry carries the device address to which a physical address is
> + * to be mapped with required permissions in flag. The pa, len is expected to
> + * be a physically contiguous memory region.
> + */
> +struct qcom_devmem_info {
> +	u64 da;
> +	u64 pa;
> +	u32 len;
> +	u32 flags;
> +};
> +
> +/**
> + * struct qcom_devmem_table - iommu devmem entries
> + * @num_entries: number of devmem entries
> + * @entries: devmem entries
> + *
> + * The table that carries each devmem resource entry.
> + */
> +struct qcom_devmem_table {
> +	int num_entries;
> +	struct qcom_devmem_info entries[0];
> +};
> +
>   void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
>   			void (*rproc_dumpfn_t)(struct rproc *rproc,
>   				struct rproc_dump_segment *segment, void *dest, size_t offset,
> @@ -65,6 +95,11 @@ int qcom_map_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t me
>   void qcom_add_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm);
>   void qcom_remove_pdm_subdev(struct rproc *rproc, struct qcom_rproc_pdm *pdm);
>   
> +int qcom_map_devmem(struct rproc *rproc, struct qcom_devmem_table *table,
> +		    bool use_sid, unsigned long sid);
> +void qcom_unmap_devmem(struct rproc *rproc, struct qcom_devmem_table *table,
> +		       bool use_sid);
> +
>   #if IS_ENABLED(CONFIG_QCOM_SYSMON)
>   struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
>   					   const char *name,

Re: [PATCH 3/6] remoteproc: qcom: Add helper function to support IOMMU devmem translation

[Mukesh Ojha]

On Mon, Oct 07, 2024 at 10:08:16AM +0200, neil.armstrong@linaro.org wrote:
> On 04/10/2024 23:23, Mukesh Ojha wrote:
> > From: Shiraz Hashim <quic_shashim@quicinc.com>
> > 
> > Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > translation set up for remote processors is managed by QHEE itself
> > however, for a case when these remote processors has to run under KVM
> 
> This is not true, KVM is a Linux hypervisor, remote processors have
> nothing to do with KVM, please rephrase.

Thanks, perhaps something like this,

"However, when same SoC runs with KVM configuration, remoteproc IOMMU
translation needs to be set from Linux host running remoteproc PAS
driver"

-Mukesh

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Mukesh Ojha]

On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> On 04/10/2024 23:23, Mukesh Ojha wrote:
> > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > translation for remote processors is managed by QHEE and if the same SoC
> > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > mapped from Linux PAS driver before remoteproc is brought up and
> > unmapped once it is tear down and apart from this, SHM bridge also need
> > to set up to enable memory protection on both remoteproc meta data
> > memory as well as for the carveout region.
> > 
> > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > hypervisors.
> > 
> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > ---
> >   drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> >   1 file changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > index ac339145e072..13bd13f1b989 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > @@ -122,6 +122,7 @@ struct qcom_adsp {
> >   	struct qcom_devmem_table *devmem;
> >   	struct qcom_tzmem_area *tzmem;
> > +	unsigned long sid;
> >   };
> >   static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
> > @@ -310,9 +311,21 @@ static int adsp_start(struct rproc *rproc)
> >   	if (ret)
> >   		return ret;
> > +	ret = qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, true, true, adsp->sid);
> > +	if (ret) {
> > +		dev_err(adsp->dev, "iommu mapping failed, ret: %d\n", ret);
> > +		goto disable_irqs;
> > +	}
> > +
> > +	ret = qcom_map_devmem(rproc, adsp->devmem, true, adsp->sid);
> > +	if (ret) {
> > +		dev_err(adsp->dev, "devmem iommu mapping failed, ret: %d\n", ret);
> > +		goto unmap_carveout;
> > +	}
> > +
> >   	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> >   	if (ret < 0)
> > -		goto disable_irqs;
> > +		goto unmap_devmem;
> >   	ret = clk_prepare_enable(adsp->xo);
> >   	if (ret)
> > @@ -400,6 +413,10 @@ static int adsp_start(struct rproc *rproc)
> >   	clk_disable_unprepare(adsp->xo);
> >   disable_proxy_pds:
> >   	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> > +unmap_devmem:
> > +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
> > +unmap_carveout:
> > +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
> >   disable_irqs:
> >   	qcom_q6v5_unprepare(&adsp->q6v5);
> > @@ -445,6 +462,9 @@ static int adsp_stop(struct rproc *rproc)
> >   			dev_err(adsp->dev, "failed to shutdown dtb: %d\n", ret);
> >   	}
> > +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
> > +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
> > +
> >   	handover = qcom_q6v5_unprepare(&adsp->q6v5);
> >   	if (handover)
> >   		qcom_pas_handover(&adsp->q6v5);
> > @@ -844,6 +864,25 @@ static int adsp_probe(struct platform_device *pdev)
> >   	}
> >   	platform_set_drvdata(pdev, adsp);
> > +	if (of_property_present(pdev->dev.of_node, "iommus")) {
> > +		struct of_phandle_args args;
> > +
> > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		rproc->has_iommu = true;
> > +		adsp->sid = args.args[0];
> > +		of_node_put(args.np);
> > +		ret = adsp_devmem_init(adsp);
> > +		if (ret)
> > +			return ret;
> 
> Why don't you get this table from the firmware like presumably QHEE does ?

Well, AFAIK, QHEE(EL2) has this information statically present and does
not get it from anywhere., but will confirm this twice..

-Mukesh

Re: [PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Mukesh Ojha]

On Sun, Oct 06, 2024 at 10:38:01PM +0300, Dmitry Baryshkov wrote:
> On Sat, Oct 05, 2024 at 02:53:54AM GMT, Mukesh Ojha wrote:
> > From: Shiraz Hashim <quic_shashim@quicinc.com>
> > 
> > Qualcomm’s PAS implementation for remote processors only supports a
> > single stage of IOMMU translation and is presently managed by the
> > Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> > such as with a KVM hypervisor, IOMMU translations need to be set up by
> > the KVM host. Remoteproc needs carveout memory region and its resource
> > (device memory) permissions to be set before it comes up, and this
> > information is presently available statically with QHEE.
> > 
> > In the absence of QHEE, the boot firmware needs to overlay this
> > information based on SoCs running with either QHEE or a KVM hypervisor
> > (CPUs booted in EL2).
> > 
> > The qcom,devmem property provides IOMMU devmem translation information
> > intended for non-QHEE based systems.
> > 
> > Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
> > Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > ---
> >  .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
> >  .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
> >  2 files changed, 62 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > index 63a82e7a8bf8..068e177ad934 100644
> > --- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > @@ -52,6 +52,48 @@ properties:
> >      minItems: 1
> >      maxItems: 3
> >  
> > +  iommus:
> > +    maxItems: 1
> > +
> > +  qcom,devmem:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    description:
> > +      Qualcomm’s PAS implementation for remote processors only supports a
> > +      single stage of IOMMU translation and is presently managed by the
> > +      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> > +      such as with a KVM hypervisor, IOMMU translations need to be set up by
> > +      the KVM host. Remoteproc might need some device resources and related
> > +      access permissions to be set before it comes up, and this information is
> > +      presently available statically with QHEE.
> > +
> > +      In the absence of QHEE, the boot firmware needs to overlay this
> > +      information based on SoCs running with either QHEE or a KVM hypervisor
> > +      (CPUs booted in EL2).
> > +
> > +      The qcom,devmem property provides IOMMU devmem translation information
> > +      intended for non-QHEE based systems. It is an array of u32 values
> > +      describing the device memory regions for which IOMMU translations need to
> > +      be set up before bringing up Remoteproc. This array consists of 4-tuples
> > +      defining the device address, physical address, size, and attribute flags
> > +      with which it has to be mapped.
> 
> I'd expect that this kind of information is hardware-dependent. As such
> it can go to the driver itself, rather than the device tree. The driver
> can use compatible string to select the correct table.
> 

IIUC, are you saying that to move this into driver file and override the
compatible string via overlay ?

> > +
> > +      remoteproc@3000000 {
> > +          ...
> > +
> > +          qcom,devmem = <0x82000 0x82000 0x2000 0x3>,
> > +                        <0x92000 0x92000 0x1000 0x1>;
> > +      }
> > +
> > +    items:
> > +      items:
> > +        - description: device address
> > +        - description: physical address
> > +        - description: size of mapping
> > +        - description: |
> > +            iommu attributes - IOMMU_READ, IOMMU_WRITE, IOMMU_CACHE, IOMMU_NOEXEC, IOMMU_MMIO
> > +          enum: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
> > +                  25, 26, 27, 28, 29, 30, 31 ]
> 
> Attributes should definitely be defined and then the DT should use
> defines rather than the raw values.
>

Ack.

-Mukesh

Re: [PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Dmitry Baryshkov]

On Mon, 7 Oct 2024 at 17:35, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
> On Sun, Oct 06, 2024 at 10:38:01PM +0300, Dmitry Baryshkov wrote:
> > On Sat, Oct 05, 2024 at 02:53:54AM GMT, Mukesh Ojha wrote:
> > > From: Shiraz Hashim <quic_shashim@quicinc.com>
> > >
> > > Qualcomm’s PAS implementation for remote processors only supports a
> > > single stage of IOMMU translation and is presently managed by the
> > > Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> > > such as with a KVM hypervisor, IOMMU translations need to be set up by
> > > the KVM host. Remoteproc needs carveout memory region and its resource
> > > (device memory) permissions to be set before it comes up, and this
> > > information is presently available statically with QHEE.
> > >
> > > In the absence of QHEE, the boot firmware needs to overlay this
> > > information based on SoCs running with either QHEE or a KVM hypervisor
> > > (CPUs booted in EL2).
> > >
> > > The qcom,devmem property provides IOMMU devmem translation information
> > > intended for non-QHEE based systems.
> > >
> > > Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
> > > Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > >  .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
> > >  .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
> > >  2 files changed, 62 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > > index 63a82e7a8bf8..068e177ad934 100644
> > > --- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > > @@ -52,6 +52,48 @@ properties:
> > >      minItems: 1
> > >      maxItems: 3
> > >
> > > +  iommus:
> > > +    maxItems: 1
> > > +
> > > +  qcom,devmem:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > +    description:
> > > +      Qualcomm’s PAS implementation for remote processors only supports a
> > > +      single stage of IOMMU translation and is presently managed by the
> > > +      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> > > +      such as with a KVM hypervisor, IOMMU translations need to be set up by
> > > +      the KVM host. Remoteproc might need some device resources and related
> > > +      access permissions to be set before it comes up, and this information is
> > > +      presently available statically with QHEE.
> > > +
> > > +      In the absence of QHEE, the boot firmware needs to overlay this
> > > +      information based on SoCs running with either QHEE or a KVM hypervisor
> > > +      (CPUs booted in EL2).
> > > +
> > > +      The qcom,devmem property provides IOMMU devmem translation information
> > > +      intended for non-QHEE based systems. It is an array of u32 values
> > > +      describing the device memory regions for which IOMMU translations need to
> > > +      be set up before bringing up Remoteproc. This array consists of 4-tuples
> > > +      defining the device address, physical address, size, and attribute flags
> > > +      with which it has to be mapped.
> >
> > I'd expect that this kind of information is hardware-dependent. As such
> > it can go to the driver itself, rather than the device tree. The driver
> > can use compatible string to select the correct table.
> >
>
> IIUC, are you saying that to move this into driver file and override the
> compatible string via overlay ?

Ideally we should live without compat overrides. On the other hand,
sc7180 and sc7280 provide an example of doing exactly that.

>
> > > +
> > > +      remoteproc@3000000 {
> > > +          ...
> > > +
> > > +          qcom,devmem = <0x82000 0x82000 0x2000 0x3>,
> > > +                        <0x92000 0x92000 0x1000 0x1>;
> > > +      }
> > > +
> > > +    items:
> > > +      items:
> > > +        - description: device address
> > > +        - description: physical address
> > > +        - description: size of mapping
> > > +        - description: |
> > > +            iommu attributes - IOMMU_READ, IOMMU_WRITE, IOMMU_CACHE, IOMMU_NOEXEC, IOMMU_MMIO
> > > +          enum: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
> > > +                  25, 26, 27, 28, 29, 30, 31 ]
> >
> > Attributes should definitely be defined and then the DT should use
> > defines rather than the raw values.
> >
>
> Ack.
>
> -Mukesh
>


-- 
With best wishes
Dmitry

Re: [PATCH 0/6] Peripheral Image Loader support for Qualcomm SoCs

[Mukesh Ojha]

On Sun, Oct 06, 2024 at 10:34:19PM +0300, Dmitry Baryshkov wrote:
> On Sat, Oct 05, 2024 at 02:53:53AM GMT, Mukesh Ojha wrote:
> > Qualcomm is looking to enable remote processors on the SA8775p SoC
> > running KVM Linux host and is currently trying to figure out an
> > upstream-compatible solution for the IOMMU translation scheme problem it
> > is facing when SoCs running with KVM. This issue arises due to
> > differences in how IOMMU translation is currently handled on SoCs
> > running Qualcomm EL2 hypervisor(QHEE) where IOMMU translation for any
> > device is completely owned by it and the other issue is that remote
> > processors firmware does not contain resource table where these IOMMU
> > configuration setting will be present.
> > 
> > Qualcomm SoCs running with the QHEE(EL2) have been utilizing the
> > Peripheral Authentication Service (PAS) from its TrustZone (TZ) firmware
> > to securely authenticate and reset via a single SMC call
> > _auth_and_reset_.  This call first gets trapped to QHEE, which then
> > makes a call to TZ for authentication. Once it is done, the call returns
> > to QHEE, which sets up the IOMMU translation scheme for these remote
> > processors and later brings them out of reset. The design of the
> > Qualcomm EL2 hypervisor dictates that the Linux host OS running at EL1
> > is not allowed to set up IOMMU translation for remote processors,
> > and only a single stage is being configured for them.
> > 
> > To make the remote processors’ bring-up (PAS) sequence
> > hypervisor-independent, the auth_and_reset SMC call is now entirely
> > handled by TZ. However, the problem of IOMMU handling still remains with
> > the KVM host, which has no knowledge of the remote processors’ IOMMU
> > configuration.
> > 
> > We have looked up one approach where SoC remoteproc device tree could
> > contain resources like iommus for remoteproc carveout and qcom,devmem
> > specific binding for device memory needed for remoteproc and these
> > properties are optional and will only be overlaid by the firmware if it
> > is running with non-QHEE based hypervisor like KVM.
> 
> Can you follow the approach that has been implemented for existing
> systems (ChromeOS) not using QHEE? See drivers/remoteproc/qcom_q6v5_adsp.c
> If this approach can not be used, please describe why.
>

I believe, drivers/remoteproc/qcom_q6v5_adsp.c does not follow TZ's PAS
method (Secure) of handling remoteproc that may be the reason it has
been kept separately from drivers/remoteproc/qcom_q6v5_pas.c . If we
keep this implementation align with current PAS driver we would acheive
more code reusability with less code change.

However, I am not against, if we want to keep this as separate driver
with qcom_q6v5_pas_common.c shared between the current pas driver
with QHEE vs this implementation with non-QHEE.

-Mukesh

> > 
> > - Patch 1/6 adds the iommus and qcom,devmem binding for PAS common yaml.
> > - Patch 2/6 and 3/6 add helper function to IOMMU map and unmap carveout
> >   and device memory region.
> > - Patch 4/6 adds a function to parse individual field of qcom,devmem property.
> > - Patch 5/6 add helpers to create/destroy SHM bridge for remoteproc
> >   carveout and to get memory from tzmem SHM bridge pool for remoteproc
> >   firmware metadata.
> > - Patch 6/6 enable all the required support to enable remoteproc for
> >   non-QHEE hypervisor based systems like KVM host via parsing the iommus
> >   properties and mapping/unmapping carveout and device memory based on
> >   it.
> > 
> > Komal Bajaj (1):
> >   remoteproc: qcom: Add iommu map_unmap helper function
> > 
> > Mukesh Ojha (2):
> >   remoteproc: qcom: Add support of SHM bridge to enable memory
> >     protection
> >   remoteproc: qcom: Enable map/unmap and SHM bridge support
> > 
> > Shiraz Hashim (3):
> >   dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and
> >     qcom,devmem property
> >   remoteproc: qcom: Add helper function to support IOMMU devmem
> >     translation
> >   remoteproc: qcom: Add support to parse qcom,devmem property
> > 
> >  .../bindings/remoteproc/qcom,pas-common.yaml  |  42 +++++
> >  .../bindings/remoteproc/qcom,sa8775p-pas.yaml |  20 +++
> >  drivers/firmware/qcom/qcom_scm.c              |  29 +++-
> >  drivers/firmware/qcom/qcom_tzmem.c            |  14 +-
> >  drivers/remoteproc/qcom_common.c              | 148 ++++++++++++++++++
> >  drivers/remoteproc/qcom_common.h              |  38 +++++
> >  drivers/remoteproc/qcom_q6v5_pas.c            | 140 ++++++++++++++++-
> >  include/linux/firmware/qcom/qcom_scm.h        |   1 +
> >  include/linux/firmware/qcom/qcom_tzmem.h      |  10 ++
> >  9 files changed, 423 insertions(+), 19 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Mukesh Ojha]

On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > translation for remote processors is managed by QHEE and if the same SoC
> > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > mapped from Linux PAS driver before remoteproc is brought up and
> > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > to set up to enable memory protection on both remoteproc meta data
> > > memory as well as for the carveout region.
> > > 
> > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > hypervisors.
> > > 
> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > ---
> > >   drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > >   1 file changed, 40 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > index ac339145e072..13bd13f1b989 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > > @@ -122,6 +122,7 @@ struct qcom_adsp {
> > >   	struct qcom_devmem_table *devmem;
> > >   	struct qcom_tzmem_area *tzmem;
> > > +	unsigned long sid;
> > >   };
> > >   static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
> > > @@ -310,9 +311,21 @@ static int adsp_start(struct rproc *rproc)
> > >   	if (ret)
> > >   		return ret;
> > > +	ret = qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, true, true, adsp->sid);
> > > +	if (ret) {
> > > +		dev_err(adsp->dev, "iommu mapping failed, ret: %d\n", ret);
> > > +		goto disable_irqs;
> > > +	}
> > > +
> > > +	ret = qcom_map_devmem(rproc, adsp->devmem, true, adsp->sid);
> > > +	if (ret) {
> > > +		dev_err(adsp->dev, "devmem iommu mapping failed, ret: %d\n", ret);
> > > +		goto unmap_carveout;
> > > +	}
> > > +
> > >   	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> > >   	if (ret < 0)
> > > -		goto disable_irqs;
> > > +		goto unmap_devmem;
> > >   	ret = clk_prepare_enable(adsp->xo);
> > >   	if (ret)
> > > @@ -400,6 +413,10 @@ static int adsp_start(struct rproc *rproc)
> > >   	clk_disable_unprepare(adsp->xo);
> > >   disable_proxy_pds:
> > >   	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
> > > +unmap_devmem:
> > > +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
> > > +unmap_carveout:
> > > +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
> > >   disable_irqs:
> > >   	qcom_q6v5_unprepare(&adsp->q6v5);
> > > @@ -445,6 +462,9 @@ static int adsp_stop(struct rproc *rproc)
> > >   			dev_err(adsp->dev, "failed to shutdown dtb: %d\n", ret);
> > >   	}
> > > +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
> > > +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
> > > +
> > >   	handover = qcom_q6v5_unprepare(&adsp->q6v5);
> > >   	if (handover)
> > >   		qcom_pas_handover(&adsp->q6v5);
> > > @@ -844,6 +864,25 @@ static int adsp_probe(struct platform_device *pdev)
> > >   	}
> > >   	platform_set_drvdata(pdev, adsp);
> > > +	if (of_property_present(pdev->dev.of_node, "iommus")) {
> > > +		struct of_phandle_args args;
> > > +
> > > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		rproc->has_iommu = true;
> > > +		adsp->sid = args.args[0];
> > > +		of_node_put(args.np);
> > > +		ret = adsp_devmem_init(adsp);
> > > +		if (ret)
> > > +			return ret;
> > 
> > Why don't you get this table from the firmware like presumably QHEE does ?
> 
> Well, AFAIK, QHEE(EL2) has this information statically present and does
> not get it from anywhere., but will confirm this twice..

Double confirmed, device memory region required by remoteproc is
statically present with QHEE.

-Mukesh

> 
> -Mukesh
> 

Re: [PATCH 0/6] Peripheral Image Loader support for Qualcomm SoCs

[Shiraz Hashim]

On Sun, Oct 06, 2024 at 10:34:19PM +0300, Dmitry Baryshkov wrote:
> On Sat, Oct 05, 2024 at 02:53:53AM GMT, Mukesh Ojha wrote:
> > Qualcomm is looking to enable remote processors on the SA8775p SoC
> > running KVM Linux host and is currently trying to figure out an
> > upstream-compatible solution for the IOMMU translation scheme problem it
> > is facing when SoCs running with KVM. This issue arises due to
> > differences in how IOMMU translation is currently handled on SoCs
> > running Qualcomm EL2 hypervisor(QHEE) where IOMMU translation for any
> > device is completely owned by it and the other issue is that remote
> > processors firmware does not contain resource table where these IOMMU
> > configuration setting will be present.
> > 
> > Qualcomm SoCs running with the QHEE(EL2) have been utilizing the
> > Peripheral Authentication Service (PAS) from its TrustZone (TZ) firmware
> > to securely authenticate and reset via a single SMC call
> > _auth_and_reset_.  This call first gets trapped to QHEE, which then
> > makes a call to TZ for authentication. Once it is done, the call returns
> > to QHEE, which sets up the IOMMU translation scheme for these remote
> > processors and later brings them out of reset. The design of the
> > Qualcomm EL2 hypervisor dictates that the Linux host OS running at EL1
> > is not allowed to set up IOMMU translation for remote processors,
> > and only a single stage is being configured for them.
> > 
> > To make the remote processors’ bring-up (PAS) sequence
> > hypervisor-independent, the auth_and_reset SMC call is now entirely
> > handled by TZ. However, the problem of IOMMU handling still remains with
> > the KVM host, which has no knowledge of the remote processors’ IOMMU
> > configuration.
> > 
> > We have looked up one approach where SoC remoteproc device tree could
> > contain resources like iommus for remoteproc carveout and qcom,devmem
> > specific binding for device memory needed for remoteproc and these
> > properties are optional and will only be overlaid by the firmware if it
> > is running with non-QHEE based hypervisor like KVM.
> 
> Can you follow the approach that has been implemented for existing
> systems (ChromeOS) not using QHEE? See drivers/remoteproc/qcom_q6v5_adsp.c
> If this approach can not be used, please describe why.
> 

The intent is to reuse same PAS based remoteproc implementation
assisted by TZ with or without QHEE while qcom_q6v5_adsp.c caters to
independent control at Linux.
So it better suites to support it from qcom_q6v5_pas.c .

regards
Shiraz

Re: [PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Shiraz Hashim]

On Mon, Oct 07, 2024 at 06:25:01PM +0200, Dmitry Baryshkov wrote:
> On Mon, 7 Oct 2024 at 17:35, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> >
> > On Sun, Oct 06, 2024 at 10:38:01PM +0300, Dmitry Baryshkov wrote:
> > > On Sat, Oct 05, 2024 at 02:53:54AM GMT, Mukesh Ojha wrote:
> > > > From: Shiraz Hashim <quic_shashim@quicinc.com>
> > > >
> > > > Qualcomm’s PAS implementation for remote processors only supports a
> > > > single stage of IOMMU translation and is presently managed by the
> > > > Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> > > > such as with a KVM hypervisor, IOMMU translations need to be set up by
> > > > the KVM host. Remoteproc needs carveout memory region and its resource
> > > > (device memory) permissions to be set before it comes up, and this
> > > > information is presently available statically with QHEE.
> > > >
> > > > In the absence of QHEE, the boot firmware needs to overlay this
> > > > information based on SoCs running with either QHEE or a KVM hypervisor
> > > > (CPUs booted in EL2).
> > > >
> > > > The qcom,devmem property provides IOMMU devmem translation information
> > > > intended for non-QHEE based systems.
> > > >
> > > > Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
> > > > Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > ---
> > > >  .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
> > > >  .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
> > > >  2 files changed, 62 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > > > index 63a82e7a8bf8..068e177ad934 100644
> > > > --- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> > > > @@ -52,6 +52,48 @@ properties:
> > > >      minItems: 1
> > > >      maxItems: 3
> > > >
> > > > +  iommus:
> > > > +    maxItems: 1
> > > > +
> > > > +  qcom,devmem:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > > +    description:
> > > > +      Qualcomm’s PAS implementation for remote processors only supports a
> > > > +      single stage of IOMMU translation and is presently managed by the
> > > > +      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> > > > +      such as with a KVM hypervisor, IOMMU translations need to be set up by
> > > > +      the KVM host. Remoteproc might need some device resources and related
> > > > +      access permissions to be set before it comes up, and this information is
> > > > +      presently available statically with QHEE.
> > > > +
> > > > +      In the absence of QHEE, the boot firmware needs to overlay this
> > > > +      information based on SoCs running with either QHEE or a KVM hypervisor
> > > > +      (CPUs booted in EL2).
> > > > +
> > > > +      The qcom,devmem property provides IOMMU devmem translation information
> > > > +      intended for non-QHEE based systems. It is an array of u32 values
> > > > +      describing the device memory regions for which IOMMU translations need to
> > > > +      be set up before bringing up Remoteproc. This array consists of 4-tuples
> > > > +      defining the device address, physical address, size, and attribute flags
> > > > +      with which it has to be mapped.
> > >
> > > I'd expect that this kind of information is hardware-dependent. As such
> > > it can go to the driver itself, rather than the device tree. The driver
> > > can use compatible string to select the correct table.
> > >
> >
> > IIUC, are you saying that to move this into driver file and override the
> > compatible string via overlay ?
> 
> Ideally we should live without compat overrides. On the other hand,
> sc7180 and sc7280 provide an example of doing exactly that.

I am not sure if there can arise a case where updated adsp firmware
for particular board(s) may require additional access.

Having it in device tree adds a convenience to deal with such
variance. 

regards
Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[neil.armstrong]

On 08/10/2024 08:21, Mukesh Ojha wrote:
> On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
>> On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
>>> On 04/10/2024 23:23, Mukesh Ojha wrote:
>>>> For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
>>>> translation for remote processors is managed by QHEE and if the same SoC
>>>> run under KVM, remoteproc carveout and devmem region should be IOMMU
>>>> mapped from Linux PAS driver before remoteproc is brought up and
>>>> unmapped once it is tear down and apart from this, SHM bridge also need
>>>> to set up to enable memory protection on both remoteproc meta data
>>>> memory as well as for the carveout region.
>>>>
>>>> Enable the support required to run Qualcomm remoteprocs on non-QHEE
>>>> hypervisors.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
>>>>    1 file changed, 40 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>>>> index ac339145e072..13bd13f1b989 100644
>>>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>>>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>>>> @@ -122,6 +122,7 @@ struct qcom_adsp {
>>>>    	struct qcom_devmem_table *devmem;
>>>>    	struct qcom_tzmem_area *tzmem;
>>>> +	unsigned long sid;
>>>>    };
>>>>    static void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
>>>> @@ -310,9 +311,21 @@ static int adsp_start(struct rproc *rproc)
>>>>    	if (ret)
>>>>    		return ret;
>>>> +	ret = qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, true, true, adsp->sid);
>>>> +	if (ret) {
>>>> +		dev_err(adsp->dev, "iommu mapping failed, ret: %d\n", ret);
>>>> +		goto disable_irqs;
>>>> +	}
>>>> +
>>>> +	ret = qcom_map_devmem(rproc, adsp->devmem, true, adsp->sid);
>>>> +	if (ret) {
>>>> +		dev_err(adsp->dev, "devmem iommu mapping failed, ret: %d\n", ret);
>>>> +		goto unmap_carveout;
>>>> +	}
>>>> +
>>>>    	ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>>>>    	if (ret < 0)
>>>> -		goto disable_irqs;
>>>> +		goto unmap_devmem;
>>>>    	ret = clk_prepare_enable(adsp->xo);
>>>>    	if (ret)
>>>> @@ -400,6 +413,10 @@ static int adsp_start(struct rproc *rproc)
>>>>    	clk_disable_unprepare(adsp->xo);
>>>>    disable_proxy_pds:
>>>>    	adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>>>> +unmap_devmem:
>>>> +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
>>>> +unmap_carveout:
>>>> +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
>>>>    disable_irqs:
>>>>    	qcom_q6v5_unprepare(&adsp->q6v5);
>>>> @@ -445,6 +462,9 @@ static int adsp_stop(struct rproc *rproc)
>>>>    			dev_err(adsp->dev, "failed to shutdown dtb: %d\n", ret);
>>>>    	}
>>>> +	qcom_unmap_devmem(rproc, adsp->devmem, adsp->sid);
>>>> +	qcom_map_unmap_carveout(rproc, adsp->mem_phys, adsp->mem_size, false, true, adsp->sid);
>>>> +
>>>>    	handover = qcom_q6v5_unprepare(&adsp->q6v5);
>>>>    	if (handover)
>>>>    		qcom_pas_handover(&adsp->q6v5);
>>>> @@ -844,6 +864,25 @@ static int adsp_probe(struct platform_device *pdev)
>>>>    	}
>>>>    	platform_set_drvdata(pdev, adsp);
>>>> +	if (of_property_present(pdev->dev.of_node, "iommus")) {
>>>> +		struct of_phandle_args args;
>>>> +
>>>> +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>> +		rproc->has_iommu = true;
>>>> +		adsp->sid = args.args[0];
>>>> +		of_node_put(args.np);
>>>> +		ret = adsp_devmem_init(adsp);
>>>> +		if (ret)
>>>> +			return ret;
>>>
>>> Why don't you get this table from the firmware like presumably QHEE does ?
>>
>> Well, AFAIK, QHEE(EL2) has this information statically present and does
>> not get it from anywhere., but will confirm this twice..
> 
> Double confirmed, device memory region required by remoteproc is
> statically present with QHEE.

Right, in this case why those tables can't be embedded in the elf .resource_table
like it's done with qcom_q6v5_adsp.c by calling rproc_elf_load_rsc_table()
and let the remoteproc framework load the resource table and setup
the devmem ssmu_map ?

Neil

> 
> -Mukesh
> 
>>
>> -Mukesh
>>

Re: [PATCH 3/6] remoteproc: qcom: Add helper function to support IOMMU devmem translation

[neil.armstrong]

Hi,

On 07/10/2024 16:37, Mukesh Ojha wrote:
> On Mon, Oct 07, 2024 at 10:08:16AM +0200, neil.armstrong@linaro.org wrote:
>> On 04/10/2024 23:23, Mukesh Ojha wrote:
>>> From: Shiraz Hashim <quic_shashim@quicinc.com>
>>>
>>> Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
>>> translation set up for remote processors is managed by QHEE itself
>>> however, for a case when these remote processors has to run under KVM
>>
>> This is not true, KVM is a Linux hypervisor, remote processors have
>> nothing to do with KVM, please rephrase.
> 
> Thanks, perhaps something like this,
> 
> "However, when same SoC runs with KVM configuration, remoteproc IOMMU
> translation needs to be set from Linux host running remoteproc PAS
> driver"

Thanks but I still don't see what KVM has to do here, KVM is an an optional
Linux kernel feature, Linux can be configured without KVM and still perfectly
startup those remoteprocs.

Neil

> 
> -Mukesh
> 

Re: [PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Krzysztof Kozlowski]

On 09/10/2024 16:04, Shiraz Hashim wrote:
> On Mon, Oct 07, 2024 at 06:25:01PM +0200, Dmitry Baryshkov wrote:
>> On Mon, 7 Oct 2024 at 17:35, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>>>
>>> On Sun, Oct 06, 2024 at 10:38:01PM +0300, Dmitry Baryshkov wrote:
>>>> On Sat, Oct 05, 2024 at 02:53:54AM GMT, Mukesh Ojha wrote:
>>>>> From: Shiraz Hashim <quic_shashim@quicinc.com>
>>>>>
>>>>> Qualcomm’s PAS implementation for remote processors only supports a
>>>>> single stage of IOMMU translation and is presently managed by the
>>>>> Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
>>>>> such as with a KVM hypervisor, IOMMU translations need to be set up by
>>>>> the KVM host. Remoteproc needs carveout memory region and its resource
>>>>> (device memory) permissions to be set before it comes up, and this
>>>>> information is presently available statically with QHEE.
>>>>>
>>>>> In the absence of QHEE, the boot firmware needs to overlay this
>>>>> information based on SoCs running with either QHEE or a KVM hypervisor
>>>>> (CPUs booted in EL2).
>>>>>
>>>>> The qcom,devmem property provides IOMMU devmem translation information
>>>>> intended for non-QHEE based systems.
>>>>>
>>>>> Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
>>>>> Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>> ---
>>>>>  .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
>>>>>  .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
>>>>>  2 files changed, 62 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
>>>>> index 63a82e7a8bf8..068e177ad934 100644
>>>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
>>>>> @@ -52,6 +52,48 @@ properties:
>>>>>      minItems: 1
>>>>>      maxItems: 3
>>>>>
>>>>> +  iommus:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  qcom,devmem:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>>>> +    description:
>>>>> +      Qualcomm’s PAS implementation for remote processors only supports a
>>>>> +      single stage of IOMMU translation and is presently managed by the
>>>>> +      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
>>>>> +      such as with a KVM hypervisor, IOMMU translations need to be set up by
>>>>> +      the KVM host. Remoteproc might need some device resources and related
>>>>> +      access permissions to be set before it comes up, and this information is
>>>>> +      presently available statically with QHEE.
>>>>> +
>>>>> +      In the absence of QHEE, the boot firmware needs to overlay this
>>>>> +      information based on SoCs running with either QHEE or a KVM hypervisor
>>>>> +      (CPUs booted in EL2).
>>>>> +
>>>>> +      The qcom,devmem property provides IOMMU devmem translation information
>>>>> +      intended for non-QHEE based systems. It is an array of u32 values
>>>>> +      describing the device memory regions for which IOMMU translations need to
>>>>> +      be set up before bringing up Remoteproc. This array consists of 4-tuples
>>>>> +      defining the device address, physical address, size, and attribute flags
>>>>> +      with which it has to be mapped.
>>>>
>>>> I'd expect that this kind of information is hardware-dependent. As such
>>>> it can go to the driver itself, rather than the device tree. The driver
>>>> can use compatible string to select the correct table.
>>>>
>>>
>>> IIUC, are you saying that to move this into driver file and override the
>>> compatible string via overlay ?
>>
>> Ideally we should live without compat overrides. On the other hand,
>> sc7180 and sc7280 provide an example of doing exactly that.
> 
> I am not sure if there can arise a case where updated adsp firmware
> for particular board(s) may require additional access.
> 
> Having it in device tree adds a convenience to deal with such
> variance. 
> 

That's a downstream argument... Just look at the downstream DTS.
Everything, even software properties, can be added to DT, right?

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: remoteproc: qcom,pas-common: Introduce iommus and qcom,devmem property

[Shiraz Hashim]

On Thu, Oct 10, 2024 at 09:15:59AM +0200, Krzysztof Kozlowski wrote:
> On 09/10/2024 16:04, Shiraz Hashim wrote:
> > On Mon, Oct 07, 2024 at 06:25:01PM +0200, Dmitry Baryshkov wrote:
> >> On Mon, 7 Oct 2024 at 17:35, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> >>>
> >>> On Sun, Oct 06, 2024 at 10:38:01PM +0300, Dmitry Baryshkov wrote:
> >>>> On Sat, Oct 05, 2024 at 02:53:54AM GMT, Mukesh Ojha wrote:
> >>>>> From: Shiraz Hashim <quic_shashim@quicinc.com>
> >>>>>
> >>>>> Qualcomm’s PAS implementation for remote processors only supports a
> >>>>> single stage of IOMMU translation and is presently managed by the
> >>>>> Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> >>>>> such as with a KVM hypervisor, IOMMU translations need to be set up by
> >>>>> the KVM host. Remoteproc needs carveout memory region and its resource
> >>>>> (device memory) permissions to be set before it comes up, and this
> >>>>> information is presently available statically with QHEE.
> >>>>>
> >>>>> In the absence of QHEE, the boot firmware needs to overlay this
> >>>>> information based on SoCs running with either QHEE or a KVM hypervisor
> >>>>> (CPUs booted in EL2).
> >>>>>
> >>>>> The qcom,devmem property provides IOMMU devmem translation information
> >>>>> intended for non-QHEE based systems.
> >>>>>
> >>>>> Signed-off-by: Shiraz Hashim <quic_shashim@quicinc.com>
> >>>>> Co-Developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> >>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> >>>>> ---
> >>>>>  .../bindings/remoteproc/qcom,pas-common.yaml  | 42 +++++++++++++++++++
> >>>>>  .../bindings/remoteproc/qcom,sa8775p-pas.yaml | 20 +++++++++
> >>>>>  2 files changed, 62 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> >>>>> index 63a82e7a8bf8..068e177ad934 100644
> >>>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
> >>>>> @@ -52,6 +52,48 @@ properties:
> >>>>>      minItems: 1
> >>>>>      maxItems: 3
> >>>>>
> >>>>> +  iommus:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  qcom,devmem:
> >>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> >>>>> +    description:
> >>>>> +      Qualcomm’s PAS implementation for remote processors only supports a
> >>>>> +      single stage of IOMMU translation and is presently managed by the
> >>>>> +      Qualcomm EL2 hypervisor (QHEE) if it is present. In the absence of QHEE,
> >>>>> +      such as with a KVM hypervisor, IOMMU translations need to be set up by
> >>>>> +      the KVM host. Remoteproc might need some device resources and related
> >>>>> +      access permissions to be set before it comes up, and this information is
> >>>>> +      presently available statically with QHEE.
> >>>>> +
> >>>>> +      In the absence of QHEE, the boot firmware needs to overlay this
> >>>>> +      information based on SoCs running with either QHEE or a KVM hypervisor
> >>>>> +      (CPUs booted in EL2).
> >>>>> +
> >>>>> +      The qcom,devmem property provides IOMMU devmem translation information
> >>>>> +      intended for non-QHEE based systems. It is an array of u32 values
> >>>>> +      describing the device memory regions for which IOMMU translations need to
> >>>>> +      be set up before bringing up Remoteproc. This array consists of 4-tuples
> >>>>> +      defining the device address, physical address, size, and attribute flags
> >>>>> +      with which it has to be mapped.
> >>>>
> >>>> I'd expect that this kind of information is hardware-dependent. As such
> >>>> it can go to the driver itself, rather than the device tree. The driver
> >>>> can use compatible string to select the correct table.
> >>>>
> >>>
> >>> IIUC, are you saying that to move this into driver file and override the
> >>> compatible string via overlay ?
> >>
> >> Ideally we should live without compat overrides. On the other hand,
> >> sc7180 and sc7280 provide an example of doing exactly that.
> > 
> > I am not sure if there can arise a case where updated adsp firmware
> > for particular board(s) may require additional access.
> > 
> > Having it in device tree adds a convenience to deal with such
> > variance. 
> > 
> 
> That's a downstream argument... Just look at the downstream DTS.
> Everything, even software properties, can be added to DT, right?

I was thinking this binding is similar to iommu-addresses approach,
however that is under reserved-memory enumerating memory regions.

regards
Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Shiraz Hashim]

On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> On 08/10/2024 08:21, Mukesh Ojha wrote:
> > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > memory as well as for the carveout region.
> > > > > 
> > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > hypervisors.
> > > > > 
> > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > ---
> > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > index ac339145e072..13bd13f1b989 100644
> > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c

<snip>

> > > > > +		struct of_phandle_args args;
> > > > > +
> > > > > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +
> > > > > +		rproc->has_iommu = true;
> > > > > +		adsp->sid = args.args[0];
> > > > > +		of_node_put(args.np);
> > > > > +		ret = adsp_devmem_init(adsp);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > 
> > > > Why don't you get this table from the firmware like presumably
> > > > QHEE does ?
> > > 
> > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > and does not get it from anywhere., but will confirm this
> > > twice..
> > 
> > Double confirmed, device memory region required by remoteproc is
> > statically present with QHEE.
> 
> Right, in this case why those tables can't be embedded in the elf
> .resource_table like it's done with qcom_q6v5_adsp.c by calling
> rproc_elf_load_rsc_table() and let the remoteproc framework load the
> resource table and setup the devmem ssmu_map ?

Mainly for two reasons -

firmware images on platforms where we like to bring additional no-qhee
support do not have resource table.

QCOM PAS implementation for secure remoteproc supports single TZ call
of auth_and_rest that authenticates and brings remoteproc out of
reset. And we don't have provision to authenticate resource table
before it is used for devmem/iommu setup.

regards
Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Dmitry Baryshkov]

On Fri, Oct 11, 2024 at 10:35:18AM GMT, Shiraz Hashim wrote:
> On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > memory as well as for the carveout region.
> > > > > > 
> > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > hypervisors.
> > > > > > 
> > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > ---
> > > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> 
> <snip>
> 
> > > > > > +		struct of_phandle_args args;
> > > > > > +
> > > > > > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		rproc->has_iommu = true;
> > > > > > +		adsp->sid = args.args[0];
> > > > > > +		of_node_put(args.np);
> > > > > > +		ret = adsp_devmem_init(adsp);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > 
> > > > > Why don't you get this table from the firmware like presumably
> > > > > QHEE does ?
> > > > 
> > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > and does not get it from anywhere., but will confirm this
> > > > twice..
> > > 
> > > Double confirmed, device memory region required by remoteproc is
> > > statically present with QHEE.
> > 
> > Right, in this case why those tables can't be embedded in the elf
> > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > resource table and setup the devmem ssmu_map ?
> 
> Mainly for two reasons -
> 
> firmware images on platforms where we like to bring additional no-qhee
> support do not have resource table.
> 
> QCOM PAS implementation for secure remoteproc supports single TZ call
> of auth_and_rest that authenticates and brings remoteproc out of
> reset. And we don't have provision to authenticate resource table
> before it is used for devmem/iommu setup.

So normally TZ / QHEE have the platform-specific resource table? Isn't
it tied to the firmware binary?

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Shiraz Hashim]

On Fri, Oct 11, 2024 at 09:23:05AM +0300, Dmitry Baryshkov wrote:
> On Fri, Oct 11, 2024 at 10:35:18AM GMT, Shiraz Hashim wrote:
> > On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > > memory as well as for the carveout region.
> > > > > > > 
> > > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > > hypervisors.
> > > > > > > 
> > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > > ---
> > > > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > 
> > <snip>
> > 
> > > > > > > +		struct of_phandle_args args;
> > > > > > > +
> > > > > > > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > > +		if (ret < 0)
> > > > > > > +			return ret;
> > > > > > > +
> > > > > > > +		rproc->has_iommu = true;
> > > > > > > +		adsp->sid = args.args[0];
> > > > > > > +		of_node_put(args.np);
> > > > > > > +		ret = adsp_devmem_init(adsp);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > 
> > > > > > Why don't you get this table from the firmware like presumably
> > > > > > QHEE does ?
> > > > > 
> > > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > > and does not get it from anywhere., but will confirm this
> > > > > twice..
> > > > 
> > > > Double confirmed, device memory region required by remoteproc is
> > > > statically present with QHEE.
> > > 
> > > Right, in this case why those tables can't be embedded in the elf
> > > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > > resource table and setup the devmem ssmu_map ?
> > 
> > Mainly for two reasons -
> > 
> > firmware images on platforms where we like to bring additional no-qhee
> > support do not have resource table.
> > 
> > QCOM PAS implementation for secure remoteproc supports single TZ call
> > of auth_and_rest that authenticates and brings remoteproc out of
> > reset. And we don't have provision to authenticate resource table
> > before it is used for devmem/iommu setup.
> 
> So normally TZ / QHEE have the platform-specific resource table? Isn't
> it tied to the firmware binary?

Yes this table is with QHEE and not firmware binary. Now with no-qhee
case, this patch series is proposing to get it from device tree.

regards
Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[neil.armstrong]

On 11/10/2024 07:05, Shiraz Hashim wrote:
> On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
>> On 08/10/2024 08:21, Mukesh Ojha wrote:
>>> On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
>>>> On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
>>>>> On 04/10/2024 23:23, Mukesh Ojha wrote:
>>>>>> For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
>>>>>> translation for remote processors is managed by QHEE and if the same SoC
>>>>>> run under KVM, remoteproc carveout and devmem region should be IOMMU
>>>>>> mapped from Linux PAS driver before remoteproc is brought up and
>>>>>> unmapped once it is tear down and apart from this, SHM bridge also need
>>>>>> to set up to enable memory protection on both remoteproc meta data
>>>>>> memory as well as for the carveout region.
>>>>>>
>>>>>> Enable the support required to run Qualcomm remoteprocs on non-QHEE
>>>>>> hypervisors.
>>>>>>
>>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>>> ---
>>>>>>     drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
>>>>>>     1 file changed, 40 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
>>>>>> index ac339145e072..13bd13f1b989 100644
>>>>>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>>>>>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> 
> <snip>
> 
>>>>>> +		struct of_phandle_args args;
>>>>>> +
>>>>>> +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
>>>>>> +		if (ret < 0)
>>>>>> +			return ret;
>>>>>> +
>>>>>> +		rproc->has_iommu = true;
>>>>>> +		adsp->sid = args.args[0];
>>>>>> +		of_node_put(args.np);
>>>>>> +		ret = adsp_devmem_init(adsp);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>
>>>>> Why don't you get this table from the firmware like presumably
>>>>> QHEE does ?
>>>>
>>>> Well, AFAIK, QHEE(EL2) has this information statically present
>>>> and does not get it from anywhere., but will confirm this
>>>> twice..
>>>
>>> Double confirmed, device memory region required by remoteproc is
>>> statically present with QHEE.
>>
>> Right, in this case why those tables can't be embedded in the elf
>> .resource_table like it's done with qcom_q6v5_adsp.c by calling
>> rproc_elf_load_rsc_table() and let the remoteproc framework load the
>> resource table and setup the devmem ssmu_map ?
> 
> Mainly for two reasons -
> 
> firmware images on platforms where we like to bring additional no-qhee
> support do not have resource table.
> 
> QCOM PAS implementation for secure remoteproc supports single TZ call
> of auth_and_rest that authenticates and brings remoteproc out of
> reset. And we don't have provision to authenticate resource table
> before it is used for devmem/iommu setup.

Why not authenticate a separate binary containing the resource table ?

Adding the resources to DT is a no go since it's clearly related to what
the firmare will be using at runtime, so either it should go in a
.resource_table section or can be moved in a signed .mbn that can
be authenticated by TZ.

Remoteproc doesn't dictate how to load the resource table, there's helpers
to load it from an elf, but it can be loaded by other means.

Neil

> 
> regards
> Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Mukesh Ojha]

On Fri, Oct 11, 2024 at 09:23:05AM +0300, Dmitry Baryshkov wrote:
> On Fri, Oct 11, 2024 at 10:35:18AM GMT, Shiraz Hashim wrote:
> > On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > > memory as well as for the carveout region.
> > > > > > > 
> > > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > > hypervisors.
> > > > > > > 
> > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > > ---
> > > > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > 
> > <snip>
> > 
> > > > > > > +		struct of_phandle_args args;
> > > > > > > +
> > > > > > > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > > +		if (ret < 0)
> > > > > > > +			return ret;
> > > > > > > +
> > > > > > > +		rproc->has_iommu = true;
> > > > > > > +		adsp->sid = args.args[0];
> > > > > > > +		of_node_put(args.np);
> > > > > > > +		ret = adsp_devmem_init(adsp);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > 
> > > > > > Why don't you get this table from the firmware like presumably
> > > > > > QHEE does ?
> > > > > 
> > > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > > and does not get it from anywhere., but will confirm this
> > > > > twice..
> > > > 
> > > > Double confirmed, device memory region required by remoteproc is
> > > > statically present with QHEE.
> > > 
> > > Right, in this case why those tables can't be embedded in the elf
> > > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > > resource table and setup the devmem ssmu_map ?
> > 
> > Mainly for two reasons -
> > 
> > firmware images on platforms where we like to bring additional no-qhee
> > support do not have resource table.
> > 
> > QCOM PAS implementation for secure remoteproc supports single TZ call
> > of auth_and_rest that authenticates and brings remoteproc out of
> > reset. And we don't have provision to authenticate resource table
> > before it is used for devmem/iommu setup.
> 
> So normally TZ / QHEE have the platform-specific resource table? Isn't
> it tied to the firmware binary?

It is not TZ, but QHEE that statically keeps remoteproc resources with it.

-Mukesh

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Dmitry Baryshkov]

On Fri, 11 Oct 2024 at 10:09, Shiraz Hashim <quic_shashim@quicinc.com> wrote:
>
> On Fri, Oct 11, 2024 at 09:23:05AM +0300, Dmitry Baryshkov wrote:
> > On Fri, Oct 11, 2024 at 10:35:18AM GMT, Shiraz Hashim wrote:
> > > On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > > > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > > > memory as well as for the carveout region.
> > > > > > > >
> > > > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > > > hypervisors.
> > > > > > > >
> > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > > > ---
> > > > > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > >
> > > <snip>
> > >
> > > > > > > > +         struct of_phandle_args args;
> > > > > > > > +
> > > > > > > > +         ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > > > +         if (ret < 0)
> > > > > > > > +                 return ret;
> > > > > > > > +
> > > > > > > > +         rproc->has_iommu = true;
> > > > > > > > +         adsp->sid = args.args[0];
> > > > > > > > +         of_node_put(args.np);
> > > > > > > > +         ret = adsp_devmem_init(adsp);
> > > > > > > > +         if (ret)
> > > > > > > > +                 return ret;
> > > > > > >
> > > > > > > Why don't you get this table from the firmware like presumably
> > > > > > > QHEE does ?
> > > > > >
> > > > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > > > and does not get it from anywhere., but will confirm this
> > > > > > twice..
> > > > >
> > > > > Double confirmed, device memory region required by remoteproc is
> > > > > statically present with QHEE.
> > > >
> > > > Right, in this case why those tables can't be embedded in the elf
> > > > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > > > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > > > resource table and setup the devmem ssmu_map ?
> > >
> > > Mainly for two reasons -
> > >
> > > firmware images on platforms where we like to bring additional no-qhee
> > > support do not have resource table.
> > >
> > > QCOM PAS implementation for secure remoteproc supports single TZ call
> > > of auth_and_rest that authenticates and brings remoteproc out of
> > > reset. And we don't have provision to authenticate resource table
> > > before it is used for devmem/iommu setup.
> >
> > So normally TZ / QHEE have the platform-specific resource table? Isn't
> > it tied to the firmware binary?
>
> Yes this table is with QHEE and not firmware binary. Now with no-qhee
> case, this patch series is proposing to get it from device tree.

If it is platform-specific (rather than being device-specific), then
it should go to the driver, not the DT.

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Shiraz Hashim]

On Fri, Oct 11, 2024 at 09:09:08AM +0200, neil.armstrong@linaro.org wrote:
> On 11/10/2024 07:05, Shiraz Hashim wrote:
> > On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > > memory as well as for the carveout region.
> > > > > > > 
> > > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > > hypervisors.
> > > > > > > 
> > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > > ---
> > > > > > >     drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > > >     1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > 
> > <snip>
> > 
> > > > > > > +		struct of_phandle_args args;
> > > > > > > +
> > > > > > > +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > > +		if (ret < 0)
> > > > > > > +			return ret;
> > > > > > > +
> > > > > > > +		rproc->has_iommu = true;
> > > > > > > +		adsp->sid = args.args[0];
> > > > > > > +		of_node_put(args.np);
> > > > > > > +		ret = adsp_devmem_init(adsp);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > 
> > > > > > Why don't you get this table from the firmware like presumably
> > > > > > QHEE does ?
> > > > > 
> > > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > > and does not get it from anywhere., but will confirm this
> > > > > twice..
> > > > 
> > > > Double confirmed, device memory region required by remoteproc is
> > > > statically present with QHEE.
> > > 
> > > Right, in this case why those tables can't be embedded in the elf
> > > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > > resource table and setup the devmem ssmu_map ?
> > 
> > Mainly for two reasons -
> > 
> > firmware images on platforms where we like to bring additional no-qhee
> > support do not have resource table.
> > 
> > QCOM PAS implementation for secure remoteproc supports single TZ call
> > of auth_and_rest that authenticates and brings remoteproc out of
> > reset. And we don't have provision to authenticate resource table
> > before it is used for devmem/iommu setup.
> 
> Why not authenticate a separate binary containing the resource table ?
> 
> Adding the resources to DT is a no go since it's clearly related to what
> the firmare will be using at runtime,

Sorry didn't understand how is it classified as runtime. Similar to
resources required to bring up a device, these correspond to resources
required to be handled before bringing up a remoteproc.

> so either it should go in a .resource_table section or can be moved
> in a signed .mbn that can be authenticated by TZ.

TZ doesn't have a separate authentication call as of now.

If DT is strictly a no go, would moving it to driver itself be an
acceptable option ? inline with what Dmitry suggesting.

regards
Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Shiraz Hashim]

On Fri, Oct 11, 2024 at 10:12:09AM +0300, Dmitry Baryshkov wrote:
> On Fri, 11 Oct 2024 at 10:09, Shiraz Hashim <quic_shashim@quicinc.com> wrote:
> >
> > On Fri, Oct 11, 2024 at 09:23:05AM +0300, Dmitry Baryshkov wrote:
> > > On Fri, Oct 11, 2024 at 10:35:18AM GMT, Shiraz Hashim wrote:
> > > > On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > > > > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > > > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > > > > memory as well as for the carveout region.
> > > > > > > > >
> > > > > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > > > > hypervisors.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > > > > ---
> > > > > > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > > >
> > > > <snip>
> > > >
> > > > > > > > > +         struct of_phandle_args args;
> > > > > > > > > +
> > > > > > > > > +         ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > > > > +         if (ret < 0)
> > > > > > > > > +                 return ret;
> > > > > > > > > +
> > > > > > > > > +         rproc->has_iommu = true;
> > > > > > > > > +         adsp->sid = args.args[0];
> > > > > > > > > +         of_node_put(args.np);
> > > > > > > > > +         ret = adsp_devmem_init(adsp);
> > > > > > > > > +         if (ret)
> > > > > > > > > +                 return ret;
> > > > > > > >
> > > > > > > > Why don't you get this table from the firmware like presumably
> > > > > > > > QHEE does ?
> > > > > > >
> > > > > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > > > > and does not get it from anywhere., but will confirm this
> > > > > > > twice..
> > > > > >
> > > > > > Double confirmed, device memory region required by remoteproc is
> > > > > > statically present with QHEE.
> > > > >
> > > > > Right, in this case why those tables can't be embedded in the elf
> > > > > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > > > > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > > > > resource table and setup the devmem ssmu_map ?
> > > >
> > > > Mainly for two reasons -
> > > >
> > > > firmware images on platforms where we like to bring additional no-qhee
> > > > support do not have resource table.
> > > >
> > > > QCOM PAS implementation for secure remoteproc supports single TZ call
> > > > of auth_and_rest that authenticates and brings remoteproc out of
> > > > reset. And we don't have provision to authenticate resource table
> > > > before it is used for devmem/iommu setup.
> > >
> > > So normally TZ / QHEE have the platform-specific resource table? Isn't
> > > it tied to the firmware binary?
> >
> > Yes this table is with QHEE and not firmware binary. Now with no-qhee
> > case, this patch series is proposing to get it from device tree.
> 
> If it is platform-specific (rather than being device-specific), then
> it should go to the driver, not the DT.

Just to be clear, your reference to platform is SoC specific and
device is board ?

regards
Shiraz

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Dmitry Baryshkov]

On Mon, 14 Oct 2024 at 15:31, Shiraz Hashim <quic_shashim@quicinc.com> wrote:
>
> On Fri, Oct 11, 2024 at 10:12:09AM +0300, Dmitry Baryshkov wrote:
> > On Fri, 11 Oct 2024 at 10:09, Shiraz Hashim <quic_shashim@quicinc.com> wrote:
> > >
> > > On Fri, Oct 11, 2024 at 09:23:05AM +0300, Dmitry Baryshkov wrote:
> > > > On Fri, Oct 11, 2024 at 10:35:18AM GMT, Shiraz Hashim wrote:
> > > > > On Thu, Oct 10, 2024 at 08:57:56AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > On 08/10/2024 08:21, Mukesh Ojha wrote:
> > > > > > > On Mon, Oct 07, 2024 at 08:22:39PM +0530, Mukesh Ojha wrote:
> > > > > > > > On Mon, Oct 07, 2024 at 10:05:08AM +0200, neil.armstrong@linaro.org wrote:
> > > > > > > > > On 04/10/2024 23:23, Mukesh Ojha wrote:
> > > > > > > > > > For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> > > > > > > > > > translation for remote processors is managed by QHEE and if the same SoC
> > > > > > > > > > run under KVM, remoteproc carveout and devmem region should be IOMMU
> > > > > > > > > > mapped from Linux PAS driver before remoteproc is brought up and
> > > > > > > > > > unmapped once it is tear down and apart from this, SHM bridge also need
> > > > > > > > > > to set up to enable memory protection on both remoteproc meta data
> > > > > > > > > > memory as well as for the carveout region.
> > > > > > > > > >
> > > > > > > > > > Enable the support required to run Qualcomm remoteprocs on non-QHEE
> > > > > > > > > > hypervisors.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > > > > > > > ---
> > > > > > > > > >    drivers/remoteproc/qcom_q6v5_pas.c | 41 +++++++++++++++++++++++++++++-
> > > > > > > > > >    1 file changed, 40 insertions(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > > > > index ac339145e072..13bd13f1b989 100644
> > > > > > > > > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > > > > > > > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > > > >
> > > > > <snip>
> > > > >
> > > > > > > > > > +         struct of_phandle_args args;
> > > > > > > > > > +
> > > > > > > > > > +         ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> > > > > > > > > > +         if (ret < 0)
> > > > > > > > > > +                 return ret;
> > > > > > > > > > +
> > > > > > > > > > +         rproc->has_iommu = true;
> > > > > > > > > > +         adsp->sid = args.args[0];
> > > > > > > > > > +         of_node_put(args.np);
> > > > > > > > > > +         ret = adsp_devmem_init(adsp);
> > > > > > > > > > +         if (ret)
> > > > > > > > > > +                 return ret;
> > > > > > > > >
> > > > > > > > > Why don't you get this table from the firmware like presumably
> > > > > > > > > QHEE does ?
> > > > > > > >
> > > > > > > > Well, AFAIK, QHEE(EL2) has this information statically present
> > > > > > > > and does not get it from anywhere., but will confirm this
> > > > > > > > twice..
> > > > > > >
> > > > > > > Double confirmed, device memory region required by remoteproc is
> > > > > > > statically present with QHEE.
> > > > > >
> > > > > > Right, in this case why those tables can't be embedded in the elf
> > > > > > .resource_table like it's done with qcom_q6v5_adsp.c by calling
> > > > > > rproc_elf_load_rsc_table() and let the remoteproc framework load the
> > > > > > resource table and setup the devmem ssmu_map ?
> > > > >
> > > > > Mainly for two reasons -
> > > > >
> > > > > firmware images on platforms where we like to bring additional no-qhee
> > > > > support do not have resource table.
> > > > >
> > > > > QCOM PAS implementation for secure remoteproc supports single TZ call
> > > > > of auth_and_rest that authenticates and brings remoteproc out of
> > > > > reset. And we don't have provision to authenticate resource table
> > > > > before it is used for devmem/iommu setup.
> > > >
> > > > So normally TZ / QHEE have the platform-specific resource table? Isn't
> > > > it tied to the firmware binary?
> > >
> > > Yes this table is with QHEE and not firmware binary. Now with no-qhee
> > > case, this patch series is proposing to get it from device tree.
> >
> > If it is platform-specific (rather than being device-specific), then
> > it should go to the driver, not the DT.
>
> Just to be clear, your reference to platform is SoC specific and
> device is board ?

Yes.


-- 
With best wishes
Dmitry

Re: [PATCH 3/6] remoteproc: qcom: Add helper function to support IOMMU devmem translation

[Konrad Dybcio]

On 10.10.2024 8:59 AM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 07/10/2024 16:37, Mukesh Ojha wrote:
>> On Mon, Oct 07, 2024 at 10:08:16AM +0200, neil.armstrong@linaro.org wrote:
>>> On 04/10/2024 23:23, Mukesh Ojha wrote:
>>>> From: Shiraz Hashim <quic_shashim@quicinc.com>
>>>>
>>>> Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
>>>> translation set up for remote processors is managed by QHEE itself
>>>> however, for a case when these remote processors has to run under KVM
>>>
>>> This is not true, KVM is a Linux hypervisor, remote processors have
>>> nothing to do with KVM, please rephrase.
>>
>> Thanks, perhaps something like this,
>>
>> "However, when same SoC runs with KVM configuration, remoteproc IOMMU
>> translation needs to be set from Linux host running remoteproc PAS
>> driver"
> 
> Thanks but I still don't see what KVM has to do here, KVM is an an optional
> Linux kernel feature, Linux can be configured without KVM and still perfectly
> startup those remoteprocs.

Mukesh, KVM is a very specific use case. What you're referring to is
really "no QHEE / Gunyah". We can do s/KVM/Hyper-V (or almost any
other software running at EL2) and your claims still hold.

Konrad

Re: [PATCH 5/6] remoteproc: qcom: Add support of SHM bridge to enable memory protection

[Konrad Dybcio]

On 4.10.2024 11:23 PM, Mukesh Ojha wrote:
> Qualcomm SoCs running with the Qualcomm EL2 hypervisor(QHEE) have been
> utilizing the Peripheral Authentication Service (PAS) from its TrustZone
> (TZ) firmware to securely authenticate and reset via sequence of SMC
> calls like qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(), and
> qcom_scm_pas_auth_and_reset().
> 
> Memory protection need to be enabled for both meta data memory region and
> remoteproc carveout memory region.

Will TZ refuse to start the remoteprocs if this is not the case?

> For memory passed to Qualcomm TrustZone, the memory should be part of
> SHM bridge memory. However, when QHEE is present, PAS SMC calls are
> getting trapped in QHEE, which create or gets memory from SHM bridge for
> both meta data memory and for remoteproc carve out regions before it get
> passed to TZ.  However, in absence of QHEE hypervisor, Linux need to
> create SHM bridge for both meta data in qcom_scm_pas_init_image() and
> for remoteproc memory before the call being made to
> qcom_scm_pas_auth_and_reset().
> 
> For qcom_scm_pas_init_image() call, metadata content need to be copied
> to the buffer allocated from SHM bridge before making the SMC call.
> 
> For qcom_scm_pas_auth_and_reset(), remoteproc memory region need to be
> protected and for that SHM bridge need to be created. Make
> qcom_tzmem_init_area() and qcom_tzmem_cleanup_area() exported symbol so
> that it could be used to create SHM bridge for remoteproc region.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c         | 29 +++++++++++-----
>  drivers/firmware/qcom/qcom_tzmem.c       | 14 +++-----
>  drivers/remoteproc/qcom_q6v5_pas.c       | 44 ++++++++++++++++++++++++
>  include/linux/firmware/qcom/qcom_scm.h   |  1 +
>  include/linux/firmware/qcom/qcom_tzmem.h | 10 ++++++
>  5 files changed, 80 insertions(+), 18 deletions(-)

This changes files in two separate subsystems. That implies this
patch should be split in two. Or three.

> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..dafc07dc181f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -591,15 +591,19 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>  	 * data blob, so make sure it's physically contiguous, 4K aligned and
>  	 * non-cachable to avoid XPU violations.
>  	 *
> -	 * For PIL calls the hypervisor creates SHM Bridges for the blob
> -	 * buffers on behalf of Linux so we must not do it ourselves hence
> -	 * not using the TZMem allocator here.
> +	 * For PIL calls the hypervisor like Gunyah or older QHEE creates SHM
> +	 * Bridges for the blob buffers on behalf of Linux so we must not do it
> +	 * ourselves hence use TZMem allocator only when these hypervisors are
> +	 * not present.

This is a bit hard to read.. How about:

PIL calls require SHMBridge is set up for shared memory regions.
Qualcomm hypervisors (Gunyah, QHEE) already take care of this.
Only create new bridges if they're absent.

[...]

Konrad

Re: [PATCH 6/6] remoteproc: qcom: Enable map/unmap and SHM bridge support

[Konrad Dybcio]

On 4.10.2024 11:23 PM, Mukesh Ojha wrote:
> For Qualcomm SoCs runnning with Qualcomm EL2 hypervisor(QHEE), IOMMU
> translation for remote processors is managed by QHEE and if the same SoC
> run under KVM, remoteproc carveout and devmem region should be IOMMU
> mapped from Linux PAS driver before remoteproc is brought up and
> unmapped once it is tear down and apart from this, SHM bridge also need
> to set up to enable memory protection on both remoteproc meta data
> memory as well as for the carveout region.

!Gunyah != KVM

> Enable the support required to run Qualcomm remoteprocs on non-QHEE
> hypervisors.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---

[...]

>  
> +	if (of_property_present(pdev->dev.of_node, "iommus")) {
> +		struct of_phandle_args args;
> +
> +		ret = of_parse_phandle_with_args(pdev->dev.of_node, "iommus", "#iommu-cells", 0, &args);
> +		if (ret < 0)
> +			return ret;
> +
> +		rproc->has_iommu = true;
> +		adsp->sid = args.args[0];

Do we ignore the SMR mask completely?

This ties the implementation very closely to arm-smmu-v2. While I don't
expect any changes in there, this is something to keep in mind..

Konrad