[edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
Ni, Ray
ray.ni at intel.com
Fri Jan 15 08:33:04 UTC 2021
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, January 15, 2021 1:36 AM
> To: Zeng, Star <star.zeng at intel.com>; Ni, Ray <ray.ni at intel.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: [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 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 (#70390): https://edk2.groups.io/g/devel/message/70390
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