From: "Laszlo Ersek" <lersek@redhat.com>
To: "Ni, Ray" <ray.ni@intel.com>, "Zeng, Star" <star.zeng@intel.com>,
"devel@edk2.groups.io" <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
Date: Wed, 13 Jan 2021 10:16:36 +0100 [thread overview]
Message-ID: <65d582e9-95cf-7ceb-e379-f1df68a57478@redhat.com> (raw)
In-Reply-To: <CO1PR11MB4930CC9FEAAA2E0BB93AA08B8CA90@CO1PR11MB4930.namprd11.prod.outlook.com>
On 01/13/21 09:12, Ni, Ray wrote:
> Star,
> Thanks for pointing that.
> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is allocated but CpuS3Data
> doesn't do that.
Sorry that I missed this; I did grep the tree for
[PreSmmInit]RegisterTable, but apparently didn't pay enough attention to
RegisterCpuFeaturesLib. OVMF does not use that library.
Ray: can you please post your fix as a standalone patch?
Or else, if it's more convenient, please just push the fix somewhere (as
a standalone patch) where I can fetch it from. It would be great if you
could write a commit message too.
Then, I will post a v2 of this series, including your fix for
RegisterCpuFeaturesLib (as a patch under your authorship).
Star and Eric can then review your patch on the list -- neither I nor
you can review your patch, as you are the author of it, and I'll be
posting it.
Thanks!
Laszlo
>
> 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;
> + }
>
> //
> // 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/RegisterC
>> 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-13 9:16 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 [this message]
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
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=65d582e9-95cf-7ceb-e379-f1df68a57478@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