[edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
Laszlo Ersek
lersek at redhat.com
Thu Jan 14 17:35:47 UTC 2021
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 (#70319): https://edk2.groups.io/g/devel/message/70319
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