Re: [PATCH 05/10] firewall: introduce stm32_firewall framework
From: Gatien CHEVALLIER
Date: Thu Jul 13 2023 - 09:59:04 EST
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@xxxxxxxxxxx>
---
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