From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22f.google.com (mail-it0-x22f.google.com [IPv6:2607:f8b0:4001:c0b::22f]) (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 A055B1A1DEF for ; Mon, 17 Oct 2016 07:16:01 -0700 (PDT) Received: by mail-it0-x22f.google.com with SMTP id 66so9310724itl.1 for ; Mon, 17 Oct 2016 07:16:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=CbijoSSUhMkvX0zJg9bf5T5tBgGCMssUVMa7wJYOnVI=; b=g41BmiJJKVTiT8NiBY0vDKNPsID0u3PFMX0X5Q3oZV5Ar81ucVLr3AQv5We3rPzxIs H6gwelSJ08+jNELRN218Pku1KbfHY+ZystvvPNOmMdGe95D0eZIyv3p7/tjQus8NC2Tk 2rEj/hBlE4mnrixfjxaMIhMOpt5KkVFhl8XT8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=CbijoSSUhMkvX0zJg9bf5T5tBgGCMssUVMa7wJYOnVI=; b=Y4WW7fdaiwIiNJ/SNqyo0Dr28y7rgQFXwN1nGedsv+3UxMkF762zhkJY6p8LVaNI4n SnZSVXKpH86hByNKdzxz6qFZ9wCOW1d5EiakR8/0M3EytWkVQeIWDzpjEnRfRBJ+LyGt DyHVLxi/W8uJaZ9lgR8M35JHzhkBJE+8Al92t6phMVLeX22aatGZboj7fwzqGAo2VJD+ r8HBVmCEh6PBifaWrSDyma3zdBJXMZFlYDX5LghYLHSYgfnEFI8SWIIkX05R0fuJ88S6 8+GFAPKNZUHvfgxZwEN8pET0qTirc2S/9akmhhHAVbquMraeuC6WI/z9MGJoZQVJFLmX Wj9g== X-Gm-Message-State: AA6/9Rk7m/At3TmH92NOOfTS7m2hsAmWEFUD4YMd4xFvSCM12v2eO35ANO7ZreSkEwUeyTODJuvybh7F+c/1POev X-Received: by 10.36.5.65 with SMTP id 62mr8566914itl.63.1476713759823; Mon, 17 Oct 2016 07:15:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.5.139 with HTTP; Mon, 17 Oct 2016 07:15:59 -0700 (PDT) In-Reply-To: <20161017132525.GW3471@bivouac.eciton.net> References: <1476443381-30175-1-git-send-email-bhupesh.sharma@nxp.com> <20161017132525.GW3471@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 17 Oct 2016 15:15:59 +0100 Message-ID: To: Leif Lindholm Cc: Bhupesh Sharma , "edk2-devel@ml01.01.org" , "linaro-uefi@lists.linaro.org" , Evan Lloyd , Ryan Harkin 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 14:16:01 -0000 Content-Type: text/plain; charset=UTF-8 On 17 October 2016 at 14:25, Leif Lindholm 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 >> > 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? > Nope.