From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.312.1610130293312300271 for ; Fri, 08 Jan 2021 10:24:53 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IwpT+4mX; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610130292; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=46cF/WyumCwrEFi6pFGmJo5VBayvv+J5XPtexnyIJrk=; b=IwpT+4mXt4LUpJcNHDqOvjVqLgcm3fZQRJQwBsASTY23U0cdSh5948VmfY22G4OT8Cq6kY rm5no+i3QRPO0I/6oUiGwl2B8XHMvEECr667E/IkPK8V7BqsiHdIS69LRQdsK3bMCYh6KS uwPWYnIZj7gp0ASpK8Re0ISWxYEu18Q= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-466-FVUXRLLvNE2GBG5AMjDvyA-1; Fri, 08 Jan 2021 13:24:47 -0500 X-MC-Unique: FVUXRLLvNE2GBG5AMjDvyA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 455C7800D62; Fri, 8 Jan 2021 18:24:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-112.ams2.redhat.com [10.36.113.112]) by smtp.corp.redhat.com (Postfix) with ESMTP id 139761001281; Fri, 8 Jan 2021 18:24:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V3] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c To: devel@edk2.groups.io, star.zeng@intel.com Cc: "Ni, Ray" , "Dong, Eric" References: <20210107104650.27404-1-star.zeng@intel.com> <2586b96a-1acf-7cae-ff5f-42b79158c775@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 8 Jan 2021 19:24:44 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 On Behalf Of Laszlo >> Ersek >> Sent: Thursday, January 7, 2021 7:01 PM >> To: Zeng, Star ; devel@edk2.groups.io >> Cc: Ni, Ray ; Dong, Eric >> 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 >>> Reviewed-by: Ray Ni >>> Reviewed-by: Laszlo Ersek >>> Cc: Ray Ni >>> Cc: Eric Dong >>> Cc: Laszlo Ersek >>> --- >>> 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.
>>> +Copyright (c) 2006 - 2021, Intel Corporation. All rights >>> +reserved.
>>> 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. >>> >> >> >> >> >> > > > > > >