CPU count limitation in CpuMpPei BIST processing

CPU count limitation in CpuMpPei BIST processing

[Laszlo Ersek]

Hi Eric, Mike, Ray,

with "master" being at commit 3cde0d553d93, please consider the
CollectBistDataFromPpi() function in "UefiCpuPkg/CpuMpPei/CpuBist.c":

> /**
>   Collects BIST data from PPI.
>
>   This function collects BIST data from Sec Platform Information2 PPI
>   or SEC Platform Information PPI.
>
>   @param PeiServices         Pointer to PEI Services Table
>
> **/
> VOID
> CollectBistDataFromPpi (
>   IN CONST EFI_PEI_SERVICES             **PeiServices
>   )
> {
>   EFI_STATUS                            Status;
>   EFI_PEI_PPI_DESCRIPTOR                *SecInformationDescriptor;
>   EFI_SEC_PLATFORM_INFORMATION_RECORD2  *SecPlatformInformation2;
>   EFI_SEC_PLATFORM_INFORMATION_RECORD   *SecPlatformInformation;
>   UINTN                                 NumberOfData;
>   EFI_SEC_PLATFORM_INFORMATION_CPU      *CpuInstance;
>   EFI_SEC_PLATFORM_INFORMATION_CPU      BspCpuInstance;
>   UINTN                                 ProcessorNumber;
>   UINTN                                 CpuIndex;
>   EFI_PROCESSOR_INFORMATION             ProcessorInfo;
>   EFI_HEALTH_FLAGS                      BistData;
>   UINTN                                 NumberOfProcessors;
>   UINTN                                 NumberOfEnabledProcessors;
>   UINTN                                 BistInformationSize;
>   EFI_SEC_PLATFORM_INFORMATION_RECORD2  *PlatformInformationRecord2;
>   EFI_SEC_PLATFORM_INFORMATION_CPU      *CpuInstanceInHob;
>
>
>   MpInitLibGetNumberOfProcessors(&NumberOfProcessors, &NumberOfEnabledProcessors);
>
>   BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
>                         sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
>   Status = PeiServicesAllocatePool (
>              (UINTN) BistInformationSize,
>              (VOID **) &PlatformInformationRecord2
>              );
>   ASSERT_EFI_ERROR (Status);

If "NumberOfProcessors" is large enough, such as ~1024, then
"BistInformationSize" will exceed ~64KB, and PeiServicesAllocatePool()
will fail with EFI_OUT_OF_RESOURCES. The reason is that pool allocations
in PEI are implemented with memory alloaction HOBs, and HOBs can't be
larger than ~64KB. (See PeiAllocatePool() in
"MdeModulePkg/Core/Pei/Memory/MemoryServices.c".)

It wouldn't be too hard to fall back to PeiServicesAllocatePages() here.
Page allocation has a good chance to succeed at this point, because
CpuMpPei only calls CollectBistDataFromPpi() *after* permanent PEI RAM
has been installed:

- Entry point:

  CpuMpPeimInit()                            [UefiCpuPkg/CpuMpPei/CpuMpPei.c]
    PeiServicesNotifyPpi(mPostMemNotifyList)
      gEfiPeiMemoryDiscoveredPpiGuid
      MemoryDiscoveredPpiNotifyCallback

- PPI notify:

  MemoryDiscoveredPpiNotifyCallback() [UefiCpuPkg/CpuMpPei/CpuPaging.c]
    InitializeCpuMpWorker()           [UefiCpuPkg/CpuMpPei/CpuMpPei.c]
      CollectBistDataFromPpi()        [UefiCpuPkg/CpuMpPei/CpuBist.c]

However, even if such an allocation of *pages* succeeded, there would be
another problem:

>   PlatformInformationRecord2->NumberOfCpus = (UINT32)NumberOfProcessors;
>
>   SecPlatformInformation2 = NULL;
>   SecPlatformInformation  = NULL;
>   NumberOfData            = 0;
>   CpuInstance             = NULL;
>   //
>   // Get BIST information from Sec Platform Information2 Ppi firstly
>   //
>   Status = GetBistInfoFromPpi (
>              PeiServices,
>              &gEfiSecPlatformInformation2PpiGuid,
>              &SecInformationDescriptor,
>              (VOID *) &SecPlatformInformation2,
>              NULL
>              );
>   if (Status == EFI_SUCCESS) {
>     //
>     // Sec Platform Information2 PPI includes BSP/APs' BIST information
>     //
>     NumberOfData = SecPlatformInformation2->NumberOfCpus;
>     CpuInstance  = SecPlatformInformation2->CpuInstance;
>   } else {
>     //
>     // Otherwise, get BIST information from Sec Platform Information Ppi
>     //
>     Status = GetBistInfoFromPpi (
>                PeiServices,
>                &gEfiSecPlatformInformationPpiGuid,
>                &SecInformationDescriptor,
>                (VOID *) &SecPlatformInformation,
>                NULL
>                );
>     if (Status == EFI_SUCCESS) {
>       NumberOfData = 1;
>       //
>       // SEC Platform Information only includes BSP's BIST information
>       // and does not have BSP's APIC ID
>       //
>       BspCpuInstance.CpuLocation = GetInitialApicId ();
>       BspCpuInstance.InfoRecord.IA32HealthFlags.Uint32  = SecPlatformInformation->IA32HealthFlags.Uint32;
>       CpuInstance = &BspCpuInstance;
>     } else {
>       DEBUG ((EFI_D_INFO, "Does not find any stored CPU BIST information from PPI!\n"));
>     }
>   }
>   for (ProcessorNumber = 0; ProcessorNumber < NumberOfProcessors; ProcessorNumber ++) {
>     MpInitLibGetProcessorInfo (ProcessorNumber, &ProcessorInfo, &BistData);
>     for (CpuIndex = 0; CpuIndex < NumberOfData; CpuIndex ++) {
>       ASSERT (CpuInstance != NULL);
>       if (ProcessorInfo.ProcessorId == CpuInstance[CpuIndex].CpuLocation) {
>         //
>         // Update processor's BIST data if it is already stored before
>         //
>         BistData = CpuInstance[CpuIndex].InfoRecord.IA32HealthFlags;
>       }
>     }
>     if (BistData.Uint32 != 0) {
>       //
>       // Report Status Code that self test is failed
>       //
>       REPORT_STATUS_CODE (
>         EFI_ERROR_CODE | EFI_ERROR_MAJOR,
>         (EFI_COMPUTING_UNIT_HOST_PROCESSOR | EFI_CU_HP_EC_SELF_TEST)
>         );
>     }
>     DEBUG ((EFI_D_INFO, "  APICID - 0x%08x, BIST - 0x%08x\n",
>             (UINT32) ProcessorInfo.ProcessorId,
>             BistData
>             ));
>     CpuInstanceInHob = PlatformInformationRecord2->CpuInstance;
>     CpuInstanceInHob[ProcessorNumber].CpuLocation = (UINT32) ProcessorInfo.ProcessorId;
>     CpuInstanceInHob[ProcessorNumber].InfoRecord.IA32HealthFlags = BistData;
>   }
>
>   //
>   // Build SecPlatformInformation2 PPI GUIDed HOB that also could be consumed
>   // by CPU MP driver to get CPU BIST data
>   //
>   BuildGuidDataHob (
>     &gEfiSecPlatformInformation2PpiGuid,
>     PlatformInformationRecord2,
>     (UINTN) BistInformationSize
>     );

Namely the BuildGuidDataHob() call above. Regardless of how we allocate
"PlatformInformationRecord2", here an attempt is made to copy its
contents into a GUID-ed HOB -- but the ~64KB HOB size limit applies to
GUID-ed HOBs just the same. In "MdePkg/Library/PeiHobLib/HobLib.c",
function BuildGuidHob(), we'd trip the following assert:

  //
  // Make sure that data length is not too long.
  //
  ASSERT (DataLength <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

So... what is the whole idea behind this HOB?

The GUID itself is from the PI spec, but there it stands for a PPI
(namely EFI_SEC_PLATFORM_INFORMATION2_PPI), not a HOB.

It seems to me like the GUIDed HOB is an implementation detail in edk2;
it is not a standard HOB:

- CpuMpPei uses the HOB for stashing the information that underlies the
  EFI_SEC_PLATFORM_INFORMATION2_PPI.PlatformInformation2() service that
  CpuMpPei provides. (See the SecPlatformInformation2() function quoted
  at the bottom.)

- CpuDxe consumes the HOB for fetching information about CPUs in the
  CollectBistDataFromHob() function [UefiCpuPkg/CpuDxe/CpuMp.c].

I can see no other uses for this HOB.

In my opinion:

- The allocation should be a page allocation,

- The GUID-ed HOB should be replaced with a Dynamic PCD that points to
  the allocated pages.

This would remove the size limit, and allow both PEI and DXE to continue
accessing the same information. (PCDs are not available in the DXE_CORE,
while HOBs are, but the DXE_CORE has no use for the CPU Built-In Self
Test (BIST) information.)

What are your thoughts?

--*--

Some other comments on this code:

- The "SecPlatformInformation2" local variables in GetBistInfoFromPpi()
  and CollectBistDataFromPpi() [UefiCpuPkg/CpuMpPei/CpuBist.c] hide the
  identically named SecPlatformInformation2() function, which is
  declared in the same file. This is a serious coding style bug IMO.

- The "BistData" local variable in CollectBistDataFromPpi() has type
  EFI_HEALTH_FLAGS, which is a union type. But "BistData" is used in
  assignments (with the assignment operator "="), and logged with
  DEBUG() macro, as if it were a scalar (UINT32). The assignment per se
  is valid C, but an edk2 coding style violation. Printing the whole
  "BistData" union with "%x" in DEBUG is also very questionable.

Thanks
Laszlo

>
>   if (SecPlatformInformation2 != NULL) {
>     if (NumberOfData < NumberOfProcessors) {
>       //
>       // Reinstall SecPlatformInformation2 PPI to include new BIST information
>       //
>       Status = PeiServicesReInstallPpi (
>                  SecInformationDescriptor,
>                  &mPeiSecPlatformInformation2Ppi
>                  );
>       ASSERT_EFI_ERROR (Status);
>     }
>   } else {
>     //
>     // Install SecPlatformInformation2 PPI
>     //
>     Status = PeiServicesInstallPpi (&mPeiSecPlatformInformation2Ppi);
>     ASSERT_EFI_ERROR(Status);
>   }
> }
>



> /** @file
>   Update and publish processors' BIST information.
>
>   Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
>
> #include "CpuMpPei.h"
>
> EFI_SEC_PLATFORM_INFORMATION2_PPI mSecPlatformInformation2Ppi = {
>   SecPlatformInformation2
> };
>
> EFI_PEI_PPI_DESCRIPTOR mPeiSecPlatformInformation2Ppi = {
>   (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>   &gEfiSecPlatformInformation2PpiGuid,
>   &mSecPlatformInformation2Ppi
> };
>
> /**
>   Implementation of the PlatformInformation2 service in EFI_SEC_PLATFORM_INFORMATION2_PPI.
>
>   @param  PeiServices                The pointer to the PEI Services Table.
>   @param  StructureSize              The pointer to the variable describing size of the input buffer.
>   @param  PlatformInformationRecord2 The pointer to the EFI_SEC_PLATFORM_INFORMATION_RECORD2.
>
>   @retval EFI_SUCCESS                The data was successfully returned.
>   @retval EFI_BUFFER_TOO_SMALL       The buffer was too small. The current buffer size needed to
>                                      hold the record is returned in StructureSize.
>
> **/
> EFI_STATUS
> EFIAPI
> SecPlatformInformation2 (
>   IN CONST EFI_PEI_SERVICES                   **PeiServices,
>   IN OUT UINT64                               *StructureSize,
>      OUT EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2
>   )
> {
>   EFI_HOB_GUID_TYPE       *GuidHob;
>   VOID                    *DataInHob;
>   UINTN                   DataSize;
>
>   GuidHob = GetFirstGuidHob (&gEfiSecPlatformInformation2PpiGuid);
>   if (GuidHob == NULL) {
>     *StructureSize = 0;
>     return EFI_SUCCESS;
>   }
>
>   DataInHob = GET_GUID_HOB_DATA (GuidHob);
>   DataSize  = GET_GUID_HOB_DATA_SIZE (GuidHob);
>
>   //
>   // return the information from BistHob
>   //
>   if ((*StructureSize) < (UINT64) DataSize) {
>     *StructureSize = (UINT64) DataSize;
>     return EFI_BUFFER_TOO_SMALL;
>   }
>
>   *StructureSize = (UINT64) DataSize;
>   CopyMem (PlatformInformationRecord2, DataInHob, DataSize);
>   return EFI_SUCCESS;
> }
>
> /**
>   Worker function to get CPUs' BIST by calling SecPlatformInformationPpi
>   or SecPlatformInformation2Ppi.
>
>   @param  PeiServices         Pointer to PEI Services Table
>   @param  Guid                PPI Guid
>   @param  PpiDescriptor       Return a pointer to instance of the
>                               EFI_PEI_PPI_DESCRIPTOR
>   @param  BistInformationData Pointer to BIST information data
>   @param  BistInformationSize Return the size in bytes of BIST information
>
>   @retval EFI_SUCCESS         Retrieve of the BIST data successfully
>   @retval EFI_NOT_FOUND       No sec platform information(2) ppi export
>   @retval EFI_DEVICE_ERROR    Failed to get CPU Information
>
> **/
> EFI_STATUS
> GetBistInfoFromPpi (
>   IN CONST EFI_PEI_SERVICES     **PeiServices,
>   IN CONST EFI_GUID             *Guid,
>      OUT EFI_PEI_PPI_DESCRIPTOR **PpiDescriptor,
>      OUT VOID                   **BistInformationData,
>      OUT UINT64                 *BistInformationSize OPTIONAL
>   )
> {
>   EFI_STATUS                            Status;
>   EFI_SEC_PLATFORM_INFORMATION2_PPI     *SecPlatformInformation2Ppi;
>   EFI_SEC_PLATFORM_INFORMATION_RECORD2  *SecPlatformInformation2;
>   UINT64                                InformationSize;
>
>   Status = PeiServicesLocatePpi (
>              Guid,                                // GUID
>              0,                                   // INSTANCE
>              PpiDescriptor,                       // EFI_PEI_PPI_DESCRIPTOR
>              (VOID **)&SecPlatformInformation2Ppi // PPI
>              );
>   if (Status == EFI_NOT_FOUND) {
>     return EFI_NOT_FOUND;
>   }
>
>   if (Status == EFI_SUCCESS) {
>     //
>     // Get the size of the sec platform information2(BSP/APs' BIST data)
>     //
>     InformationSize         = 0;
>     SecPlatformInformation2 = NULL;
>     Status = SecPlatformInformation2Ppi->PlatformInformation2 (
>                                            PeiServices,
>                                            &InformationSize,
>                                            SecPlatformInformation2
>                                            );
>     if (Status == EFI_BUFFER_TOO_SMALL) {
>       Status = PeiServicesAllocatePool (
>                  (UINTN) InformationSize,
>                  (VOID **) &SecPlatformInformation2
>                  );
>       if (Status == EFI_SUCCESS) {
>         //
>         // Retrieve BIST data
>         //
>         Status = SecPlatformInformation2Ppi->PlatformInformation2 (
>                                                PeiServices,
>                                                &InformationSize,
>                                                SecPlatformInformation2
>                                                );
>         if (Status == EFI_SUCCESS) {
>           *BistInformationData = SecPlatformInformation2;
>           if (BistInformationSize != NULL) {
>             *BistInformationSize = InformationSize;
>           }
>           return EFI_SUCCESS;
>         }
>       }
>     }
>   }
>
>   return EFI_DEVICE_ERROR;
> }
>