[edk2-devel] [PATCH 0/6] Create and consume a new gMpInformationHobGuid2 in UefiCpuPkg.

[edk2-devel] [PATCH 0/6] Create and consume a new gMpInformationHobGuid2 in UefiCpuPkg.

[duntan]

Create and consume gMpInformationHobGuid2 in UefiCpuPkg in this patch series.

Currently, there is a gMpInformationHobGuid defined, created and consumed only in StandaloneMmPkg.
The HOB contains the EFI_PROCESSOR_INFORMATION structure for each CPU and the number of processors.
This is the same as the information that PiSmmCpuDxeSmm uses MpService Protocol to get.

This new gMpInformationHobGuid2 also contains the NumberOfProcessors and the EFI_PROCESSOR_INFORMATION for each CPU. 
lso the HOB is extended to support the case that the maximum HOB length is not enough for all CPU.
So there might be more than one HOB instance in the HOB list. Each HOB describes the corresponding CPU index range.

The plan is to create gMpInformationHob2Guid in CpuMpPei module. Then PiSmmCpuDxeSmm and other MM_STANDALONE modules can consume the hob.
This can avoid calling MpService Protocol in PiSmmCpuDxeSmm. Also the issue that one gMpInformationHobGuid might be not enough when CPU number is 1~2000 or bigger can be solved.

Also the code to extracting information from gSmmBaseHobGuid is modified to remove the assumption that there is only one HOB instance of gSmmBaseHobGuid.

Dun Tan (6):
  UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg
  UefiCpuPkg: Build MpInfo2HOB in CpuMpPei
  UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
  UefiCpuPkg: Add a new field in MpInfo2 HOB
  UefiCpuPkg: Cache core type in MpInfo2 HOB
  UefiCpuPkg: Avoid assuming only one smmbasehob

 UefiCpuPkg/CpuMpPei/CpuMpPei.c               | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/CpuMpPei/CpuMpPei.h               |   6 +++++-
 UefiCpuPkg/CpuMpPei/CpuMpPei.inf             |   3 ++-
 UefiCpuPkg/Include/Guid/MpInformation2.h     |  58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |   2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   8 ++++----
 UefiCpuPkg/UefiCpuPkg.dec                    |   3 +++
 8 files changed, 515 insertions(+), 62 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Guid/MpInformation2.h

-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112062): https://edk2.groups.io/g/devel/message/112062
Mute This Topic: https://groups.io/mt/102987136/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

[duntan]

Create gMpInformationHobGuid2 in UefiCpuPkg.

Currently, there is a gMpInformationHobGuid defined,
created and consumed only in StandaloneMmPkg. The HOB
contains the EFI_PROCESSOR_INFORMATION structure for
each CPU and the number of processors. This is the same
as the information that PiSmmCpuDxeSmm uses MpService
Protocol to get.

This new gMpInformationHobGuid2 also contains the
NumberOfProcessors and the EFI_PROCESSOR_INFORMATION
for each CPU. Also the HOB is extended to support the
case that the maximum HOB length is not enough for all
CPU. So there might be more than one HOB instance in the
HOB list. Each HOB describes the corresponding CPU index
range.

The plan is to create gMpInformationHob2Guid in CpuMpPei
module(implemented in next commit). Then PiSmmCpuDxeSmm
and other MM_STANDALONE modules can consume the hob. This
can avoid calling MpService Protocol in PiSmmCpuDxeSmm.
Also the issue that one gMpInformationHobGuid might be not
enough when CPU number is 1~2000 or bigger can be solved.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Include/Guid/MpInformation2.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.dec                |  3 +++
 2 files changed, 59 insertions(+)

diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h b/UefiCpuPkg/Include/Guid/MpInformation2.h
new file mode 100644
index 0000000000..4164ce1c30
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
@@ -0,0 +1,56 @@
+/** @file
+  EFI MP information protocol provides a lightweight MP_SERVICES_PROTOCOL.
+
+  MP information protocol only provides static information of MP processor.
+
+  If SwitchBSP or Enable/DisableAP in MP service is called between the HOB
+  production and HOB consumption, EFI_PROCESSOR_INFORMATION.StatusFlag and
+  NumberOfEnabledProcessors fields in this HOB may be invalidated.
+
+  Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef MP_INFORMATION2_H_
+#define MP_INFORMATION2_H_
+
+#include <Protocol/MpService.h>
+#include <PiPei.h>
+#include <Ppi/SecPlatformInformation.h>
+
+#define MP_INFORMATION2_HOB_REVISION  1
+
+#define MP_INFORMATION2_GUID \
+  { \
+    0x417a7f64, 0xf4e9, 0x4b32, {0x84, 0x6a, 0x5c, 0xc4, 0xd8, 0x62, 0x18, 0x79}  \
+  }
+
+typedef struct {
+  EFI_PROCESSOR_INFORMATION    ProcessorInfo;
+  //
+  // Add more fields in future
+  //
+} MP_INFORMATION2_ENTRY;
+
+typedef struct {
+  UINT16                   NumberOfProcessors;
+  UINT16                   EntrySize;
+  UINT8                    Version;
+  UINT8                    Reserved[3];
+  UINT64                   ProcessorIndex;
+  MP_INFORMATION2_ENTRY    MpInformation[0];
+} MP_INFORMATION2_HOB_DATA;
+
+//
+// Producer of MP_INFORMATION2_HOB_DATA should assign sizeof (MP_INFORMATION2_ENTRY) to MP_INFORMATION2_HOB_DATA.EntrySize
+// Consumer of MP_INFORMATION2_HOB_DATA should use below macro or similar logic to get the individual entry
+// as the entry structure might be updated to include more information.
+//
+#define GET_MP_INFORMATION_ENTRY(MpInfoHobData, Index) \
+    (MP_INFORMATION2_ENTRY *)((UINTN)&((MP_INFORMATION2_HOB_DATA *)(MpInfoHobData))->MpInformation + (MpInfoHobData)->EntrySize * Index)
+
+extern EFI_GUID  gMpInformationHobGuid2;
+
+#endif
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 0b5431dbf7..61bd34ef17 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -85,6 +85,9 @@
   ## Include/Guid/SmmBaseHob.h
   gSmmBaseHobGuid      = { 0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73 }}
 
+  ## Include/Guid/MpInformation2.h
+  gMpInformationHobGuid2         = { 0x417a7f64, 0xf4e9, 0x4b32, {0x84, 0x6a, 0x5c, 0xc4, 0xd8, 0x62, 0x18, 0x79 }}
+
 [Protocols]
   ## Include/Protocol/SmmCpuService.h
   gEfiSmmCpuServiceProtocolGuid   = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94, 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112063): https://edk2.groups.io/g/devel/message/112063
Mute This Topic: https://groups.io/mt/102987137/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

[Ni, Ray]

> +  MP information protocol only provides static information of MP processor.
> +
> +  If SwitchBSP or Enable/DisableAP in MP service is called between the HOB
> +  production and HOB consumption,
> EFI_PROCESSOR_INFORMATION.StatusFlag and
> +  NumberOfEnabledProcessors fields in this HOB may be invalidated.

1. There is no NumberOfEnabledProcessors field.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112101): https://edk2.groups.io/g/devel/message/112101
Mute This Topic: https://groups.io/mt/102987137/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

[duntan]

Thanks for the comments.
Will remove this field.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, December 6, 2023 5:09 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [PATCH 1/6] UefiCpuPkg: Create gMpInformationHobGuid2 in UefiCpuPkg

> +  MP information protocol only provides static information of MP processor.
> +
> +  If SwitchBSP or Enable/DisableAP in MP service is called between 
> + the HOB  production and HOB consumption,
> EFI_PROCESSOR_INFORMATION.StatusFlag and
> +  NumberOfEnabledProcessors fields in this HOB may be invalidated.

1. There is no NumberOfEnabledProcessors field.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112143): https://edk2.groups.io/g/devel/message/112143
Mute This Topic: https://groups.io/mt/102987137/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

[duntan]

Build MpInfo2HOB in CpuMpPei module so that later
PiSmmCpuDxe or other StandaloneMm module can consume
the HOB.
Since there might be more one gMpInformationHobGuid2
in HOB list, CpuMpPei create a gMpInformationHobGuid2
with 0 value NumberOfProcessors field in the end of the
process to indicate it's the last MP_INFORMATION2_HOB.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/CpuMpPei/CpuMpPei.h   |  4 +++-
 UefiCpuPkg/CpuMpPei/CpuMpPei.inf |  3 ++-
 3 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index b504bea3cf..4d80d52799 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -541,6 +541,92 @@ InitializeMpExceptionStackSwitchHandlers (
   FreePages (SwitchStackData, EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
 }
 
+/**
+  Create gMpInformationHobGuid2.
+**/
+VOID
+BuildMpInformationHob (
+  VOID
+  )
+{
+  EFI_STATUS                Status;
+  UINTN                     ProcessorIndex;
+  UINTN                     NumberOfProcessors;
+  UINTN                     NumberOfEnabledProcessors;
+  UINTN                     NumberOfProcessorsInHob;
+  UINTN                     MaxProcessorsPerHob;
+  MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
+  MP_INFORMATION2_ENTRY     *MpInformation2Entry;
+  UINTN                     Index;
+
+  ProcessorIndex        = 0;
+  MpInformation2HobData = NULL;
+  MpInformation2Entry   = NULL;
+
+  Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
+  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 gMpInformationHobGuid2 with 0 value NumberOfProcessors
+  // fields to indicate it's the last MP_INFORMATION2_HOB.
+  //
+  while (NumberOfProcessorsInHob != 0) {
+    NumberOfProcessorsInHob = MIN (NumberOfProcessors - ProcessorIndex, MaxProcessorsPerHob);
+    MpInformation2HobData   = BuildGuidHob (
+                                &gMpInformationHobGuid2,
+                                sizeof (MP_INFORMATION2_HOB_DATA) + sizeof (MP_INFORMATION2_ENTRY) * NumberOfProcessorsInHob
+                                );
+    ASSERT (MpInformation2HobData != NULL);
+
+    MpInformation2HobData->Version            = MP_INFORMATION2_HOB_REVISION;
+    MpInformation2HobData->ProcessorIndex     = ProcessorIndex;
+    MpInformation2HobData->NumberOfProcessors = (UINT16)NumberOfProcessorsInHob;
+    MpInformation2HobData->EntrySize          = sizeof (MP_INFORMATION2_ENTRY);
+
+    DEBUG ((DEBUG_INFO, "BuildMpInformationHob\n"));
+
+    for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
+      MpInformation2Entry = GET_MP_INFORMATION_ENTRY (MpInformation2HobData, Index);
+      Status              = MpInitLibGetProcessorInfo (
+                              (Index + ProcessorIndex) | CPU_V2_EXTENDED_TOPOLOGY,
+                              &MpInformation2Entry->ProcessorInfo,
+                              NULL
+                              );
+      ASSERT_EFI_ERROR (Status);
+      DEBUG ((
+        DEBUG_INFO,
+        "ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x\n",
+        Index + ProcessorIndex,
+        MpInformation2Entry->ProcessorInfo.ProcessorId,
+        MpInformation2Entry->ProcessorInfo.StatusFlag
+        ));
+      DEBUG ((
+        DEBUG_INFO,
+        "Location (Package/Core/Thread) = %d/%d/%d\n",
+        MpInformation2Entry->ProcessorInfo.Location.Package,
+        MpInformation2Entry->ProcessorInfo.Location.Core,
+        MpInformation2Entry->ProcessorInfo.Location.Thread
+        ));
+      DEBUG ((
+        DEBUG_INFO,
+        "Location2 (Package/Die/Tile/Module/Core/Thread) = %d/%d/%d/%d/%d/%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;
+  }
+}
+
 /**
   Initializes MP and exceptions handlers.
 
@@ -602,6 +688,11 @@ InitializeCpuMpWorker (
   Status = PeiServicesInstallPpi (mPeiCpuMpPpiList);
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // Create gMpInformationHobGuid2
+  //
+  BuildMpInformationHob ();
+
   return Status;
 }
 
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
index 1b9a94e18f..a40fd2c077 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
@@ -1,7 +1,7 @@
 /** @file
   Definitions to install Multiple Processor PPI.
 
-  Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2015 - 2023, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -30,6 +30,8 @@
 #include <Library/MemoryAllocationLib.h>
 #include <Library/CpuPageTableLib.h>
 
+#include <Guid/MpInformation2.h>
+
 extern EFI_PEI_PPI_DESCRIPTOR  mPeiCpuMpPpiDesc;
 
 /**
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
index 865be5627e..812fa179bd 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
@@ -1,7 +1,7 @@
 ## @file
 #  CPU driver installs CPU PI Multi-processor PPI.
 #
-#  Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2015 - 2023, Intel Corporation. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -50,6 +50,7 @@
 
 [Guids]
   gEdkiiMigratedFvInfoGuid                                             ## SOMETIMES_CONSUMES     ## HOB
+  gMpInformationHobGuid2                        ## PRODUCES
 
 [Ppis]
   gEfiPeiMpServicesPpiGuid                      ## PRODUCES
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112064): https://edk2.groups.io/g/devel/message/112064
Mute This Topic: https://groups.io/mt/102987138/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

[Ni, Ray]

4 minor comments:

> +    DEBUG ((DEBUG_INFO, "BuildMpInformationHob\n"));

1. DEBUG ("Creating MpInformation2 HOB...\n")

> +
> +    for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
> +      MpInformation2Entry = GET_MP_INFORMATION_ENTRY
> (MpInformation2HobData, Index);

2. Since EntrySize equals to sizeof (MP_INFORMATION2_ENTRY), is it ok to just use MpInformation2HobData->Entry[Index]?

3. Do you think "Entry[0]" is more proper than "MpInformation[0]"?

> +      DEBUG ((
> +        DEBUG_INFO,
> +        "ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x\n",
> +        Index + ProcessorIndex,
> +        MpInformation2Entry->ProcessorInfo.ProcessorId,
> +        MpInformation2Entry->ProcessorInfo.StatusFlag
> +        ));

4. How about the debug messages are as follows:
<space><space>Processor[0000]: ProcessorId = 0x00000000, StatusFlag = 0x00000001\n
<space><space><space><space>Location = Package:0 Core:0 Thread:0\n
<space><space><space><space>Location2 = Package:0 Die:0 Tile:0 Module:0 Core:0 Thread:0\n
If a number has "0x" prefix, it uses hex format, otherwise it uses dec format. The debug message should
clearly tell what format the number follows.
Extra 2 spaces in Location/Location2 are to tell that these are extra info for the processor #0.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112102): https://edk2.groups.io/g/devel/message/112102
Mute This Topic: https://groups.io/mt/102987138/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

[duntan]

Will change the commit based on the comments.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, December 6, 2023 5:24 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [PATCH 2/6] UefiCpuPkg: Build MpInfo2HOB in CpuMpPei

4 minor comments:

> +    DEBUG ((DEBUG_INFO, "BuildMpInformationHob\n"));

1. DEBUG ("Creating MpInformation2 HOB...\n")

> +
> +    for (Index = 0; Index < NumberOfProcessorsInHob; Index++) {
> +      MpInformation2Entry = GET_MP_INFORMATION_ENTRY
> (MpInformation2HobData, Index);

2. Since EntrySize equals to sizeof (MP_INFORMATION2_ENTRY), is it ok to just use MpInformation2HobData->Entry[Index]?

3. Do you think "Entry[0]" is more proper than "MpInformation[0]"?

> +      DEBUG ((
> +        DEBUG_INFO,
> +        "ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x\n",
> +        Index + ProcessorIndex,
> +        MpInformation2Entry->ProcessorInfo.ProcessorId,
> +        MpInformation2Entry->ProcessorInfo.StatusFlag
> +        ));

4. How about the debug messages are as follows:
<space><space>Processor[0000]: ProcessorId = 0x00000000, StatusFlag = 0x00000001\n <space><space><space><space>Location = Package:0 Core:0 Thread:0\n
<space><space><space><space>Location2 = Package:0 Die:0 Tile:0 Module:0 Core:0 Thread:0\n If a number has "0x" prefix, it uses hex format, otherwise it uses dec format. The debug message should clearly tell what format the number follows.
Extra 2 spaces in Location/Location2 are to tell that these are extra info for the processor #0.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112144): https://edk2.groups.io/g/devel/message/112144
Mute This Topic: https://groups.io/mt/102987138/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[duntan]

Consume MpInfo2Hob in PiSmmCpuDxe driver to get
NumberOfProcessors, MaxNumberOfCpus and
EFI_PROCESSOR_INFORMATION for all CPU from the
MpInformation2 HOB.
This can avoid calling MP service.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |   2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |   8 ++++----
 3 files changed, 171 insertions(+), 48 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 1d022a7051..d8d488b253 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -586,6 +586,152 @@ SmmReadyToLockEventNotify (
   return EFI_SUCCESS;
 }
 
+/**
+  Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on ProcessorIndex.
+
+  @param[in] Buffer1            pointer to MP_INFORMATION2_HOB_DATA poiner to compare
+  @param[in] Buffer2            pointer to second MP_INFORMATION2_HOB_DATA pointer to compare
+
+  @retval 0                     Buffer1 equal to Buffer2
+  @retval <0                    Buffer1 is less than Buffer2
+  @retval >0                    Buffer1 is greater than Buffer2
+**/
+INTN
+EFIAPI
+MpInformation2HobCompare (
+  IN  CONST VOID  *Buffer1,
+  IN  CONST VOID  *Buffer2
+  )
+{
+  if ((*(MP_INFORMATION2_HOB_DATA **)Buffer1)->ProcessorIndex > (*(MP_INFORMATION2_HOB_DATA **)Buffer2)->ProcessorIndex) {
+    return 1;
+  } else if ((*(MP_INFORMATION2_HOB_DATA **)Buffer1)->ProcessorIndex < (*(MP_INFORMATION2_HOB_DATA **)Buffer2)->ProcessorIndex) {
+    return -1;
+  }
+
+  return 0;
+}
+
+/**
+  Extract NumberOfCpus, MaxNumberOfCpus and EFI_PROCESSOR_INFORMATION for all CPU from MpInformation2 HOB.
+
+  @param[out] NumberOfCpus           Pointer to NumberOfCpus.
+  @param[out] MaxNumberOfCpus        Pointer to MaxNumberOfCpus.
+  @param[out] ProcessorInfoPointer   Pointer to EFI_PROCESSOR_INFORMATION buffer pointer.
+
+  @retval EFI_SUCCESS                Successfully extract information from MpInformation2 HOB.
+  @retval RETURN_BUFFER_TOO_SMALL    Fail to allocate memory.
+**/
+EFI_STATUS
+GetMpInfoFromMpInfo2Hob (
+  OUT UINTN                      *NumberOfCpus,
+  OUT UINTN                      *MaxNumberOfCpus,
+  OUT EFI_PROCESSOR_INFORMATION  **ProcessorInfoPointer
+  )
+{
+  EFI_HOB_GUID_TYPE          *GuidHob;
+  EFI_HOB_GUID_TYPE          *FirstMpInfor2Hob;
+  MP_INFORMATION2_HOB_DATA   *MpInformation2HobData;
+  UINTN                      CpuCount;
+  UINTN                      HobCount;
+  UINTN                      Index;
+  MP_INFORMATION2_HOB_DATA   **MpInfomation2Buffer;
+  UINTN                      SortBuffer;
+  UINTN                      ProcessorIndex;
+  UINT64                     PrevProcessorIndex;
+  MP_INFORMATION2_ENTRY      *MpInformation2Entry;
+  EFI_PROCESSOR_INFORMATION  *ProcessorInfo;
+
+  GuidHob               = NULL;
+  MpInformation2HobData = NULL;
+  FirstMpInfor2Hob      = NULL;
+  MpInfomation2Buffer   = NULL;
+  CpuCount              = 0;
+  Index                 = 0;
+  HobCount              = 0;
+  PrevProcessorIndex    = 0;
+
+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);
+  ASSERT (FirstMpInfor2Hob != NULL);
+  GuidHob = FirstMpInfor2Hob;
+  while (GuidHob != NULL) {
+    MpInformation2HobData = GET_GUID_HOB_DATA (GuidHob);
+
+    //
+    // This is the last MpInformationHob in the HOB list.
+    //
+    if (MpInformation2HobData->NumberOfProcessors == 0) {
+      ASSERT (HobCount != 0);
+      break;
+    }
+
+    HobCount++;
+    CpuCount += MpInformation2HobData->NumberOfProcessors;
+    GuidHob   = GetNextGuidHob (&gMpInformationHobGuid2, GET_NEXT_HOB (GuidHob));
+  }
+
+  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
+  *NumberOfCpus = CpuCount;
+
+  //
+  // If support CPU hot plug, we need to allocate resources for possibly hot-added processors
+  //
+  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    *MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+  } else {
+    *MaxNumberOfCpus = CpuCount;
+  }
+
+  MpInfomation2Buffer = AllocatePool (sizeof (MP_INFORMATION2_HOB_DATA *) * HobCount);
+  ASSERT (MpInfomation2Buffer != NULL);
+  if (MpInfomation2Buffer == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  //
+  // Record each MpInformation2Hob pointer in the MpInfomation2Buffer.
+  //
+  GuidHob = FirstMpInfor2Hob;
+  ASSERT (GuidHob != NULL);
+  while (Index < HobCount) {
+    MpInfomation2Buffer[Index++] = GET_GUID_HOB_DATA (GuidHob);
+    GuidHob                      = GetNextGuidHob (&gMpInformationHobGuid2, GET_NEXT_HOB (GuidHob));
+  }
+
+  ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EFI_PROCESSOR_INFORMATION) * (*MaxNumberOfCpus));
+  ASSERT (ProcessorInfo != NULL);
+  if (ProcessorInfo == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  QuickSort (MpInfomation2Buffer, HobCount, sizeof (MP_INFORMATION2_HOB_DATA *), (BASE_SORT_COMPARE)MpInformation2HobCompare, &SortBuffer);
+  for (Index = 0; Index < HobCount; Index++) {
+    //
+    // Make sure no overlap and no gap in the CPU range covered by each HOB
+    //
+    ASSERT (MpInfomation2Buffer[Index]->ProcessorIndex == PrevProcessorIndex);
+
+    //
+    // Cache each EFI_PROCESSOR_INFORMATION in order.
+    //
+    for (ProcessorIndex = 0; ProcessorIndex < MpInfomation2Buffer[Index]->NumberOfProcessors; ProcessorIndex++) {
+      MpInformation2Entry = GET_MP_INFORMATION_ENTRY (MpInfomation2Buffer[Index], ProcessorIndex);
+      CopyMem (
+        ProcessorInfo + PrevProcessorIndex + ProcessorIndex,
+        &MpInformation2Entry->ProcessorInfo,
+        sizeof (EFI_PROCESSOR_INFORMATION)
+        );
+    }
+
+    PrevProcessorIndex += MpInfomation2Buffer[Index]->NumberOfProcessors;
+  }
+
+  *ProcessorInfoPointer = ProcessorInfo;
+
+  FreePool (MpInfomation2Buffer);
+  return EFI_SUCCESS;
+}
+
 /**
   The module Entry Point of the CPU SMM driver.
 
@@ -603,26 +749,24 @@ PiCpuSmmEntry (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  EFI_STATUS                Status;
-  EFI_MP_SERVICES_PROTOCOL  *MpServices;
-  UINTN                     NumberOfEnabledProcessors;
-  UINTN                     Index;
-  VOID                      *Buffer;
-  UINTN                     BufferPages;
-  UINTN                     TileCodeSize;
-  UINTN                     TileDataSize;
-  UINTN                     TileSize;
-  UINT8                     *Stacks;
-  VOID                      *Registration;
-  UINT32                    RegEax;
-  UINT32                    RegEbx;
-  UINT32                    RegEcx;
-  UINT32                    RegEdx;
-  UINTN                     FamilyId;
-  UINTN                     ModelId;
-  UINT32                    Cr3;
-  EFI_HOB_GUID_TYPE         *GuidHob;
-  SMM_BASE_HOB_DATA         *SmmBaseHobData;
+  EFI_STATUS         Status;
+  UINTN              Index;
+  VOID               *Buffer;
+  UINTN              BufferPages;
+  UINTN              TileCodeSize;
+  UINTN              TileDataSize;
+  UINTN              TileSize;
+  UINT8              *Stacks;
+  VOID               *Registration;
+  UINT32             RegEax;
+  UINT32             RegEbx;
+  UINT32             RegEcx;
+  UINT32             RegEdx;
+  UINTN              FamilyId;
+  UINTN              ModelId;
+  UINT32             Cr3;
+  EFI_HOB_GUID_TYPE  *GuidHob;
+  SMM_BASE_HOB_DATA  *SmmBaseHobData;
 
   GuidHob        = NULL;
   SmmBaseHobData = NULL;
@@ -654,17 +798,10 @@ PiCpuSmmEntry (
   FindSmramInfo (&mCpuHotPlugData.SmrrBase, &mCpuHotPlugData.SmrrSize);
 
   //
-  // Get MP Services Protocol
-  //
-  Status = SystemTable->BootServices->LocateProtocol (&gEfiMpServiceProtocolGuid, NULL, (VOID **)&MpServices);
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Use MP Services Protocol to retrieve the number of processors and number of enabled processors
+  // Retrive NumberOfProcessors, MaxNumberOfCpus and EFI_PROCESSOR_INFORMATION for all CPU from MpInformation2 HOB.
   //
-  Status = MpServices->GetNumberOfProcessors (MpServices, &mNumberOfCpus, &NumberOfEnabledProcessors);
+  Status = GetMpInfoFromMpInfo2Hob (&mNumberOfCpus, &mMaxNumberOfCpus, &gSmmCpuPrivate->ProcessorInfo);
   ASSERT_EFI_ERROR (Status);
-  ASSERT (mNumberOfCpus <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
 
   //
   // If support CPU hot plug, PcdCpuSmmEnableBspElection should be set to TRUE.
@@ -690,15 +827,6 @@ PiCpuSmmEntry (
   mAddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & PAGING_1G_ADDRESS_MASK_64;
   DEBUG ((DEBUG_INFO, "mAddressEncMask = 0x%lx\n", mAddressEncMask));
 
-  //
-  // If support CPU hot plug, we need to allocate resources for possibly hot-added processors
-  //
-  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
-    mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
-  } else {
-    mMaxNumberOfCpus = mNumberOfCpus;
-  }
-
   gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus = mMaxNumberOfCpus;
 
   PERF_CODE (
@@ -908,9 +1036,6 @@ PiCpuSmmEntry (
   //
   // Allocate buffer for pointers to array in  SMM_CPU_PRIVATE_DATA.
   //
-  gSmmCpuPrivate->ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EFI_PROCESSOR_INFORMATION) * mMaxNumberOfCpus);
-  ASSERT (gSmmCpuPrivate->ProcessorInfo != NULL);
-
   gSmmCpuPrivate->Operation = (SMM_CPU_OPERATION *)AllocatePool (sizeof (SMM_CPU_OPERATION) * mMaxNumberOfCpus);
   ASSERT (gSmmCpuPrivate->Operation != NULL);
 
@@ -945,8 +1070,6 @@ PiCpuSmmEntry (
     gSmmCpuPrivate->Operation[Index]        = SmmCpuNone;
 
     if (Index < mNumberOfCpus) {
-      Status = MpServices->GetProcessorInfo (MpServices, Index | CPU_V2_EXTENDED_TOPOLOGY, &gSmmCpuPrivate->ProcessorInfo[Index]);
-      ASSERT_EFI_ERROR (Status);
       mCpuHotPlugData.ApicId[Index] = gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId;
 
       DEBUG ((
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 20ada465c2..f18345881b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -14,7 +14,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <PiSmm.h>
 
-#include <Protocol/MpService.h>
 #include <Protocol/SmmConfiguration.h>
 #include <Protocol/SmmCpu.h>
 #include <Protocol/SmmAccess2.h>
@@ -27,6 +26,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/MemoryAttributesTable.h>
 #include <Guid/PiSmmMemoryAttributesTable.h>
 #include <Guid/SmmBaseHob.h>
+#include <Guid/MpInformation2.h>
 
 #include <Library/BaseLib.h>
 #include <Library/IoLib.h>
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 5d52ed7d13..b8aa2442cd 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -106,7 +106,6 @@
 
 [Protocols]
   gEfiSmmAccess2ProtocolGuid               ## CONSUMES
-  gEfiMpServiceProtocolGuid                ## CONSUMES
   gEfiSmmConfigurationProtocolGuid         ## PRODUCES
   gEfiSmmCpuProtocolGuid                   ## PRODUCES
   gEfiSmmReadyToLockProtocolGuid           ## NOTIFY
@@ -120,6 +119,7 @@
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
   gEfiMemoryAttributesTableGuid            ## CONSUMES ## SystemTable
   gSmmBaseHobGuid                          ## CONSUMES
+  gMpInformationHobGuid2                   ## CONSUMES
 
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
@@ -153,11 +153,11 @@
 [FixedPcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmMpTokenCountPerChunk               ## CONSUMES
 
+[Depex]
+  TRUE
+
 [Pcd.X64]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess        ## CONSUMES
 
-[Depex]
-  gEfiMpServiceProtocolGuid
-
 [UserExtensions.TianoCore."ExtraFiles"]
   PiSmmCpuDxeSmmExtra.uni
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112065): https://edk2.groups.io/g/devel/message/112065
Mute This Topic: https://groups.io/mt/102987139/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[Ni, Ray]

1. The function name can be "GetMpInformation()" without mentioning "FromMpInfo2Hob".

> +  EFI_HOB_GUID_TYPE          *GuidHob;
> +  EFI_HOB_GUID_TYPE          *FirstMpInfor2Hob;

2. "FirstMpInfo2Hob". Please remove "r".

>+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);

3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the 2nd while-loop without needing to look for MpInfo2Hob from beginning.

> +
> +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +  *NumberOfCpus = CpuCount;

4. There is no "return" before "*NumberOfCpus" assignment. So, why not remove "CpuCount" and directly udpates
"*NumberOfCpus" in the while-loop?

> +
> +  MpInfomation2Buffer = AllocatePool (sizeof
> (MP_INFORMATION2_HOB_DATA *) * HobCount);

5. MpInfomation2Buffer -> MpInfo2Hobs


6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?

> +  for (Index = 0; Index < HobCount; Index++) {
7. Index -> HobIndex

> +      CopyMem (
> +        ProcessorInfo + PrevProcessorIndex + ProcessorIndex,

8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]

> +
> +  *ProcessorInfoPointer = ProcessorInfo;

9. If you let the function just return ProcessorInfo and NULL when failure happens, will it simplify the code?

> 
> +[Depex]
> +  TRUE
> -[Depex]
> -  gEfiMpServiceProtocolGuid

10. The depex change means that CpuSmm driver could run before CpuMp driver runs. Have you verified if CpuSmm can start well even removing CpuMp DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112106): https://edk2.groups.io/g/devel/message/112106
Mute This Topic: https://groups.io/mt/102987139/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[duntan]

Will change the code based on comments 1-9.

About comments 10, "10. The depex change means that CpuSmm driver could run before CpuMp driver runs. Have you verified if CpuSmm can start well even removing CpuMp DXE driver?"

Yes, I verified it in OvmfIa32X64 boot. CpuSmm can start well even removing CpuMp DXE driver.(also removed some checking for gEfiCpuArchProtocolGuid)

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, December 6, 2023 5:55 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

1. The function name can be "GetMpInformation()" without mentioning "FromMpInfo2Hob".

> +  EFI_HOB_GUID_TYPE          *GuidHob;
> +  EFI_HOB_GUID_TYPE          *FirstMpInfor2Hob;

2. "FirstMpInfo2Hob". Please remove "r".

>+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);

3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the 2nd while-loop without needing to look for MpInfo2Hob from beginning.

> +
> +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +  *NumberOfCpus = CpuCount;

4. There is no "return" before "*NumberOfCpus" assignment. So, why not remove "CpuCount" and directly udpates "*NumberOfCpus" in the while-loop?

> +
> +  MpInfomation2Buffer = AllocatePool (sizeof
> (MP_INFORMATION2_HOB_DATA *) * HobCount);

5. MpInfomation2Buffer -> MpInfo2Hobs


6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?

> +  for (Index = 0; Index < HobCount; Index++) {
7. Index -> HobIndex

> +      CopyMem (
> +        ProcessorInfo + PrevProcessorIndex + ProcessorIndex,

8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]

> +
> +  *ProcessorInfoPointer = ProcessorInfo;

9. If you let the function just return ProcessorInfo and NULL when failure happens, will it simplify the code?

> 
> +[Depex]
> +  TRUE
> -[Depex]
> -  gEfiMpServiceProtocolGuid

10. The depex change means that CpuSmm driver could run before CpuMp driver runs. Have you verified if CpuSmm can start well even removing CpuMp DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112146): https://edk2.groups.io/g/devel/message/112146
Mute This Topic: https://groups.io/mt/102987139/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

[Ni, Ray]

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, December 7, 2023 8:23 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
> 
> Will change the code based on comments 1-9.
> 
> About comments 10, "10. The depex change means that CpuSmm driver could
> run before CpuMp driver runs. Have you verified if CpuSmm can start well
> even removing CpuMp DXE driver?"
> 
> Yes, I verified it in OvmfIa32X64 boot. CpuSmm can start well even removing
> CpuMp DXE driver.
Great!

> (also removed some checking for gEfiCpuArchProtocolGuid)
I assume the checking for CpuArch protocol is from other modules.


> 
> Thanks,
> Dun
> 
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, December 6, 2023 5:55 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
> 
> 1. The function name can be "GetMpInformation()" without mentioning
> "FromMpInfo2Hob".
> 
> > +  EFI_HOB_GUID_TYPE          *GuidHob;
> > +  EFI_HOB_GUID_TYPE          *FirstMpInfor2Hob;
> 
> 2. "FirstMpInfo2Hob". Please remove "r".
> 
> >+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);
> 
> 3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the
> 2nd while-loop without needing to look for MpInfo2Hob from beginning.
> 
> > +
> > +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> > +  *NumberOfCpus = CpuCount;
> 
> 4. There is no "return" before "*NumberOfCpus" assignment. So, why not
> remove "CpuCount" and directly udpates "*NumberOfCpus" in the
> while-loop?
> 
> > +
> > +  MpInfomation2Buffer = AllocatePool (sizeof
> > (MP_INFORMATION2_HOB_DATA *) * HobCount);
> 
> 5. MpInfomation2Buffer -> MpInfo2Hobs
> 
> 
> 6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?
> 
> > +  for (Index = 0; Index < HobCount; Index++) {
> 7. Index -> HobIndex
> 
> > +      CopyMem (
> > +        ProcessorInfo + PrevProcessorIndex + ProcessorIndex,
> 
> 8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]
> 
> > +
> > +  *ProcessorInfoPointer = ProcessorInfo;
> 
> 9. If you let the function just return ProcessorInfo and NULL when failure
> happens, will it simplify the code?
> 
> >
> > +[Depex]
> > +  TRUE
> > -[Depex]
> > -  gEfiMpServiceProtocolGuid
> 
> 10. The depex change means that CpuSmm driver could run before CpuMp
> driver runs. Have you verified if CpuSmm can start well even removing CpuMp
> DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112150): https://edk2.groups.io/g/devel/message/112150
Mute This Topic: https://groups.io/mt/102987139/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 4/6] UefiCpuPkg: Add a new field in MpInfo2 HOB

[duntan]

Add new field CoreType in gMpInformationHobGuid2

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Include/Guid/MpInformation2.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h b/UefiCpuPkg/Include/Guid/MpInformation2.h
index 4164ce1c30..cb654d6f05 100644
--- a/UefiCpuPkg/Include/Guid/MpInformation2.h
+++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
@@ -29,6 +29,8 @@
 
 typedef struct {
   EFI_PROCESSOR_INFORMATION    ProcessorInfo;
+  UINT8                        CoreType;
+  UINT8                        Reserved[7];
   //
   // Add more fields in future
   //
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112066): https://edk2.groups.io/g/devel/message/112066
Mute This Topic: https://groups.io/mt/102987140/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 4/6] UefiCpuPkg: Add a new field in MpInfo2 HOB

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, December 5, 2023 1:49 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH 4/6] UefiCpuPkg: Add a new field in MpInfo2 HOB
> 
> Add new field CoreType in gMpInformationHobGuid2
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Include/Guid/MpInformation2.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h
> b/UefiCpuPkg/Include/Guid/MpInformation2.h
> index 4164ce1c30..cb654d6f05 100644
> --- a/UefiCpuPkg/Include/Guid/MpInformation2.h
> +++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
> @@ -29,6 +29,8 @@
> 
>  typedef struct {
>    EFI_PROCESSOR_INFORMATION    ProcessorInfo;
> +  UINT8                        CoreType;
> +  UINT8                        Reserved[7];
>    //
>    // Add more fields in future
>    //
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112107): https://edk2.groups.io/g/devel/message/112107
Mute This Topic: https://groups.io/mt/102987140/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

[duntan]

Cache core type in MpInfo2 HOB by CpuMpPei module.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 UefiCpuPkg/CpuMpPei/CpuMpPei.h |  2 ++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index 4d80d52799..8f6863f4d9 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -541,6 +541,30 @@ InitializeMpExceptionStackSwitchHandlers (
   FreePages (SwitchStackData, EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
 }
 
+/**
+  Get CPU core type.
+
+  @param[in, out] Buffer  Argument of the procedure.
+**/
+VOID
+EFIAPI
+GetProcessorCoreType (
+  IN OUT VOID  *Buffer
+  )
+{
+  EFI_STATUS                               Status;
+  UINT8                                    *CoreType;
+  CPUID_NATIVE_MODEL_ID_AND_CORE_TYPE_EAX  NativeModelIdAndCoreTypeEax;
+  UINTN                                    ProcessorIndex;
+
+  Status = MpInitLibWhoAmI (&ProcessorIndex);
+  ASSERT_EFI_ERROR (Status);
+
+  CoreType = (UINT8 *)Buffer;
+  AsmCpuidEx (CPUID_HYBRID_INFORMATION, CPUID_HYBRID_INFORMATION_MAIN_LEAF, &NativeModelIdAndCoreTypeEax.Uint32, NULL, NULL, NULL);
+  CoreType[ProcessorIndex] = (UINT8)NativeModelIdAndCoreTypeEax.Bits.CoreType;
+}
+
 /**
   Create gMpInformationHobGuid2.
 **/
@@ -558,13 +582,36 @@ BuildMpInformationHob (
   MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
   MP_INFORMATION2_ENTRY     *MpInformation2Entry;
   UINTN                     Index;
+  UINT8                     *CoreType;
+  UINT32                    CpuidMaxInput;
+  UINTN                     Pages;
 
   ProcessorIndex        = 0;
   MpInformation2HobData = NULL;
   MpInformation2Entry   = NULL;
+  CoreType              = NULL;
+  Pages                 = 0;
 
   Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
   ASSERT_EFI_ERROR (Status);
+
+  //
+  // Get Processors CoreType
+  //
+  AsmCpuid (CPUID_SIGNATURE, &CpuidMaxInput, NULL, NULL, NULL);
+  if (CpuidMaxInput >= CPUID_HYBRID_INFORMATION) {
+    Pages    = EFI_SIZE_TO_PAGES (sizeof (UINT8) * NumberOfProcessors);
+    CoreType = AllocatePages (Pages);
+    ASSERT (CoreType != NULL);
+
+    Status = MpInitLibStartupAllCPUs (
+               GetProcessorCoreType,
+               0,
+               (VOID *)CoreType
+               );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   MaxProcessorsPerHob     = ((MAX_UINT16 & 7) - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_INFORMATION2_HOB_DATA)) / sizeof (MP_INFORMATION2_ENTRY);
   NumberOfProcessorsInHob = MaxProcessorsPerHob;
 
@@ -597,12 +644,16 @@ BuildMpInformationHob (
                               NULL
                               );
       ASSERT_EFI_ERROR (Status);
+
+      MpInformation2Entry->CoreType = (CoreType != NULL) ? CoreType[Index + ProcessorIndex] : 0;
+
       DEBUG ((
         DEBUG_INFO,
-        "ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x\n",
+        "ProcessorIndex = %x, ProcessorId = %lx, StatusFlag = %x, CoreType = %x\n",
         Index + ProcessorIndex,
         MpInformation2Entry->ProcessorInfo.ProcessorId,
-        MpInformation2Entry->ProcessorInfo.StatusFlag
+        MpInformation2Entry->ProcessorInfo.StatusFlag,
+        MpInformation2Entry->CoreType
         ));
       DEBUG ((
         DEBUG_INFO,
@@ -625,6 +676,10 @@ BuildMpInformationHob (
 
     ProcessorIndex += NumberOfProcessorsInHob;
   }
+
+  if (CoreType != NULL) {
+    FreePages (CoreType, Pages);
+  }
 }
 
 /**
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
index a40fd2c077..e7d07ffd64 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
@@ -32,6 +32,8 @@
 
 #include <Guid/MpInformation2.h>
 
+#include <Register/Cpuid.h>
+
 extern EFI_PEI_PPI_DESCRIPTOR  mPeiCpuMpPpiDesc;
 
 /**
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112067): https://edk2.groups.io/g/devel/message/112067
Mute This Topic: https://groups.io/mt/102987141/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

[Ni, Ray]

>  /**
>    Create gMpInformationHobGuid2.
>  **/
> @@ -558,13 +582,36 @@ BuildMpInformationHob (
>    MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
>    MP_INFORMATION2_ENTRY     *MpInformation2Entry;
>    UINTN                     Index;
> +  UINT8                     *CoreType;
> +  UINT32                    CpuidMaxInput;
> +  UINTN                     Pages;

1. CoreTypes, CoreTypePages


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112113): https://edk2.groups.io/g/devel/message/112113
Mute This Topic: https://groups.io/mt/102987141/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

[duntan]

Will change the commit based on the comments

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, December 6, 2023 6:02 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [PATCH 5/6] UefiCpuPkg: Cache core type in MpInfo2 HOB

>  /**
>    Create gMpInformationHobGuid2.
>  **/
> @@ -558,13 +582,36 @@ BuildMpInformationHob (
>    MP_INFORMATION2_HOB_DATA  *MpInformation2HobData;
>    MP_INFORMATION2_ENTRY     *MpInformation2Entry;
>    UINTN                     Index;
> +  UINT8                     *CoreType;
> +  UINT32                    CpuidMaxInput;
> +  UINTN                     Pages;

1. CoreTypes, CoreTypePages


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112147): https://edk2.groups.io/g/devel/message/112147
Mute This Topic: https://groups.io/mt/102987141/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[duntan]

Modify the gSmmBaseHobGuid consumption code to
remove the asuumption that there is only one
gSmmBaseHobGuid. If the CPU number is big enough,
there will be more than one SmmBaseHob in the
HOB list.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 130 insertions(+), 12 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index d8d488b253..36fa5e779a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -586,6 +586,130 @@ SmmReadyToLockEventNotify (
   return EFI_SUCCESS;
 }
 
+/**
+  Function to compare 2 SMM_BASE_HOB_DATA pointer based on ProcessorIndex.
+
+  @param[in] Buffer1            pointer to SMM_BASE_HOB_DATA poiner to compare
+  @param[in] Buffer2            pointer to second SMM_BASE_HOB_DATA pointer to compare
+
+  @retval 0                     Buffer1 equal to Buffer2
+  @retval <0                    Buffer1 is less than Buffer2
+  @retval >0                    Buffer1 is greater than Buffer2
+**/
+INTN
+EFIAPI
+SmBaseHobCompare (
+  IN  CONST VOID  *Buffer1,
+  IN  CONST VOID  *Buffer2
+  )
+{
+  if ((*(SMM_BASE_HOB_DATA **)Buffer1)->ProcessorIndex > (*(SMM_BASE_HOB_DATA **)Buffer2)->ProcessorIndex) {
+    return 1;
+  } else if ((*(SMM_BASE_HOB_DATA **)Buffer1)->ProcessorIndex < (*(SMM_BASE_HOB_DATA **)Buffer2)->ProcessorIndex) {
+    return -1;
+  }
+
+  return 0;
+}
+
+/**
+  Extract SmBase for all CPU from SmmBase HOB.
+
+  @param[in]  FirstSmmBaseGuidHob    Pointer to First SmmBaseGuidHob.
+  @param[in]  MaxNumberOfCpus        Max NumberOfCpus.
+  @param[out] SmBaseBufferPointer    Pointer to SmBase buffer pointer.
+
+  @retval EFI_SUCCESS                Successfully extract SmBase for all CPU from SmmBase HOB.
+  @retval RETURN_BUFFER_TOO_SMALL    Fail to allocate memory.
+**/
+EFI_STATUS
+GetSmBaseFromSmmBaseHob (
+  IN  EFI_HOB_GUID_TYPE  *FirstSmmBaseGuidHob,
+  IN  UINTN              MaxNumberOfCpus,
+  OUT UINTN              **SmBaseBufferPointer
+  )
+{
+  UINTN              HobCount;
+  EFI_HOB_GUID_TYPE  *GuidHob;
+  SMM_BASE_HOB_DATA  *SmmBaseHobData;
+  UINTN              NumberOfProcessors;
+  SMM_BASE_HOB_DATA  **SmBaseHobPointerBuffer;
+  UINTN              *SmBaseBuffer;
+  UINTN              Index;
+  UINTN              SortBuffer;
+  UINTN              ProcessorIndex;
+  UINT64             PrevProcessorIndex;
+
+  SmmBaseHobData     = NULL;
+  Index              = 0;
+  ProcessorIndex     = 0;
+  PrevProcessorIndex = 0;
+  HobCount           = 0;
+  NumberOfProcessors = 0;
+  GuidHob            = FirstSmmBaseGuidHob;
+
+  while (GuidHob != NULL) {
+    HobCount++;
+    SmmBaseHobData      = GET_GUID_HOB_DATA (GuidHob);
+    NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
+    GuidHob             = GetNextGuidHob (&gSmmBaseHobGuid, GET_NEXT_HOB (GuidHob));
+  }
+
+  ASSERT (NumberOfProcessors == MaxNumberOfCpus);
+
+  SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount);
+  ASSERT (SmBaseHobPointerBuffer != NULL);
+  if (SmBaseHobPointerBuffer == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  //
+  // Record each SmmBaseHob pointer in the SmBaseHobPointerBuffer.
+  //
+  GuidHob = FirstSmmBaseGuidHob;
+  ASSERT (GuidHob != NULL);
+  while (Index < HobCount) {
+    SmBaseHobPointerBuffer[Index++] = GET_GUID_HOB_DATA (GuidHob);
+    GuidHob                         = GetNextGuidHob (&gSmmBaseHobGuid, GET_NEXT_HOB (GuidHob));
+  }
+
+  SmBaseBuffer = (UINTN *)AllocatePool (sizeof (UINTN) * (MaxNumberOfCpus));
+  ASSERT (SmBaseBuffer != NULL);
+  if (SmBaseBuffer == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  QuickSort (SmBaseHobPointerBuffer, HobCount, sizeof (SMM_BASE_HOB_DATA *), (BASE_SORT_COMPARE)SmBaseHobCompare, &SortBuffer);
+  for (Index = 0; Index < HobCount; Index++) {
+    //
+    // Make sure no overlap and no gap in the CPU range covered by each HOB
+    //
+    ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex == PrevProcessorIndex);
+
+    //
+    // Cache each SmBase in order.
+    //
+    if (sizeof (UINTN) == sizeof (UINT64)) {
+      CopyMem (
+        SmBaseBuffer + PrevProcessorIndex,
+        &SmBaseHobPointerBuffer[Index]->SmBase,
+        sizeof (UINT64) * SmBaseHobPointerBuffer[Index]->NumberOfProcessors
+        );
+    } else {
+      for (ProcessorIndex = 0; ProcessorIndex < SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
+        SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] = (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
+      }
+    }
+
+    PrevProcessorIndex += SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
+  }
+
+  FreePool (SmBaseHobPointerBuffer);
+
+  *SmBaseBufferPointer = SmBaseBuffer;
+  return EFI_SUCCESS;
+}
+
 /**
   Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on ProcessorIndex.
 
@@ -766,10 +890,8 @@ PiCpuSmmEntry (
   UINTN              ModelId;
   UINT32             Cr3;
   EFI_HOB_GUID_TYPE  *GuidHob;
-  SMM_BASE_HOB_DATA  *SmmBaseHobData;
 
-  GuidHob        = NULL;
-  SmmBaseHobData = NULL;
+  GuidHob = NULL;
 
   PERF_FUNCTION_BEGIN ();
 
@@ -1002,12 +1124,8 @@ PiCpuSmmEntry (
       return RETURN_BUFFER_TOO_SMALL;
     }
 
-    SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
-
-    //
-    // Assume single instance of HOB produced, expect the HOB.NumberOfProcessors equals to the mMaxNumberOfCpus.
-    //
-    ASSERT (SmmBaseHobData->NumberOfProcessors == (UINT32)mMaxNumberOfCpus && SmmBaseHobData->ProcessorIndex == 0);
+    Status = GetSmBaseFromSmmBaseHob (GuidHob, mMaxNumberOfCpus, &mCpuHotPlugData.SmBase);
+    ASSERT_EFI_ERROR (Status);
     mSmmRelocated = TRUE;
   } else {
     //
@@ -1053,8 +1171,6 @@ PiCpuSmmEntry (
   //
   mCpuHotPlugData.ApicId = (UINT64 *)AllocatePool (sizeof (UINT64) * mMaxNumberOfCpus);
   ASSERT (mCpuHotPlugData.ApicId != NULL);
-  mCpuHotPlugData.SmBase = (UINTN *)AllocatePool (sizeof (UINTN) * mMaxNumberOfCpus);
-  ASSERT (mCpuHotPlugData.SmBase != NULL);
   mCpuHotPlugData.ArrayLength = (UINT32)mMaxNumberOfCpus;
 
   //
@@ -1063,7 +1179,9 @@ PiCpuSmmEntry (
   // size for each CPU in the platform
   //
   for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
-    mCpuHotPlugData.SmBase[Index] = mSmmRelocated ? (UINTN)SmmBaseHobData->SmBase[Index] : (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+    if (!mSmmRelocated) {
+      mCpuHotPlugData.SmBase[Index] = (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+    }
 
     gSmmCpuPrivate->CpuSaveStateSize[Index] = sizeof (SMRAM_SAVE_STATE_MAP);
     gSmmCpuPrivate->CpuSaveState[Index]     = (VOID *)(mCpuHotPlugData.SmBase[Index] + SMRAM_SAVE_STATE_MAP_OFFSET);
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112068): https://edk2.groups.io/g/devel/message/112068
Mute This Topic: https://groups.io/mt/102987142/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[Ni, Ray]

> +EFI_STATUS
> +GetSmBaseFromSmmBaseHob (
> +  IN  EFI_HOB_GUID_TYPE  *FirstSmmBaseGuidHob,
> +  IN  UINTN              MaxNumberOfCpus,
> +  OUT UINTN              **SmBaseBufferPointer
> +  )

1. It's a bit strange that caller should locate the first GuidHob.
Can you update the existing code as follows:
mCpuHotPlugData.SmBase = GetSmBase(mMaxNumberOfCpus);
if (mCpuHotPlugData.SmBase != NULL) {
  mSmmRelocated = TRUE;
}


> +{
> +  UINTN              HobCount;
> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  SMM_BASE_HOB_DATA  *SmmBaseHobData;
> +  UINTN              NumberOfProcessors;
> +  SMM_BASE_HOB_DATA  **SmBaseHobPointerBuffer;
> +  UINTN              *SmBaseBuffer;
> +  UINTN              Index;
> +  UINTN              SortBuffer;
> +  UINTN              ProcessorIndex;
> +  UINT64             PrevProcessorIndex;
> +
> +  SmmBaseHobData     = NULL;
> +  Index              = 0;
> +  ProcessorIndex     = 0;
> +  PrevProcessorIndex = 0;
> +  HobCount           = 0;
> +  NumberOfProcessors = 0;
> +  GuidHob            = FirstSmmBaseGuidHob;
> +
> +  while (GuidHob != NULL) {
> +    HobCount++;
> +    SmmBaseHobData      = GET_GUID_HOB_DATA (GuidHob);
> +    NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
> +    GuidHob             = GetNextGuidHob (&gSmmBaseHobGuid,
> GET_NEXT_HOB (GuidHob));

2. We could break the while-loop when NumberOfProcessors equals to the value we retrieved from MpInfo2Hob. Right?
This can speed up the code when there are lots of HOBs after the last SmmBaseHob instance.

> +  }
> +
> +  ASSERT (NumberOfProcessors == MaxNumberOfCpus);

3. ASSERT may fail when HotPlug is TRUE?


> +
> +  SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *)
> * HobCount);

4. SmBaseHobPointerBuffer -> SmBaseHobs

> +  for (Index = 0; Index < HobCount; Index++) {
> +    //
> +    // Make sure no overlap and no gap in the CPU range covered by each
> HOB
> +    //
> +    ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex ==
> PrevProcessorIndex);

5. similarly, can you move "PrevProcessorIndex" assignment to just above "for"?

> +
> +    //
> +    // Cache each SmBase in order.
> +    //
> +    if (sizeof (UINTN) == sizeof (UINT64)) {
> +      CopyMem (
> +        SmBaseBuffer + PrevProcessorIndex,
> +        &SmBaseHobPointerBuffer[Index]->SmBase,
> +        sizeof (UINT64) *
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors
> +        );
> +    } else {
> +      for (ProcessorIndex = 0; ProcessorIndex <
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
> +        SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =
> (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
> +      }
> +    }


6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UINT64 *?
Or, you always use for-loop to copy SmBase value for each cpu.

> +
> +    PrevProcessorIndex +=
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
> +  }
> +
> +  FreePool (SmBaseHobPointerBuffer);
> +
> +  *SmBaseBufferPointer = SmBaseBuffer;

7. Similarly, how about return SmBaseBuffer?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112119): https://edk2.groups.io/g/devel/message/112119
Mute This Topic: https://groups.io/mt/102987142/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[duntan]

Updated in original message.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, December 6, 2023 6:15 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

> +EFI_STATUS
> +GetSmBaseFromSmmBaseHob (
> +  IN  EFI_HOB_GUID_TYPE  *FirstSmmBaseGuidHob,
> +  IN  UINTN              MaxNumberOfCpus,
> +  OUT UINTN              **SmBaseBufferPointer
> +  )

1. It's a bit strange that caller should locate the first GuidHob.
Can you update the existing code as follows:
mCpuHotPlugData.SmBase = GetSmBase(mMaxNumberOfCpus); if (mCpuHotPlugData.SmBase != NULL) {
  mSmmRelocated = TRUE;
}

Dun: Ok, will change code to this.

> +{
> +  UINTN              HobCount;
> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  SMM_BASE_HOB_DATA  *SmmBaseHobData;
> +  UINTN              NumberOfProcessors;
> +  SMM_BASE_HOB_DATA  **SmBaseHobPointerBuffer;
> +  UINTN              *SmBaseBuffer;
> +  UINTN              Index;
> +  UINTN              SortBuffer;
> +  UINTN              ProcessorIndex;
> +  UINT64             PrevProcessorIndex;
> +
> +  SmmBaseHobData     = NULL;
> +  Index              = 0;
> +  ProcessorIndex     = 0;
> +  PrevProcessorIndex = 0;
> +  HobCount           = 0;
> +  NumberOfProcessors = 0;
> +  GuidHob            = FirstSmmBaseGuidHob;
> +
> +  while (GuidHob != NULL) {
> +    HobCount++;
> +    SmmBaseHobData      = GET_GUID_HOB_DATA (GuidHob);
> +    NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
> +    GuidHob             = GetNextGuidHob (&gSmmBaseHobGuid,
> GET_NEXT_HOB (GuidHob));

2. We could break the while-loop when NumberOfProcessors equals to the value we retrieved from MpInfo2Hob. Right?
This can speed up the code when there are lots of HOBs after the last SmmBaseHob instance.

Dun: If the code flow break before finding all potential SmmBaseHob instance, there may be more SmmBaseHob instance covering NumberOfProcessors more than the expected value. The code is to catch this case. Do you think we should also catch this?

> +  }
> +
> +  ASSERT (NumberOfProcessors == MaxNumberOfCpus);

3. ASSERT may fail when HotPlug is TRUE?

Dun: If HotPlug, I think the SmBase count should be PcdCpuMaxLogicalProcessorNumber instead of the NumberOfProcessors extracted from MpInfo2Hob?


> +
> +  SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *)
> * HobCount);

4. SmBaseHobPointerBuffer -> SmBaseHobs

Dun: will change the naming.

> +  for (Index = 0; Index < HobCount; Index++) {
> +    //
> +    // Make sure no overlap and no gap in the CPU range covered by 
> + each
> HOB
> +    //
> +    ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex ==
> PrevProcessorIndex);

5. similarly, can you move "PrevProcessorIndex" assignment to just above "for"?

Dun: Will change the code

> +
> +    //
> +    // Cache each SmBase in order.
> +    //
> +    if (sizeof (UINTN) == sizeof (UINT64)) {
> +      CopyMem (
> +        SmBaseBuffer + PrevProcessorIndex,
> +        &SmBaseHobPointerBuffer[Index]->SmBase,
> +        sizeof (UINT64) *
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors
> +        );
> +    } else {
> +      for (ProcessorIndex = 0; ProcessorIndex <
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
> +        SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =
> (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
> +      }
> +    }


6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UINT64 *?
Or, you always use for-loop to copy SmBase value for each cpu.

Dun: Ok, will always use for-loop to copy SmBase value for each cpu.

> +
> +    PrevProcessorIndex +=
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
> +  }
> +
> +  FreePool (SmBaseHobPointerBuffer);
> +
> +  *SmBaseBufferPointer = SmBaseBuffer;

7. Similarly, how about return SmBaseBuffer?

Dun: Ok, will change the code


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112148): https://edk2.groups.io/g/devel/message/112148
Mute This Topic: https://groups.io/mt/102987142/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

[Ni, Ray]

> 
> 2. We could break the while-loop when NumberOfProcessors equals to the
> value we retrieved from MpInfo2Hob. Right?
> This can speed up the code when there are lots of HOBs after the last
> SmmBaseHob instance.
> 
> Dun: If the code flow break before finding all potential SmmBaseHob instance,
> there may be more SmmBaseHob instance covering NumberOfProcessors
> more than the expected value. The code is to catch this case. Do you think we
> should also catch this?

When there are more instances than expected, we treat it as a failure.
When number of instances is expected, we treat gap or overlap as a failure.
We do not need to distinguish between the two kinds of failures.
So, we could assume the number of instances is expected, then check if all the
found instances can cover all processors.
If not, we treat it as a failure.
> 
> > +  }
> > +
> > +  ASSERT (NumberOfProcessors == MaxNumberOfCpus);
> 
> 3. ASSERT may fail when HotPlug is TRUE?
> 
> Dun: If HotPlug, I think the SmBase count should be
> PcdCpuMaxLogicalProcessorNumber instead of the NumberOfProcessors
> extracted from MpInfo2Hob?

You are right!


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112149): https://edk2.groups.io/g/devel/message/112149
Mute This Topic: https://groups.io/mt/102987142/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-