From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0613.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe1f::613]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 72F341A1DFF for ; Wed, 26 Oct 2016 22:21:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=LqGPFUvGRs8qT9QJCrvy5r9M2s6RJbpW0wXRGo6ADLw=; b=saY9rWiXp4v9RQOaERVVvxeJpnZzb+wwxbexHkCpT30PTSGz8N0osbma8c7ITxtdSB+YlKn0hobxknUgZUee6p9+FoaLdyds9BcQjzc9T6PTh0gGk3sBUsagjh/AkmbiKIzjgZP+UA3iLUaOomxy0O1+pEmMevNV5ZXjJVLt/vQ= Received: from AM4PR0401MB2289.eurprd04.prod.outlook.com (10.165.45.12) by AM4PR0401MB2290.eurprd04.prod.outlook.com (10.165.45.13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.669.16; Thu, 27 Oct 2016 05:21:27 +0000 Received: from AM4PR0401MB2289.eurprd04.prod.outlook.com ([10.165.45.12]) by AM4PR0401MB2289.eurprd04.prod.outlook.com ([10.165.45.12]) with mapi id 15.01.0669.024; Thu, 27 Oct 2016 05:21:28 +0000 From: Bhupesh Sharma 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" Thread-Topic: [edk2] [RESEND PATCH 1/1] ArmPlatformPkg/ArmTrustZone: Add support for specifying Subregions to be disabled Thread-Index: AQHSJg0TdnbQqmbe1UKsTtgYkyg4c6CscvEQgAA0y4CADy9GkA== Date: Thu, 27 Oct 2016 05:21:27 +0000 Message-ID: References: <1476443381-30175-1-git-send-email-bhupesh.sharma@nxp.com> <20161017132525.GW3471@bivouac.eciton.net> In-Reply-To: <20161017132525.GW3471@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=bhupesh.sharma@nxp.com; x-originating-ip: [192.88.169.1] x-ms-office365-filtering-correlation-id: 89054116-cc20-42bf-6434-08d3fe291a7b x-microsoft-exchange-diagnostics: 1; AM4PR0401MB2290; 7:rqqUxpG1fWUZIaF2sfy9Acd0XzRxqlquWhb9xUl0M4kymGuZGNEce29yWuboATTumV1606u+C4Bx2oItdlMe+8LZ6gLrgwEptQUqRTBbAqoW1p9djI3dLsqS/AfiAH6/bAOEyjS23zzm+bz1VO+IwdxLiDYzgHpFLLyvbcjtFRiyaHCKq9zj/m8VCutphiN9ZteybM/3R+I7oO0ndeDtPsD8UuNKNUpG/inALWJlt8f8ZNlxbB1QoKL9loXKhWKKIUK5lpScJOquiFPnWykSDxYakawGjvb9OZ++B9QSyHno27y7nNSuzEAa/mS+kLNKfMy38ZxMblyNB57wDcxDjgz8Gu8NjtQe+BV/dDuWhVM= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM4PR0401MB2290; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(192374486261705)(185117386973197)(162533806227266); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040176)(6045074)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026)(6046074)(6072074); SRVR:AM4PR0401MB2290; BCL:0; PCL:0; RULEID:; SRVR:AM4PR0401MB2290; x-forefront-prvs: 0108A997B2 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(377454003)(199003)(24454002)(189002)(51914003)(86362001)(2900100001)(3660700001)(92566002)(7696004)(575784001)(105586002)(50986999)(106356001)(106116001)(54356999)(33656002)(76576001)(101416001)(5001770100001)(97736004)(189998001)(15975445007)(2950100002)(8936002)(76176999)(5660300001)(305945005)(586003)(6116002)(10400500002)(3846002)(5002640100001)(77096005)(74316002)(9686002)(19580405001)(66066001)(68736007)(19580395003)(3280700002)(7846002)(7736002)(4326007)(87936001)(81156014)(2906002)(81166006)(8676002)(102836003)(122556002)(217873001)(19627235001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR0401MB2290; H:AM4PR0401MB2289.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2016 05:21:27.9629 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0401MB2290 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: Thu, 27 Oct 2016 05:21:32 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 >=20 > On Mon, Oct 17, 2016 at 10:18:01AM +0000, Bhupesh Sharma wrote: > > Hi Ard, Leif, > > > > Any comments on this patch ? >=20 > You didn't cc me before :) >=20 > 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? >=20 > > > 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' >=20 > This is not actually what the register is called in the link you're > providing. >=20 > > > register can be viewed here: > > > > http://infocenter.arm.com/help/index.jsp?topic=3D/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/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) =3D=3D 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); >=20 > TAB used (convert to spaces). Ok. =20 > > > } 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); >=20 > TAB used (convert to spaces). Ok. =20 > > > } > > > > > > // 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); >=20 > TAB used (convert to spaces). Ok. =20 > > > } 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); >=20 > 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 :) >=20 > > > } > > > > > > // 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)); >=20 > 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. =20 > > > > > > 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 > > > ); >=20 > 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 >=20 > 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. >=20 > Ard: comments on deleting the RTSM one? >=20 > Regards, >=20 > Leif >=20 > > > > > > #endif > > > -- > > > 1.9.1 > > > > > > > Regards, > > Bhupesh > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel