On 17 October 2016 at 14:25, Leif Lindholm <leif.lindh...@linaro.org> wrote: > 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.sha...@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.sha...@nxp.com> >> > Cc: Ard Biesheuvel <ard.biesheu...@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/CTA9x4S >> > ec.c >> > b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9x4S >> > ec.c >> > index 6fa0774..d358d65 100644 >> > --- >> > a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9x4S >> > 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). > >> > } 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). > >> > } >> > >> > // 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). > >> > } 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). > > (These are all found by BaseTools/Scripts/PatchCheck.py, which also > points out that the subject line is too long.) > >> > } >> > >> > // 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. > >> > >> > 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? >
Nope. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
