imx.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
@ 2025-02-26 16:53 Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 1/5] dt-bindings: bus: add documentation " Laurentiu Mihalcea
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-02-26 16:53 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The AIPSTZ bridge offers some security-related configurations which can
be used to restrict master access to certain peripherals on the bridge.

Normally, this could be done from a secure environment such as ATF before
Linux boots but the configuration of AIPSTZ5 is lost each time the power
domain is powered off and then powered on. Because of this, it has to be
configured each time the power domain is turned on and before any master
tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).

The child-parent relationship between the bridge and its peripherals
should guarantee that the bridge is configured before the AP attempts
to access the IPs.

Other masters should use the 'access-controllers' property to enforce
a dependency between their device and the bridge device (see the DSP,
for example).

At the moment, we only want to apply a default, more relaxed
configuration, which is why the number of access controller cells
is 0.

The initial version of the series can be found at [1]. The new version
should provide better management of the device dependencies.

[1]: https://lore.kernel.org/linux-arm-kernel/20241119130726.2761726-1-daniel.baluta@nxp.com/

---
Changes in v2:
* adress Frank Li's comments
* pick up some A-b/R-b's
* don't use "simple-bus" as the second compatible. As per Krzysztof's
comment, AIPSTZ is not a "simple-bus".
---

Laurentiu Mihalcea (5):
  dt-bindings: bus: add documentation for the IMX AIPSTZ bridge
  dt-bindings: dsp: fsl,dsp: document 'access-controllers' property
  bus: add driver for IMX AIPSTZ bridge
  arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'
  arm64: dts: imx8mp: make 'dsp' node depend on 'aips5'

 .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 86 +++++++++++++++++
 .../devicetree/bindings/dsp/fsl,dsp.yaml      |  3 +
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |  9 +-
 drivers/bus/Kconfig                           |  6 ++
 drivers/bus/Makefile                          |  1 +
 drivers/bus/imx-aipstz.c                      | 92 +++++++++++++++++++
 6 files changed, 194 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
 create mode 100644 drivers/bus/imx-aipstz.c

-- 
2.34.1


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

* [PATCH v2 1/5] dt-bindings: bus: add documentation for the IMX AIPSTZ bridge
  2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
@ 2025-02-26 16:53 ` Laurentiu Mihalcea
  2025-02-26 21:16   ` Krzysztof Kozlowski
  2025-02-26 16:53 ` [PATCH v2 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property Laurentiu Mihalcea
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-02-26 16:53 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Add documentation for IMX AIPSTZ bridge.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml

diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
new file mode 100644
index 000000000000..dfcfe4a8ae74
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bus/fsl,imx8mp-aipstz.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Secure AHB to IP Slave bus (AIPSTZ) bridge
+
+description:
+  The secure AIPS bridge (AIPSTZ) acts as a bridge for AHB masters
+  issuing transactions to IP Slave peripherals. Additionally, this module
+  offers access control configurations meant to restrict which peripherals
+  a master can access.
+
+maintainers:
+  - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
+
+properties:
+  compatible:
+    const: fsl,imx8mp-aipstz
+
+  reg:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  "#address-cells":
+    enum: [1, 2]
+
+  "#size-cells":
+    enum: [1, 2]
+
+  "#access-controller-cells":
+    const: 0
+
+  ranges: true
+
+# borrowed from simple-bus.yaml, no additional requirements for children
+patternProperties:
+  "@(0|[1-9a-f][0-9a-f]*)$":
+    type: object
+    additionalProperties: true
+    properties:
+      reg:
+        items:
+          minItems: 2
+          maxItems: 4
+        minItems: 1
+        maxItems: 1024
+      ranges:
+        oneOf:
+          - items:
+              minItems: 3
+              maxItems: 7
+            minItems: 1
+            maxItems: 1024
+          - $ref: /schemas/types.yaml#/definitions/flag
+    anyOf:
+      - required:
+          - reg
+      - required:
+          - ranges
+
+required:
+  - compatible
+  - reg
+  - power-domains
+  - "#address-cells"
+  - "#size-cells"
+  - "#access-controller-cells"
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    bus@30df0000 {
+      compatible = "fsl,imx8mp-aipstz";
+      reg = <0x30df0000 0x10000>;
+      power-domains = <&pgc_audio>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      #access-controller-cells = <0>;
+      ranges;
+    };
-- 
2.34.1


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

* [PATCH v2 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property
  2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 1/5] dt-bindings: bus: add documentation " Laurentiu Mihalcea
@ 2025-02-26 16:53 ` Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 3/5] bus: add driver for IMX AIPSTZ bridge Laurentiu Mihalcea
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-02-26 16:53 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Some DSP instances may have their access to certain peripherals
conditioned by a bus access controller such as the one from the
AIPSTZ bridge.

Add the optional 'access-controllers' property, which may be used
in such cases.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
index ab93ffd3d2e5..869df7fcece0 100644
--- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
+++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
@@ -82,6 +82,9 @@ properties:
     description:
       Phandle to syscon block which provide access for processor enablement
 
+  access-controllers:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.34.1


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

* [PATCH v2 3/5] bus: add driver for IMX AIPSTZ bridge
  2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 1/5] dt-bindings: bus: add documentation " Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property Laurentiu Mihalcea
@ 2025-02-26 16:53 ` Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5' Laurentiu Mihalcea
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-02-26 16:53 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The secure AHB to IP Slave (AIPSTZ) bus bridge provides access control
configurations meant to restrict access to certain peripherals.
Some of the configurations include:

	1) Marking masters as trusted for R/W. Based on this
	(and the configuration of the accessed peripheral), the bridge
	may choose to abort the R/W transactions issued by certain
	masters.

	2) Allowing/disallowing write accesses to peripherals.

Add driver for this IP. Since there's currently no framework for
access controllers (and since there's currently no need for having
flexibility w.r.t the configurations) all this driver does is it
applies a relaxed, "default" configuration, in which all masters
are trusted for R/W.

Note that some instances of this IP (e.g: AIPSTZ5 on i.MX8MP) may be tied
to a power domain and may lose their configuration when the domain is
powered off. This is why the configuration has to be restored when the
domain is powered on.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 drivers/bus/Kconfig      |  6 +++
 drivers/bus/Makefile     |  1 +
 drivers/bus/imx-aipstz.c | 92 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 drivers/bus/imx-aipstz.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index ff669a8ccad9..fe7600283e70 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -87,6 +87,12 @@ config HISILICON_LPC
 	  Driver to enable I/O access to devices attached to the Low Pin
 	  Count bus on the HiSilicon Hip06/7 SoC.
 
+config IMX_AIPSTZ
+	tristate "Support for IMX Secure AHB to IP Slave bus (AIPSTZ) bridge"
+	depends on ARCH_MXC
+	help
+	  Enable support for IMX AIPSTZ bridge.
+
 config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index cddd4984d6af..8e693fe8a03a 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
 
 obj-$(CONFIG_BT1_APB)		+= bt1-apb.o
 obj-$(CONFIG_BT1_AXI)		+= bt1-axi.o
+obj-$(CONFIG_IMX_AIPSTZ)	+= imx-aipstz.o
 obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
 obj-$(CONFIG_INTEL_IXP4XX_EB)	+= intel-ixp4xx-eb.o
 obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
diff --git a/drivers/bus/imx-aipstz.c b/drivers/bus/imx-aipstz.c
new file mode 100644
index 000000000000..c90580da17a1
--- /dev/null
+++ b/drivers/bus/imx-aipstz.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#define IMX_AIPSTZ_MPR0 0x0
+
+struct imx_aipstz_config {
+	u32 mpr0;
+};
+
+static void imx_aipstz_apply_default(void __iomem *base,
+				     const struct imx_aipstz_config *default_cfg)
+{
+	writel(default_cfg->mpr0, base + IMX_AIPSTZ_MPR0);
+}
+
+static int imx_aipstz_probe(struct platform_device *pdev)
+{
+	const struct imx_aipstz_config *default_cfg;
+	void __iomem *base;
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(base))
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "failed to get/ioremap memory\n");
+
+	default_cfg = of_device_get_match_data(&pdev->dev);
+
+	imx_aipstz_apply_default(base, default_cfg);
+
+	dev_set_drvdata(&pdev->dev, base);
+
+	pm_runtime_set_active(&pdev->dev);
+	devm_pm_runtime_enable(&pdev->dev);
+
+	return devm_of_platform_populate(&pdev->dev);
+}
+
+static int imx_aipstz_runtime_resume(struct device *dev)
+{
+	const struct imx_aipstz_config *default_cfg;
+	void __iomem *base;
+
+	base = dev_get_drvdata(dev);
+	default_cfg = of_device_get_match_data(dev);
+
+	/* restore potentially lost configuration during domain power-off */
+	imx_aipstz_apply_default(base, default_cfg);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx_aipstz_pm_ops = {
+	RUNTIME_PM_OPS(NULL, imx_aipstz_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
+/*
+ * following configuration is equivalent to:
+ *	masters 0-7 => trusted for R/W + use AHB's HPROT[1] to det. privilege
+ */
+static const struct imx_aipstz_config imx8mp_aipstz_default_cfg = {
+	.mpr0 = 0x77777777,
+};
+
+static const struct of_device_id imx_aipstz_of_ids[] = {
+	{ .compatible = "fsl,imx8mp-aipstz", .data = &imx8mp_aipstz_default_cfg },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, imx_aipstz_of_ids);
+
+static struct platform_driver imx_aipstz_of_driver = {
+	.probe = imx_aipstz_probe,
+	.driver = {
+		.name = "imx-aipstz",
+		.of_match_table = imx_aipstz_of_ids,
+		.pm = pm_ptr(&imx_aipstz_pm_ops),
+	},
+};
+module_platform_driver(imx_aipstz_of_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("IMX secure AHB to IP Slave bus (AIPSTZ) bridge driver");
+MODULE_AUTHOR("Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>");
-- 
2.34.1


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

* [PATCH v2 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'
  2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
                   ` (2 preceding siblings ...)
  2025-02-26 16:53 ` [PATCH v2 3/5] bus: add driver for IMX AIPSTZ bridge Laurentiu Mihalcea
@ 2025-02-26 16:53 ` Laurentiu Mihalcea
  2025-02-26 16:53 ` [PATCH v2 5/5] arm64: dts: imx8mp: make 'dsp' node depend on 'aips5' Laurentiu Mihalcea
  2025-02-26 21:22 ` [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Marco Felsch
  5 siblings, 0 replies; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-02-26 16:53 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

AIPS5 is actually AIPSTZ5 as it offers some security-related
configurations. Since these configurations need to be applied before
accessing any of the peripherals on the bus, it's better to make AIPSTZ5
be their parent instead of keeping AIPS5 and adding a child node for
AIPSTZ5. Also, because of the security configurations, the address space
of the bus has to be changed to that of the configuration registers.

Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the
missing 'power-domains' property. The domain needs to be powered on before
attempting to configure the security-related registers.

The DT node name is not changed to avoid potential issues with DTs in
which this node is referenced.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index e0d3b8cba221..3097acb4bd21 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
 			};
 		};
 
-		aips5: bus@30c00000 {
-			compatible = "fsl,aips-bus", "simple-bus";
-			reg = <0x30c00000 0x400000>;
+		aips5: bus@30df0000 {
+			compatible = "fsl,imx8mp-aipstz";
+			reg = <0x30df0000 0x10000>;
+			power-domains = <&pgc_audio>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#access-controller-cells = <0>;
 			ranges;
 
 			spba-bus@30c00000 {
-- 
2.34.1


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

* [PATCH v2 5/5] arm64: dts: imx8mp: make 'dsp' node depend on 'aips5'
  2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
                   ` (3 preceding siblings ...)
  2025-02-26 16:53 ` [PATCH v2 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5' Laurentiu Mihalcea
@ 2025-02-26 16:53 ` Laurentiu Mihalcea
  2025-02-26 21:22 ` [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Marco Felsch
  5 siblings, 0 replies; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-02-26 16:53 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The DSP needs to access peripherals on AIPSTZ5 (to communicate with
the AP using AUDIOMIX MU, for instance). To do so, the security-related
registers of the bridge have to be configured before the DSP is started.
Enforce a dependency on AIPSTZ5 by adding the 'access-controllers'
property to the 'dsp' node.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 3097acb4bd21..3550e44d00b5 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -2423,6 +2423,7 @@ dsp: dsp@3b6e8000 {
 			mboxes = <&mu2 2 0>, <&mu2 2 1>,
 				<&mu2 3 0>, <&mu2 3 1>;
 			memory-region = <&dsp_reserved>;
+			access-controllers = <&aips5>;
 			status = "disabled";
 		};
 	};
-- 
2.34.1


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

* Re: [PATCH v2 1/5] dt-bindings: bus: add documentation for the IMX AIPSTZ bridge
  2025-02-26 16:53 ` [PATCH v2 1/5] dt-bindings: bus: add documentation " Laurentiu Mihalcea
@ 2025-02-26 21:16   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-26 21:16 UTC (permalink / raw)
  To: Laurentiu Mihalcea, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Daniel Baluta, Shengjiu Wang, Frank Li
  Cc: Pengutronix Kernel Team, imx, linux-arm-kernel, linux-kernel

On 26/02/2025 17:53, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> 
> Add documentation for IMX AIPSTZ bridge.
> 

Please use get_maintainers so this will reach proper lists and
automation. Or just start using b4, so you won't remove cc-entries (v1
looked correct).


> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 86 +++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
> 
> diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
> new file mode 100644
> index 000000000000..dfcfe4a8ae74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bus/fsl,imx8mp-aipstz.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Secure AHB to IP Slave bus (AIPSTZ) bridge
> +
> +description:
> +  The secure AIPS bridge (AIPSTZ) acts as a bridge for AHB masters
> +  issuing transactions to IP Slave peripherals. Additionally, this module
> +  offers access control configurations meant to restrict which peripherals
> +  a master can access.
> +
> +maintainers:
> +  - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> +
> +properties:
> +  compatible:
> +    const: fsl,imx8mp-aipstz
> +
> +  reg:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    enum: [1, 2]
> +
> +  "#size-cells":
> +    enum: [1, 2]

Why flexible? This is either 32- or 64-bit addressing in the sources,
isn't it?

> +
> +  "#access-controller-cells":
> +    const: 0
> +
> +  ranges: true
> +
> +# borrowed from simple-bus.yaml, no additional requirements for children
> +patternProperties:
> +  "@(0|[1-9a-f][0-9a-f]*)$":
> +    type: object
> +    additionalProperties: true
> +    properties:
> +      reg:
> +        items:
> +          minItems: 2
> +          maxItems: 4
> +        minItems: 1
> +        maxItems: 1024
> +      ranges:
> +        oneOf:
> +          - items:
> +              minItems: 3
> +              maxItems: 7
> +            minItems: 1
> +            maxItems: 1024
> +          - $ref: /schemas/types.yaml#/definitions/flag
> +    anyOf:
> +      - required:
> +          - reg
> +      - required:
> +          - ranges
> +
> +required:
> +  - compatible
> +  - reg
> +  - power-domains
> +  - "#address-cells"
> +  - "#size-cells"
> +  - "#access-controller-cells"
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bus@30df0000 {
> +      compatible = "fsl,imx8mp-aipstz";
> +      reg = <0x30df0000 0x10000>;
> +      power-domains = <&pgc_audio>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      #access-controller-cells = <0>;
> +      ranges;

Add at least one child, so your pattern will be tested.



Best regards,
Krzysztof

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

* Re: [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
  2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
                   ` (4 preceding siblings ...)
  2025-02-26 16:53 ` [PATCH v2 5/5] arm64: dts: imx8mp: make 'dsp' node depend on 'aips5' Laurentiu Mihalcea
@ 2025-02-26 21:22 ` Marco Felsch
  2025-02-27 11:28   ` Marco Felsch
  5 siblings, 1 reply; 13+ messages in thread
From: Marco Felsch @ 2025-02-26 21:22 UTC (permalink / raw)
  To: Laurentiu Mihalcea
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li, imx, linux-arm-kernel, Pengutronix Kernel Team,
	linux-kernel

Hi,

On 25-02-26, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> 
> The AIPSTZ bridge offers some security-related configurations which can
> be used to restrict master access to certain peripherals on the bridge.
> 
> Normally, this could be done from a secure environment such as ATF before
> Linux boots but the configuration of AIPSTZ5 is lost each time the power
> domain is powered off and then powered on. Because of this, it has to be
> configured each time the power domain is turned on and before any master
> tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).

My question still stands:

Setting these bits requires very often that the core is running at EL3
(e.g. secure-monitor) which is not the case for Linux. Can you please
provide more information how Linux can set these bits?

Regards,
  Marco

> The child-parent relationship between the bridge and its peripherals
> should guarantee that the bridge is configured before the AP attempts
> to access the IPs.
> 
> Other masters should use the 'access-controllers' property to enforce
> a dependency between their device and the bridge device (see the DSP,
> for example).
> 
> At the moment, we only want to apply a default, more relaxed
> configuration, which is why the number of access controller cells
> is 0.
> 
> The initial version of the series can be found at [1]. The new version
> should provide better management of the device dependencies.
> 
> [1]: https://lore.kernel.org/linux-arm-kernel/20241119130726.2761726-1-daniel.baluta@nxp.com/
> 
> ---
> Changes in v2:
> * adress Frank Li's comments
> * pick up some A-b/R-b's
> * don't use "simple-bus" as the second compatible. As per Krzysztof's
> comment, AIPSTZ is not a "simple-bus".
> ---
> 
> Laurentiu Mihalcea (5):
>   dt-bindings: bus: add documentation for the IMX AIPSTZ bridge
>   dt-bindings: dsp: fsl,dsp: document 'access-controllers' property
>   bus: add driver for IMX AIPSTZ bridge
>   arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'
>   arm64: dts: imx8mp: make 'dsp' node depend on 'aips5'
> 
>  .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 86 +++++++++++++++++
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      |  3 +
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi     |  9 +-
>  drivers/bus/Kconfig                           |  6 ++
>  drivers/bus/Makefile                          |  1 +
>  drivers/bus/imx-aipstz.c                      | 92 +++++++++++++++++++
>  6 files changed, 194 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
>  create mode 100644 drivers/bus/imx-aipstz.c
> 
> -- 
> 2.34.1
> 
> 
> 

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

* Re: [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
  2025-02-26 21:22 ` [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Marco Felsch
@ 2025-02-27 11:28   ` Marco Felsch
  2025-03-04 22:58     ` Laurentiu Mihalcea
  0 siblings, 1 reply; 13+ messages in thread
From: Marco Felsch @ 2025-02-27 11:28 UTC (permalink / raw)
  To: Laurentiu Mihalcea
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li, imx, linux-arm-kernel, Pengutronix Kernel Team,
	linux-kernel

Hi Laurentiu,

On 25-02-26, Marco Felsch wrote:
> Hi,
> 
> On 25-02-26, Laurentiu Mihalcea wrote:
> > From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> > 
> > The AIPSTZ bridge offers some security-related configurations which can
> > be used to restrict master access to certain peripherals on the bridge.
> > 
> > Normally, this could be done from a secure environment such as ATF before
> > Linux boots but the configuration of AIPSTZ5 is lost each time the power
> > domain is powered off and then powered on. Because of this, it has to be
> > configured each time the power domain is turned on and before any master
> > tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).
> 
> My question still stands:
> 
> Setting these bits requires very often that the core is running at EL3
> (e.g. secure-monitor) which is not the case for Linux. Can you please
> provide more information how Linux can set these bits?

Sorry I didn't noticed your response:

https://lore.kernel.org/all/a62ab860-5e0e-4ebc-af1f-6fb7ac621e2b@gmail.com/

If EL1 is allowed to set the security access configuration of the IP
cores doesn't this mean that a backdoor can be opened? E.g. your
secure-boot system configures one I2C IP core to be accessible only from
secure-world S-EL1 (OP-TEE) and after the power-domain was power-cycled
it's accessible from EL1 again. This doesn't seem right. Why should a
user be able to limit the access permissions to an IP core to only be
accessible from secure-world if the IP core is accessible from
normal-world after the power-domain was power-cycled.

Regards,
  Marco

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

* Re: [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
  2025-02-27 11:28   ` Marco Felsch
@ 2025-03-04 22:58     ` Laurentiu Mihalcea
  2025-03-07 14:39       ` Marco Felsch
  0 siblings, 1 reply; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-03-04 22:58 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li, imx, linux-arm-kernel, Pengutronix Kernel Team,
	linux-kernel



On 2/27/2025 1:28 PM, Marco Felsch wrote:
> Hi Laurentiu,
>
> On 25-02-26, Marco Felsch wrote:
>> Hi,
>>
>> On 25-02-26, Laurentiu Mihalcea wrote:
>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>>
>>> The AIPSTZ bridge offers some security-related configurations which can
>>> be used to restrict master access to certain peripherals on the bridge.
>>>
>>> Normally, this could be done from a secure environment such as ATF before
>>> Linux boots but the configuration of AIPSTZ5 is lost each time the power
>>> domain is powered off and then powered on. Because of this, it has to be
>>> configured each time the power domain is turned on and before any master
>>> tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).
>> My question still stands:
>>
>> Setting these bits requires very often that the core is running at EL3
>> (e.g. secure-monitor) which is not the case for Linux. Can you please
>> provide more information how Linux can set these bits?
> Sorry I didn't noticed your response:
>
> https://lore.kernel.org/all/a62ab860-5e0e-4ebc-af1f-6fb7ac621e2b@gmail.com/
>
> If EL1 is allowed to set the security access configuration of the IP
> cores doesn't this mean that a backdoor can be opened? E.g. your
> secure-boot system configures one I2C IP core to be accessible only from
> secure-world S-EL1 (OP-TEE) and after the power-domain was power-cycled
> it's accessible from EL1 again. This doesn't seem right. Why should a
> user be able to limit the access permissions to an IP core to only be
> accessible from secure-world if the IP core is accessible from
> normal-world after the power-domain was power-cycled.
>
> Regards,
>   Marco

I'm no security expert so please feel free to correct me if I get something wrong.

This isn't about S/NS world. The bridge AC doesn't offer any configurations for
denying access to peripherals based on S/NS world. AFAIK that's the job of the CSU
(central security unit), which is a different IP.

Perhaps I shouldn't have used the term "trusted" as it might have ended up creating
more confusion? If so, please do let me know so I can maybe add a comment about
it in one of the commit messages. In this context, "master X is trusted for read/writes"
means "master X is allowed to perform read/write transactions".

Even if the bridge is configured to allow read/write transactions from a master
(i.e: master is marked as trusted for read/writes) that wouldn't be very helpful.
You'd still have to bypass the CSU configuration which as far as I understand is also
used by the bridge to deny access to peripherals (e.g: if transaction is secure+privileged
then forward to peripheral, otherwise abort it). See the "4.7.6.1 Security Block"
and "4.7.4  Access Protections" chapters from the IMX8MP RM.

Given all of this, I think the purpose of this IP's AC is to add some extra,
light, security features on top of the CSU.

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

* Re: [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
  2025-03-04 22:58     ` Laurentiu Mihalcea
@ 2025-03-07 14:39       ` Marco Felsch
  2025-03-10 20:06         ` Laurentiu Mihalcea
  0 siblings, 1 reply; 13+ messages in thread
From: Marco Felsch @ 2025-03-07 14:39 UTC (permalink / raw)
  To: Laurentiu Mihalcea
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li, imx, linux-arm-kernel, Pengutronix Kernel Team,
	linux-kernel

On 25-03-05, Laurentiu Mihalcea wrote:
> 
> On 2/27/2025 1:28 PM, Marco Felsch wrote:
> > Hi Laurentiu,
> >
> > On 25-02-26, Marco Felsch wrote:
> >> Hi,
> >>
> >> On 25-02-26, Laurentiu Mihalcea wrote:
> >>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >>>
> >>> The AIPSTZ bridge offers some security-related configurations which can
> >>> be used to restrict master access to certain peripherals on the bridge.
> >>>
> >>> Normally, this could be done from a secure environment such as ATF before
> >>> Linux boots but the configuration of AIPSTZ5 is lost each time the power
> >>> domain is powered off and then powered on. Because of this, it has to be
> >>> configured each time the power domain is turned on and before any master
> >>> tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).
> >> My question still stands:
> >>
> >> Setting these bits requires very often that the core is running at EL3
> >> (e.g. secure-monitor) which is not the case for Linux. Can you please
> >> provide more information how Linux can set these bits?
> > Sorry I didn't noticed your response:
> >
> > https://lore.kernel.org/all/a62ab860-5e0e-4ebc-af1f-6fb7ac621e2b@gmail.com/
> >
> > If EL1 is allowed to set the security access configuration of the IP
> > cores doesn't this mean that a backdoor can be opened? E.g. your
> > secure-boot system configures one I2C IP core to be accessible only from
> > secure-world S-EL1 (OP-TEE) and after the power-domain was power-cycled
> > it's accessible from EL1 again. This doesn't seem right. Why should a
> > user be able to limit the access permissions to an IP core to only be
> > accessible from secure-world if the IP core is accessible from
> > normal-world after the power-domain was power-cycled.
> >
> > Regards,
> >   Marco
> 
> I'm no security expert so please feel free to correct me if I get
> something wrong.
> 
> This isn't about S/NS world. The bridge AC doesn't offer any
> configurations for denying access to peripherals based on S/NS world.

It does, please see the AIPSTZ_OPACR register definition. The imx-atf of
sets OPACR registers to 0 (of course), which means that the S/NS is not
checked _but_ it can be configured.

Also please see chapter 4.7.6.1 Security Block:

The AIPSTZ contains a security block that is connected to each
off-platform peripheral. This block filters accesses based on
write/read, non-secure, and supervisor signals.

> AFAIK that's the job of the CSU (central security unit), which is a
> different IP.

Please see above.

> Perhaps I shouldn't have used the term "trusted" as it might have
> ended up creating more confusion? If so, please do let me know so I
> can maybe add a comment about it in one of the commit messages. In
> this context, "master X is trusted for read/writes" means "master X is
> allowed to perform read/write transactions".

No you didn't confused me but you triggered my interest :) and I started
to check the (S)TRM.

> Even if the bridge is configured to allow read/write transactions from
> a master (i.e: master is marked as trusted for read/writes) that
> wouldn't be very helpful.

We're talking about the IP access permissions, right. If the
"secure-I2C" is accessible from NS world this would make a difference of
course.

> You'd still have to bypass the CSU configuration which as far as I
> understand is also used by the bridge to deny access to peripherals
> (e.g: if transaction is secure+privileged then forward to peripheral,
> otherwise abort it). See the "4.7.6.1 Security Block" and "4.7.4 
> Access Protections" chapters from the IMX8MP RM.

I have read this too, also that the AIPSTZ can force the mode into
user_mode regardless of the CSU settings, if I get this correct.

What I don't understand as of now is the interaction of the AIPSTZ and
the CSU. You can configure different bus-masters within the CSU to be
S/NS as well as the pheripherals. Now the part which I don't understand
right now: According the OPACx register description:

x0xx SP0 — This peripheral does not require supervisor privilege level
           for accesses.
x1xx SP1 — This peripheral requires supervisor privilege level for
           accesses. The master privilege level must indicate supervisor
	   via the hprot[1] access attribute, and the MPROTx[MPL] control
	   bit for the master must be set. If not, the access is
	   terminated with an error response and no peripheral access is
	   initiated on the IPS bus.

The peripheral can be configured via the AIPSTZ as well. So which IP
(CSU or AIPSTZ) override the other if the settings don't match, e.g. if
CSU says: "this I2C controller for secure-world" and the AIPSTZ says:
"this I2C is for non-secure-world".

> Given all of this, I think the purpose of this IP's AC is to add some
> extra, light, security features on top of the CSU.

Or to override the CSU settings like the MPROTOx values:

xxx0 MPL0 — Accesses from this master are forced to user-mode
           (ips_supervisor_access is forced to zero) regardless of the
	   hprot[1] access attribute.
xxx1 MPL1 — Accesses from this master are not forced to user-mode. The
            hprot[1] access attribute is used directly to determine
	    ips_supervisor_access.

Can you pleae elaborate a bit more how NXP designed the interaction
between both the AIPSTZ and the CSU?

Regards,
  Marco

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

* Re: [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
  2025-03-07 14:39       ` Marco Felsch
@ 2025-03-10 20:06         ` Laurentiu Mihalcea
  2025-03-11 11:37           ` Marco Felsch
  0 siblings, 1 reply; 13+ messages in thread
From: Laurentiu Mihalcea @ 2025-03-10 20:06 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li, imx, linux-arm-kernel, Pengutronix Kernel Team,
	linux-kernel



On 3/7/2025 4:39 PM, Marco Felsch wrote:
> On 25-03-05, Laurentiu Mihalcea wrote:
>> On 2/27/2025 1:28 PM, Marco Felsch wrote:
>>> Hi Laurentiu,
>>>
>>> On 25-02-26, Marco Felsch wrote:
>>>> Hi,
>>>>
>>>> On 25-02-26, Laurentiu Mihalcea wrote:
>>>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>>>>
>>>>> The AIPSTZ bridge offers some security-related configurations which can
>>>>> be used to restrict master access to certain peripherals on the bridge.
>>>>>
>>>>> Normally, this could be done from a secure environment such as ATF before
>>>>> Linux boots but the configuration of AIPSTZ5 is lost each time the power
>>>>> domain is powered off and then powered on. Because of this, it has to be
>>>>> configured each time the power domain is turned on and before any master
>>>>> tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).
>>>> My question still stands:
>>>>
>>>> Setting these bits requires very often that the core is running at EL3
>>>> (e.g. secure-monitor) which is not the case for Linux. Can you please
>>>> provide more information how Linux can set these bits?
>>> Sorry I didn't noticed your response:
>>>
>>> https://lore.kernel.org/all/a62ab860-5e0e-4ebc-af1f-6fb7ac621e2b@gmail.com/
>>>
>>> If EL1 is allowed to set the security access configuration of the IP
>>> cores doesn't this mean that a backdoor can be opened? E.g. your
>>> secure-boot system configures one I2C IP core to be accessible only from
>>> secure-world S-EL1 (OP-TEE) and after the power-domain was power-cycled
>>> it's accessible from EL1 again. This doesn't seem right. Why should a
>>> user be able to limit the access permissions to an IP core to only be
>>> accessible from secure-world if the IP core is accessible from
>>> normal-world after the power-domain was power-cycled.
>>>
>>> Regards,
>>>   Marco
>> I'm no security expert so please feel free to correct me if I get
>> something wrong.
>>
>> This isn't about S/NS world. The bridge AC doesn't offer any
>> configurations for denying access to peripherals based on S/NS world.
> It does, please see the AIPSTZ_OPACR register definition. The imx-atf of
> sets OPACR registers to 0 (of course), which means that the S/NS is not
> checked _but_ it can be configured.

which bits are you referring to more precisely? because, from the OPACR
register definition we have:

1) TP (Trusted protect) - bit 0 => controls whether the peripheral allows
transactions from an untrusted master. A master is considered trusted if
MTR/MTW (from MPR registers) is set to 1 (MTR means trusted for read,
MTW means trusted for write)

2) WP (Write protect) - bit 1 => controls whether the peripherals allows
write transactions (i.e: is write protected or not)

3) SP (Supervisor protect) - bit 2 => controls whether the master needs
supervisor privilege or not to issue transactions to the peripheral. For
Cortex-A53 this refers to the execution level (EL0 - EL3). There's no S/NS
checks here. EL1-EL3 are supervisor accesses, EL0 is not.

4) BW (Buffer Writes) - bit 3 => some flow control configuration bit I'd assume


>
> Also please see chapter 4.7.6.1 Security Block:
>
> The AIPSTZ contains a security block that is connected to each
> off-platform peripheral. This block filters accesses based on
> write/read, non-secure, and supervisor signals.

yep, but this block is not configured by the AIPSTZ. AFAIK it's configured
by the CSU. So, as far as I understand it, the interaction is as follows:

basically you have the CSU which offers some security-related configurations
(see "Table 4-16 Security Levels" from the section you've mentioned). These
configurations are used by the AIPS bridges to filter transactions.

For example: assume you have peripheral X on AIPS5. The user configures
the CSU such that peripheral X should only accept R/W transactions from
privileged S world. Now, assume Cortex-A53 issues a transaction from
NS EL1 (Linux, for example). The bridge will receive the transaction and
check to see if it's privileged and S world. Since it's not then the transaction
will be aborted.

The AIPS bridge offers no S/NS world-related configuration options. All you can
do with it is:

1) Mark certain masters as "trusted" and block read/writes based on that (via MPR's
MTR/MTW and OPACR's TP)
2) Force transactions to user privilege (via MPR's MPL)
3) Make certain peripherals deny unprivileged transactions (via OPACR's SP)

>
>> AFAIK that's the job of the CSU (central security unit), which is a
>> different IP.
> Please see above.
>
>> Perhaps I shouldn't have used the term "trusted" as it might have
>> ended up creating more confusion? If so, please do let me know so I
>> can maybe add a comment about it in one of the commit messages. In
>> this context, "master X is trusted for read/writes" means "master X is
>> allowed to perform read/write transactions".
> No you didn't confused me but you triggered my interest :) and I started
> to check the (S)TRM.

glad to have picked your interest with this series!

>
>> Even if the bridge is configured to allow read/write transactions from
>> a master (i.e: master is marked as trusted for read/writes) that
>> wouldn't be very helpful.
> We're talking about the IP access permissions, right. If the
> "secure-I2C" is accessible from NS world this would make a difference of
> course.

It wouldn't though. No configuration in the AIPS bridge AC will make the I2C
accesible from NS world.

>
>> You'd still have to bypass the CSU configuration which as far as I
>> understand is also used by the bridge to deny access to peripherals
>> (e.g: if transaction is secure+privileged then forward to peripheral,
>> otherwise abort it). See the "4.7.6.1 Security Block" and "4.7.4 
>> Access Protections" chapters from the IMX8MP RM.
> I have read this too, also that the AIPSTZ can force the mode into
> user_mode regardless of the CSU settings, if I get this correct.
>
> What I don't understand as of now is the interaction of the AIPSTZ and
> the CSU. You can configure different bus-masters within the CSU to be
> S/NS as well as the pheripherals. Now the part which I don't understand
> right now: According the OPACx register description:
>
> x0xx SP0 — This peripheral does not require supervisor privilege level
>            for accesses.
> x1xx SP1 — This peripheral requires supervisor privilege level for
>            accesses. The master privilege level must indicate supervisor
> 	   via the hprot[1] access attribute, and the MPROTx[MPL] control
> 	   bit for the master must be set. If not, the access is
> 	   terminated with an error response and no peripheral access is
> 	   initiated on the IPS bus.
>
> The peripheral can be configured via the AIPSTZ as well. So which IP
> (CSU or AIPSTZ) override the other if the settings don't match, e.g. if
> CSU says: "this I2C controller for secure-world" and the AIPSTZ says:
> "this I2C is for non-secure-world".

the SP bit you've mentioned is used to deny unprivileged transactions.
The privilege is given by the execution level for CA53. EL0 transactions are
unprivileged, EL1-EL3 are privileged, but this can depend based on the type
of access. See Table 7-10 "Cortex-A53 MPCore mode and ARPROT and AWPROT
values" from ARM's Cortex-A53 MPCore Processor Technical Reference Manual
for details.

the bit has nothing to do with S/NS world so there's no overriding the
S/NS world-related policy given by the CSU.

>
>> Given all of this, I think the purpose of this IP's AC is to add some
>> extra, light, security features on top of the CSU.
> Or to override the CSU settings like the MPROTOx values:
>
> xxx0 MPL0 — Accesses from this master are forced to user-mode
>            (ips_supervisor_access is forced to zero) regardless of the
> 	   hprot[1] access attribute.
> xxx1 MPL1 — Accesses from this master are not forced to user-mode. The
>             hprot[1] access attribute is used directly to determine
> 	    ips_supervisor_access.
>
> Can you pleae elaborate a bit more how NXP designed the interaction
> between both the AIPSTZ and the CSU?

I believe this will override the privilege level of the transaction at the
bridge level. This could be useful if you want to, for instance, force
transactions for peripherals under AIPS5 to user but use HPROT[1]
(or the value you've configured via the CSU) for peripherals under AIPS4.
Not sure of an usecase in which you'd want that though.

anyways, hope my comments will shed a bit more light on this.

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

* Re: [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge
  2025-03-10 20:06         ` Laurentiu Mihalcea
@ 2025-03-11 11:37           ` Marco Felsch
  0 siblings, 0 replies; 13+ messages in thread
From: Marco Felsch @ 2025-03-11 11:37 UTC (permalink / raw)
  To: Laurentiu Mihalcea
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Fabio Estevam, Daniel Baluta, Shengjiu Wang,
	Frank Li, imx, linux-arm-kernel, Pengutronix Kernel Team,
	linux-kernel

On 25-03-10, Laurentiu Mihalcea wrote:
> 
> 
> On 3/7/2025 4:39 PM, Marco Felsch wrote:
> > On 25-03-05, Laurentiu Mihalcea wrote:
> >> On 2/27/2025 1:28 PM, Marco Felsch wrote:
> >>> Hi Laurentiu,
> >>>
> >>> On 25-02-26, Marco Felsch wrote:
> >>>> Hi,
> >>>>
> >>>> On 25-02-26, Laurentiu Mihalcea wrote:
> >>>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >>>>>
> >>>>> The AIPSTZ bridge offers some security-related configurations which can
> >>>>> be used to restrict master access to certain peripherals on the bridge.
> >>>>>
> >>>>> Normally, this could be done from a secure environment such as ATF before
> >>>>> Linux boots but the configuration of AIPSTZ5 is lost each time the power
> >>>>> domain is powered off and then powered on. Because of this, it has to be
> >>>>> configured each time the power domain is turned on and before any master
> >>>>> tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).
> >>>> My question still stands:
> >>>>
> >>>> Setting these bits requires very often that the core is running at EL3
> >>>> (e.g. secure-monitor) which is not the case for Linux. Can you please
> >>>> provide more information how Linux can set these bits?
> >>> Sorry I didn't noticed your response:
> >>>
> >>> https://lore.kernel.org/all/a62ab860-5e0e-4ebc-af1f-6fb7ac621e2b@gmail.com/
> >>>
> >>> If EL1 is allowed to set the security access configuration of the IP
> >>> cores doesn't this mean that a backdoor can be opened? E.g. your
> >>> secure-boot system configures one I2C IP core to be accessible only from
> >>> secure-world S-EL1 (OP-TEE) and after the power-domain was power-cycled
> >>> it's accessible from EL1 again. This doesn't seem right. Why should a
> >>> user be able to limit the access permissions to an IP core to only be
> >>> accessible from secure-world if the IP core is accessible from
> >>> normal-world after the power-domain was power-cycled.
> >>>
> >>> Regards,
> >>>   Marco
> >> I'm no security expert so please feel free to correct me if I get
> >> something wrong.
> >>
> >> This isn't about S/NS world. The bridge AC doesn't offer any
> >> configurations for denying access to peripherals based on S/NS world.
> > It does, please see the AIPSTZ_OPACR register definition. The imx-atf of
> > sets OPACR registers to 0 (of course), which means that the S/NS is not
> > checked _but_ it can be configured.
> 
> which bits are you referring to more precisely? because, from the OPACR
> register definition we have:
> 
> 1) TP (Trusted protect) - bit 0 => controls whether the peripheral allows
> transactions from an untrusted master. A master is considered trusted if
> MTR/MTW (from MPR registers) is set to 1 (MTR means trusted for read,
> MTW means trusted for write)
> 
> 2) WP (Write protect) - bit 1 => controls whether the peripherals allows
> write transactions (i.e: is write protected or not)
> 
> 3) SP (Supervisor protect) - bit 2 => controls whether the master needs
> supervisor privilege or not to issue transactions to the peripheral. For
> Cortex-A53 this refers to the execution level (EL0 - EL3). There's no S/NS
> checks here. EL1-EL3 are supervisor accesses, EL0 is not.

Argh.. my head mixed the supervisor phrase :/

> 4) BW (Buffer Writes) - bit 3 => some flow control configuration bit I'd assume
> 
> 
> >
> > Also please see chapter 4.7.6.1 Security Block:
> >
> > The AIPSTZ contains a security block that is connected to each
> > off-platform peripheral. This block filters accesses based on
> > write/read, non-secure, and supervisor signals.
> 
> yep, but this block is not configured by the AIPSTZ. AFAIK it's configured
> by the CSU. So, as far as I understand it, the interaction is as follows:
> 
> basically you have the CSU which offers some security-related configurations
> (see "Table 4-16 Security Levels" from the section you've mentioned). These
> configurations are used by the AIPS bridges to filter transactions.
> 
> For example: assume you have peripheral X on AIPS5. The user configures
> the CSU such that peripheral X should only accept R/W transactions from
> privileged S world. Now, assume Cortex-A53 issues a transaction from
> NS EL1 (Linux, for example). The bridge will receive the transaction and
> check to see if it's privileged and S world. Since it's not then the transaction
> will be aborted.
> 
> The AIPS bridge offers no S/NS world-related configuration options. All you can
> do with it is:
> 
> 1) Mark certain masters as "trusted" and block read/writes based on that (via MPR's
> MTR/MTW and OPACR's TP)
> 2) Force transactions to user privilege (via MPR's MPL)
> 3) Make certain peripherals deny unprivileged transactions (via OPACR's SP)

Okay, thanks for the explanation with your input and the the TRM 4.7.6.1
saying:

8<--------------------------------------------------------------
A 3-bit input, 8-bit output translation block can be used such that only
three register bits are required to set the security profile and the
translation block will drive the correct 8-bit configuration vector.
Each peripheral connected to the AIPSTZ would require this translation
block. The top level AIPSTZ has this three bit input line
`csu_sec_level[2:0]' corresponding to each peripheral X.
8<--------------------------------------------------------------

I think that I understood the AIPSTZ user/supervisor now. The master
user/supervisor privileges are provided by the CSU HPx settings and can
be forced to user-mode via the AIPSTZ MPRTOx MPL0 bit.

The remaining bits from the config vector between the CSU and the AIPSTZ
are not taken into account.

Thanks for the clarification!

Regards,
  Marco

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

end of thread, other threads:[~2025-03-11 11:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-26 16:53 [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Laurentiu Mihalcea
2025-02-26 16:53 ` [PATCH v2 1/5] dt-bindings: bus: add documentation " Laurentiu Mihalcea
2025-02-26 21:16   ` Krzysztof Kozlowski
2025-02-26 16:53 ` [PATCH v2 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property Laurentiu Mihalcea
2025-02-26 16:53 ` [PATCH v2 3/5] bus: add driver for IMX AIPSTZ bridge Laurentiu Mihalcea
2025-02-26 16:53 ` [PATCH v2 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5' Laurentiu Mihalcea
2025-02-26 16:53 ` [PATCH v2 5/5] arm64: dts: imx8mp: make 'dsp' node depend on 'aips5' Laurentiu Mihalcea
2025-02-26 21:22 ` [PATCH v2 0/5] imx8mp: add support for the IMX AIPSTZ bridge Marco Felsch
2025-02-27 11:28   ` Marco Felsch
2025-03-04 22:58     ` Laurentiu Mihalcea
2025-03-07 14:39       ` Marco Felsch
2025-03-10 20:06         ` Laurentiu Mihalcea
2025-03-11 11:37           ` Marco Felsch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).