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

["Zeng, Star" <star.zeng@intel.com>]

Thanks Laszlo.

Ray, Laszlo or Eric, need your help to commit it if no further comment to the patch.


Thanks,
Star

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Thursday, January 7, 2021 7:01 PM
> To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH V3] UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> SMRAM consumption in CpuS3.c
> 
> On 01/07/21 11:46, Star Zeng 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>
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 73
> > ++++++++++++++++++++++++-------
> >  1 file changed, 56 insertions(+), 17 deletions(-)
> 
> Thanks for the update, my R-b stands.
> 
> Laszlo
> 
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 9592430636ec..724e5460ba6f 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,33 @@ 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;
> > +
> > +  if (RegisterTable != NULL) {
> > +    for (Index = 0; Index < NumberOfCpus; Index++) {
> > +      if (RegisterTable[Index].TableLength != 0) {
> > +        return FALSE;
> > +      }
> > +    }
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> >  /**
> >    Get ACPI CPU data.
> >
> > @@ -1032,23 +1063,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.
> >
> 
> 
> 
> 
>