1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH V4] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c

Zeng, Star posted 1 patch 3 years, 3 months ago
Failed in applying to current master (apply log)
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 74 ++++++++++++++++++++++++-------
1 file changed, 57 insertions(+), 17 deletions(-)
[edk2-devel] [PATCH V4] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c
Posted by Zeng, Star 3 years, 3 months ago 
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@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---

Notes:
   V2: Use "DestinationRegisterTableList[Index].TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY)" directly to cover Ray's comment.
   V3: Handle "RegisterTable == NULL" case to cover Laszlo's comment.
   V4: Add @param[in] for NumberOfCpus parameter of IsRegisterTableEmpty().

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 74 ++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 17 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 9592430636ec..ab7f39aa2bd4 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,34 @@ CopyRegisterTable (
   }
 }
 
+/**
+  Check whether the register table is empty or not.
+
+  @param[in] RegisterTable  Point to the register table.
+  @param[in] NumberOfCpus   Number of CPUs.
+
+  @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;
+
+  if (RegisterTable != NULL) {
+    for (Index = 0; Index < NumberOfCpus; Index++) {
+      if (RegisterTable[Index].TableLength != 0) {
+        return FALSE;
+      }
+    }
+  }
+
+  return TRUE;
+}
+
 /**
   Get ACPI CPU data.
 
@@ -1032,23 +1064,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 (#70097): https://edk2.groups.io/g/devel/message/70097
Mute This Topic: https://groups.io/mt/79586940/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V4] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c
Posted by Laszlo Ersek 3 years, 3 months ago 
On 01/11/21 02:54, 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@intel.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>    V2: Use "DestinationRegisterTableList[Index].TableLength * sizeof (CPU_REGISTER_TABLE_ENTRY)" directly to cover Ray's comment.
>    V3: Handle "RegisterTable == NULL" case to cover Laszlo's comment.
>    V4: Add @param[in] for NumberOfCpus parameter of IsRegisterTableEmpty().
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 74 ++++++++++++++++++++++++-------
>  1 file changed, 57 insertions(+), 17 deletions(-)

Merged as commit e992cc3f4859, via
<https://github.com/tianocore/edk2/pull/1331>.

Thanks
Laszlo

> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 9592430636ec..ab7f39aa2bd4 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,34 @@ CopyRegisterTable (
>    }
>  }
>  
> +/**
> +  Check whether the register table is empty or not.
> +
> +  @param[in] RegisterTable  Point to the register table.
> +  @param[in] NumberOfCpus   Number of CPUs.
> +
> +  @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;
> +
> +  if (RegisterTable != NULL) {
> +    for (Index = 0; Index < NumberOfCpus; Index++) {
> +      if (RegisterTable[Index].TableLength != 0) {
> +        return FALSE;
> +      }
> +    }
> +  }
> +
> +  return TRUE;
> +}
> +
>  /**
>    Get ACPI CPU data.
>  
> @@ -1032,23 +1064,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.
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70115): https://edk2.groups.io/g/devel/message/70115
Mute This Topic: https://groups.io/mt/79586940/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.