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

[Manorit Chawdhry]

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?

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