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

[Laszlo Ersek <lersek@redhat.com>]

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