From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, 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
Date: Fri, 8 Jan 2021 19:24:44 +0100 [thread overview]
Message-ID: <f1e54132-f9e6-79e2-f346-41d55a14715a@redhat.com> (raw)
In-Reply-To: <DM6PR11MB4058FA8F5C28B99BFB20863FE3AE0@DM6PR11MB4058.namprd11.prod.outlook.com>
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
>> -----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.
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
>
next prev parent reply other threads:[~2021-01-08 18:24 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-07 10:46 [PATCH V3] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c Zeng, Star
2021-01-07 11:00 ` Laszlo Ersek
2021-01-08 14:13 ` [edk2-devel] " Zeng, Star
2021-01-08 18:24 ` Laszlo Ersek [this message]
2021-01-11 1:56 ` Zeng, Star
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f1e54132-f9e6-79e2-f346-41d55a14715a@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox