[PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct change

[PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct change

[Wu, Hao A]

The series will resolve a backward compatibility issue with pre-built
binaries (e.g. FSP) introduced by commit 88bd0661661.

The relocation of 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
fields in structure CPU_MP_DATA may cause access issue for platforms that
use pre-built FSP binary, since the offset of these microcode related
fields in CPU_MP_DATA can be different between PEI phase (in the pre-built
binary) and DXE phase (in current code implementation).

The series will use the newly introduced EDKII microcode patch HOB instead
for the DXE phase to get the information of the loaded (done in PEI phase)
microcode patches data.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>

Hao A Wu (2):
  Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in
    CPU_MP_DATA
  UefiCpuPkg/MpInitLib: Not pass microcode info between archs in
    CPU_MP_DATA

 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.h          | 27 +++++++++++-
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43 ++++++++++++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
 5 files changed, 82 insertions(+), 14 deletions(-)

-- 
2.12.0.windows.1

[PATCH v1 1/2] Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA

[Wu, Hao A]

This reverts commit 88bd06616617ef2569f093f7b51893c11ad78e26.

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2465

Commit 88bd0661661 relocates the 'MicrocodePatchAddress' and
'MicrocodePatchRegionSize' fields in structure CPU_MP_DATA to ensure that
they can be properly passed between different architectures.

However, such change is not backward compatible with the scenario like
pre-existing binaries such as FSP. These binaries are built when the code
base has a different version of the CPU_MP_DATA structure definition. This
may cause issues when accessing the 'MicrocodePatchAddress' and
'MicrocodePatchRegionSize' fields, since their offsets are different
(between PEI phase in the FSP binaries and DXE phase in current code
implementation).

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 7c62d75acc..d7e20f0b74 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -219,8 +219,6 @@ struct _CPU_MP_DATA {
   UINT64                         CpuInfoInHob;
   UINT32                         CpuCount;
   UINT32                         BspNumber;
-  UINT64                         MicrocodePatchAddress;
-  UINT64                         MicrocodePatchRegionSize;
   //
   // The above fields data will be passed from PEI to DXE
   // Please make sure the fields offset same in the different
@@ -264,6 +262,8 @@ struct _CPU_MP_DATA {
   UINT8                          Vector;
   BOOLEAN                        PeriodicMode;
   BOOLEAN                        TimerInterruptState;
+  UINT64                         MicrocodePatchAddress;
+  UINT64                         MicrocodePatchRegionSize;
 
   //
   // Whether need to use Init-Sipi-Sipi to wake up the APs.
-- 
2.12.0.windows.1

Re: [PATCH v1 1/2] Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: Thursday, February 6, 2020 1:24 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH v1 1/2] Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA

This reverts commit 88bd06616617ef2569f093f7b51893c11ad78e26.

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2465

Commit 88bd0661661 relocates the 'MicrocodePatchAddress' and 'MicrocodePatchRegionSize' fields in structure CPU_MP_DATA to ensure that they can be properly passed between different architectures.

However, such change is not backward compatible with the scenario like pre-existing binaries such as FSP. These binaries are built when the code base has a different version of the CPU_MP_DATA structure definition. This may cause issues when accessing the 'MicrocodePatchAddress' and 'MicrocodePatchRegionSize' fields, since their offsets are different (between PEI phase in the FSP binaries and DXE phase in current code implementation).

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 7c62d75acc..d7e20f0b74 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -219,8 +219,6 @@ struct _CPU_MP_DATA {
   UINT64                         CpuInfoInHob;
   UINT32                         CpuCount;
   UINT32                         BspNumber;
-  UINT64                         MicrocodePatchAddress;
-  UINT64                         MicrocodePatchRegionSize;
   //
   // The above fields data will be passed from PEI to DXE
   // Please make sure the fields offset same in the different @@ -264,6 +262,8 @@ struct _CPU_MP_DATA {
   UINT8                          Vector;
   BOOLEAN                        PeriodicMode;
   BOOLEAN                        TimerInterruptState;
+  UINT64                         MicrocodePatchAddress;
+  UINT64                         MicrocodePatchRegionSize;
 
   //
   // Whether need to use Init-Sipi-Sipi to wake up the APs.
--
2.12.0.windows.1

Re: [PATCH v1 1/2] Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA

[Ni, Ray]

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

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, February 6, 2020 1:24 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH v1 1/2] Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA
> 
> This reverts commit 88bd06616617ef2569f093f7b51893c11ad78e26.
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2465
> 
> Commit 88bd0661661 relocates the 'MicrocodePatchAddress' and
> 'MicrocodePatchRegionSize' fields in structure CPU_MP_DATA to ensure that
> they can be properly passed between different architectures.
> 
> However, such change is not backward compatible with the scenario like
> pre-existing binaries such as FSP. These binaries are built when the code
> base has a different version of the CPU_MP_DATA structure definition. This
> may cause issues when accessing the 'MicrocodePatchAddress' and
> 'MicrocodePatchRegionSize' fields, since their offsets are different
> (between PEI phase in the FSP binaries and DXE phase in current code
> implementation).
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 7c62d75acc..d7e20f0b74 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -219,8 +219,6 @@ struct _CPU_MP_DATA {
>    UINT64                         CpuInfoInHob;
>    UINT32                         CpuCount;
>    UINT32                         BspNumber;
> -  UINT64                         MicrocodePatchAddress;
> -  UINT64                         MicrocodePatchRegionSize;
>    //
>    // The above fields data will be passed from PEI to DXE
>    // Please make sure the fields offset same in the different
> @@ -264,6 +262,8 @@ struct _CPU_MP_DATA {
>    UINT8                          Vector;
>    BOOLEAN                        PeriodicMode;
>    BOOLEAN                        TimerInterruptState;
> +  UINT64                         MicrocodePatchAddress;
> +  UINT64                         MicrocodePatchRegionSize;
> 
>    //
>    // Whether need to use Init-Sipi-Sipi to wake up the APs.
> --
> 2.12.0.windows.1

[PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info between archs in CPU_MP_DATA

[Wu, Hao A]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2465

Commit 89164babec:
UefiCpuPkg/MpInitLib: don't shadow the microcode patch twice.

attempted to use 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
fields to avoid loading the microcode patches data into memory again in
the DXE phase.

However, the CPU_MP_DATA structure has members with type 'UINTN' or
pointer before the microcode patch related fields. This may cause issues
when PEI and DXE are of different archs (e.g. PEI - IA32, DXE - x64),
since the microcode patch related fields will have different offsets in
the CPU_MP_DATA structure.

Commit 88bd066166:
UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA

tried to resolve the above-mentioned issue by relocating the fields
'MicrocodePatchRegionSize' and 'MicrocodePatchAddress' before members with
different size between different archs. But it failed to take the case of
pre-built binaries (e.g. FSP) into consideration.

Binaries can be built when the code base had a different version of the
CPU_MP_DATA structure definition. This may cause issues when accessing
these microcode patch related fields, since their offsets are different
(between PEI phase in the binaries and DXE phase in current code
implementation).

This commit will use the newly introduced EDKII microcode patch HOB
instead for the DXE phase to get the information of the loaded microcode
patches data done in the PEI phase. And the 'MicrocodePatchRegionSize' and
'MicrocodePatchAddress' fields in CPU_MP_DATA will not be used to pass
information between phases.

For pre-built binaries, they can be classified into 3 types with regard to
the time when they are being built:

A. Before commit 89164babec
   (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
    were not being used to skip microcode load in DXE)

For this case, the EDKII microcode patch HOB will not be produced. This
commit will load the microcode patches data again in DXE. Such behavior is
the same with the code base back then.

B. After commit 89164babec, before commit e1ed55738e
   (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
    being used to skip microcode load in DXE, but failed to work properly
    between differnt archs.)

For this case, the EDKII microcode patch HOB will not be produced as well.
This commit will also load the microcode patches data again in DXE.

But since commit 89164babec failed to keep the detection and application
of microcode patches working properly in DXE after skipping the load, we
fall back to the origin behavior (that is to load the microcode patches
data again in DXE).

C. After commit e1ed55738e
   (In other words, EDKII microcode patch HOB will be produced.)

For this case, it will have the same behavior with the BIOS built from
the current source codes.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.h          | 23 +++++++++++
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43 ++++++++++++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
 5 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index bf5d18d521..9e6cce0895 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  MP Initialize Library instance for DXE driver.
 #
-#  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -58,6 +58,7 @@ [Protocols]
 [Guids]
   gEfiEventExitBootServicesGuid                 ## CONSUMES  ## Event
   gEfiEventLegacyBootGuid                       ## SOMETIMES_CONSUMES  ## Event
+  gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index d7e20f0b74..a6eab5f3d7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -29,6 +29,8 @@
 #include <Library/MtrrLib.h>
 #include <Library/HobLib.h>
 
+#include <Guid/MicrocodePatchHob.h>
+
 #include <IndustryStandard/FirmwareInterfaceTable.h>
 
 
@@ -600,6 +602,27 @@ ShadowMicrocodeUpdatePatch (
   );
 
 /**
+  Get the cached microcode patch base address and size from the microcode patch
+  information cache HOB.
+
+  @param[out] Address       Base address of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+  @param[out] RegionSize    Size of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+
+  @retval  TRUE     The microcode patch information cache HOB is found.
+  @retval  FALSE    The microcode patch information cache HOB is not found.
+
+**/
+BOOLEAN
+GetMicrocodePatchInfoFromHob (
+  UINT64                         *Address,
+  UINT64                         *RegionSize
+  );
+
+/**
   Detect whether Mwait-monitor feature is supported.
 
   @retval TRUE    Mwait-monitor feature is supported.
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 247f835b09..67e214d463 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -739,3 +739,46 @@ ShadowMicrocodeUpdatePatch (
     ShadowMicrocodePatchByPcd (CpuMpData);
   }
 }
+
+/**
+  Get the cached microcode patch base address and size from the microcode patch
+  information cache HOB.
+
+  @param[out] Address       Base address of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+  @param[out] RegionSize    Size of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+
+  @retval  TRUE     The microcode patch information cache HOB is found.
+  @retval  FALSE    The microcode patch information cache HOB is not found.
+
+**/
+BOOLEAN
+GetMicrocodePatchInfoFromHob (
+  UINT64                         *Address,
+  UINT64                         *RegionSize
+  )
+{
+  EFI_HOB_GUID_TYPE            *GuidHob;
+  EDKII_MICROCODE_PATCH_HOB    *MicrocodePathHob;
+
+  GuidHob = GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid);
+  if (GuidHob == NULL) {
+    DEBUG((DEBUG_INFO, "%a: Microcode patch cache HOB is not found.\n", __FUNCTION__));
+    return FALSE;
+  }
+
+  MicrocodePathHob = GET_GUID_HOB_DATA (GuidHob);
+
+  *Address    = MicrocodePathHob->MicrocodePatchAddress;
+  *RegionSize = MicrocodePathHob->MicrocodePatchRegionSize;
+
+  DEBUG((
+    DEBUG_INFO, "%a: MicrocodeBase = 0x%lx, MicrocodeSize = 0x%lx\n",
+    __FUNCTION__, *Address, *RegionSize
+    ));
+
+  return TRUE;
+}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 855d37ba3e..d0fbc17ce5 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1682,10 +1682,6 @@ MpInitLibInitialize (
   CpuMpData->SwitchBspFlag    = FALSE;
   CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
   CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber);
-  if (OldCpuMpData != NULL) {
-    CpuMpData->MicrocodePatchRegionSize = OldCpuMpData->MicrocodePatchRegionSize;
-    CpuMpData->MicrocodePatchAddress    = OldCpuMpData->MicrocodePatchAddress;
-  }
   InitializeSpinLock(&CpuMpData->MpLock);
 
   //
@@ -1740,11 +1736,6 @@ MpInitLibInitialize (
       //
       CollectProcessorCount (CpuMpData);
     }
-
-    //
-    // Load required microcode patches data into memory
-    //
-    ShadowMicrocodeUpdatePatch (CpuMpData);
   } else {
     //
     // APs have been wakeup before, just get the CPU Information
@@ -1762,6 +1753,17 @@ MpInitLibInitialize (
     }
   }
 
+  if (!GetMicrocodePatchInfoFromHob (
+         &CpuMpData->MicrocodePatchAddress,
+         &CpuMpData->MicrocodePatchRegionSize
+         )) {
+    //
+    // The microcode patch information cache HOB does not exist, which means
+    // the microcode patches data has not been loaded into memory yet
+    //
+    ShadowMicrocodeUpdatePatch (CpuMpData);
+  }
+
   //
   // Detect and apply Microcode on BSP
   //
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 06e3f5d0d3..6ecbed39ec 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -1,7 +1,7 @@
 /** @file
   MP initialize support functions for PEI phase.
 
-  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -9,7 +9,6 @@
 #include "MpLib.h"
 #include <Library/PeiServicesLib.h>
 #include <Guid/S3SmmInitDone.h>
-#include <Guid/MicrocodePatchHob.h>
 
 /**
   S3 SMM Init Done notification function.
-- 
2.12.0.windows.1

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info between archs in CPU_MP_DATA

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Thursday, February 6, 2020 1:24 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info between archs in CPU_MP_DATA

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2465

Commit 89164babec:
UefiCpuPkg/MpInitLib: don't shadow the microcode patch twice.

attempted to use 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
fields to avoid loading the microcode patches data into memory again in the DXE phase.

However, the CPU_MP_DATA structure has members with type 'UINTN' or pointer before the microcode patch related fields. This may cause issues when PEI and DXE are of different archs (e.g. PEI - IA32, DXE - x64), since the microcode patch related fields will have different offsets in the CPU_MP_DATA structure.

Commit 88bd066166:
UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA

tried to resolve the above-mentioned issue by relocating the fields 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress' before members with different size between different archs. But it failed to take the case of pre-built binaries (e.g. FSP) into consideration.

Binaries can be built when the code base had a different version of the CPU_MP_DATA structure definition. This may cause issues when accessing these microcode patch related fields, since their offsets are different (between PEI phase in the binaries and DXE phase in current code implementation).

This commit will use the newly introduced EDKII microcode patch HOB instead for the DXE phase to get the information of the loaded microcode patches data done in the PEI phase. And the 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress' fields in CPU_MP_DATA will not be used to pass information between phases.

For pre-built binaries, they can be classified into 3 types with regard to the time when they are being built:

A. Before commit 89164babec
   (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
    were not being used to skip microcode load in DXE)

For this case, the EDKII microcode patch HOB will not be produced. This commit will load the microcode patches data again in DXE. Such behavior is the same with the code base back then.

B. After commit 89164babec, before commit e1ed55738e
   (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
    being used to skip microcode load in DXE, but failed to work properly
    between differnt archs.)

For this case, the EDKII microcode patch HOB will not be produced as well.
This commit will also load the microcode patches data again in DXE.

But since commit 89164babec failed to keep the detection and application of microcode patches working properly in DXE after skipping the load, we fall back to the origin behavior (that is to load the microcode patches data again in DXE).

C. After commit e1ed55738e
   (In other words, EDKII microcode patch HOB will be produced.)

For this case, it will have the same behavior with the BIOS built from the current source codes.

Cc: Michael Kubacki <michael.a.kubacki@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.h          | 23 +++++++++++
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43 ++++++++++++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
 5 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index bf5d18d521..9e6cce0895 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  MP Initialize Library instance for DXE driver.
 #
-#  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2016 - 2020, Intel Corporation. All rights 
+reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent  #  ## @@ -58,6 +58,7 @@ [Protocols]  [Guids]
   gEfiEventExitBootServicesGuid                 ## CONSUMES  ## Event
   gEfiEventLegacyBootGuid                       ## SOMETIMES_CONSUMES  ## Event
+  gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index d7e20f0b74..a6eab5f3d7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -29,6 +29,8 @@
 #include <Library/MtrrLib.h>
 #include <Library/HobLib.h>
 
+#include <Guid/MicrocodePatchHob.h>
+
 #include <IndustryStandard/FirmwareInterfaceTable.h>
 
 
@@ -600,6 +602,27 @@ ShadowMicrocodeUpdatePatch (
   );
 
 /**
+  Get the cached microcode patch base address and size from the 
+ microcode patch  information cache HOB.
+
+  @param[out] Address       Base address of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+  @param[out] RegionSize    Size of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+
+  @retval  TRUE     The microcode patch information cache HOB is found.
+  @retval  FALSE    The microcode patch information cache HOB is not found.
+
+**/
+BOOLEAN
+GetMicrocodePatchInfoFromHob (
+  UINT64                         *Address,
+  UINT64                         *RegionSize
+  );
+
+/**
   Detect whether Mwait-monitor feature is supported.
 
   @retval TRUE    Mwait-monitor feature is supported.
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 247f835b09..67e214d463 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -739,3 +739,46 @@ ShadowMicrocodeUpdatePatch (
     ShadowMicrocodePatchByPcd (CpuMpData);
   }
 }
+
+/**
+  Get the cached microcode patch base address and size from the 
+microcode patch
+  information cache HOB.
+
+  @param[out] Address       Base address of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+  @param[out] RegionSize    Size of the microcode patches data.
+                            It will be updated if the microcode patch
+                            information cache HOB is found.
+
+  @retval  TRUE     The microcode patch information cache HOB is found.
+  @retval  FALSE    The microcode patch information cache HOB is not found.
+
+**/
+BOOLEAN
+GetMicrocodePatchInfoFromHob (
+  UINT64                         *Address,
+  UINT64                         *RegionSize
+  )
+{
+  EFI_HOB_GUID_TYPE            *GuidHob;
+  EDKII_MICROCODE_PATCH_HOB    *MicrocodePathHob;
+
+  GuidHob = GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid);  if 
+ (GuidHob == NULL) {
+    DEBUG((DEBUG_INFO, "%a: Microcode patch cache HOB is not found.\n", __FUNCTION__));
+    return FALSE;
+  }
+
+  MicrocodePathHob = GET_GUID_HOB_DATA (GuidHob);
+
+  *Address    = MicrocodePathHob->MicrocodePatchAddress;
+  *RegionSize = MicrocodePathHob->MicrocodePatchRegionSize;
+
+  DEBUG((
+    DEBUG_INFO, "%a: MicrocodeBase = 0x%lx, MicrocodeSize = 0x%lx\n",
+    __FUNCTION__, *Address, *RegionSize
+    ));
+
+  return TRUE;
+}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 855d37ba3e..d0fbc17ce5 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1682,10 +1682,6 @@ MpInitLibInitialize (
   CpuMpData->SwitchBspFlag    = FALSE;
   CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
   CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber);
-  if (OldCpuMpData != NULL) {
-    CpuMpData->MicrocodePatchRegionSize = OldCpuMpData->MicrocodePatchRegionSize;
-    CpuMpData->MicrocodePatchAddress    = OldCpuMpData->MicrocodePatchAddress;
-  }
   InitializeSpinLock(&CpuMpData->MpLock);
 
   //
@@ -1740,11 +1736,6 @@ MpInitLibInitialize (
       //
       CollectProcessorCount (CpuMpData);
     }
-
-    //
-    // Load required microcode patches data into memory
-    //
-    ShadowMicrocodeUpdatePatch (CpuMpData);
   } else {
     //
     // APs have been wakeup before, just get the CPU Information @@ -1762,6 +1753,17 @@ MpInitLibInitialize (
     }
   }
 
+  if (!GetMicrocodePatchInfoFromHob (
+         &CpuMpData->MicrocodePatchAddress,
+         &CpuMpData->MicrocodePatchRegionSize
+         )) {
+    //
+    // The microcode patch information cache HOB does not exist, which means
+    // the microcode patches data has not been loaded into memory yet
+    //
+    ShadowMicrocodeUpdatePatch (CpuMpData);  }
+
   //
   // Detect and apply Microcode on BSP
   //
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 06e3f5d0d3..6ecbed39ec 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -1,7 +1,7 @@
 /** @file
   MP initialize support functions for PEI phase.
 
-  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2020, Intel Corporation. All rights 
+ reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -9,7 +9,6 @@
 #include "MpLib.h"
 #include <Library/PeiServicesLib.h>
 #include <Guid/S3SmmInitDone.h>
-#include <Guid/MicrocodePatchHob.h>
 
 /**
   S3 SMM Init Done notification function.
--
2.12.0.windows.1

Re: [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info between archs in CPU_MP_DATA

[Ni, Ray]

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

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, February 6, 2020 1:24 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Kubacki, Michael A
> <michael.a.kubacki@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info
> between archs in CPU_MP_DATA
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2465
> 
> Commit 89164babec:
> UefiCpuPkg/MpInitLib: don't shadow the microcode patch twice.
> 
> attempted to use 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
> fields to avoid loading the microcode patches data into memory again in
> the DXE phase.
> 
> However, the CPU_MP_DATA structure has members with type 'UINTN' or
> pointer before the microcode patch related fields. This may cause issues
> when PEI and DXE are of different archs (e.g. PEI - IA32, DXE - x64),
> since the microcode patch related fields will have different offsets in
> the CPU_MP_DATA structure.
> 
> Commit 88bd066166:
> UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA
> 
> tried to resolve the above-mentioned issue by relocating the fields
> 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress' before members with
> different size between different archs. But it failed to take the case of
> pre-built binaries (e.g. FSP) into consideration.
> 
> Binaries can be built when the code base had a different version of the
> CPU_MP_DATA structure definition. This may cause issues when accessing
> these microcode patch related fields, since their offsets are different
> (between PEI phase in the binaries and DXE phase in current code
> implementation).
> 
> This commit will use the newly introduced EDKII microcode patch HOB
> instead for the DXE phase to get the information of the loaded microcode
> patches data done in the PEI phase. And the 'MicrocodePatchRegionSize' and
> 'MicrocodePatchAddress' fields in CPU_MP_DATA will not be used to pass
> information between phases.
> 
> For pre-built binaries, they can be classified into 3 types with regard to
> the time when they are being built:
> 
> A. Before commit 89164babec
>    (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
>     were not being used to skip microcode load in DXE)
> 
> For this case, the EDKII microcode patch HOB will not be produced. This
> commit will load the microcode patches data again in DXE. Such behavior is
> the same with the code base back then.
> 
> B. After commit 89164babec, before commit e1ed55738e
>    (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
>     being used to skip microcode load in DXE, but failed to work properly
>     between differnt archs.)
> 
> For this case, the EDKII microcode patch HOB will not be produced as well.
> This commit will also load the microcode patches data again in DXE.
> 
> But since commit 89164babec failed to keep the detection and application
> of microcode patches working properly in DXE after skipping the load, we
> fall back to the origin behavior (that is to load the microcode patches
> data again in DXE).
> 
> C. After commit e1ed55738e
>    (In other words, EDKII microcode patch HOB will be produced.)
> 
> For this case, it will have the same behavior with the BIOS built from
> the current source codes.
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 23 +++++++++++
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43 ++++++++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
>  5 files changed, 80 insertions(+), 12 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index bf5d18d521..9e6cce0895 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  MP Initialize Library instance for DXE driver.
>  #
> -#  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
>  ##
> @@ -58,6 +58,7 @@ [Protocols]
>  [Guids]
>    gEfiEventExitBootServicesGuid                 ## CONSUMES  ## Event
>    gEfiEventLegacyBootGuid                       ## SOMETIMES_CONSUMES  ## Event
> +  gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ##
> HOB
> 
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index d7e20f0b74..a6eab5f3d7 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -29,6 +29,8 @@
>  #include <Library/MtrrLib.h>
>  #include <Library/HobLib.h>
> 
> +#include <Guid/MicrocodePatchHob.h>
> +
>  #include <IndustryStandard/FirmwareInterfaceTable.h>
> 
> 
> @@ -600,6 +602,27 @@ ShadowMicrocodeUpdatePatch (
>    );
> 
>  /**
> +  Get the cached microcode patch base address and size from the microcode
> patch
> +  information cache HOB.
> +
> +  @param[out] Address       Base address of the microcode patches data.
> +                            It will be updated if the microcode patch
> +                            information cache HOB is found.
> +  @param[out] RegionSize    Size of the microcode patches data.
> +                            It will be updated if the microcode patch
> +                            information cache HOB is found.
> +
> +  @retval  TRUE     The microcode patch information cache HOB is found.
> +  @retval  FALSE    The microcode patch information cache HOB is not found.
> +
> +**/
> +BOOLEAN
> +GetMicrocodePatchInfoFromHob (
> +  UINT64                         *Address,
> +  UINT64                         *RegionSize
> +  );
> +
> +/**
>    Detect whether Mwait-monitor feature is supported.
> 
>    @retval TRUE    Mwait-monitor feature is supported.
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 247f835b09..67e214d463 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -739,3 +739,46 @@ ShadowMicrocodeUpdatePatch (
>      ShadowMicrocodePatchByPcd (CpuMpData);
>    }
>  }
> +
> +/**
> +  Get the cached microcode patch base address and size from the microcode
> patch
> +  information cache HOB.
> +
> +  @param[out] Address       Base address of the microcode patches data.
> +                            It will be updated if the microcode patch
> +                            information cache HOB is found.
> +  @param[out] RegionSize    Size of the microcode patches data.
> +                            It will be updated if the microcode patch
> +                            information cache HOB is found.
> +
> +  @retval  TRUE     The microcode patch information cache HOB is found.
> +  @retval  FALSE    The microcode patch information cache HOB is not found.
> +
> +**/
> +BOOLEAN
> +GetMicrocodePatchInfoFromHob (
> +  UINT64                         *Address,
> +  UINT64                         *RegionSize
> +  )
> +{
> +  EFI_HOB_GUID_TYPE            *GuidHob;
> +  EDKII_MICROCODE_PATCH_HOB    *MicrocodePathHob;
> +
> +  GuidHob = GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid);
> +  if (GuidHob == NULL) {
> +    DEBUG((DEBUG_INFO, "%a: Microcode patch cache HOB is not found.\n",
> __FUNCTION__));
> +    return FALSE;
> +  }
> +
> +  MicrocodePathHob = GET_GUID_HOB_DATA (GuidHob);
> +
> +  *Address    = MicrocodePathHob->MicrocodePatchAddress;
> +  *RegionSize = MicrocodePathHob->MicrocodePatchRegionSize;
> +
> +  DEBUG((
> +    DEBUG_INFO, "%a: MicrocodeBase = 0x%lx, MicrocodeSize = 0x%lx\n",
> +    __FUNCTION__, *Address, *RegionSize
> +    ));
> +
> +  return TRUE;
> +}
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 855d37ba3e..d0fbc17ce5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1682,10 +1682,6 @@ MpInitLibInitialize (
>    CpuMpData->SwitchBspFlag    = FALSE;
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
>    CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData->CpuData +
> MaxLogicalProcessorNumber);
> -  if (OldCpuMpData != NULL) {
> -    CpuMpData->MicrocodePatchRegionSize = OldCpuMpData-
> >MicrocodePatchRegionSize;
> -    CpuMpData->MicrocodePatchAddress    = OldCpuMpData-
> >MicrocodePatchAddress;
> -  }
>    InitializeSpinLock(&CpuMpData->MpLock);
> 
>    //
> @@ -1740,11 +1736,6 @@ MpInitLibInitialize (
>        //
>        CollectProcessorCount (CpuMpData);
>      }
> -
> -    //
> -    // Load required microcode patches data into memory
> -    //
> -    ShadowMicrocodeUpdatePatch (CpuMpData);
>    } else {
>      //
>      // APs have been wakeup before, just get the CPU Information
> @@ -1762,6 +1753,17 @@ MpInitLibInitialize (
>      }
>    }
> 
> +  if (!GetMicrocodePatchInfoFromHob (
> +         &CpuMpData->MicrocodePatchAddress,
> +         &CpuMpData->MicrocodePatchRegionSize
> +         )) {
> +    //
> +    // The microcode patch information cache HOB does not exist, which means
> +    // the microcode patches data has not been loaded into memory yet
> +    //
> +    ShadowMicrocodeUpdatePatch (CpuMpData);
> +  }
> +
>    //
>    // Detect and apply Microcode on BSP
>    //
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 06e3f5d0d3..6ecbed39ec 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    MP initialize support functions for PEI phase.
> 
> -  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -9,7 +9,6 @@
>  #include "MpLib.h"
>  #include <Library/PeiServicesLib.h>
>  #include <Guid/S3SmmInitDone.h>
> -#include <Guid/MicrocodePatchHob.h>
> 
>  /**
>    S3 SMM Init Done notification function.
> --
> 2.12.0.windows.1

Re: [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct change

[Wu, Hao A]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, February 06, 2020 1:24 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A; Kubacki, Michael A; Kinney, Michael D; Dong, Eric; Ni, Ray;
> Laszlo Ersek
> Subject: [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct
> change
> 
> The series will resolve a backward compatibility issue with pre-built
> binaries (e.g. FSP) introduced by commit 88bd0661661.
> 
> The relocation of 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
> fields in structure CPU_MP_DATA may cause access issue for platforms that
> use pre-built FSP binary, since the offset of these microcode related
> fields in CPU_MP_DATA can be different between PEI phase (in the pre-built
> binary) and DXE phase (in current code implementation).
> 
> The series will use the newly introduced EDKII microcode patch HOB instead
> for the DXE phase to get the information of the loaded (done in PEI phase)
> microcode patches data.


Sorry, I forgot to mention that

The series is also available at:
https://github.com/hwu25/edk2/tree/patch_mpinitlib_fsp_v1

Tests done for the series:
1. OS boot successfully on platform (not using FSP binary) when the EDKII
   microcode patch HOB is produced. Debug messages show that the microcode
   detection and application work properly.
2. OS boot successfully on platform (not using FSP binary) when the EDKII
   microcode patch HOB is NOT produced. Debug messages show that the microcode
   detection and application work properly.

Note:
At this moment, I am not able to verify the OS boot on platform that uses
pre-built FSP binary. I am seeking help to verify this case.

Best Regards,
Hao Wu


> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Hao A Wu (2):
>   Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in
>     CPU_MP_DATA
>   UefiCpuPkg/MpInitLib: Not pass microcode info between archs in
>     CPU_MP_DATA
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 27 +++++++++++-
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43 ++++++++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
>  5 files changed, 82 insertions(+), 14 deletions(-)
> 
> --
> 2.12.0.windows.1

Re: [edk2-devel] [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct change

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wu, Hao A
> Sent: Thursday, February 06, 2020 2:21 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A; Kinney, Michael D; Dong, Eric; Ni, Ray; Laszlo Ersek
> Subject: Re: [edk2-devel] [PATCH v1 0/2] Fix backward incompatible
> CPU_MP_DATA struct change
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Thursday, February 06, 2020 1:24 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A; Kubacki, Michael A; Kinney, Michael D; Dong, Eric; Ni, Ray;
> > Laszlo Ersek
> > Subject: [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct
> > change
> >
> > The series will resolve a backward compatibility issue with pre-built
> > binaries (e.g. FSP) introduced by commit 88bd0661661.
> >
> > The relocation of 'MicrocodePatchRegionSize' and
> 'MicrocodePatchAddress'
> > fields in structure CPU_MP_DATA may cause access issue for platforms
> that
> > use pre-built FSP binary, since the offset of these microcode related
> > fields in CPU_MP_DATA can be different between PEI phase (in the pre-
> built
> > binary) and DXE phase (in current code implementation).
> >
> > The series will use the newly introduced EDKII microcode patch HOB
> instead
> > for the DXE phase to get the information of the loaded (done in PEI phase)
> > microcode patches data.
> 
> 
> Sorry, I forgot to mention that
> 
> The series is also available at:
> https://github.com/hwu25/edk2/tree/patch_mpinitlib_fsp_v1
> 
> Tests done for the series:
> 1. OS boot successfully on platform (not using FSP binary) when the EDKII
>    microcode patch HOB is produced. Debug messages show that the
> microcode
>    detection and application work properly.
> 2. OS boot successfully on platform (not using FSP binary) when the EDKII
>    microcode patch HOB is NOT produced. Debug messages show that the
> microcode
>    detection and application work properly.
> 
> Note:
> At this moment, I am not able to verify the OS boot on platform that uses
> pre-built FSP binary. I am seeking help to verify this case.


Additional information:
For the above note, I am able to verify that this series can resolve the boot
issue on platforms that use pre-built FSP binaries.

Since the series has already received the R-b tags from reviewers/maintainers,
I plan to push it soon.

Best Regards,
Hao Wu


> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> >
> > Hao A Wu (2):
> >   Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in
> >     CPU_MP_DATA
> >   UefiCpuPkg/MpInitLib: Not pass microcode info between archs in
> >     CPU_MP_DATA
> >
> >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
> >  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 27 +++++++++++-
> >  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43
> ++++++++++++++++++++
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
> >  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
> >  5 files changed, 82 insertions(+), 14 deletions(-)
> >
> > --
> > 2.12.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct change

[Wu, Hao A]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Tuesday, February 11, 2020 10:55 AM
> To: devel@edk2.groups.io; Wu, Hao A
> Cc: Kubacki, Michael A; Kinney, Michael D; Dong, Eric; Ni, Ray; Laszlo Ersek
> Subject: RE: [edk2-devel] [PATCH v1 0/2] Fix backward incompatible
> CPU_MP_DATA struct change
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Wu, Hao A
> > Sent: Thursday, February 06, 2020 2:21 PM
> > To: devel@edk2.groups.io
> > Cc: Kubacki, Michael A; Kinney, Michael D; Dong, Eric; Ni, Ray; Laszlo Ersek
> > Subject: Re: [edk2-devel] [PATCH v1 0/2] Fix backward incompatible
> > CPU_MP_DATA struct change
> >
> > > -----Original Message-----
> > > From: Wu, Hao A
> > > Sent: Thursday, February 06, 2020 1:24 PM
> > > To: devel@edk2.groups.io
> > > Cc: Wu, Hao A; Kubacki, Michael A; Kinney, Michael D; Dong, Eric; Ni, Ray;
> > > Laszlo Ersek
> > > Subject: [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct
> > > change
> > >
> > > The series will resolve a backward compatibility issue with pre-built
> > > binaries (e.g. FSP) introduced by commit 88bd0661661.
> > >
> > > The relocation of 'MicrocodePatchRegionSize' and
> > 'MicrocodePatchAddress'
> > > fields in structure CPU_MP_DATA may cause access issue for platforms
> > that
> > > use pre-built FSP binary, since the offset of these microcode related
> > > fields in CPU_MP_DATA can be different between PEI phase (in the pre-
> > built
> > > binary) and DXE phase (in current code implementation).
> > >
> > > The series will use the newly introduced EDKII microcode patch HOB
> > instead
> > > for the DXE phase to get the information of the loaded (done in PEI phase)
> > > microcode patches data.
> >
> >
> > Sorry, I forgot to mention that
> >
> > The series is also available at:
> > https://github.com/hwu25/edk2/tree/patch_mpinitlib_fsp_v1
> >
> > Tests done for the series:
> > 1. OS boot successfully on platform (not using FSP binary) when the EDKII
> >    microcode patch HOB is produced. Debug messages show that the
> > microcode
> >    detection and application work properly.
> > 2. OS boot successfully on platform (not using FSP binary) when the EDKII
> >    microcode patch HOB is NOT produced. Debug messages show that the
> > microcode
> >    detection and application work properly.
> >
> > Note:
> > At this moment, I am not able to verify the OS boot on platform that uses
> > pre-built FSP binary. I am seeking help to verify this case.
> 
> 
> Additional information:
> For the above note, I am able to verify that this series can resolve the boot
> issue on platforms that use pre-built FSP binaries.
> 
> Since the series has already received the R-b tags from
> reviewers/maintainers,
> I plan to push it soon.


Thanks all.
The series has been pushed via commits ccb4c38a50..348a34d984.

Best Regards,
Hao Wu


> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > >
> > > Hao A Wu (2):
> > >   Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in
> > >     CPU_MP_DATA
> > >   UefiCpuPkg/MpInitLib: Not pass microcode info between archs in
> > >     CPU_MP_DATA
> > >
> > >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
> > >  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 27 +++++++++++-
> > >  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43
> > ++++++++++++++++++++
> > >  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
> > >  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
> > >  5 files changed, 82 insertions(+), 14 deletions(-)
> > >
> > > --
> > > 2.12.0.windows.1
> >
> >
> > 

Re: [PATCH v1 0/2] Fix backward incompatible CPU_MP_DATA struct change

[Laszlo Ersek]

On 02/06/20 06:23, Hao A Wu wrote:
> The series will resolve a backward compatibility issue with pre-built
> binaries (e.g. FSP) introduced by commit 88bd0661661.
> 
> The relocation of 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress'
> fields in structure CPU_MP_DATA may cause access issue for platforms that
> use pre-built FSP binary, since the offset of these microcode related
> fields in CPU_MP_DATA can be different between PEI phase (in the pre-built
> binary) and DXE phase (in current code implementation).
> 
> The series will use the newly introduced EDKII microcode patch HOB instead
> for the DXE phase to get the information of the loaded (done in PEI phase)
> microcode patches data.
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Hao A Wu (2):
>   Revert UefiCpuPkg/MpInitLib: Relocate microcode patch fields in
>     CPU_MP_DATA
>   UefiCpuPkg/MpInitLib: Not pass microcode info between archs in
>     CPU_MP_DATA
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 27 +++++++++++-
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 43 ++++++++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 20 +++++----
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  3 +-
>  5 files changed, 82 insertions(+), 14 deletions(-)
> 

Looks plausible to me, but my review doesn't (and shouldn't) carry much
weight here.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo