Re: [PATCH 05/10] firewall: introduce stm32_firewall framework

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

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