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

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

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Saturday, January 9, 2021 2:25 AM
> To: devel@edk2.groups.io; Zeng, Star <star.zeng@intel.com>
> 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
> 
> Hi Star,
> 
> On 01/08/21 15:13, Zeng, Star wrote:
> > Thanks Laszlo.
> >
> > Ray, Laszlo or Eric, need your help to commit it if no further comment to
> the patch.
> 
> the merge request
> 
>   https://github.com/tianocore/edk2/pull/1326
> 
> failed, due to the following ECC error report:
> 
>   ERROR - EFI coding style error
>   ERROR - *Error code: 9002
>   ERROR - *The function headers should follow Doxygen special
> documentation blocks in section 2.3.5
>   ERROR - *file:
> //home/vsts/work/1/s/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>   ERROR - *Line number: 985
>   ERROR - *in Comment, <@retval TRUE> does NOT consistent with
> parameter name NumberOfCpus
> 
> As far as I understand, the issue is that the "NumberOfCpus" parameter of
> IsRegisterTableEmpty() does not have its own @param[in] stanza, in the
> preceding comment block.
> 
> Please submit a v4.

Thanks Laszlo for the help.
Just sent the V4 patch.

Thanks,
Star

> 
> Thanks,
> Laszlo
> 
> >> -----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.
> >>>
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> > 
> >
> >