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-chawd...@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
