[PATCH 0/4] Fix XPU violation during modem metadata authentication

[PATCH 0/4] Fix XPU violation during modem metadata authentication

[Sibi Sankar]

The memory region allocated using dma_alloc_attr with no kernel mapping
attribute set would still be a part of the linear kernel map. Any access
to this region by the application processor after assigning it to the
remote Q6 will result in a XPU violation. Fix this by replacing the
dynamically allocated memory region with a no-map carveout and unmap the
modem metadata memory region before passing control to the remote Q6.
The addition of the carveout and memunmap is required only on SoCs that
mandate memory protection before transferring control to Q6, hence the
driver falls back to dynamic memory allocation in the absence of the
modem metadata carveout.

Relevant discussions on the mailing list:
https://lore.kernel.org/lkml/20221114110329.68413-1-manivannan.sadhasivam@linaro.org/

Depends on:
https://patchwork.kernel.org/project/linux-arm-msm/cover/20221124184333.133911-1-krzysztof.kozlowski@linaro.org/

Reported-by: Amit Pundir <amit.pundir@linaro.org>
https://people.linaro.org/~amit.pundir/linaro-sid-developer-dragonboard-845c-569/6.1-rc4_defconfig
Reproduced with ^^ defconfig SDM845 SoCs

Sibi Sankar (4):
  arm64: dts: qcom: Introduce a carveout for modem metadata
  dt-bindings: remoteproc: qcom: sc7180: Update memory-region
    requirements
  dt-bindings: remoteproc: qcom: q6v5: Update memory region requirements
  remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem
    headers

 .../bindings/remoteproc/qcom,q6v5.txt         | 29 ++++++-
 .../remoteproc/qcom,sc7180-mss-pil.yaml       |  3 +-
 .../remoteproc/qcom,sc7280-mss-pil.yaml       |  3 +-
 .../boot/dts/qcom/msm8996-xiaomi-common.dtsi  |  6 ++
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  9 ++
 arch/arm64/boot/dts/qcom/msm8998.dtsi         |  9 ++
 arch/arm64/boot/dts/qcom/sc7180-idp.dts       |  7 +-
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  7 +-
 .../dts/qcom/sc7280-herobrine-lte-sku.dtsi    |  7 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  9 ++
 drivers/remoteproc/qcom_q6v5_mss.c            | 85 +++++++++++++------
 11 files changed, 142 insertions(+), 32 deletions(-)

-- 
2.17.1

[PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Sibi Sankar]

Introduce a new carveout for modem metadata. This will serve as a
replacement for the memory region used by MSA to authenticate modem
ELF headers.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi    | 6 ++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi                  | 9 +++++++++
 arch/arm64/boot/dts/qcom/msm8998.dtsi                  | 9 +++++++++
 arch/arm64/boot/dts/qcom/sc7180-idp.dts                | 7 ++++++-
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi           | 7 ++++++-
 arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 7 ++++++-
 arch/arm64/boot/dts/qcom/sdm845.dtsi                   | 9 +++++++++
 7 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
index 5b47b8de69da..4242f8587c19 100644
--- a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
@@ -127,6 +127,12 @@
 			reg = <0x0 0xf6f00000 0x0 0x100000>;
 			no-map;
 		};
+
+		/delete-node/ memory@91700000;
+		mdata_mem: memory@f7100000 {
+			reg = <0x0 0xf7100000 0x0 0x4000>;
+			no-map;
+		};
 	};
 
 	vph_pwr: vph-pwr-regulator {
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index cc65f52bb80f..3f5fb08e2341 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -462,6 +462,11 @@
 			reg = <0x0 0x91500000 0x0 0x200000>;
 			no-map;
 		};
+
+		mdata_mem: memory@91700000 {
+			reg = <0x0 0x91700000 0x0 0x4000>;
+			no-map;
+		};
 	};
 
 	rpm-glink {
@@ -2458,6 +2463,10 @@
 				memory-region = <&mpss_mem>;
 			};
 
+			metadata {
+				memory-region = <&mdata_mem>;
+			};
+
 			smd-edge {
 				interrupts = <GIC_SPI 449 IRQ_TYPE_EDGE_RISING>;
 
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 539382dab0ad..02e81fd5702d 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -59,6 +59,11 @@
 			qcom,vmid = <15>;
 		};
 
+		mdata_mem: memory@89100000 {
+			reg = <0x0 0x89100000 0x0 0x4000>;
+			no-map;
+		};
+
 		spss_mem: memory@8ab00000 {
 			reg = <0x0 0x8ab00000 0x0 0x700000>;
 			no-map;
@@ -1357,6 +1362,10 @@
 				memory-region = <&mpss_mem>;
 			};
 
+			metadata {
+				memory-region = <&mdata_mem>;
+			};
+
 			glink-edge {
 				interrupts = <GIC_SPI 452 IRQ_TYPE_EDGE_RISING>;
 				label = "modem";
diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index b27b5f0e2b6b..ff0ef8bcba2f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -80,6 +80,11 @@
 			reg = <0x0 0x94400000 0x0 0x200000>;
 			no-map;
 		};
+
+		mdata_mem: memory@94e00000 {
+			reg = <0x0 0x94e00000 0x0 0x4000>;
+			no-map;
+		};
 	};
 };
 
@@ -382,7 +387,7 @@
 	clock-names = "iface", "bus", "nav", "snoc_axi", "mnoc_axi", "xo";
 
 	iommus = <&apps_smmu 0x461 0x0>, <&apps_smmu 0x444 0x3>;
-	memory-region = <&mba_mem &mpss_mem>;
+	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
 
 	resets = <&aoss_reset AOSS_CC_MSS_RESTART>,
 		 <&pdc_reset PDC_MODEM_SYNC_RESET>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index d134d172a3c5..3f2e7175afd8 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -81,6 +81,11 @@
 			reg = <0x0 0x94400000 0x0 0x200000>;
 			no-map;
 		};
+
+		mdata_mem: memory@94e00000 {
+			reg = <0x0 0x94e00000 0x0 0x4000>;
+			no-map;
+		};
 	};
 
 	aliases {
@@ -865,7 +870,7 @@ hp_i2c: &i2c9 {
 	clock-names = "iface", "bus", "nav", "snoc_axi", "mnoc_axi", "xo";
 
 	iommus = <&apps_smmu 0x461 0x0>, <&apps_smmu 0x444 0x3>;
-	memory-region = <&mba_mem &mpss_mem>;
+	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
 
 	/* This gets overridden for SKUs with LTE support. */
 	firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn",
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
index bf522a64b172..bda0495aa0b5 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
@@ -17,6 +17,11 @@
 			reg = <0x0 0x9c700000 0x0 0x200000>;
 			no-map;
 		};
+
+		mdata_mem: memory@9d100000 {
+			reg = <0x0 0x9d100000 0x0 0x4000>;
+			no-map;
+		};
 	};
 };
 
@@ -32,7 +37,7 @@
 
 	iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
 	interconnects = <&mc_virt MASTER_LLCC 0 &mc_virt SLAVE_EBI1 0>;
-	memory-region = <&mba_mem>, <&mpss_mem>;
+	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
 	firmware-name = "qcom/sc7280-herobrine/modem/mba.mbn",
 			"qcom/sc7280-herobrine/modem/qdsp6sw.mbn";
 
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 65032b94b46d..56050f35c232 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -122,6 +122,11 @@
 			qcom,vmid = <15>;
 		};
 
+		mdata_mem: memory@89700000 {
+			reg = <0 0x89700000 0 0x4000>;
+			no-map;
+		};
+
 		qseecom_mem: qseecom@8ab00000 {
 			reg = <0 0x8ab00000 0 0x1400000>;
 			no-map;
@@ -3283,6 +3288,10 @@
 				memory-region = <&mpss_region>;
 			};
 
+			metadata {
+				memory-region = <&mdata_mem>;
+			};
+
 			glink-edge {
 				interrupts = <GIC_SPI 449 IRQ_TYPE_EDGE_RISING>;
 				label = "modem";
-- 
2.17.1

[PATCH 2/4] dt-bindings: remoteproc: qcom: sc7180: Update memory-region requirements

[Sibi Sankar]

Update the bindings to reflect the addition of the new modem metadata
carveout reference to the memory-region property.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
 .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
index e4a7da8020f4..b1402bef0ebe 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
@@ -95,6 +95,7 @@ properties:
     items:
       - description: MBA reserved region
       - description: modem reserved region
+      - description: metadata reserved region
 
   firmware-name:
     $ref: /schemas/types.yaml#/definitions/string-array
@@ -223,7 +224,7 @@ examples:
                         <&rpmhpd SC7180_MSS>;
         power-domain-names = "cx", "mx", "mss";
 
-        memory-region = <&mba_mem>, <&mpss_mem>;
+        memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
 
         qcom,qmp = <&aoss_qmp>;
 
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
index b4de0521a89d..005cb21732af 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
@@ -95,6 +95,7 @@ properties:
     items:
       - description: MBA reserved region
       - description: modem reserved region
+      - description: metadata reserved region
 
   firmware-name:
     $ref: /schemas/types.yaml#/definitions/string-array
@@ -240,7 +241,7 @@ examples:
                         <&rpmhpd SC7280_MSS>;
         power-domain-names = "cx", "mss";
 
-        memory-region = <&mba_mem>, <&mpss_mem>;
+        memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
 
         qcom,qmp = <&aoss_qmp>;
 
-- 
2.17.1

[PATCH 3/4] dt-bindings: remoteproc: qcom: q6v5: Update memory region requirements

[Sibi Sankar]

Update the bindings to reflect the addition of the new modem metadata
carveout on SoCs that use memory protection before transferring control
to the remote Q6.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

https://lore.kernel.org/all/20220511161602.117772-7-sireeshkodali1@gmail.com/
Sireesh had started the conversion to yaml a while back. I'll take over
the conversion from v2.

 .../bindings/remoteproc/qcom,q6v5.txt         | 29 +++++++++++++++++--
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index d0ebd16ee0e1..89772d026363 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -104,8 +104,24 @@ on the Qualcomm Hexagon core.
 		    must be "mss_restart", "pdc_reset" for the modem
 		    sub-system on SDM845 SoCs
 
-For devices where the mba and mpss sub-nodes are not specified, mba/mpss region
-should be referenced as follows:
+For devices with the compatible strings below and where the mba, mpss and
+metadata sub-nodes are not specified, mba/mpss/mdata region should be
+referenced as follows:
+  "qcom,msm8996-mss-pil"
+  "qcom,msm8998-mss-pil"
+  "qcom,sdm845-mss-pil"
+- memory-region:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to mba, mpss and metadata reserved-memory regions.
+
+For devices with the compatible strings below and where the mba and mpss
+sub-nodes are not specified, mba/mpss region should be referenced as follows:
+  "qcom,q6v5-pil",
+  "qcom,ipq8074-wcss-pil"
+  "qcom,qcs404-wcss-pil"
+  "qcom,msm8916-mss-pil",
+  "qcom,msm8974-mss-pil"
 - memory-region:
 	Usage: required
 	Value type: <phandle>
@@ -198,7 +214,14 @@ on platforms which do not have TrustZone.
 
 = SUBNODES:
 The Hexagon node must contain two subnodes, named "mba" and "mpss" representing
-the memory regions used by the Hexagon firmware. Each sub-node must contain:
+the memory regions used by the Hexagon firmware. For devices with the compatible
+string below, an additional third subnode named "metadata" representing the modem
+metadata memory region should also be present.
+  "qcom,msm8996-mss-pil"
+  "qcom,msm8998-mss-pil"
+  "qcom,sdm845-mss-pil"
+
+Each sub-node must contain:
 
 - memory-region:
 	Usage: required
-- 
2.17.1

[PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Sibi Sankar]

The memory region allocated using dma_alloc_attr with no kernel mapping
attribute set would still be a part of the linear kernel map. Any access
to this region by the application processor after assigning it to the
remote Q6 will result in a XPU violation. Fix this by replacing the
dynamically allocated memory region with a no-map carveout and unmap the
modem metadata memory region before passing control to the remote Q6.

Reported-by: Amit Pundir <amit.pundir@linaro.org>
Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

The addition of the carveout and memunmap is required only on SoCs that
mandate memory protection before transferring control to Q6, hence the
driver falls back to dynamic memory allocation in the absence of the
modem metadata carveout.

 drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 24 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index fddb63cffee0..8264275ecbd0 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -211,6 +211,7 @@ struct q6v5 {
 	size_t mba_size;
 	size_t dp_size;
 
+	phys_addr_t mdata_phys;
 	phys_addr_t mpss_phys;
 	phys_addr_t mpss_reloc;
 	size_t mpss_size;
@@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 {
 	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
 	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
+	void *mdata_region;
 	struct page **pages;
 	struct page *page;
 	dma_addr_t phys;
@@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
-	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
-	if (!page) {
-		kfree(metadata);
-		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
-		return -ENOMEM;
-	}
+	if (qproc->mdata_phys) {
+		mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
+		if (!mdata_region) {
+			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
+				&qproc->mdata_phys, size);
+			ret = -EBUSY;
+			goto free_dma_attrs;
+		}
 
-	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
-	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
-	if (!pages) {
-		ret = -ENOMEM;
-		goto free_dma_attrs;
-	}
+		memcpy(mdata_region, metadata, size);
+		memunmap(mdata_region);
+		phys = qproc->mdata_phys;
+	} else {
+		page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
+		if (!page) {
+			kfree(metadata);
+			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
+			return -ENOMEM;
+		}
 
-	for (i = 0; i < count; i++)
-		pages[i] = nth_page(page, i);
+		count = PAGE_ALIGN(size) >> PAGE_SHIFT;
+		pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
+		if (!pages) {
+			ret = -ENOMEM;
+			goto free_dma_attrs;
+		}
 
-	vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
-	kfree(pages);
-	if (!vaddr) {
-		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
-		ret = -EBUSY;
-		goto free_dma_attrs;
-	}
+		for (i = 0; i < count; i++)
+			pages[i] = nth_page(page, i);
 
-	memcpy(vaddr, metadata, size);
+		vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
+		kfree(pages);
+		if (!vaddr) {
+			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
+			ret = -EBUSY;
+			goto free_dma_attrs;
+		}
 
-	vunmap(vaddr);
+		memcpy(vaddr, metadata, size);
+
+		vunmap(vaddr);
+	}
 
 	/* Hypervisor mapping to access metadata by modem */
 	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
@@ -1008,7 +1024,8 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 			 "mdt buffer not reclaimed system may become unstable\n");
 
 free_dma_attrs:
-	dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
+	if (!qproc->mdata_phys)
+		dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
 	kfree(metadata);
 
 	return ret < 0 ? ret : 0;
@@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
 	qproc->mpss_phys = qproc->mpss_reloc = r.start;
 	qproc->mpss_size = resource_size(&r);
 
+	if (!child) {
+		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
+	} else {
+		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
+		node = of_parse_phandle(child, "memory-region", 0);
+		of_node_put(child);
+	}
+
+	if (!node)
+		return 0;
+
+	ret = of_address_to_resource(node, 0, &r);
+	of_node_put(node);
+	if (ret) {
+		dev_err(qproc->dev, "unable to resolve metadata region\n");
+		return ret;
+	}
+
+	qproc->mdata_phys = r.start;
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Robin Murphy]

On 2022-12-13 14:07, Sibi Sankar wrote:
> The memory region allocated using dma_alloc_attr with no kernel mapping
> attribute set would still be a part of the linear kernel map. Any access
> to this region by the application processor after assigning it to the
> remote Q6 will result in a XPU violation. Fix this by replacing the
> dynamically allocated memory region with a no-map carveout and unmap the
> modem metadata memory region before passing control to the remote Q6.
> 
> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> The addition of the carveout and memunmap is required only on SoCs that
> mandate memory protection before transferring control to Q6, hence the
> driver falls back to dynamic memory allocation in the absence of the
> modem metadata carveout.

The DMA_ATTR_NO_KERNEL_MAPPING stuff is still broken and pointless, so 
I'd expect to see this solution replacing it, not being added alongside. 
It's just silly to say pass the "I don't need a CPU mapping" flag, then 
manually open-code the same CPU mapping you would have got if you 
hadn't, in a way that only works at all when a cacheable alias exists 
anyway.

Thanks,
Robin.

>   drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
>   1 file changed, 61 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index fddb63cffee0..8264275ecbd0 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -211,6 +211,7 @@ struct q6v5 {
>   	size_t mba_size;
>   	size_t dp_size;
>   
> +	phys_addr_t mdata_phys;
>   	phys_addr_t mpss_phys;
>   	phys_addr_t mpss_reloc;
>   	size_t mpss_size;
> @@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>   {
>   	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
>   	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> +	void *mdata_region;
>   	struct page **pages;
>   	struct page *page;
>   	dma_addr_t phys;
> @@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>   	if (IS_ERR(metadata))
>   		return PTR_ERR(metadata);
>   
> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!page) {
> -		kfree(metadata);
> -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> -		return -ENOMEM;
> -	}
> +	if (qproc->mdata_phys) {
> +		mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> +		if (!mdata_region) {
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> +				&qproc->mdata_phys, size);
> +			ret = -EBUSY;
> +			goto free_dma_attrs;
> +		}
>   
> -	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> -	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> -	if (!pages) {
> -		ret = -ENOMEM;
> -		goto free_dma_attrs;
> -	}
> +		memcpy(mdata_region, metadata, size);
> +		memunmap(mdata_region);
> +		phys = qproc->mdata_phys;
> +	} else {
> +		page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +		if (!page) {
> +			kfree(metadata);
> +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> +			return -ENOMEM;
> +		}
>   
> -	for (i = 0; i < count; i++)
> -		pages[i] = nth_page(page, i);
> +		count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +		pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> +		if (!pages) {
> +			ret = -ENOMEM;
> +			goto free_dma_attrs;
> +		}
>   
> -	vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> -	kfree(pages);
> -	if (!vaddr) {
> -		dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
> -		ret = -EBUSY;
> -		goto free_dma_attrs;
> -	}
> +		for (i = 0; i < count; i++)
> +			pages[i] = nth_page(page, i);
>   
> -	memcpy(vaddr, metadata, size);
> +		vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> +		kfree(pages);
> +		if (!vaddr) {
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", &phys, size);
> +			ret = -EBUSY;
> +			goto free_dma_attrs;
> +		}
>   
> -	vunmap(vaddr);
> +		memcpy(vaddr, metadata, size);
> +
> +		vunmap(vaddr);
> +	}
>   
>   	/* Hypervisor mapping to access metadata by modem */
>   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> @@ -1008,7 +1024,8 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>   			 "mdt buffer not reclaimed system may become unstable\n");
>   
>   free_dma_attrs:
> -	dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
> +	if (!qproc->mdata_phys)
> +		dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
>   	kfree(metadata);
>   
>   	return ret < 0 ? ret : 0;
> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>   	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>   	qproc->mpss_size = resource_size(&r);
>   
> +	if (!child) {
> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
> +	} else {
> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> +		node = of_parse_phandle(child, "memory-region", 0);
> +		of_node_put(child);
> +	}
> +
> +	if (!node)
> +		return 0;
> +
> +	ret = of_address_to_resource(node, 0, &r);
> +	of_node_put(node);
> +	if (ret) {
> +		dev_err(qproc->dev, "unable to resolve metadata region\n");
> +		return ret;
> +	}
> +
> +	qproc->mdata_phys = r.start;
> +
>   	return 0;
>   }
>   

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Sibi Sankar]

Hey Robin,

Thanks for taking time to review the series.

On 12/13/22 20:37, Robin Murphy wrote:
> On 2022-12-13 14:07, Sibi Sankar wrote:
>> The memory region allocated using dma_alloc_attr with no kernel mapping
>> attribute set would still be a part of the linear kernel map. Any access
>> to this region by the application processor after assigning it to the
>> remote Q6 will result in a XPU violation. Fix this by replacing the
>> dynamically allocated memory region with a no-map carveout and unmap the
>> modem metadata memory region before passing control to the remote Q6.
>>
>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem 
>> ownership switch")
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> The addition of the carveout and memunmap is required only on SoCs that
>> mandate memory protection before transferring control to Q6, hence the
>> driver falls back to dynamic memory allocation in the absence of the
>> modem metadata carveout.
> 
> The DMA_ATTR_NO_KERNEL_MAPPING stuff is still broken and pointless, so 
> I'd expect to see this solution replacing it, not being added alongside. 
> It's just silly to say pass the "I don't need a CPU mapping" flag, then 
> manually open-code the same CPU mapping you would have got if you 
> hadn't, in a way that only works at all when a cacheable alias exists 
> anyway.

only a subset of SoCs supported by the driver are affected by
the bug i.e. on the others dma_alloc_attr would still work
without problems. I can perhaps drop the NO_KERNEL_MAPPING along
with the vmap/vunmap and simplify things for those SoCs.

- Sibi

> 
> Thanks,
> Robin.
> 
>>   drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
>>   1 file changed, 61 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index fddb63cffee0..8264275ecbd0 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -211,6 +211,7 @@ struct q6v5 {
>>       size_t mba_size;
>>       size_t dp_size;
>> +    phys_addr_t mdata_phys;
>>       phys_addr_t mpss_phys;
>>       phys_addr_t mpss_reloc;
>>       size_t mpss_size;
>> @@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5 
>> *qproc, const struct firmware *fw,
>>   {
>>       unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | 
>> DMA_ATTR_NO_KERNEL_MAPPING;
>>       unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
>> +    void *mdata_region;
>>       struct page **pages;
>>       struct page *page;
>>       dma_addr_t phys;
>> @@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5 
>> *qproc, const struct firmware *fw,
>>       if (IS_ERR(metadata))
>>           return PTR_ERR(metadata);
>> -    page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, 
>> dma_attrs);
>> -    if (!page) {
>> -        kfree(metadata);
>> -        dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> -        return -ENOMEM;
>> -    }
>> +    if (qproc->mdata_phys) {
>> +        mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
>> +        if (!mdata_region) {
>> +            dev_err(qproc->dev, "unable to map memory region: 
>> %pa+%zx\n",
>> +                &qproc->mdata_phys, size);
>> +            ret = -EBUSY;
>> +            goto free_dma_attrs;
>> +        }
>> -    count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> -    pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
>> -    if (!pages) {
>> -        ret = -ENOMEM;
>> -        goto free_dma_attrs;
>> -    }
>> +        memcpy(mdata_region, metadata, size);
>> +        memunmap(mdata_region);
>> +        phys = qproc->mdata_phys;
>> +    } else {
>> +        page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, 
>> dma_attrs);
>> +        if (!page) {
>> +            kfree(metadata);
>> +            dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> +            return -ENOMEM;
>> +        }
>> -    for (i = 0; i < count; i++)
>> -        pages[i] = nth_page(page, i);
>> +        count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +        pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
>> +        if (!pages) {
>> +            ret = -ENOMEM;
>> +            goto free_dma_attrs;
>> +        }
>> -    vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
>> -    kfree(pages);
>> -    if (!vaddr) {
>> -        dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", 
>> &phys, size);
>> -        ret = -EBUSY;
>> -        goto free_dma_attrs;
>> -    }
>> +        for (i = 0; i < count; i++)
>> +            pages[i] = nth_page(page, i);
>> -    memcpy(vaddr, metadata, size);
>> +        vaddr = vmap(pages, count, flags, 
>> pgprot_dmacoherent(PAGE_KERNEL));
>> +        kfree(pages);
>> +        if (!vaddr) {
>> +            dev_err(qproc->dev, "unable to map memory region: 
>> %pa+%zx\n", &phys, size);
>> +            ret = -EBUSY;
>> +            goto free_dma_attrs;
>> +        }
>> -    vunmap(vaddr);
>> +        memcpy(vaddr, metadata, size);
>> +
>> +        vunmap(vaddr);
>> +    }
>>       /* Hypervisor mapping to access metadata by modem */
>>       mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>> @@ -1008,7 +1024,8 @@ static int q6v5_mpss_init_image(struct q6v5 
>> *qproc, const struct firmware *fw,
>>                "mdt buffer not reclaimed system may become unstable\n");
>>   free_dma_attrs:
>> -    dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
>> +    if (!qproc->mdata_phys)
>> +        dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
>>       kfree(metadata);
>>       return ret < 0 ? ret : 0;
>> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 
>> *qproc)
>>       qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>       qproc->mpss_size = resource_size(&r);
>> +    if (!child) {
>> +        node = of_parse_phandle(qproc->dev->of_node, "memory-region", 
>> 2);
>> +    } else {
>> +        child = of_get_child_by_name(qproc->dev->of_node, "metadata");
>> +        node = of_parse_phandle(child, "memory-region", 0);
>> +        of_node_put(child);
>> +    }
>> +
>> +    if (!node)
>> +        return 0;
>> +
>> +    ret = of_address_to_resource(node, 0, &r);
>> +    of_node_put(node);
>> +    if (ret) {
>> +        dev_err(qproc->dev, "unable to resolve metadata region\n");
>> +        return ret;
>> +    }
>> +
>> +    qproc->mdata_phys = r.start;
>> +
>>       return 0;
>>   }

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Manivannan Sadhasivam]

On Tue, Dec 13, 2022 at 09:27:04PM +0530, Sibi Sankar wrote:
> Hey Robin,
> 
> Thanks for taking time to review the series.
> 
> On 12/13/22 20:37, Robin Murphy wrote:
> > On 2022-12-13 14:07, Sibi Sankar wrote:
> > > The memory region allocated using dma_alloc_attr with no kernel mapping
> > > attribute set would still be a part of the linear kernel map. Any access
> > > to this region by the application processor after assigning it to the
> > > remote Q6 will result in a XPU violation. Fix this by replacing the
> > > dynamically allocated memory region with a no-map carveout and unmap the
> > > modem metadata memory region before passing control to the remote Q6.
> > > 
> > > Reported-by: Amit Pundir <amit.pundir@linaro.org>
> > > Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for
> > > mem ownership switch")
> > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> > > ---
> > > 
> > > The addition of the carveout and memunmap is required only on SoCs that
> > > mandate memory protection before transferring control to Q6, hence the
> > > driver falls back to dynamic memory allocation in the absence of the
> > > modem metadata carveout.
> > 
> > The DMA_ATTR_NO_KERNEL_MAPPING stuff is still broken and pointless, so
> > I'd expect to see this solution replacing it, not being added alongside.
> > It's just silly to say pass the "I don't need a CPU mapping" flag, then
> > manually open-code the same CPU mapping you would have got if you
> > hadn't, in a way that only works at all when a cacheable alias exists
> > anyway.
> 
> only a subset of SoCs supported by the driver are affected by
> the bug i.e. on the others dma_alloc_attr would still work
> without problems. I can perhaps drop the NO_KERNEL_MAPPING along
> with the vmap/vunmap and simplify things for those SoCs.
> 

Or perhaps revert fc156629b23a?

Thanks,
Mani

> - Sibi
> 
> > 
> > Thanks,
> > Robin.
> > 
> > >   drivers/remoteproc/qcom_q6v5_mss.c | 85 +++++++++++++++++++++---------
> > >   1 file changed, 61 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
> > > b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index fddb63cffee0..8264275ecbd0 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -211,6 +211,7 @@ struct q6v5 {
> > >       size_t mba_size;
> > >       size_t dp_size;
> > > +    phys_addr_t mdata_phys;
> > >       phys_addr_t mpss_phys;
> > >       phys_addr_t mpss_reloc;
> > >       size_t mpss_size;
> > > @@ -935,6 +936,7 @@ static int q6v5_mpss_init_image(struct q6v5
> > > *qproc, const struct firmware *fw,
> > >   {
> > >       unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS |
> > > DMA_ATTR_NO_KERNEL_MAPPING;
> > >       unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> > > +    void *mdata_region;
> > >       struct page **pages;
> > >       struct page *page;
> > >       dma_addr_t phys;
> > > @@ -951,34 +953,48 @@ static int q6v5_mpss_init_image(struct q6v5
> > > *qproc, const struct firmware *fw,
> > >       if (IS_ERR(metadata))
> > >           return PTR_ERR(metadata);
> > > -    page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL,
> > > dma_attrs);
> > > -    if (!page) {
> > > -        kfree(metadata);
> > > -        dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > -        return -ENOMEM;
> > > -    }
> > > +    if (qproc->mdata_phys) {
> > > +        mdata_region = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> > > +        if (!mdata_region) {
> > > +            dev_err(qproc->dev, "unable to map memory region:
> > > %pa+%zx\n",
> > > +                &qproc->mdata_phys, size);
> > > +            ret = -EBUSY;
> > > +            goto free_dma_attrs;
> > > +        }
> > > -    count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > > -    pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> > > -    if (!pages) {
> > > -        ret = -ENOMEM;
> > > -        goto free_dma_attrs;
> > > -    }
> > > +        memcpy(mdata_region, metadata, size);
> > > +        memunmap(mdata_region);
> > > +        phys = qproc->mdata_phys;
> > > +    } else {
> > > +        page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL,
> > > dma_attrs);
> > > +        if (!page) {
> > > +            kfree(metadata);
> > > +            dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > +            return -ENOMEM;
> > > +        }
> > > -    for (i = 0; i < count; i++)
> > > -        pages[i] = nth_page(page, i);
> > > +        count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > > +        pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> > > +        if (!pages) {
> > > +            ret = -ENOMEM;
> > > +            goto free_dma_attrs;
> > > +        }
> > > -    vaddr = vmap(pages, count, flags, pgprot_dmacoherent(PAGE_KERNEL));
> > > -    kfree(pages);
> > > -    if (!vaddr) {
> > > -        dev_err(qproc->dev, "unable to map memory region:
> > > %pa+%zx\n", &phys, size);
> > > -        ret = -EBUSY;
> > > -        goto free_dma_attrs;
> > > -    }
> > > +        for (i = 0; i < count; i++)
> > > +            pages[i] = nth_page(page, i);
> > > -    memcpy(vaddr, metadata, size);
> > > +        vaddr = vmap(pages, count, flags,
> > > pgprot_dmacoherent(PAGE_KERNEL));
> > > +        kfree(pages);
> > > +        if (!vaddr) {
> > > +            dev_err(qproc->dev, "unable to map memory region:
> > > %pa+%zx\n", &phys, size);
> > > +            ret = -EBUSY;
> > > +            goto free_dma_attrs;
> > > +        }
> > > -    vunmap(vaddr);
> > > +        memcpy(vaddr, metadata, size);
> > > +
> > > +        vunmap(vaddr);
> > > +    }
> > >       /* Hypervisor mapping to access metadata by modem */
> > >       mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> > > @@ -1008,7 +1024,8 @@ static int q6v5_mpss_init_image(struct q6v5
> > > *qproc, const struct firmware *fw,
> > >                "mdt buffer not reclaimed system may become unstable\n");
> > >   free_dma_attrs:
> > > -    dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
> > > +    if (!qproc->mdata_phys)
> > > +        dma_free_attrs(qproc->dev, size, page, phys, dma_attrs);
> > >       kfree(metadata);
> > >       return ret < 0 ? ret : 0;
> > > @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct
> > > q6v5 *qproc)
> > >       qproc->mpss_phys = qproc->mpss_reloc = r.start;
> > >       qproc->mpss_size = resource_size(&r);
> > > +    if (!child) {
> > > +        node = of_parse_phandle(qproc->dev->of_node,
> > > "memory-region", 2);
> > > +    } else {
> > > +        child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> > > +        node = of_parse_phandle(child, "memory-region", 0);
> > > +        of_node_put(child);
> > > +    }
> > > +
> > > +    if (!node)
> > > +        return 0;
> > > +
> > > +    ret = of_address_to_resource(node, 0, &r);
> > > +    of_node_put(node);
> > > +    if (ret) {
> > > +        dev_err(qproc->dev, "unable to resolve metadata region\n");
> > > +        return ret;
> > > +    }
> > > +
> > > +    qproc->mdata_phys = r.start;
> > > +
> > >       return 0;
> > >   }

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Krzysztof Kozlowski]

On 13/12/2022 15:07, Sibi Sankar wrote:
> Introduce a new carveout for modem metadata. This will serve as a
> replacement for the memory region used by MSA to authenticate modem
> ELF headers.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>

Thank you for your patch. There is something to discuss/improve.

>  
>  	aliases {
> @@ -865,7 +870,7 @@ hp_i2c: &i2c9 {
>  	clock-names = "iface", "bus", "nav", "snoc_axi", "mnoc_axi", "xo";
>  
>  	iommus = <&apps_smmu 0x461 0x0>, <&apps_smmu 0x444 0x3>;
> -	memory-region = <&mba_mem &mpss_mem>;
> +	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
>  
>  	/* This gets overridden for SKUs with LTE support. */
>  	firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn",
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
> index bf522a64b172..bda0495aa0b5 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
> @@ -17,6 +17,11 @@
>  			reg = <0x0 0x9c700000 0x0 0x200000>;
>  			no-map;
>  		};
> +
> +		mdata_mem: memory@9d100000 {
> +			reg = <0x0 0x9d100000 0x0 0x4000>;
> +			no-map;
> +		};
>  	};
>  };
>  
> @@ -32,7 +37,7 @@
>  
>  	iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
>  	interconnects = <&mc_virt MASTER_LLCC 0 &mc_virt SLAVE_EBI1 0>;
> -	memory-region = <&mba_mem>, <&mpss_mem>;
> +	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;

Only two memory regions are allowed by bindings... unless you fix it in
further patchset. If so, please re-order to have the bindings first. It
helps reviewers not to have such questions. :)


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: remoteproc: qcom: sc7180: Update memory-region requirements

[Krzysztof Kozlowski]

On 13/12/2022 15:07, Sibi Sankar wrote:
> Update the bindings to reflect the addition of the new modem metadata
> carveout reference to the memory-region property.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
>  .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml    | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
> index e4a7da8020f4..b1402bef0ebe 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
> @@ -95,6 +95,7 @@ properties:
>      items:
>        - description: MBA reserved region
>        - description: modem reserved region
> +      - description: metadata reserved region

Which makes the third item now required, also for all out of tree DTS
and other users of the bindings. Please write a bit more in commit msg
why this is necessary (e.g. was it broken before?). I assume the driver
does not break the ABI?

Best regards,
Krzysztof

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Krzysztof Kozlowski]

On 13/12/2022 15:07, Sibi Sankar wrote:
> The memory region allocated using dma_alloc_attr with no kernel mapping
> attribute set would still be a part of the linear kernel map. Any access
> to this region by the application processor after assigning it to the
> remote Q6 will result in a XPU violation. Fix this by replacing the
> dynamically allocated memory region with a no-map carveout and unmap the
> modem metadata memory region before passing control to the remote Q6.
> 
> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---

Thank you for your patch. There is something to discuss/improve.
>  
>  	return ret < 0 ? ret : 0;
> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>  	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>  	qproc->mpss_size = resource_size(&r);
>  
> +	if (!child) {
> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
> +	} else {
> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");

Bindings do not allow to have child "metadata", do they?

> +		node = of_parse_phandle(child, "memory-region", 0);
> +		of_node_put(child);
> +	}
> +
> +	if (!node)
> +		return 0;
> +
> +	ret = of_address_to_resource(node, 0, &r);
> +	of_node_put(node);
> +	if (ret) {
> +		dev_err(qproc->dev, "unable to resolve metadata region\n");
> +		return ret;
> +	}
> +
> +	qproc->mdata_phys = r.start;
> +
>  	return 0;
>  }
>  

Best regards,
Krzysztof

Re: [PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Sibi Sankar]

Hey Krzysztof,

Thanks for taking time to review the series.

On 12/14/22 01:10, Krzysztof Kozlowski wrote:
> On 13/12/2022 15:07, Sibi Sankar wrote:
>> Introduce a new carveout for modem metadata. This will serve as a
>> replacement for the memory region used by MSA to authenticate modem
>> ELF headers.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>>   
>>   	aliases {
>> @@ -865,7 +870,7 @@ hp_i2c: &i2c9 {
>>   	clock-names = "iface", "bus", "nav", "snoc_axi", "mnoc_axi", "xo";
>>   
>>   	iommus = <&apps_smmu 0x461 0x0>, <&apps_smmu 0x444 0x3>;
>> -	memory-region = <&mba_mem &mpss_mem>;
>> +	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
>>   
>>   	/* This gets overridden for SKUs with LTE support. */
>>   	firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn",
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
>> index bf522a64b172..bda0495aa0b5 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
>> @@ -17,6 +17,11 @@
>>   			reg = <0x0 0x9c700000 0x0 0x200000>;
>>   			no-map;
>>   		};
>> +
>> +		mdata_mem: memory@9d100000 {
>> +			reg = <0x0 0x9d100000 0x0 0x4000>;
>> +			no-map;
>> +		};
>>   	};
>>   };
>>   
>> @@ -32,7 +37,7 @@
>>   
>>   	iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
>>   	interconnects = <&mc_virt MASTER_LLCC 0 &mc_virt SLAVE_EBI1 0>;
>> -	memory-region = <&mba_mem>, <&mpss_mem>;
>> +	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
> 
> Only two memory regions are allowed by bindings... unless you fix it in
> further patchset. If so, please re-order to have the bindings first. It
> helps reviewers not to have such questions. :)

I felt that Rob's dt_bindings check bot might report an error
if the dt changes weren't placed before the bindings changes.
But since you asked for the logical order I guess the bindings
check are done only after the entire series is applied. I'll
change the order in the next re-spin.

- Sibi
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Krzysztof Kozlowski]

On 14/12/2022 07:49, Sibi Sankar wrote:
> Hey Krzysztof,
> 
> Thanks for taking time to review the series.
> 
> On 12/14/22 01:10, Krzysztof Kozlowski wrote:
>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>> Introduce a new carveout for modem metadata. This will serve as a
>>> replacement for the memory region used by MSA to authenticate modem
>>> ELF headers.
>>>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>>   
>>>   	aliases {
>>> @@ -865,7 +870,7 @@ hp_i2c: &i2c9 {
>>>   	clock-names = "iface", "bus", "nav", "snoc_axi", "mnoc_axi", "xo";
>>>   
>>>   	iommus = <&apps_smmu 0x461 0x0>, <&apps_smmu 0x444 0x3>;
>>> -	memory-region = <&mba_mem &mpss_mem>;
>>> +	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
>>>   
>>>   	/* This gets overridden for SKUs with LTE support. */
>>>   	firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn",
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
>>> index bf522a64b172..bda0495aa0b5 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi
>>> @@ -17,6 +17,11 @@
>>>   			reg = <0x0 0x9c700000 0x0 0x200000>;
>>>   			no-map;
>>>   		};
>>> +
>>> +		mdata_mem: memory@9d100000 {
>>> +			reg = <0x0 0x9d100000 0x0 0x4000>;
>>> +			no-map;
>>> +		};
>>>   	};
>>>   };
>>>   
>>> @@ -32,7 +37,7 @@
>>>   
>>>   	iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
>>>   	interconnects = <&mc_virt MASTER_LLCC 0 &mc_virt SLAVE_EBI1 0>;
>>> -	memory-region = <&mba_mem>, <&mpss_mem>;
>>> +	memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>;
>>
>> Only two memory regions are allowed by bindings... unless you fix it in
>> further patchset. If so, please re-order to have the bindings first. It
>> helps reviewers not to have such questions. :)
> 
> I felt that Rob's dt_bindings check bot might report an error
> if the dt changes weren't placed before the bindings changes.
> But since you asked for the logical order I guess the bindings
> check are done only after the entire series is applied. I'll
> change the order in the next re-spin.

AFAIR, Rob's bot ignores DTS patches anyway.

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: remoteproc: qcom: sc7180: Update memory-region requirements

[Sibi Sankar]

On 12/14/22 01:11, Krzysztof Kozlowski wrote:
> On 13/12/2022 15:07, Sibi Sankar wrote:
>> Update the bindings to reflect the addition of the new modem metadata
>> carveout reference to the memory-region property.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
>>   .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml    | 3 ++-
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>> index e4a7da8020f4..b1402bef0ebe 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>> @@ -95,6 +95,7 @@ properties:
>>       items:
>>         - description: MBA reserved region
>>         - description: modem reserved region
>> +      - description: metadata reserved region
> 
> Which makes the third item now required, also for all out of tree DTS
> and other users of the bindings. Please write a bit more in commit msg
> why this is necessary (e.g. was it broken before?). I assume the driver
> does not break the ABI?

I'll pad the commit msg with some of the additional info from patch 4.
commit c44094eee32f "arm64: dma: Drop cache invalidation from
arch_dma_prep_coherent()" exposed a bug in the driver affecting SoCs
from msm8996 on wards. The application processor accessing the
dynamically allocated region after giving control to the modem results
in a XPU violation. The recommended fix was to use a no-map carveout
instead and memunmap before giving control to the modem. The future
kernels that are paired with an older dtbs would crash during modem
bootup since we would continue to use dma_alloc_attr. But all the other
combinations (old kernel/new dtb) will continue to work.

- Sibi

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Sibi Sankar]

On 12/14/22 01:17, Krzysztof Kozlowski wrote:
> On 13/12/2022 15:07, Sibi Sankar wrote:
>> The memory region allocated using dma_alloc_attr with no kernel mapping
>> attribute set would still be a part of the linear kernel map. Any access
>> to this region by the application processor after assigning it to the
>> remote Q6 will result in a XPU violation. Fix this by replacing the
>> dynamically allocated memory region with a no-map carveout and unmap the
>> modem metadata memory region before passing control to the remote Q6.
>>
>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
> 
> Thank you for your patch. There is something to discuss/improve.
>>   
>>   	return ret < 0 ? ret : 0;
>> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>>   	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>   	qproc->mpss_size = resource_size(&r);
>>   
>> +	if (!child) {
>> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
>> +	} else {
>> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> 
> Bindings do not allow to have child "metadata", do they?

memory-region property was used to specify mba/mpss region in a phandle
array only from SC7180 SoC. All the older dtbs in the wild/upstream
still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
as to not break bindings when newer kernel uses a older dtb.

- Sibi

> 
>> +		node = of_parse_phandle(child, "memory-region", 0);
>> +		of_node_put(child);
>> +	}
>> +
>> +	if (!node)
>> +		return 0;
>> +
>> +	ret = of_address_to_resource(node, 0, &r);
>> +	of_node_put(node);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "unable to resolve metadata region\n");
>> +		return ret;
>> +	}
>> +
>> +	qproc->mdata_phys = r.start;
>> +
>>   	return 0;
>>   }
>>   
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Krzysztof Kozlowski]

On 13/12/2022 15:07, Sibi Sankar wrote:
> Introduce a new carveout for modem metadata. This will serve as a
> replacement for the memory region used by MSA to authenticate modem
> ELF headers.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi    | 6 ++++++
>  arch/arm64/boot/dts/qcom/msm8996.dtsi                  | 9 +++++++++
>  arch/arm64/boot/dts/qcom/msm8998.dtsi                  | 9 +++++++++
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts                | 7 ++++++-
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi           | 7 ++++++-
>  arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 7 ++++++-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi                   | 9 +++++++++
>  7 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
> index 5b47b8de69da..4242f8587c19 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
> @@ -127,6 +127,12 @@
>  			reg = <0x0 0xf6f00000 0x0 0x100000>;
>  			no-map;
>  		};
> +
> +		/delete-node/ memory@91700000;
> +		mdata_mem: memory@f7100000 {
> +			reg = <0x0 0xf7100000 0x0 0x4000>;
> +			no-map;
> +		};
>  	};
>  
>  	vph_pwr: vph-pwr-regulator {
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index cc65f52bb80f..3f5fb08e2341 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -462,6 +462,11 @@
>  			reg = <0x0 0x91500000 0x0 0x200000>;
>  			no-map;
>  		};
> +
> +		mdata_mem: memory@91700000 {
> +			reg = <0x0 0x91700000 0x0 0x4000>;
> +			no-map;
> +		};
>  	};
>  
>  	rpm-glink {
> @@ -2458,6 +2463,10 @@
>  				memory-region = <&mpss_mem>;
>  			};
>  
> +			metadata {

Does it pass dtbs_check?

Best regards,
Krzysztof

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Krzysztof Kozlowski]

On 14/12/2022 11:33, Sibi Sankar wrote:
> 
> 
> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>> The memory region allocated using dma_alloc_attr with no kernel mapping
>>> attribute set would still be a part of the linear kernel map. Any access
>>> to this region by the application processor after assigning it to the
>>> remote Q6 will result in a XPU violation. Fix this by replacing the
>>> dynamically allocated memory region with a no-map carveout and unmap the
>>> modem metadata memory region before passing control to the remote Q6.
>>>
>>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>
>> Thank you for your patch. There is something to discuss/improve.
>>>   
>>>   	return ret < 0 ? ret : 0;
>>> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>>>   	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>>   	qproc->mpss_size = resource_size(&r);
>>>   
>>> +	if (!child) {
>>> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
>>> +	} else {
>>> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
>>
>> Bindings do not allow to have child "metadata", do they?
> 
> memory-region property was used to specify mba/mpss region in a phandle
> array only from SC7180 SoC. All the older dtbs in the wild/upstream
> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
> as to not break bindings when newer kernel uses a older dtb.

This does not explain why you extend the driver without extending the
bindings. You do not do it for legacy stuff but for SC7180. But even for
legacy devices you cannot add new properties without having it in some
legacy bindings.


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: remoteproc: qcom: sc7180: Update memory-region requirements

[Krzysztof Kozlowski]

On 14/12/2022 11:25, Sibi Sankar wrote:
> 
> 
> On 12/14/22 01:11, Krzysztof Kozlowski wrote:
>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>> Update the bindings to reflect the addition of the new modem metadata
>>> carveout reference to the memory-region property.
>>>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
>>>   .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml    | 3 ++-
>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>>> index e4a7da8020f4..b1402bef0ebe 100644
>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>>> @@ -95,6 +95,7 @@ properties:
>>>       items:
>>>         - description: MBA reserved region
>>>         - description: modem reserved region
>>> +      - description: metadata reserved region
>>
>> Which makes the third item now required, also for all out of tree DTS
>> and other users of the bindings. Please write a bit more in commit msg
>> why this is necessary (e.g. was it broken before?). I assume the driver
>> does not break the ABI?
> 
> I'll pad the commit msg with some of the additional info from patch 4.
> commit c44094eee32f "arm64: dma: Drop cache invalidation from
> arch_dma_prep_coherent()" exposed a bug in the driver affecting SoCs
> from msm8996 on wards. The application processor accessing the
> dynamically allocated region after giving control to the modem results
> in a XPU violation. The recommended fix was to use a no-map carveout
> instead and memunmap before giving control to the modem. The future
> kernels that are paired with an older dtbs would crash during modem

Then it's an ABI break.

> bootup since we would continue to use dma_alloc_attr. But all the other
> combinations (old kernel/new dtb) will continue to work.

Does it mean that old kernel with old DTB was working? If yes, then it's
ABI break without clear benefits.

Best regards,
Krzysztof

Re: [PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Sibi Sankar]

On 12/14/22 16:57, Krzysztof Kozlowski wrote:
> On 13/12/2022 15:07, Sibi Sankar wrote:
>> Introduce a new carveout for modem metadata. This will serve as a
>> replacement for the memory region used by MSA to authenticate modem
>> ELF headers.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi    | 6 ++++++
>>   arch/arm64/boot/dts/qcom/msm8996.dtsi                  | 9 +++++++++
>>   arch/arm64/boot/dts/qcom/msm8998.dtsi                  | 9 +++++++++
>>   arch/arm64/boot/dts/qcom/sc7180-idp.dts                | 7 ++++++-
>>   arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi           | 7 ++++++-
>>   arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 7 ++++++-
>>   arch/arm64/boot/dts/qcom/sdm845.dtsi                   | 9 +++++++++
>>   7 files changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
>> index 5b47b8de69da..4242f8587c19 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
>> @@ -127,6 +127,12 @@
>>   			reg = <0x0 0xf6f00000 0x0 0x100000>;
>>   			no-map;
>>   		};
>> +
>> +		/delete-node/ memory@91700000;
>> +		mdata_mem: memory@f7100000 {
>> +			reg = <0x0 0xf7100000 0x0 0x4000>;
>> +			no-map;
>> +		};
>>   	};
>>   
>>   	vph_pwr: vph-pwr-regulator {
>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> index cc65f52bb80f..3f5fb08e2341 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> @@ -462,6 +462,11 @@
>>   			reg = <0x0 0x91500000 0x0 0x200000>;
>>   			no-map;
>>   		};
>> +
>> +		mdata_mem: memory@91700000 {
>> +			reg = <0x0 0x91700000 0x0 0x4000>;
>> +			no-map;
>> +		};
>>   	};
>>   
>>   	rpm-glink {
>> @@ -2458,6 +2463,10 @@
>>   				memory-region = <&mpss_mem>;
>>   			};
>>   
>> +			metadata {
> 
> Does it pass dtbs_check?

^^ portion of the bindings aren't in yaml yet please see patch 3.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Sibi Sankar]

On 12/14/22 16:58, Krzysztof Kozlowski wrote:
> On 14/12/2022 11:33, Sibi Sankar wrote:
>>
>>
>> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>>> The memory region allocated using dma_alloc_attr with no kernel mapping
>>>> attribute set would still be a part of the linear kernel map. Any access
>>>> to this region by the application processor after assigning it to the
>>>> remote Q6 will result in a XPU violation. Fix this by replacing the
>>>> dynamically allocated memory region with a no-map carveout and unmap the
>>>> modem metadata memory region before passing control to the remote Q6.
>>>>
>>>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>>>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>> ---
>>>
>>> Thank you for your patch. There is something to discuss/improve.
>>>>    
>>>>    	return ret < 0 ? ret : 0;
>>>> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>>>>    	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>>>    	qproc->mpss_size = resource_size(&r);
>>>>    
>>>> +	if (!child) {
>>>> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
>>>> +	} else {
>>>> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
>>>
>>> Bindings do not allow to have child "metadata", do they?
>>
>> memory-region property was used to specify mba/mpss region in a phandle
>> array only from SC7180 SoC. All the older dtbs in the wild/upstream
>> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
>> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
>> as to not break bindings when newer kernel uses a older dtb.
> 
> This does not explain why you extend the driver without extending the
> bindings. You do not do it for legacy stuff but for SC7180. But even for
> legacy devices you cannot add new properties without having it in some
> legacy bindings.

https://patchwork.kernel.org/project/linux-arm-msm/patch/20221213140724.8612-4-quic_sibis@quicinc.com/

The legacy bindings are a part of patch 3 ^^.

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/4] dt-bindings: remoteproc: qcom: sc7180: Update memory-region requirements

[Sibi Sankar]

On 12/14/22 17:00, Krzysztof Kozlowski wrote:
> On 14/12/2022 11:25, Sibi Sankar wrote:
>>
>>
>> On 12/14/22 01:11, Krzysztof Kozlowski wrote:
>>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>>> Update the bindings to reflect the addition of the new modem metadata
>>>> carveout reference to the memory-region property.
>>>>
>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>> ---
>>>>    .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
>>>>    .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml    | 3 ++-
>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>>>> index e4a7da8020f4..b1402bef0ebe 100644
>>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml
>>>> @@ -95,6 +95,7 @@ properties:
>>>>        items:
>>>>          - description: MBA reserved region
>>>>          - description: modem reserved region
>>>> +      - description: metadata reserved region
>>>
>>> Which makes the third item now required, also for all out of tree DTS
>>> and other users of the bindings. Please write a bit more in commit msg
>>> why this is necessary (e.g. was it broken before?). I assume the driver
>>> does not break the ABI?
>>
>> I'll pad the commit msg with some of the additional info from patch 4.
>> commit c44094eee32f "arm64: dma: Drop cache invalidation from
>> arch_dma_prep_coherent()" exposed a bug in the driver affecting SoCs
>> from msm8996 on wards. The application processor accessing the
>> dynamically allocated region after giving control to the modem results
>> in a XPU violation. The recommended fix was to use a no-map carveout
>> instead and memunmap before giving control to the modem. The future
>> kernels that are paired with an older dtbs would crash during modem
> 
> Then it's an ABI break.
> 
>> bootup since we would continue to use dma_alloc_attr. But all the other
>> combinations (old kernel/new dtb) will continue to work.
> 
> Does it mean that old kernel with old DTB was working? If yes, then it's
> ABI break without clear benefits.

commit c44094eee32f is going to land regardless soon and will break
modem on mainline and any other branches that picks up the patch.
The suggested way to fix it (no-map carveout) requires this bindings
change.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Robin Murphy]

On 2022-12-13 16:07, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 09:27:04PM +0530, Sibi Sankar wrote:
>> Hey Robin,
>>
>> Thanks for taking time to review the series.
>>
>> On 12/13/22 20:37, Robin Murphy wrote:
>>> On 2022-12-13 14:07, Sibi Sankar wrote:
>>>> The memory region allocated using dma_alloc_attr with no kernel mapping
>>>> attribute set would still be a part of the linear kernel map. Any access
>>>> to this region by the application processor after assigning it to the
>>>> remote Q6 will result in a XPU violation. Fix this by replacing the
>>>> dynamically allocated memory region with a no-map carveout and unmap the
>>>> modem metadata memory region before passing control to the remote Q6.
>>>>
>>>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>>>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for
>>>> mem ownership switch")
>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>> ---
>>>>
>>>> The addition of the carveout and memunmap is required only on SoCs that
>>>> mandate memory protection before transferring control to Q6, hence the
>>>> driver falls back to dynamic memory allocation in the absence of the
>>>> modem metadata carveout.
>>>
>>> The DMA_ATTR_NO_KERNEL_MAPPING stuff is still broken and pointless, so
>>> I'd expect to see this solution replacing it, not being added alongside.
>>> It's just silly to say pass the "I don't need a CPU mapping" flag, then
>>> manually open-code the same CPU mapping you would have got if you
>>> hadn't, in a way that only works at all when a cacheable alias exists
>>> anyway.
>>
>> only a subset of SoCs supported by the driver are affected by
>> the bug i.e. on the others dma_alloc_attr would still work
>> without problems. I can perhaps drop the NO_KERNEL_MAPPING along
>> with the vmap/vunmap and simplify things for those SoCs.
>>
> 
> Or perhaps revert fc156629b23a?

Oh, indeed, if it's already self-contained that's even neater. Basically 
that whole commit is based on a misunderstanding, doesn't actually do 
what it thinks it does, and you'd be far better off not maintaining the 
extra code.

Thanks,
Robin.

Re: [PATCH 1/4] arm64: dts: qcom: Introduce a carveout for modem metadata

[Krzysztof Kozlowski]

On 14/12/2022 12:44, Sibi Sankar wrote:
> 
> 
> On 12/14/22 16:57, Krzysztof Kozlowski wrote:
>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>> Introduce a new carveout for modem metadata. This will serve as a
>>> replacement for the memory region used by MSA to authenticate modem
>>> ELF headers.
>>>
>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi    | 6 ++++++
>>>   arch/arm64/boot/dts/qcom/msm8996.dtsi                  | 9 +++++++++
>>>   arch/arm64/boot/dts/qcom/msm8998.dtsi                  | 9 +++++++++
>>>   arch/arm64/boot/dts/qcom/sc7180-idp.dts                | 7 ++++++-
>>>   arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi           | 7 ++++++-
>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 7 ++++++-
>>>   arch/arm64/boot/dts/qcom/sdm845.dtsi                   | 9 +++++++++
>>>   7 files changed, 51 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
>>> index 5b47b8de69da..4242f8587c19 100644
>>> --- a/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi
>>> @@ -127,6 +127,12 @@
>>>   			reg = <0x0 0xf6f00000 0x0 0x100000>;
>>>   			no-map;
>>>   		};
>>> +
>>> +		/delete-node/ memory@91700000;
>>> +		mdata_mem: memory@f7100000 {
>>> +			reg = <0x0 0xf7100000 0x0 0x4000>;
>>> +			no-map;
>>> +		};
>>>   	};
>>>   
>>>   	vph_pwr: vph-pwr-regulator {
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>>> index cc65f52bb80f..3f5fb08e2341 100644
>>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>>> @@ -462,6 +462,11 @@
>>>   			reg = <0x0 0x91500000 0x0 0x200000>;
>>>   			no-map;
>>>   		};
>>> +
>>> +		mdata_mem: memory@91700000 {
>>> +			reg = <0x0 0x91700000 0x0 0x4000>;
>>> +			no-map;
>>> +		};
>>>   	};
>>>   
>>>   	rpm-glink {
>>> @@ -2458,6 +2463,10 @@
>>>   				memory-region = <&mpss_mem>;
>>>   			};
>>>   
>>> +			metadata {
>>
>> Does it pass dtbs_check?
> 
> ^^ portion of the bindings aren't in yaml yet please see patch 3.

Ah, you touch here multiple files. Please split per SoC.

Best regards,
Krzysztof

Re: [PATCH 4/4] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers

[Krzysztof Kozlowski]

On 14/12/2022 12:51, Sibi Sankar wrote:
> 
> 
> On 12/14/22 16:58, Krzysztof Kozlowski wrote:
>> On 14/12/2022 11:33, Sibi Sankar wrote:
>>>
>>>
>>> On 12/14/22 01:17, Krzysztof Kozlowski wrote:
>>>> On 13/12/2022 15:07, Sibi Sankar wrote:
>>>>> The memory region allocated using dma_alloc_attr with no kernel mapping
>>>>> attribute set would still be a part of the linear kernel map. Any access
>>>>> to this region by the application processor after assigning it to the
>>>>> remote Q6 will result in a XPU violation. Fix this by replacing the
>>>>> dynamically allocated memory region with a no-map carveout and unmap the
>>>>> modem metadata memory region before passing control to the remote Q6.
>>>>>
>>>>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>>>>> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")
>>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>>> ---
>>>>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>>    
>>>>>    	return ret < 0 ? ret : 0;
>>>>> @@ -1882,6 +1899,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>>>>>    	qproc->mpss_phys = qproc->mpss_reloc = r.start;
>>>>>    	qproc->mpss_size = resource_size(&r);
>>>>>    
>>>>> +	if (!child) {
>>>>> +		node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
>>>>> +	} else {
>>>>> +		child = of_get_child_by_name(qproc->dev->of_node, "metadata");
>>>>
>>>> Bindings do not allow to have child "metadata", do they?
>>>
>>> memory-region property was used to specify mba/mpss region in a phandle
>>> array only from SC7180 SoC. All the older dtbs in the wild/upstream
>>> still had sub-nodes to achieve the same. Patch 3 allows for a sub-set
>>> of the SoCs (MSM8996/MSM8998/SDM845) to use metadata as a sub-node so
>>> as to not break bindings when newer kernel uses a older dtb.
>>
>> This does not explain why you extend the driver without extending the
>> bindings. You do not do it for legacy stuff but for SC7180. But even for
>> legacy devices you cannot add new properties without having it in some
>> legacy bindings.
> 
> https://patchwork.kernel.org/project/linux-arm-msm/patch/20221213140724.8612-4-quic_sibis@quicinc.com/
> 
> The legacy bindings are a part of patch 3 ^^.

Ah, ok.

Best regards,
Krzysztof

Re: [PATCH 0/4] Fix XPU violation during modem metadata authentication

[Amit Pundir]

On Tue, 13 Dec 2022 at 19:37, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
> The memory region allocated using dma_alloc_attr with no kernel mapping
> attribute set would still be a part of the linear kernel map. Any access
> to this region by the application processor after assigning it to the
> remote Q6 will result in a XPU violation. Fix this by replacing the
> dynamically allocated memory region with a no-map carveout and unmap the
> modem metadata memory region before passing control to the remote Q6.
> The addition of the carveout and memunmap is required only on SoCs that
> mandate memory protection before transferring control to Q6, hence the
> driver falls back to dynamic memory allocation in the absence of the
> modem metadata carveout.
>
> Relevant discussions on the mailing list:
> https://lore.kernel.org/lkml/20221114110329.68413-1-manivannan.sadhasivam@linaro.org/
>
> Depends on:
> https://patchwork.kernel.org/project/linux-arm-msm/cover/20221124184333.133911-1-krzysztof.kozlowski@linaro.org/
>
> Reported-by: Amit Pundir <amit.pundir@linaro.org>

Smoke tested this series on db845c (SDM845) running v6.2-rc1, with the
upstream workaround (b7d9aae40484 Revert "arm64: dma: Drop cache
invalidation from arch_dma_prep_coherent()") reverted and I can no
longer reproduce the above crash in my limited (10+) test runs so far.
So for the entire series:

Tested-by: Amit Pundir <amit.pundir@linaro.org>

Regards,
Amit Pundir

> https://people.linaro.org/~amit.pundir/linaro-sid-developer-dragonboard-845c-569/6.1-rc4_defconfig
> Reproduced with ^^ defconfig SDM845 SoCs
>
> Sibi Sankar (4):
>   arm64: dts: qcom: Introduce a carveout for modem metadata
>   dt-bindings: remoteproc: qcom: sc7180: Update memory-region
>     requirements
>   dt-bindings: remoteproc: qcom: q6v5: Update memory region requirements
>   remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem
>     headers
>
>  .../bindings/remoteproc/qcom,q6v5.txt         | 29 ++++++-
>  .../remoteproc/qcom,sc7180-mss-pil.yaml       |  3 +-
>  .../remoteproc/qcom,sc7280-mss-pil.yaml       |  3 +-
>  .../boot/dts/qcom/msm8996-xiaomi-common.dtsi  |  6 ++
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |  9 ++
>  arch/arm64/boot/dts/qcom/msm8998.dtsi         |  9 ++
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts       |  7 +-
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  7 +-
>  .../dts/qcom/sc7280-herobrine-lte-sku.dtsi    |  7 +-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          |  9 ++
>  drivers/remoteproc/qcom_q6v5_mss.c            | 85 +++++++++++++------
>  11 files changed, 142 insertions(+), 32 deletions(-)
>
> --
> 2.17.1
>