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

[Manorit Chawdhry]

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)
>                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!

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