[edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
Ni, Ray
ray.ni at intel.com
Tue Jan 19 12:52:53 UTC 2021
https://github.com/niruiyu/edk2/commit/7091aa50b9d87240b14e5a74398eab010ffc4d3d
Laszlo, Star,
Please check this code change. I verified S3 boot in an internal platform.
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 4:37 PM
> To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io; Zeng, Star <star.zeng at intel.com>
> 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
>
> On 01/15/21 09:33, Ni, Ray wrote:
> > Laszlo,
> > I will test my patches internally and find a way to give you the patch to be included in your V2.
>
> Thank you!
> Laszlo
>
> >
> > 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 (#70538): https://edk2.groups.io/g/devel/message/70538
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