Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled

[Bhupesh Sharma <bhupesh.sharma@nxp.com>]

Hi Leif,

Thanks for the review.

Please see my replies in-line.

> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Monday, October 17, 2016 6:55 PM
> 
> On Mon, Oct 17, 2016 at 10:18:01AM +0000, Bhupesh Sharma wrote:
> > Hi Ard, Leif,
> >
> > Any comments on this patch ?
> 
> You didn't cc me before :)
> 
> But more importantly, I don't really have any platform to test this on,
> so I could use a Tested-by: from someone who does. Evan, do you?
> 
> > > From: Bhupesh Sharma [mailto:bhupesh.sharma@nxp.com]
> > > Sent: Friday, October 14, 2016 4:40 PM
> > >
> > > ARM TZASC-380 IP provides a mechanism to split memory regions being
> > > protected via it into eight equal-sized sub-regions, with a bit
> > > setting allowing the corresponding subregion to be disabled.
> > >
> > > Several NXP/FSL SoCs support the TZASC-380 IP block and allow the
> > > DDR connected via the TZASC to be partitioned into regions having
> > > different security settings.
> > >
> > > This patch enables this support and can be used for SoCs which
> > > support such partition of DDR regions.
> > >
> > > Details of the 'subregion_disable'
> 
> This is not actually what the register is called in the link you're
> providing.
> 
> > > register can be viewed here:
> > >
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0431c
> > > /CJ
> > > ABCFHB.html
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@nxp.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  .../Library/ArmVExpressSecLibCTA9x4/CTA9x4Sec.c     | 21
> > > ++++++++++++++-------
> > >  ArmPlatformPkg/Drivers/ArmTrustZone/ArmTrustZone.c  |  5 +++--
> > >  ArmPlatformPkg/Include/Drivers/ArmTrustzone.h       |  3 ++-
> > >  3 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git
> > >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9
> > > x4S
> > > ec.c
> > >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9
> > > x4S
> > > ec.c
> > > index 6fa0774..d358d65 100644
> > > ---
> > >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9
> > > x4S
> > > ec.c
> > > +++
> > >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9
> > > +++ x4Sec.c
> > > @@ -72,18 +72,21 @@ ArmPlatformSecTrustzoneInit (
> > >    // NOR Flash 0 non secure (BootMon)
> > >    TZASCSetRegion(ARM_VE_TZASC_BASE,1,TZASC_REGION_ENABLED,
> > >        ARM_VE_SMB_NOR0_BASE,0,
> > > -      TZASC_REGION_SIZE_64MB, TZASC_REGION_SECURITY_NSRW);
> > > +      TZASC_REGION_SIZE_64MB, TZASC_REGION_SECURITY_NSRW,
> > > +      0);
> > >
> > >    // NOR Flash 1. The first half of the NOR Flash1 must be secure
> > > for the secure firmware (sec_uefi.bin)
> > >    if (PcdGetBool (PcdTrustzoneSupport) == TRUE) {
> > >      //Note: Your OS Kernel must be aware of the secure regions
> > > before to enable this region
> > >      TZASCSetRegion(ARM_VE_TZASC_BASE,2,TZASC_REGION_ENABLED,
> > >          ARM_VE_SMB_NOR1_BASE + SIZE_32MB,0,
> > > -        TZASC_REGION_SIZE_32MB, TZASC_REGION_SECURITY_NSRW);
> > > +        TZASC_REGION_SIZE_32MB, TZASC_REGION_SECURITY_NSRW,
> > > +	0);
> 
> TAB used (convert to spaces).

Ok.
 
> > >    } else {
> > >      TZASCSetRegion(ARM_VE_TZASC_BASE,2,TZASC_REGION_ENABLED,
> > >          ARM_VE_SMB_NOR1_BASE,0,
> > > -        TZASC_REGION_SIZE_64MB, TZASC_REGION_SECURITY_NSRW);
> > > +        TZASC_REGION_SIZE_64MB, TZASC_REGION_SECURITY_NSRW,
> > > +	0);
> 
> TAB used (convert to spaces).

Ok.
 
> > >    }
> > >
> > >    // Base of SRAM. Only half of SRAM in Non Secure world @@ -92,22
> > > +95,26 @@ ArmPlatformSecTrustzoneInit (
> > >      //Note: Your OS Kernel must be aware of the secure regions
> > > before to enable this region
> > >      TZASCSetRegion(ARM_VE_TZASC_BASE,3,TZASC_REGION_ENABLED,
> > >          ARM_VE_SMB_SRAM_BASE,0,
> > > -        TZASC_REGION_SIZE_16MB, TZASC_REGION_SECURITY_NSRW);
> > > +        TZASC_REGION_SIZE_16MB, TZASC_REGION_SECURITY_NSRW,
> > > +	0);
> 
> TAB used (convert to spaces).

Ok.
 
> > >    } else {
> > >      TZASCSetRegion(ARM_VE_TZASC_BASE,3,TZASC_REGION_ENABLED,
> > >          ARM_VE_SMB_SRAM_BASE,0,
> > > -        TZASC_REGION_SIZE_32MB, TZASC_REGION_SECURITY_NSRW);
> > > +        TZASC_REGION_SIZE_32MB, TZASC_REGION_SECURITY_NSRW,
> > > +	0);
> 
> TAB used (convert to spaces).

Ok.
> (These are all found by BaseTools/Scripts/PatchCheck.py, which also
> points out that the subject line is too long.)

Thanks for the pointer. I was looking for a checkpatch.pl kind of
equivalent for EDK2. Looks like I found one now :)

> 
> > >    }
> > >
> > >    // Memory Mapped Peripherals. All in non secure world
> > >    TZASCSetRegion(ARM_VE_TZASC_BASE,4,TZASC_REGION_ENABLED,
> > >        ARM_VE_SMB_PERIPH_BASE,0,
> > > -      TZASC_REGION_SIZE_64MB, TZASC_REGION_SECURITY_NSRW);
> > > +      TZASC_REGION_SIZE_64MB, TZASC_REGION_SECURITY_NSRW,
> > > +      0);
> > >
> > >    // MotherBoard Peripherals and On-chip peripherals.
> > >    TZASCSetRegion(ARM_VE_TZASC_BASE,5,TZASC_REGION_ENABLED,
> > >        ARM_VE_SMB_MB_ON_CHIP_PERIPH_BASE,0,
> > > -      TZASC_REGION_SIZE_256MB, TZASC_REGION_SECURITY_NSRW);
> > > +      TZASC_REGION_SIZE_256MB, TZASC_REGION_SECURITY_NSRW,
> > > +      0);
> > >  }
> > >
> > >  /**
> > > diff --git a/ArmPlatformPkg/Drivers/ArmTrustZone/ArmTrustZone.c
> > > b/ArmPlatformPkg/Drivers/ArmTrustZone/ArmTrustZone.c
> > > index 070c0dc..5cd41ef 100644
> > > --- a/ArmPlatformPkg/Drivers/ArmTrustZone/ArmTrustZone.c
> > > +++ b/ArmPlatformPkg/Drivers/ArmTrustZone/ArmTrustZone.c
> > > @@ -87,7 +87,8 @@ TZASCSetRegion (
> > >    IN  UINTN LowAddress,
> > >    IN  UINTN HighAddress,
> > >    IN  UINTN Size,
> > > -  IN  UINTN Security
> > > +  IN  UINTN Security,
> > > +  IN  UINTN SubregionDisableMask
> > >    )
> > >  {
> > >    UINT32*     Region;
> > > @@ -100,7 +101,7 @@ TZASCSetRegion (
> > >
> > >    MmioWrite32((UINTN)(Region), LowAddress&0xFFFF8000);
> > >    MmioWrite32((UINTN)(Region+1), HighAddress);
> > > -  MmioWrite32((UINTN)(Region+2), ((Security & 0xF) <<28) | ((Size
> &
> > > 0x3F) << 1) | (Enabled & 0x1));
> > > +  MmioWrite32((UINTN)(Region+2), ((Security & 0xF) <<28) |
> > > + ((SubregionDisableMask & 0xFF) << 8) | ((Size & 0x3F) << 1) |
> > > (Enabled
> > > + & 0x1));
> 
> I think these additions tip the code over to where a temporary variable
> should be used for the register value.

Ok. Will change this in V2.
 
> > >
> > >    return EFI_SUCCESS;
> > >  }
> > > diff --git a/ArmPlatformPkg/Include/Drivers/ArmTrustzone.h
> > > b/ArmPlatformPkg/Include/Drivers/ArmTrustzone.h
> > > index 78e98aa..1ba963d 100644
> > > --- a/ArmPlatformPkg/Include/Drivers/ArmTrustzone.h
> > > +++ b/ArmPlatformPkg/Include/Drivers/ArmTrustzone.h
> > > @@ -82,7 +82,8 @@ TZASCSetRegion (
> > >    IN  UINTN LowAddress,
> > >    IN  UINTN HighAddress,
> > >    IN  UINTN Size,
> > > -  IN  UINTN Security
> > > +  IN  UINTN Security,
> > > +  IN  UINTN SubregionDisableMask
> > >    );
> 
> This modifies a prototype of a function implemented in:
> ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9x4Sec
> .c
> ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/RTSMSec.c
> ArmPlatformPkg/Library/ArmPlatformSecLibNull/ArmPlatformLibNullSec.c
> 
> But only modifies the implementation in one of them.
> Arguably, the RTSM one could simply be deleted (probably), but if we're
> keeping this library, then the LibNullSec should at least be kept up to
> date.
> 
> Ard: comments on deleting the RTSM one?
> 
> Regards,
> 
> Leif
> 
> > >
> > >  #endif
> > > --
> > > 1.9.1
> > >
> >
> > Regards,
> > Bhupesh
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel