public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled
@ 2016-10-14 11:09 Bhupesh Sharma
  2016-10-17 10:18 ` Bhupesh Sharma
  0 siblings, 1 reply; 6+ messages in thread
From: Bhupesh Sharma @ 2016-10-14 11:09 UTC (permalink / raw)
  To: edk2-devel; +Cc: linaro-uefi, Bhupesh Sharma, Ard Biesheuvel

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' register can be viewed here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0431c/CJABCFHB.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/CTA9x4Sec.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9x4Sec.c
index 6fa0774..d358d65 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9x4Sec.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA9x4Sec.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);
   } 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);
   }
 
   // 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);
   } 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);
   }
 
   // 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));
 
   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
   );
 
 #endif
-- 
1.9.1




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled
  2016-10-14 11:09 [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled Bhupesh Sharma
@ 2016-10-17 10:18 ` Bhupesh Sharma
  2016-10-17 13:25   ` Leif Lindholm
  0 siblings, 1 reply; 6+ messages in thread
From: Bhupesh Sharma @ 2016-10-17 10:18 UTC (permalink / raw)
  To: edk2-devel@ml01.01.org, Ard Biesheuvel, Leif Lindholm
  Cc: linaro-uefi@lists.linaro.org, Bhupesh Sharma

Hi Ard, Leif,

Any comments on this patch ?

> 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' 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/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);
>    } 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);
>    }
> 
>    // 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);
>    } 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);
>    }
> 
>    // 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));
> 
>    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
>    );
> 
>  #endif
> --
> 1.9.1
> 

Regards,
Bhupesh


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled
  2016-10-17 10:18 ` Bhupesh Sharma
@ 2016-10-17 13:25   ` Leif Lindholm
  2016-10-17 14:15     ` Ard Biesheuvel
  2016-10-27  5:21     ` Bhupesh Sharma
  0 siblings, 2 replies; 6+ messages in thread
From: Leif Lindholm @ 2016-10-17 13:25 UTC (permalink / raw)
  To: Bhupesh Sharma
  Cc: edk2-devel@ml01.01.org, Ard Biesheuvel,
	linaro-uefi@lists.linaro.org, evan.lloyd, ryan.harkin

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/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?

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled
  2016-10-17 13:25   ` Leif Lindholm
@ 2016-10-17 14:15     ` Ard Biesheuvel
  2016-10-27  5:23       ` Bhupesh Sharma
  2016-10-27  5:21     ` Bhupesh Sharma
  1 sibling, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2016-10-17 14:15 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: Bhupesh Sharma, edk2-devel@ml01.01.org,
	linaro-uefi@lists.linaro.org, Evan Lloyd, Ryan Harkin

On 17 October 2016 at 14:25, Leif Lindholm <leif.lindholm@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.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/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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled
  2016-10-17 13:25   ` Leif Lindholm
  2016-10-17 14:15     ` Ard Biesheuvel
@ 2016-10-27  5:21     ` Bhupesh Sharma
  1 sibling, 0 replies; 6+ messages in thread
From: Bhupesh Sharma @ 2016-10-27  5:21 UTC (permalink / raw)
  To: Leif Lindholm, Bhupesh Sharma
  Cc: edk2-devel@ml01.01.org, Ard Biesheuvel,
	linaro-uefi@lists.linaro.org, evan.lloyd@arm.com,
	ryan.harkin@linaro.org

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled
  2016-10-17 14:15     ` Ard Biesheuvel
@ 2016-10-27  5:23       ` Bhupesh Sharma
  0 siblings, 0 replies; 6+ messages in thread
From: Bhupesh Sharma @ 2016-10-27  5:23 UTC (permalink / raw)
  To: Ard Biesheuvel, Leif Lindholm, Bhupesh Sharma
  Cc: edk2-devel@ml01.01.org, linaro-uefi@lists.linaro.org, Evan Lloyd,
	Ryan Harkin

Hi Ard,

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, October 17, 2016 7:46 PM
> 
> On 17 October 2016 at 14:25, Leif Lindholm <leif.lindholm@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.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.ddi0431
> >> > c/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/CTA
> >> > 9x4S
> >> > ec.c
> >> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA
> >> > 9x4S
> >> > ec.c
> >> > index 6fa0774..d358d65 100644
> >> > ---
> >> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA
> >> > 9x4S
> >> > ec.c
> >> > +++
> >> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/CTA
> >> > 9
> >> > +++ 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/CTA9x4Se
> > c.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.

Ok, so should I send these changes for RTSM model also or should I update the LibNullSec.
Please let me know.

Thanks,
Bhupesh

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-10-27  5:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-14 11:09 [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled Bhupesh Sharma
2016-10-17 10:18 ` Bhupesh Sharma
2016-10-17 13:25   ` Leif Lindholm
2016-10-17 14:15     ` Ard Biesheuvel
2016-10-27  5:23       ` Bhupesh Sharma
2016-10-27  5:21     ` Bhupesh Sharma

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox