[PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Leo Duran]

This patch set fixes an issue introduced recently in MpInitLib, where we read
a PlatformId MSR that is not implemented on AMD processors.

The proposed solution is to export the StandardSignatureIsAuthenticAMD function
from LocalApicLib, so that it may be used by MpInitLib or any other module that
consumes LocalApicLib.

Alternatively, we considered creating a new library, but opted against it as
that would incur quite a bit of churning across modules that consume MpInitLib.

BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(),
but it never affected AMD-based platforms as the flow never gets that far, since
the Detect routine bails out early when it finds the size of the patch is zero.


Leo Duran (2):
  UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD
    function
  UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.

 UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++-----------
 .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++-----------
 UefiCpuPkg/Library/MpInitLib/Microcode.c           | 17 +++++++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c               | 11 ++++-
 5 files changed, 87 insertions(+), 50 deletions(-)

-- 
2.7.4

[PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Leo Duran]

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

The StandardSignatureIsAuthenticAMD function was introduced locally
to help divert code paths pertinent (or not) to AMD processors.
This patch exports that function so that it may serve the same purpose
in other modules that consume LocalApicLib.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Leo Duran <leo.duran@amd.com>
---
 UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++-----------
 .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++-----------
 3 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h b/UefiCpuPkg/Include/Library/LocalApicLib.h
index 96b93aa..a6e9dc6 100644
--- a/UefiCpuPkg/Include/Library/LocalApicLib.h
+++ b/UefiCpuPkg/Include/Library/LocalApicLib.h
@@ -5,6 +5,8 @@
   handles cases where local APIC is disabled.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
+
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -16,6 +18,19 @@
 #define LOCAL_APIC_MODE_X2APIC  0x2  ///< x2APIC mode.
 
 /**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  );
+
+/**
   Retrieve the base address of local APIC.
 
   @return The base address of local APIC.
diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
index 33ea15c..cebf1b3 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
@@ -4,7 +4,7 @@
   This local APIC library instance supports xAPIC mode only.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
-  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
+  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -27,28 +27,6 @@
 //
 
 /**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
-/**
   Determine if the CPU supports the Local APIC Base Address MSR.
 
   @retval TRUE  The CPU supports the Local APIC Base Address MSR.
@@ -76,6 +54,29 @@ LocalApicBaseAddressMsrSupported (
 }
 
 /**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  )
+{
+  UINT32  RegEbx;
+  UINT32  RegEcx;
+  UINT32  RegEdx;
+
+  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
+  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
+          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
+          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
+}
+
+/**
   Retrieve the base address of local APIC.
 
   @return The base address of local APIC.
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
index d0f92b3..01996b1 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
@@ -5,7 +5,7 @@
   which have xAPIC and x2APIC modes.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
-  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
+  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -28,28 +28,6 @@
 //
 
 /**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
-/**
   Determine if the CPU supports the Local APIC Base Address MSR.
 
   @retval TRUE  The CPU supports the Local APIC Base Address MSR.
@@ -77,6 +55,29 @@ LocalApicBaseAddressMsrSupported (
 }
 
 /**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  )
+{
+  UINT32  RegEbx;
+  UINT32  RegEcx;
+  UINT32  RegEdx;
+
+  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
+  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
+          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
+          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
+}
+
+/**
   Retrieve the base address of local APIC.
 
   @return The base address of local APIC.
-- 
2.7.4

[PATCH 2/2] UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.

[Leo Duran]

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

This patch uses the newly exported StandardSignatureIsAuthenticAMD function
from LocalApicLib, to divert code paths not pertinent to AMD processors.
Specifically, the PlatformId MSR and embedded Microcode patches are not
relevant on AMD-based platforms.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Leo Duran <leo.duran@amd.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 17 +++++++++++++++--
 UefiCpuPkg/Library/MpInitLib/MpLib.c     | 11 +++++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 1562959..750681d 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -2,6 +2,8 @@
   Implementation of loading microcode on processors.
 
   Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
+
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -97,9 +99,13 @@ MicrocodeDetect (
   UINT32                                  ThreadId;
   BOOLEAN                                 IsBspCallIn;
 
-  if (CpuMpData->MicrocodePatchRegionSize == 0) {
+  //
+  // NOTE: Embedded Microcode patches are not relevant on AMD platforms.
+  //
+  if (CpuMpData->MicrocodePatchRegionSize == 0 ||
+      StandardSignatureIsAuthenticAMD ()) {
     //
-    // There is no microcode patches
+    // There are no microcode patches
     //
     return;
   }
@@ -350,6 +356,13 @@ IsProcessorMatchedMicrocodePatch (
   UINTN          Index;
   CPU_AP_DATA    *CpuData;
 
+  //
+  // NOTE: PlatformId or embedded Microcode patches are not relevant on AMD platforms.
+  //
+  if (StandardSignatureIsAuthenticAMD ()) {
+    return FALSE;
+  }
+
   for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
     CpuData = &CpuMpData->CpuData[Index];
     if ((ProcessorSignature == CpuData->ProcessorSignature) &&
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d0fbc17..290e7bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2,6 +2,8 @@
   CPU MP Initialize Library common functions.
 
   Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
+
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -564,8 +566,13 @@ InitializeApData (
   CpuMpData->CpuData[ProcessorNumber].Waiting    = FALSE;
   CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE;
 
-  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
-  CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
+  //
+  // NOTE: PlatformId is not relevant on AMD platforms.
+  //
+  if (!StandardSignatureIsAuthenticAMD ()) {
+    PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
+    CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)PlatformIdMsr.Bits.PlatformId;
+  }
 
   AsmCpuid (
     CPUID_VERSION_INFO,
-- 
2.7.4

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Laszlo Ersek]

Hi Leo,

On 02/25/20 20:39, Leo Duran wrote:
> This patch set fixes an issue introduced recently in MpInitLib, where we read
> a PlatformId MSR that is not implemented on AMD processors.
> 
> The proposed solution is to export the StandardSignatureIsAuthenticAMD function
> from LocalApicLib, so that it may be used by MpInitLib or any other module that
> consumes LocalApicLib.
> 
> Alternatively, we considered creating a new library, but opted against it as
> that would incur quite a bit of churning across modules that consume MpInitLib.
> 
> BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(),
> but it never affected AMD-based platforms as the flow never gets that far, since
> the Detect routine bails out early when it finds the size of the patch is zero.
> 
> 
> Leo Duran (2):
>   UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD
>     function
>   UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.
> 
>  UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++-----------
>  .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++-----------
>  UefiCpuPkg/Library/MpInitLib/Microcode.c           | 17 +++++++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c               | 11 ++++-
>  5 files changed, 87 insertions(+), 50 deletions(-)
> 

from my perspective I'm OK with this approach:

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

but Ray and Eric have the final word on this, of course.

Thanks
Laszlo

Re: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Dong, Eric]

Hi Leo,

I check the code and find the real change for the C files are add "EFIAPI" in the code.  Can you help to refine the change and only keep the real changes?

Thanks,
Eric

-----Original Message-----
From: Leo Duran <leo.duran@amd.com> 
Sent: Wednesday, February 26, 2020 3:39 AM
To: devel@edk2.groups.io
Cc: Leo Duran <leo.duran@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

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

The StandardSignatureIsAuthenticAMD function was introduced locally to help divert code paths pertinent (or not) to AMD processors.
This patch exports that function so that it may serve the same purpose in other modules that consume LocalApicLib.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Leo Duran <leo.duran@amd.com>
---
 UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++-----------
 .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++-----------
 3 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h b/UefiCpuPkg/Include/Library/LocalApicLib.h
index 96b93aa..a6e9dc6 100644
--- a/UefiCpuPkg/Include/Library/LocalApicLib.h
+++ b/UefiCpuPkg/Include/Library/LocalApicLib.h
@@ -5,6 +5,8 @@
   handles cases where local APIC is disabled.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
+
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -16,6 +18,19 @@
 #define LOCAL_APIC_MODE_X2APIC  0x2  ///< x2APIC mode.
 
 /**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  );
+
+/**
   Retrieve the base address of local APIC.
 
   @return The base address of local APIC.
diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
index 33ea15c..cebf1b3 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
@@ -4,7 +4,7 @@
   This local APIC library instance supports xAPIC mode only.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
-  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
+  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -27,28 +27,6 @@
 //
 
 /**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
-/**
   Determine if the CPU supports the Local APIC Base Address MSR.
 
   @retval TRUE  The CPU supports the Local APIC Base Address MSR.
@@ -76,6 +54,29 @@ LocalApicBaseAddressMsrSupported (  }
 
 /**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  )
+{
+  UINT32  RegEbx;
+  UINT32  RegEcx;
+  UINT32  RegEdx;
+
+  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
+  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
+          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
+          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
+}
+
+/**
   Retrieve the base address of local APIC.
 
   @return The base address of local APIC.
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
index d0f92b3..01996b1 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
@@ -5,7 +5,7 @@
   which have xAPIC and x2APIC modes.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
-  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
+  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -28,28 +28,6 @@
 //
 
 /**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
-/**
   Determine if the CPU supports the Local APIC Base Address MSR.
 
   @retval TRUE  The CPU supports the Local APIC Base Address MSR.
@@ -77,6 +55,29 @@ LocalApicBaseAddressMsrSupported (  }
 
 /**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  )
+{
+  UINT32  RegEbx;
+  UINT32  RegEcx;
+  UINT32  RegEdx;
+
+  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
+  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
+          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
+          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
+}
+
+/**
   Retrieve the base address of local APIC.
 
   @return The base address of local APIC.
--
2.7.4

Re: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Duran, Leo]

Hi Eric,

I added EFIAPI as the function is now intended to be called externally, as a member of "LocalApicLib".
And to that end I added the function prototype in UefiCpuPkg/Include/Library/LocalApicLib,h.

But perhaps I've misunderstood your question?

Thanks,
Leo.


> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Tuesday, February 25, 2020 8:13 PM
> To: Duran, Leo <leo.duran@amd.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> StandardSignatureIsAuthenticAMD function
> 
> Hi Leo,
> 
> I check the code and find the real change for the C files are add "EFIAPI" in
> the code.  Can you help to refine the change and only keep the real changes?
> 
> Thanks,
> Eric
> 
> -----Original Message-----
> From: Leo Duran <leo.duran@amd.com>
> Sent: Wednesday, February 26, 2020 3:39 AM
> To: devel@edk2.groups.io
> Cc: Leo Duran <leo.duran@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> StandardSignatureIsAuthenticAMD function
> 
> REF:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzill
> a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> duran%40amd.com%7C19e3e4c20ffa41d8de8508d7ba591944%7C3dd8961f
> e4884e608e11a82d994e183d%7C0%7C0%7C637182764151879166&amp;s
> data=n%2B3eeGu7%2BvmVCz2QP6aeJuiqQ08gX5ZaCAuDkNMO%2Bb8%3D&a
> mp;reserved=0
> 
> The StandardSignatureIsAuthenticAMD function was introduced locally to
> help divert code paths pertinent (or not) to AMD processors.
> This patch exports that function so that it may serve the same purpose in
> other modules that consume LocalApicLib.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
>  UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++--------
> ---
>  .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++----------
> -
>  3 files changed, 63 insertions(+), 46 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h
> b/UefiCpuPkg/Include/Library/LocalApicLib.h
> index 96b93aa..a6e9dc6 100644
> --- a/UefiCpuPkg/Include/Library/LocalApicLib.h
> +++ b/UefiCpuPkg/Include/Library/LocalApicLib.h
> @@ -5,6 +5,8 @@
>    handles cases where local APIC is disabled.
> 
>    Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> +
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -16,6 +18,19 @@
>  #define LOCAL_APIC_MODE_X2APIC  0x2  ///< x2APIC mode.
> 
>  /**
> +  Determine if the standard CPU signature is "AuthenticAMD".
> +
> +  @retval TRUE  The CPU signature matches.
> +  @retval FALSE The CPU signature does not match.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +StandardSignatureIsAuthenticAMD (
> +  VOID
> +  );
> +
> +/**
>    Retrieve the base address of local APIC.
> 
>    @return The base address of local APIC.
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index 33ea15c..cebf1b3 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -4,7 +4,7 @@
>    This local APIC library instance supports xAPIC mode only.
> 
>    Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
> -  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -27,28 +27,6 @@
>  //
> 
>  /**
> -  Determine if the standard CPU signature is "AuthenticAMD".
> -
> -  @retval TRUE  The CPU signature matches.
> -  @retval FALSE The CPU signature does not match.
> -
> -**/
> -BOOLEAN
> -StandardSignatureIsAuthenticAMD (
> -  VOID
> -  )
> -{
> -  UINT32  RegEbx;
> -  UINT32  RegEcx;
> -  UINT32  RegEdx;
> -
> -  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> -  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> -          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> -          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> -}
> -
> -/**
>    Determine if the CPU supports the Local APIC Base Address MSR.
> 
>    @retval TRUE  The CPU supports the Local APIC Base Address MSR.
> @@ -76,6 +54,29 @@ LocalApicBaseAddressMsrSupported (  }
> 
>  /**
> +  Determine if the standard CPU signature is "AuthenticAMD".
> +
> +  @retval TRUE  The CPU signature matches.
> +  @retval FALSE The CPU signature does not match.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +StandardSignatureIsAuthenticAMD (
> +  VOID
> +  )
> +{
> +  UINT32  RegEbx;
> +  UINT32  RegEcx;
> +  UINT32  RegEdx;
> +
> +  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> +  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> +          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> +          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> +}
> +
> +/**
>    Retrieve the base address of local APIC.
> 
>    @return The base address of local APIC.
> diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> index d0f92b3..01996b1 100644
> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> @@ -5,7 +5,7 @@
>    which have xAPIC and x2APIC modes.
> 
>    Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
> -  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -28,28 +28,6 @@
>  //
> 
>  /**
> -  Determine if the standard CPU signature is "AuthenticAMD".
> -
> -  @retval TRUE  The CPU signature matches.
> -  @retval FALSE The CPU signature does not match.
> -
> -**/
> -BOOLEAN
> -StandardSignatureIsAuthenticAMD (
> -  VOID
> -  )
> -{
> -  UINT32  RegEbx;
> -  UINT32  RegEcx;
> -  UINT32  RegEdx;
> -
> -  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> -  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> -          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> -          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> -}
> -
> -/**
>    Determine if the CPU supports the Local APIC Base Address MSR.
> 
>    @retval TRUE  The CPU supports the Local APIC Base Address MSR.
> @@ -77,6 +55,29 @@ LocalApicBaseAddressMsrSupported (  }
> 
>  /**
> +  Determine if the standard CPU signature is "AuthenticAMD".
> +
> +  @retval TRUE  The CPU signature matches.
> +  @retval FALSE The CPU signature does not match.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +StandardSignatureIsAuthenticAMD (
> +  VOID
> +  )
> +{
> +  UINT32  RegEbx;
> +  UINT32  RegEcx;
> +  UINT32  RegEdx;
> +
> +  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> +  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> +          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> +          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> +}
> +
> +/**
>    Retrieve the base address of local APIC.
> 
>    @return The base address of local APIC.
> --
> 2.7.4

Re: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Dong, Eric]

Hi Leo,

Yes, I means you also change the cod position in the c file, so in the patch file, it seems like it has other changes. 
My recommendation is to refine the patch to not change the code postion.

-----Original Message-----
From: Duran, Leo <leo.duran@amd.com> 
Sent: Wednesday, February 26, 2020 10:41 AM
To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

Hi Eric,

I added EFIAPI as the function is now intended to be called externally, as a member of "LocalApicLib".
And to that end I added the function prototype in UefiCpuPkg/Include/Library/LocalApicLib,h.

But perhaps I've misunderstood your question?

Thanks,
Leo.


> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Tuesday, February 25, 2020 8:13 PM
> To: Duran, Leo <leo.duran@amd.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export 
> StandardSignatureIsAuthenticAMD function
> 
> Hi Leo,
> 
> I check the code and find the real change for the C files are add 
> "EFIAPI" in the code.  Can you help to refine the change and only keep the real changes?
> 
> Thanks,
> Eric
> 
> -----Original Message-----
> From: Leo Duran <leo.duran@amd.com>
> Sent: Wednesday, February 26, 2020 3:39 AM
> To: devel@edk2.groups.io
> Cc: Leo Duran <leo.duran@amd.com>; Dong, Eric <eric.dong@intel.com>; 
> Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export 
> StandardSignatureIsAuthenticAMD function
> 
> REF:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> ill a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> duran%40amd.com%7C19e3e4c20ffa41d8de8508d7ba591944%7C3dd8961f
> e4884e608e11a82d994e183d%7C0%7C0%7C637182764151879166&amp;s
> data=n%2B3eeGu7%2BvmVCz2QP6aeJuiqQ08gX5ZaCAuDkNMO%2Bb8%3D&a
> mp;reserved=0
> 
> The StandardSignatureIsAuthenticAMD function was introduced locally to 
> help divert code paths pertinent (or not) to AMD processors.
> This patch exports that function so that it may serve the same purpose 
> in other modules that consume LocalApicLib.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
>  UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++--------
> ---
>  .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++----------
> -
>  3 files changed, 63 insertions(+), 46 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h
> b/UefiCpuPkg/Include/Library/LocalApicLib.h
> index 96b93aa..a6e9dc6 100644
> --- a/UefiCpuPkg/Include/Library/LocalApicLib.h
> +++ b/UefiCpuPkg/Include/Library/LocalApicLib.h
> @@ -5,6 +5,8 @@
>    handles cases where local APIC is disabled.
> 
>    Copyright (c) 2010 - 2019, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> +
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -16,6 +18,19 @@
>  #define LOCAL_APIC_MODE_X2APIC  0x2  ///< x2APIC mode.
> 
>  /**
> +  Determine if the standard CPU signature is "AuthenticAMD".
> +
> +  @retval TRUE  The CPU signature matches.
> +  @retval FALSE The CPU signature does not match.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +StandardSignatureIsAuthenticAMD (
> +  VOID
> +  );
> +
> +/**
>    Retrieve the base address of local APIC.
> 
>    @return The base address of local APIC.
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index 33ea15c..cebf1b3 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -4,7 +4,7 @@
>    This local APIC library instance supports xAPIC mode only.
> 
>    Copyright (c) 2010 - 2019, Intel Corporation. All rights 
> reserved.<BR>
> -  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -27,28 +27,6 @@
>  //
> 
>  /**
> -  Determine if the standard CPU signature is "AuthenticAMD".
> -
> -  @retval TRUE  The CPU signature matches.
> -  @retval FALSE The CPU signature does not match.
> -
> -**/
> -BOOLEAN
> -StandardSignatureIsAuthenticAMD (
> -  VOID
> -  )
> -{
> -  UINT32  RegEbx;
> -  UINT32  RegEcx;
> -  UINT32  RegEdx;
> -
> -  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> -  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> -          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> -          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> -}
> -
> -/**
>    Determine if the CPU supports the Local APIC Base Address MSR.
> 
>    @retval TRUE  The CPU supports the Local APIC Base Address MSR.
> @@ -76,6 +54,29 @@ LocalApicBaseAddressMsrSupported (  }
> 
>  /**
> +  Determine if the standard CPU signature is "AuthenticAMD".
> +
> +  @retval TRUE  The CPU signature matches.
> +  @retval FALSE The CPU signature does not match.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +StandardSignatureIsAuthenticAMD (
> +  VOID
> +  )
> +{
> +  UINT32  RegEbx;
> +  UINT32  RegEcx;
> +  UINT32  RegEdx;
> +
> +  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> +  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> +          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> +          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> +}
> +
> +/**
>    Retrieve the base address of local APIC.
> 
>    @return The base address of local APIC.
> diff --git 
> a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> index d0f92b3..01996b1 100644
> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> @@ -5,7 +5,7 @@
>    which have xAPIC and x2APIC modes.
> 
>    Copyright (c) 2010 - 2019, Intel Corporation. All rights 
> reserved.<BR>
> -  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -28,28 +28,6 @@
>  //
> 
>  /**
> -  Determine if the standard CPU signature is "AuthenticAMD".
> -
> -  @retval TRUE  The CPU signature matches.
> -  @retval FALSE The CPU signature does not match.
> -
> -**/
> -BOOLEAN
> -StandardSignatureIsAuthenticAMD (
> -  VOID
> -  )
> -{
> -  UINT32  RegEbx;
> -  UINT32  RegEcx;
> -  UINT32  RegEdx;
> -
> -  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> -  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> -          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> -          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> -}
> -
> -/**
>    Determine if the CPU supports the Local APIC Base Address MSR.
> 
>    @retval TRUE  The CPU supports the Local APIC Base Address MSR.
> @@ -77,6 +55,29 @@ LocalApicBaseAddressMsrSupported (  }
> 
>  /**
> +  Determine if the standard CPU signature is "AuthenticAMD".
> +
> +  @retval TRUE  The CPU signature matches.
> +  @retval FALSE The CPU signature does not match.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +StandardSignatureIsAuthenticAMD (
> +  VOID
> +  )
> +{
> +  UINT32  RegEbx;
> +  UINT32  RegEcx;
> +  UINT32  RegEdx;
> +
> +  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> +  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> +          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> +          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> +}
> +
> +/**
>    Retrieve the base address of local APIC.
> 
>    @return The base address of local APIC.
> --
> 2.7.4

Re: [PATCH 2/2] UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.

[Ni, Ray]

+ Hao Wu and Siyuan Fu for review.

> -----Original Message-----
> From: Leo Duran <leo.duran@amd.com>
> Sent: Wednesday, February 26, 2020 3:39 AM
> To: devel@edk2.groups.io
> Cc: Leo Duran <leo.duran@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH 2/2] UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
> 
> This patch uses the newly exported StandardSignatureIsAuthenticAMD function
> from LocalApicLib, to divert code paths not pertinent to AMD processors.
> Specifically, the PlatformId MSR and embedded Microcode patches are not
> relevant on AMD-based platforms.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 17 +++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.c     | 11 +++++++++--
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 1562959..750681d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -2,6 +2,8 @@
>    Implementation of loading microcode on processors.
> 
>    Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> +
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -97,9 +99,13 @@ MicrocodeDetect (
>    UINT32                                  ThreadId;
>    BOOLEAN                                 IsBspCallIn;
> 
> -  if (CpuMpData->MicrocodePatchRegionSize == 0) {
> +  //
> +  // NOTE: Embedded Microcode patches are not relevant on AMD platforms.
> +  //
> +  if (CpuMpData->MicrocodePatchRegionSize == 0 ||
> +      StandardSignatureIsAuthenticAMD ()) {
>      //
> -    // There is no microcode patches
> +    // There are no microcode patches
>      //
>      return;
>    }
> @@ -350,6 +356,13 @@ IsProcessorMatchedMicrocodePatch (
>    UINTN          Index;
>    CPU_AP_DATA    *CpuData;
> 
> +  //
> +  // NOTE: PlatformId or embedded Microcode patches are not relevant on AMD platforms.
> +  //
> +  if (StandardSignatureIsAuthenticAMD ()) {
> +    return FALSE;
> +  }
> +
>    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>      CpuData = &CpuMpData->CpuData[Index];
>      if ((ProcessorSignature == CpuData->ProcessorSignature) &&
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d0fbc17..290e7bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2,6 +2,8 @@
>    CPU MP Initialize Library common functions.
> 
>    Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> +
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -564,8 +566,13 @@ InitializeApData (
>    CpuMpData->CpuData[ProcessorNumber].Waiting    = FALSE;
>    CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE;
> 
> -  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> -  CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> +  //
> +  // NOTE: PlatformId is not relevant on AMD platforms.
> +  //
> +  if (!StandardSignatureIsAuthenticAMD ()) {
> +    PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> +    CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)PlatformIdMsr.Bits.PlatformId;
> +  }
> 
>    AsmCpuid (
>      CPUID_VERSION_INFO,
> --
> 2.7.4

Re: [PATCH 2/2] UefiCpuPkg: MpInitLib: Exclude code no pertinent to AMD processors.

[Siyuan, Fu]

Acked-by: Siyuan Fu <siyuan.fu@intel.com>

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: 2020年2月26日 15:46
> To: Leo Duran <leo.duran@amd.com>; devel@edk2.groups.io; Wu, Hao A
> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH 2/2] UefiCpuPkg: MpInitLib: Exclude code no pertinent
> to AMD processors.
> 
> + Hao Wu and Siyuan Fu for review.
> 
> > -----Original Message-----
> > From: Leo Duran <leo.duran@amd.com>
> > Sent: Wednesday, February 26, 2020 3:39 AM
> > To: devel@edk2.groups.io
> > Cc: Leo Duran <leo.duran@amd.com>; Dong, Eric <eric.dong@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: [PATCH 2/2] UefiCpuPkg: MpInitLib: Exclude code no pertinent to
> AMD processors.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
> >
> > This patch uses the newly exported StandardSignatureIsAuthenticAMD
> function
> > from LocalApicLib, to divert code paths not pertinent to AMD processors.
> > Specifically, the PlatformId MSR and embedded Microcode patches are not
> > relevant on AMD-based platforms.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/Microcode.c | 17 +++++++++++++++--
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c     | 11 +++++++++--
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index 1562959..750681d 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -2,6 +2,8 @@
> >    Implementation of loading microcode on processors.
> >
> >    Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> > +
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -97,9 +99,13 @@ MicrocodeDetect (
> >    UINT32                                  ThreadId;
> >    BOOLEAN                                 IsBspCallIn;
> >
> > -  if (CpuMpData->MicrocodePatchRegionSize == 0) {
> > +  //
> > +  // NOTE: Embedded Microcode patches are not relevant on AMD
> platforms.
> > +  //
> > +  if (CpuMpData->MicrocodePatchRegionSize == 0 ||
> > +      StandardSignatureIsAuthenticAMD ()) {
> >      //
> > -    // There is no microcode patches
> > +    // There are no microcode patches
> >      //
> >      return;
> >    }
> > @@ -350,6 +356,13 @@ IsProcessorMatchedMicrocodePatch (
> >    UINTN          Index;
> >    CPU_AP_DATA    *CpuData;
> >
> > +  //
> > +  // NOTE: PlatformId or embedded Microcode patches are not relevant on
> AMD platforms.
> > +  //
> > +  if (StandardSignatureIsAuthenticAMD ()) {
> > +    return FALSE;
> > +  }
> > +
> >    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> >      CpuData = &CpuMpData->CpuData[Index];
> >      if ((ProcessorSignature == CpuData->ProcessorSignature) &&
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index d0fbc17..290e7bf 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -2,6 +2,8 @@
> >    CPU MP Initialize Library common functions.
> >
> >    Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> > +
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -564,8 +566,13 @@ InitializeApData (
> >    CpuMpData->CpuData[ProcessorNumber].Waiting    = FALSE;
> >    CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ?
> TRUE : FALSE;
> >
> > -  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > -  CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)
> PlatformIdMsr.Bits.PlatformId;
> > +  //
> > +  // NOTE: PlatformId is not relevant on AMD platforms.
> > +  //
> > +  if (!StandardSignatureIsAuthenticAMD ()) {
> > +    PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > +    CpuMpData->CpuData[ProcessorNumber].PlatformId =
> (UINT8)PlatformIdMsr.Bits.PlatformId;
> > +  }
> >
> >    AsmCpuid (
> >      CPUID_VERSION_INFO,
> > --
> > 2.7.4

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Ni, Ray]

Leo,

> > BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(),
> > but it never affected AMD-based platforms as the flow never gets that far, since
> > the Detect routine bails out early when it finds the size of the patch is zero.

You are saying that PlatformId MSR access is not performed by CPU in old code because of the zero size uCode.
But now with Hao or Siyuan's change, the PlatformId MSR access is always performed even when there is no uCode. It sounds like a regression to optimization to me.
Did you evaluate the path to avoid accessing PlatformID MSR when uCode doesn't exist? So that the API to detect AMD processor is not needed at all.

Thanks,
Ray

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Liming Gao]

Leo:
  Now, we enter into Hard Feature Freeze phase until edk2-stable202002 tag is created at 2020-02-28. Only critical bug fix is still allowed.

  Do you request to catch this fix into this stable tag? If yes, please work closely with UefiCpuPkg maintainers to catch the stable tag release schedule 2020-02-28.

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Wednesday, February 26, 2020 3:57 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; leo.duran@amd.com; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib
> 
> Leo,
> 
> > > BTW, reading the PlatformId MSR was already being done by MicrocodeDetect(),
> > > but it never affected AMD-based platforms as the flow never gets that far, since
> > > the Detect routine bails out early when it finds the size of the patch is zero.
> 
> You are saying that PlatformId MSR access is not performed by CPU in old code because of the zero size uCode.
> But now with Hao or Siyuan's change, the PlatformId MSR access is always performed even when there is no uCode. It sounds like a
> regression to optimization to me.
> Did you evaluate the path to avoid accessing PlatformID MSR when uCode doesn't exist? So that the API to detect AMD processor is not
> needed at all.
> 
> Thanks,
> Ray
> 
> 

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Laszlo Ersek]

On 02/26/20 06:05, Dong, Eric wrote:
> Hi Leo,
> 
> Yes, I means you also change the cod position in the c file, so in the patch file, it seems like it has other changes. 
> My recommendation is to refine the patch to not change the code postion.

Indeed I noticed that too. I figured this change -- moving the code
around, beyond purely making it public -- was intentional. I assumed the
new position of the function within the source code made more sense to Leo.

I agree the code movement should be explained at least in the commit
message.

Thanks
Laszlo

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Duran, Leo]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 26, 2020 5:13 AM
> To: devel@edk2.groups.io; eric.dong@intel.com; Duran, Leo
> <leo.duran@amd.com>
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> StandardSignatureIsAuthenticAMD function
> 
> On 02/26/20 06:05, Dong, Eric wrote:
> > Hi Leo,
> >
> > Yes, I means you also change the cod position in the c file, so in the patch
> file, it seems like it has other changes.
> > My recommendation is to refine the patch to not change the code postion.
> 
> Indeed I noticed that too. I figured this change -- moving the code around,
> beyond purely making it public -- was intentional. I assumed the new position
> of the function within the source code made more sense to Leo.
[Duran, Leo] You're exactly right, Laszlo, let me explain:

The function was moved down because the top spots are reserved for internal functions, according to this comment in the source::
//
// Library internal functions
//

In this case, there were two internal (local) functions at the top of the file:
BOOLEAN
StandardSignatureIsAuthenticAMD (
  VOID
  )
{
}

BOOLEAN
LocalApicBaseAddressMsrSupported (
  VOID
  )
{
}

So I moved them like this:
BOOLEAN
LocalApicBaseAddressMsrSupported (
  VOID
  )
{
}

BOOLEAN
EFIAPI
StandardSignatureIsAuthenticAMD (
  VOID
  )
{
}

Other functions that follow are external functions, using the EFIAPI prefix.
Leo.

> 
> I agree the code movement should be explained at least in the commit
> message.
> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Wednesday, February 26, 2020 3:56 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> Leo:
>   Now, we enter into Hard Feature Freeze phase until edk2-stable202002 tag
> is created at 2020-02-28. Only critical bug fix is still allowed.
> 
>   Do you request to catch this fix into this stable tag? If yes, please work closely
> with UefiCpuPkg maintainers to catch the stable tag release schedule 2020-
> 02-28.
> 
> Thanks
> Liming
[Duran, Leo] 
I'm not sure I understand what my next steps are, perhaps I need to go back to the BUGZILLA?
(This issue is a "must-fix", so perhaps I can tag it as such in the BUG report.).

Thanks,
Leo.

> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> > Sent: Wednesday, February 26, 2020 3:57 PM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io;
> > leo.duran@amd.com; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > Leo,
> >
> > > > BTW, reading the PlatformId MSR was already being done by
> > > > MicrocodeDetect(), but it never affected AMD-based platforms as
> > > > the flow never gets that far, since the Detect routine bails out early
> when it finds the size of the patch is zero.
> >
> > You are saying that PlatformId MSR access is not performed by CPU in old
> code because of the zero size uCode.
> > But now with Hao or Siyuan's change, the PlatformId MSR access is
> > always performed even when there is no uCode. It sounds like a regression
> to optimization to me.
> > Did you evaluate the path to avoid accessing PlatformID MSR when uCode
> > doesn't exist? So that the API to detect AMD processor is not needed at all.
> >
> > Thanks,
> > Ray
> >
> > 

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni@intel.com]
> Sent: Wednesday, February 26, 2020 2:57 AM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Duran, Leo
> <leo.duran@amd.com>; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> Leo,
> 
> > > BTW, reading the PlatformId MSR was already being done by
> > > MicrocodeDetect(), but it never affected AMD-based platforms as the
> > > flow never gets that far, since the Detect routine bails out early when it
> finds the size of the patch is zero.
> 
> You are saying that PlatformId MSR access is not performed by CPU in old
> code because of the zero size uCode.
> But now with Hao or Siyuan's change, the PlatformId MSR access is always
> performed even when there is no uCode. It sounds like a regression to
> optimization to me.
> Did you evaluate the path to avoid accessing PlatformID MSR when uCode
> doesn't exist? So that the API to detect AMD processor is not needed at all.
[Duran, Leo] 
Hi Ray,
I think your summary is pretty accurate, except that I'd say that avoiding a READ from the PlatformId MSR
should happen solely based on the fact that the MSR simply does not exist on AMD processors.
Then as a result of that,  the usage of the PlatformId (as it relates to microcode or anything else) must then be dealt with separately.

To that end, I think I covered all cases where the MSR is being read, and also where PlatformId is being used.
(I also added comments for each case)

Thanks,
Leo.

> 
> Thanks,
> Ray

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

BTW,

I also considered adding a flag to CPU_MP_DATA to make the usage of PlatformId a bit more explicit.
E.g., something like CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId... So the init code would look like this:

  //
  // NOTE: PlatformId is not relevant on AMD platforms.
  //
  if (StandardSignatureIsAuthenticAMD ()) {
    CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
  else {
    PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
    CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)PlatformIdMsr.Bits.PlatformId;
    CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
  }

This way "IsValidPlatformId" could be checked prior to using "PlatformId".
Anyway, that seemed a bit overkill, so I opted against it... thoughts?

Leo.


> -----Original Message-----
> From: Duran, Leo
> Sent: Wednesday, February 26, 2020 10:25 AM
> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> 
> 
> > -----Original Message-----
> > From: Ni, Ray [mailto:ray.ni@intel.com]
> > Sent: Wednesday, February 26, 2020 2:57 AM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Duran, Leo
> > <leo.duran@amd.com>; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > Leo,
> >
> > > > BTW, reading the PlatformId MSR was already being done by
> > > > MicrocodeDetect(), but it never affected AMD-based platforms as
> > > > the flow never gets that far, since the Detect routine bails out
> > > > early when it
> > finds the size of the patch is zero.
> >
> > You are saying that PlatformId MSR access is not performed by CPU in
> > old code because of the zero size uCode.
> > But now with Hao or Siyuan's change, the PlatformId MSR access is
> > always performed even when there is no uCode. It sounds like a
> > regression to optimization to me.
> > Did you evaluate the path to avoid accessing PlatformID MSR when uCode
> > doesn't exist? So that the API to detect AMD processor is not needed at all.
> [Duran, Leo]
> Hi Ray,
> I think your summary is pretty accurate, except that I'd say that avoiding a
> READ from the PlatformId MSR should happen solely based on the fact that
> the MSR simply does not exist on AMD processors.
> Then as a result of that,  the usage of the PlatformId (as it relates to microcode
> or anything else) must then be dealt with separately.
> 
> To that end, I think I covered all cases where the MSR is being read, and also
> where PlatformId is being used.
> (I also added comments for each case)
> 
> Thanks,
> Leo.
> 
> >
> > Thanks,
> > Ray

Re: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Duran, Leo]

> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Wednesday, February 26, 2020 12:06 AM
> To: Duran, Leo <leo.duran@amd.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> StandardSignatureIsAuthenticAMD function
> 
> Hi Leo,
> 
> Yes, I means you also change the cod position in the c file, so in the patch file,
> it seems like it has other changes.
> My recommendation is to refine the patch to not change the code postion.
[Duran, Leo] 
Hi Eric,
Apologies for my replies being out of order.

Actually, changing code position was done on purpose! :-).
For details, please refer to my reply Laszlo on the same topic.

Thanks,
Leo.

> 
> -----Original Message-----
> From: Duran, Leo <leo.duran@amd.com>
> Sent: Wednesday, February 26, 2020 10:41 AM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> StandardSignatureIsAuthenticAMD function
> 
> Hi Eric,
> 
> I added EFIAPI as the function is now intended to be called externally, as a
> member of "LocalApicLib".
> And to that end I added the function prototype in
> UefiCpuPkg/Include/Library/LocalApicLib,h.
> 
> But perhaps I've misunderstood your question?
> 
> Thanks,
> Leo.
> 
> 
> > -----Original Message-----
> > From: Dong, Eric [mailto:eric.dong@intel.com]
> > Sent: Tuesday, February 25, 2020 8:13 PM
> > To: Duran, Leo <leo.duran@amd.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: RE: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> > StandardSignatureIsAuthenticAMD function
> >
> > Hi Leo,
> >
> > I check the code and find the real change for the C files are add
> > "EFIAPI" in the code.  Can you help to refine the change and only keep the
> real changes?
> >
> > Thanks,
> > Eric
> >
> > -----Original Message-----
> > From: Leo Duran <leo.duran@amd.com>
> > Sent: Wednesday, February 26, 2020 3:39 AM
> > To: devel@edk2.groups.io
> > Cc: Leo Duran <leo.duran@amd.com>; Dong, Eric <eric.dong@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
> > StandardSignatureIsAuthenticAMD function
> >
> > REF:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> > ill
> a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> >
> duran%40amd.com%7C19e3e4c20ffa41d8de8508d7ba591944%7C3dd8961f
> >
> e4884e608e11a82d994e183d%7C0%7C0%7C637182764151879166&amp;s
> >
> data=n%2B3eeGu7%2BvmVCz2QP6aeJuiqQ08gX5ZaCAuDkNMO%2Bb8%3D&a
> > mp;reserved=0
> >
> > The StandardSignatureIsAuthenticAMD function was introduced locally to
> > help divert code paths pertinent (or not) to AMD processors.
> > This patch exports that function so that it may serve the same purpose
> > in other modules that consume LocalApicLib.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > ---
> >  UefiCpuPkg/Include/Library/LocalApicLib.h          | 15 +++++++
> >  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c     | 47 +++++++++++-----
> ---
> > ---
> >  .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c        | 47 +++++++++++--------
> --
> > -
> >  3 files changed, 63 insertions(+), 46 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h
> > b/UefiCpuPkg/Include/Library/LocalApicLib.h
> > index 96b93aa..a6e9dc6 100644
> > --- a/UefiCpuPkg/Include/Library/LocalApicLib.h
> > +++ b/UefiCpuPkg/Include/Library/LocalApicLib.h
> > @@ -5,6 +5,8 @@
> >    handles cases where local APIC is disabled.
> >
> >    Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> > +  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
> > +
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -16,6 +18,19 @@
> >  #define LOCAL_APIC_MODE_X2APIC  0x2  ///< x2APIC mode.
> >
> >  /**
> > +  Determine if the standard CPU signature is "AuthenticAMD".
> > +
> > +  @retval TRUE  The CPU signature matches.
> > +  @retval FALSE The CPU signature does not match.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +StandardSignatureIsAuthenticAMD (
> > +  VOID
> > +  );
> > +
> > +/**
> >    Retrieve the base address of local APIC.
> >
> >    @return The base address of local APIC.
> > diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > index 33ea15c..cebf1b3 100644
> > --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > @@ -4,7 +4,7 @@
> >    This local APIC library instance supports xAPIC mode only.
> >
> >    Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> > -  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> > +  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -27,28 +27,6 @@
> >  //
> >
> >  /**
> > -  Determine if the standard CPU signature is "AuthenticAMD".
> > -
> > -  @retval TRUE  The CPU signature matches.
> > -  @retval FALSE The CPU signature does not match.
> > -
> > -**/
> > -BOOLEAN
> > -StandardSignatureIsAuthenticAMD (
> > -  VOID
> > -  )
> > -{
> > -  UINT32  RegEbx;
> > -  UINT32  RegEcx;
> > -  UINT32  RegEdx;
> > -
> > -  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> > -  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> > -          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> > -          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> > -}
> > -
> > -/**
> >    Determine if the CPU supports the Local APIC Base Address MSR.
> >
> >    @retval TRUE  The CPU supports the Local APIC Base Address MSR.
> > @@ -76,6 +54,29 @@ LocalApicBaseAddressMsrSupported (  }
> >
> >  /**
> > +  Determine if the standard CPU signature is "AuthenticAMD".
> > +
> > +  @retval TRUE  The CPU signature matches.
> > +  @retval FALSE The CPU signature does not match.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +StandardSignatureIsAuthenticAMD (
> > +  VOID
> > +  )
> > +{
> > +  UINT32  RegEbx;
> > +  UINT32  RegEcx;
> > +  UINT32  RegEdx;
> > +
> > +  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> > +  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> > +          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> > +          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> > +}
> > +
> > +/**
> >    Retrieve the base address of local APIC.
> >
> >    @return The base address of local APIC.
> > diff --git
> > a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > index d0f92b3..01996b1 100644
> > --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > @@ -5,7 +5,7 @@
> >    which have xAPIC and x2APIC modes.
> >
> >    Copyright (c) 2010 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> > -  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> > +  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -28,28 +28,6 @@
> >  //
> >
> >  /**
> > -  Determine if the standard CPU signature is "AuthenticAMD".
> > -
> > -  @retval TRUE  The CPU signature matches.
> > -  @retval FALSE The CPU signature does not match.
> > -
> > -**/
> > -BOOLEAN
> > -StandardSignatureIsAuthenticAMD (
> > -  VOID
> > -  )
> > -{
> > -  UINT32  RegEbx;
> > -  UINT32  RegEcx;
> > -  UINT32  RegEdx;
> > -
> > -  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> > -  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> > -          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> > -          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> > -}
> > -
> > -/**
> >    Determine if the CPU supports the Local APIC Base Address MSR.
> >
> >    @retval TRUE  The CPU supports the Local APIC Base Address MSR.
> > @@ -77,6 +55,29 @@ LocalApicBaseAddressMsrSupported (  }
> >
> >  /**
> > +  Determine if the standard CPU signature is "AuthenticAMD".
> > +
> > +  @retval TRUE  The CPU signature matches.
> > +  @retval FALSE The CPU signature does not match.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +StandardSignatureIsAuthenticAMD (
> > +  VOID
> > +  )
> > +{
> > +  UINT32  RegEbx;
> > +  UINT32  RegEcx;
> > +  UINT32  RegEdx;
> > +
> > +  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
> > +  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
> > +          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
> > +          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
> > +}
> > +
> > +/**
> >    Retrieve the base address of local APIC.
> >
> >    @return The base address of local APIC.
> > --
> > 2.7.4

Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export StandardSignatureIsAuthenticAMD function

[Laszlo Ersek]

On 02/26/20 16:03, Duran, Leo wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, February 26, 2020 5:13 AM
>> To: devel@edk2.groups.io; eric.dong@intel.com; Duran, Leo
>> <leo.duran@amd.com>
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: LocalApicLib: Export
>> StandardSignatureIsAuthenticAMD function
>>
>> On 02/26/20 06:05, Dong, Eric wrote:
>>> Hi Leo,
>>>
>>> Yes, I means you also change the cod position in the c file, so in the patch
>> file, it seems like it has other changes.
>>> My recommendation is to refine the patch to not change the code postion.
>>
>> Indeed I noticed that too. I figured this change -- moving the code around,
>> beyond purely making it public -- was intentional. I assumed the new position
>> of the function within the source code made more sense to Leo.
> [Duran, Leo] You're exactly right, Laszlo, let me explain:
> 
> The function was moved down because the top spots are reserved for internal functions, according to this comment in the source::
> //
> // Library internal functions
> //
> 
> In this case, there were two internal (local) functions at the top of the file:
> BOOLEAN
> StandardSignatureIsAuthenticAMD (
>   VOID
>   )
> {
> }
> 
> BOOLEAN
> LocalApicBaseAddressMsrSupported (
>   VOID
>   )
> {
> }
> 
> So I moved them like this:
> BOOLEAN
> LocalApicBaseAddressMsrSupported (
>   VOID
>   )
> {
> }
> 
> BOOLEAN
> EFIAPI
> StandardSignatureIsAuthenticAMD (
>   VOID
>   )
> {
> }
> 
> Other functions that follow are external functions, using the EFIAPI prefix.
> Leo.

makes sense, thanks.
Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Laszlo Ersek]

On 02/26/20 16:46, Duran, Leo wrote:
> BTW,
> 
> I also considered adding a flag to CPU_MP_DATA to make the usage of PlatformId a bit more explicit.
> E.g., something like CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId... So the init code would look like this:
> 
>   //
>   // NOTE: PlatformId is not relevant on AMD platforms.
>   //
>   if (StandardSignatureIsAuthenticAMD ()) {
>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
>   else {
>     PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
>     CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)PlatformIdMsr.Bits.PlatformId;
>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
>   }
> 
> This way "IsValidPlatformId" could be checked prior to using "PlatformId".
> Anyway, that seemed a bit overkill, so I opted against it... thoughts?

I think a global flag is justified; in the above approach,
"IsValidPlatformId" would not vary across "ProcessorNumber", so it does
look like useless generality.

Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Laszlo Ersek]

Hi Leo,

On 02/26/20 16:11, Duran, Leo wrote:
> 
> 
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Wednesday, February 26, 2020 3:56 AM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
>> <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, Michael D
>> <michael.d.kinney@intel.com>; afish@apple.com; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
>> MpInitLib
>>
>> Leo:
>>   Now, we enter into Hard Feature Freeze phase until edk2-stable202002 tag
>> is created at 2020-02-28. Only critical bug fix is still allowed.
>>
>>   Do you request to catch this fix into this stable tag? If yes, please work closely
>> with UefiCpuPkg maintainers to catch the stable tag release schedule 2020-
>> 02-28.
>>
>> Thanks
>> Liming
> [Duran, Leo] 
> I'm not sure I understand what my next steps are, perhaps I need to go back to the BUGZILLA?
> (This issue is a "must-fix", so perhaps I can tag it as such in the BUG report.).

the issue will clearly be fixed, the question is: when.

Edk2 is now in hard feature freeze for the upcoming edk2-stable202002
tag. See the schedule and the definitions here:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning

The question is whether you want this fix to be part of the
edk2-stable202002 tag. If not, then there's no rush, the patch series
(the final, reviewed version) will be merged after the tag (after
2020-02-28).

If you do want to get the fix into edk2-stable202002, then:

- you need to demonstrate that the patches implement an important bugfix
(not a feature addition) -- it helps if you describe the problem
symptoms in this case,

- and you and Eric & Ray need to collaborate in a hurry to get the
patches refreshed as necessary, reviewed, and merged.


Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 26, 2020 11:25 AM
> To: Duran, Leo <leo.duran@amd.com>; Gao, Liming <liming.gao@intel.com>;
> devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com
> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> Hi Leo,
> 
> On 02/26/20 16:11, Duran, Leo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Gao, Liming [mailto:liming.gao@intel.com]
> >> Sent: Wednesday, February 26, 2020 3:56 AM
> >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> >> <lersek@redhat.com>; Duran, Leo <leo.duran@amd.com>; Wu, Hao A
> >> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> >> Cc: Dong, Eric <eric.dong@intel.com>; leif@nuviainc.com; Kinney,
> >> Michael D <michael.d.kinney@intel.com>; afish@apple.com; Gao, Liming
> >> <liming.gao@intel.com>
> >> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> >> MpInitLib
> >>
> >> Leo:
> >>   Now, we enter into Hard Feature Freeze phase until
> >> edk2-stable202002 tag is created at 2020-02-28. Only critical bug fix is still
> allowed.
> >>
> >>   Do you request to catch this fix into this stable tag? If yes,
> >> please work closely with UefiCpuPkg maintainers to catch the stable
> >> tag release schedule 2020- 02-28.
> >>
> >> Thanks
> >> Liming
> > [Duran, Leo]
> > I'm not sure I understand what my next steps are, perhaps I need to go back
> to the BUGZILLA?
> > (This issue is a "must-fix", so perhaps I can tag it as such in the BUG report.).
> 
> the issue will clearly be fixed, the question is: when.
> 
> Edk2 is now in hard feature freeze for the upcoming edk2-stable202002 tag.
> See the schedule and the definitions here:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-
> Planning&amp;data=02%7C01%7Cleo.duran%40amd.com%7Cdec7fb5d02784
> 01bcf1008d7bad87003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7
> C0%7C637183311078914692&amp;sdata=R%2FBNmDgmA1m7rRyK%2F2DSn
> Os3Cw8W3ZgPVZoZe3QPNJg%3D&amp;reserved=0
> 
> The question is whether you want this fix to be part of the
> edk2-stable202002 tag. If not, then there's no rush, the patch series (the final,
> reviewed version) will be merged after the tag (after 2020-02-28).
> 
> If you do want to get the fix into edk2-stable202002, then:
> 
> - you need to demonstrate that the patches implement an important bugfix
> (not a feature addition) -- it helps if you describe the problem symptoms in
> this case,
> 
> - and you and Eric & Ray need to collaborate in a hurry to get the patches
> refreshed as necessary, reviewed, and merged.
> 
[Duran, Leo] 
It sounds like either way the merge would happen reasonably soon.
So although it's "bug-fix" as opposed to a "feature-add", I'll defer to the maintainers for an optimal merge plan.
And I suppose folks with a different opinion could possibly chime in on the opened ticket.

Thanks,
Leo.


> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 26, 2020 11:21 AM
> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> On 02/26/20 16:46, Duran, Leo wrote:
> > BTW,
> >
> > I also considered adding a flag to CPU_MP_DATA to make the usage of
> PlatformId a bit more explicit.
> > E.g., something like CpuMpData-
> >CpuData[ProcessorNumber].IsValidPlatformId... So the init code would look
> like this:
> >
> >   //
> >   // NOTE: PlatformId is not relevant on AMD platforms.
> >   //
> >   if (StandardSignatureIsAuthenticAMD ()) {
> >     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
> >   else {
> >     PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> >     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> (UINT8)PlatformIdMsr.Bits.PlatformId;
> >     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
> >   }
> >
> > This way "IsValidPlatformId" could be checked prior to using "PlatformId".
> > Anyway, that seemed a bit overkill, so I opted against it... thoughts?
> 
> I think a global flag is justified; in the above approach, "IsValidPlatformId"
> would not vary across "ProcessorNumber", so it does look like useless
> generality.
[Duran, Leo] 
Great point, Laszlo.
Indeed, global makes senses in the case!
I can prepare a v2-set to incorporate that.

Thanks,
Leo
 
> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

Laszlo, et al,

I suppose the same can be said about the actual "PlatformId"... it should be a single/global read, correct?
But I'd prefer not tackling that in this patch-set (I'll defer to someone that may want to take that on as an optimization/clean-up.).

Leo.

> -----Original Message-----
> From: Duran, Leo
> Sent: Wednesday, February 26, 2020 11:40 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> 
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, February 26, 2020 11:21 AM
> > To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > On 02/26/20 16:46, Duran, Leo wrote:
> > > BTW,
> > >
> > > I also considered adding a flag to CPU_MP_DATA to make the usage of
> > PlatformId a bit more explicit.
> > > E.g., something like CpuMpData-
> > >CpuData[ProcessorNumber].IsValidPlatformId... So the init code would
> > >look
> > like this:
> > >> > >   //
> > >   // NOTE: PlatformId is not relevant on AMD platforms.
> > >   //
> > >   if (StandardSignatureIsAuthenticAMD ()) {
> > >     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
> > >   else {
> > >     PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > >     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> > (UINT8)PlatformIdMsr.Bits.PlatformId;
> > >     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
> > >   }
> > >
> > > This way "IsValidPlatformId" could be checked prior to using "PlatformId".
> > > Anyway, that seemed a bit overkill, so I opted against it... thoughts?
> >
> > I think a global flag is justified; in the above approach, "IsValidPlatformId"
> > would not vary across "ProcessorNumber", so it does look like useless
> > generality.
> [Duran, Leo]
> Great point, Laszlo.
> Indeed, global makes senses in the case!
> I can prepare a v2-set to incorporate that.
> 
> Thanks,
> Leo
> 
> >
> > Thanks
> > Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Laszlo Ersek]

On 02/26/20 17:39, Duran, Leo wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, February 26, 2020 11:21 AM
>> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
>> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
>> <siyuan.fu@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
>> MpInitLib
>>
>> On 02/26/20 16:46, Duran, Leo wrote:
>>> BTW,
>>>
>>> I also considered adding a flag to CPU_MP_DATA to make the usage of
>> PlatformId a bit more explicit.
>>> E.g., something like CpuMpData-
>>> CpuData[ProcessorNumber].IsValidPlatformId... So the init code would look
>> like this:
>>>
>>>   //
>>>   // NOTE: PlatformId is not relevant on AMD platforms.
>>>   //
>>>   if (StandardSignatureIsAuthenticAMD ()) {
>>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
>>>   else {
>>>     PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
>>>     CpuMpData->CpuData[ProcessorNumber].PlatformId =
>> (UINT8)PlatformIdMsr.Bits.PlatformId;
>>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
>>>   }
>>>
>>> This way "IsValidPlatformId" could be checked prior to using "PlatformId".
>>> Anyway, that seemed a bit overkill, so I opted against it... thoughts?
>>
>> I think a global flag is justified; in the above approach, "IsValidPlatformId"
>> would not vary across "ProcessorNumber", so it does look like useless
>> generality.
> [Duran, Leo] 
> Great point, Laszlo.
> Indeed, global makes senses in the case!
> I can prepare a v2-set to incorporate that.

No, sorry, that wasn't what I meant. I didn't try to suggest a global
variable. Instead, I meant that a "global check" (conceptually, i.e.
regardless of particular processor number) made sense.

I'm also not particularly *against* a global variable. In other words, I
didn't try to comment on using a global variable *at all*.

Using a global variable might as well work, I just feel that your
current patches are good enough.

Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 26, 2020 12:45 PM
> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> On 02/26/20 17:39, Duran, Leo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Wednesday, February 26, 2020 11:21 AM
> >> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> >> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> >> <siyuan.fu@intel.com>
> >> Cc: Dong, Eric <eric.dong@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> >> MpInitLib
> >>
> >> On 02/26/20 16:46, Duran, Leo wrote:
> >>> BTW,
> >>>
> >>> I also considered adding a flag to CPU_MP_DATA to make the usage of
> >> PlatformId a bit more explicit.
> >>> E.g., something like CpuMpData-
> >>> CpuData[ProcessorNumber].IsValidPlatformId... So the init code would
> >>> look
> >> like this:
> >>>
> >>>   //
> >>>   // NOTE: PlatformId is not relevant on AMD platforms.
> >>>   //
> >>>   if (StandardSignatureIsAuthenticAMD ()) {
> >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
> >>>   else {
> >>>     PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> >>>     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> >> (UINT8)PlatformIdMsr.Bits.PlatformId;
> >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
> >>>   }
> >>>
> >>> This way "IsValidPlatformId" could be checked prior to using "PlatformId".
> >>> Anyway, that seemed a bit overkill, so I opted against it... thoughts?
> >>
> >> I think a global flag is justified; in the above approach, "IsValidPlatformId"
> >> would not vary across "ProcessorNumber", so it does look like useless
> >> generality.
> > [Duran, Leo]
> > Great point, Laszlo.
> > Indeed, global makes senses in the case!
> > I can prepare a v2-set to incorporate that.
> 
> No, sorry, that wasn't what I meant. I didn't try to suggest a global variable.
> Instead, I meant that a "global check" (conceptually, i.e.
> regardless of particular processor number) made sense.
> 
> I'm also not particularly *against* a global variable. In other words, I didn't try
> to comment on using a global variable *at all*.
> 
> Using a global variable might as well work, I just feel that your current patches
> are good enough.
[Duran, Leo] 
Great... I hear you.
Then, I'd prefer not refactoring further at this point.... I hope Ray & Eric agree.

Thanks for your feedback!
Leo.

> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Ni, Ray]

Leo and all,
I replied in https://bugzilla.tianocore.org/show_bug.cgi?id=2556 for a more general question about how uCode is used in AMD processors.

Because this package recently exposed a new interface ShadowMicrocodePpi, I try to involve Leo in the review from AMD uCode's needs.

Thanks,
Ray

> -----Original Message-----
> From: Duran, Leo <leo.duran@amd.com>
> Sent: Thursday, February 27, 2020 1:52 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Wu, Hao A
> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib
> 
> 
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, February 26, 2020 12:45 PM
> > To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > On 02/26/20 17:39, Duran, Leo wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > >> Sent: Wednesday, February 26, 2020 11:21 AM
> > >> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > >> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > >> <siyuan.fu@intel.com>
> > >> Cc: Dong, Eric <eric.dong@intel.com>
> > >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > >> MpInitLib
> > >>
> > >> On 02/26/20 16:46, Duran, Leo wrote:
> > >>> BTW,
> > >>>
> > >>> I also considered adding a flag to CPU_MP_DATA to make the usage of
> > >> PlatformId a bit more explicit.
> > >>> E.g., something like CpuMpData-
> > >>> CpuData[ProcessorNumber].IsValidPlatformId... So the init code would
> > >>> look
> > >> like this:
> > >>>
> > >>>   //
> > >>>   // NOTE: PlatformId is not relevant on AMD platforms.
> > >>>   //
> > >>>   if (StandardSignatureIsAuthenticAMD ()) {
> > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = FALSE;
> > >>>   else {
> > >>>     PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > >>>     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> > >> (UINT8)PlatformIdMsr.Bits.PlatformId;
> > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
> > >>>   }
> > >>>
> > >>> This way "IsValidPlatformId" could be checked prior to using "PlatformId".
> > >>> Anyway, that seemed a bit overkill, so I opted against it... thoughts?
> > >>
> > >> I think a global flag is justified; in the above approach, "IsValidPlatformId"
> > >> would not vary across "ProcessorNumber", so it does look like useless
> > >> generality.
> > > [Duran, Leo]
> > > Great point, Laszlo.
> > > Indeed, global makes senses in the case!
> > > I can prepare a v2-set to incorporate that.
> >
> > No, sorry, that wasn't what I meant. I didn't try to suggest a global variable.
> > Instead, I meant that a "global check" (conceptually, i.e.
> > regardless of particular processor number) made sense.
> >
> > I'm also not particularly *against* a global variable. In other words, I didn't try
> > to comment on using a global variable *at all*.
> >
> > Using a global variable might as well work, I just feel that your current patches
> > are good enough.
> [Duran, Leo]
> Great... I hear you.
> Then, I'd prefer not refactoring further at this point.... I hope Ray & Eric agree.
> 
> Thanks for your feedback!
> Leo.
> 
> >
> > Thanks
> > Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni@intel.com]
> Sent: Thursday, February 27, 2020 12:55 AM
> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> Leo and all,
> I replied in
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzill
> a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> duran%40amd.com%7C40786a41798d4173dd8508d7bb49aaa7%7C3dd8961
> fe4884e608e11a82d994e183d%7C0%7C0%7C637183797396444421&amp;s
> data=UoLRg%2ByFl%2BxyPB41xu1wOHpsf2euBrSe2HuD4CskTWg%3D&amp;r
> eserved=0 for a more general question about how uCode is used in AMD
> processors.
> 
> Because this package recently exposed a new interface
> ShadowMicrocodePpi, I try to involve Leo in the review from AMD uCode's
> needs.
> 
> Thanks,
> Ray
[Duran, Leo] Hi Ray, I just updated the ticket to say:
AMD handles microcode patches outside of the context of UEFI. So EDK2 hooks (ShadowMicrocode PPI, et al) are not required.
(Hence my comments in the MpInitLib patch I just submitted)

> 
> > -----Original Message-----
> > From: Duran, Leo <leo.duran@amd.com>
> > Sent: Thursday, February 27, 2020 1:52 AM
> > To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> >
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > Sent: Wednesday, February 26, 2020 12:45 PM
> > > To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > <siyuan.fu@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > > MpInitLib
> > >
> > > On 02/26/20 17:39, Duran, Leo wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > >> Sent: Wednesday, February 26, 2020 11:21 AM
> > > >> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > > >> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > >> <siyuan.fu@intel.com>
> > > >> Cc: Dong, Eric <eric.dong@intel.com>
> > > >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug
> > > >> in MpInitLib
> > > >>
> > > >> On 02/26/20 16:46, Duran, Leo wrote:
> > > >>> BTW,
> > > >>>
> > > >>> I also considered adding a flag to CPU_MP_DATA to make the usage
> > > >>> of
> > > >> PlatformId a bit more explicit.
> > > >>> E.g., something like CpuMpData-
> > > >>> CpuData[ProcessorNumber].IsValidPlatformId... So the init code
> > > >>> would look
> > > >> like this:
> > > >>>
> > > >>>   //
> > > >>>   // NOTE: PlatformId is not relevant on AMD platforms.
> > > >>>   //
> > > >>>   if (StandardSignatureIsAuthenticAMD ()) {
> > > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId =
> FALSE;
> > > >>>   else {
> > > >>>     PlatformIdMsr.Uint64 = AsmReadMsr64
> (MSR_IA32_PLATFORM_ID);
> > > >>>     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> > > >> (UINT8)PlatformIdMsr.Bits.PlatformId;
> > > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
> > > >>>   }
> > > >>>
> > > >>> This way "IsValidPlatformId" could be checked prior to using
> "PlatformId".
> > > >>> Anyway, that seemed a bit overkill, so I opted against it... thoughts?
> > > >>
> > > >> I think a global flag is justified; in the above approach,
> "IsValidPlatformId"
> > > >> would not vary across "ProcessorNumber", so it does look like
> > > >> useless generality.
> > > > [Duran, Leo]
> > > > Great point, Laszlo.
> > > > Indeed, global makes senses in the case!
> > > > I can prepare a v2-set to incorporate that.
> > >
> > > No, sorry, that wasn't what I meant. I didn't try to suggest a global variable.
> > > Instead, I meant that a "global check" (conceptually, i.e.
> > > regardless of particular processor number) made sense.
> > >
> > > I'm also not particularly *against* a global variable. In other
> > > words, I didn't try to comment on using a global variable *at all*.
> > >
> > > Using a global variable might as well work, I just feel that your
> > > current patches are good enough.
> > [Duran, Leo]
> > Great... I hear you.
> > Then, I'd prefer not refactoring further at this point.... I hope Ray & Eric
> agree.
> >
> > Thanks for your feedback!
> > Leo.
> >
> > >
> > > Thanks
> > > Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Ni, Ray]

Leo,
Thanks for your quick reply.

My most concern part to your patch is to expose a new API in LocalApicLib checking whether the processor is AMD type. LocalApicLib is a library that operates on Local APIC registers while checking CPU type deals with CPUID signature. It's not proper to mix the two things together just because LocalApicLib needs to know CPU type in some of the operation.

Because BaseLib already provides AsmCpuid() API and the check of CPU ID signature is just one line of comparison, I prefer to call AsmCpuid() directly in MpInitLib.

And in the patch to MpInitLib, since the AMD platform can set the two microcode PCDs to 0 to skip the microcode detection, I think the only place that needs to take care is in InitializeApData().

What do you think?

Thanks,
Ray



> -----Original Message-----
> From: Duran, Leo <leo.duran@amd.com>
> Sent: Friday, February 28, 2020 2:17 AM
> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib
> 
> 
> 
> > -----Original Message-----
> > From: Ni, Ray [mailto:ray.ni@intel.com]
> > Sent: Thursday, February 27, 2020 12:55 AM
> > To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.com>;
> > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> > Leo and all,
> > I replied in
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzill
> > a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> > duran%40amd.com%7C40786a41798d4173dd8508d7bb49aaa7%7C3dd8961
> > fe4884e608e11a82d994e183d%7C0%7C0%7C637183797396444421&amp;s
> > data=UoLRg%2ByFl%2BxyPB41xu1wOHpsf2euBrSe2HuD4CskTWg%3D&amp;r
> > eserved=0 for a more general question about how uCode is used in AMD
> > processors.
> >
> > Because this package recently exposed a new interface
> > ShadowMicrocodePpi, I try to involve Leo in the review from AMD uCode's
> > needs.
> >
> > Thanks,
> > Ray
> [Duran, Leo] Hi Ray, I just updated the ticket to say:
> AMD handles microcode patches outside of the context of UEFI. So EDK2 hooks
> (ShadowMicrocode PPI, et al) are not required.
> (Hence my comments in the MpInitLib patch I just submitted)
> 
> >
> > > -----Original Message-----
> > > From: Duran, Leo <leo.duran@amd.com>
> > > Sent: Thursday, February 27, 2020 1:52 AM
> > > To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> > > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > <siyuan.fu@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > > MpInitLib
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > Sent: Wednesday, February 26, 2020 12:45 PM
> > > > To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > > > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > > <siyuan.fu@intel.com>
> > > > Cc: Dong, Eric <eric.dong@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > > > MpInitLib
> > > >
> > > > On 02/26/20 17:39, Duran, Leo wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > >> Sent: Wednesday, February 26, 2020 11:21 AM
> > > > >> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > > > >> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > > >> <siyuan.fu@intel.com>
> > > > >> Cc: Dong, Eric <eric.dong@intel.com>
> > > > >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug
> > > > >> in MpInitLib
> > > > >>
> > > > >> On 02/26/20 16:46, Duran, Leo wrote:
> > > > >>> BTW,
> > > > >>>
> > > > >>> I also considered adding a flag to CPU_MP_DATA to make the usage
> > > > >>> of
> > > > >> PlatformId a bit more explicit.
> > > > >>> E.g., something like CpuMpData-
> > > > >>> CpuData[ProcessorNumber].IsValidPlatformId... So the init code
> > > > >>> would look
> > > > >> like this:
> > > > >>>
> > > > >>>   //
> > > > >>>   // NOTE: PlatformId is not relevant on AMD platforms.
> > > > >>>   //
> > > > >>>   if (StandardSignatureIsAuthenticAMD ()) {
> > > > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId =
> > FALSE;
> > > > >>>   else {
> > > > >>>     PlatformIdMsr.Uint64 = AsmReadMsr64
> > (MSR_IA32_PLATFORM_ID);
> > > > >>>     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> > > > >> (UINT8)PlatformIdMsr.Bits.PlatformId;
> > > > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId = TRUE;
> > > > >>>   }
> > > > >>>
> > > > >>> This way "IsValidPlatformId" could be checked prior to using
> > "PlatformId".
> > > > >>> Anyway, that seemed a bit overkill, so I opted against it... thoughts?
> > > > >>
> > > > >> I think a global flag is justified; in the above approach,
> > "IsValidPlatformId"
> > > > >> would not vary across "ProcessorNumber", so it does look like
> > > > >> useless generality.
> > > > > [Duran, Leo]
> > > > > Great point, Laszlo.
> > > > > Indeed, global makes senses in the case!
> > > > > I can prepare a v2-set to incorporate that.
> > > >
> > > > No, sorry, that wasn't what I meant. I didn't try to suggest a global variable.
> > > > Instead, I meant that a "global check" (conceptually, i.e.
> > > > regardless of particular processor number) made sense.
> > > >
> > > > I'm also not particularly *against* a global variable. In other
> > > > words, I didn't try to comment on using a global variable *at all*.
> > > >
> > > > Using a global variable might as well work, I just feel that your
> > > > current patches are good enough.
> > > [Duran, Leo]
> > > Great... I hear you.
> > > Then, I'd prefer not refactoring further at this point.... I hope Ray & Eric
> > agree.
> > >
> > > Thanks for your feedback!
> > > Leo.
> > >
> > > >
> > > > Thanks
> > > > Laszlo

Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib

[Duran, Leo]

> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni@intel.com]
> Sent: Friday, February 28, 2020 1:47 AM
> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> MpInitLib
> 
> Leo,
> Thanks for your quick reply.
> 
> My most concern part to your patch is to expose a new API in LocalApicLib
> checking whether the processor is AMD type. LocalApicLib is a library that
> operates on Local APIC registers while checking CPU type deals with CPUID
> signature. It's not proper to mix the two things together just because
> LocalApicLib needs to know CPU type in some of the operation.
[Duran, Leo] 
I can't argue that point strongly.
It just seemed "related enough" to warrant the export to avoid duplication.

> 
> Because BaseLib already provides AsmCpuid() API and the check of CPU ID
> signature is just one line of comparison, I prefer to call AsmCpuid() directly in
> MpInitLib.
> 
> And in the patch to MpInitLib, since the AMD platform can set the two
> microcode PCDs to 0 to skip the microcode detection, I think the only place
> that needs to take care is in InitializeApData().
> 
> What do you think?
[Duran, Leo] 
I take this to mean duplicating the Signature function in MpLib.c
So, I will submit that patch instead.

> 
> Thanks,
> Ray
> 
> 
> 
> > -----Original Message-----
> > From: Duran, Leo <leo.duran@amd.com>
> > Sent: Friday, February 28, 2020 2:17 AM
> > To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > MpInitLib
> >
> >
> >
> > > -----Original Message-----
> > > From: Ni, Ray [mailto:ray.ni@intel.com]
> > > Sent: Thursday, February 27, 2020 12:55 AM
> > > To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek
> > > <lersek@redhat.com>; devel@edk2.groups.io; Wu, Hao A
> > > <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug in
> > > MpInitLib
> > >
> > > Leo and all,
> > > I replied in
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbu
> > > gzill
> > >
> a.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7Cleo.
> > >
> duran%40amd.com%7C40786a41798d4173dd8508d7bb49aaa7%7C3dd8961
> > >
> fe4884e608e11a82d994e183d%7C0%7C0%7C637183797396444421&amp;s
> > >
> data=UoLRg%2ByFl%2BxyPB41xu1wOHpsf2euBrSe2HuD4CskTWg%3D&amp;r
> > > eserved=0 for a more general question about how uCode is used in AMD
> > > processors.
> > >
> > > Because this package recently exposed a new interface
> > > ShadowMicrocodePpi, I try to involve Leo in the review from AMD
> > > uCode's needs.
> > >
> > > Thanks,
> > > Ray
> > [Duran, Leo] Hi Ray, I just updated the ticket to say:
> > AMD handles microcode patches outside of the context of UEFI. So EDK2
> > hooks (ShadowMicrocode PPI, et al) are not required.
> > (Hence my comments in the MpInitLib patch I just submitted)
> >
> > >
> > > > -----Original Message-----
> > > > From: Duran, Leo <leo.duran@amd.com>
> > > > Sent: Thursday, February 27, 2020 1:52 AM
> > > > To: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> > > > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > > <siyuan.fu@intel.com>
> > > > Cc: Dong, Eric <eric.dong@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix bug
> > > > in MpInitLib
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > > Sent: Wednesday, February 26, 2020 12:45 PM
> > > > > To: Duran, Leo <leo.duran@amd.com>; Ni, Ray <ray.ni@intel.com>;
> > > > > devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Fu, Siyuan
> > > > > <siyuan.fu@intel.com>
> > > > > Cc: Dong, Eric <eric.dong@intel.com>
> > > > > Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix
> > > > > bug in MpInitLib
> > > > >
> > > > > On 02/26/20 17:39, Duran, Leo wrote:
> > > > > >
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > > >> Sent: Wednesday, February 26, 2020 11:21 AM
> > > > > >> To: Duran, Leo <leo.duran@amd.com>; Ni, Ray
> > > > > >> <ray.ni@intel.com>; devel@edk2.groups.io; Wu, Hao A
> > > > > >> <hao.a.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> > > > > >> Cc: Dong, Eric <eric.dong@intel.com>
> > > > > >> Subject: Re: [edk2-devel] [PATCH 0/2] UefiCpuPkg/Library: Fix
> > > > > >> bug in MpInitLib
> > > > > >>
> > > > > >> On 02/26/20 16:46, Duran, Leo wrote:
> > > > > >>> BTW,
> > > > > >>>
> > > > > >>> I also considered adding a flag to CPU_MP_DATA to make the
> > > > > >>> usage of
> > > > > >> PlatformId a bit more explicit.
> > > > > >>> E.g., something like CpuMpData-
> > > > > >>> CpuData[ProcessorNumber].IsValidPlatformId... So the init
> > > > > >>> code would look
> > > > > >> like this:
> > > > > >>>
> > > > > >>>   //
> > > > > >>>   // NOTE: PlatformId is not relevant on AMD platforms.
> > > > > >>>   //
> > > > > >>>   if (StandardSignatureIsAuthenticAMD ()) {
> > > > > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId =
> > > FALSE;
> > > > > >>>   else {
> > > > > >>>     PlatformIdMsr.Uint64 = AsmReadMsr64
> > > (MSR_IA32_PLATFORM_ID);
> > > > > >>>     CpuMpData->CpuData[ProcessorNumber].PlatformId =
> > > > > >> (UINT8)PlatformIdMsr.Bits.PlatformId;
> > > > > >>>     CpuMpData->CpuData[ProcessorNumber].IsValidPlatformId =
> TRUE;
> > > > > >>>   }
> > > > > >>>
> > > > > >>> This way "IsValidPlatformId" could be checked prior to using
> > > "PlatformId".
> > > > > >>> Anyway, that seemed a bit overkill, so I opted against it...
> thoughts?
> > > > > >>
> > > > > >> I think a global flag is justified; in the above approach,
> > > "IsValidPlatformId"
> > > > > >> would not vary across "ProcessorNumber", so it does look like
> > > > > >> useless generality.
> > > > > > [Duran, Leo]
> > > > > > Great point, Laszlo.
> > > > > > Indeed, global makes senses in the case!
> > > > > > I can prepare a v2-set to incorporate that.
> > > > >
> > > > > No, sorry, that wasn't what I meant. I didn't try to suggest a global
> variable.
> > > > > Instead, I meant that a "global check" (conceptually, i.e.
> > > > > regardless of particular processor number) made sense.
> > > > >
> > > > > I'm also not particularly *against* a global variable. In other
> > > > > words, I didn't try to comment on using a global variable *at all*.
> > > > >
> > > > > Using a global variable might as well work, I just feel that
> > > > > your current patches are good enough.
> > > > [Duran, Leo]
> > > > Great... I hear you.
> > > > Then, I'd prefer not refactoring further at this point.... I hope
> > > > Ray & Eric
> > > agree.
> > > >
> > > > Thanks for your feedback!
> > > > Leo.
> > > >
> > > > >
> > > > > Thanks
> > > > > Laszlo