[edk2-devel] CPU count limitation in CpuMpPei BIST processing
Laszlo Ersek
lersek at redhat.com
Wed Jun 30 19:52:14 UTC 2021
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;
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77376): https://edk2.groups.io/g/devel/message/77376
Mute This Topic: https://groups.io/mt/83900998/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list