[edk2-devel] [PATCH 3/3] OvmfPkg/CpuS3DataDxe: do not allocate useless register tables
Ard Biesheuvel
ard.biesheuvel at arm.com
Thu Jan 14 12:53:20 UTC 2021
On 1/12/21 8:19 PM, Laszlo Ersek wrote:
> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> arrays in ACPI_CPU_DATA just so every processor in the system can have its
> own empty register table, matched by APIC ID. This has never been useful
> in practice.
>
> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), simply leave both
> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> initialized to the zero address. This simplifies the driver, and saves
> both normal RAM (boot services data type memory) and -- in PiSmmCpuDxeSmm
> -- SMRAM.
>
> (This simplification backs out a good chunk of commit 1158fc8e2c7b
> ("OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug", 2020-03-04).
> But CpuS3DataDxe still differs between UefiCpuPkg and OvmfPkg, due to the
> latter supporting CPU hotplug; thus, we can't remove OvmfPkg/CpuS3DataDxe
> altogether.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel at arm.com>
> ---
> OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 -
> OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 70 +-------------------
> 2 files changed, 1 insertion(+), 70 deletions(-)
>
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index ceae1d4078c7..228d5ae1b260 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -42,7 +42,6 @@ [LibraryClasses]
> BaseLib
> BaseMemoryLib
> DebugLib
> - IoLib
> MemoryAllocationLib
> MtrrLib
> UefiBootServicesTableLib
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> index bac7285aa2f3..5ffe1f3cd74e 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -23,7 +23,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> -#include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/MtrrLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> @@ -31,9 +30,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Protocol/MpService.h>
> #include <Guid/EventGroup.h>
>
> -#include <IndustryStandard/Q35MchIch9.h>
> -#include <IndustryStandard/QemuCpuHotplug.h>
> -
> //
> // Data structure used to allocate ACPI_CPU_DATA and its supporting structures
> //
> @@ -168,17 +164,12 @@ CpuS3DataInitialize (
> EFI_MP_SERVICES_PROTOCOL *MpServices;
> UINTN NumberOfCpus;
> VOID *Stack;
> - UINTN TableSize;
> - CPU_REGISTER_TABLE *RegisterTable;
> - UINTN Index;
> - EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> UINTN GdtSize;
> UINTN IdtSize;
> VOID *Gdt;
> VOID *Idt;
> EFI_EVENT Event;
> ACPI_CPU_DATA *OldAcpiCpuData;
> - BOOLEAN FetchPossibleApicIds;
>
> if (!PcdGetBool (PcdAcpiS3Enable)) {
> return EFI_UNSUPPORTED;
> @@ -193,13 +184,7 @@ CpuS3DataInitialize (
> ASSERT (AcpiCpuDataEx != NULL);
> AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>
> - //
> - // The "SMRAM at default SMBASE" feature guarantees that
> - // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.
> - //
> - FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);
> -
> - if (FetchPossibleApicIds) {
> + if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
> NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> } else {
> UINTN NumberOfEnabledProcessors;
> @@ -271,59 +256,6 @@ CpuS3DataInitialize (
> AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
> AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
> CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
> - } else {
> - //
> - // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> - //
> - TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> - RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> - ASSERT (RegisterTable != NULL);
> -
> - if (FetchPossibleApicIds) {
> - //
> - // Write a valid selector so that other hotplug registers can be
> - // accessed.
> - //
> - IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);
> - //
> - // We'll be fetching the APIC IDs.
> - //
> - IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,
> - QEMU_CPUHP_CMD_GET_ARCH_ID);
> - }
> - for (Index = 0; Index < NumberOfCpus; Index++) {
> - UINT32 InitialApicId;
> -
> - if (FetchPossibleApicIds) {
> - IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,
> - (UINT32)Index);
> - InitialApicId = IoRead32 (
> - ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);
> - } else {
> - Status = MpServices->GetProcessorInfo (
> - MpServices,
> - Index,
> - &ProcessorInfoBuffer
> - );
> - ASSERT_EFI_ERROR (Status);
> - InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
> - }
> -
> - DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,
> - (UINT64)Index, InitialApicId));
> -
> - RegisterTable[Index].InitialApicId = InitialApicId;
> - RegisterTable[Index].TableLength = 0;
> - RegisterTable[Index].AllocatedSize = 0;
> - RegisterTable[Index].RegisterTableEntry = 0;
> -
> - RegisterTable[NumberOfCpus + Index].InitialApicId = InitialApicId;
> - RegisterTable[NumberOfCpus + Index].TableLength = 0;
> - RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
> - RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
> - }
> - AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> - AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> }
>
> //
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70288): https://edk2.groups.io/g/devel/message/70288
Mute This Topic: https://groups.io/mt/79632020/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