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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-