Message-ID: <87a5vzdhmv.fsf@epam.com>
Date: Thu, 13 Jul 2023 14:13:26 +0000
From: Oleksii Moisieiev <Oleksii_Moisieiev@...m.com>
To: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
CC: Rob Herring <robh@...nel.org>,
        "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>,
        "herbert@...dor.apana.org.au"
	<herbert@...dor.apana.org.au>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "alexandre.torgue@...s.st.com"
	<alexandre.torgue@...s.st.com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "olivier.moysan@...s.st.com"
	<olivier.moysan@...s.st.com>,
        "arnaud.pouliquen@...s.st.com"
	<arnaud.pouliquen@...s.st.com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "fabrice.gasnier@...s.st.com" <fabrice.gasnier@...s.st.com>,
        "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
        "ulf.hansson@...aro.org"
	<ulf.hansson@...aro.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "hugues.fruchet@...s.st.com" <hugues.fruchet@...s.st.com>,
        "lee@...nel.org"
	<lee@...nel.org>,
        "will@...nel.org" <will@...nel.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "arnd@...nel.org"
	<arnd@...nel.org>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com"
	<linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "dmaengine@...r.kernel.org"
	<dmaengine@...r.kernel.org>,
        "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>,
        "linux-iio@...r.kernel.org"
	<linux-iio@...r.kernel.org>,
        "alsa-devel@...a-project.org"
	<alsa-devel@...a-project.org>,
        "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>,
        "linux-mmc@...r.kernel.org"
	<linux-mmc@...r.kernel.org>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>,
        "linux-serial@...r.kernel.org"
	<linux-serial@...r.kernel.org>,
        "linux-spi@...r.kernel.org"
	<linux-spi@...r.kernel.org>,
        "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>,
        Peng Fan <peng.fan@....nxp.com>
Subject: Re: [PATCH 05/10] firewall: introduce stm32_firewall framework


Hello Gatien,

Gatien CHEVALLIER <gatien.chevallier@...s.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@...s.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

Powered by blists - more mailing lists