Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(STM32) ETZPC firewall controller updates #7066

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

GseoC
Copy link
Contributor

@GseoC GseoC commented Oct 2, 2024

This P-R modifies the ETZPC driver so that the firewall configuration of the peripheral managed by ETZPC can be described in the device tree. This increases flexibility and allows per platform firewall configuration.

Moreover, add an ETZPC firewall bus probing mechanism that checks if the peripherals that are under the ETZPC and used in OP-TEE are secured before adding their drivers to the probe list. This is a protection against unsafe use of non secure peripherals in the secure world.

Some device tree cleaning as well.

@GseoC
Copy link
Contributor Author

GseoC commented Oct 2, 2024

Hi @jneuhauser,
do you mind testing (even reviewing) this series please? I think dhcom boards will fail to boot with this patchset and without a minimalist ETZPC configuration. I'm thinking of adding at least:

&etzpc {
	st,decprot =
		<&etzpc DECPROT(STM32MP1_ETZPC_TT_FDCAN_ID, DECPROT_S_RW, DECPROT_UNLOCK)>;
};

to the board device tree files since you seem to use the CAN peripherals in OP-TEE.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For commit "dts: stm32: default disabled DMA at SoC level for stm32mp15 platforms":
Fix the commit message header line: s/disabled/disable/ and prefer an explicit message body: s/This node/DMA node in stm32mp15* SoC DTSI files/

Having an explicit message body (not requiring to read the header line for understand which part is changes) is a comment that applies to most of the commits in this series.

For commit "dts: stm32: disable ADC2 on stm32mp135f-dk":
in commit message: the change removes the ADC2 node configuration (incl. enable). The commit message should reflect that, something like: "Remove ADC2 configuration in stm32mp135-dk.dts since OP-TEE does not use the device".

For commit "dts: stm32: disable VREFBUF on stm32mp15x platforms"
Prefer an explicit message! "VREFBUF is currently not used on stm32mp15-dk boards**, so disable it." Also fix the header line: s/stm32mp15x/stm32mp15 DKx/".

Few other comments, see below.

core/drivers/stm32_rng.c Show resolved Hide resolved
core/include/dt-bindings/firewall/stm32mp15-etzpc.h Outdated Show resolved Hide resolved
core/include/dt-bindings/firewall/stm32mp13-etzpc.h Outdated Show resolved Hide resolved
core/arch/arm/dts/stm32mp131.dtsi Outdated Show resolved Hide resolved
core/arch/arm/dts/stm32mp157a-dk1-scmi.dts Outdated Show resolved Hide resolved
core/drivers/stm32_etzpc.c Outdated Show resolved Hide resolved
core/arch/arm/plat-stm32mp1/main.c Outdated Show resolved Hide resolved
core/arch/arm/plat-stm32mp1/main.c Outdated Show resolved Hide resolved
core/include/drivers/stm32_etzpc.h Outdated Show resolved Hide resolved
core/include/drivers/stm32_etzpc.h Outdated Show resolved Hide resolved
@etienne-lms
Copy link
Contributor

I missed to post a comment on commit "drivers: stm32_etzpc: new driver to use firewall API":
There are many changes in stm32_etzpc.c when it is moved from core/drivers/ to core/drivers/firewall/. Could it be possible to split to change in 2? 1 commit that moves the source file location (possibly with minor changes, that Git UIs/diff can show) and 1 that makes all the other changes (field/variable/function names etc...). If that's too tricky, I'm fine, tell me and I'll make my way to check and review the changes.

@GseoC
Copy link
Contributor Author

GseoC commented Oct 8, 2024

@etienne-lms Comments addressed in the new series but I didn't put the patches on top as I had to reorganise the commits. I added a fix for RNG as well and a patch for DH flavorlist.

@jneuhauser, let me know if that's ok for you. Peripheral should probe thanks to the simple-bus compatible but there will be no ETZPC config set. Maybe you do want to add an ETZPC configuration, we can discuss about that.

@GseoC
Copy link
Contributor Author

GseoC commented Oct 9, 2024

@jneuhauser, I took the liberty to set an ETZPC config on your boards that reflects what was hard-coded before. DDR resources/STGENC/BKPSRAM are secure. All the rest is non-secure.

I see that you use non-securable peripherals in OP-TEE. Therefore, I've introduced a CFG_STM32_ALLOW_UNSAFE_PROBE switch so they can still be probed and used by OP-TEE but note that disabling this or CFG_INSECURE will result in boot issues on your boards.

Let me know if that's ok. This series will also solve a compile and boot issue on mp15x platforms

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"dts: stm32: default disable DMA at SoC level for stm32mp15 platforms",
"dts: stm32: disable ADC2 on stm32mp135f-dk",
"dts: stm32: disable VREFBUF on stm32mp15-dkx platforms",
"dts: stm32: define ETZPC as an access controller for stm32mp15 platforms" (with a minor comment) and
"dts: stm32: define ETZPC as an access controller for stm32mp13 platforms".
"plat-stm32mp1: default enable CFG_DRIVERS_FIREWALL".

For commit "dts: stm32: use st,stm32mp15-i2c-non-secure compatible for the I2C4", I need to chek the stm32_i2c driver.
(edited: look ok to me, my R-b tag in #7066 (comment))

For commit "dt-bindings: add platform specific ETZPC bindings": few review comments not addressed.

Comments for commit "dts: stm32: add the ETZPC configuration table for stm32mp1x boards".

core/arch/arm/dts/stm32mp157c-ed1-scmi.dts Show resolved Hide resolved
core/arch/arm/dts/stm32mp157c-dk2-scmi.dts Show resolved Hide resolved
core/arch/arm/dts/stm32mp157a-dk1-scmi.dts Show resolved Hide resolved
core/arch/arm/dts/stm32mp131.dtsi Show resolved Hide resolved
core/arch/arm/dts/stm32mp151.dtsi Show resolved Hide resolved
core/arch/arm/dts/stm32mp151.dtsi Show resolved Hide resolved
@etienne-lms
Copy link
Contributor

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"drivers: stm32_etzpc: move the stm32_etzpc driver to the firewall folder" and
"plat-stm32mp1: add CFG_STM32_ALLOW_UNSAFE_PROBE to probe unsafe peripherals".

@etienne-lms
Copy link
Contributor

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"dts: stm32: use st,stm32mp15-i2c-non-secure compatible for the I2C4".

@GseoC
Copy link
Contributor Author

GseoC commented Oct 10, 2024

Given the comments were straight forward, I didn't bother with additional review patches. I addressed and squashed them.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"dt-bindings: add platform specific ETZPC bindings",
"dts: stm32: add the ETZPC configuration table for stm32mp1x boards",
"drivers: stm32_etzpc: update driver to set ETZPC configuration from DT",
"drivers: stm32_etzpc: new driver to use firewall API" and
"drivers: stm32_rng: embed ETZPC functions when CFG_STM32_ETZPC is set".

Regarding "drivers: stm32_etzpc: update driver to set ETZPC configuration from DT", now that the peripheral secure hardening configuration is defined by the DT, we will have to remove call to stm32mp_register_[non_]secure_periph_iomem() calls in platform and driver since they are not really useful.

@GseoC
Copy link
Contributor Author

GseoC commented Oct 14, 2024

Tags applied, thanks

@jneuhauser
Copy link
Contributor

Hello and sorry for the long delay...

Unfortunately, I don't have time for an indepth review at the moment.

The changes related to our boards and the other boards are ok, so Acked-by: Johann Neuhauser <jneuhauser@dh-electronics.com> for commit "dts: stm32: add the ETZPC configuration table for stm32mp1x boards".

I wonder if it makes more sense to define the firewall configuration in the files stm32mp15xx-dhcor-som.dtsi and stm32mp15xx-dhcom-som.dtsi, which represent the "System on Module" soldered or attached to the baseboard?

Regards, Johann

DMA node in stm32mp15* SoC DTSI files shouldn't be enabled by default,
we don't even have a driver to handle it. Therefore default disable it.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Remove ADC2 configuration in stm32mp135-dk.dts since OP-TEE does not
use the device.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
VREFBUF is currently not used on stm32mp15-dkx platforms,
so disable it.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Use st,stm32mp15-i2c-non-secure compatible for the I2C4 as it is
currently non-secure on stm32mp15 dkx and evx platforms.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Define ETZPC bindings for STM32MP15 and STM32MP13 and add these
header files into the stm32mp_dt_bindings helper. While there, also
update some includes to fix the path errors.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Add the tables defining the ETZPC firewall controller configuration
that will be set at boot time on stm32mp1x boards.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Acked-by: Johann Neuhauser <jneuhauser@dh-electronics.com>
ETZPC is a firewall controller. Add the access-controllers property to
all ETZPC sub-nodes on stm32mp15x platforms. Also add the "simple-bus"
compatible for backward compatibility and "#access-controllers-cells"
to the ETZPC node.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
ETZPC is a firewall controller. Add the access-controllers property to
all ETZPC sub-nodes on stm32mp13 platforms. Also add the "simple-bus"
compatible for backward compatibility and "#access-controllers-cells"
to the ETZPC node.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Default enable the CFG_DRIVERS_FIREWALL switch that is used to enable
the support of the firewall framework.

On this platform, only the ETZPC is a firewall controller for now.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Remove old implementation where the ETZPC configuration was a hard
coded table in the shared resources file and use the device tree to
get it.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
The ETZPC is a firewall controller. Therefore, move the stm32_etzpc driver
to the firewall folder.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
…herals

Add CFG_STM32_ALLOW_UNSAFE_PROBE that allows to unsafely probe
peripherals. This means that the firewall configuration will not be
checked before probing a peripheral. Default enable this switch for
DH platforms that use non-securable peripherals in OP-TEE.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Implement stm32_etzpc.c driver in the firewall driver directory.
Use the new firewall API to populate the firewall bus and register
the ETZPC as a firewall provider.

Implement a driver specific firewall bus probe that will
only probe secure peripherals and implement firewall exceptions for
which no firewall operations will be done when CFG_INSECURE is set.
This allows, for example, to share a console with the non-secure world
for development purposes.

The ETZPC driver register the following ops:
-set_conf
-acquire_access
-acquire_memory_access

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
On platforms when CFG_STM32_ETZPC is disabled, ETZPC cannot be
interrogated to get decprot attributes. Therefore do not embed ETZPC
related code.

While there, revert commit 326382a ("drivers: stm32_rng: MP15 RNG is
non-secure when PRNG is enable") and prefer to use ETZPC API.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Fixes: d773ec0 ("drivers: stm32_rng: update clock and power management")
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
@GseoC
Copy link
Contributor Author

GseoC commented Oct 21, 2024

@jneuhauser 's tag applied. I also addressed your comment and placed ETZPC config in the dhcom/dhcor dtsi files.

@etienne-lms , when rebasing, I reverted the 326382a059a8 ("drivers: stm32_rng: MP15 RNG is non-secure when PRNG is enable") patch and squashed it in the last. I kept your tag, let me know if that's ok

@etienne-lms
Copy link
Contributor

Looks all good to me. All review tags are applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants