From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM04-BN3-obe.outbound.protection.outlook.com (NAM04-BN3-obe.outbound.protection.outlook.com [40.107.68.104]) by mx.groups.io with SMTP id smtpd.web11.4082.1596655011448362233 for ; Wed, 05 Aug 2020 12:16:51 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=GegfROk6; spf=pass (domain: microsoft.com, ip: 40.107.68.104, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YTi94NOWU6gCP6sv2UAemlrT1wJ3tRlNTUphyBpND+d4VAw5a4GtqmNEen77B4qyUBGGT/0WgUVydGS1W652lJmDCFRwicEOP7Jf3B71050suYppk/HvaN0OfavJlWwnZ+GNcAZ+bTMswZyTzmgyl/4hKDWNiAwBjgpQkQPLXYowCKpASMibD9al8lo8sInFqPH79NLsXui+p+PeYD3SXrXisgJryYvsYGaQKtK1QfmLWIKHVGR+tMAHOF5nyzyqkMe7YMShYMc9PnaR0Uc7r4vcc0LSK+TiqA/PdLxUdnX8O3t5z14jJbBcV9zur/DQiUEM2M2HKUvehhfT8TE07Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=x/sSdDAYrrj5cYV05JCK9lXBtff9V8fcFr6DldWHNvI=; b=a6PHf0fhjKcyhWkEpsAPFWt6x2iYSuwJSvqyfLOkbqKDNqiDfse4B6xrOK72PxdQDDWnzCVVv31852/vm3n8AEKLA6WdQm/cFx5FiU1sVQajE1ezRQWmWBExXVvo56cwXAinpmSGNjZuZaZen3n9lL4Cw+m3hPTbaewA8Iu27hrmAbd6IUW2BwFSACBph/UsrfXDM4qnizasnoUgpW8nXcsPsejGyyMSJdaO18ZqUTUEqqjyQAHjBV0xii9dLw8DwlciyrEj1ItTzwDTYZy6W/LPnsfLwsjfvrcXL4smBwod/Pjq7qqrxCSncpzjVkeIPgyfKpPjV2Q+YGLbZrPUdw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=x/sSdDAYrrj5cYV05JCK9lXBtff9V8fcFr6DldWHNvI=; b=GegfROk6KomllrlSccp9iq6cHl1ybdeX6IDJ/bHAN4l2NQEZzoGs4s8rZud5hCOK56S+xFhHEHR4pIg+RUrx2cMm1VhxpxVmu2xPDI0+OgD3LiDJ5rst7xqWp4OQBeRuKKyrnO7vuHmkq8Hf7gwrvnpPYuDjeIGJjXvZiaSnKZ8= Received: from CY4PR21MB0743.namprd21.prod.outlook.com (2603:10b6:903:b2::9) by CY4PR21MB0136.namprd21.prod.outlook.com (2603:10b6:903:b2::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3261.0; Wed, 5 Aug 2020 19:16:48 +0000 Received: from CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::2ca0:7d3e:e918:c47a]) by CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::2ca0:7d3e:e918:c47a%11]) with mapi id 15.20.3283.003; Wed, 5 Aug 2020 19:16:48 +0000 From: "Bret Barkelew" To: "devel@edk2.groups.io" , "leif@nuviainc.com" , Pankaj Bansal CC: Meenakshi Aggarwal , "devel@edk2.groups.io" , Ard Biesheuvel Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM Thread-Topic: [EXTERNAL] Re: [edk2-devel] [PATCH edk2-platforms 2/3] Silicon/NXP: Add support for reserving a chunk from RAM Thread-Index: AQHWazrWQQmyopobekO3AkeozbdBr6kp40wk Date: Wed, 5 Aug 2020 19:16:48 +0000 Message-ID: References: <20200708051933.8123-1-pankaj.bansal@oss.nxp.com> <20200708051933.8123-3-pankaj.bansal@oss.nxp.com>,<20200805151222.GP31778@vanye> In-Reply-To: <20200805151222.GP31778@vanye> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2020-08-05T19:16:10.7803034Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=microsoft.com; x-originating-ip: [174.21.66.92] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 8e3aef72-9e49-418e-7915-08d839741a16 x-ms-traffictypediagnostic: CY4PR21MB0136: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: bbCuC+K+zMDV6A47IutgsCkid07OP7XEv0gwodep+90Pd6DxXPBN+sz+9kWy4E5rDO1a7YuhN7EKO1KgHBaeWnveS/FufXK93bAN0tCbJeLxRKGxkJOV2H3jLnYCbOdk2bi/+NOJ4oxaRikwPNnUXSfVEmUAON3XlJ25YzH/IQ3qsbxPM2ChxXAVzikVAA+jXW0Ey+9AF+hoe0scdzfT0Pyea0UClZzKOtXyBQTOOQl+iM7PmnKKUU6PLwE0CTefrbVDRO5QFwsJ/KmLTR0k5HiYKbe2lvcb2zf4YrcG3vlS5ccM4Cqg3GXtrJja16pgWHlCln54o8OkyqnXxoIXCBOjQXi1BA3p6JE6WlQEgqQXFAmDrnPLOymLUI6GqM6n7tEVl8MJxfzrfIa1/C6fyQ== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CY4PR21MB0743.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(136003)(346002)(39860400002)(366004)(376002)(396003)(30864003)(10290500003)(52536014)(55016002)(66946007)(66556008)(64756008)(66476007)(86362001)(8676002)(76116006)(166002)(478600001)(9686003)(966005)(8936002)(2906002)(66446008)(83380400001)(8990500004)(316002)(26005)(33656002)(19627235002)(7696005)(4326008)(186003)(54906003)(6506007)(110136005)(82950400001)(82960400001)(71200400001)(5660300002)(53546011);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: gFbneC3okyL21l9nTQtwsfaGUeyI4nLm5SJdA5hpMRvzE42m13B+01sYSvpnvbbDSeobpwAC1wB9+jflb1i9qDeBIXGk9RF08lDnPl9TVJxZ+RgXVkYSIas+kHF1GQZEDFuEqbTDWYiQPcJ1yDqhO/6Ot3P5AzvpnPslQvogSV7TwBvIpVd3RbWlL/xCegmx7mhUyusiUtxmE6Zt1U9jQ81psYHkbsHXuZeJIDLGaULPpEv657joB3QBIQ3drqbgW6Vs/9E7kpZIt7PILyThsJsHndy4I5OHPYCuZFLW00IRTnJRXGYt9Dfnja1fZByF8vW7NuPtwQJ1r+H5l/W41O81daplqAExWf7FtYBC1Bt/4539n8HVnR8IIYcL8h0yKeT2sId/b0eCMD1rE0CHU99M+2Se/WAPKKAc2E9MMoKyKdT0lekuvh/rbAW5DrjaFcSDYS9GCL7nM60o94eCuc2UDA/dqorvy2a6QYDvSujq33k1rJfgE/19eTSRypYPKOCmvW+ZM98BgUFWjmYqk44F9NwHhscNECum5cwFoPZkuT9URNTHdngAnl1Qo/Mo7szZvGqp6ndJdCgHrL4xvFKXwMgLMFQEnt1SZLI00hHGXBB0bgbpD9mGJkLta/Gei8yp+dqCxPMguuV8mb+Guw== x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CY4PR21MB0743.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8e3aef72-9e49-418e-7915-08d839741a16 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Aug 2020 19:16:48.7945 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ToRWoL0EEBw3Qd/2oTjtHMLmvBQZuS8mUytZxu/dMGJ/aRbNhE7Tou0gKxZx7TcsYROBqy27hTxDwikez/1Ppg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR21MB0136 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CY4PR21MB07433F2464A8F96834F66ED4EF4B0CY4PR21MB0743namp_" --_000_CY4PR21MB07433F2464A8F96834F66ED4EF4B0CY4PR21MB0743namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Agreed with your gut feeling. - Bret From: Leif Lindholm via groups.io Sent: Wednesday, August 5, 2020 8:12 AM To: Pankaj Bansal Cc: Meenakshi Aggarwal; devel@edk2.grou= ps.io; Ard Biesheuvel Subject: [EXTERNAL] Re: [edk2-devel] [PATCH edk2-platforms 2/3] Silicon/NX= P: Add support for reserving a chunk from RAM On Wed, Jul 08, 2020 at 00:19:32 -0500, Pankaj Bansal wrote: > From: Pankaj Bansal > > Some NXP SOCs have some specialized IP blocks (like MC), which > require DDR memory to operate. This DDR memory should not be managed > by OS or UEFI. > > Moreover to ensure that these IP blocks always get memory, and maximum > contiguous RAM is available for UEFI and OS to use, add the support for > reserving a chunk from RAM before reporting available RAM to UEFI. I can't shake the feeling this code has the wrong level of complexity. It's reserving *one* memory window, at a given alignment. > Signed-off-by: Pankaj Bansal > --- > Silicon/NXP/NxpQoriqLs.dec | 10 ++ > Silicon/NXP/LX2160A/LX2160A.dsc.inc | 4 + > Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf | 3 + > Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c | 142 +++++++= ++++++++++++- > 4 files changed, 157 insertions(+), 2 deletions(-) > > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec > index 188a9fe1f382..0e762066e547 100644 > --- a/Silicon/NXP/NxpQoriqLs.dec > +++ b/Silicon/NXP/NxpQoriqLs.dec > @@ -41,3 +41,13 @@ [PcdsDynamic.common] > gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x000006= 00 > gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601 > gNxpQoriqLsTokenSpaceGuid.PcdPciHideRootPort|FALSE|BOOLEAN|0x00000602 > + > + # Reserved RAM Base address alignment. This number ought to be Power = of two > + # in case no alignment is needed, this number should be 1. (As has been pointed out to me in the past, 1 is also a power of 2...) > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x1|UINT64|0x000006= 03 > + # Size of the RAM to be reserved. This RAM region is neither reported= to UEFI > + # nor to OS "Reported" is an inaccurate description of the mechanism involved. > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x00000604 > + # Reserved RAM Base address which is calculated based on PcdReservedM= emSize > + # and PcdReservedMemAlignment > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase|0x0|UINT64|0x00000605 Why use a Pcd for something that is calculated at runtime and needs to be used immediately? Moreover, I see this Pcd getting set, but I then don't see it getting used ever? > diff --git a/Silicon/NXP/LX2160A/LX2160A.dsc.inc b/Silicon/NXP/LX2160A/L= X2160A.dsc.inc > index 43e361464c8e..755ca169f213 100644 > --- a/Silicon/NXP/LX2160A/LX2160A.dsc.inc > +++ b/Silicon/NXP/LX2160A/LX2160A.dsc.inc > @@ -29,6 +29,10 @@ [PcdsDynamicDefault.common] > gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x6200000 > gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xC0C0000 > > +[PcdsDynamicHii] > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|L"ReservedMemAlignm= ent"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|L"ReservedMemSize"|gEfiG= lobalVariableGuid|0x0|0x20000000|NV,BS > + And here we get Hii involved, but I see no connection to a user interface form. > [PcdsFixedAtBuild.common] > gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x23A0000 > gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x2390000 > diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf b= /Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf > index a33f8cd3f743..ed23a86b43d9 100644 > --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf > +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf > @@ -49,6 +49,9 @@ [FixedPcd] > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase > + gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize > > [Depex] > TRUE > diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c b/S= ilicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c > index 11d1f1260b35..b416323a4ced 100644 > --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c > +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c > @@ -118,6 +118,127 @@ GetDramRegionsInfo ( > return EFI_BUFFER_TOO_SMALL; > } > > +/** > + Calculate the base address of Reserved RAM. > + Reserved RAM is not reported to either UEFI or OS. Here is the word "reported" again. A region is included in the memory map or not. If we are setting parts of the memory map aside, then we should do that by marking them as EfiReservedMemoryType. > + > + @param[in, out] DramRegions Array of type DRAM_REGION_INFO. The size= of this > + array must be one more (+ 1) than the ma= ximum > + regions supported on platform. This is b= ecause, > + if due to Reserved RAM alignment require= ments a > + hole is created in any DRAM region, then= the RAM > + after hole gets reported to UEFI and the= n > + subsequently to OS. which is why, the la= st entry > + of this array will not be parsed while > + calculating Reserved RAM base address. C= aller > + must ensure that last entry of this arra= y is zero > + initialized. > + @param[in] NumRegions Size of DramRegions array (including +1 = for hole) > + @param[in] ReservedMemSize Size of RAM to be reserved. > + > + @return if successful Address of the Reserved RAM region, 0 otherwis= e. > +**/ > +STATIC > +UINTN > +CalculateReservedMemBase ( > + IN DRAM_REGION_INFO *DramRegions, > + IN UINT32 NumRegions, > + IN UINTN ReservedMemSize > +) > +{ > + UINTN ReservedMemAlignment; > + EFI_PHYSICAL_ADDRESS AlignmentMask; > + UINTN RegionBaseAddress; > + UINTN RegionSize; > + UINTN ReservedBaseAddress; > + INTN Index; > + INTN Index2; > + > + ReservedMemAlignment =3D PcdGet64 (PcdReservedMemAlignment); > + // > + // Compute alignment bit mask > + // > + if (ReservedMemAlignment) { > + AlignmentMask =3D LShiftU64 (1, LowBitSet64(ReservedMemAlignment)) = - 1; > + } else { > + AlignmentMask =3D 0; > + } > + > + // The DRAM region info is sorted based on the RAM address is SOC mem= ory map. > + // i.e. DramRegions[0] is at lower address, as compared to DramRegion= s[1]. > + // The goal to start from last region is to find the topmost RAM regi= on that > + // can contain Reserved RAM region i.e. PcdReservedMemSize. > + // Since this RAM is not reported to either UEFI or OS, This ensures = that > + // maximum amount of lower RAM (32 bit addresses) are left > + // for OS to allocate to devices that can only work with 32bit physic= al > + // addresses. E.g. legacy devices that need to DMA to 32bit addresses= . > + for (Index =3D NumRegions - 2; Index >=3D0; Index--) { > + RegionBaseAddress =3D DramRegions[Index].BaseAddress; > + RegionSize =3D DramRegions[Index].Size; > + > + if (ReservedMemSize > RegionSize) { > + continue; > + } > + > + ReservedBaseAddress =3D (RegionBaseAddress + RegionSize - ReservedM= emSize); > + ReservedBaseAddress &=3D (~AlignmentMask); > + if (ReservedBaseAddress < RegionBaseAddress) { > + continue; > + } > + > + // found the region from which reserved mem is to be carved out > + // Need to modify the region size and create/delete region if need = be > + > + RegionSize -=3D ReservedMemSize; > + if (RegionSize =3D=3D 0) { > + // delete the region but maintain the sorted list of regions > + for (Index2 =3D Index; Index2 < NumRegions; Index2++) { > + CopyMem ( > + &DramRegions[Index2], > + &DramRegions[Index2 + 1], > + sizeof (DRAM_REGION_INFO) > + ); > + } > + break; > + } > + > + if (ReservedBaseAddress - RegionBaseAddress) { > + DramRegions[Index].Size =3D ReservedBaseAddress - RegionBaseAddre= ss; > + RegionSize -=3D DramRegions[Index].Size; > + } else { > + DramRegions[Index].BaseAddress =3D ReservedBaseAddress + Reserved= MemSize; > + DramRegions[Index].Size =3D RegionSize; > + RegionSize =3D 0; > + } > + > + if (RegionSize =3D=3D 0) { > + break; > + } > + > + // A hole has been created in DRAM regions due to Reserved RAM alig= nment > + // requirements. create a new DRAM region for DRAM memory after hol= e. > + // Maintain the sorted list of regions > + for (Index2 =3D NumRegions; Index2 > (Index + 1); Index2--) { > + CopyMem ( > + &DramRegions[Index2], > + &DramRegions[Index2 - 1], > + sizeof (DRAM_REGION_INFO) > + ); > + } > + DramRegions[Index2].BaseAddress =3D ReservedBaseAddress + ReservedM= emSize; > + DramRegions[Index2].Size =3D RegionSize; > + RegionSize =3D 0; > + > + break; > + } > + > + if (Index =3D=3D -1) { > + return 0; > + } else { > + return ReservedBaseAddress; > + } > +} > + > /** > Get the installed RAM information. > Initialize Memory HOBs (Resource Descriptor HOBs) > @@ -135,7 +256,9 @@ MemoryInitPeiLibConstructor ( > UINTN BaseAddress; > UINTN Size; > UINTN Top; > - DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS]; > + // Extra region gets created if we want to reserve a memory region an= d that > + // creates a memory hole because of alignment requirements > + DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS + 1]; No. Just no. > EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > UINTN FdBase; > UINTN FdTop; > @@ -155,6 +278,21 @@ MemoryInitPeiLibConstructor ( > > (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions)); > > + // Get the reserved memory size from non volatile storage > + Size =3D PcdGet64 (PcdReservedMemSize); > + if (Size) { > + BaseAddress =3D CalculateReservedMemBase ( > + DramRegions, > + ARRAY_SIZE (DramRegions), > + Size > + ); > + if (BaseAddress) { > + DEBUG ((DEBUG_INFO, "ReservedMem: start 0x%lx, size 0x%lx\n", > + BaseAddress, Size)); > + PcdSet64S (PcdReservedMemBase, BaseAddress); PcdReservedMemBase set but never used. And how do the other controllers find out about what region has ended up being set aside for them? At what point during the boot process do they need it? / Leif > + } > + } > + > FdBase =3D (UINTN)PcdGet64 (PcdFdBaseAddress); > FdTop =3D FdBase + (UINTN)PcdGet32 (PcdFdSize); > > @@ -168,7 +306,7 @@ MemoryInitPeiLibConstructor ( > // This ensures that maximum amount of lower RAM (32 bit addresses) a= re left > // for OS to allocate to devices that can only work with 32bit physic= al > // addresses. E.g. legacy devices that need to DMA to 32bit addresses= . > - for (Index =3D MAX_DRAM_REGIONS - 1; Index >=3D 0; Index--) { > + for (Index =3D MAX_DRAM_REGIONS; Index >=3D 0; Index--) { > if (DramRegions[Index].Size =3D=3D 0) { > continue; > } > -- > 2.17.1 > --_000_CY4PR21MB07433F2464A8F96834F66ED4EF4B0CY4PR21MB0743namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Agreed with your gut feeling.

 

- Bret

 

From: Leif Lindholm via groups.io
Sent: Wednesday, August 5, 2020 8:12 AM
To: Pankaj Bansal<= br> Cc: Meenakshi Aggarwa= l; devel@edk2.groups.io; Ard= Biesheuvel
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH edk2-platforms 2/3] Sil= icon/NXP: Add support for reserving a chunk from RAM

 

On Wed, Jul 08, 2020= at 00:19:32 -0500, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
>
> Some NXP SOCs have some specialized IP blocks (like MC), which
> require DDR memory to operate. This DDR memory should not be managed<= br> > by OS or UEFI.
>
> Moreover to ensure that these IP blocks always get memory, and maximu= m
> contiguous RAM is available for UEFI and OS to use, add the support f= or
> reserving a chunk from RAM before reporting available RAM to UEFI.
I can't shake the feeling this code has the wrong level of
complexity. It's reserving *one* memory window, at a given alignment.

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/NxpQoriqLs.dec      &= nbsp;           &nbs= p;             = |  10 ++
>  Silicon/NXP/LX2160A/LX2160A.dsc.inc    &nbs= p;            &= nbsp;     |   4 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |&nbs= p;  3 +
>  Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c &n= bsp; | 142 +++++++++++++++++++-
>  4 files changed, 157 insertions(+), 2 deletions(-)
>
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec<= br> > index 188a9fe1f382..0e762066e547 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -41,3 +41,13 @@ [PcdsDynamic.common]
>    gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALS= E|BOOLEAN|0x00000600
>    gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BO= OLEAN|0x00000601
>    gNxpQoriqLsTokenSpaceGuid.PcdPciHideRootPort|FALSE|= BOOLEAN|0x00000602
> +
> +  # Reserved RAM Base address alignment. This number ought to b= e Power of two
> +  # in case no alignment is needed, this number should be 1.
(As has been pointed out to me in the past, 1 is also a power of 2...)

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|0x1|UINT64|= 0x00000603
> +  # Size of the RAM to be reserved. This RAM region is neither = reported to UEFI
> +  # nor to OS

"Reported" is an inaccurate description of the mechanism involve= d.

> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|0x0|UINT64|0x000= 00604
> +  # Reserved RAM Base address which is calculated based on PcdR= eservedMemSize
> +  # and PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase|0x0|UINT64|0x000= 00605

Why  use a Pcd for something that is calculated at runtime and needs<= br> to be used immediately? Moreover, I see this Pcd getting set, but I
then don't see it getting used ever?

> diff --git a/Silicon/NXP/LX2160A/LX2160A.dsc.inc b/Silicon/NXP/LX2160= A/LX2160A.dsc.inc
> index 43e361464c8e..755ca169f213 100644
> --- a/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> +++ b/Silicon/NXP/LX2160A/LX2160A.dsc.inc
> @@ -29,6 +29,10 @@ [PcdsDynamicDefault.common]
>    gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x62000= 00
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xC= 0C0000
> 
> +[PcdsDynamicHii]
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment|L"Rese= rvedMemAlignment"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize|L"ReservedM= emSize"|gEfiGlobalVariableGuid|0x0|0x20000000|NV,BS
> +

And here we get Hii involved, but I see no connection to a user
interface form.

>  [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x= 23A0000
>    gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x= 2390000
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.in= f b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> index a33f8cd3f743..ed23a86b43d9 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
> @@ -49,6 +49,9 @@ [FixedPcd]
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemAlignment
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemBase
> +  gNxpQoriqLsTokenSpaceGuid.PcdReservedMemSize
> 
>  [Depex]
>    TRUE
> diff --git a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c = b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> index 11d1f1260b35..b416323a4ced 100644
> --- a/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> +++ b/Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
> @@ -118,6 +118,127 @@ GetDramRegionsInfo (
>    return EFI_BUFFER_TOO_SMALL;
>  }
> 
> +/**
> +  Calculate the base address of Reserved RAM.
> +  Reserved RAM is not reported to either UEFI or OS.

Here is the word "reported" again.
A region is included in the memory map or not.
If we are setting parts of the memory map aside, then we should do
that by marking them as EfiReservedMemoryType.

> +
> +  @param[in, out] DramRegions  Array of type DRAM_REGION_I= NFO. The size of this
> +           &n= bsp;            = ;       array must be one more (+ 1) than the= maximum
> +           &n= bsp;            = ;       regions supported on platform. This i= s because,
> +           &n= bsp;            = ;       if due to Reserved RAM alignment requ= irements a
> +           &n= bsp;            = ;       hole is created in any DRAM region, t= hen the RAM
> +           &n= bsp;            = ;       after hole gets reported to UEFI and = then
> +           &n= bsp;            = ;       subsequently to OS. which is why, the= last entry
> +           &n= bsp;            = ;       of this array will not be parsed whil= e
> +           &n= bsp;            = ;       calculating Reserved RAM base address= . Caller
> +           &n= bsp;            = ;       must ensure that last entry of this a= rray is zero
> +           &n= bsp;            = ;       initialized.
> +  @param[in] NumRegions      &nbs= p; Size of DramRegions array (including +1 for hole)
> +  @param[in] ReservedMemSize   Size of RAM to be rese= rved.
> +
> +  @return  if successful Address of the Reserved RAM regio= n, 0 otherwise.
> +**/
> +STATIC
> +UINTN
> +CalculateReservedMemBase (
> +  IN DRAM_REGION_INFO *DramRegions,
> +  IN UINT32        &nbs= p;  NumRegions,
> +  IN UINTN         = ;   ReservedMemSize
> +)
> +{
> +  UINTN         &n= bsp;       ReservedMemAlignment;
> +  EFI_PHYSICAL_ADDRESS  AlignmentMask;
> +  UINTN         &n= bsp;       RegionBaseAddress;
> +  UINTN         &n= bsp;       RegionSize;
> +  UINTN         &n= bsp;       ReservedBaseAddress;
> +  INTN         &nb= sp;        Index;
> +  INTN         &nb= sp;        Index2;
> +
> +  ReservedMemAlignment =3D PcdGet64 (PcdReservedMemAlignment);<= br> > +  //
> +  // Compute alignment bit mask
> +  //
> +  if (ReservedMemAlignment) {
> +    AlignmentMask =3D LShiftU64 (1, LowBitSet64(Reser= vedMemAlignment)) - 1;
> +  } else {
> +    AlignmentMask =3D 0;
> +  }
> +
> +  // The DRAM region info is sorted based on the RAM address is= SOC memory map.
> +  // i.e. DramRegions[0] is at lower address, as compared to Dr= amRegions[1].
> +  // The goal to start from last region is to find the topmost = RAM region that
> +  // can contain Reserved RAM region i.e. PcdReservedMemSize. > +  // Since this RAM is not reported to either UEFI or OS, This = ensures that
> +  // maximum amount of lower RAM (32 bit addresses) are left > +  // for OS to allocate to devices that can only work with 32bi= t physical
> +  // addresses. E.g. legacy devices that need to DMA to 32bit a= ddresses.
> +  for (Index =3D NumRegions - 2; Index >=3D0; Index--) {
> +    RegionBaseAddress =3D DramRegions[Index].BaseAddr= ess;
> +    RegionSize =3D DramRegions[Index].Size;
> +
> +    if (ReservedMemSize > RegionSize) {
> +      continue;
> +    }
> +
> +    ReservedBaseAddress =3D (RegionBaseAddress + Regi= onSize - ReservedMemSize);
> +    ReservedBaseAddress &=3D (~AlignmentMask); > +    if (ReservedBaseAddress < RegionBaseAddress) {=
> +      continue;
> +    }
> +
> +    // found the region from which reserved mem is to= be carved out
> +    // Need to modify the region size and create/dele= te region if need be
> +
> +    RegionSize -=3D ReservedMemSize;
> +    if (RegionSize =3D=3D 0) {
> +      // delete the region but maintain the= sorted list of regions
> +      for (Index2 =3D Index; Index2 < Nu= mRegions; Index2++) {
> +        CopyMem (
> +          &DramRegi= ons[Index2],
> +          &DramRegi= ons[Index2 + 1],
> +          sizeof (DRAM_= REGION_INFO)
> +        );
> +      }
> +      break;
> +    }
> +
> +    if (ReservedBaseAddress - RegionBaseAddress) { > +      DramRegions[Index].Size =3D ReservedB= aseAddress - RegionBaseAddress;
> +      RegionSize -=3D DramRegions[Index].Si= ze;
> +    } else {
> +      DramRegions[Index].BaseAddress =3D Re= servedBaseAddress + ReservedMemSize;
> +      DramRegions[Index].Size =3D RegionSiz= e;
> +      RegionSize =3D 0;
> +    }
> +
> +    if (RegionSize =3D=3D 0) {
> +      break;
> +    }
> +
> +    // A hole has been created in DRAM regions due to= Reserved RAM alignment
> +    // requirements. create a new DRAM region for DRA= M memory after hole.
> +    // Maintain the sorted list of regions
> +    for (Index2 =3D NumRegions; Index2 > (Index + = 1); Index2--) {
> +      CopyMem (
> +        &DramRegions[Index2],=
> +        &DramRegions[Index2 -= 1],
> +        sizeof (DRAM_REGION_INFO)=
> +      );
> +    }
> +    DramRegions[Index2].BaseAddress =3D ReservedBaseA= ddress + ReservedMemSize;
> +    DramRegions[Index2].Size =3D RegionSize;
> +    RegionSize =3D 0;
> +
> +    break;
> +  }
> +
> +  if (Index =3D=3D -1) {
> +    return 0;
> +  } else {
> +    return ReservedBaseAddress;
> +  }
> +}
> +
>  /**
>    Get the installed RAM information.
>    Initialize Memory HOBs (Resource Descriptor HOBs) > @@ -135,7 +256,9 @@ MemoryInitPeiLibConstructor (
>    UINTN       &nbs= p;            &= nbsp;    BaseAddress;
>    UINTN       &nbs= p;            &= nbsp;    Size;
>    UINTN       &nbs= p;            &= nbsp;    Top;
> -  DRAM_REGION_INFO       &nb= sp;      DramRegions[MAX_DRAM_REGIONS];
> +  // Extra region gets created if we want to reserve a memory r= egion and that
> +  // creates a memory hole because of alignment requirements > +  DRAM_REGION_INFO       &nb= sp;      DramRegions[MAX_DRAM_REGIONS + 1];

No. Just no.

>    EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAtt= ributes;
>    UINTN       &nbs= p;            &= nbsp;    FdBase;
>    UINTN       &nbs= p;            &= nbsp;    FdTop;
> @@ -155,6 +278,21 @@ MemoryInitPeiLibConstructor (
> 
>    (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (= DramRegions));
> 
> +  // Get the reserved memory size from non volatile storage
> +  Size =3D PcdGet64 (PcdReservedMemSize);
> +  if (Size) {
> +    BaseAddress =3D CalculateReservedMemBase (
> +           &n= bsp;        DramRegions,
> +           &n= bsp;        ARRAY_SIZE (DramRegions), > +           &n= bsp;        Size
> +           &n= bsp;      );
> +    if (BaseAddress) {
> +      DEBUG ((DEBUG_INFO, "ReservedMem= : start 0x%lx, size 0x%lx\n",
> +        BaseAddress, Size));
> +      PcdSet64S (PcdReservedMemBase, BaseAd= dress);

PcdReservedMemBase set but never used.

And how do the other controllers find out about what region has ended
up being set aside for them? At what point during the boot process do
they need it?

/
    Leif

> +    }
> +  }
> +
>    FdBase =3D (UINTN)PcdGet64 (PcdFdBaseAddress);
>    FdTop =3D FdBase + (UINTN)PcdGet32 (PcdFdSize);
> 
> @@ -168,7 +306,7 @@ MemoryInitPeiLibConstructor (
>    // This ensures that maximum amount of lower RAM (3= 2 bit addresses) are left
>    // for OS to allocate to devices that can only work= with 32bit physical
>    // addresses. E.g. legacy devices that need to DMA = to 32bit addresses.
> -  for (Index =3D MAX_DRAM_REGIONS - 1; Index >=3D 0; Index--= ) {
> +  for (Index =3D MAX_DRAM_REGIONS; Index >=3D 0; Index--) {<= br> >      if (DramRegions[Index].Size =3D=3D 0) {=
>        continue;
>      }
> --
> 2.17.1
>

 

--_000_CY4PR21MB07433F2464A8F96834F66ED4EF4B0CY4PR21MB0743namp_--