[edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
Laszlo Ersek
lersek at redhat.com
Fri Jan 15 08:37:10 UTC 2021
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 (#70392): https://edk2.groups.io/g/devel/message/70392
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