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. VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801MB1790.eurprd08.prod.outlook.com">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 <[email protected]> Suggested-by: Fan Jeff <[email protected]> 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 <[email protected]> > Cc: Ruiyu Ni <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <[email protected]> > --- > .../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; > } > > /** > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
