| Message ID | 20230705172759.1610753-1-gatien.chevallier@foss.st.com |
|---|---|
| Headers | show |
| Series | Introduce STM32 Firewall framework | expand |
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.
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
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"; > }; > };
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
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"; >> }; >> }; >
On Wed, Jul 05, 2023 at 07:27:54PM +0200, Gatien Chevallier wrote: > Introduce a firewall framework that offers to firewall consumers different > firewall services such as the ability to check their access rights against > their firewall controller(s). > > The firewall framework offers a generic API that is defined in firewall > controllers drivers to best fit the specificity of each firewall. > > There are various types of firewalls: > -Peripheral firewalls that filter accesses to peripherals > -Memory firewalls that filter accesses to memories or memory regions > -Resource firewalls that filter accesses to internal resources such as > reset and clock controllers How do resource firewalls work? Access to registers for some clocks in a clock controller are disabled? Or something gates off clocks/resets to a block? It might make more sense for "resource" accesses to be managed within those resource APIs (i.e. the clock and reset frameworks) and leave this framework to bus accesses. > A firewall controller must be probed at arch_initcall level and register > to the framework so that consumers can use their services. initcall ordering hacks should not be needed. We have both deferred probe and fw_devlinks to avoid that problem. > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> > --- > MAINTAINERS | 5 + > arch/arm64/Kconfig.platforms | 1 + > drivers/bus/Kconfig | 10 + > drivers/bus/Makefile | 1 + > drivers/bus/stm32_firewall.c | 252 ++++++++++++++++++++++ > drivers/bus/stm32_firewall.h | 83 +++++++ Why something stm32 specific? We know there are multiple platforms wanting something in this area. Wasn't the last attempt common? For a common binding, I'm not eager to accept anything new with only 1 user. > include/linux/bus/stm32_firewall_device.h | 134 ++++++++++++ > 7 files changed, 486 insertions(+) > create mode 100644 drivers/bus/stm32_firewall.c > create mode 100644 drivers/bus/stm32_firewall.h > create mode 100644 include/linux/bus/stm32_firewall_device.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 41385f01fa98..fabf95ba9b86 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20123,6 +20123,11 @@ T: git git://linuxtv.org/media_tree.git > F: Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml > F: drivers/media/i2c/st-mipid02.c > > +ST STM32 FIREWALL > +M: Gatien Chevallier <gatien.chevallier@foss.st.com> > +S: Maintained > +F: drivers/bus/stm32_firewall.c > + > ST STM32 I2C/SMBUS DRIVER > M: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com> > M: Alain Volmat <alain.volmat@foss.st.com> > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index 6069120199bb..5a46e90f1e4e 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -293,6 +293,7 @@ config ARCH_STM32 > select ARM_SMC_MBOX > select ARM_SCMI_PROTOCOL > select COMMON_CLK_SCMI > + select STM32_FIREWALL > help > This enables support for ARMv8 based STMicroelectronics > STM32 family, including: > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index fcfa280df98a..4d54a7ea52b2 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -163,6 +163,16 @@ config QCOM_SSC_BLOCK_BUS > i2c/spi/uart controllers, a hexagon core, and a clock controller > which provides clocks for the above. > > +config STM32_FIREWALL > + bool "STM32 Firewall framework" > + depends on ARCH_STM32 > + default MACH_STM32MP157 || MACH_STM32MP13 || MACH_STM32MP25 > + help > + Say y to enable firewall framework and its services. Firewall > + controllers will be able to register to the framework. Firewall > + controllers must be initialized and register to the firewall framework > + at arch_initcall level. > + > config SUN50I_DE2_BUS > bool "Allwinner A64 DE2 Bus Driver" > default ARM64 > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index d90eed189a65..fc0511450ec2 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_OMAP_INTERCONNECT) += omap_l3_smx.o omap_l3_noc.o > obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o > obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o > obj-$(CONFIG_QCOM_SSC_BLOCK_BUS) += qcom-ssc-block-bus.o > +obj-$(CONFIG_STM32_FIREWALL) += stm32_firewall.o > obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o > obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o > obj-$(CONFIG_OF) += simple-pm-bus.o > diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c > new file mode 100644 > index 000000000000..510db5bc6eaf > --- /dev/null > +++ b/drivers/bus/stm32_firewall.c > @@ -0,0 +1,252 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later The default kernel license is GPL-2.0-only. Why the deviation? > +/* > + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved > + */ > + > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/bus/stm32_firewall_device.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +#include "stm32_firewall.h" > + > +/* Corresponds to STM32_FIREWALL_MAX_EXTRA_ARGS + firewall controller reference + firewall ID */ > +#define STM32_FIREWALL_MAX_ARGS (STM32_FIREWALL_MAX_EXTRA_ARGS + 2) > + > +static LIST_HEAD(firewall_controller_list); > +static DEFINE_MUTEX(firewall_controller_list_lock); > + > +static int stm32_firewall_get_id(struct device_node *np, u32 *id) > +{ > + u32 feature_domain_cell[2]; > + > + /* Get property from device node */ > + if (of_property_read_u32_array(np, "feature-domains", > + feature_domain_cell, > + ARRAY_SIZE(feature_domain_cell))) { > + pr_err("Unable to find get firewall ID property\n"); > + return -ENODEV; > + } > + > + *id = feature_domain_cell[1]; > + > + return 0; > +} > + > +/* Firewall device API */ > + > +int stm32_firewall_get_firewall(struct device_node *np, > + struct stm32_firewall *firewall) > +{ > + struct stm32_firewall_controller *ctrl; > + struct of_phandle_args args; > + u32 controller_phandle; > + bool match = false; > + size_t i; > + int err; > + > + if (!firewall) > + return -EINVAL; > + > + /* The controller phandle is always the first argument of the feature-domains property. */ > + err = of_property_read_u32(np, "feature-domains", &controller_phandle); Why do you need to parse the property twice? > + if (err) { > + pr_err("Unable to get feature-domains property for node %s\n", np->full_name); > + return err; > + } > + > + /* Parse property with phandle parsed out */ > + err = of_parse_phandle_with_args(np, "feature-domains", "#feature-domain-cells", 0, &args); > + if (err) { > + pr_err("Unable to read feature-domains arguments for node %s\n", np->full_name); > + return err; > + } > + > + /* The phandle is parsed out */ > + if (args.args_count > STM32_FIREWALL_MAX_ARGS - 1) > + return -EINVAL; > + > + of_node_put(np); > + > + /* Check if the parsed phandle corresponds to a registered firewall controller */ > + mutex_lock(&firewall_controller_list_lock); > + list_for_each_entry(ctrl, &firewall_controller_list, entry) { > + if (ctrl->dev->of_node->phandle == controller_phandle) { > + match = true; > + firewall->firewall_ctrl = ctrl; > + break; > + } > + } > + mutex_unlock(&firewall_controller_list_lock); > + if (!match) { > + firewall->firewall_ctrl = NULL; > + pr_err("No firewall controller registered for %s\n", np->full_name); > + return -ENODEV; > + } > + > + /* > + * The firewall ID is always the second argument of the feature-domains property. > + * The first argument is already parsed out, so args.args[0] is the second argument. > + */ > + firewall->firewall_id = args.args[0]; > + > + /* Extra args start at the third argument */ > + for (i = 0; i < args.args_count; i++) > + firewall->extra_args[i] = args.args[i + 1]; > + > + /* Remove the firewall ID arg that is not an extra argument */ > + if (args.args_count >= 1) > + firewall->extra_args_size = args.args_count - 1; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(stm32_firewall_get_firewall); > + > +int stm32_firewall_grant_access(struct stm32_firewall *firewall) > +{ > + struct stm32_firewall_controller *firewall_controller; > + > + if (!firewall || firewall->firewall_id == U32_MAX) > + return -EINVAL; > + > + firewall_controller = firewall->firewall_ctrl; > + > + if (!firewall_controller) > + return -ENODEV; > + > + return firewall_controller->grant_access(firewall_controller, firewall->firewall_id); > +} > +EXPORT_SYMBOL_GPL(stm32_firewall_grant_access); > + > +int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) > +{ > + struct stm32_firewall_controller *firewall_controller; > + > + if (!firewall || subsystem_id == U32_MAX || firewall->firewall_id == U32_MAX) > + return -EINVAL; > + > + firewall_controller = firewall->firewall_ctrl; > + > + if (!firewall_controller) > + return -ENODEV; > + > + return firewall_controller->grant_access(firewall_controller, subsystem_id); > +} > +EXPORT_SYMBOL_GPL(stm32_firewall_grant_access_by_id); > + > +void stm32_firewall_release_access(struct stm32_firewall *firewall) > +{ > + struct stm32_firewall_controller *firewall_controller; > + > + if (!firewall || firewall->firewall_id == U32_MAX) { > + pr_err("Incorrect arguments when releasing a firewall access"); > + return; > + } > + > + firewall_controller = firewall->firewall_ctrl; > + > + if (!firewall_controller) { > + pr_debug("No firewall controller to release"); > + return; > + } > + > + firewall_controller->release_access(firewall_controller, firewall->firewall_id); > +} > +EXPORT_SYMBOL_GPL(stm32_firewall_release_access); > + > +void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) > +{ > + struct stm32_firewall_controller *firewall_controller; > + > + if (!firewall || subsystem_id == U32_MAX || firewall->firewall_id == U32_MAX) { > + pr_err("Incorrect arguments when releasing a firewall access"); > + return; > + } > + > + firewall_controller = firewall->firewall_ctrl; > + > + if (!firewall_controller) { > + pr_debug("No firewall controller to release"); > + return; > + } > + > + firewall_controller->release_access(firewall_controller, subsystem_id); > +} > +EXPORT_SYMBOL_GPL(stm32_firewall_release_access_by_id); > + > +/* Firewall controller API */ > + > +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller) > +{ > + pr_info("Registering firewall controller %s", dev_name(firewall_controller->dev)); > + > + if (!firewall_controller) > + return -ENODEV; > + > + mutex_lock(&firewall_controller_list_lock); > + list_add_tail(&firewall_controller->entry, &firewall_controller_list); > + mutex_unlock(&firewall_controller_list_lock); > + > + return 0; > + > +} > + > +void stm32_firewall_controller_unregister(struct stm32_firewall_controller *firewall_controller) > +{ > + struct stm32_firewall_controller *ctrl, *tmp; > + bool controller_removed = false; > + > + if (!firewall_controller) { > + pr_debug("Null reference while unregistering firewall controller"); > + return; > + } > + > + mutex_lock(&firewall_controller_list_lock); > + list_for_each_entry_safe(ctrl, tmp, &firewall_controller_list, entry) { > + if (ctrl == firewall_controller) { > + controller_removed = true; > + list_del_init(&ctrl->entry); > + break; > + } > + } > + mutex_unlock(&firewall_controller_list_lock); > + > + if (!controller_removed) > + pr_debug("There was no firewall controller named %s to unregister", > + dev_name(firewall_controller->dev)); > +} > + > +void stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller) > +{ > + struct device_node *child; > + struct device *parent; > + u32 firewall_id; > + int err; > + > + parent = firewall_controller->dev; > + > + dev_dbg(parent, "Populating %s system bus\n", dev_name(firewall_controller->dev)); > + > + for_each_available_child_of_node(dev_of_node(parent), child) { > + err = stm32_firewall_get_id(child, &firewall_id); > + if (err < 0 || > + firewall_controller->grant_access(firewall_controller, firewall_id)) { > + /* > + * Peripheral access not allowed or not defined. > + * Mark the node as populated so platform bus won't probe it > + */ > + of_node_set_flag(child, OF_POPULATED); > + dev_err(parent, "%s: Device driver will not be probed\n", > + child->full_name); > + } > + } > +} > diff --git a/drivers/bus/stm32_firewall.h b/drivers/bus/stm32_firewall.h > new file mode 100644 > index 000000000000..8d92e8c1ab77 > --- /dev/null > +++ b/drivers/bus/stm32_firewall.h > @@ -0,0 +1,83 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved > + */ > + > +#ifndef _STM32_FIREWALL_H > +#define _STM32_FIREWALL_H > + > +#include <linux/kernel.h> > +#include <linux/list.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +/** > + * STM32_PERIPHERAL_FIREWALL: This type of firewall protects peripherals > + * STM32_MEMORY_FIREWALL: This type of firewall protects memories/subsets of memory > + * zones > + * STM32_RESOURCE_FIREWALL: This type of firewall protects internal resources > + * STM32_NOTYPE_FIREWALL: Undefined firewall type > + */ > + > +#define STM32_PERIPHERAL_FIREWALL BIT(1) > +#define STM32_MEMORY_FIREWALL BIT(2) > +#define STM32_RESOURCE_FIREWALL BIT(3) > +#define STM32_NOTYPE_FIREWALL BIT(4) > + > +/** > + * struct stm32_firewall_controller - Information on firewall controller supplying services > + * > + * @name Name of the firewall controller > + * @dev Device reference of the firewall controller > + * @mmio Base address of the firewall controller > + * @entry List entry of the firewall controller list > + * @type Type of firewall > + * @max_entries Number of entries covered by the firewall > + * @grant_access Callback used to grant access for a device access against a > + * firewall controller > + * @release_access Callback used to release resources taken by a device when access was > + * granted > + * @grant_memory_range_access Callback used to grant access for a device to a given memory region > + */ > +struct stm32_firewall_controller { > + const char *name; > + struct device *dev; > + void __iomem *mmio; > + struct list_head entry; > + unsigned int type; > + unsigned int max_entries; > + > + int (*grant_access)(struct stm32_firewall_controller *ctrl, u32 id); > + void (*release_access)(struct stm32_firewall_controller *ctrl, u32 id); > + int (*grant_memory_range_access)(struct stm32_firewall_controller *ctrl, phys_addr_t paddr, > + size_t size); > +}; > + > +/** > + * int stm32_firewall_controller_register - Register a firewall controller to the STM32 firewall > + * framework > + * @firewall_controller Firewall controller to register > + * > + * Returns 0 in case of success or -ENODEV if no controller was given. > + */ > +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller); > + > +/** > + * int stm32_firewall_controller_unregister - Unregister a firewall controller from the STM32 > + * firewall framework > + * @firewall_controller Firewall controller to unregister > + */ > +void stm32_firewall_controller_unregister(struct stm32_firewall_controller *firewall_controller); > + > +/** > + * stm32_firewall_populate_bus - Populate device tree nodes that have a correct firewall > + * configuration. This is used at boot-time only, as a sanity check > + * between device tree and firewalls hardware configurations to > + * prevent a kernel crash when a device driver is not granted access > + * > + * @firewall_controller Firewall controller which nodes will be populated or not > + */ > +void stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller); > + > +#endif /* _STM32_FIREWALL_H */ > diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h > new file mode 100644 > index 000000000000..ccaecea7fc6c > --- /dev/null > +++ b/include/linux/bus/stm32_firewall_device.h > @@ -0,0 +1,134 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved > + */ > + > +#ifndef STM32_FIREWALL_DEVICE_H > +#define STM32_FIREWALL_DEVICE_H > + > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +#define STM32_FIREWALL_MAX_EXTRA_ARGS 5 > + > +/* Opaque reference to stm32_firewall_controller */ > +struct stm32_firewall_controller; > + > +/** > + * stm32_firewall - Information on a device's firewall. Each device can have more than one firewall. > + * > + * @firewall_ctrl Pointer referencing a firewall controller of the device. It is > + * opaque so a device cannot manipulate the controller's ops or access > + * the controller's data > + * @extra_args Extra arguments that are implementation dependent > + * @extra_args_size Number of extra arguments > + * @firewall_id Firewall ID associated the device for this firewall controller > + */ > +struct stm32_firewall { > + struct stm32_firewall_controller *firewall_ctrl; > + u32 extra_args[STM32_FIREWALL_MAX_EXTRA_ARGS]; > + size_t extra_args_size; > + u32 firewall_id; > +}; > + > +#if IS_ENABLED(CONFIG_STM32_FIREWALL) > +/** > + * stm32_firewall_get_firewall - Get the firewall(s) associated to given device. > + * The firewall controller reference is always the first argument > + * of the feature-domains property. > + * The firewall ID is always the second argument of the > + * feature-domains property. > + * > + * @np Device node to parse > + * @firewall Resulting firewall reference(s) > + * > + * Returns 0 on success, -ENODEV if there's no match with a firewall controller or appropriate errno > + * code if error occurred. > + */ > +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall); > + > +/** > + * stm32_firewall_grant_access - Request firewall access rights and grant access. > + * > + * @firewall Firewall reference containing the ID to check against its firewall > + * controller > + * > + * Returns 0 if access is granted, -EACCES if access is denied, -ENODEV if firewall is null or > + * appropriate errno code if error occurred > + */ > +int stm32_firewall_grant_access(struct stm32_firewall *firewall); > + > +/** > + * stm32_firewall_release_access - Release access granted from a call to > + * stm32_firewall_grant_access(). > + * > + * @firewall Firewall reference containing the ID to check against its firewall > + * controller > + */ > +void stm32_firewall_release_access(struct stm32_firewall *firewall); > + > +/** > + * stm32_firewall_grant_access_by_id - Request firewall access rights of a given device > + * based on a specific firewall ID > + * > + * Warnings: > + * There is no way to ensure that the given ID will correspond to the firewall referenced in the > + * device node if the ID did not come from stm32_firewall_get_firewall(). In that case, this > + * function must be used with caution. > + * This function should be used for subsystem resources that do not have the same firewall ID > + * as their parent. > + * U32_MAX is an invalid ID. > + * > + * @firewall Firewall reference containing the firewall controller > + * @subsystem_id Firewall ID of the subsystem resource > + * > + * Returns 0 if access is granted, -EACCES if access is denied, -ENODEV if firewall is null or > + * appropriate errno code if error occurred > + */ > +int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id); > + > +/** > + * stm32_firewall_release_access_by_id - Release access granted from a call to > + * stm32_firewall_grant_access_by_id(). > + * > + * Warnings: > + * There is no way to ensure that the given ID will correspond to the firewall referenced in the > + * device node if the ID did not come from stm32_firewall_get_firewall(). In that case, this > + * function must be used with caution. > + * This function should be used for subsystem resources that do not have the same firewall ID > + * as their parent. > + * U32_MAX is an invalid ID. > + * > + * @firewall Firewall reference containing the firewall controller > + * @subsystem_id Firewall ID of the subsystem resource > + */ > +void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id); > + > +#else /* CONFIG_STM32_FIREWALL */ > + > +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall) > +{ > + return -ENODEV; > +} > + > +int stm32_firewall_grant_access(struct stm32_firewall *firewall) > +{ > + return -ENODEV; > +} > + > +void stm32_firewall_release_access(struct stm32_firewall *firewall) > +{ > +} > + > +int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) > +{ > + return -ENODEV; > +} > + > +void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) > +{ > +} > + > +#endif /* CONFIG_STM32_FIREWALL */ > +#endif /* STM32_FIREWALL_DEVICE_H */ > -- > 2.25.1 >
On 7/6/23 17:09, Rob Herring wrote: > On Wed, Jul 05, 2023 at 07:27:54PM +0200, Gatien Chevallier wrote: >> Introduce a firewall framework that offers to firewall consumers different >> firewall services such as the ability to check their access rights against >> their firewall controller(s). >> >> The firewall framework offers a generic API that is defined in firewall >> controllers drivers to best fit the specificity of each firewall. >> >> There are various types of firewalls: >> -Peripheral firewalls that filter accesses to peripherals >> -Memory firewalls that filter accesses to memories or memory regions >> -Resource firewalls that filter accesses to internal resources such as >> reset and clock controllers > > How do resource firewalls work? Access to registers for some clocks in a > clock controller are disabled? Or something gates off clocks/resets to > a block? To take a practical example: A clock controller can be firewall-aware and have its own firewall registers to configure. To access a clock/reset that is handled this way, a device would need to check this "resource firewall". I thought that for these kinds of hardware blocks, having a common API would help. > > It might make more sense for "resource" accesses to be managed within > those resource APIs (i.e. the clock and reset frameworks) and leave this > framework to bus accesses. > Okay, I'll drop this for V2 if you find that the above explaination do not justify this. >> A firewall controller must be probed at arch_initcall level and register >> to the framework so that consumers can use their services. > > initcall ordering hacks should not be needed. We have both deferred > probe and fw_devlinks to avoid that problem. > Greg also doubts this. Drivers like reset/clock controllers drivers (core_initcall level) will have a dependency on the firewall controllers in order to initialize their resources. I was not sure how to manage these dependencies. Now, looking at init/main.c, I've realized that core_initcall() level comes before arch_initcall() level... If managed by fw_devlink, the feature-domains property should be supported as well I suppose? I'm not sure how to handle this properly. I'd welcome your suggestion. >> >> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> >> --- >> MAINTAINERS | 5 + >> arch/arm64/Kconfig.platforms | 1 + >> drivers/bus/Kconfig | 10 + >> drivers/bus/Makefile | 1 + >> drivers/bus/stm32_firewall.c | 252 ++++++++++++++++++++++ >> drivers/bus/stm32_firewall.h | 83 +++++++ > > Why something stm32 specific? We know there are multiple platforms > wanting something in this area. Wasn't the last attempt common? > > For a common binding, I'm not eager to accept anything new with only 1 > user. > Last attempt was common for the feature-domain bindings. The system-bus driver was ST-specific. I don't know if other platforms needs this kind of framework. Are you suggesting that this framework should be generic? Or that this framework should have a st-specific property? I've oriented this firewall framework to serve ST purpose. There may be a need for other platforms but I'm not sure that this framework serves them well. One can argue that it is quite minimalist and covers basic purposes of a hardware firewall but I would need more feedback from other vendors to submit it as a generic one. >> include/linux/bus/stm32_firewall_device.h | 134 ++++++++++++ >> 7 files changed, 486 insertions(+) >> create mode 100644 drivers/bus/stm32_firewall.c >> create mode 100644 drivers/bus/stm32_firewall.h >> create mode 100644 include/linux/bus/stm32_firewall_device.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 41385f01fa98..fabf95ba9b86 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -20123,6 +20123,11 @@ T: git git://linuxtv.org/media_tree.git >> F: Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml >> F: drivers/media/i2c/st-mipid02.c >> >> +ST STM32 FIREWALL >> +M: Gatien Chevallier <gatien.chevallier@foss.st.com> >> +S: Maintained >> +F: drivers/bus/stm32_firewall.c >> + >> ST STM32 I2C/SMBUS DRIVER >> M: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com> >> M: Alain Volmat <alain.volmat@foss.st.com> >> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms >> index 6069120199bb..5a46e90f1e4e 100644 >> --- a/arch/arm64/Kconfig.platforms >> +++ b/arch/arm64/Kconfig.platforms >> @@ -293,6 +293,7 @@ config ARCH_STM32 >> select ARM_SMC_MBOX >> select ARM_SCMI_PROTOCOL >> select COMMON_CLK_SCMI >> + select STM32_FIREWALL >> help >> This enables support for ARMv8 based STMicroelectronics >> STM32 family, including: >> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig >> index fcfa280df98a..4d54a7ea52b2 100644 >> --- a/drivers/bus/Kconfig >> +++ b/drivers/bus/Kconfig >> @@ -163,6 +163,16 @@ config QCOM_SSC_BLOCK_BUS >> i2c/spi/uart controllers, a hexagon core, and a clock controller >> which provides clocks for the above. >> >> +config STM32_FIREWALL >> + bool "STM32 Firewall framework" >> + depends on ARCH_STM32 >> + default MACH_STM32MP157 || MACH_STM32MP13 || MACH_STM32MP25 >> + help >> + Say y to enable firewall framework and its services. Firewall >> + controllers will be able to register to the framework. Firewall >> + controllers must be initialized and register to the firewall framework >> + at arch_initcall level. >> + >> config SUN50I_DE2_BUS >> bool "Allwinner A64 DE2 Bus Driver" >> default ARM64 >> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile >> index d90eed189a65..fc0511450ec2 100644 >> --- a/drivers/bus/Makefile >> +++ b/drivers/bus/Makefile >> @@ -26,6 +26,7 @@ obj-$(CONFIG_OMAP_INTERCONNECT) += omap_l3_smx.o omap_l3_noc.o >> obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o >> obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o >> obj-$(CONFIG_QCOM_SSC_BLOCK_BUS) += qcom-ssc-block-bus.o >> +obj-$(CONFIG_STM32_FIREWALL) += stm32_firewall.o >> obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o >> obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o >> obj-$(CONFIG_OF) += simple-pm-bus.o >> diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c >> new file mode 100644 >> index 000000000000..510db5bc6eaf >> --- /dev/null >> +++ b/drivers/bus/stm32_firewall.c >> @@ -0,0 +1,252 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later > > The default kernel license is GPL-2.0-only. Why the deviation? > >> +/* >> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/bits.h> >> +#include <linux/bus/stm32_firewall_device.h> >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/init.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> +#include <linux/types.h> >> + >> +#include "stm32_firewall.h" >> + >> +/* Corresponds to STM32_FIREWALL_MAX_EXTRA_ARGS + firewall controller reference + firewall ID */ >> +#define STM32_FIREWALL_MAX_ARGS (STM32_FIREWALL_MAX_EXTRA_ARGS + 2) >> + >> +static LIST_HEAD(firewall_controller_list); >> +static DEFINE_MUTEX(firewall_controller_list_lock); >> + >> +static int stm32_firewall_get_id(struct device_node *np, u32 *id) >> +{ >> + u32 feature_domain_cell[2]; >> + >> + /* Get property from device node */ >> + if (of_property_read_u32_array(np, "feature-domains", >> + feature_domain_cell, >> + ARRAY_SIZE(feature_domain_cell))) { >> + pr_err("Unable to find get firewall ID property\n"); >> + return -ENODEV; >> + } >> + >> + *id = feature_domain_cell[1]; >> + >> + return 0; >> +} >> + >> +/* Firewall device API */ >> + >> +int stm32_firewall_get_firewall(struct device_node *np, >> + struct stm32_firewall *firewall) >> +{ >> + struct stm32_firewall_controller *ctrl; >> + struct of_phandle_args args; >> + u32 controller_phandle; >> + bool match = false; >> + size_t i; >> + int err; >> + >> + if (!firewall) >> + return -EINVAL; >> + >> + /* The controller phandle is always the first argument of the feature-domains property. */ >> + err = of_property_read_u32(np, "feature-domains", &controller_phandle); > > Why do you need to parse the property twice? > The first parsing is to have the first argument, which is the controller phandle. The second parsing is here to get the firewall arguments based on the number of arguments defined by #feature-domain-cells. Maybe using of_property_read_u32_array() would be better. I did not want to close the door for supporting several feature domain controllers, hence multiple phandles. of_parse_phandle_with_args() seemed fine for this purpose but the phandle is parsed out. Best regards, Gatien >> + if (err) { >> + pr_err("Unable to get feature-domains property for node %s\n", np->full_name); >> + return err; >> + } >> + >> + /* Parse property with phandle parsed out */ >> + err = of_parse_phandle_with_args(np, "feature-domains", "#feature-domain-cells", 0, &args); >> + if (err) { >> + pr_err("Unable to read feature-domains arguments for node %s\n", np->full_name); >> + return err; >> + } >> + >> + /* The phandle is parsed out */ >> + if (args.args_count > STM32_FIREWALL_MAX_ARGS - 1) >> + return -EINVAL; >> + >> + of_node_put(np); >> + >> + /* Check if the parsed phandle corresponds to a registered firewall controller */ >> + mutex_lock(&firewall_controller_list_lock); >> + list_for_each_entry(ctrl, &firewall_controller_list, entry) { >> + if (ctrl->dev->of_node->phandle == controller_phandle) { >> + match = true; >> + firewall->firewall_ctrl = ctrl; >> + break; >> + } >> + } >> + mutex_unlock(&firewall_controller_list_lock); >> + if (!match) { >> + firewall->firewall_ctrl = NULL; >> + pr_err("No firewall controller registered for %s\n", np->full_name); >> + return -ENODEV; >> + } >> + >> + /* >> + * The firewall ID is always the second argument of the feature-domains property. >> + * The first argument is already parsed out, so args.args[0] is the second argument. >> + */ >> + firewall->firewall_id = args.args[0]; >> + >> + /* Extra args start at the third argument */ >> + for (i = 0; i < args.args_count; i++) >> + firewall->extra_args[i] = args.args[i + 1]; >> + >> + /* Remove the firewall ID arg that is not an extra argument */ >> + if (args.args_count >= 1) >> + firewall->extra_args_size = args.args_count - 1; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_get_firewall); >> + >> +int stm32_firewall_grant_access(struct stm32_firewall *firewall) >> +{ >> + struct stm32_firewall_controller *firewall_controller; >> + >> + if (!firewall || firewall->firewall_id == U32_MAX) >> + return -EINVAL; >> + >> + firewall_controller = firewall->firewall_ctrl; >> + >> + if (!firewall_controller) >> + return -ENODEV; >> + >> + return firewall_controller->grant_access(firewall_controller, firewall->firewall_id); >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_grant_access); >> + >> +int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) >> +{ >> + struct stm32_firewall_controller *firewall_controller; >> + >> + if (!firewall || subsystem_id == U32_MAX || firewall->firewall_id == U32_MAX) >> + return -EINVAL; >> + >> + firewall_controller = firewall->firewall_ctrl; >> + >> + if (!firewall_controller) >> + return -ENODEV; >> + >> + return firewall_controller->grant_access(firewall_controller, subsystem_id); >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_grant_access_by_id); >> + >> +void stm32_firewall_release_access(struct stm32_firewall *firewall) >> +{ >> + struct stm32_firewall_controller *firewall_controller; >> + >> + if (!firewall || firewall->firewall_id == U32_MAX) { >> + pr_err("Incorrect arguments when releasing a firewall access"); >> + return; >> + } >> + >> + firewall_controller = firewall->firewall_ctrl; >> + >> + if (!firewall_controller) { >> + pr_debug("No firewall controller to release"); >> + return; >> + } >> + >> + firewall_controller->release_access(firewall_controller, firewall->firewall_id); >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_release_access); >> + >> +void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) >> +{ >> + struct stm32_firewall_controller *firewall_controller; >> + >> + if (!firewall || subsystem_id == U32_MAX || firewall->firewall_id == U32_MAX) { >> + pr_err("Incorrect arguments when releasing a firewall access"); >> + return; >> + } >> + >> + firewall_controller = firewall->firewall_ctrl; >> + >> + if (!firewall_controller) { >> + pr_debug("No firewall controller to release"); >> + return; >> + } >> + >> + firewall_controller->release_access(firewall_controller, subsystem_id); >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_release_access_by_id); >> + >> +/* Firewall controller API */ >> + >> +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller) >> +{ >> + pr_info("Registering firewall controller %s", dev_name(firewall_controller->dev)); >> + >> + if (!firewall_controller) >> + return -ENODEV; >> + >> + mutex_lock(&firewall_controller_list_lock); >> + list_add_tail(&firewall_controller->entry, &firewall_controller_list); >> + mutex_unlock(&firewall_controller_list_lock); >> + >> + return 0; >> + >> +} >> + >> +void stm32_firewall_controller_unregister(struct stm32_firewall_controller *firewall_controller) >> +{ >> + struct stm32_firewall_controller *ctrl, *tmp; >> + bool controller_removed = false; >> + >> + if (!firewall_controller) { >> + pr_debug("Null reference while unregistering firewall controller"); >> + return; >> + } >> + >> + mutex_lock(&firewall_controller_list_lock); >> + list_for_each_entry_safe(ctrl, tmp, &firewall_controller_list, entry) { >> + if (ctrl == firewall_controller) { >> + controller_removed = true; >> + list_del_init(&ctrl->entry); >> + break; >> + } >> + } >> + mutex_unlock(&firewall_controller_list_lock); >> + >> + if (!controller_removed) >> + pr_debug("There was no firewall controller named %s to unregister", >> + dev_name(firewall_controller->dev)); >> +} >> + >> +void stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller) >> +{ >> + struct device_node *child; >> + struct device *parent; >> + u32 firewall_id; >> + int err; >> + >> + parent = firewall_controller->dev; >> + >> + dev_dbg(parent, "Populating %s system bus\n", dev_name(firewall_controller->dev)); >> + >> + for_each_available_child_of_node(dev_of_node(parent), child) { >> + err = stm32_firewall_get_id(child, &firewall_id); >> + if (err < 0 || >> + firewall_controller->grant_access(firewall_controller, firewall_id)) { >> + /* >> + * Peripheral access not allowed or not defined. >> + * Mark the node as populated so platform bus won't probe it >> + */ >> + of_node_set_flag(child, OF_POPULATED); >> + dev_err(parent, "%s: Device driver will not be probed\n", >> + child->full_name); >> + } >> + } >> +} >> diff --git a/drivers/bus/stm32_firewall.h b/drivers/bus/stm32_firewall.h >> new file mode 100644 >> index 000000000000..8d92e8c1ab77 >> --- /dev/null >> +++ b/drivers/bus/stm32_firewall.h >> @@ -0,0 +1,83 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved >> + */ >> + >> +#ifndef _STM32_FIREWALL_H >> +#define _STM32_FIREWALL_H >> + >> +#include <linux/kernel.h> >> +#include <linux/list.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/types.h> >> + >> +/** >> + * STM32_PERIPHERAL_FIREWALL: This type of firewall protects peripherals >> + * STM32_MEMORY_FIREWALL: This type of firewall protects memories/subsets of memory >> + * zones >> + * STM32_RESOURCE_FIREWALL: This type of firewall protects internal resources >> + * STM32_NOTYPE_FIREWALL: Undefined firewall type >> + */ >> + >> +#define STM32_PERIPHERAL_FIREWALL BIT(1) >> +#define STM32_MEMORY_FIREWALL BIT(2) >> +#define STM32_RESOURCE_FIREWALL BIT(3) >> +#define STM32_NOTYPE_FIREWALL BIT(4) >> + >> +/** >> + * struct stm32_firewall_controller - Information on firewall controller supplying services >> + * >> + * @name Name of the firewall controller >> + * @dev Device reference of the firewall controller >> + * @mmio Base address of the firewall controller >> + * @entry List entry of the firewall controller list >> + * @type Type of firewall >> + * @max_entries Number of entries covered by the firewall >> + * @grant_access Callback used to grant access for a device access against a >> + * firewall controller >> + * @release_access Callback used to release resources taken by a device when access was >> + * granted >> + * @grant_memory_range_access Callback used to grant access for a device to a given memory region >> + */ >> +struct stm32_firewall_controller { >> + const char *name; >> + struct device *dev; >> + void __iomem *mmio; >> + struct list_head entry; >> + unsigned int type; >> + unsigned int max_entries; >> + >> + int (*grant_access)(struct stm32_firewall_controller *ctrl, u32 id); >> + void (*release_access)(struct stm32_firewall_controller *ctrl, u32 id); >> + int (*grant_memory_range_access)(struct stm32_firewall_controller *ctrl, phys_addr_t paddr, >> + size_t size); >> +}; >> + >> +/** >> + * int stm32_firewall_controller_register - Register a firewall controller to the STM32 firewall >> + * framework >> + * @firewall_controller Firewall controller to register >> + * >> + * Returns 0 in case of success or -ENODEV if no controller was given. >> + */ >> +int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller); >> + >> +/** >> + * int stm32_firewall_controller_unregister - Unregister a firewall controller from the STM32 >> + * firewall framework >> + * @firewall_controller Firewall controller to unregister >> + */ >> +void stm32_firewall_controller_unregister(struct stm32_firewall_controller *firewall_controller); >> + >> +/** >> + * stm32_firewall_populate_bus - Populate device tree nodes that have a correct firewall >> + * configuration. This is used at boot-time only, as a sanity check >> + * between device tree and firewalls hardware configurations to >> + * prevent a kernel crash when a device driver is not granted access >> + * >> + * @firewall_controller Firewall controller which nodes will be populated or not >> + */ >> +void stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller); >> + >> +#endif /* _STM32_FIREWALL_H */ >> diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h >> new file mode 100644 >> index 000000000000..ccaecea7fc6c >> --- /dev/null >> +++ b/include/linux/bus/stm32_firewall_device.h >> @@ -0,0 +1,134 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved >> + */ >> + >> +#ifndef STM32_FIREWALL_DEVICE_H >> +#define STM32_FIREWALL_DEVICE_H >> + >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/types.h> >> + >> +#define STM32_FIREWALL_MAX_EXTRA_ARGS 5 >> + >> +/* Opaque reference to stm32_firewall_controller */ >> +struct stm32_firewall_controller; >> + >> +/** >> + * stm32_firewall - Information on a device's firewall. Each device can have more than one firewall. >> + * >> + * @firewall_ctrl Pointer referencing a firewall controller of the device. It is >> + * opaque so a device cannot manipulate the controller's ops or access >> + * the controller's data >> + * @extra_args Extra arguments that are implementation dependent >> + * @extra_args_size Number of extra arguments >> + * @firewall_id Firewall ID associated the device for this firewall controller >> + */ >> +struct stm32_firewall { >> + struct stm32_firewall_controller *firewall_ctrl; >> + u32 extra_args[STM32_FIREWALL_MAX_EXTRA_ARGS]; >> + size_t extra_args_size; >> + u32 firewall_id; >> +}; >> + >> +#if IS_ENABLED(CONFIG_STM32_FIREWALL) >> +/** >> + * stm32_firewall_get_firewall - Get the firewall(s) associated to given device. >> + * The firewall controller reference is always the first argument >> + * of the feature-domains property. >> + * The firewall ID is always the second argument of the >> + * feature-domains property. >> + * >> + * @np Device node to parse >> + * @firewall Resulting firewall reference(s) >> + * >> + * Returns 0 on success, -ENODEV if there's no match with a firewall controller or appropriate errno >> + * code if error occurred. >> + */ >> +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall); >> + >> +/** >> + * stm32_firewall_grant_access - Request firewall access rights and grant access. >> + * >> + * @firewall Firewall reference containing the ID to check against its firewall >> + * controller >> + * >> + * Returns 0 if access is granted, -EACCES if access is denied, -ENODEV if firewall is null or >> + * appropriate errno code if error occurred >> + */ >> +int stm32_firewall_grant_access(struct stm32_firewall *firewall); >> + >> +/** >> + * stm32_firewall_release_access - Release access granted from a call to >> + * stm32_firewall_grant_access(). >> + * >> + * @firewall Firewall reference containing the ID to check against its firewall >> + * controller >> + */ >> +void stm32_firewall_release_access(struct stm32_firewall *firewall); >> + >> +/** >> + * stm32_firewall_grant_access_by_id - Request firewall access rights of a given device >> + * based on a specific firewall ID >> + * >> + * Warnings: >> + * There is no way to ensure that the given ID will correspond to the firewall referenced in the >> + * device node if the ID did not come from stm32_firewall_get_firewall(). In that case, this >> + * function must be used with caution. >> + * This function should be used for subsystem resources that do not have the same firewall ID >> + * as their parent. >> + * U32_MAX is an invalid ID. >> + * >> + * @firewall Firewall reference containing the firewall controller >> + * @subsystem_id Firewall ID of the subsystem resource >> + * >> + * Returns 0 if access is granted, -EACCES if access is denied, -ENODEV if firewall is null or >> + * appropriate errno code if error occurred >> + */ >> +int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id); >> + >> +/** >> + * stm32_firewall_release_access_by_id - Release access granted from a call to >> + * stm32_firewall_grant_access_by_id(). >> + * >> + * Warnings: >> + * There is no way to ensure that the given ID will correspond to the firewall referenced in the >> + * device node if the ID did not come from stm32_firewall_get_firewall(). In that case, this >> + * function must be used with caution. >> + * This function should be used for subsystem resources that do not have the same firewall ID >> + * as their parent. >> + * U32_MAX is an invalid ID. >> + * >> + * @firewall Firewall reference containing the firewall controller >> + * @subsystem_id Firewall ID of the subsystem resource >> + */ >> +void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id); >> + >> +#else /* CONFIG_STM32_FIREWALL */ >> + >> +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall) >> +{ >> + return -ENODEV; >> +} >> + >> +int stm32_firewall_grant_access(struct stm32_firewall *firewall) >> +{ >> + return -ENODEV; >> +} >> + >> +void stm32_firewall_release_access(struct stm32_firewall *firewall) >> +{ >> +} >> + >> +int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) >> +{ >> + return -ENODEV; >> +} >> + >> +void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id) >> +{ >> +} >> + >> +#endif /* CONFIG_STM32_FIREWALL */ >> +#endif /* STM32_FIREWALL_DEVICE_H */ >> -- >> 2.25.1 >>
On Fri, Jul 07, 2023 at 03:43:15PM +0200, Gatien CHEVALLIER wrote: > > > On 7/6/23 17:09, Rob Herring wrote: > > On Wed, Jul 05, 2023 at 07:27:54PM +0200, Gatien Chevallier wrote: > > > Introduce a firewall framework that offers to firewall consumers different > > > firewall services such as the ability to check their access rights against > > > their firewall controller(s). > > > > > > The firewall framework offers a generic API that is defined in firewall > > > controllers drivers to best fit the specificity of each firewall. > > > > > > There are various types of firewalls: > > > -Peripheral firewalls that filter accesses to peripherals > > > -Memory firewalls that filter accesses to memories or memory regions > > > -Resource firewalls that filter accesses to internal resources such as > > > reset and clock controllers > > > > How do resource firewalls work? Access to registers for some clocks in a > > clock controller are disabled? Or something gates off clocks/resets to > > a block? > > To take a practical example: > > A clock controller can be firewall-aware and have its own firewall registers > to configure. To access a clock/reset that is handled this way, a device > would need to check this "resource firewall". I thought that for these kinds > of hardware blocks, having a common API would help. We already have the concept of 'protected clocks' which are ones controlled by secure mode which limits what Linux can do with them. I think you should extend this mechanism if needed and use the existing clock/reset APIs for managing resources. > > > > It might make more sense for "resource" accesses to be managed within > > those resource APIs (i.e. the clock and reset frameworks) and leave this > > framework to bus accesses. > > > > Okay, I'll drop this for V2 if you find that the above explaination do not > justify this. > > > > A firewall controller must be probed at arch_initcall level and register > > > to the framework so that consumers can use their services. > > > > initcall ordering hacks should not be needed. We have both deferred > > probe and fw_devlinks to avoid that problem. > > > > Greg also doubts this. > > Drivers like reset/clock controllers drivers (core_initcall level) will have > a dependency on the firewall controllers in order to initialize their > resources. I was not sure how to manage these dependencies. > > Now, looking at init/main.c, I've realized that core_initcall() level comes > before arch_initcall() level... > > If managed by fw_devlink, the feature-domains property should be supported > as well I suppose? I'm not sure how to handle this properly. I'd welcome > your suggestion. DT parent/child child dependencies are already handled which might be enough for you. Otherwise, adding a new provider/consumer binding is a couple of lines to add the property names. See drivers/of/property.c. > > > Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> > > > --- > > > MAINTAINERS | 5 + > > > arch/arm64/Kconfig.platforms | 1 + > > > drivers/bus/Kconfig | 10 + > > > drivers/bus/Makefile | 1 + > > > drivers/bus/stm32_firewall.c | 252 ++++++++++++++++++++++ > > > drivers/bus/stm32_firewall.h | 83 +++++++ > > > > Why something stm32 specific? We know there are multiple platforms > > wanting something in this area. Wasn't the last attempt common? > > > > For a common binding, I'm not eager to accept anything new with only 1 > > user. > > > > Last attempt was common for the feature-domain bindings. The system-bus > driver was ST-specific. I don't know if other platforms needs this kind > of framework. Are you suggesting that this framework should be generic? Or > that this framework should have a st-specific property? Ah right, the posting for SCMI device permissions was the binding only. The binding should be generic and support more than 1 user. That somewhat implies a generic framework, but not necessarily. > I've oriented this firewall framework to serve ST purpose. There may be a > need for other platforms but I'm not sure that this framework serves them > well. One can argue that it is quite minimalist and covers basic purposes of > a hardware firewall but I would need more feedback from other vendors to > submit it as a generic one. We already know there are at least 2 users. Why would we make the 2nd user refactor your driver into a common framework? [...] > > > +int stm32_firewall_get_firewall(struct device_node *np, > > > + struct stm32_firewall *firewall) > > > +{ > > > + struct stm32_firewall_controller *ctrl; > > > + struct of_phandle_args args; > > > + u32 controller_phandle; > > > + bool match = false; > > > + size_t i; > > > + int err; > > > + > > > + if (!firewall) > > > + return -EINVAL; > > > + > > > + /* The controller phandle is always the first argument of the feature-domains property. */ > > > + err = of_property_read_u32(np, "feature-domains", &controller_phandle); > > > > Why do you need to parse the property twice? > > > > The first parsing is to have the first argument, which is the controller > phandle. The second parsing is here to get the firewall arguments based on > the number of arguments defined by #feature-domain-cells. Maybe using > of_property_read_u32_array() would be better. No. It's not a u32 array. It's a phandle+args property, so you should only use phandle+args APIs. > I did not want to close the > door for supporting several feature domain controllers, hence multiple > phandles. of_parse_phandle_with_args() seemed fine for this purpose but the > phandle is parsed out. There's an iterator for handling multiple phandle+args cases. Rob
Hello Rob, On 7/7/23 17:07, Rob Herring wrote: > On Fri, Jul 07, 2023 at 03:43:15PM +0200, Gatien CHEVALLIER wrote: >> >> >> On 7/6/23 17:09, Rob Herring wrote: >>> On Wed, Jul 05, 2023 at 07:27:54PM +0200, Gatien Chevallier wrote: >>>> Introduce a firewall framework that offers to firewall consumers different >>>> firewall services such as the ability to check their access rights against >>>> their firewall controller(s). >>>> >>>> The firewall framework offers a generic API that is defined in firewall >>>> controllers drivers to best fit the specificity of each firewall. >>>> >>>> There are various types of firewalls: >>>> -Peripheral firewalls that filter accesses to peripherals >>>> -Memory firewalls that filter accesses to memories or memory regions >>>> -Resource firewalls that filter accesses to internal resources such as >>>> reset and clock controllers >>> >>> How do resource firewalls work? Access to registers for some clocks in a >>> clock controller are disabled? Or something gates off clocks/resets to >>> a block? >> >> To take a practical example: >> >> A clock controller can be firewall-aware and have its own firewall registers >> to configure. To access a clock/reset that is handled this way, a device >> would need to check this "resource firewall". I thought that for these kinds >> of hardware blocks, having a common API would help. > > We already have the concept of 'protected clocks' which are ones > controlled by secure mode which limits what Linux can do with them. I > think you should extend this mechanism if needed and use the existing > clock/reset APIs for managing resources. > Ok, thank you for the input. I'll remove this type of firewall for V2 as I no longer have a use case. >>> >>> It might make more sense for "resource" accesses to be managed within >>> those resource APIs (i.e. the clock and reset frameworks) and leave this >>> framework to bus accesses. >>> >> >> Okay, I'll drop this for V2 if you find that the above explaination do not >> justify this. >> >>>> A firewall controller must be probed at arch_initcall level and register >>>> to the framework so that consumers can use their services. >>> >>> initcall ordering hacks should not be needed. We have both deferred >>> probe and fw_devlinks to avoid that problem. >>> >> >> Greg also doubts this. >> >> Drivers like reset/clock controllers drivers (core_initcall level) will have >> a dependency on the firewall controllers in order to initialize their >> resources. I was not sure how to manage these dependencies. >> >> Now, looking at init/main.c, I've realized that core_initcall() level comes >> before arch_initcall() level... >> >> If managed by fw_devlink, the feature-domains property should be supported >> as well I suppose? I'm not sure how to handle this properly. I'd welcome >> your suggestion. > > DT parent/child child dependencies are already handled which might be > enough for you. Otherwise, adding a new provider/consumer binding is a > couple of lines to add the property names. See drivers/of/property.c. > Ok, I'll try with a modification of drivers/of/property.c as the parent/child dependency won't be enough. Thanks for pointing this out. >>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> >>>> --- >>>> MAINTAINERS | 5 + >>>> arch/arm64/Kconfig.platforms | 1 + >>>> drivers/bus/Kconfig | 10 + >>>> drivers/bus/Makefile | 1 + >>>> drivers/bus/stm32_firewall.c | 252 ++++++++++++++++++++++ >>>> drivers/bus/stm32_firewall.h | 83 +++++++ >>> >>> Why something stm32 specific? We know there are multiple platforms >>> wanting something in this area. Wasn't the last attempt common? >>> >>> For a common binding, I'm not eager to accept anything new with only 1 >>> user. >>> >> >> Last attempt was common for the feature-domain bindings. The system-bus >> driver was ST-specific. I don't know if other platforms needs this kind >> of framework. Are you suggesting that this framework should be generic? Or >> that this framework should have a st-specific property? > > Ah right, the posting for SCMI device permissions was the binding only. > The binding should be generic and support more than 1 user. That somewhat > implies a generic framework, but not necessarily. > >> I've oriented this firewall framework to serve ST purpose. There may be a >> need for other platforms but I'm not sure that this framework serves them >> well. One can argue that it is quite minimalist and covers basic purposes of >> a hardware firewall but I would need more feedback from other vendors to >> submit it as a generic one. > > We already know there are at least 2 users. Why would we make the 2nd > user refactor your driver into a common framework? > > [...] > If one thinks this framework is generic enough so it can be of use for them, so yes, I can submit it as a common framework. I'm not that sure Oleksii finds a use case with it. He seemed interested by the bindings. Maybe I'm wrong Oleksii? For V2, I'd rather submit it again as an ST-specific framework again to address the generic comments. This way, other people have time to manifest themselves. >>>> +int stm32_firewall_get_firewall(struct device_node *np, >>>> + struct stm32_firewall *firewall) >>>> +{ >>>> + struct stm32_firewall_controller *ctrl; >>>> + struct of_phandle_args args; >>>> + u32 controller_phandle; >>>> + bool match = false; >>>> + size_t i; >>>> + int err; >>>> + >>>> + if (!firewall) >>>> + return -EINVAL; >>>> + >>>> + /* The controller phandle is always the first argument of the feature-domains property. */ >>>> + err = of_property_read_u32(np, "feature-domains", &controller_phandle); >>> >>> Why do you need to parse the property twice? >>> >> >> The first parsing is to have the first argument, which is the controller >> phandle. The second parsing is here to get the firewall arguments based on >> the number of arguments defined by #feature-domain-cells. Maybe using >> of_property_read_u32_array() would be better. > > No. It's not a u32 array. It's a phandle+args property, so you should > only use phandle+args APIs. > >> I did not want to close the >> door for supporting several feature domain controllers, hence multiple >> phandles. of_parse_phandle_with_args() seemed fine for this purpose but the >> phandle is parsed out. > > There's an iterator for handling multiple phandle+args cases. > > Rob Ok, will look into that for V2. Best regards, Gatien
Hello Gatien, Gatien CHEVALLIER <gatien.chevallier@foss.st.com> writes: > Hello Rob, > > On 7/7/23 17:07, Rob Herring wrote: >> On Fri, Jul 07, 2023 at 03:43:15PM +0200, Gatien CHEVALLIER wrote: >>> >>> >>> On 7/6/23 17:09, Rob Herring wrote: >>>> On Wed, Jul 05, 2023 at 07:27:54PM +0200, Gatien Chevallier wrote: >>>>> Introduce a firewall framework that offers to firewall consumers different >>>>> firewall services such as the ability to check their access rights against >>>>> their firewall controller(s). >>>>> >>>>> The firewall framework offers a generic API that is defined in firewall >>>>> controllers drivers to best fit the specificity of each firewall. >>>>> >>>>> There are various types of firewalls: >>>>> -Peripheral firewalls that filter accesses to peripherals >>>>> -Memory firewalls that filter accesses to memories or memory regions >>>>> -Resource firewalls that filter accesses to internal resources such as >>>>> reset and clock controllers >>>> >>>> How do resource firewalls work? Access to registers for some clocks in a >>>> clock controller are disabled? Or something gates off clocks/resets to >>>> a block? >>> >>> To take a practical example: >>> >>> A clock controller can be firewall-aware and have its own firewall registers >>> to configure. To access a clock/reset that is handled this way, a device >>> would need to check this "resource firewall". I thought that for these kinds >>> of hardware blocks, having a common API would help. >> We already have the concept of 'protected clocks' which are ones >> controlled by secure mode which limits what Linux can do with them. I >> think you should extend this mechanism if needed and use the existing >> clock/reset APIs for managing resources. >> > > Ok, thank you for the input. I'll remove this type of firewall for V2 as > I no longer have a use case. > >>>> >>>> It might make more sense for "resource" accesses to be managed within >>>> those resource APIs (i.e. the clock and reset frameworks) and leave this >>>> framework to bus accesses. >>>> >>> >>> Okay, I'll drop this for V2 if you find that the above explaination do not >>> justify this. >>> >>>>> A firewall controller must be probed at arch_initcall level and register >>>>> to the framework so that consumers can use their services. >>>> >>>> initcall ordering hacks should not be needed. We have both deferred >>>> probe and fw_devlinks to avoid that problem. >>>> >>> >>> Greg also doubts this. >>> >>> Drivers like reset/clock controllers drivers (core_initcall level) will have >>> a dependency on the firewall controllers in order to initialize their >>> resources. I was not sure how to manage these dependencies. >>> >>> Now, looking at init/main.c, I've realized that core_initcall() level comes >>> before arch_initcall() level... >>> >>> If managed by fw_devlink, the feature-domains property should be supported >>> as well I suppose? I'm not sure how to handle this properly. I'd welcome >>> your suggestion. >> DT parent/child child dependencies are already handled which might >> be >> enough for you. Otherwise, adding a new provider/consumer binding is a >> couple of lines to add the property names. See drivers/of/property.c. >> > > Ok, I'll try with a modification of drivers/of/property.c as the > parent/child dependency won't be enough. Thanks for pointing this out. > >>>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> >>>>> --- >>>>> MAINTAINERS | 5 + >>>>> arch/arm64/Kconfig.platforms | 1 + >>>>> drivers/bus/Kconfig | 10 + >>>>> drivers/bus/Makefile | 1 + >>>>> drivers/bus/stm32_firewall.c | 252 ++++++++++++++++++++++ >>>>> drivers/bus/stm32_firewall.h | 83 +++++++ >>>> >>>> Why something stm32 specific? We know there are multiple platforms >>>> wanting something in this area. Wasn't the last attempt common? >>>> >>>> For a common binding, I'm not eager to accept anything new with only 1 >>>> user. >>>> >>> >>> Last attempt was common for the feature-domain bindings. The system-bus >>> driver was ST-specific. I don't know if other platforms needs this kind >>> of framework. Are you suggesting that this framework should be generic? Or >>> that this framework should have a st-specific property? >> Ah right, the posting for SCMI device permissions was the binding >> only. >> The binding should be generic and support more than 1 user. That somewhat >> implies a generic framework, but not necessarily. >> >>> I've oriented this firewall framework to serve ST purpose. There may be a >>> need for other platforms but I'm not sure that this framework serves them >>> well. One can argue that it is quite minimalist and covers basic purposes of >>> a hardware firewall but I would need more feedback from other vendors to >>> submit it as a generic one. >> We already know there are at least 2 users. Why would we make the >> 2nd >> user refactor your driver into a common framework? >> [...] >> > > If one thinks this framework is generic enough so it can be of use for > them, so yes, I can submit it as a common framework. I'm not that sure > Oleksii finds a use case with it. He seemed interested by the bindings. > Maybe I'm wrong Oleksii? > Correct. I'm interested only in bindings which should be processed by the hypervisor and removed from the OS DT the Kernel running in VM wouldn't know it exists. > For V2, I'd rather submit it again as an ST-specific framework again to > address the generic comments. This way, other people have time to > manifest themselves. > >>>>> +int stm32_firewall_get_firewall(struct device_node *np, [snip] > > Best regards, > Gatien
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
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