* [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function. @ 2018-08-03 2:10 Eric Dong 2018-08-03 16:45 ` Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Eric Dong @ 2018-08-03 2:10 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni Current code logic can't confirm CpuS3DataDxe driver start before CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid. Add implementation for AllocateAcpiCpuData function to remove this assumption. Pass OS boot and resume from S3 test. Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong <eric.dong@intel.com> --- .../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++-- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c index 902a339529..0722db6c64 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c @@ -207,11 +207,62 @@ AllocateAcpiCpuData ( VOID ) { + EFI_STATUS Status; + UINTN NumberOfCpus; + UINTN NumberOfEnabledProcessors; + ACPI_CPU_DATA *AcpiCpuData; + EFI_PHYSICAL_ADDRESS Address; + UINTN TableSize; + CPU_REGISTER_TABLE *RegisterTable; + UINTN Index; + EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer; + + Address = BASE_4GB - 1; + Status = gBS->AllocatePages ( + AllocateMaxAddress, + EfiACPIMemoryNVS, + EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)), + &Address + ); + ASSERT_EFI_ERROR (Status); + AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address; + ASSERT (AcpiCpuData != NULL); + + GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors); + AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus; + // - // CpuS3DataDxe will do it. + // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs // - ASSERT (FALSE); - return NULL; + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); + Address = BASE_4GB - 1; + Status = gBS->AllocatePages ( + AllocateMaxAddress, + EfiACPIMemoryNVS, + EFI_SIZE_TO_PAGES (TableSize), + &Address + ); + ASSERT_EFI_ERROR (Status); + RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address; + + for (Index = 0; Index < NumberOfCpus; Index++) { + Status = GetProcessorInformation (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); + + return AcpiCpuData; } /** -- 2.15.0.windows.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function. 2018-08-03 2:10 [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function Eric Dong @ 2018-08-03 16:45 ` Laszlo Ersek 2018-08-08 1:04 ` Dong, Eric 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2018-08-03 16:45 UTC (permalink / raw) To: Eric Dong Cc: edk2-devel, Ruiyu Ni, Marvin Häuser, Jeff Fan, Ard Biesheuvel Hello Eric, OVMF does not include CpuFeaturesPei / CpuFeaturesDxe, and so it doesn't consume this library. I can't provide test results, but I have some superficial comments. First, I'm adding Marvin and Jeff -- I *vaguely* recall that this issue may have been raised by Marvin. Hm... yes, it seems so: [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency. http://mid.mail-archive.com/VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801MB1790.eurprd08.prod.outlook.com https://lists.01.org/pipermail/edk2-devel/2018-May/025163.html I have three questions here: (1) Do we have a TianoCore BZ about this? (2) If not, does the currently proposed commit message capture the issue in enough detail? Should we reference Marvin's initial report and/or a TianoCore BZ (if we have one)? (3) The implementation seems to follow Jeff's idea. Marvin and Jeff, can you please help with the review? Either way, I propose the following two tags to be appended: Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com> Suggested-by: Fan Jeff <vanjeff_919@hotmail.com> And one technical question follows below: On 08/03/18 04:10, Eric Dong wrote: > Current code logic can't confirm CpuS3DataDxe driver start before > CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not > valid. Add implementation for AllocateAcpiCpuData function to remove > this assumption. > > Pass OS boot and resume from S3 test. > > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.dong@intel.com> > --- > .../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++-- > 1 file changed, 54 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > index 902a339529..0722db6c64 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > @@ -207,11 +207,62 @@ AllocateAcpiCpuData ( > VOID > ) > { > + EFI_STATUS Status; > + UINTN NumberOfCpus; > + UINTN NumberOfEnabledProcessors; > + ACPI_CPU_DATA *AcpiCpuData; > + EFI_PHYSICAL_ADDRESS Address; > + UINTN TableSize; > + CPU_REGISTER_TABLE *RegisterTable; > + UINTN Index; > + EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer; > + > + Address = BASE_4GB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); I understand that this code is for IA32 and X64 only, but still, Ard has recently introduced a DxeServicesLib API for this kind of allocation: it's called AllocatePeiAccessiblePages(). See commit a40e0b7aa918. (4) Is it feasible to use that function here (and in the second instance below)? >From a library dependency standpoint, I think it should be fine: we are modifying the DXE instance of the RegisterCpuFeaturesLib class, so a dependency on DxeServicesLib should be in order. Now, if this allocation *must* be satisfied from below 4GB, even if the PEI phase has access to >=4GB RAM (as determined by PhitHob->EfiFreeMemoryTop > MAX_UINT32 ), then my suggestion is wrong (because in that case, AllocatePeiAccessiblePages() wouldn't restrict the allocation under 4GB). CC'ing Ard too. Thanks! Laszlo > + AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address; > + ASSERT (AcpiCpuData != NULL); > + > + GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors); > + AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus; > + > // > - // CpuS3DataDxe will do it. > + // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs > // > - ASSERT (FALSE); > - return NULL; > + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); > + Address = BASE_4GB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (TableSize), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address; > + > + for (Index = 0; Index < NumberOfCpus; Index++) { > + Status = GetProcessorInformation (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); > + > + return AcpiCpuData; > } > > /** > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function. 2018-08-03 16:45 ` Laszlo Ersek @ 2018-08-08 1:04 ` Dong, Eric 0 siblings, 0 replies; 3+ messages in thread From: Dong, Eric @ 2018-08-08 1:04 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Ni, Ruiyu, Marvin Häuser, Jeff Fan, Ard Biesheuvel Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Saturday, August 4, 2018 12:46 AM > To: Dong, Eric <eric.dong@intel.com> > Cc: edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Marvin Häuser > <Marvin.Haeuser@outlook.com>; Jeff Fan <vanjeff_919@hotmail.com>; Ard > Biesheuvel <ard.biesheuvel@linaro.org> > Subject: Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement > AllocateAcpiCpuData function. > > Hello Eric, > > OVMF does not include CpuFeaturesPei / CpuFeaturesDxe, and so it doesn't > consume this library. I can't provide test results, but I have some superficial > comments. > > First, I'm adding Marvin and Jeff -- I *vaguely* recall that this issue may have > been raised by Marvin. Hm... yes, it seems so: > > [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency. > > http://mid.mail- > archive.com/VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801 > MB1790.eurprd08.prod.outlook.com > https://lists.01.org/pipermail/edk2-devel/2018-May/025163.html > > I have three questions here: > > (1) Do we have a TianoCore BZ about this? > > (2) If not, does the currently proposed commit message capture the issue in > enough detail? Should we reference Marvin's initial report and/or a > TianoCore BZ (if we have one)? > Yes, it has an BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=959 I will include this info in my next version patch. > (3) The implementation seems to follow Jeff's idea. Marvin and Jeff, can you > please help with the review? > > Either way, I propose the following two tags to be appended: > > Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com> > Suggested-by: Fan Jeff <vanjeff_919@hotmail.com> > Sure, will include them in my next version patch. > > And one technical question follows below: > > On 08/03/18 04:10, Eric Dong wrote: > > Current code logic can't confirm CpuS3DataDxe driver start before > > CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid. > > Add implementation for AllocateAcpiCpuData function to remove this > > assumption. > > > > Pass OS boot and resume from S3 test. > > > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <eric.dong@intel.com> > > --- > > .../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++-- > > 1 file changed, 54 insertions(+), 3 deletions(-) > > > > diff --git > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib. > > c > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib. > > c > > index 902a339529..0722db6c64 100644 > > --- > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib. > > c > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeatures > > +++ Lib.c > > @@ -207,11 +207,62 @@ AllocateAcpiCpuData ( > > VOID > > ) > > { > > + EFI_STATUS Status; > > + UINTN NumberOfCpus; > > + UINTN NumberOfEnabledProcessors; > > + ACPI_CPU_DATA *AcpiCpuData; > > + EFI_PHYSICAL_ADDRESS Address; > > + UINTN TableSize; > > + CPU_REGISTER_TABLE *RegisterTable; > > + UINTN Index; > > + EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer; > > + > > + Address = BASE_4GB - 1; > > + Status = gBS->AllocatePages ( > > + AllocateMaxAddress, > > + EfiACPIMemoryNVS, > > + EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)), > > + &Address > > + ); > > + ASSERT_EFI_ERROR (Status); > > I understand that this code is for IA32 and X64 only, but still, Ard has recently > introduced a DxeServicesLib API for this kind of allocation: > it's called AllocatePeiAccessiblePages(). See commit a40e0b7aa918. > > (4) Is it feasible to use that function here (and in the second instance below)? > After check the code, I found current usage modal not required it below 4G, also it doesn't need ACPI NVS type memory. I will update it in my next version change. > From a library dependency standpoint, I think it should be fine: we are > modifying the DXE instance of the RegisterCpuFeaturesLib class, so a > dependency on DxeServicesLib should be in order. > > Now, if this allocation *must* be satisfied from below 4GB, even if the PEI > phase has access to >=4GB RAM (as determined by > > PhitHob->EfiFreeMemoryTop > MAX_UINT32 > > ), then my suggestion is wrong (because in that case, > AllocatePeiAccessiblePages() wouldn't restrict the allocation under 4GB). > > CC'ing Ard too. > > Thanks! > Laszlo > > > + AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address; ASSERT > > + (AcpiCpuData != NULL); > > + > > + GetNumberOfProcessor (&NumberOfCpus, > &NumberOfEnabledProcessors); > > + AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus; > > + > > // > > - // CpuS3DataDxe will do it. > > + // Allocate buffer for empty RegisterTable and > > + PreSmmInitRegisterTable for all CPUs > > // > > - ASSERT (FALSE); > > - return NULL; > > + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); > > + Address = BASE_4GB - 1; Status = gBS->AllocatePages ( > > + AllocateMaxAddress, > > + EfiACPIMemoryNVS, > > + EFI_SIZE_TO_PAGES (TableSize), > > + &Address > > + ); > > + ASSERT_EFI_ERROR (Status); > > + RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address; > > + > > + for (Index = 0; Index < NumberOfCpus; Index++) { > > + Status = GetProcessorInformation (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); > > + > > + return AcpiCpuData; > > } > > > > /** > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-08 1:04 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-03 2:10 [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function Eric Dong 2018-08-03 16:45 ` Laszlo Ersek 2018-08-08 1:04 ` Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox