Re: [PATCH v6 10/11] ARM: dts: stm32: add ETZPC as a system bus for STM32MP15x boards

Gatien CHEVALLIER <gatien.chevallier@xxxxxxxxxxx> · Mon, 27 Nov 2023 14:46:51 +0100

Hi,

A gentle reminder on the questions below.

I'm also thinking about moving the STM32 firewall framework to a
specific access-controllers folder if that's ok.

Best regards,
Gatien

On 10/27/23 17:37, Gatien CHEVALLIER wrote:

On 10/24/23 18:39, Rob Herring wrote:
On Mon, Oct 16, 2023 at 02:02:39PM +0200, Gatien CHEVALLIER wrote:
Hi Rob,

On 10/12/23 17:30, Rob Herring wrote:
On Wed, Oct 11, 2023 at 10:49:58AM +0200, Gatien CHEVALLIER wrote:
Hi Rob,

On 10/10/23 20:42, Rob Herring wrote:
On Tue, Oct 10, 2023 at 02:57:18PM +0200, Gatien Chevallier wrote:
ETZPC is a firewall controller. Put all peripherals filtered by the
ETZPC as ETZPC subnodes and reference ETZPC as an
access-control-provider.

For more information on which peripheral is securable or supports 
MCU
isolation, please read the STM32MP15 reference manual.

Signed-off-by: Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
---

Changes in V6:
           - Renamed access-controller to access-controllers
           - Removal of access-control-provider property

Changes in V5:
           - Renamed feature-domain* to access-control*

    arch/arm/boot/dts/st/stm32mp151.dtsi  | 2756 
+++++++++++++------------
    arch/arm/boot/dts/st/stm32mp153.dtsi  |   52 +-
    arch/arm/boot/dts/st/stm32mp15xc.dtsi |   19 +-
    3 files changed, 1450 insertions(+), 1377 deletions(-)

This is not reviewable. Change the indentation and any non-functional
change in one patch and then actual changes in another.

Ok, I'll make it easier to read.

This is also an ABI break. Though I'm not sure it's avoidable. All 
the
devices below the ETZPC node won't probe on existing kernel. A
simple-bus fallback for ETZPC node should solve that.

I had one issue when trying with a simple-bus fallback that was the
drivers were probing even though the access rights aren't correct.
Hence the removal of the simple-bus compatible in the STM32MP25 patch.

But it worked before, right? So the difference is you have either added
new devices which need setup or your firmware changed how devices are
setup (or not setup). Certainly can't fix the latter case. You just 
need
to be explicit about what you are doing to users.

I should've specified it was during a test where I deliberately set
incorrect rights on a peripheral and enabled its node to see if the
firewall would allow the creation of the device.

Even though a node is tagged with the OF_POPULATED flag when checking
the access rights with the firewall controller, it seems that when
simple-bus is probing, there's no check of this flag.

It shouldn't. Those flags are for creating the devices (or not) and
removing only devices of_platform_populate() created.

About the "simple-bus" being a fallback, I think I understood why I saw
that the devices were created.

All devices under a node whose compatible is "simple-bus" are created
in of_platform_device_create_pdata(), called by
of_platform_default_populate_init() at arch_initcall level. This
before the firewall-controller has a chance to populate it's bus.

Therefore, when I flag nodes when populating the firewall-bus, the
devices are already created. The "simple-bus" mechanism is not a
fallback here as it precedes the driver probe.

Is there a safe way to safely remove/disable a device created this way?

There's 2 ways to handle this. Either controlling creating the device or
controlling probing the device. The latter should just work with
fw_devlink dependency. The former probably needs some adjustment to
simple-pm-bus driver if you have 'simple-bus' compatible. You want it to
probe on old kernels and not probe on new kernels with your firewall
driver. Look at the commit history for simple-pm-bus. There was some
discussion on it as well.

Hi Rob,

First, thank you for your suggestions.

Regarding controlling probing the device: the philosophy of the firewall
controller was to check a device secure configuration to determine if
its associated driver should be probed (+handle some firewall
resources). I'd rather avoid it so that the device isn't created at all.

I took a look on the simple-bus driver side. I don't see an obvious way
on how to do it as the firewall controller driver is a module while the
devices being populated is done at arch initcall level.

I ended up with two propositions:

1)I took a shot at implementing a new flag "OF_ACCESS_GRANTED" that
should be set in the first call of the of_platform_bus_create()
function for every child node of a "default bus" (simple-bus,
simple-pm-bus, ...) having the access-controllers property.
This flag should be unset by the access controller if the access is
not granted. This covers the particular case where the access controller
has a simple-bus fallback whilst not creating the devices on the first
try for the bus' childs.

This way, the first round of of_platform_populate() done at arch init
call level won't create the devices of an access controller child
nodes. Then, the firewall controller has a chance to clear the flag
before the second call to this function by the simple-pm-bus driver.

If the controller module isn't present, then it's a simple-bus
behavior to extent of the child devices not being all created in the
first place. This shouldn't be an issue as in only concerns childs
of such bus that aren't probed before the bus driver.

I have a patch that I can send as RFC on top of my series if my
explanation isn't clear enough.

2)Make the STM32_FIREWALL configuration switch select the OF_DYNAMIC
one. This way I can use of_detach_node() function to remove the node
from the device tree. The cons of this is the device tree is now
used at runtime.

Are you considering one of these two proposition as a viable solution?

Best regards,
Gatien

Devices that are under the firewall controller (simple-bus) node
should not be probed before it as they're child of it.

fw_devlink should take care of parent/child dependencies without any
explicit handling of the access ctrl binding.

Rob