[edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
Zeng, Star
star.zeng at intel.com
Thu Jan 14 01:55:30 UTC 2021
Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the function's second call.
> -----Original Message-----
> From: Ni, Ray <ray.ni at intel.com>
> Sent: Wednesday, January 13, 2021 4:12 PM
> To: Zeng, Star <star.zeng at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> 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>
> 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 at intel.com>
> > Sent: Wednesday, January 13, 2021 3:17 PM
> > To: Ni, Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> > 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>; Zeng,
> Star
> > <star.zeng at 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 at intel.com>
> > Sent: Wednesday, January 13, 2021 2:12 PM
> > To: Laszlo Ersek <lersek at redhat.com>; 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>; Zeng,
> Star
> > <star.zeng at intel.com>
> > Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> useless
> > register tables
> >
> > 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 (#70266): https://edk2.groups.io/g/devel/message/70266
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]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list