Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function.

["Dong, Eric" <eric.dong@intel.com>]

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;
> >  }
> >
> >  /**
> >