[edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c
Laszlo Ersek
lersek at redhat.com
Wed Jan 6 18:46:28 UTC 2021
On 01/06/21 18:56, Laszlo Ersek wrote:
> On 01/06/21 07:48, Zeng, Star wrote:
>> 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..9b1f952c8a74 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 = 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.
>>
>
> This patch is a step in the right direction, but, IMO, it can be improved.
>
> The IsRegisterTableEmpty() function should also handle the case when the
> "RegisterTable" parameter is NULL. The function should then also return
> TRUE.
>
> And then, two more patches would be nice:
>
>
> (2) The register table allocation in
> "UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c" should be removed.
>
> Right now, the code there allocates memory just so it can set the
> "TableLength" and "AllocatedSize" fields to zero, for each "InitialApicId".
>
> It's a waste -- the memory is allocated only for ensuring that
> PiSmmCpuDxeSmm does not program any CPU registers during S3 resume.
>
> Setting "AcpiCpuData->RegisterTable" and
> "AcpiCpuData->PreSmmInitRegisterTable" should have the same effect.
... sorry, I meant: setting "AcpiCpuData->RegisterTable" and
"AcpiCpuData->PreSmmInitRegisterTable" *TO NULL* should have the same
effect.
Thanks,
Laszlo
>
>
> (3) The same simplification should be then mirrored to
> "OvmfPkg/CpuS3DataDxe/CpuS3Data.c".
>
> I can write the OvmfPkg patch myself, of course, but first I'd like to
> see this use case confirmed / supported.
>
> This patch looks OK otherwise.
>
> Thanks!
> Laszlo
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69855): https://edk2.groups.io/g/devel/message/69855
Mute This Topic: https://groups.io/mt/79470147/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