[edk2-devel] [PATCH V3] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c
Zeng, Star
star.zeng at intel.com
Mon Jan 11 01:56:15 UTC 2021
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: Saturday, January 9, 2021 2:25 AM
> To: devel at edk2.groups.io; Zeng, Star <star.zeng at intel.com>
> Cc: Ni, Ray <ray.ni at intel.com>; Dong, Eric <eric.dong at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> Laszlo
> >> Ersek
> >> Sent: Thursday, January 7, 2021 7:01 PM
> >> To: Zeng, Star <star.zeng at intel.com>; devel at edk2.groups.io
> >> Cc: Ni, Ray <ray.ni at intel.com>; Dong, Eric <eric.dong at 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 at intel.com>
> >>> Reviewed-by: Ray Ni <ray.ni at intel.com>
> >>> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
> >>> Cc: Ray Ni <ray.ni at intel.com>
> >>> Cc: Eric Dong <eric.dong at intel.com>
> >>> Cc: Laszlo Ersek <lersek at 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.
> >>>
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70098): https://edk2.groups.io/g/devel/message/70098
Mute This Topic: https://groups.io/mt/79496286/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list