Messages in this thread | | | Date | Mon, 27 Nov 2023 14:46:51 +0100 | From | Gatien CHEVALLIER <> | Subject | Re: [PATCH v6 10/11] ARM: dts: stm32: add ETZPC as a system bus for STM32MP15x boards |
| |
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
| |