[edk2-devel] [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c
Zeng, Star
star.zeng at intel.com
Wed Jan 6 06:39:55 UTC 2021
Good catch, thanks.
It was my over consideration.
Let me send a new version patch.
Thanks,
Star
-----Original Message-----
From: Ni, Ray <ray.ni at intel.com>
Sent: Wednesday, January 6, 2021 2:34 PM
To: Zeng, Star <star.zeng at intel.com>; devel at edk2.groups.io
Cc: Dong, Eric <eric.dong at intel.com>; Laszlo Ersek <lersek at redhat.com>
Subject: RE: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c
Star,
Just one minor comments:
DestinationRegisterTableList[Index].AllocatedSize = (UINT32) MultU64x32 (DestinationRegisterTableList[Index].TableLength, sizeof (CPU_REGISTER_TABLE_ENTRY));
Please directly use " DestinationRegisterTableList[Index].TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY)".
Thanks,
Ray
> -----Original Message-----
> From: Zeng, Star <star.zeng at intel.com>
> Sent: Monday, January 4, 2021 1:14 PM
> To: devel at edk2.groups.io
> Cc: Zeng, Star <star.zeng at intel.com>; Ni, Ray <ray.ni at intel.com>;
> Dong, Eric <eric.dong at intel.com>; Laszlo Ersek <lersek at redhat.com>
> Subject: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption
> in CpuS3.c
>
> This patch makes two refinements to reduce SMRAM consumption in CpuS3.c.
> 1. Only do CopyRegisterTable() when register table is not empty,
> IsRegisterTableEmpty() is created to check whether the register table
> is empty or not.
>
> Take empty PreSmmInitRegisterTable as example, about 24K SMRAM
> consumption
> could be reduced when mAcpiCpuData.NumberOfCpus=1024.
> sizeof (CPU_REGISTER_TABLE) = 24
> mAcpiCpuData.NumberOfCpus = 1024 = 1K
> mAcpiCpuData.NumberOfCpus * sizeof (CPU_REGISTER_TABLE) = 24K
>
> 2. Only copy table entries buffer instead of whole buffer.
> AllocatedSize in SourceRegisterTableList is the whole buffer size.
> Actually, only the table entries buffer needs to be copied, and the size
> is TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY).
>
> Take AllocatedSize=0x1000=4096, TableLength=100 and
> NumberOfCpus=1024 as example,
> about 1696K SMRAM consumption could be reduced.
> sizeof (CPU_REGISTER_TABLE_ENTRY) = 24
> TableLength = 100
> TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY) = 2400
> AllocatedSize = 0x1000 = 4096
> AllocatedSize - TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY) =
> 4096 -
> 2400 = 1696
> NumberOfCpus = 1024 = 1K
> NumberOfCpus * (AllocatedSize - TableLength * sizeof
> (CPU_REGISTER_TABLE_ENTRY)) = 1696K
>
> This patch also corrects the CopyRegisterTable() function description.
>
> Signed-off-by: Star Zeng <star.zeng at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 71 +++++++++++++++++++++++------
> --
> 1 file changed, 54 insertions(+), 17 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 9592430636ec..d87c2efb87e7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -1,7 +1,7 @@
> /** @file
> Code for Processor S3 restoration
>
> -Copyright (c) 2006 - 2020, Intel Corporation. All rights
> reserved.<BR>
> +Copyright (c) 2006 - 2021, Intel Corporation. All rights
> +reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -487,6 +487,9 @@ SetRegister (
> } else {
> RegisterTables = (CPU_REGISTER_TABLE
> *)(UINTN)mAcpiCpuData.RegisterTable;
> }
> + if (RegisterTables == NULL) {
> + return;
> + }
>
> InitApicId = GetInitialApicId ();
> RegisterTable = NULL;
> @@ -948,7 +951,7 @@ InitSmmS3ResumeState ( }
>
> /**
> - Copy register table from ACPI NVS memory into SMRAM.
> + Copy register table from non-SMRAM into SMRAM.
>
> @param[in] DestinationRegisterTableList Points to destination register table.
> @param[in] SourceRegisterTableList Points to source register table.
> @@ -967,7 +970,8 @@ CopyRegisterTable (
>
> CopyMem (DestinationRegisterTableList, SourceRegisterTableList,
> NumberOfCpus * sizeof (CPU_REGISTER_TABLE));
> for (Index = 0; Index < NumberOfCpus; Index++) {
> - if (DestinationRegisterTableList[Index].AllocatedSize != 0) {
> + if (DestinationRegisterTableList[Index].TableLength != 0) {
> + DestinationRegisterTableList[Index].AllocatedSize = (UINT32)
> + MultU64x32
> (DestinationRegisterTableList[Index].TableLength, sizeof
> (CPU_REGISTER_TABLE_ENTRY));
> RegisterTableEntry = AllocateCopyPool (
> DestinationRegisterTableList[Index].AllocatedSize,
> (VOID
> *)(UINTN)SourceRegisterTableList[Index].RegisterTableEntry
> @@ -978,6 +982,31 @@ CopyRegisterTable (
> }
> }
>
> +/**
> + Check whether the register table is empty or not.
> +
> + @param[in] RegisterTable Point to the register table.
> +
> + @retval TRUE The register table is empty.
> + @retval FALSE The register table is not empty.
> +**/
> +BOOLEAN
> +IsRegisterTableEmpty (
> + IN CPU_REGISTER_TABLE *RegisterTable,
> + IN UINT32 NumberOfCpus
> + )
> +{
> + UINTN Index;
> +
> + for (Index = 0; Index < NumberOfCpus; Index++) {
> + if (RegisterTable[Index].TableLength != 0) {
> + return FALSE;
> + }
> + }
> +
> + return TRUE;
> +}
> +
> /**
> Get ACPI CPU data.
>
> @@ -1032,23 +1061,31 @@ GetAcpiCpuData (
>
> CopyMem ((VOID *)(UINTN)mAcpiCpuData.IdtrProfile, (VOID
> *)(UINTN)AcpiCpuData->IdtrProfile, sizeof (IA32_DESCRIPTOR));
>
> - mAcpiCpuData.PreSmmInitRegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus
> * sizeof (CPU_REGISTER_TABLE));
> - ASSERT (mAcpiCpuData.PreSmmInitRegisterTable != 0);
> + if (!IsRegisterTableEmpty ((CPU_REGISTER_TABLE
> + *)(UINTN)AcpiCpuData-
> >PreSmmInitRegisterTable, mAcpiCpuData.NumberOfCpus)) {
> + mAcpiCpuData.PreSmmInitRegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus
> * sizeof (CPU_REGISTER_TABLE));
> + ASSERT (mAcpiCpuData.PreSmmInitRegisterTable != 0);
>
> - CopyRegisterTable (
> - (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.PreSmmInitRegisterTable,
> - (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->PreSmmInitRegisterTable,
> - mAcpiCpuData.NumberOfCpus
> - );
> + CopyRegisterTable (
> + (CPU_REGISTER_TABLE
> *)(UINTN)mAcpiCpuData.PreSmmInitRegisterTable,
> + (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->PreSmmInitRegisterTable,
> + mAcpiCpuData.NumberOfCpus
> + );
> + } else {
> + mAcpiCpuData.PreSmmInitRegisterTable = 0; }
>
> - mAcpiCpuData.RegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus
> * sizeof (CPU_REGISTER_TABLE));
> - ASSERT (mAcpiCpuData.RegisterTable != 0);
> + if (!IsRegisterTableEmpty ((CPU_REGISTER_TABLE
> + *)(UINTN)AcpiCpuData-
> >RegisterTable, mAcpiCpuData.NumberOfCpus)) {
> + mAcpiCpuData.RegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus
> * sizeof (CPU_REGISTER_TABLE));
> + ASSERT (mAcpiCpuData.RegisterTable != 0);
>
> - CopyRegisterTable (
> - (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.RegisterTable,
> - (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->RegisterTable,
> - mAcpiCpuData.NumberOfCpus
> - );
> + CopyRegisterTable (
> + (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.RegisterTable,
> + (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->RegisterTable,
> + mAcpiCpuData.NumberOfCpus
> + );
> + } else {
> + mAcpiCpuData.RegisterTable = 0;
> + }
>
> //
> // Copy AP's GDT, IDT and Machine Check handler into SMRAM.
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69790): https://edk2.groups.io/g/devel/message/69790
Mute This Topic: https://groups.io/mt/79418569/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list