Thanks for the feedback Chasel and Sai! I have incorporated all your
suggestions.
> -----Original Message-----
> From: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>
> Sent: Thursday, May 9, 2024 6:15 PM
> To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>;
> devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.c...@intel.com>; Liming Gao
> <gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@intel.com>; Chuang,
> Rosen <rosen.chu...@intel.com>; Kasbekar, Saloni
> <saloni.kasbe...@intel.com>
> Subject: RE: [edk2-platforms] [PATCH v1 1/2] MinPlatform: Add
> MpInfo2HobPei
>
> Hi Nate,
> Looks good.
> In addition to optimization suggested by Chasel to save unnecessary call to
> locate PPI, you might also want to consider checking for checking "no error"
> status for locate PPI and perhaps avoid a "goto" label.
> With that, Reviewed-by: Sai Chaganty <rangasai.v.chaga...@intel.com> for
> the whole patch series.
>
> Thanks,
> Sai
>
>
> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>
> Sent: Wednesday, May 8, 2024 5:09 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.c...@intel.com>; Liming Gao
> <gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@intel.com>; Chaganty,
> Rangasai V <rangasai.v.chaga...@intel.com>; Chuang, Rosen
> <rosen.chu...@intel.com>; Kasbekar, Saloni <saloni.kasbe...@intel.com>
> Subject: [edk2-platforms] [PATCH v1 1/2] MinPlatform: Add MpInfo2HobPei
>
> MpInfo2HobPei provides backwards compatibility between FSP binaries built
> with older versions of EDK II and the latest EDK II.
>
> Newer versions of CpuMpPei produce the gMpInformation2HobGuid. This
> HOB is required by newer implementations of the CPU DXE driver, however
> older versions of CpuMpPei do not produce it. This PEIM will check if
> CpuMpPei creates gMpInformation2HobGuid and if it does not it creates it.
>
> Cc: Chasel Chiu <chasel.c...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Sai Chaganty <rangasai.v.chaga...@intel.com>
> Cc: Rosen Chuang <rosen.chu...@intel.com>
> Cc: Saloni Kasbekar <saloni.kasbe...@intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desim...@intel.com>
> ---
> .../FspWrapper/MpInfo2HobPei/MpInfo2HobPei.c | 236
> ++++++++++++++++++
> .../MpInfo2HobPei/MpInfo2HobPei.inf | 47 ++++
> .../Intel/MinPlatformPkg/MinPlatformPkg.dsc | 3 +-
> 3 files changed, 285 insertions(+), 1 deletion(-) create mode 100644
> Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2HobPe
> i.c
> create mode 100644
> Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2HobPe
> i.inf
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2Hob
> Pei.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2Hob
> Pei.c
> new file mode 100644
> index 0000000000..4cbc4cf7e6
> --- /dev/null
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2Hob
> P
> +++ ei.c
> @@ -0,0 +1,236 @@
> +/** @file
> + Multi-processor Info 2 HOB PEIM.
> +
> + The purpose of this PEIM is to provide backwards compatibility
> + between FSP binaries built with older versions of EDK II and the latest
> EDK II.
> +
> + Newer versions of CpuMpPei produce the gMpInformation2HobGuid. This
> + HOB is required by newer implementations of the CPU DXE driver,
> + however older versions of CpuMpPei do not produce it. This PEIM will
> + check if CpuMpPei creates gMpInformation2HobGuid and if it does not it
> creates it.
> +
> +Copyright (c) 2024, Intel Corporation. All rights reserved.<BR>
> +
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +#include <Library/BaseLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h> #include
> +<Library/PeiServicesLib.h>
> +
> +#include <Ppi/MpServices2.h>
> +#include <Guid/MpInformation2.h>
> +#include <Register/Cpuid.h>
> +
> +typedef struct {
> + EDKII_PEI_MP_SERVICES2_PPI *CpuMpPpi2;
> + UINT8 *CoreTypes;
> +} GET_PROCESSOR_CORE_TYPE_BUFFER;
> +
> +/**
> + Get CPU core type.
> +
> + @param[in, out] Buffer Argument of the procedure.
> +**/
> +VOID
> +EFIAPI
> +GetProcessorCoreType (
> + IN OUT VOID *Buffer
> + )
> +{
> + EFI_STATUS Status;
> + UINT8 *CoreTypes;
> + CPUID_NATIVE_MODEL_ID_AND_CORE_TYPE_EAX
> NativeModelIdAndCoreTypeEax;
> + UINTN ProcessorIndex;
> + GET_PROCESSOR_CORE_TYPE_BUFFER *Params;
> +
> + Params = (GET_PROCESSOR_CORE_TYPE_BUFFER *)Buffer; Status =
> + Params->CpuMpPpi2->WhoAmI (Params->CpuMpPpi2, &ProcessorIndex);
> + ASSERT_EFI_ERROR (Status);
> +
> + CoreTypes = Params->CoreTypes;
> + AsmCpuidEx (CPUID_HYBRID_INFORMATION,
> +CPUID_HYBRID_INFORMATION_MAIN_LEAF,
> +&NativeModelIdAndCoreTypeEax.Uint32, NULL, NULL, NULL);
> + CoreTypes[ProcessorIndex] =
> +(UINT8)NativeModelIdAndCoreTypeEax.Bits.CoreType;
> +}
> +
> +/**
> + Create gMpInformation2HobGuid.
> +**/
> +VOID
> +BuildMpInformationHob (
> + IN EDKII_PEI_MP_SERVICES2_PPI *CpuMpPpi2
> + )
> +{
> + GET_PROCESSOR_CORE_TYPE_BUFFER Buffer;
> + EFI_STATUS Status;
> + UINTN ProcessorIndex;
> + UINTN NumberOfProcessors;
> + UINTN NumberOfEnabledProcessors;
> + UINTN NumberOfProcessorsInHob;
> + UINTN MaxProcessorsPerHob;
> + MP_INFORMATION2_HOB_DATA *MpInformation2HobData;
> + MP_INFORMATION2_ENTRY *MpInformation2Entry;
> + UINTN Index;
> + UINT8 *CoreTypes;
> + UINT32 CpuidMaxInput;
> + UINTN CoreTypePages;
> +
> + ProcessorIndex = 0;
> + MpInformation2HobData = NULL;
> + MpInformation2Entry = NULL;
> + CoreTypes = NULL;
> + CoreTypePages = 0;
> +
> + Status = CpuMpPpi2->GetNumberOfProcessors (
> + CpuMpPpi2,
> + &NumberOfProcessors,
> + &NumberOfEnabledProcessors
> + );
> + ASSERT_EFI_ERROR (Status);
> + if (EFI_ERROR (Status)) {
> + goto Done;
> + }
> +
> + //
> + // Get Processors CoreType
> + //
> + AsmCpuid (CPUID_SIGNATURE, &CpuidMaxInput, NULL, NULL, NULL); if
> + (CpuidMaxInput >= CPUID_HYBRID_INFORMATION) {
> + CoreTypePages = EFI_SIZE_TO_PAGES (sizeof (UINT8) *
> NumberOfProcessors);
> + CoreTypes = AllocatePages (CoreTypePages);
> + ASSERT (CoreTypes != NULL);
> + if (CoreTypes == NULL) {
> + goto Done;
> + }
> +
> + Buffer.CoreTypes = CoreTypes;
> + Buffer.CpuMpPpi2 = CpuMpPpi2;
> + Status = CpuMpPpi2->StartupAllCPUs (
> + CpuMpPpi2,
> + GetProcessorCoreType,
> + 0,
> + (VOID *)&Buffer
> + );
> + ASSERT_EFI_ERROR (Status);
> + }
> +
> + MaxProcessorsPerHob = ((MAX_UINT16 & ~7) - sizeof
> (EFI_HOB_GUID_TYPE) - sizeof (MP_INFORMATION2_HOB_DATA)) / sizeof
> (MP_INFORMATION2_ENTRY);
> + NumberOfProcessorsInHob = MaxProcessorsPerHob;
> +
> + //
> + // Create MP_INFORMATION2_HOB. when the max HobLength 0xFFF8 is
> not
> + enough, there // will be a MP_INFORMATION2_HOB series in the HOB list.
> + // In the HOB list, there is a gMpInformation2HobGuid with 0 value
> + NumberOfProcessors // fields to indicate it's the last
> MP_INFORMATION2_HOB.
> + //
> + while (NumberOfProcessorsInHob != 0) {
> + NumberOfProcessorsInHob = MIN (NumberOfProcessors -
> ProcessorIndex, MaxProcessorsPerHob);
> + MpInformation2HobData = BuildGuidHob (
> + &gMpInformation2HobGuid,
> + sizeof (MP_INFORMATION2_HOB_DATA) + sizeof
> (MP_INFORMATION2_ENTRY) * NumberOfProcessorsInHob
> + );
> + ASSERT (MpInformation2HobData != NULL);
> + if (MpInformation2HobData == NULL) {
> + goto Done;
> + }
> +
> + MpInformation2HobData->Version =
> MP_INFORMATION2_HOB_REVISION;
> + MpInformation2HobData->ProcessorIndex = ProcessorIndex;
> + MpInformation2HobData->NumberOfProcessors =
> (UINT16)NumberOfProcessorsInHob;
> + MpInformation2HobData->EntrySize = sizeof
> (MP_INFORMATION2_ENTRY);
> +
> + DEBUG ((DEBUG_INFO, "Creating MpInformation2 HOB...\n"));
> +
> + for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
> + MpInformation2Entry = &MpInformation2HobData->Entry[Index];
> + Status = CpuMpPpi2->GetProcessorInfo (
> + CpuMpPpi2,
> + (Index + ProcessorIndex) |
> CPU_V2_EXTENDED_TOPOLOGY,
> + &MpInformation2Entry->ProcessorInfo
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + MpInformation2Entry->CoreType = (CoreTypes != NULL) ?
> + CoreTypes[Index + ProcessorIndex] : 0;
> +
> + DEBUG ((
> + DEBUG_INFO,
> + " Processor[%04d]: ProcessorId = 0x%lx, StatusFlag = 0x%x, CoreType
> =
> 0x%x\n",
> + Index + ProcessorIndex,
> + MpInformation2Entry->ProcessorInfo.ProcessorId,
> + MpInformation2Entry->ProcessorInfo.StatusFlag,
> + MpInformation2Entry->CoreType
> + ));
> + DEBUG ((
> + DEBUG_INFO,
> + " Location = Package:%d Core:%d Thread:%d\n",
> + MpInformation2Entry->ProcessorInfo.Location.Package,
> + MpInformation2Entry->ProcessorInfo.Location.Core,
> + MpInformation2Entry->ProcessorInfo.Location.Thread
> + ));
> + DEBUG ((
> + DEBUG_INFO,
> + " Location2 = Package:%d Die:%d Tile:%d Module:%d Core:%d
> Thread:%d\n",
> + MpInformation2Entry-
> >ProcessorInfo.ExtendedInformation.Location2.Package,
> + MpInformation2Entry-
> >ProcessorInfo.ExtendedInformation.Location2.Die,
> + MpInformation2Entry-
> >ProcessorInfo.ExtendedInformation.Location2.Tile,
> + MpInformation2Entry-
> >ProcessorInfo.ExtendedInformation.Location2.Module,
> + MpInformation2Entry-
> >ProcessorInfo.ExtendedInformation.Location2.Core,
> + MpInformation2Entry-
> >ProcessorInfo.ExtendedInformation.Location2.Thread
> + ));
> + }
> +
> + ProcessorIndex += NumberOfProcessorsInHob; }
> +
> +Done:
> + if (CoreTypes != NULL) {
> + FreePages (CoreTypes, CoreTypePages);
> + }
> +}
> +
> +/**
> + Check if CpuMpPei creates gMpInformation2HobGuid and if it does not
> +it
> + creates it.
> +
> + @param[in] ImageHandle Handle for the image of this driver
> + @param[in] SystemTable Pointer to the EFI System Table
> +
> + @retval EFI_UNSUPPORTED
> +**/
> +EFI_STATUS
> +EFIAPI
> +MpInfo2HobPeiEntryPoint (
> + IN EFI_PEI_FILE_HANDLE FileHandle,
> + IN CONST EFI_PEI_SERVICES **PeiServices
> + )
> +{
> + EFI_STATUS Status;
> + EFI_PEI_PPI_DESCRIPTOR *TempPpiDescriptor;
> + EDKII_PEI_MP_SERVICES2_PPI *CpuMpPpi2;
> + EFI_HOB_GUID_TYPE *GuidHob;
> +
> + Status = PeiServicesLocatePpi (
> + &gEdkiiPeiMpServices2PpiGuid,
> + 0,
> + &TempPpiDescriptor,
> + (VOID **)&CpuMpPpi2
> + );
> + ASSERT_EFI_ERROR (Status);
> + if (EFI_ERROR (Status)) {
> + goto Done;
> + }
> +
> + GuidHob = GetFirstGuidHob (&gMpInformation2HobGuid); if (GuidHob ==
> + NULL) {
> + DEBUG ((DEBUG_INFO, "gMpInformation2HobGuid was not created by
> CpuMpPei, creating now\n"));
> + BuildMpInformationHob (CpuMpPpi2);
> + }
> +
> +Done:
> + return Status;
> +}
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2Hob
> Pei.inf
> b/Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2Hob
> Pei.inf
> new file mode 100644
> index 0000000000..eecfdbf422
> --- /dev/null
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2Hob
> P
> +++ ei.inf
> @@ -0,0 +1,47 @@
> +### @file
> +# Component information file for the Multi-processor Info 2 HOB PEIM.
> +#
> +# The purpose of this PEIM is to provide backwards compatibility
> +between FSP # binaries built with older versions of EDK II and the latest EDK
> II.
> +#
> +# Newer versions of CpuMpPei produce the gMpInformation2HobGuid. This
> +HOB is # required by newer implementations of the CPU DXE driver,
> +however older # versions of CpuMpPei do not produce it. This PEIM will
> +check if CpuMpPei # creates gMpInformation2HobGuid and if it does not it
> creates it.
> +#
> +# Copyright (c) 2024, Intel Corporation. All rights reserved.<BR> # #
> +SPDX-License-Identifier: BSD-2-Clause-Patent # ###
> +
> +[Defines]
> + INF_VERSION = 0x00010017
> + BASE_NAME = MpInfo2HobPei
> + FILE_GUID = 010B5607-D5B3-4302-BCBC-C1A68087E9BE
> + VERSION_STRING = 1.0
> + MODULE_TYPE = PEIM
> + ENTRY_POINT = MpInfo2HobPeiEntryPoint
> +
> +[LibraryClasses]
> + PeimEntryPoint
> + DebugLib
> + MemoryAllocationLib
> + HobLib
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + UefiCpuPkg/UefiCpuPkg.dec
> + MinPlatformPkg/MinPlatformPkg.dec
> +
> +[Sources]
> + MpInfo2HobPei.c
> +
> +[Guids]
> + gMpInformation2HobGuid ## SOMETIMES_PRODUCES ##
> HOB
> +
> +[Ppis]
> + gEdkiiPeiMpServices2PpiGuid ## CONSUMES
> +
> +[Depex]
> + gEdkiiPeiMpServices2PpiGuid
> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> index ecb4d8f65e..30cdf1fb82 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> @@ -1,7 +1,7 @@
> ## @file
> # Platform description.
> #
> -# Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2024, Intel Corporation. All rights
> +reserved.<BR>
> # Copyright (c) Microsoft Corporation.<BR> # # SPDX-License-Identifier:
> BSD-
> 2-Clause-Patent @@ -150,6 +150,7 @@
>
> MinPlatformPkg/Bds/Library/DxePlatformBootManagerLib/DxePlatformBoot
> ManagerLib.inf
>
> MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
> + MinPlatformPkg/FspWrapper/MpInfo2HobPei/MpInfo2HobPei.inf
>
> MinPlatformPkg/FspWrapper/Library/PeiFspWrapperHobProcessLib/PeiFspW
> rapperHobProcessLib.inf
>
> MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecFsp
> WrapperPlatformSecLib.inf
>
> MinPlatformPkg/FspWrapper/Library/PeiFspWrapperPlatformLib/PeiFspWrap
> perPlatformLib.inf
> --
> 2.44.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118874): https://edk2.groups.io/g/devel/message/118874
Mute This Topic: https://groups.io/mt/105992897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
