[edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni at intel.com>

(I guess you don't want to put Redhat copyright in the patch 1&2 so the copyright year is not updated.
Since it's a simple change that make the logic more neat, I am ok with that.)

Will you create a pull request to merge all 3 patches together?

> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: Wednesday, January 13, 2021 3:20 AM
> To: devel at edk2.groups.io
> Cc: Dong, Eric <eric.dong at intel.com>; Philippe Mathieu-Daudé
> <philmd at redhat.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Ni, Ray
> <ray.ni at intel.com>; Zeng, Star <star.zeng at intel.com>
> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> register tables
> 
> 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.
> 
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
> Cc: Rahul Kumar <rahul1.kumar at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Star Zeng <star.zeng at intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
> 
> Notes:
>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the OVMF
> IA32
>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>     as long as no CPUs are hot-plugged.
> 
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index 2be335d91903..078af36cfb41 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>    UINTN                      NumberOfCpus;
>    UINTN                      NumberOfEnabledProcessors;
>    VOID                       *Stack;
> -  UINTN                      TableSize;
> -  CPU_REGISTER_TABLE         *RegisterTable;
> -  UINTN                      Index;
> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>    UINTN                      GdtSize;
>    UINTN                      IdtSize;
>    VOID                       *Gdt;
> @@ -255,34 +251,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);
> -
> -    for (Index = 0; Index < NumberOfCpus; Index++) {
> -      Status = MpServices->GetProcessorInfo (
> -                           MpServices,
> -                           Index,
> -                           &ProcessorInfoBuffer
> -                           );
> -      ASSERT_EFI_ERROR (Status);
> -
> -      RegisterTable[Index].InitialApicId      =
> (UINT32)ProcessorInfoBuffer.ProcessorId;
> -      RegisterTable[Index].TableLength        = 0;
> -      RegisterTable[Index].AllocatedSize      = 0;
> -      RegisterTable[Index].RegisterTableEntry = 0;
> -
> -      RegisterTable[NumberOfCpus + Index].InitialApicId      =
> (UINT32)ProcessorInfoBuffer.ProcessorId;
> -      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);
>    }
> 
>    //
> --
> 2.19.1.3.g30247aa5d201
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70202): https://edk2.groups.io/g/devel/message/70202
Mute This Topic: https://groups.io/mt/79632018/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-