From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-x22c.google.com (mail-qt0-x22c.google.com [IPv6:2607:f8b0:400d:c0d::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 54BD41A1E29 for ; Mon, 17 Oct 2016 06:25:30 -0700 (PDT) Received: by mail-qt0-x22c.google.com with SMTP id q7so120924815qtq.1 for ; Mon, 17 Oct 2016 06:25:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=vwgTQh2eUJZae0uxLGgYb1rDPWw8P8K4lAnbzvQD7iU=; b=PoZbY/yTaNHALHbvNnFHEqKpnv65TpMjgU61i5fkR0oAOep3W9AhMaRMt78RfSf5LG pMTqioXGuegzqfOY82WckvAJyI/CGVQzyFSWKhQe5IewSGzV7HBbrp9F/0v+OicqMqFl IP0+GlhF5wBcEWtgnExlbW975g+78aptvw7b0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=vwgTQh2eUJZae0uxLGgYb1rDPWw8P8K4lAnbzvQD7iU=; b=kC/PEdAzbyUmX0plipmj9E4qpltRD+AamFqeW5nHN8lBrUXJflXESjlB6bVuOpxQ5v PlcZF2CUT3RRdHs99NMb5lLGJxoMhrcwzcmoWJ9KAfuPiF7h52118Dp+sSlmynNB6+Td R4yXZu4oBmVwVZsoZDCLEBKaITtzs6i+0ZpOfYVsLGJO1gpacRtwlx/P5TUWqutXRtfc vZDJCy/+59jFrviEOUBLagEUqU9icWfOq8N7ShgJTLF011w4G/tb3fY9+NZs5ambcxEy 9zjWta+WiSN3yF/aX3olsVWMVi1VdcNoSN9D0vl9zK7b4jjqYfZbLyrl5nRZSm0EvkSL NGlQ== X-Gm-Message-State: AA6/9Rk1/7WO3bqniRGgbMB2OLxxt6y/yzDmJy4YbCfSxINXZRRGadjwxNXYdOvAIybRWVth X-Received: by 10.28.157.84 with SMTP id g81mr7864316wme.29.1476710728153; Mon, 17 Oct 2016 06:25:28 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id w1sm2487465wje.36.2016.10.17.06.25.26 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Oct 2016 06:25:27 -0700 (PDT) Date: Mon, 17 Oct 2016 14:25:25 +0100 From: Leif Lindholm To: Bhupesh Sharma Cc: "edk2-devel@ml01.01.org" , Ard Biesheuvel , "linaro-uefi@lists.linaro.org" , evan.lloyd@arm.com, ryan.harkin@linaro.org Message-ID: <20161017132525.GW3471@bivouac.eciton.net> References: <1476443381-30175-1-git-send-email-bhupesh.sharma@nxp.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Oct 2016 13:25:30 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > Cc: Ard Biesheuvel > > --- > > .../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