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

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

It should work.

Thanks,
Star
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, January 13, 2021 4:12 PM
To: Zeng, Star <star.zeng@intel.com>; 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>
Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

Star,
Thanks for pointing that.
RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is allocated but CpuS3Data doesn't do that.

Can following change fix the problem?

--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -937,21 +937,19 @@ GetAcpiCpuData (
   EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
 
   AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
-  if (AcpiCpuData != NULL) {
-    return AcpiCpuData;
-  }
-
-  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
-  ASSERT (AcpiCpuData != NULL);
+  if (AcpiCpuData == NULL) {
+    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+    ASSERT (AcpiCpuData != NULL);
 
-  //
-  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
-  //
-  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
-  ASSERT_EFI_ERROR (Status);
+    //
+    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
+    //
+    Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
+    ASSERT_EFI_ERROR (Status);
 
-  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
-  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+    GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;  }
 
   //
   // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs

Thanks,
Ray

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Wednesday, January 13, 2021 3:17 PM
> To: Ni, Ray <ray.ni@intel.com>; 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
> 
> 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/Regis
> terC
> puFeaturesLib/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
> >