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

[Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>]

Hello Gatien,

Gatien CHEVALLIER <gatien.chevallier@foss.st.com> writes:

> 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?
>

Correct. I'm interested only in bindings which should be processed by
the hypervisor and removed from the OS DT the Kernel running in VM wouldn't
know it exists.

> 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,

[snip]

>
> Best regards,
> Gatien


-- 
Thanks,
Oleksii