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

[Gatien CHEVALLIER <gatien.chevallier@foss.st.com>]

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@foss.st.com>
>>>>>>> ---
>>>>>>>
>>>>>>> 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