From: "Laszlo Ersek" <lersek@redhat.com>
To: "Ni, Ray" <ray.ni@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"Zeng, Star" <star.zeng@intel.com>
Cc: "Dong, Eric" <eric.dong@intel.com>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Kumar, Rahul1" <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
Date: Fri, 15 Jan 2021 09:37:10 +0100 [thread overview]
Message-ID: <fc0ebddb-5fd8-ba83-5cea-2af1f800bc42@redhat.com> (raw)
In-Reply-To: <CO1PR11MB493010E4FA0B4BFACD6168EF8CA70@CO1PR11MB4930.namprd11.prod.outlook.com>
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Friday, January 15, 2021 1:36 AM
>> To: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
>> <rahul1.kumar@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@intel.com>
>>>> Sent: Wednesday, January 13, 2021 4:12 PM
>>>> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>>> devel@edk2.groups.io
>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@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@intel.com>
>>>>> Sent: Wednesday, January 13, 2021 3:17 PM
>>>>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>>>> devel@edk2.groups.io
>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
>>>> Star
>>>>> <star.zeng@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@intel.com>
>>>>> Sent: Wednesday, January 13, 2021 2:12 PM
>>>>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
>>>> Star
>>>>> <star.zeng@intel.com>
>>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
>>>> useless
>>>>> register tables
>>>>>
>>>>> Reviewed-by: Ray Ni <ray.ni@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@redhat.com>
>>>>>> Sent: Wednesday, January 13, 2021 3:20 AM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
>>>> Ray
>>>>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@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@intel.com>
>>>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>>>>>> Signed-off-by: Laszlo Ersek <lersek@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
>>>>>>
>>>
>>
>>
>>
>>
>>
>
next prev parent reply other threads:[~2021-01-15 8:37 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-12 19:19 [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
2021-01-13 5:00 ` Ni, Ray
2021-01-15 9:28 ` Philippe Mathieu-Daudé
2021-01-15 9:34 ` Laszlo Ersek
2021-01-12 19:19 ` [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables Laszlo Ersek
2021-01-13 6:12 ` Ni, Ray
2021-01-13 7:16 ` Zeng, Star
2021-01-13 8:12 ` Ni, Ray
2021-01-13 8:40 ` Zeng, Star
2021-01-13 9:16 ` Laszlo Ersek
2021-01-14 1:55 ` Zeng, Star
2021-01-14 17:35 ` Laszlo Ersek
2021-01-15 8:33 ` [edk2-devel] " Ni, Ray
2021-01-15 8:37 ` Laszlo Ersek [this message]
2021-01-19 12:52 ` Ni, Ray
2021-01-19 13:48 ` Laszlo Ersek
2021-01-13 9:10 ` Laszlo Ersek
2021-01-12 19:19 ` [PATCH 3/3] OvmfPkg/CpuS3DataDxe: " Laszlo Ersek
2021-01-14 12:53 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fc0ebddb-5fd8-ba83-5cea-2af1f800bc42@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox