Hi Laszlo,
> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Saturday, August 4, 2018 12:46 AM > To: Dong, Eric <[email protected]> > Cc: [email protected]; Ni, Ruiyu <[email protected]>; Marvin Häuser > <[email protected]>; Jeff Fan <[email protected]>; Ard > Biesheuvel <[email protected]> > 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 <[email protected]> > Suggested-by: Fan Jeff <[email protected]> > 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 <[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/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; > > } > > > > /** > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
