mbox series

[00/10] Introduce STM32 Firewall framework

Message ID 20230705172759.1610753-1-gatien.chevallier@foss.st.com
Headers show
Series Introduce STM32 Firewall framework | expand

Message

Gatien CHEVALLIER July 5, 2023, 5:27 p.m. UTC
Introduce STM32 Firewall framework for STM32MP1x and STM32MP2x
platforms. STM32MP1x(ETZPC) and STM32MP2x(RIFSC) Firewall controllers
register to the framework to offer firewall services such as access
granting.

This series of patches is a new approach on the previous STM32 system
bus, history is available here:
https://lore.kernel.org/lkml/20230127164040.1047583/

The need for such framework arises from the fact that there are now
multiple hardware firewalls implemented across multiple products.
Drivers are shared between different products, using the same code.
When it comes to firewalls, the purpose mostly stays the same: Protect
hardware resources. But the implementation differs, and there are
multiple types of firewalls: peripheral, memory, ... 

Some hardware firewall controllers such as the RIFSC implemented on
STM32MP2x platforms may require to take ownership of a resource before
being able to use it, hence the requirement for firewall services to
take/release the ownership of such resources.

On the other hand, hardware firewall configurations are becoming
more and more complex. These mecanisms prevent platform crashes
or other firewall-related incoveniences by denying access to some
resources.

The stm32 firewall framework offers an API that is defined in
firewall controllers drivers to best fit the specificity of each
firewall.

For every peripherals protected by either the ETZPC or the RIFSC, the
firewall framework checks the firewall controlelr registers to see if
the peripheral's access is granted to the Linux kernel. If not, the
peripheral is configured as secure, the node is marked populated,
so that the driver is not probed for that device.

The firewall framework relies on the feature-domain-controller device
tree bindings: https://lore.kernel.org/lkml/0c0a82bb-18ae-d057-562b.
It is used by peripherals to reference a domain controller, in this
case a firewall feature domain. The bus uses the ID referenced by
the feature-domains property to know where to look in the firewall
to get the security configuration for the peripheral. This allows
a device tree description rather than a hardcoded peripheral table
in the bus driver.

The STM32 ETZPC device is responsible for filtering accesses based on
security level, or co-processor isolation for any resource connected
to it.

The RIFSC is responsible for filtering accesses based on Compartment
ID / security level / privilege level for any resource connected to
it.

STM32MP13/15/25 SoC device tree files are updated in this series to
implement this mecanism.

Oleksii Moisieiev (1):
  dt-bindings: Document common device controller bindings

Gatien Chevallier (9):
  dt-bindings: bus: add device tree bindings for RIFSC
  dt-bindings: bus: add device tree bindings for ETZPC
  dt-bindings: treewide: add feature-domains description in binding
    files
  firewall: introduce stm32_firewall framework
  bus: rifsc: introduce RIFSC firewall controller driver
  arm64: dts: st: add RIFSC as a domain controller for STM32MP25x boards
  bus: etzpc: introduce ETZPC firewall controller driver
  ARM: dts: stm32: add ETZPC as a system bus for STM32MP15x boards
  ARM: dts: stm32: add ETZPC as a system bus for STM32MP13x boards

 .../bindings/bus/st,stm32-etzpc.yaml          |   90 +
 .../bindings/bus/st,stm32-rifsc.yaml          |  101 +
 .../bindings/crypto/st,stm32-hash.yaml        |    4 +
 .../devicetree/bindings/dma/st,stm32-dma.yaml |    4 +
 .../bindings/dma/st,stm32-dmamux.yaml         |    4 +
 .../feature-domain-controller.yaml            |   84 +
 .../devicetree/bindings/i2c/st,stm32-i2c.yaml |    4 +
 .../bindings/iio/adc/st,stm32-adc.yaml        |    4 +
 .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml  |    4 +
 .../bindings/iio/dac/st,stm32-dac.yaml        |    4 +
 .../bindings/media/cec/st,stm32-cec.yaml      |    4 +
 .../bindings/media/st,stm32-dcmi.yaml         |    4 +
 .../memory-controllers/st,stm32-fmc2-ebi.yaml |    4 +
 .../bindings/mfd/st,stm32-lptimer.yaml        |    4 +
 .../bindings/mfd/st,stm32-timers.yaml         |    5 +
 .../devicetree/bindings/mmc/arm,pl18x.yaml    |    4 +
 .../devicetree/bindings/net/stm32-dwmac.yaml  |    4 +
 .../bindings/phy/phy-stm32-usbphyc.yaml       |    4 +
 .../bindings/regulator/st,stm32-vrefbuf.yaml  |    4 +
 .../devicetree/bindings/rng/st,stm32-rng.yaml |    4 +
 .../bindings/serial/st,stm32-uart.yaml        |    4 +
 .../bindings/sound/st,stm32-i2s.yaml          |    4 +
 .../bindings/sound/st,stm32-sai.yaml          |    4 +
 .../bindings/sound/st,stm32-spdifrx.yaml      |    4 +
 .../bindings/spi/st,stm32-qspi.yaml           |    4 +
 .../devicetree/bindings/spi/st,stm32-spi.yaml |    4 +
 .../devicetree/bindings/usb/dwc2.yaml         |    4 +
 MAINTAINERS                                   |    7 +
 arch/arm/boot/dts/st/stm32mp131.dtsi          | 1027 +++---
 arch/arm/boot/dts/st/stm32mp133.dtsi          |   51 +-
 arch/arm/boot/dts/st/stm32mp13xc.dtsi         |   19 +-
 arch/arm/boot/dts/st/stm32mp13xf.dtsi         |   19 +-
 arch/arm/boot/dts/st/stm32mp151.dtsi          | 2757 +++++++++--------
 arch/arm/boot/dts/st/stm32mp153.dtsi          |   52 +-
 arch/arm/boot/dts/st/stm32mp15xc.dtsi         |   19 +-
 arch/arm64/Kconfig.platforms                  |    1 +
 arch/arm64/boot/dts/st/stm32mp251.dtsi        |    5 +-
 drivers/bus/Kconfig                           |   10 +
 drivers/bus/Makefile                          |    1 +
 drivers/bus/stm32_etzpc.c                     |  137 +
 drivers/bus/stm32_firewall.c                  |  252 ++
 drivers/bus/stm32_firewall.h                  |   83 +
 drivers/bus/stm32_rifsc.c                     |  248 ++
 include/linux/bus/stm32_firewall_device.h     |  134 +
 44 files changed, 3276 insertions(+), 1918 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml
 create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
 create mode 100644 Documentation/devicetree/bindings/feature-controllers/feature-domain-controller.yaml
 create mode 100644 drivers/bus/stm32_etzpc.c
 create mode 100644 drivers/bus/stm32_firewall.c
 create mode 100644 drivers/bus/stm32_firewall.h
 create mode 100644 drivers/bus/stm32_rifsc.c
 create mode 100644 include/linux/bus/stm32_firewall_device.h

Comments

Rob Herring (Arm) July 5, 2023, 7:39 p.m. UTC | #1
On Wed, 05 Jul 2023 19:27:52 +0200, Gatien Chevallier wrote:
> Document ETZPC (Extended TrustZone protection controller). ETZPC is a
> firewall controller.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> ---
>  .../bindings/bus/st,stm32-etzpc.yaml          | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml: title: 'STM32 Extended TrustZone protection controller bindings' should not be valid under {'pattern': '([Bb]inding| [Ss]chema)'}
	hint: Everything is a binding/schema, no need to say it. Describe what hardware the binding is for.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
Documentation/devicetree/bindings/bus/st,stm32-etzpc.example.dtb: /example-0/etzpc@5c007000: failed to match any schema with compatible: ['st,stm32mp13-sys-bus']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230705172759.1610753-4-gatien.chevallier@foss.st.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring (Arm) July 5, 2023, 7:39 p.m. UTC | #2
On Wed, 05 Jul 2023 19:27:51 +0200, Gatien Chevallier wrote:
> Document RIFSC (RIF security controller). RIFSC is a firewall controller
> composed of different kinds of hardware resources.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> ---
>  .../bindings/bus/st,stm32-rifsc.yaml          | 101 ++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml: title: 'STM32 Resource isolation framework security controller bindings' should not be valid under {'pattern': '([Bb]inding| [Ss]chema)'}
	hint: Everything is a binding/schema, no need to say it. Describe what hardware the binding is for.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230705172759.1610753-3-gatien.chevallier@foss.st.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski July 6, 2023, 6:28 a.m. UTC | #3
On 05/07/2023 19:27, Gatien Chevallier wrote:
> Document RIFSC (RIF security controller). RIFSC is a firewall controller
> composed of different kinds of hardware resources.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>

A nit, subject: drop second/last, redundant "device tree bindings for".
The "dt-bindings" prefix is already stating that these are bindings. 4
words of your 6 word subject is meaningless...

> ---
>  .../bindings/bus/st,stm32-rifsc.yaml          | 101 ++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
> new file mode 100644
> index 000000000000..68d585ed369c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml

Filename like compatible, unless you know list of compatibles will
grow... but then add them.

> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bus/st,stm32-rifsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STM32 Resource isolation framework security controller bindings

Drop bindings

> +
> +maintainers:
> +  - Gatien Chevallier <gatien.chevallier@foss.st.com>
> +
> +description: |
> +  Resource isolation framework (RIF) is a comprehensive set of hardware blocks
> +  designed to enforce and manage isolation of STM32 hardware resources like
> +  memory and peripherals.
> +
> +  The RIFSC (RIF security controller) is composed of three sets of registers,
> +  each managing a specific set of hardware resources:
> +    - RISC registers associated with RISUP logic (resource isolation device unit
> +      for peripherals), assign all non-RIF aware peripherals to zero, one or
> +      any security domains (secure, privilege, compartment).
> +    - RIMC registers: associated with RIMU logic (resource isolation master
> +      unit), assign all non RIF-aware bus master to one security domain by
> +      setting secure, privileged and compartment information on the system bus.
> +      Alternatively, the RISUP logic controlling the device port access to a
> +      peripheral can assign target bus attributes to this peripheral master port
> +      (supported attribute: CID).
> +    - RISC registers associated with RISAL logic (resource isolation device unit
> +      for address space - Lite version), assign address space subregions to one
> +      security domains (secure, privilege, compartment).
> +
> +properties:
> +  compatible:
> +    const: st,stm32mp25-rifsc
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  "#feature-domain-cells":
> +    const: 1
> +
> +  ranges: true
> +
> +  feature-domain-controller: true
> +
> +patternProperties:
> +  "^.*@[0-9a-f]+$":
> +    description: Peripherals
> +    type: object
> +    properties:
> +      feature-domains:
> +        minItems: 1
> +        maxItems: 2
> +        description:
> +          The first argument must always be a phandle that references to the
> +          firewall controller of the peripheral. The second can contain the
> +          platform specific firewall ID of the peripheral.

It does not make much sense to me to have hierarchy parent-child and via
phandle at the same time. You express the similar relationship twice.

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - feature-domain-controller
> +  - "#feature-domain-cells"
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    // In this example, the usart2 device refers to rifsc as its domain
> +    // controller.
> +    // Access rights are verified before creating devices.
> +
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    rifsc: rifsc-bus@42080000 {
> +        compatible = "st,stm32mp25-rifsc";
> +        reg = <0x42080000 0x1000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +        feature-domain-controller;
> +        #feature-domain-cells = <1>;
> +
> +        usart2: serial@400e0000 {
> +            compatible = "st,stm32h7-uart";
> +            reg = <0x400e0000 0x400>;
> +            interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&ck_flexgen_08>;
> +            feature-domains = <&rifsc 32>;
> +            status = "disabled";

No status in the examples.

> +        };
> +    };

Best regards,
Krzysztof
Alexandre TORGUE July 6, 2023, 9:25 a.m. UTC | #4
Hi Gatien

On 7/5/23 19:27, Gatien Chevallier wrote:
> RIFSC is a firewall controller. Change its compatible so that is matches
> the documentation and reference RIFSC as a feature-domain-controller.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> ---
>   arch/arm64/boot/dts/st/stm32mp251.dtsi | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> index 5268a4321841..62101084cab8 100644
> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
> @@ -106,17 +106,20 @@ soc@0 {
>   		ranges = <0x0 0x0 0x0 0x80000000>;
>   
>   		rifsc: rifsc-bus@42080000 {
> -			compatible = "simple-bus";
> +			compatible = "st,stm32mp25-rifsc";

You could keep "simple-bus" compatible (in second position). In case of 
the RIFSC is not probed, the platform will be able to boot. If you agree 
you can use the same for ETZPC.

Cheers
Alex

>   			reg = <0x42080000 0x1000>;
>   			#address-cells = <1>;
>   			#size-cells = <1>;
>   			ranges;
> +			feature-domain-controller;
> +			#feature-domain-cells = <1>;
>   
>   			usart2: serial@400e0000 {
>   				compatible = "st,stm32h7-uart";
>   				reg = <0x400e0000 0x400>;
>   				interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
>   				clocks = <&ck_flexgen_08>;
> +				feature-domains = <&rifsc 32>;
>   				status = "disabled";
>   			};
>   		};
Gatien CHEVALLIER July 6, 2023, 9:29 a.m. UTC | #5
Hello Krzysztof,

Firstly, I will correct the bindings error pointed by Rob's robot.
Obviously, I did not pass the bindings check the proper way or maybe I'm 
running an old version.

On 7/6/23 08:28, Krzysztof Kozlowski wrote:
> On 05/07/2023 19:27, Gatien Chevallier wrote:
>> Document RIFSC (RIF security controller). RIFSC is a firewall controller
>> composed of different kinds of hardware resources.
>>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> 
> A nit, subject: drop second/last, redundant "device tree bindings for".
> The "dt-bindings" prefix is already stating that these are bindings. 4
> words of your 6 word subject is meaningless...

Ack, I will rephrase, it is indeed redundant

> 
>> ---
>>   .../bindings/bus/st,stm32-rifsc.yaml          | 101 ++++++++++++++++++
>>   1 file changed, 101 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>> new file mode 100644
>> index 000000000000..68d585ed369c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
> 
> Filename like compatible, unless you know list of compatibles will
> grow... but then add them.
> 
>> @@ -0,0 +1,101 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bus/st,stm32-rifsc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STM32 Resource isolation framework security controller bindings
> 
> Drop bindings

Ack

> 
>> +
>> +maintainers:
>> +  - Gatien Chevallier <gatien.chevallier@foss.st.com>
>> +
>> +description: |
>> +  Resource isolation framework (RIF) is a comprehensive set of hardware blocks
>> +  designed to enforce and manage isolation of STM32 hardware resources like
>> +  memory and peripherals.
>> +
>> +  The RIFSC (RIF security controller) is composed of three sets of registers,
>> +  each managing a specific set of hardware resources:
>> +    - RISC registers associated with RISUP logic (resource isolation device unit
>> +      for peripherals), assign all non-RIF aware peripherals to zero, one or
>> +      any security domains (secure, privilege, compartment).
>> +    - RIMC registers: associated with RIMU logic (resource isolation master
>> +      unit), assign all non RIF-aware bus master to one security domain by
>> +      setting secure, privileged and compartment information on the system bus.
>> +      Alternatively, the RISUP logic controlling the device port access to a
>> +      peripheral can assign target bus attributes to this peripheral master port
>> +      (supported attribute: CID).
>> +    - RISC registers associated with RISAL logic (resource isolation device unit
>> +      for address space - Lite version), assign address space subregions to one
>> +      security domains (secure, privilege, compartment).
>> +
>> +properties:
>> +  compatible:
>> +    const: st,stm32mp25-rifsc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 1
>> +
>> +  "#feature-domain-cells":
>> +    const: 1
>> +
>> +  ranges: true
>> +
>> +  feature-domain-controller: true
>> +
>> +patternProperties:
>> +  "^.*@[0-9a-f]+$":
>> +    description: Peripherals
>> +    type: object
>> +    properties:
>> +      feature-domains:
>> +        minItems: 1
>> +        maxItems: 2
>> +        description:
>> +          The first argument must always be a phandle that references to the
>> +          firewall controller of the peripheral. The second can contain the
>> +          platform specific firewall ID of the peripheral.
> 
> It does not make much sense to me to have hierarchy parent-child and via
> phandle at the same time. You express the similar relationship twice
Thank you for pointing this out.

About the parent-child relation:

The bus-like device tree architecture allows a bus-probe mechanism with 
which we want to check accesses of peripherals before probing their 
driver. This has several advantages:
-This bus architecture provides a clearer view of the hardware.
-No peripheral driver modifications as it is fully handled by the 
firewall drivers.
-Drivers for devices that aren't accessible will not even be probed => 
no probe fail.

It would be possible to manage this mechanism another way by handling 
probe deferrals in drivers. But it would mean modifying every driver 
with a check on ST firewall that we probe and some of them aren't from 
STMicroelectronics.

About the phandle relation:

I agree on the fact that this double expression of the relationship is 
redundant.

I've done it this way because there will be other nodes outside the 
RIFSC node that will need to reference it as their feature-domain 
controller. I kept the same information in the property to be coherent 
between all.

For nodes under the RIFSC, the phandle is indeed useless and could be 
removed, just to leave the firewall ID. And I'm inclined to do so. I 
just have one worry on the YAML binding files where I will have a 
pattern property in the RIFSC that will state something and maybe 
another description in the peripheral YAML files. What is your take on that?

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - feature-domain-controller
>> +  - "#feature-domain-cells"
>> +  - ranges
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    // In this example, the usart2 device refers to rifsc as its domain
>> +    // controller.
>> +    // Access rights are verified before creating devices.
>> +
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    rifsc: rifsc-bus@42080000 {
>> +        compatible = "st,stm32mp25-rifsc";
>> +        reg = <0x42080000 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +        feature-domain-controller;
>> +        #feature-domain-cells = <1>;
>> +
>> +        usart2: serial@400e0000 {
>> +            compatible = "st,stm32h7-uart";
>> +            reg = <0x400e0000 0x400>;
>> +            interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
>> +            clocks = <&ck_flexgen_08>;
>> +            feature-domains = <&rifsc 32>;
>> +            status = "disabled";
> 
> No status in the examples.
> 
>> +        };
>> +    };
> 
> Best regards,
> Krzysztof
> 

Best regards,
Gatien
Gatien CHEVALLIER July 6, 2023, 9:30 a.m. UTC | #6
Hi Alex,

On 7/6/23 11:25, Alexandre TORGUE wrote:
> Hi Gatien
> 
> On 7/5/23 19:27, Gatien Chevallier wrote:
>> RIFSC is a firewall controller. Change its compatible so that is matches
>> the documentation and reference RIFSC as a feature-domain-controller.
>>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>> ---
>>   arch/arm64/boot/dts/st/stm32mp251.dtsi | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi 
>> b/arch/arm64/boot/dts/st/stm32mp251.dtsi
>> index 5268a4321841..62101084cab8 100644
>> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
>> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
>> @@ -106,17 +106,20 @@ soc@0 {
>>           ranges = <0x0 0x0 0x0 0x80000000>;
>>           rifsc: rifsc-bus@42080000 {
>> -            compatible = "simple-bus";
>> +            compatible = "st,stm32mp25-rifsc";
> 
> You could keep "simple-bus" compatible (in second position). In case of 
> the RIFSC is not probed, the platform will be able to boot. If you agree 
> you can use the same for ETZPC.
> 
> Cheers
> Alex

Sure, good point.

I'll change that in V2

Best regards,
Gatien
> 
>>               reg = <0x42080000 0x1000>;
>>               #address-cells = <1>;
>>               #size-cells = <1>;
>>               ranges;
>> +            feature-domain-controller;
>> +            #feature-domain-cells = <1>;
>>               usart2: serial@400e0000 {
>>                   compatible = "st,stm32h7-uart";
>>                   reg = <0x400e0000 0x400>;
>>                   interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
>>                   clocks = <&ck_flexgen_08>;
>> +                feature-domains = <&rifsc 32>;
>>                   status = "disabled";
>>               };
>>           };
>
Gatien CHEVALLIER July 20, 2023, 2:58 p.m. UTC | #7
Hello Krzysztof,

On 7/6/23 11:29, Gatien CHEVALLIER wrote:
> Hello Krzysztof,
> 
> Firstly, I will correct the bindings error pointed by Rob's robot.
> Obviously, I did not pass the bindings check the proper way or maybe I'm 
> running an old version.
> 
> On 7/6/23 08:28, Krzysztof Kozlowski wrote:
>> On 05/07/2023 19:27, Gatien Chevallier wrote:
>>> Document RIFSC (RIF security controller). RIFSC is a firewall controller
>>> composed of different kinds of hardware resources.
>>>
>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>
>> A nit, subject: drop second/last, redundant "device tree bindings for".
>> The "dt-bindings" prefix is already stating that these are bindings. 4
>> words of your 6 word subject is meaningless...
> 
> Ack, I will rephrase, it is indeed redundant
> 
>>
>>> ---
>>>   .../bindings/bus/st,stm32-rifsc.yaml          | 101 ++++++++++++++++++
>>>   1 file changed, 101 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml 
>>> b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>> new file mode 100644
>>> index 000000000000..68d585ed369c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>
>> Filename like compatible, unless you know list of compatibles will
>> grow... but then add them.
>>
>>> @@ -0,0 +1,101 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/bus/st,stm32-rifsc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: STM32 Resource isolation framework security controller bindings
>>
>> Drop bindings
> 
> Ack
> 
>>
>>> +
>>> +maintainers:
>>> +  - Gatien Chevallier <gatien.chevallier@foss.st.com>
>>> +
>>> +description: |
>>> +  Resource isolation framework (RIF) is a comprehensive set of 
>>> hardware blocks
>>> +  designed to enforce and manage isolation of STM32 hardware 
>>> resources like
>>> +  memory and peripherals.
>>> +
>>> +  The RIFSC (RIF security controller) is composed of three sets of 
>>> registers,
>>> +  each managing a specific set of hardware resources:
>>> +    - RISC registers associated with RISUP logic (resource isolation 
>>> device unit
>>> +      for peripherals), assign all non-RIF aware peripherals to 
>>> zero, one or
>>> +      any security domains (secure, privilege, compartment).
>>> +    - RIMC registers: associated with RIMU logic (resource isolation 
>>> master
>>> +      unit), assign all non RIF-aware bus master to one security 
>>> domain by
>>> +      setting secure, privileged and compartment information on the 
>>> system bus.
>>> +      Alternatively, the RISUP logic controlling the device port 
>>> access to a
>>> +      peripheral can assign target bus attributes to this peripheral 
>>> master port
>>> +      (supported attribute: CID).
>>> +    - RISC registers associated with RISAL logic (resource isolation 
>>> device unit
>>> +      for address space - Lite version), assign address space 
>>> subregions to one
>>> +      security domains (secure, privilege, compartment).
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: st,stm32mp25-rifsc
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 1
>>> +
>>> +  "#feature-domain-cells":
>>> +    const: 1
>>> +
>>> +  ranges: true
>>> +
>>> +  feature-domain-controller: true
>>> +
>>> +patternProperties:
>>> +  "^.*@[0-9a-f]+$":
>>> +    description: Peripherals
>>> +    type: object
>>> +    properties:
>>> +      feature-domains:
>>> +        minItems: 1
>>> +        maxItems: 2
>>> +        description:
>>> +          The first argument must always be a phandle that 
>>> references to the
>>> +          firewall controller of the peripheral. The second can 
>>> contain the
>>> +          platform specific firewall ID of the peripheral.
>>
>> It does not make much sense to me to have hierarchy parent-child and via
>> phandle at the same time. You express the similar relationship twice
> Thank you for pointing this out.
> 
> About the parent-child relation:
> 
> The bus-like device tree architecture allows a bus-probe mechanism with 
> which we want to check accesses of peripherals before probing their 
> driver. This has several advantages:
> -This bus architecture provides a clearer view of the hardware.
> -No peripheral driver modifications as it is fully handled by the 
> firewall drivers.
> -Drivers for devices that aren't accessible will not even be probed => 
> no probe fail.
> 
> It would be possible to manage this mechanism another way by handling 
> probe deferrals in drivers. But it would mean modifying every driver 
> with a check on ST firewall that we probe and some of them aren't from 
> STMicroelectronics.
> 
> About the phandle relation:
> 
> I agree on the fact that this double expression of the relationship is 
> redundant.
> 
> I've done it this way because there will be other nodes outside the 
> RIFSC node that will need to reference it as their feature-domain 
> controller. I kept the same information in the property to be coherent 
> between all.
> 
> For nodes under the RIFSC, the phandle is indeed useless and could be 
> removed, just to leave the firewall ID. And I'm inclined to do so. I 
> just have one worry on the YAML binding files where I will have a 
> pattern property in the RIFSC that will state something and maybe 
> another description in the peripheral YAML files. What is your take on 
> that?
> 

Looking back at it, feature-domains is a phandle-array. I guess I can't
derogate to the following architecture:

items:
   - items:
       - description: A phandle
       - description: 1st arg cell
       - description: 2nd arg cell

can I?

Some devices' nodes that are not subnodes of the firewall controllers
will need the phandle reference. Should I keep the redundant information
then?

Best regards,
Gatien

>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - "#address-cells"
>>> +  - "#size-cells"
>>> +  - feature-domain-controller
>>> +  - "#feature-domain-cells"
>>> +  - ranges
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    // In this example, the usart2 device refers to rifsc as its domain
>>> +    // controller.
>>> +    // Access rights are verified before creating devices.
>>> +
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    rifsc: rifsc-bus@42080000 {
>>> +        compatible = "st,stm32mp25-rifsc";
>>> +        reg = <0x42080000 0x1000>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges;
>>> +        feature-domain-controller;
>>> +        #feature-domain-cells = <1>;
>>> +
>>> +        usart2: serial@400e0000 {
>>> +            compatible = "st,stm32h7-uart";
>>> +            reg = <0x400e0000 0x400>;
>>> +            interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
>>> +            clocks = <&ck_flexgen_08>;
>>> +            feature-domains = <&rifsc 32>;
>>> +            status = "disabled";
>>
>> No status in the examples.
>>
>>> +        };
>>> +    };
>>
>> Best regards,
>> Krzysztof
>>
> 
> Best regards,
> Gatien
Gatien CHEVALLIER July 25, 2023, 2:07 p.m. UTC | #8
Hi Alex,

On 7/6/23 11:30, Gatien CHEVALLIER wrote:
> Hi Alex,
> 
> On 7/6/23 11:25, Alexandre TORGUE wrote:
>> Hi Gatien
>>
>> On 7/5/23 19:27, Gatien Chevallier wrote:
>>> RIFSC is a firewall controller. Change its compatible so that is matches
>>> the documentation and reference RIFSC as a feature-domain-controller.
>>>
>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>> ---
>>>   arch/arm64/boot/dts/st/stm32mp251.dtsi | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi 
>>> b/arch/arm64/boot/dts/st/stm32mp251.dtsi
>>> index 5268a4321841..62101084cab8 100644
>>> --- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
>>> +++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
>>> @@ -106,17 +106,20 @@ soc@0 {
>>>           ranges = <0x0 0x0 0x0 0x80000000>;
>>>           rifsc: rifsc-bus@42080000 {
>>> -            compatible = "simple-bus";
>>> +            compatible = "st,stm32mp25-rifsc";
>>
>> You could keep "simple-bus" compatible (in second position). In case 
>> of the RIFSC is not probed, the platform will be able to boot. If you 
>> agree you can use the same for ETZPC.
>>
>> Cheers
>> Alex
> 
> Sure, good point.
> 
> I'll change that in V2
> 
> Best regards,
> Gatien

Actually, it would be a bad idea to keep "simple-bus" as a compatible.

Practical example:
1) Firewall controller forbids a device probe by marking the device's
node as populated (OF_POPULATED flag).
2) The simple-bus, which is simple, populates all the devices
from the device tree data, overriding what the firewall bus has done.
3)=>Forbidden device's driver will be probed.

I think it's best to keep one compatible. If someone wants these drivers
as external modules, then it'll be best to handle this differently.
I'll resubmit with a single compatible for V2, please do not
hesitate to comment on the V2 if you're not okay with this.

Best regards,
Gatien

>>
>>>               reg = <0x42080000 0x1000>;
>>>               #address-cells = <1>;
>>>               #size-cells = <1>;
>>>               ranges;
>>> +            feature-domain-controller;
>>> +            #feature-domain-cells = <1>;
>>>               usart2: serial@400e0000 {
>>>                   compatible = "st,stm32h7-uart";
>>>                   reg = <0x400e0000 0x400>;
>>>                   interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
>>>                   clocks = <&ck_flexgen_08>;
>>> +                feature-domains = <&rifsc 32>;
>>>                   status = "disabled";
>>>               };
>>>           };
>>