From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM04-SN1-obe.outbound.protection.outlook.com (NAM04-SN1-obe.outbound.protection.outlook.com [40.92.11.17]) by mx.groups.io with SMTP id smtpd.web08.4555.1609915040557799986 for ; Tue, 05 Jan 2021 22:37:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=jnFTQu2M; spf=pass (domain: outlook.com, ip: 40.92.11.17, mailfrom: kun.q@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Jma4/d4dnMx2eq5KrUmqbeB2qVICX4wMIw/5NBOm/mH3VKufh6lWy1y+p7kw1oJdT90d1M1SWmvTgMx8P0VFdVI8szDyKFpxgLAR7WZIgS4GC/H0CLMRAnP9E4EaPNczlKmeZHGW24/gEjp1QS1aE03/AyY95CtazHrpDt0Xmxp1J5h1pImtUpjO93/CJyEMYLnCDOzfvOjeJR0sZFayKhwFFTczudGl1fdkz9S3U8LAX4iN2wNLpj0+VZLelntJdP7YboTdn92r3Gveap5pvRHZRG5WeCGFkM9jZjyx4vX+fy7WlNawhFJ+8MsTC5x+I06rTSLDfhpmeFlaqt0bkA== 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=05rHUS0+ymNmPqpaawInBsHpzYQZL6KZm321w8M9tfQ=; b=e+1rFK/kQtamLqR7DMTtFA9SsdqRd4TCLyUKXkjewHte2a0j7PxLMO/cj/x1wu2EB/TYLQbrlLk8Mqi8F4yQNQQgKfTCKL+ACaAcZzQL4HSxvPh/6FCNCDGMPQF0OLkv9WjIP6yyK5d6rFR9YU6wLHMWNHyQtg7XfPc92U0zOEOYRbw4Vnd0Q1c50160MUjaqbSqVvFIcG3eaeVT4aOXJbTU+fhUxHSnReb901hr0HBZOd6+HCe7JHBnM40IjBdhZFn1HS6xTGoMWeO8sECRO+52GzpDgaNIWw8Rohl23NTIEM7gIPy5i2tgZA9nJ2e7VxWgaSk94vLZaNrte5O4nQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=05rHUS0+ymNmPqpaawInBsHpzYQZL6KZm321w8M9tfQ=; b=jnFTQu2Mvi3pi89nHOVedSLQpagSR/CCSHW4jazy+BIZzkWIbGt/Prl381ObjnIwnvdAxhRhQcmEfaWgIAdBx43DAsTX6zyvhNRQl9YFWyTMP5ktGnXCa+gM72apn2REzHMzbvTL/pox/Nfzoi04yKXllFt79qjPezR3gd0BtTyor1Te3laQM0FglK2o4O73zQuJQ//FEzE7A9iDwn5aXL3PqZHcREhwir4OmMu4mJ3JBaPHeQ0Nj5ViAgCM0PS3ALqUK1tEbDpu2BWh6q0uic8kVjbkMXabQ2HT8yPh+Cs0SJUouU/HHhxn62hfeJr3NjYk5VKqz30Voirb1TY/Eg== Received: from BN3NAM04FT021.eop-NAM04.prod.protection.outlook.com (10.152.92.52) by BN3NAM04HT171.eop-NAM04.prod.protection.outlook.com (10.152.93.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3742.6; Wed, 6 Jan 2021 06:37:18 +0000 Received: from MWHPR06MB3102.namprd06.prod.outlook.com (2a01:111:e400:7e4e::52) by BN3NAM04FT021.mail.protection.outlook.com (2a01:111:e400:7e4e::150) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3742.6 via Frontend Transport; Wed, 6 Jan 2021 06:37:18 +0000 Received: from MWHPR06MB3102.namprd06.prod.outlook.com ([fe80::acb3:ab69:563d:b0d6]) by MWHPR06MB3102.namprd06.prod.outlook.com ([fe80::acb3:ab69:563d:b0d6%5]) with mapi id 15.20.3721.024; Wed, 6 Jan 2021 06:37:18 +0000 From: "Kun Qin" To: "devel@edk2.groups.io" , "jiewen.yao@intel.com" CC: Ard Biesheuvel , Sami Mujawar , Supreeth Venkatesh Subject: Re: [edk2-devel] [PATCH v2 04/16] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture Thread-Topic: [edk2-devel] [PATCH v2 04/16] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture Thread-Index: AQHW45Tzu/aahsB3xEGlRCPbu4rGAaoZ87gAgAAlDlo= Date: Wed, 6 Jan 2021 06:37:17 +0000 Message-ID: References: <20210105185935.3769-1-kun.q@outlook.com> , In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-incomingtopheadermarker: OriginalChecksum:D95A837C9A566B823EC4D6FF1C575230149B835416D926BE3119BF296FE56AB3;UpperCasedChecksum:D33D7E17576C2EDF2E5C77CDC59FE5359849733A1CAFDF976B13B0C585ED17FD;SizeAsReceived:7240;Count:44 x-tmn: [oFgibTwtsLETrICOHM7O8DaoOf2IZK69] x-ms-publictraffictype: Email x-incomingheadercount: 44 x-eopattributedmessage: 0 x-ms-office365-filtering-correlation-id: e3ba5c9d-35be-4301-bc39-08d8b20d834d x-ms-traffictypediagnostic: BN3NAM04HT171: x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: uGX8gxhULzag80GQd+k9UTBBJWT94mdbc8Xfb6vb43W/7LUy/BzO6j1oOjRIiVd5CgrpemLAMowfXo5CEgYV5vZqv7wF13RZoGbDnQV4zUsI/2MLkqBYtV4VihanVA068qe886BqAQLs4KvLCLNHxP0HOzglST6YWtmfZ6IdtRSzEpVIxBj0DJkiUNRSTle1+z/x5IB0jkjTkiKkptkJuh8dZkRzbGss4WypjOld0VtWtZgdbu7etQOR63pbmqpTJy8hwZo6nyRQJTHbWAqa4UbaG5JKhFjNQ9uuwnFWU1Y= x-ms-exchange-antispam-messagedata: ey2FIZBhjuW0qmukV313pYjvXBDjo3DBuhKEBYAXVjL9BoLpk+3tJ+xP8Z9rNSBZMmYiADbTd1k/Dfu3eOXUFrKfGLu2/DGz0batkVN8KaUr1Z12Dp0V1wDD42RFZZJLvosJDqix9rxRTKJceU08Yw== x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-AuthSource: BN3NAM04FT021.eop-NAM04.prod.protection.outlook.com X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-Network-Message-Id: e3ba5c9d-35be-4301-bc39-08d8b20d834d X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jan 2021 06:37:17.9744 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3NAM04HT171 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MWHPR06MB3102B75BA84E56FEED00166EF3D00MWHPR06MB3102namp_" --_000_MWHPR06MB3102B75BA84E56FEED00166EF3D00MWHPR06MB3102namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Jiewen, Before introducing the MmRange initialization logic, I was hitting a bug c= aused by `MmIsBufferOutsideMmValid` since it returns TRUE regardless of wha= t I passed in. After the patch, this function start to return FALSE when I = verify with regions overlapping with MmRanges. As per OS memory protection, I guess you mean the logic around block every= thing but runtime, ACPI, and reserved memory after ready to lock? I think w= e should bring in those logic at some point. But as of now, that routine wo= uld need information on DXE memory map, which needs extra efforts (such as = a designated handler) to fetch under standalone MM environment. On the other hand, one proposal is for standalone MM, only MmRam and regio= ns requested through a specific protocol/handler are allowed. So that we ca= n implement this MmMemLib from a different approach. We can discuss more ab= out this proposal separately if you would like to. Thanks, Kun From: Yao, Jiewen Sent: Tuesday, January 5, 2021 19:38 To: Kun Qin; devel@edk2.groups.io Cc: Ard Biesheuvel; Sami Mujawar; Supreeth Venkatesh Subject: Re: [edk2-devel] [PATCH v2 04/16] StandaloneMmPkg: StandaloneMmMe= mLib: Extends support for X64 architecture Thanks Qin. A quick question: May I know what test you have run for this change? Also, I think this patch only protect the MM memory, but not OS memory. Is= that expected? Will you consider adding OS memory protection later? > -----Original Message----- > From: Kun Qin > Sent: Wednesday, January 6, 2021 2:59 AM > To: devel@edk2.groups.io > Cc: Ard Biesheuvel ; Sami Mujawar > ; Yao, Jiewen ; Supreeth > Venkatesh > Subject: [PATCH v2 04/16] StandaloneMmPkg: StandaloneMmMemLib: > Extends support for X64 architecture > > This change extends StandaloneMmMemLib library to support X64 > architecture. The implementation is ported from > MdePkg/Library/SmmMemLib. > > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Jiewen Yao > Cc: Supreeth Venkatesh > > Signed-off-by: Kun Qin > --- > > Notes: > v2: > - Added routine to fix bug of not initializing MmRanges [Jiewen] > - Extends support to x86 instead of x64 only [Hao] > > > StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneM > mMemLibInternal.c | 26 ++++ > > StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib > .c | 52 +++++++ > > StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMe > mLibInternal.c | 155 ++++++++++++++++++++ > > StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib > .inf | 13 +- > 4 files changed, 245 insertions(+), 1 deletion(-) > > diff --git > a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone > MmMemLibInternal.c > b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone > MmMemLibInternal.c > index cb7c5e677a6b..46dfce5cac86 100644 > --- > a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone > MmMemLibInternal.c > +++ > b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone > MmMemLibInternal.c > @@ -40,4 +40,30 @@ > MmMemLibInternalCalculateMaximumSupportAddress ( > DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumSupportAddress > =3D 0x%lx\n", mMmMemLibInternalMaximumSupportAddress)); > } > > +/** > + Initialize cached Mmram Ranges from HOB. > + > + @retval EFI_UNSUPPORTED The routine is unable to extract MMRAM > information. > + @retval EFI_SUCCESS MmRanges are populated successfully. > + > +**/ > +EFI_STATUS > +MmMemLibInternalPopulateMmramRanges ( > + VOID > + ) > +{ > + // Not implemented for AARCH64. > +} > + > +/** > + Deinitialize cached Mmram Ranges. > + > +**/ > +VOID > +MmMemLibInternalFreeMmramRanges ( > + VOID > + ) > +{ > + // Not implemented for AARCH64. > +} > > diff --git > a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.c > b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.c > index f82cdb3ba816..f43af2b1cc9b 100644 > --- > a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.c > +++ > b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.c > @@ -37,6 +37,27 @@ > MmMemLibInternalCalculateMaximumSupportAddress ( > VOID > ); > > +/** > + Initialize cached Mmram Ranges from HOB. > + > + @retval EFI_UNSUPPORTED The routine is unable to extract MMRAM > information. > + @retval EFI_SUCCESS MmRanges are populated successfully. > + > +**/ > +EFI_STATUS > +MmMemLibInternalPopulateMmramRanges ( > + VOID > + ); > + > +/** > + Deinitialize cached Mmram Ranges. > + > +**/ > +VOID > +MmMemLibInternalFreeMmramRanges ( > + VOID > + ); > + > /** > This function check if the buffer is valid per processor architecture= and not > overlap with MMRAM. > > @@ -253,11 +274,42 @@ MemLibConstructor ( > IN EFI_MM_SYSTEM_TABLE *MmSystemTable > ) > { > + EFI_STATUS Status; > > // > // Calculate and save maximum support address > // > MmMemLibInternalCalculateMaximumSupportAddress (); > > + // > + // Initialize cached Mmram Ranges from HOB. > + // > + Status =3D MmMemLibInternalPopulateMmramRanges (); > + > + return Status; > +} > + > +/** > + Destructor for the library. free any resources for Mm Mem library > + > + @param ImageHandle The image handle of the process. > + @param SystemTable The EFI System Table pointer. > + > + @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS. > + > +**/ > +EFI_STATUS > +EFIAPI > +MemLibDestructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_MM_SYSTEM_TABLE *MmSystemTable > + ) > +{ > + > + // > + // Deinitialize cached Mmram Ranges. > + // > + MmMemLibInternalFreeMmramRanges (); > + > return EFI_SUCCESS; > } > diff --git > a/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmM > emLibInternal.c > b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMm > MemLibInternal.c > new file mode 100644 > index 000000000000..1a978541716a > --- /dev/null > +++ > b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMm > MemLibInternal.c > @@ -0,0 +1,155 @@ > +/** @file > + Internal ARCH Specific file of MM memory check library. > + > + MM memory check library implementation. This library consumes > MM_ACCESS_PROTOCOL > + to get MMRAM information. In order to use this library instance, the > platform should produce > + all MMRAM range via MM_ACCESS_PROTOCOL, including the range for > firmware (like MM Core > + and MM driver) and/or specific dedicated hardware. > + > + Copyright (c) 2015, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> + Copyright (c) Microsoft Corporation. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +// > +// Maximum support address used to check input buffer > +// > +extern EFI_PHYSICAL_ADDRESS > mMmMemLibInternalMaximumSupportAddress; > +extern EFI_MMRAM_DESCRIPTOR *mMmMemLibInternalMmramRanges; > +extern UINTN mMmMemLibInternalMmramCount; > + > +/** > + Calculate and save the maximum support address. > + > +**/ > +VOID > +MmMemLibInternalCalculateMaximumSupportAddress ( > + VOID > + ) > +{ > + VOID *Hob; > + UINT32 RegEax; > + UINT8 PhysicalAddressBits; > + > + // > + // Get physical address bits supported. > + // > + Hob =3D GetFirstHob (EFI_HOB_TYPE_CPU); > + if (Hob !=3D NULL) { > + PhysicalAddressBits =3D ((EFI_HOB_CPU *) Hob)->SizeOfMemorySpace; > + } else { > + AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL); > + if (RegEax >=3D 0x80000008) { > + AsmCpuid (0x80000008, &RegEax, NULL, NULL, NULL); > + PhysicalAddressBits =3D (UINT8) RegEax; > + } else { > + PhysicalAddressBits =3D 36; > + } > + } > + // > + // IA-32e paging translates 48-bit linear addresses to 52-bit physica= l > addresses. > + // > + ASSERT (PhysicalAddressBits <=3D 52); > + if (PhysicalAddressBits > 48) { > + PhysicalAddressBits =3D 48; > + } > + > + // > + // Save the maximum support address in one global variable > + // > + mMmMemLibInternalMaximumSupportAddress =3D > (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, PhysicalAddressBits) - 1); > + DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumSupportAddress > =3D 0x%lx\n", mMmMemLibInternalMaximumSupportAddress)); > +} > + > +/** > + Initialize cached Mmram Ranges from HOB. > + > + @retval EFI_UNSUPPORTED The routine is unable to extract MMRAM > information. > + @retval EFI_SUCCESS MmRanges are populated successfully. > + > +**/ > +EFI_STATUS > +MmMemLibInternalPopulateMmramRanges ( > + VOID > + ) > +{ > + VOID *HobStart; > + EFI_HOB_GUID_TYPE *GuidHob; > + MM_CORE_DATA_HOB_DATA *DataInHob; > + MM_CORE_PRIVATE_DATA *MmCorePrivateData; > + EFI_HOB_GUID_TYPE *MmramRangesHob; > + EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHobData; > + EFI_MMRAM_DESCRIPTOR *MmramDescriptors; > + > + HobStart =3D GetHobList (); > + DEBUG ((DEBUG_INFO, "%a - 0x%x\n", __FUNCTION__, HobStart)); > + > + // > + // Extract MM Core Private context from the Hob. If absent search for > + // a Hob containing the MMRAM ranges > + // > + GuidHob =3D GetNextGuidHob (&gMmCoreDataHobGuid, HobStart); > + if (GuidHob =3D=3D NULL) { > + MmramRangesHob =3D GetFirstGuidHob > (&gEfiMmPeiMmramMemoryReserveGuid); > + if (MmramRangesHob =3D=3D NULL) { > + return EFI_UNSUPPORTED; > + } > + > + MmramRangesHobData =3D GET_GUID_HOB_DATA (MmramRangesHob); > + if (MmramRangesHobData =3D=3D NULL || MmramRangesHobData- > >Descriptor =3D=3D NULL) { > + return EFI_UNSUPPORTED; > + } > + > + mMmMemLibInternalMmramCount =3D MmramRangesHobData- > >NumberOfMmReservedRegions; > + MmramDescriptors =3D MmramRangesHobData->Descriptor; > + } else { > + DataInHob =3D GET_GUID_HOB_DATA (GuidHob); > + if (DataInHob =3D=3D NULL) { > + return EFI_UNSUPPORTED; > + } > + > + MmCorePrivateData =3D (MM_CORE_PRIVATE_DATA *) (UINTN) > DataInHob->Address; > + if (MmCorePrivateData =3D=3D NULL || MmCorePrivateData- > >MmramRanges =3D=3D 0) { > + return EFI_UNSUPPORTED; > + } > + > + mMmMemLibInternalMmramCount =3D (UINTN) MmCorePrivateData- > >MmramRangeCount; > + MmramDescriptors =3D (EFI_MMRAM_DESCRIPTOR *) (UINTN) > MmCorePrivateData->MmramRanges; > + } > + > + mMmMemLibInternalMmramRanges =3D AllocatePool > (mMmMemLibInternalMmramCount * sizeof (EFI_MMRAM_DESCRIPTOR)); > + if (mMmMemLibInternalMmramRanges) { > + CopyMem (mMmMemLibInternalMmramRanges, > + MmramDescriptors, > + mMmMemLibInternalMmramCount * sizeof > (EFI_MMRAM_DESCRIPTOR)); > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + Deinitialize cached Mmram Ranges. > + > +**/ > +VOID > +MmMemLibInternalFreeMmramRanges ( > + VOID > + ) > +{ > + if (mMmMemLibInternalMmramRanges !=3D NULL) { > + FreePool (mMmMemLibInternalMmramRanges); > + } > +} > + > diff --git > a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.inf > b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.inf > index 49da02e54e6d..062b0d7a1161 100644 > --- > a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.inf > +++ > b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem > Lib.inf > @@ -8,6 +8,7 @@ > # > # Copyright (c) 2015, Intel Corporation. All rights reserved.
> # Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> +# Copyright (c) Microsoft Corporation. > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -22,16 +23,20 @@ [Defines] > PI_SPECIFICATION_VERSION =3D 0x00010032 > LIBRARY_CLASS =3D MemLib|MM_STANDALONE > MM_CORE_STANDALONE > CONSTRUCTOR =3D MemLibConstructor > + DESTRUCTOR =3D MemLibDestructor > > # > # The following information is for reference only and not required by t= he > build tools. > # > -# VALID_ARCHITECTURES =3D AARCH64 > +# VALID_ARCHITECTURES =3D IA32 X64 AARCH64 > # > > [Sources.Common] > StandaloneMmMemLib.c > > +[Sources.IA32, Sources.X64] > + X86StandaloneMmMemLibInternal.c > + > [Sources.AARCH64] > AArch64/StandaloneMmMemLibInternal.c > > @@ -42,3 +47,9 @@ [Packages] > [LibraryClasses] > BaseMemoryLib > DebugLib > + HobLib > + MemoryAllocationLib > + > +[Guids] > + gMmCoreDataHobGuid ## SOMETIMES_CONSUMES ## HOB > + gEfiMmPeiMmramMemoryReserveGuid ## SOMETIMES_CONSUMES > ## HOB > -- > 2.30.0.windows.1 --_000_MWHPR06MB3102B75BA84E56FEED00166EF3D00MWHPR06MB3102namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Jiewen,

 

Before introducing the MmRange initialization logic= , I was hitting a bug caused by `MmIsBufferOutsideMmValid` since it returns= TRUE regardless of what I passed in. After the patch, this function start = to return FALSE when I verify with regions overlapping with MmRanges.

 

As per OS memory protection, I guess you mean the l= ogic around block everything but runtime, ACPI, and reserved memory after r= eady to lock? I think we should bring in those logic at some point. But as = of now, that routine would need information on DXE memory map, which needs extra efforts (such as a designated handle= r) to fetch under standalone MM environment.

 

On the other hand, one proposal is for standalone M= M, only MmRam and regions requested through a specific protocol/handler are= allowed. So that we can implement this MmMemLib from a different approach.= We can discuss more about this proposal separately if you would like to.

 

Thanks,

Kun

 

From: Yao, Jiewen
Sent: Tuesday, January 5, 2021 19:38
To: Kun Qin; devel@edk2.groups.io
Cc: Ard Biesheuvel; <= a href=3D"mailto:sami.mujawar@arm.com"> Sami Mujawar; Supreeth V= enkatesh
Subject: Re: [edk2-devel] [PATCH v2 04/16] StandaloneMmPkg: Standal= oneMmMemLib: Extends support for X64 architecture

 

Thanks Qin.
A quick question: May I know what test you have run for this change?

Also, I think this patch only protect the MM memory, but not OS memory. Is= that expected?
Will you consider adding OS memory protection later?


> -----Original Message-----
> From: Kun Qin <kun.q@outlook.com>
> Sent: Wednesday, January 6, 2021 2:59 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Sami Mujawar
> <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>= ;; Supreeth
> Venkatesh <supreeth.venkatesh@arm.com>
> Subject: [PATCH v2 04/16] StandaloneMmPkg: StandaloneMmMemLib:
> Extends support for X64 architecture
>
> This change extends StandaloneMmMemLib library to support X64
> architecture. The implementation is ported from
> MdePkg/Library/SmmMemLib.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
>
> Signed-off-by: Kun Qin <kun.q@outlook.com>
> ---
>
> Notes:
>     v2:
>     - Added routine to fix bug of not initializin= g MmRanges [Jiewen]
>     - Extends support to x86 instead of x64 only = [Hao]
>
>
> StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneM
> mMemLibInternal.c |  26 ++++
>
> StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib
> .c           &= nbsp;     |  52 +++++++
>
> StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMe
> mLibInternal.c      | 155 ++++++++++++++++++= ++
>
> StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib
> .inf           = ;    |  13 +-
>  4 files changed, 245 insertions(+), 1 deletion(-)
>
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> index cb7c5e677a6b..46dfce5cac86 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> @@ -40,4 +40,30 @@
> MmMemLibInternalCalculateMaximumSupportAddress (
>    DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumS= upportAddress
> =3D 0x%lx\n", mMmMemLibInternalMaximumSupportAddress));
>  }
>
> +/**
> +  Initialize cached Mmram Ranges from HOB.
> +
> +  @retval EFI_UNSUPPORTED   The routine is unable to = extract MMRAM
> information.
> +  @retval EFI_SUCCESS       MmRan= ges are populated successfully.
> +
> +**/
> +EFI_STATUS
> +MmMemLibInternalPopulateMmramRanges (
> +  VOID
> +  )
> +{
> +  // Not implemented for AARCH64.
> +}
> +
> +/**
> +  Deinitialize cached Mmram Ranges.
> +
> +**/
> +VOID
> +MmMemLibInternalFreeMmramRanges (
> +  VOID
> +  )
> +{
> +  // Not implemented for AARCH64.
> +}
>
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> index f82cdb3ba816..f43af2b1cc9b 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> @@ -37,6 +37,27 @@
> MmMemLibInternalCalculateMaximumSupportAddress (
>    VOID
>    );
>
> +/**
> +  Initialize cached Mmram Ranges from HOB.
> +
> +  @retval EFI_UNSUPPORTED   The routine is unable to = extract MMRAM
> information.
> +  @retval EFI_SUCCESS       MmRan= ges are populated successfully.
> +
> +**/
> +EFI_STATUS
> +MmMemLibInternalPopulateMmramRanges (
> +  VOID
> +  );
> +
> +/**
> +  Deinitialize cached Mmram Ranges.
> +
> +**/
> +VOID
> +MmMemLibInternalFreeMmramRanges (
> +  VOID
> +  );
> +
>  /**
>    This function check if the buffer is valid per proc= essor architecture and not
> overlap with MMRAM.
>
> @@ -253,11 +274,42 @@ MemLibConstructor (
>    IN EFI_MM_SYSTEM_TABLE    *MmSystemT= able
>    )
>  {
> +  EFI_STATUS Status;
>
>    //
>    // Calculate and save maximum support address
>    //
>    MmMemLibInternalCalculateMaximumSupportAddress ();<= br> >
> +  //
> +  // Initialize cached Mmram Ranges from HOB.
> +  //
> +  Status =3D MmMemLibInternalPopulateMmramRanges ();
> +
> +  return Status;
> +}
> +
> +/**
> +  Destructor for the library.  free any resources for Mm M= em library
> +
> +  @param ImageHandle    The image handle of the = process.
> +  @param SystemTable    The EFI System Table poi= nter.
> +
> +  @retval EFI_SUCCESS   The constructor always return= s EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +MemLibDestructor (
> +  IN EFI_HANDLE        =      ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE    *MmSystemTable
> +  )
> +{
> +
> +  //
> +  // Deinitialize cached Mmram Ranges.
> +  //
> +  MmMemLibInternalFreeMmramRanges ();
> +
>    return EFI_SUCCESS;
>  }
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmM
> emLibInternal.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMm
> MemLibInternal.c
> new file mode 100644
> index 000000000000..1a978541716a
> --- /dev/null
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMm
> MemLibInternal.c
> @@ -0,0 +1,155 @@
> +/** @file
> +  Internal ARCH Specific file of MM memory check library.
> +
> +  MM memory check library implementation. This library consumes=
> MM_ACCESS_PROTOCOL
> +  to get MMRAM information. In order to use this library instan= ce, the
> platform should produce
> +  all MMRAM range via MM_ACCESS_PROTOCOL, including the range f= or
> firmware (like MM Core
> +  and MM driver) and/or specific dedicated hardware.
> +
> +  Copyright (c) 2015, Intel Corporation. All rights reserved.&l= t;BR>
> +  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.&= lt;BR>
> +  Copyright (c) Microsoft Corporation.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#include <PiMm.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +
> +#include <Guid/MmCoreData.h>
> +#include <Guid/MmramMemoryReserve.h>
> +
> +//
> +// Maximum support address used to check input buffer
> +//
> +extern EFI_PHYSICAL_ADDRESS
> mMmMemLibInternalMaximumSupportAddress;
> +extern EFI_MMRAM_DESCRIPTOR *mMmMemLibInternalMmramRanges;
> +extern UINTN         &n= bsp;       mMmMemLibInternalMmramCount;
> +
> +/**
> +  Calculate and save the maximum support address.
> +
> +**/
> +VOID
> +MmMemLibInternalCalculateMaximumSupportAddress (
> +  VOID
> +  )
> +{
> +  VOID         *Hob; > +  UINT32       RegEax;
> +  UINT8        PhysicalAddre= ssBits;
> +
> +  //
> +  // Get physical address bits supported.
> +  //
> +  Hob =3D GetFirstHob (EFI_HOB_TYPE_CPU);
> +  if (Hob !=3D NULL) {
> +    PhysicalAddressBits =3D ((EFI_HOB_CPU *) Hob)->= ;SizeOfMemorySpace;
> +  } else {
> +    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NU= LL);
> +    if (RegEax >=3D 0x80000008) {
> +      AsmCpuid (0x80000008, &RegEax, NU= LL, NULL, NULL);
> +      PhysicalAddressBits =3D (UINT8) RegEa= x;
> +    } else {
> +      PhysicalAddressBits =3D 36;
> +    }
> +  }
> +  //
> +  // IA-32e paging translates 48-bit linear addresses to 52-bit= physical
> addresses.
> +  //
> +  ASSERT (PhysicalAddressBits <=3D 52);
> +  if (PhysicalAddressBits > 48) {
> +    PhysicalAddressBits =3D 48;
> +  }
> +
> +  //
> +  // Save the maximum support address in one global variable > +  //
> +  mMmMemLibInternalMaximumSupportAddress =3D
> (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, PhysicalAddressBits) - 1)= ;
> +  DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumSupportAddr= ess
> =3D 0x%lx\n", mMmMemLibInternalMaximumSupportAddress));
> +}
> +
> +/**
> +  Initialize cached Mmram Ranges from HOB.
> +
> +  @retval EFI_UNSUPPORTED   The routine is unable to = extract MMRAM
> information.
> +  @retval EFI_SUCCESS       MmRan= ges are populated successfully.
> +
> +**/
> +EFI_STATUS
> +MmMemLibInternalPopulateMmramRanges (
> +  VOID
> +  )
> +{
> +  VOID         &nb= sp;            =       *HobStart;
> +  EFI_HOB_GUID_TYPE       &n= bsp;       *GuidHob;
> +  MM_CORE_DATA_HOB_DATA      &nbs= p;    *DataInHob;
> +  MM_CORE_PRIVATE_DATA       = ;     *MmCorePrivateData;
> +  EFI_HOB_GUID_TYPE       &n= bsp;       *MmramRangesHob;
> +  EFI_MMRAM_HOB_DESCRIPTOR_BLOCK  *MmramRangesHobData;
> +  EFI_MMRAM_DESCRIPTOR       = ;     *MmramDescriptors;
> +
> +  HobStart =3D GetHobList ();
> +  DEBUG ((DEBUG_INFO, "%a - 0x%x\n", __FUNCTION__, Ho= bStart));
> +
> +  //
> +  // Extract MM Core Private context from the Hob. If absent se= arch for
> +  // a Hob containing the MMRAM ranges
> +  //
> +  GuidHob =3D GetNextGuidHob (&gMmCoreDataHobGuid, HobStart= );
> +  if (GuidHob =3D=3D NULL) {
> +    MmramRangesHob =3D GetFirstGuidHob
> (&gEfiMmPeiMmramMemoryReserveGuid);
> +    if (MmramRangesHob =3D=3D NULL) {
> +      return EFI_UNSUPPORTED;
> +    }
> +
> +    MmramRangesHobData =3D GET_GUID_HOB_DATA (MmramRa= ngesHob);
> +    if (MmramRangesHobData =3D=3D NULL || MmramRanges= HobData-
> >Descriptor =3D=3D NULL) {
> +      return EFI_UNSUPPORTED;
> +    }
> +
> +    mMmMemLibInternalMmramCount =3D MmramRangesHobDat= a-
> >NumberOfMmReservedRegions;
> +    MmramDescriptors =3D MmramRangesHobData->Descr= iptor;
> +  } else {
> +    DataInHob =3D GET_GUID_HOB_DATA (GuidHob);
> +    if (DataInHob =3D=3D NULL) {
> +      return EFI_UNSUPPORTED;
> +    }
> +
> +    MmCorePrivateData =3D (MM_CORE_PRIVATE_DATA *) (U= INTN)
> DataInHob->Address;
> +    if (MmCorePrivateData =3D=3D NULL || MmCorePrivat= eData-
> >MmramRanges =3D=3D 0) {
> +      return EFI_UNSUPPORTED;
> +    }
> +
> +    mMmMemLibInternalMmramCount =3D (UINTN) MmCorePri= vateData-
> >MmramRangeCount;
> +    MmramDescriptors =3D (EFI_MMRAM_DESCRIPTOR *) (UI= NTN)
> MmCorePrivateData->MmramRanges;
> +  }
> +
> +  mMmMemLibInternalMmramRanges =3D AllocatePool
> (mMmMemLibInternalMmramCount * sizeof (EFI_MMRAM_DESCRIPTOR));
> +  if (mMmMemLibInternalMmramRanges) {
> +    CopyMem (mMmMemLibInternalMmramRanges,
> +           &n= bsp; MmramDescriptors,
> +           &n= bsp; mMmMemLibInternalMmramCount * sizeof
> (EFI_MMRAM_DESCRIPTOR));
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Deinitialize cached Mmram Ranges.
> +
> +**/
> +VOID
> +MmMemLibInternalFreeMmramRanges (
> +  VOID
> +  )
> +{
> +  if (mMmMemLibInternalMmramRanges !=3D NULL) {
> +    FreePool (mMmMemLibInternalMmramRanges);
> +  }
> +}
> +
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> index 49da02e54e6d..062b0d7a1161 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> @@ -8,6 +8,7 @@
>  #
>  #  Copyright (c) 2015, Intel Corporation. All rights reser= ved.<BR>
>  #  Copyright (c) 2016 - 2018, ARM Limited. All rights rese= rved.<BR>
> +#  Copyright (c) Microsoft Corporation.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -22,16 +23,20 @@ [Defines]
>    PI_SPECIFICATION_VERSION    &nb= sp;  =3D 0x00010032
>    LIBRARY_CLASS      &n= bsp;           =3D MemLib= |MM_STANDALONE
> MM_CORE_STANDALONE
>    CONSTRUCTOR      &nbs= p;             = = =3D MemLibConstructor
> +  DESTRUCTOR        &nb= sp;            =3D M= emLibDestructor
>
>  #
>  # The following information is for reference only and not requi= red by the
> build tools.
>  #
> -#  VALID_ARCHITECTURES       = ;    =3D AARCH64
> +#  VALID_ARCHITECTURES       = ;    =3D IA32 X64 AARCH64
>  #
>
>  [Sources.Common]
>    StandaloneMmMemLib.c
>
> +[Sources.IA32, Sources.X64]
> +  X86StandaloneMmMemLibInternal.c
> +
>  [Sources.AARCH64]
>    AArch64/StandaloneMmMemLibInternal.c
>
> @@ -42,3 +47,9 @@ [Packages]
>  [LibraryClasses]
>    BaseMemoryLib
>    DebugLib
> +  HobLib
> +  MemoryAllocationLib
> +
> +[Guids]
> +  gMmCoreDataHobGuid       &= nbsp;          ## SOMETIMES_CO= NSUMES ## HOB
> +  gEfiMmPeiMmramMemoryReserveGuid     ## SO= METIMES_CONSUMES
> ## HOB
> --
> 2.30.0.windows.1





 

--_000_MWHPR06MB3102B75BA84E56FEED00166EF3D00MWHPR06MB3102namp_--