[RFC PATCH 2/2] binman: j721e: Add firewall configurations for atf

[Andrew Davis]

On 9/8/23 6:43 AM, Manorit Chawdhry wrote:
> Hi Andrew,
> 
> On 08:54-20230906, Andrew Davis wrote:
>> On 9/6/23 12:18 AM, Manorit Chawdhry wrote:
>>> Hi Andrew,
>>>
>>> On 10:22-20230905, Andrew Davis wrote:
>>>> On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
>>>>> The following commits adds the configuration of firewalls required to
>>>>> protect ATF and OP-TEE memory region from non-secure reads and
>>>>> writes using master and slave firewalls present in our K3 SOCs.
>>>>>
>>>>> Signed-off-by: Manorit Chawdhry <m-chawdhry at ti.com>
>>>>> ---
>>>>>     arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 161 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi
>>>>> index 4f566c21a9af..0569a592597e 100644
>>>>> --- a/arch/arm/dts/k3-j721e-binman.dtsi
>>>>> +++ b/arch/arm/dts/k3-j721e-binman.dtsi
>>>>> @@ -330,6 +330,73 @@
>>>>>     					ti-secure {
>>>>>     						content = <&atf>;
>>>>>     						keyfile = "custMpk.pem";
>>>>> +						auth_in_place = <0xa02>;
>>>>> +
>>>>> +						// cpu_0_cpu_0_msmc Background Firewall - 0
>>>>
>>>> I don't have a personal preference, but I see most comments in DTS
>>>> are C style /* */.
>>>>
>>>> Also it might be easier to understand if you put the comments right before
>>>> the properties that they relate to, i.g.
>>>>
>>>> firewall-0 {
>>>> 	/* cpu_0_cpu_0_msmc */
>>>> 	id = <257>;
>>>> 	region = <0>;
>>>> 	/* Background, Locked */
>>>> 	control = <0x31a>;
>>>> 	permissions = <0xc3ffff>;
>>>> 	start_address = <0x0 0x0>;
>>>> 	end_address = <0xff 0xffffffff>;
>>>> };
>>>>
>>>> For permissions, I wonder if it would be easier to read if we add
>>>> some helper macros:
>>>>
>>>> #define FWPRIVID_ALL 0xc3
>>>>
>>>> #define FWPERM_SECURE_PRIV_WRITE      BIT(0)
>>>> #define FWPERM_SECURE_PRIV_READ       BIT(1)
>>>> #define FWPERM_SECURE_PRIV_CACHEABLE  BIT(2)
>>>> #define FWPERM_SECURE_PRIV_DEBUG      BIT(3)
>>>> #define FWPERM_SECURE_USER_WRITE      BIT(4)
>>>> ...
>>>>
>>>> Then you would have:
>>>>
>>>> permissions = <FWPRIVID_ALL |
>>>
>>> I think we'll have to do some shift here for the right calculations.
>>>
>>>     #define FWPRIVID_SHIFT 8
>>>
>>>     permissions = <(FWPRIVID_ALL << FWPRIVID_SHIFT)
>>
>> Right, I must have meant #define FWPRIVID_ALL 0xc30000 above, but
>> a shift looks better.
>>
>>>>                  FWPERM_SECURE_PRIV_WRITE |
>>>>                  FWPERM_SECURE_PRIV_READ |
>>>
>>> Also, I would like to make FWPERM_SECURE_PRIV_RWCD too as it'll be
>>> commonly used.
>>>
>>> Would be sending another RFC with this change if you are fine with the
>>> suggestions. Thank you for reviewing!
>>>
>>
>> Works for me, should get rid of several of these magic numbers.
>>
>> Andrew
> 
> firewall-0 {
>      id = <257>;
>      region = <0>;
>      control = <(FWCTRL_EN | FWCTRL_LOCK |
>                  FWCTRL_BG | FWCTRL_CACHE)>;
>      permissions = <((FWPRIVID_ALL << FWPRIVID_SHIFT) |
>                      FWPERM_SECURE_PRIV_RWCD |
>                      FWPERM_SECURE_USER_RWCD |
>                      FWPERM_NON_SECURE_PRIV_RWCD |
>                      FWPERM_NON_SECURE_USER_RWCD)>;
>      start_address = <0x0 0x0>;
>      end_address = <0xff 0xffffffff>;
> };
> 
> Does this look okay now?
> 

Yes, that can be parsed by humans now, which is much less
error-prone than bitmask'd magic numbers :)

Andrew

> Regards,
> Manorit
> 
>>
>>> Regards,
>>> Manorit
>>>
>>>>                  ...
>>>
>>>
>>>>> ;
>>>>
>>>>> +						firewall-0 {
>>>>> +							id = <257>;
>>>>> +							region = <0>;
>>>>> +							control = <0x31a>;
>>>>> +							permissions = <0xc3ffff>;
>>>>> +							start_address = <0x0 0x0>;
>>>>> +							end_address = <0xff 0xffffffff>;
>>>>> +						};
>>>>> +
>>>>> +						// cpu_0_cpu_0_msmc Foreground Firewall
>>>>> +						firewall-1 {
>>>>> +							id = <257>;
>>>>> +							region = <1>;
>>>>> +							control = <0x1a>;
>>>>> +							permissions = <0x0100ff>;
>>>>> +							start_address = <0x0 0x70000000>;
>>>>
>>>> This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR?
>>>> Not sure how you would get the end address as we don't really know ATF size..
>>>>
>>>> Andrew