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

["Ni, Ray" <ray.ni@intel.com>]

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@intel.com>
> Sent: Monday, January 4, 2021 1:14 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] 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..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