Re: [PATCH 3/3] OvmfPkg/CpuS3DataDxe: do not allocate useless register tables

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@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);
>    }
>  
>    //
>