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@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 - 0I 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 Firewallfirewall-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