From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.11988.1609955813512038163 for ; Wed, 06 Jan 2021 09:56:54 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=crt4Lh5T; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1609955812; 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=8NXPPgNmYQrrbeBSbD4OsONpecwPE6aB0rYGd94cCLQ=; b=crt4Lh5TIh0iCvb9xmtNRSyYA9u9EH6Oo+QYbj1+HdQbU6VarbRhrE966wfV1Mg7M6r6gk PrBkQQI2KtYW3BJL4RBukaCcteVvRuabxrilJty8XwhrDkTppZPVyjDhUzJRs35Ta/uZ+M IKPMx+HX4ZeLENmI9Q6Iv982yIK493Y= 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-143-fGKfaXQlP2uc_4PdMdmP0g-1; Wed, 06 Jan 2021 12:56:48 -0500 X-MC-Unique: fGKfaXQlP2uc_4PdMdmP0g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 888661005504; Wed, 6 Jan 2021 17:56:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-198.ams2.redhat.com [10.36.113.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4FE3262464; Wed, 6 Jan 2021 17:56:45 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM consumption in CpuS3.c To: devel@edk2.groups.io, star.zeng@intel.com Cc: Ray Ni , Eric Dong References: <20210106064825.18984-1-star.zeng@intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 6 Jan 2021 18:56:45 +0100 MIME-Version: 1.0 In-Reply-To: <20210106064825.18984-1-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 On 01/06/21 07:48, Zeng, Star 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 > Cc: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 71 +++++++++++++++++++++++-------- > 1 file changed, 54 insertions(+), 17 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 9592430636ec..9b1f952c8a74 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,31 @@ 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; > + > + for (Index = 0; Index < NumberOfCpus; Index++) { > + if (RegisterTable[Index].TableLength != 0) { > + return FALSE; > + } > + } > + > + return TRUE; > +} > + > /** > Get ACPI CPU data. > > @@ -1032,23 +1061,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. > This patch is a step in the right direction, but, IMO, it can be improved. The IsRegisterTableEmpty() function should also handle the case when the "RegisterTable" parameter is NULL. The function should then also return TRUE. And then, two more patches would be nice: (2) The register table allocation in "UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c" should be removed. Right now, the code there allocates memory just so it can set the "TableLength" and "AllocatedSize" fields to zero, for each "InitialApicId". It's a waste -- the memory is allocated only for ensuring that PiSmmCpuDxeSmm does not program any CPU registers during S3 resume. Setting "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable" should have the same effect. (3) The same simplification should be then mirrored to "OvmfPkg/CpuS3DataDxe/CpuS3Data.c". I can write the OvmfPkg patch myself, of course, but first I'd like to see this use case confirmed / supported. This patch looks OK otherwise. Thanks! Laszlo