[PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c

[PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c

[Zeng, Star]

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>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@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.
-- 
2.21.0.windows.1

Re: [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>


> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Wednesday, January 6, 2021 2:48 PM
> To: devel@edk2.groups.io
> Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH V2] 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@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@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.
> --
> 2.21.0.windows.1

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

[Laszlo Ersek]

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@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@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.


(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

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

[Laszlo Ersek]

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@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Laszlo Ersek <lersek@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
> 
> 
> 
> 
> 
> 

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

[Ni, Ray]

> 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.

Laszlo,
It's a good suggestion.
We also need to change CpuS3.c:SetRegister() to directly return
when mAcpiCpuData.[PreSmmInit]RegisterTable is NULL.

Considering the urgency of this patch (fix a system hang caused by smram shortage),
do you agree that we can first let this patch in?

Thanks,
Ray

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

[Laszlo Ersek]

On 01/07/21 06:23, Ni, Ray wrote:
>> 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.
>
> Laszlo,
> It's a good suggestion.
> We also need to change CpuS3.c:SetRegister() to directly return
> when mAcpiCpuData.[PreSmmInit]RegisterTable is NULL.

The current (v2) patch already contains that shortcut; please see:

> @@ -487,6 +487,9 @@ SetRegister (
>    } else {
>      RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.RegisterTable;
>    }
> +  if (RegisterTables == NULL) {
> +    return;
> +  }
>
>    InitApicId = GetInitialApicId ();
>    RegisterTable = NULL;

So that's fine, IMO.

The data flow is the following:

(1) GetAcpiCpuData() determines whether each one of the two register
tables *ouside of SMRAM* is empty or not;

(2) for each one that is empty (outside of SMRAM), the pointer to the
*copy in SMRAM* is set to NULL,

(3) for each one that is not empty (outside of SMRAM), an SMRAM copy is
made,

(4) SetRegister() correctly deals -- already in this v2 patch -- with
the the pointer into SMRAM being NULL.


My update request refers to step (1). Namely, when we determine whether
each register table *ouside of SMRAM* is empty or not, we should also
recognize when the table (outside of SMRAM) doesn't even *exist*.

Basically I'm asking that Star please post a v3 with the following
update squashed into the patch:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 9b1f952c8a74..0e7a4ba5369d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -998,6 +998,10 @@ IsRegisterTableEmpty (
>  {
>    UINTN                     Index;
>
> +  if (RegisterTable == NULL) {
> +    return TRUE;
> +  }
> +
>    for (Index = 0; Index < NumberOfCpus; Index++) {
>      if (RegisterTable[Index].TableLength != 0) {
>        return FALSE;

Back to your email:

On 01/07/21 06:23, Ni, Ray wrote:
>
> Considering the urgency of this patch (fix a system hang caused by smram shortage),
> do you agree that we can first let this patch in?

I agree that the CpuS3DataDxe changes can be done separately, later.

However, I'd like the above small hunk -- in IsRegisterTableEmpty() --
to be squashed into the current patch, before committing it.

If you think we should do this small update on commit, rather than
posting a v3, that's fine with me as well. Just please don't merge the
v2 patch without the IsRegisterTableEmpty() extension that I'm asking
for, above.

With the IsRegisterTableEmpty() update, even without a v3 posting:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

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

[Zeng, Star]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 7, 2021 6:10 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Zeng, Star
> <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm:
> Reduce SMRAM consumption in CpuS3.c
> 
> On 01/07/21 06:23, Ni, Ray wrote:
> >> 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.
> >
> > Laszlo,
> > It's a good suggestion.
> > We also need to change CpuS3.c:SetRegister() to directly return when
> > mAcpiCpuData.[PreSmmInit]RegisterTable is NULL.
> 
> The current (v2) patch already contains that shortcut; please see:
> 
> > @@ -487,6 +487,9 @@ SetRegister (
> >    } else {
> >      RegisterTables = (CPU_REGISTER_TABLE
> *)(UINTN)mAcpiCpuData.RegisterTable;
> >    }
> > +  if (RegisterTables == NULL) {
> > +    return;
> > +  }
> >
> >    InitApicId = GetInitialApicId ();
> >    RegisterTable = NULL;
> 
> So that's fine, IMO.
> 
> The data flow is the following:
> 
> (1) GetAcpiCpuData() determines whether each one of the two register
> tables *ouside of SMRAM* is empty or not;
> 
> (2) for each one that is empty (outside of SMRAM), the pointer to the *copy
> in SMRAM* is set to NULL,
> 
> (3) for each one that is not empty (outside of SMRAM), an SMRAM copy is
> made,
> 
> (4) SetRegister() correctly deals -- already in this v2 patch -- with the the
> pointer into SMRAM being NULL.
> 
> 
> My update request refers to step (1). Namely, when we determine whether
> each register table *ouside of SMRAM* is empty or not, we should also
> recognize when the table (outside of SMRAM) doesn't even *exist*.
> 
> Basically I'm asking that Star please post a v3 with the following update
> squashed into the patch:
> 
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 9b1f952c8a74..0e7a4ba5369d 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > @@ -998,6 +998,10 @@ IsRegisterTableEmpty (  {
> >    UINTN                     Index;
> >
> > +  if (RegisterTable == NULL) {
> > +    return TRUE;
> > +  }
> > +
> >    for (Index = 0; Index < NumberOfCpus; Index++) {
> >      if (RegisterTable[Index].TableLength != 0) {
> >        return FALSE;
> 
> Back to your email:
> 
> On 01/07/21 06:23, Ni, Ray wrote:
> >
> > Considering the urgency of this patch (fix a system hang caused by
> > smram shortage), do you agree that we can first let this patch in?
> 
> I agree that the CpuS3DataDxe changes can be done separately, later.
> 
> However, I'd like the above small hunk -- in IsRegisterTableEmpty() -- to be
> squashed into the current patch, before committing it.
> 
> If you think we should do this small update on commit, rather than posting a
> v3, that's fine with me as well. Just please don't merge the
> v2 patch without the IsRegisterTableEmpty() extension that I'm asking for,
> above.
> 
> With the IsRegisterTableEmpty() update, even without a v3 posting:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks Laszlo and Ray

This patch is majorly to reduce SMRAM consumption in *consumer* side of "RegisterTable".
I was preparing PATCH V3 before you sent this reply. 😊

About the change to *producer* side of "RegisterTable", UefiCpuPkg\Library\RegisterCpuFeaturesLib depended on CpuS3DataDxe before, the dependency seems have been removed by a6daab1f6cb836e4147324bb85fc17930be14e87. We may can do the change after doing more confirmation.

I just sent PATCH V3 with your both RB (suppose you both agree 😊) with a very very little difference to your proposal.

Thanks,
Star

> 
> Thanks
> Laszlo

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

[Laszlo Ersek]

On 01/07/21 11:47, Zeng, Star wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, January 7, 2021 6:10 PM
>> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Zeng, Star
>> <star.zeng@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm:
>> Reduce SMRAM consumption in CpuS3.c
>>
>> On 01/07/21 06:23, Ni, Ray wrote:
>>>> 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.
>>>
>>> Laszlo,
>>> It's a good suggestion.
>>> We also need to change CpuS3.c:SetRegister() to directly return when
>>> mAcpiCpuData.[PreSmmInit]RegisterTable is NULL.
>>
>> The current (v2) patch already contains that shortcut; please see:
>>
>>> @@ -487,6 +487,9 @@ SetRegister (
>>>    } else {
>>>      RegisterTables = (CPU_REGISTER_TABLE
>> *)(UINTN)mAcpiCpuData.RegisterTable;
>>>    }
>>> +  if (RegisterTables == NULL) {
>>> +    return;
>>> +  }
>>>
>>>    InitApicId = GetInitialApicId ();
>>>    RegisterTable = NULL;
>>
>> So that's fine, IMO.
>>
>> The data flow is the following:
>>
>> (1) GetAcpiCpuData() determines whether each one of the two register
>> tables *ouside of SMRAM* is empty or not;
>>
>> (2) for each one that is empty (outside of SMRAM), the pointer to the *copy
>> in SMRAM* is set to NULL,
>>
>> (3) for each one that is not empty (outside of SMRAM), an SMRAM copy is
>> made,
>>
>> (4) SetRegister() correctly deals -- already in this v2 patch -- with the the
>> pointer into SMRAM being NULL.
>>
>>
>> My update request refers to step (1). Namely, when we determine whether
>> each register table *ouside of SMRAM* is empty or not, we should also
>> recognize when the table (outside of SMRAM) doesn't even *exist*.
>>
>> Basically I'm asking that Star please post a v3 with the following update
>> squashed into the patch:
>>
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> index 9b1f952c8a74..0e7a4ba5369d 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> @@ -998,6 +998,10 @@ IsRegisterTableEmpty (  {
>>>    UINTN                     Index;
>>>
>>> +  if (RegisterTable == NULL) {
>>> +    return TRUE;
>>> +  }
>>> +
>>>    for (Index = 0; Index < NumberOfCpus; Index++) {
>>>      if (RegisterTable[Index].TableLength != 0) {
>>>        return FALSE;
>>
>> Back to your email:
>>
>> On 01/07/21 06:23, Ni, Ray wrote:
>>>
>>> Considering the urgency of this patch (fix a system hang caused by
>>> smram shortage), do you agree that we can first let this patch in?
>>
>> I agree that the CpuS3DataDxe changes can be done separately, later.
>>
>> However, I'd like the above small hunk -- in IsRegisterTableEmpty() -- to be
>> squashed into the current patch, before committing it.
>>
>> If you think we should do this small update on commit, rather than posting a
>> v3, that's fine with me as well. Just please don't merge the
>> v2 patch without the IsRegisterTableEmpty() extension that I'm asking for,
>> above.
>>
>> With the IsRegisterTableEmpty() update, even without a v3 posting:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks Laszlo and Ray
> 
> This patch is majorly to reduce SMRAM consumption in *consumer* side of "RegisterTable".
> I was preparing PATCH V3 before you sent this reply. 😊
> 
> About the change to *producer* side of "RegisterTable", UefiCpuPkg\Library\RegisterCpuFeaturesLib depended on CpuS3DataDxe before, the dependency seems have been removed by a6daab1f6cb836e4147324bb85fc17930be14e87. We may can do the change after doing more confirmation.

OK. Sounds good.

In any case, for the OVMF platform anyway, OvmfPkg/CpuS3DataDxe will be
able to benefit from the update, as OVMF does not use
RegisterCpuFeaturesLib. OvmfPkg/CpuS3DataDxe only has to satisfy
PiSmmCpuDxeSmm, with the ACPI_CPU_DATA contents.

Of course, if RegisterCpuFeaturesLib (after commit a6daab1f6cb8) enables
a similar simplification for UefiCpuPkg/CpuS3DataDxe too, that's even
better.

> I just sent PATCH V3 with your both RB (suppose you both agree 😊) with a very very little difference to your proposal.

Yes, it looks good.

Thanks!
Laszlo