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

["Zeng, Star" <star.zeng@intel.com>]

DxeRegisterCpuFeaturesLib still has some execution sequence dependency to UefiCpuPkg CpuS3DataDxe.
There is ASSERT to happen at  https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with this patch runs before DxeRegisterCpuFeaturesLib.

Thanks,
Star
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, January 13, 2021 2:12 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

Reviewed-by: Ray Ni <ray.ni@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@redhat.com>
> Sent: Wednesday, January 13, 2021 3:20 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé 
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Zeng, Star <star.zeng@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@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@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
>