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

["Ni, Ray" <ray.ni@intel.com>]

Laszlo,
I will test my patches internally and find a way to give you the patch to be included in your V2.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, January 15, 2021 1:36 AM
> To: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.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: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
> 
> Hi Star,
> 
> On 01/14/21 02:55, Zeng, Star wrote:
> > Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the
> function's second call.
> 
> thank you for following up here -- could you or Ray please propose an
> actual patch for RegisterCpuFeaturesLib, as I requested before?
> 
> Posting the patch is not necessary; I only need to fetch it from your
> personal repo(s) -- you can even send me the repo / branch reference
> off-list. I would include the RegisterCpuFeaturesLib patch in my v2
> posting of this series.
> 
> Thanks!
> Laszlo
> 
> >
> >> -----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;
> >> +  }
> >>
> >
> > Add check as below here.
> >
> > if (AcpiCpuData->RegisterTable == 0) {
> >
> >>    //
> >>    // 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/Regist
> >> erC
> >>> 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
> >>>>
> >
> 
> 
> 
> 
>