All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 00/11] Fix XPU violation during modem metadata authentication
@ 2023-01-09  3:48 Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema Sibi Sankar
                   ` (10 more replies)
  0 siblings, 11 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	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.

V2:
 * Convert legacy bindings to yaml
 * Revert no_kernel_mapping [Mani/Robin]
 * Pad commit message to explain bindings break [Krzysztof]
 * Split dt/bindings per SoC  [Krzysztof] 

Sibi Sankar (11):
  dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema
  dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region
  dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region
  dt-bindings: remoteproc: qcom,sc7280-mss-pil: Update memory-region
  remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region
    before/after use"
  remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem
    headers
  arm64: dts: qcom: msm8996: Add a carveout for modem metadata
  arm64: dts: qcom: msm8998: Add a carveout for modem metadata
  arm64: dts: qcom: sdm845: Add a carveout for modem metadata
  arm64: dts: qcom: sc7180: Add a carveout for modem metadata
  arm64: dts: qcom: sc7280: Add a carveout for modem metadata

 .../remoteproc/qcom,msm8996-mss-pil.yaml      | 382 ++++++++++++++++++
 .../bindings/remoteproc/qcom,q6v5.txt         | 137 +------
 .../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            |  78 ++--
 12 files changed, 486 insertions(+), 171 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml

-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-10  9:33   ` Krzysztof Kozlowski
  2023-01-09  3:48 ` [PATCH V2 02/11] dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region Sibi Sankar
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Convert MSM8996 and similar (MSM8998/SDM845) MSS PIL bindings to schema.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 .../remoteproc/qcom,msm8996-mss-pil.yaml      | 370 ++++++++++++++++++
 .../bindings/remoteproc/qcom,q6v5.txt         | 137 +------
 2 files changed, 375 insertions(+), 132 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
new file mode 100644
index 000000000000..d3d3fb2fe91d
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
@@ -0,0 +1,370 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,msm8996-mss-pil.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm MSM8996 MSS Peripheral Image Loader (and similar)
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Sibi Sankar <quic_sibis@quicinc.com>
+
+description:
+  This document describes the hardware for a component that loads and boots firmware
+  on the Qualcomm Technology Inc. MSM8996 Modem Hexagon Core (and similar).
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - qcom,msm8996-mss-pil
+          - qcom,msm8998-mss-pil
+          - qcom,sdm845-mss-pil
+
+  reg:
+    items:
+      - description: MSS QDSP6 registers
+      - description: RMB registers
+
+  reg-names:
+    items:
+      - const: qdsp6
+      - const: rmb
+
+  iommus:
+    items:
+      - description: MSA Stream 1
+      - description: MSA Stream 2
+
+  interrupts:
+    items:
+      - description: Watchdog interrupt
+      - description: Fatal interrupt
+      - description: Ready interrupt
+      - description: Handover interrupt
+      - description: Stop acknowledge interrupt
+      - description: Shutdown acknowledge interrupt
+
+  interrupt-names:
+    items:
+      - const: wdog
+      - const: fatal
+      - const: ready
+      - const: handover
+      - const: stop-ack
+      - const: shutdown-ack
+
+  clocks:
+    minItems: 8
+    maxItems: 9
+
+  clock-names:
+    minItems: 8
+    maxItems: 9
+
+  power-domains:
+    items:
+      - description: CX power domain
+      - description: MX power domain
+      - description: MSS power domain (only valid for qcom,sdm845-mss-pil)
+    minItems: 2
+
+  power-domain-names:
+    items:
+      - const: cx
+      - const: mx
+      - const: mss # only valid for qcom,sdm845-mss-pil
+    minItems: 2
+
+  pll-supply:
+    description: PLL supply
+
+  resets:
+    items:
+      - description: AOSS restart
+      - description: PDC reset (only valid for qcom,sdm845-mss-pil)
+    minItems: 1
+
+  reset-names:
+    items:
+      - const: mss_restart
+      - const: pdc_reset # only valid for qcom,sdm845-mss-pil
+    minItems: 1
+
+  qcom,qmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Reference to the AOSS side-channel message RAM.
+
+  qcom,smem-states:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: States used by the AP to signal the Hexagon core
+    items:
+      - description: Stop modem
+
+  qcom,smem-state-names:
+    description: Names of the states used by the AP to signal the Hexagon core
+    items:
+      - const: stop
+
+  qcom,halt-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Halt registers are used to halt transactions of various sub-components
+      within MSS.
+    items:
+      - items:
+          - description: phandle to TCSR syscon region
+          - description: offset to the Q6 halt register
+          - description: offset to the modem halt register
+          - description: offset to the nc halt register
+
+  memory-region:
+    items:
+      - description: MBA reserved region
+      - description: Modem reserved region
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    items:
+      - description: Name of MBA firmware
+      - description: Name of modem firmware
+
+  smd-edge:
+    $ref: /schemas/remoteproc/qcom,smd-edge.yaml#
+    description:
+      Qualcomm Shared Memory subnode which represents communication edge,
+      channels and devices related to the Modem.
+    unevaluatedProperties: false
+
+  glink-edge:
+    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
+    description:
+      Qualcomm G-Link subnode which represents communication edge, channels
+      and devices related to the Modem.
+    unevaluatedProperties: false
+
+  # Deprecated properties
+  mba:
+    type: object
+    description:
+      MBA reserved region
+    properties:
+      memory-region: true
+    required:
+      - memory-region
+    deprecated: true
+
+  mpss:
+    type: object
+    description:
+      MPSS reserved region
+    properties:
+      memory-region: true
+    required:
+      - memory-region
+    deprecated: true
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - power-domains
+  - power-domain-names
+  - resets
+  - reset-names
+  - qcom,halt-regs
+  - qcom,smem-states
+  - qcom,smem-state-names
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          const: qcom,msm8996-mss-pil
+    then:
+      properties:
+        clocks:
+          items:
+            - description: GCC MSS IFACE clock
+            - description: GCC MSS BUS clock
+            - description: GCC MSS MEM clock
+            - description: RPMH XO clock
+            - description: GCC MSS GPLL0 clock
+            - description: GCC MSS SNOC_AXI clock
+            - description: GCC MSS MNOC_AXI clock
+            - description: RPMH PNOC clock
+            - description: GCC MSS PRNG clock
+            - description: RPMH QDSS clock
+        clock-names:
+          items:
+            - const: iface
+            - const: bus
+            - const: mem
+            - const: xo
+            - const: gpll0_mss
+            - const: snoc_axi
+            - const: mnoc_axi
+            - const: pnoc
+            - const: qdss
+        glink-edge: false
+      required:
+        - pll-supply
+        - smd-edge
+    else:
+      properties:
+        pll-supply: false
+        smd-edge: false
+
+  - if:
+      properties:
+        compatible:
+          const: qcom,msm8998-mss-pil
+    then:
+      properties:
+        clocks:
+          items:
+            - description: GCC MSS IFACE clock
+            - description: GCC MSS BUS clock
+            - description: GCC MSS MEM clock
+            - description: GCC MSS GPLL0 clock
+            - description: GCC MSS SNOC_AXI clock
+            - description: GCC MSS MNOC_AXI clock
+            - description: RPMH QDSS clock
+            - description: RPMH XO clock
+        clock-names:
+          items:
+            - const: iface
+            - const: bus
+            - const: mem
+            - const: gpll0_mss
+            - const: snoc_axi
+            - const: mnoc_axi
+            - const: qdss
+            - const: xo
+      required:
+        - glink-edge
+
+  - if:
+      properties:
+        compatible:
+          const: qcom,sdm845-mss-pil
+    then:
+      properties:
+        power-domains:
+          minItems: 3
+        power-domain-names:
+          minItems: 3
+        resets:
+          minItems: 2
+        reset-names:
+          minItems: 2
+        clocks:
+          items:
+            - description: GCC MSS IFACE clock
+            - description: GCC MSS BUS clock
+            - description: GCC MSS MEM clock
+            - description: GCC MSS GPLL0 clock
+            - description: GCC MSS SNOC_AXI clock
+            - description: GCC MSS MNOC_AXI clock
+            - description: GCC MSS PRNG clock
+            - description: RPMH XO clock
+        clock-names:
+          items:
+            - const: iface
+            - const: bus
+            - const: mem
+            - const: gpll0_mss
+            - const: snoc_axi
+            - const: mnoc_axi
+            - const: prng
+            - const: xo
+      required:
+        - qcom,qmp
+        - glink-edge
+    else:
+      properties:
+        iommus: false
+        power-domains:
+          maxItems: 2
+        power-domain-names:
+          maxItems: 2
+        resets:
+          maxItems: 1
+        reset-names:
+          maxItems: 1
+        qcom,qmp: false
+
+  # Fallbacks for deprecated properties
+  - oneOf:
+      - required:
+          - memory-region
+      - required:
+          - mba
+          - mpss
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+    #include <dt-bindings/reset/qcom,sdm845-aoss.h>
+    #include <dt-bindings/reset/qcom,sdm845-pdc.h>
+
+    remoteproc@4080000 {
+        compatible = "qcom,sdm845-mss-pil";
+        reg = <0x04080000 0x408>, <0x04180000 0x48>;
+        reg-names = "qdsp6", "rmb";
+
+        interrupts-extended = <&intc GIC_SPI 266 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "wdog", "fatal", "ready", "handover", "stop-ack",
+                          "shutdown-ack";
+
+        clocks = <&gcc GCC_MSS_CFG_AHB_CLK>,
+                 <&gcc GCC_MSS_Q6_MEMNOC_AXI_CLK>,
+                 <&gcc GCC_BOOT_ROM_AHB_CLK>,
+                 <&gcc GCC_MSS_GPLL0_DIV_CLK_SRC>,
+                 <&gcc GCC_MSS_SNOC_AXI_CLK>,
+                 <&gcc GCC_MSS_MFAB_AXIS_CLK>,
+                 <&gcc GCC_PRNG_AHB_CLK>,
+                 <&rpmhcc RPMH_CXO_CLK>;
+        clock-names = "iface", "bus", "mem", "gpll0_mss",
+                      "snoc_axi", "mnoc_axi", "prng", "xo";
+
+        power-domains = <&rpmhpd SDM845_CX>,
+                        <&rpmhpd SDM845_MX>,
+                        <&rpmhpd SDM845_MSS>;
+        power-domain-names = "cx", "mx", "mss";
+
+        memory-region = <&mba_mem>, <&mpss_mem>;
+
+        resets = <&aoss_reset AOSS_CC_MSS_RESTART>,
+                 <&pdc_reset PDC_MODEM_SYNC_RESET>;
+        reset-names = "mss_restart", "pdc_reset";
+
+        qcom,halt-regs = <&tcsr_regs_1 0x3000 0x5000 0x4000>;
+
+        qcom,qmp = <&aoss_qmp>;
+
+        qcom,smem-states = <&modem_smp2p_out 0>;
+        qcom,smem-state-names = "stop";
+
+        glink-edge {
+            interrupts = <GIC_SPI 449 IRQ_TYPE_EDGE_RISING>;
+            label = "modem";
+            qcom,remote-pid = <1>;
+            mboxes = <&apss_shared 12>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 5923c0447e2d..573a88b60677 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -9,9 +9,6 @@ on the Qualcomm Hexagon core.
 	Definition: must be one of:
 		    "qcom,ipq8074-wcss-pil"
 		    "qcom,qcs404-wcss-pil"
-		    "qcom,msm8996-mss-pil"
-		    "qcom,msm8998-mss-pil"
-		    "qcom,sdm845-mss-pil"
 
 - reg:
 	Usage: required
@@ -32,23 +29,7 @@ on the Qualcomm Hexagon core.
 - interrupt-names:
 	Usage: required
 	Value type: <stringlist>
-	Definition: The interrupts needed depends on the compatible
-		    string:
-	qcom,ipq8074-wcss-pil:
-	qcom,qcs404-wcss-pil:
-		    must be "wdog", "fatal", "ready", "handover", "stop-ack"
-	qcom,msm8996-mss-pil:
-	qcom,msm8998-mss-pil:
-	qcom,sdm845-mss-pil:
-		    must be "wdog", "fatal", "ready", "handover", "stop-ack",
-		    "shutdown-ack"
-
-- firmware-name:
-	Usage: optional
-	Value type: <stringlist>
-	Definition: must list the relative firmware image paths for mba and
-		    modem. They are used for booting and authenticating the
-		    Hexagon core.
+	Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack"
 
 - clocks:
 	Usage: required
@@ -66,41 +47,23 @@ on the Qualcomm Hexagon core.
 		    "gcc_axim_cbcr", "lcc_ahbfabric_cbc", "tcsr_lcc_cbc",
 		    "lcc_abhs_cbc", "lcc_tcm_slave_cbc", "lcc_abhm_cbc",
 		    "lcc_axim_cbc", "lcc_bcr_sleep"
-	qcom,msm8996-mss-pil:
-		    must be "iface", "bus", "mem", "xo", "gpll0_mss",
-		    "snoc_axi", "mnoc_axi", "pnoc", "qdss"
-	qcom,msm8998-mss-pil:
-		    must be "iface", "bus", "mem", "xo", "gpll0_mss",
-		    "snoc_axi", "mnoc_axi", "qdss"
-	qcom,sdm845-mss-pil:
-		    must be "iface", "bus", "mem", "xo", "gpll0_mss",
-		    "snoc_axi", "mnoc_axi", "prng"
 
 - resets:
 	Usage: required
 	Value type: <phandle>
-	Definition: reference to the reset-controller for the modem sub-system
-		    reference to the list of 3 reset-controllers for the
+	Definition: reference to the list of 3 reset-controllers for the
 		    wcss sub-system
-		    reference to the list of 2 reset-controllers for the modem
-		    sub-system on SDM845 SoCs
 
 - reset-names:
 	Usage: required
 	Value type: <stringlist>
-	Definition: must be "mss_restart" for the modem sub-system
-		    must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
+	Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
 		    for the wcss sub-system
-		    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:
 - memory-region:
 	Usage: required
 	Value type: <phandle>
-	Definition: reference to the reserved-memory for the mba region followed
-		    by the mpss region
+	Definition: reference to wcss reserved-memory region.
 
 For the compatible string below the following supplies are required:
   "qcom,qcs404-wcss-pil"
@@ -110,36 +73,6 @@ For the compatible string below the following supplies are required:
 	Definition: reference to the regulators to be held on behalf of the
 		    booting of the Hexagon core
 
-For the compatible string below the following supplies are required:
-  "qcom,msm8996-mss-pil"
-- pll-supply:
-	Usage: required
-	Value type: <phandle>
-	Definition: reference to the regulators to be held on behalf of the
-		    booting of the Hexagon core
-
-- power-domains:
-	Usage: required
-	Value type: <phandle>
-	Definition: reference to power-domains that match power-domain-names
-
-- power-domain-names:
-	Usage: required
-	Value type: <stringlist>
-	Definition: The power-domains needed depend on the compatible string:
-	qcom,ipq8074-wcss-pil:
-		    no power-domain names required
-	qcom,msm8996-mss-pil:
-	qcom,msm8998-mss-pil:
-		    must be "cx", "mx"
-	qcom,sdm845-mss-pil:
-		    must be "cx", "mx", "mss"
-
-- qcom,qmp:
-	Usage: optional
-	Value type: <phandle>
-	Definition: reference to the AOSS side-channel message RAM.
-
 - qcom,smem-states:
 	Usage: required
 	Value type: <phandle>
@@ -155,16 +88,9 @@ For the compatible string below the following supplies are required:
 	Usage: required
 	Value type: <prop-encoded-array>
 	Definition: a phandle reference to a syscon representing TCSR followed
-		    by the three offsets within syscon for q6, modem and nc
+		    by the three offsets within syscon for q6, wcss and nc
 		    halt registers.
 
-The Hexagon node must contain iommus property as described in ../iommu/iommu.txt
-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:
-
 - memory-region:
 	Usage: required
 	Value type: <phandle>
@@ -174,56 +100,3 @@ The Hexagon node may also have an subnode named either "smd-edge" or
 "glink-edge" that describes the communication edge, channels and devices
 related to the Hexagon.  See ../soc/qcom/qcom,smd.yaml and
 ../soc/qcom/qcom,glink.txt for details on how to describe these.
-
-= EXAMPLE
-The following example describes the resources needed to boot control the
-Hexagon, as it is found on MSM8974 boards.
-
-	remoteproc@fc880000 {
-		compatible = "qcom,msm8974-mss-pil";
-		reg = <0xfc880000 0x100>, <0xfc820000 0x020>;
-		reg-names = "qdsp6", "rmb";
-
-		interrupts-extended = <&intc GIC_SPI 24 IRQ_TYPE_EDGE_RISING>,
-				      <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
-				      <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
-				      <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
-				      <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
-		interrupt-names = "wdog", "fatal", "ready", "handover", "stop-ack";
-
-		clocks = <&gcc GCC_MSS_Q6_BIMC_AXI_CLK>,
-			 <&gcc GCC_MSS_CFG_AHB_CLK>,
-			 <&gcc GCC_BOOT_ROM_AHB_CLK>,
-			 <&xo_board>;
-		clock-names = "iface", "bus", "mem", "xo";
-
-		resets = <&gcc GCC_MSS_RESTART>;
-		reset-names = "mss_restart";
-
-		cx-supply = <&pm8841_s2>;
-		mss-supply = <&pm8841_s3>;
-		mx-supply = <&pm8841_s1>;
-		pll-supply = <&pm8941_l12>;
-
-		qcom,halt-regs = <&tcsr_mutex_block 0x1180 0x1200 0x1280>;
-
-		qcom,smem-states = <&modem_smp2p_out 0>;
-		qcom,smem-state-names = "stop";
-
-		mba {
-			memory-region = <&mba_region>;
-		};
-
-		mpss {
-			memory-region = <&mpss_region>;
-		};
-
-		smd-edge {
-			interrupts = <GIC_SPI 25 IRQ_TYPE_EDGE_RISING>;
-
-			qcom,ipc = <&apcs 8 12>;
-			qcom,smd-edge = <0>;
-
-			label = "modem";
-		};
-	};
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 02/11] dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-10  9:35   ` Krzysztof Kozlowski
  2023-01-09  3:48 ` [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region Sibi Sankar
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

The dynamic memory region used for metadata authentication would still
be a part of the kernel mapping and any access to this region  by the
application processor after assigning it to the remote Q6 will result
in a XPU violation. This is fixed by using a no-map carveout instead.
Update the bindings to reflect the addition of the new modem metadata
carveout on MSM8996 (and similar) SoCs.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 .../bindings/remoteproc/qcom,msm8996-mss-pil.yaml  | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
index d3d3fb2fe91d..ad1a51c23949 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
@@ -123,6 +123,7 @@ properties:
     items:
       - description: MBA reserved region
       - description: Modem reserved region
+      - description: Metadata reserved region
 
   firmware-name:
     $ref: /schemas/types.yaml#/definitions/string-array
@@ -165,6 +166,16 @@ properties:
       - memory-region
     deprecated: true
 
+  metadata:
+    type: object
+    description:
+      Metadata reserved region
+    properties:
+      memory-region: true
+    required:
+      - memory-region
+    deprecated: true
+
 required:
   - compatible
   - reg
@@ -306,6 +317,7 @@ allOf:
       - required:
           - mba
           - mpss
+          - metadata
 
 additionalProperties: false
 
@@ -348,7 +360,7 @@ examples:
                         <&rpmhpd SDM845_MSS>;
         power-domain-names = "cx", "mx", "mss";
 
-        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>;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 02/11] dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09 14:30   ` Rob Herring
  2023-01-10  9:36   ` Krzysztof Kozlowski
  2023-01-09  3:48 ` [PATCH V2 04/11] dt-bindings: remoteproc: qcom,sc7280-mss-pil: " Sibi Sankar
                   ` (7 subsequent siblings)
  10 siblings, 2 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

The dynamic memory region used for metadata authentication would still
be a part of the kernel mapping and any access to this region  by the
application processor after assigning it to the remote Q6 will result
in a XPU violation. This is fixed by using a no-map carveout instead.
Update the bindings to reflect the addition of the new modem metadata
carveout on SC7180 SoC.

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

v2:
 * Pad commit message to explain bindings break [Krzysztof]
 * Split dt/bindings per SoC  [Krzysztof] 

 .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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>;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 04/11] dt-bindings: remoteproc: qcom,sc7280-mss-pil: Update memory-region
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (2 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-10  9:36   ` Krzysztof Kozlowski
  2023-01-09  3:48 ` [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use" Sibi Sankar
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

The dynamic memory region used for metadata authentication would still
be a part of the kernel mapping and any access to this region  by the
application processor after assigning it to the remote Q6 will result
in a XPU violation. This is fixed by using a no-map carveout instead.
Update the bindings to reflect the addition of the new modem metadata
carveout on SC7280 SoC.

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

v2:
 * Pad commit message to explain bindings break [Krzysztof]
 * Split dt/bindings per SoC  [Krzysztof] 

 .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml    | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use"
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (3 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 04/11] dt-bindings: remoteproc: qcom,sc7280-mss-pil: " Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09  8:18   ` Manivannan Sadhasivam
  2023-01-09  3:48 ` [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers Sibi Sankar
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.

The memory region allocated using dma_alloc_attr with no kernel mapping
attribute set would still be a part of the linear kernel map. Hence as a
precursor to using reserved memory for modem metadata region, revert back
to the simpler way of dynamic memory allocation.

Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
 1 file changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 2f4027664a0e..e2f765f87ec9 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -10,7 +10,6 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/devcoredump.h>
-#include <linux/dma-map-ops.h>
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -961,52 +960,27 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 				const char *fw_name)
 {
-	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
-	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
-	struct page **pages;
-	struct page *page;
+	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
 	dma_addr_t phys;
 	void *metadata;
 	int mdata_perm;
 	int xferop_ret;
 	size_t size;
-	void *vaddr;
-	int count;
+	void *ptr;
 	int ret;
-	int i;
 
 	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
-	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
-	if (!page) {
+	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
+	if (!ptr) {
 		kfree(metadata);
 		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
 		return -ENOMEM;
 	}
 
-	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
-	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
-	if (!pages) {
-		ret = -ENOMEM;
-		goto free_dma_attrs;
-	}
-
-	for (i = 0; i < count; i++)
-		pages[i] = nth_page(page, i);
-
-	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;
-	}
-
-	memcpy(vaddr, metadata, size);
-
-	vunmap(vaddr);
+	memcpy(ptr, metadata, size);
 
 	/* Hypervisor mapping to access metadata by modem */
 	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
@@ -1036,7 +1010,7 @@ 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);
+	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
 	kfree(metadata);
 
 	return ret < 0 ? ret : 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (4 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use" Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09  8:32   ` Manivannan Sadhasivam
  2023-01-09  3:48 ` [PATCH V2 07/11] arm64: dts: qcom: msm8996: Add a carveout for modem metadata Sibi Sankar
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Any access to the dynamically allocated metadata 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-and-tested-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>
---

v2:
 * Revert no_kernel_mapping [Mani/Robin]

 drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index e2f765f87ec9..b7a158751cef 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -215,6 +215,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;
@@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
-	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
-	if (!ptr) {
-		kfree(metadata);
-		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
-		return -ENOMEM;
+	if (qproc->mdata_phys) {
+		phys = qproc->mdata_phys;
+		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
+		if (!ptr) {
+			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
+				&qproc->mdata_phys, size);
+			ret = -EBUSY;
+			goto free_dma_attrs;
+		}
+	} else {
+		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
+		if (!ptr) {
+			kfree(metadata);
+			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
+			return -ENOMEM;
+		}
 	}
 
 	memcpy(ptr, metadata, size);
 
+	if (qproc->mdata_phys)
+		memunmap(ptr);
+
 	/* Hypervisor mapping to access metadata by modem */
 	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
 	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
@@ -1010,7 +1025,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, ptr, phys, dma_attrs);
+	if (!qproc->mdata_phys)
+		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
 	kfree(metadata);
 
 	return ret < 0 ? ret : 0;
@@ -1893,6 +1909,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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 07/11] arm64: dts: qcom: msm8996: Add a carveout for modem metadata
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (5 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 08/11] arm64: dts: qcom: msm8998: " Sibi Sankar
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Add a new carveout for modem metadata on MSM8996 SoCs.

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

v2:
 * Split dt/bindings per SoC  [Krzysztof] 

 arch/arm64/boot/dts/qcom/msm8996-xiaomi-common.dtsi | 6 ++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi               | 9 +++++++++
 2 files changed, 15 insertions(+)

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 87ff66ebde7b..2756ba5841a1 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 {
@@ -2462,6 +2467,10 @@
 				memory-region = <&mpss_mem>;
 			};
 
+			metadata {
+				memory-region = <&mdata_mem>;
+			};
+
 			smd-edge {
 				interrupts = <GIC_SPI 449 IRQ_TYPE_EDGE_RISING>;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 08/11] arm64: dts: qcom: msm8998: Add a carveout for modem metadata
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (6 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 07/11] arm64: dts: qcom: msm8996: Add a carveout for modem metadata Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 09/11] arm64: dts: qcom: sdm845: " Sibi Sankar
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Add a new carveout for modem metadata on MSM8998 SoCs.

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

v2:
 * Split dt/bindings per SoC  [Krzysztof] 

 arch/arm64/boot/dts/qcom/msm8998.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 18cc149b6be4..281a4cf0d236 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";
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 09/11] arm64: dts: qcom: sdm845: Add a carveout for modem metadata
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (7 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 08/11] arm64: dts: qcom: msm8998: " Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 10/11] arm64: dts: qcom: sc7180: " Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 11/11] arm64: dts: qcom: sc7280: " Sibi Sankar
  10 siblings, 0 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Add a new carveout for modem metadata on SDM845 SoCs.

Tested-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v2:
 * Split dt/bindings per SoC  [Krzysztof] 

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 26c4f45b6152..ccdacd605620 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;
@@ -3351,6 +3356,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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 10/11] arm64: dts: qcom: sc7180: Add a carveout for modem metadata
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (8 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 09/11] arm64: dts: qcom: sdm845: " Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  2023-01-09  3:48 ` [PATCH V2 11/11] arm64: dts: qcom: sc7280: " Sibi Sankar
  10 siblings, 0 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Add a new carveout for modem metadata on SC7180 SoCs.

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

v2:
 * Split dt/bindings per SoC  [Krzysztof] 

 arch/arm64/boot/dts/qcom/sc7180-idp.dts      | 7 ++++++-
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index c7a22c7976b7..9b8b023577ca 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 edb56c4d55a2..8b7652a94628 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",
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V2 11/11] arm64: dts: qcom: sc7280: Add a carveout for modem metadata
  2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
                   ` (9 preceding siblings ...)
  2023-01-09  3:48 ` [PATCH V2 10/11] arm64: dts: qcom: sc7180: " Sibi Sankar
@ 2023-01-09  3:48 ` Sibi Sankar
  10 siblings, 0 replies; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09  3:48 UTC (permalink / raw)
  To: andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	Sibi Sankar

Add a new carveout for modem metadata on SC7280 SoCs.

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

v2:
 * Split dt/bindings per SoC  [Krzysztof] 

 arch/arm64/boot/dts/qcom/sc7280-herobrine-lte-sku.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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 efd513164501..3a4074d6e5b7 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;
+		};
 	};
 };
 
@@ -37,7 +42,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";
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use"
  2023-01-09  3:48 ` [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use" Sibi Sankar
@ 2023-01-09  8:18   ` Manivannan Sadhasivam
  2023-01-09 10:00     ` Sibi Sankar
  0 siblings, 1 reply; 23+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-09  8:18 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: andersson, krzysztof.kozlowski+dt, robh+dt, robin.murphy, agross,
	linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	hch

+ Christoph

Hi Sibi,

On Mon, Jan 09, 2023 at 09:18:37AM +0530, Sibi Sankar wrote:
> This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
> 
> The memory region allocated using dma_alloc_attr with no kernel mapping
> attribute set would still be a part of the linear kernel map. Hence as a
> precursor to using reserved memory for modem metadata region, revert back
> to the simpler way of dynamic memory allocation.
> 
> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>

Christoph already submitted a patch that reverts fc156629b23a:
https://lore.kernel.org/linux-arm-msm/20221223092703.61927-2-hch@lst.de/

Thanks,
Mani

> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
>  1 file changed, 6 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 2f4027664a0e..e2f765f87ec9 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -10,7 +10,6 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/devcoredump.h>
> -#include <linux/dma-map-ops.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -961,52 +960,27 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>  				const char *fw_name)
>  {
> -	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
> -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> -	struct page **pages;
> -	struct page *page;
> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>  	dma_addr_t phys;
>  	void *metadata;
>  	int mdata_perm;
>  	int xferop_ret;
>  	size_t size;
> -	void *vaddr;
> -	int count;
> +	void *ptr;
>  	int ret;
> -	int i;
>  
>  	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!page) {
> +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +	if (!ptr) {
>  		kfree(metadata);
>  		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>  		return -ENOMEM;
>  	}
>  
> -	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> -	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> -	if (!pages) {
> -		ret = -ENOMEM;
> -		goto free_dma_attrs;
> -	}
> -
> -	for (i = 0; i < count; i++)
> -		pages[i] = nth_page(page, i);
> -
> -	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;
> -	}
> -
> -	memcpy(vaddr, metadata, size);
> -
> -	vunmap(vaddr);
> +	memcpy(ptr, metadata, size);
>  
>  	/* Hypervisor mapping to access metadata by modem */
>  	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> @@ -1036,7 +1010,7 @@ 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);
> +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>  	kfree(metadata);
>  
>  	return ret < 0 ? ret : 0;
> -- 
> 2.17.1
> 

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers
  2023-01-09  3:48 ` [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers Sibi Sankar
@ 2023-01-09  8:32   ` Manivannan Sadhasivam
  2023-01-09 10:05     ` Sibi Sankar
  0 siblings, 1 reply; 23+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-09  8:32 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: andersson, krzysztof.kozlowski+dt, robh+dt, robin.murphy, agross,
	linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:
> Any access to the dynamically allocated metadata 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-and-tested-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>
> ---
> 
> v2:
>  * Revert no_kernel_mapping [Mani/Robin]
> 
>  drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index e2f765f87ec9..b7a158751cef 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -215,6 +215,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;
> @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> -	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> -	if (!ptr) {
> -		kfree(metadata);
> -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> -		return -ENOMEM;
> +	if (qproc->mdata_phys) {
> +		phys = qproc->mdata_phys;
> +		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> +		if (!ptr) {
> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> +				&qproc->mdata_phys, size);
> +			ret = -EBUSY;
> +			goto free_dma_attrs;

There is no memory to free at this point.

Thanks,
Mani

> +		}
> +	} else {
> +		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> +		if (!ptr) {
> +			kfree(metadata);
> +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> +			return -ENOMEM;
> +		}
>  	}
>  
>  	memcpy(ptr, metadata, size);
>  
> +	if (qproc->mdata_phys)
> +		memunmap(ptr);
> +
>  	/* Hypervisor mapping to access metadata by modem */
>  	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>  	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
> @@ -1010,7 +1025,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, ptr, phys, dma_attrs);
> +	if (!qproc->mdata_phys)
> +		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>  	kfree(metadata);
>  
>  	return ret < 0 ? ret : 0;
> @@ -1893,6 +1909,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
> 

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use"
  2023-01-09  8:18   ` Manivannan Sadhasivam
@ 2023-01-09 10:00     ` Sibi Sankar
  2023-01-11 11:24       ` Manivannan Sadhasivam
  0 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09 10:00 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, krzysztof.kozlowski+dt, robh+dt, robin.murphy, agross,
	linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	hch

Hey Mani,
Thanks for taking time to review the series.

On 1/9/23 13:48, Manivannan Sadhasivam wrote:
> + Christoph
> 
> Hi Sibi,
> 
> On Mon, Jan 09, 2023 at 09:18:37AM +0530, Sibi Sankar wrote:
>> This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
>>
>> The memory region allocated using dma_alloc_attr with no kernel mapping
>> attribute set would still be a part of the linear kernel map. Hence as a
>> precursor to using reserved memory for modem metadata region, revert back
>> to the simpler way of dynamic memory allocation.
>>
>> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> 
> Christoph already submitted a patch that reverts fc156629b23a:
> https://lore.kernel.org/linux-arm-msm/20221223092703.61927-2-hch@lst.de/

Having ^^ revert as part of the this series makes more sense. I'll
just replace my patch with ^^ in the next re-spin.

> 
> Thanks,
> Mani
> 
>> ---
>>   drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
>>   1 file changed, 6 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 2f4027664a0e..e2f765f87ec9 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -10,7 +10,6 @@
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/devcoredump.h>
>> -#include <linux/dma-map-ops.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>> @@ -961,52 +960,27 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>>   				const char *fw_name)
>>   {
>> -	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
>> -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
>> -	struct page **pages;
>> -	struct page *page;
>> +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>>   	dma_addr_t phys;
>>   	void *metadata;
>>   	int mdata_perm;
>>   	int xferop_ret;
>>   	size_t size;
>> -	void *vaddr;
>> -	int count;
>> +	void *ptr;
>>   	int ret;
>> -	int i;
>>   
>>   	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
>>   	if (IS_ERR(metadata))
>>   		return PTR_ERR(metadata);
>>   
>> -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> -	if (!page) {
>> +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> +	if (!ptr) {
>>   		kfree(metadata);
>>   		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>>   		return -ENOMEM;
>>   	}
>>   
>> -	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> -	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
>> -	if (!pages) {
>> -		ret = -ENOMEM;
>> -		goto free_dma_attrs;
>> -	}
>> -
>> -	for (i = 0; i < count; i++)
>> -		pages[i] = nth_page(page, i);
>> -
>> -	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;
>> -	}
>> -
>> -	memcpy(vaddr, metadata, size);
>> -
>> -	vunmap(vaddr);
>> +	memcpy(ptr, metadata, size);
>>   
>>   	/* Hypervisor mapping to access metadata by modem */
>>   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>> @@ -1036,7 +1010,7 @@ 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);
>> +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>>   	kfree(metadata);
>>   
>>   	return ret < 0 ? ret : 0;
>> -- 
>> 2.17.1
>>
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers
  2023-01-09  8:32   ` Manivannan Sadhasivam
@ 2023-01-09 10:05     ` Sibi Sankar
  2023-01-11 11:23       ` Manivannan Sadhasivam
  0 siblings, 1 reply; 23+ messages in thread
From: Sibi Sankar @ 2023-01-09 10:05 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, krzysztof.kozlowski+dt, robh+dt, robin.murphy, agross,
	linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

Hey Mani,

On 1/9/23 14:02, Manivannan Sadhasivam wrote:
> On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:
>> Any access to the dynamically allocated metadata 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-and-tested-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>
>> ---
>>
>> v2:
>>   * Revert no_kernel_mapping [Mani/Robin]
>>
>>   drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
>>   1 file changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index e2f765f87ec9..b7a158751cef 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -215,6 +215,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;
>> @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>>   	if (IS_ERR(metadata))
>>   		return PTR_ERR(metadata);
>>   
>> -	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> -	if (!ptr) {
>> -		kfree(metadata);
>> -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> -		return -ENOMEM;
>> +	if (qproc->mdata_phys) {
>> +		phys = qproc->mdata_phys;
>> +		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
>> +		if (!ptr) {
>> +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
>> +				&qproc->mdata_phys, size);
>> +			ret = -EBUSY;
>> +			goto free_dma_attrs;
> 
> There is no memory to free at this point.

we would just free the metadata in the no-map carveout scenario since
mdata_phys wouldn't be NULL. I can do a kfree(metadata) directly from
this branch and return as well if you think it makes things more
readable.

> 
> Thanks,
> Mani
> 
>> +		}
>> +	} else {
>> +		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
>> +		if (!ptr) {
>> +			kfree(metadata);
>> +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
>> +			return -ENOMEM;
>> +		}
>>   	}
>>   
>>   	memcpy(ptr, metadata, size);
>>   
>> +	if (qproc->mdata_phys)
>> +		memunmap(ptr);
>> +
>>   	/* Hypervisor mapping to access metadata by modem */
>>   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
>>   	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
>> @@ -1010,7 +1025,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, ptr, phys, dma_attrs);
>> +	if (!qproc->mdata_phys)
>> +		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
>>   	kfree(metadata);
>>   
>>   	return ret < 0 ? ret : 0;
>> @@ -1893,6 +1909,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
>>
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region
  2023-01-09  3:48 ` [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region Sibi Sankar
@ 2023-01-09 14:30   ` Rob Herring
  2023-01-10  9:36   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 23+ messages in thread
From: Rob Herring @ 2023-01-09 14:30 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: manivannan.sadhasivam, will, robin.murphy, sumit.semwal, robh+dt,
	devicetree, amit.pundir, regressions, linux-kernel,
	catalin.marinas, krzysztof.kozlowski+dt, andersson,
	konrad.dybcio, linux-arm-msm, agross


On Mon, 09 Jan 2023 09:18:35 +0530, Sibi Sankar wrote:
> The dynamic memory region used for metadata authentication would still
> be a part of the kernel mapping and any access to this region  by the
> application processor after assigning it to the remote Q6 will result
> in a XPU violation. This is fixed by using a no-map carveout instead.
> Update the bindings to reflect the addition of the new modem metadata
> carveout on SC7180 SoC.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v2:
>  * Pad commit message to explain bindings break [Krzysztof]
>  * Split dt/bindings per SoC  [Krzysztof]
> 
>  .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml    | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230109034843.23759-4-quic_sibis@quicinc.com


remoteproc@4080000: memory-region: [[105], [106]] is too short
	arch/arm64/boot/dts/qcom/sc7180-idp.dtb

remoteproc@4080000: memory-region: [[122], [123]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick-r0-lte.dtb

remoteproc@4080000: memory-region: [[123], [124]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev0-auo.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev0-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev1-auo.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev1-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick-r0.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev0-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev0-inx.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-boe-rt5682s.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-inx.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-inx-rt5682s.dtb

remoteproc@4080000: memory-region: [[128], [129]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel360-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel360-wifi.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-parade.dtb

remoteproc@4080000: memory-region: [[129], [130]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r3-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r9.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-ti.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-ti.dtb

remoteproc@4080000: memory-region: [[130], [131]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r4.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r5.dtb

remoteproc@4080000: memory-region: [[131], [132]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar-r2.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar-r4.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-kingoftown-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r9.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-kb.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-lte.dtb

remoteproc@4080000: memory-region: [[132], [133]] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-kingoftown-r0.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r4.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-kb.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3-kb.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-r1-lte.dtb

remoteproc@4080000: qcom,halt-regs:0: [110] is too short
	arch/arm64/boot/dts/qcom/sc7180-idp.dtb

remoteproc@4080000: qcom,halt-regs:0: [127] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick-r0-lte.dtb

remoteproc@4080000: qcom,halt-regs:0: [128] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev0-auo.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev0-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev1-auo.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland-rev1-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick-r0.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev0-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev0-inx.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-boe.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-boe-rt5682s.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-inx.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler-rev1-inx-rt5682s.dtb

remoteproc@4080000: qcom,halt-regs:0: [133] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel360-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel360-wifi.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-parade.dtb

remoteproc@4080000: qcom,halt-regs:0: [134] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r3-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r9.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-ti.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-ti.dtb

remoteproc@4080000: qcom,halt-regs:0: [135] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r4.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r5.dtb

remoteproc@4080000: qcom,halt-regs:0: [136] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar-r2.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar-r4.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-kingoftown-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r9.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-kb.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-lte.dtb

remoteproc@4080000: qcom,halt-regs:0: [137] is too short
	arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-kingoftown-r0.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r4.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-kb.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3-kb.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dtb
	arch/arm64/boot/dts/qcom/sc7180-trogdor-r1-lte.dtb


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema
  2023-01-09  3:48 ` [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema Sibi Sankar
@ 2023-01-10  9:33   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-10  9:33 UTC (permalink / raw)
  To: Sibi Sankar, andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

On 09/01/2023 04:48, Sibi Sankar wrote:
> Convert MSM8996 and similar (MSM8998/SDM845) MSS PIL bindings to schema.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  .../remoteproc/qcom,msm8996-mss-pil.yaml      | 370 ++++++++++++++++++
>  .../bindings/remoteproc/qcom,q6v5.txt         | 137 +------
>  2 files changed, 375 insertions(+), 132 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
> new file mode 100644
> index 000000000000..d3d3fb2fe91d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
> @@ -0,0 +1,370 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,msm8996-mss-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm MSM8996 MSS Peripheral Image Loader (and similar)
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Sibi Sankar <quic_sibis@quicinc.com>
> +
> +description:
> +  This document describes the hardware for a component that loads and boots firmware
> +  on the Qualcomm Technology Inc. MSM8996 Modem Hexagon Core (and similar).

MSS Peripheral Image Loader loads and boots firmware
on the Qualcomm Technology Inc. MSM8996 Modem Hexagon Core (and similar).

> +
> +properties:
> +  compatible:
> +    oneOf:

It's not oneOf. Just enum.


> +      - enum:
> +          - qcom,msm8996-mss-pil
> +          - qcom,msm8998-mss-pil
> +          - qcom,sdm845-mss-pil
> +

With both above:

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


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 02/11] dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region
  2023-01-09  3:48 ` [PATCH V2 02/11] dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region Sibi Sankar
@ 2023-01-10  9:35   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-10  9:35 UTC (permalink / raw)
  To: Sibi Sankar, andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

On 09/01/2023 04:48, Sibi Sankar wrote:
> The dynamic memory region used for metadata authentication would still
> be a part of the kernel mapping and any access to this region  by the

Just one space before "by"

> application processor after assigning it to the remote Q6 will result
> in a XPU violation. This is fixed by using a no-map carveout instead.
> Update the bindings to reflect the addition of the new modem metadata
> carveout on MSM8996 (and similar) SoCs.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>  .../bindings/remoteproc/qcom,msm8996-mss-pil.yaml  | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
> index d3d3fb2fe91d..ad1a51c23949 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8996-mss-pil.yaml
> @@ -123,6 +123,7 @@ properties:
>      items:
>        - description: MBA reserved region
>        - description: Modem reserved region
> +      - description: Metadata reserved region
>  
>    firmware-name:
>      $ref: /schemas/types.yaml#/definitions/string-array
> @@ -165,6 +166,16 @@ properties:
>        - memory-region
>      deprecated: true
>  
> +  metadata:
> +    type: object

addutionalProperties: false

> +    description:
> +      Metadata reserved region

Blank line

> +    properties:
> +      memory-region: true

Blank line

> +    required:
> +      - memory-region
> +    deprecated: true


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region
  2023-01-09  3:48 ` [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region Sibi Sankar
  2023-01-09 14:30   ` Rob Herring
@ 2023-01-10  9:36   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-10  9:36 UTC (permalink / raw)
  To: Sibi Sankar, andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

On 09/01/2023 04:48, Sibi Sankar wrote:
> The dynamic memory region used for metadata authentication would still
> be a part of the kernel mapping and any access to this region  by the

One space, not two before "by"


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


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 04/11] dt-bindings: remoteproc: qcom,sc7280-mss-pil: Update memory-region
  2023-01-09  3:48 ` [PATCH V2 04/11] dt-bindings: remoteproc: qcom,sc7280-mss-pil: " Sibi Sankar
@ 2023-01-10  9:36   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-10  9:36 UTC (permalink / raw)
  To: Sibi Sankar, andersson, krzysztof.kozlowski+dt, robh+dt,
	manivannan.sadhasivam, robin.murphy
  Cc: agross, linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

On 09/01/2023 04:48, Sibi Sankar wrote:
> The dynamic memory region used for metadata authentication would still
> be a part of the kernel mapping and any access to this region  by the

Same problem.

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

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers
  2023-01-09 10:05     ` Sibi Sankar
@ 2023-01-11 11:23       ` Manivannan Sadhasivam
  0 siblings, 0 replies; 23+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-11 11:23 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: andersson, krzysztof.kozlowski+dt, robh+dt, robin.murphy, agross,
	linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas

On Mon, Jan 09, 2023 at 03:35:31PM +0530, Sibi Sankar wrote:
> Hey Mani,
> 
> On 1/9/23 14:02, Manivannan Sadhasivam wrote:
> > On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:
> > > Any access to the dynamically allocated metadata 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-and-tested-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>
> > > ---
> > > 
> > > v2:
> > >   * Revert no_kernel_mapping [Mani/Robin]
> > > 
> > >   drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++----
> > >   1 file changed, 42 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index e2f765f87ec9..b7a158751cef 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -215,6 +215,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;
> > > @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
> > >   	if (IS_ERR(metadata))
> > >   		return PTR_ERR(metadata);
> > > -	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > -	if (!ptr) {
> > > -		kfree(metadata);
> > > -		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > -		return -ENOMEM;
> > > +	if (qproc->mdata_phys) {
> > > +		phys = qproc->mdata_phys;
> > > +		ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC);
> > > +		if (!ptr) {
> > > +			dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
> > > +				&qproc->mdata_phys, size);
> > > +			ret = -EBUSY;
> > > +			goto free_dma_attrs;
> > 
> > There is no memory to free at this point.
> 
> we would just free the metadata in the no-map carveout scenario since
> mdata_phys wouldn't be NULL. I can do a kfree(metadata) directly from
> this branch and return as well if you think it makes things more
> readable.
> 

Oops, I missed that. But yeah it is confusing too with the current way of
freeing metadata. I'd suggest using a separate label instead.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > +		}
> > > +	} else {
> > > +		ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > +		if (!ptr) {
> > > +			kfree(metadata);
> > > +			dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > > +			return -ENOMEM;
> > > +		}
> > >   	}
> > >   	memcpy(ptr, metadata, size);
> > > +	if (qproc->mdata_phys)
> > > +		memunmap(ptr);
> > > +
> > >   	/* Hypervisor mapping to access metadata by modem */
> > >   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> > >   	ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true,
> > > @@ -1010,7 +1025,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, ptr, phys, dma_attrs);
> > > +	if (!qproc->mdata_phys)
> > > +		dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
> > >   	kfree(metadata);
> > >   	return ret < 0 ? ret : 0;
> > > @@ -1893,6 +1909,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
> > > 
> > 

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use"
  2023-01-09 10:00     ` Sibi Sankar
@ 2023-01-11 11:24       ` Manivannan Sadhasivam
  0 siblings, 0 replies; 23+ messages in thread
From: Manivannan Sadhasivam @ 2023-01-11 11:24 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: andersson, krzysztof.kozlowski+dt, robh+dt, robin.murphy, agross,
	linux-arm-msm, devicetree, linux-kernel, konrad.dybcio,
	amit.pundir, regressions, sumit.semwal, will, catalin.marinas,
	hch

On Mon, Jan 09, 2023 at 03:30:22PM +0530, Sibi Sankar wrote:
> Hey Mani,
> Thanks for taking time to review the series.
> 
> On 1/9/23 13:48, Manivannan Sadhasivam wrote:
> > + Christoph
> > 
> > Hi Sibi,
> > 
> > On Mon, Jan 09, 2023 at 09:18:37AM +0530, Sibi Sankar wrote:
> > > This reverts commit fc156629b23a21181e473e60341e3a78af25a1d4.
> > > 
> > > The memory region allocated using dma_alloc_attr with no kernel mapping
> > > attribute set would still be a part of the linear kernel map. Hence as a
> > > precursor to using reserved memory for modem metadata region, revert back
> > > to the simpler way of dynamic memory allocation.
> > > 
> > > Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> > 
> > Christoph already submitted a patch that reverts fc156629b23a:
> > https://lore.kernel.org/linux-arm-msm/20221223092703.61927-2-hch@lst.de/
> 
> Having ^^ revert as part of the this series makes more sense. I'll
> just replace my patch with ^^ in the next re-spin.
> 

That makes sense to me.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > ---
> > >   drivers/remoteproc/qcom_q6v5_mss.c | 38 +++++-------------------------
> > >   1 file changed, 6 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 2f4027664a0e..e2f765f87ec9 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -10,7 +10,6 @@
> > >   #include <linux/clk.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/devcoredump.h>
> > > -#include <linux/dma-map-ops.h>
> > >   #include <linux/dma-mapping.h>
> > >   #include <linux/interrupt.h>
> > >   #include <linux/kernel.h>
> > > @@ -961,52 +960,27 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
> > >   static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
> > >   				const char *fw_name)
> > >   {
> > > -	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS | DMA_ATTR_NO_KERNEL_MAPPING;
> > > -	unsigned long flags = VM_DMA_COHERENT | VM_FLUSH_RESET_PERMS;
> > > -	struct page **pages;
> > > -	struct page *page;
> > > +	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> > >   	dma_addr_t phys;
> > >   	void *metadata;
> > >   	int mdata_perm;
> > >   	int xferop_ret;
> > >   	size_t size;
> > > -	void *vaddr;
> > > -	int count;
> > > +	void *ptr;
> > >   	int ret;
> > > -	int i;
> > >   	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
> > >   	if (IS_ERR(metadata))
> > >   		return PTR_ERR(metadata);
> > > -	page = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > -	if (!page) {
> > > +	ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);
> > > +	if (!ptr) {
> > >   		kfree(metadata);
> > >   		dev_err(qproc->dev, "failed to allocate mdt buffer\n");
> > >   		return -ENOMEM;
> > >   	}
> > > -	count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > > -	pages = kmalloc_array(count, sizeof(struct page *), GFP_KERNEL);
> > > -	if (!pages) {
> > > -		ret = -ENOMEM;
> > > -		goto free_dma_attrs;
> > > -	}
> > > -
> > > -	for (i = 0; i < count; i++)
> > > -		pages[i] = nth_page(page, i);
> > > -
> > > -	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;
> > > -	}
> > > -
> > > -	memcpy(vaddr, metadata, size);
> > > -
> > > -	vunmap(vaddr);
> > > +	memcpy(ptr, metadata, size);
> > >   	/* Hypervisor mapping to access metadata by modem */
> > >   	mdata_perm = BIT(QCOM_SCM_VMID_HLOS);
> > > @@ -1036,7 +1010,7 @@ 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);
> > > +	dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs);
> > >   	kfree(metadata);
> > >   	return ret < 0 ? ret : 0;
> > > -- 
> > > 2.17.1
> > > 
> > 

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2023-01-11 11:24 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09  3:48 [PATCH V2 00/11] Fix XPU violation during modem metadata authentication Sibi Sankar
2023-01-09  3:48 ` [PATCH V2 01/11] dt-bindings: remoteproc: qcom,q6v5: Move MSM8996 to schema Sibi Sankar
2023-01-10  9:33   ` Krzysztof Kozlowski
2023-01-09  3:48 ` [PATCH V2 02/11] dt-bindings: remoteproc: qcom,msm8996-mss-pil: Update memory region Sibi Sankar
2023-01-10  9:35   ` Krzysztof Kozlowski
2023-01-09  3:48 ` [PATCH V2 03/11] dt-bindings: remoteproc: qcom,sc7180-mss-pil: Update memory-region Sibi Sankar
2023-01-09 14:30   ` Rob Herring
2023-01-10  9:36   ` Krzysztof Kozlowski
2023-01-09  3:48 ` [PATCH V2 04/11] dt-bindings: remoteproc: qcom,sc7280-mss-pil: " Sibi Sankar
2023-01-10  9:36   ` Krzysztof Kozlowski
2023-01-09  3:48 ` [PATCH V2 05/11] remoteproc: qcom_q6v5_mss: revert "map/unmap metadata region before/after use" Sibi Sankar
2023-01-09  8:18   ` Manivannan Sadhasivam
2023-01-09 10:00     ` Sibi Sankar
2023-01-11 11:24       ` Manivannan Sadhasivam
2023-01-09  3:48 ` [PATCH V2 06/11] remoteproc: qcom_q6v5_mss: Use a carveout to authenticate modem headers Sibi Sankar
2023-01-09  8:32   ` Manivannan Sadhasivam
2023-01-09 10:05     ` Sibi Sankar
2023-01-11 11:23       ` Manivannan Sadhasivam
2023-01-09  3:48 ` [PATCH V2 07/11] arm64: dts: qcom: msm8996: Add a carveout for modem metadata Sibi Sankar
2023-01-09  3:48 ` [PATCH V2 08/11] arm64: dts: qcom: msm8998: " Sibi Sankar
2023-01-09  3:48 ` [PATCH V2 09/11] arm64: dts: qcom: sdm845: " Sibi Sankar
2023-01-09  3:48 ` [PATCH V2 10/11] arm64: dts: qcom: sc7180: " Sibi Sankar
2023-01-09  3:48 ` [PATCH V2 11/11] arm64: dts: qcom: sc7280: " Sibi Sankar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.