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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Friday, January 15, 2021 4:37 PM > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io; Zeng, Star > <star.z...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com>; Philippe Mathieu-Daudé > <phi...@redhat.com>; Kumar, Rahul1 > <rahul1.ku...@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > >> Sent: Friday, January 15, 2021 1:36 AM > >> To: Zeng, Star <star.z...@intel.com>; Ni, Ray <ray...@intel.com>; > >> devel@edk2.groups.io > >> Cc: Dong, Eric <eric.d...@intel.com>; Philippe Mathieu-Daudé > >> <phi...@redhat.com>; Kumar, Rahul1 > >> <rahul1.ku...@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...@intel.com> > >>>> Sent: Wednesday, January 13, 2021 4:12 PM > >>>> To: Zeng, Star <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > >>>> devel@edk2.groups.io > >>>> Cc: Dong, Eric <eric.d...@intel.com>; Philippe Mathieu-Daudé > >>>> <phi...@redhat.com>; Kumar, Rahul1 <rahul1.ku...@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.z...@intel.com> > >>>>> Sent: Wednesday, January 13, 2021 3:17 PM > >>>>> To: Ni, Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > >>>>> devel@edk2.groups.io > >>>>> Cc: Dong, Eric <eric.d...@intel.com>; Philippe Mathieu-Daudé > >>>>> <phi...@redhat.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; Zeng, > >>>> Star > >>>>> <star.z...@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...@intel.com> > >>>>> Sent: Wednesday, January 13, 2021 2:12 PM > >>>>> To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io > >>>>> Cc: Dong, Eric <eric.d...@intel.com>; Philippe Mathieu-Daudé > >>>>> <phi...@redhat.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; Zeng, > >>>> Star > >>>>> <star.z...@intel.com> > >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate > >>>> useless > >>>>> register tables > >>>>> > >>>>> Reviewed-by: Ray Ni <ray...@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 <ler...@redhat.com> > >>>>>> Sent: Wednesday, January 13, 2021 3:20 AM > >>>>>> To: devel@edk2.groups.io > >>>>>> Cc: Dong, Eric <eric.d...@intel.com>; Philippe Mathieu-Daudé > >>>>>> <phi...@redhat.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; Ni, > >>>> Ray > >>>>>> <ray...@intel.com>; Zeng, Star <star.z...@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.d...@intel.com> > >>>>>> Cc: Philippe Mathieu-Daudé <phi...@redhat.com> > >>>>>> Cc: Rahul Kumar <rahul1.ku...@intel.com> > >>>>>> Cc: Ray Ni <ray...@intel.com> > >>>>>> Cc: Star Zeng <star.z...@intel.com> > >>>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159 > >>>>>> Signed-off-by: Laszlo Ersek <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-