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

[Andrew Davis]

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

> 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