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.
> >
> > 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.
> > > 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?
[...]
> > > +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
