[PATCH v4] UefiCpuPkg: Fix bug in MpInitLib

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v4] UefiCpuPkg: Fix bug in MpInitLib
@ 2020-02-29 15:05 Leo Duran
  2020-02-29 15:05 ` [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors Leo Duran
  0 siblings, 1 reply; 8+ messages in thread
From: Leo Duran @ 2020-02-29 15:05 UTC (permalink / raw)
  To: devel; +Cc: Leo Duran

This patch fixes an issue introduced recently in MpInitLib, where we read
a PlatformId MSR that is not implemented on AMD processors. The patch uses
CPUID signature check to skip reading the PlatformId MSR.

Changes since v3:
Add header file entry for CPUID definitions: Register/Amd/Cpuid.h

Leo Duran (1):
  UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 ++++++++++++++++++++++++++++++++--
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +++
 2 files changed, 35 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 UefiCpuPkg/Library/MpInitLib/MpLib.h

-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
  2020-02-29 15:05 [PATCH v4] UefiCpuPkg: Fix bug in MpInitLib Leo Duran
@ 2020-02-29 15:05 ` Leo Duran
  2020-02-29 19:50   ` Laszlo Ersek
                     ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Leo Duran @ 2020-02-29 15:05 UTC (permalink / raw)
  To: devel; +Cc: Leo Duran, Eric Dong, Ray Ni, Laszlo Ersek

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

This patch uses CPUID signature check to skip reading the PlatformId MSR,
which is not implemented on AMD processors.

The PlatformId is used for loading microcode patches, which is also not
supported and AMD-based platforms. To mitigate the PlatformId dependency,
PcdCpuMicrocodePatchAddress and PcdCpuMicrodePatchRegionSize must be set
to 0 (default value), in order to bypass microcode loading code paths.

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/MpLib.c | 34 ++++++++++++++++++++++++++++++++--
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +++
 2 files changed, 35 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 UefiCpuPkg/Library/MpInitLib/MpLib.h

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d0fbc17..d2200c3 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
 
 **/
@@ -10,6 +12,29 @@
 
 EFI_GUID mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
 
+
+/**
+  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);
+}
+
 /**
   The function will check if BSP Execute Disable is enabled.
 
@@ -564,8 +589,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,
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
old mode 100644
new mode 100755
index 455cb3f..0c89f8a
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -2,6 +2,8 @@
   Common header file for MP Initialize Library.
 
   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
 
 **/
@@ -12,6 +14,7 @@
 #include <PiPei.h>
 
 #include <Register/Intel/Cpuid.h>
+#include <Register/Amd/Cpuid.h>
 #include <Register/Intel/Msr.h>
 #include <Register/Intel/LocalApic.h>
 #include <Register/Intel/Microcode.h>
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
  2020-02-29 15:05 ` [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors Leo Duran
@ 2020-02-29 19:50   ` Laszlo Ersek
  2020-03-02  1:59   ` Ni, Ray
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2020-02-29 19:50 UTC (permalink / raw)
  To: Leo Duran, devel; +Cc: Eric Dong, Ray Ni

On 02/29/20 16:05, Leo Duran wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
> 
> This patch uses CPUID signature check to skip reading the PlatformId MSR,
> which is not implemented on AMD processors.
> 
> The PlatformId is used for loading microcode patches, which is also not
> supported and AMD-based platforms. To mitigate the PlatformId dependency,
> PcdCpuMicrocodePatchAddress and PcdCpuMicrodePatchRegionSize must be set
> to 0 (default value), in order to bypass microcode loading code paths.
> 
> 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/MpLib.c | 34 ++++++++++++++++++++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +++
>  2 files changed, 35 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 UefiCpuPkg/Library/MpInitLib/MpLib.h
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d0fbc17..d2200c3 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
>  
>  **/
> @@ -10,6 +12,29 @@
>  
>  EFI_GUID mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
>  
> +
> +/**
> +  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);
> +}
> +
>  /**
>    The function will check if BSP Execute Disable is enabled.
>  
> @@ -564,8 +589,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,
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> old mode 100644
> new mode 100755
> index 455cb3f..0c89f8a
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -2,6 +2,8 @@
>    Common header file for MP Initialize Library.
>  
>    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
>  
>  **/
> @@ -12,6 +14,7 @@
>  #include <PiPei.h>
>  
>  #include <Register/Intel/Cpuid.h>
> +#include <Register/Amd/Cpuid.h>
>  #include <Register/Intel/Msr.h>
>  #include <Register/Intel/LocalApic.h>
>  #include <Register/Intel/Microcode.h>
> 

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
  2020-02-29 15:05 ` [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors Leo Duran
  2020-02-29 19:50   ` Laszlo Ersek
@ 2020-03-02  1:59   ` Ni, Ray
       [not found]   ` <15F85A23C0D08039.4290@groups.io>
  2020-04-17  8:14   ` Gary Lin
  3 siblings, 0 replies; 8+ messages in thread
From: Ni, Ray @ 2020-03-02  1:59 UTC (permalink / raw)
  To: Leo Duran, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek

Leo,
The function name is the same as another local function defined in LocalApicLib.
You may need to add a STATIC keyword for this local function, or change it to a
different name.

Thanks,
Ray

> -----Original Message-----
> From: Leo Duran <leo.duran@amd.com>
> Sent: Saturday, February 29, 2020 11:06 PM
> 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] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD
> processors.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
> 
> This patch uses CPUID signature check to skip reading the PlatformId MSR,
> which is not implemented on AMD processors.
> 
> The PlatformId is used for loading microcode patches, which is also not
> supported and AMD-based platforms. To mitigate the PlatformId
> dependency,
> PcdCpuMicrocodePatchAddress and PcdCpuMicrodePatchRegionSize must
> be set
> to 0 (default value), in order to bypass microcode loading code paths.
> 
> 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/MpLib.c | 34
> ++++++++++++++++++++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +++
>  2 files changed, 35 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 UefiCpuPkg/Library/MpInitLib/MpLib.h
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d0fbc17..d2200c3 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
> 
>  **/
> @@ -10,6 +12,29 @@
> 
>  EFI_GUID mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
> 
> +
> +/**
> +  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);
> +}
> +
>  /**
>    The function will check if BSP Execute Disable is enabled.
> 
> @@ -564,8 +589,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,
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> old mode 100644
> new mode 100755
> index 455cb3f..0c89f8a
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -2,6 +2,8 @@
>    Common header file for MP Initialize Library.
> 
>    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
> 
>  **/
> @@ -12,6 +14,7 @@
>  #include <PiPei.h>
> 
>  #include <Register/Intel/Cpuid.h>
> +#include <Register/Amd/Cpuid.h>
>  #include <Register/Intel/Msr.h>
>  #include <Register/Intel/LocalApic.h>
>  #include <Register/Intel/Microcode.h>
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

[parent not found: <15F85A23C0D08039.4290@groups.io>]

* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
       [not found]   ` <15F85A23C0D08039.4290@groups.io>
@ 2020-03-02  4:21     ` Ni, Ray
  0 siblings, 0 replies; 8+ messages in thread
From: Ni, Ray @ 2020-03-02  4:21 UTC (permalink / raw)
  To: devel@edk2.groups.io, Ni, Ray, Leo Duran; +Cc: Dong, Eric, Laszlo Ersek

Reviewed-by: Ray Ni <ray.ni@intel.com>
I will add the "STATIC" when pushing.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Monday, March 2, 2020 10:00 AM
> To: Leo Duran <leo.duran@amd.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Skip reading
> PlatformId on AMD processors.
> 
> Leo,
> The function name is the same as another local function defined in
> LocalApicLib.
> You may need to add a STATIC keyword for this local function, or change it to
> a
> different name.
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Leo Duran <leo.duran@amd.com>
> > Sent: Saturday, February 29, 2020 11:06 PM
> > 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] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD
> > processors.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
> >
> > This patch uses CPUID signature check to skip reading the PlatformId MSR,
> > which is not implemented on AMD processors.
> >
> > The PlatformId is used for loading microcode patches, which is also not
> > supported and AMD-based platforms. To mitigate the PlatformId
> > dependency,
> > PcdCpuMicrocodePatchAddress and PcdCpuMicrodePatchRegionSize must
> > be set
> > to 0 (default value), in order to bypass microcode loading code paths.
> >
> > 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/MpLib.c | 34
> > ++++++++++++++++++++++++++++++++--
> >  UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +++
> >  2 files changed, 35 insertions(+), 2 deletions(-)
> >  mode change 100644 => 100755 UefiCpuPkg/Library/MpInitLib/MpLib.h
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index d0fbc17..d2200c3 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
> >
> >  **/
> > @@ -10,6 +12,29 @@
> >
> >  EFI_GUID mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
> >
> > +
> > +/**
> > +  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);
> > +}
> > +
> >  /**
> >    The function will check if BSP Execute Disable is enabled.
> >
> > @@ -564,8 +589,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,
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > old mode 100644
> > new mode 100755
> > index 455cb3f..0c89f8a
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -2,6 +2,8 @@
> >    Common header file for MP Initialize Library.
> >
> >    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
> >
> >  **/
> > @@ -12,6 +14,7 @@
> >  #include <PiPei.h>
> >
> >  #include <Register/Intel/Cpuid.h>
> > +#include <Register/Amd/Cpuid.h>
> >  #include <Register/Intel/Msr.h>
> >  #include <Register/Intel/LocalApic.h>
> >  #include <Register/Intel/Microcode.h>
> > --
> > 2.7.4
> 
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
  2020-02-29 15:05 ` [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors Leo Duran
                     ` (2 preceding siblings ...)
       [not found]   ` <15F85A23C0D08039.4290@groups.io>
@ 2020-04-17  8:14   ` Gary Lin
  2020-04-20  9:41     ` Laszlo Ersek
  3 siblings, 1 reply; 8+ messages in thread
From: Gary Lin @ 2020-04-17  8:14 UTC (permalink / raw)
  To: devel, leo.duran; +Cc: Eric Dong, Ray Ni, Laszlo Ersek

On Sat, Feb 29, 2020 at 09:05:45AM -0600, Leo Duran wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
> 
> This patch uses CPUID signature check to skip reading the PlatformId MSR,
> which is not implemented on AMD processors.
> 
> The PlatformId is used for loading microcode patches, which is also not
> supported and AMD-based platforms. To mitigate the PlatformId dependency,
> PcdCpuMicrocodePatchAddress and PcdCpuMicrodePatchRegionSize must be set
> to 0 (default value), in order to bypass microcode loading code paths.
> 
I got an error report about the executable bit of MpLib.h while
packaging ovmf. It turned out that this patch accidentally changed the
file mode. It would be nice if the mode can be corrected.

Thanks,

Gary Lin

> 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/MpLib.c | 34 ++++++++++++++++++++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +++
>  2 files changed, 35 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 UefiCpuPkg/Library/MpInitLib/MpLib.h
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d0fbc17..d2200c3 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
>  
>  **/
> @@ -10,6 +12,29 @@
>  
>  EFI_GUID mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
>  
> +
> +/**
> +  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);
> +}
> +
>  /**
>    The function will check if BSP Execute Disable is enabled.
>  
> @@ -564,8 +589,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,
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> old mode 100644
> new mode 100755
           ^^^^^^
	   mode change
> index 455cb3f..0c89f8a
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -2,6 +2,8 @@
>    Common header file for MP Initialize Library.
>  
>    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
>  
>  **/
> @@ -12,6 +14,7 @@
>  #include <PiPei.h>
>  
>  #include <Register/Intel/Cpuid.h>
> +#include <Register/Amd/Cpuid.h>
>  #include <Register/Intel/Msr.h>
>  #include <Register/Intel/LocalApic.h>
>  #include <Register/Intel/Microcode.h>
> -- 
> 2.7.4
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
  2020-04-17  8:14   ` Gary Lin
@ 2020-04-20  9:41     ` Laszlo Ersek
  2020-04-21 18:47       ` Duran, Leo
  0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2020-04-20  9:41 UTC (permalink / raw)
  To: Gary Lin, devel, leo.duran; +Cc: Eric Dong, Ray Ni

On 04/17/20 10:14, Gary Lin wrote:
> On Sat, Feb 29, 2020 at 09:05:45AM -0600, Leo Duran wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2556
>>
>> This patch uses CPUID signature check to skip reading the PlatformId MSR,
>> which is not implemented on AMD processors.
>>
>> The PlatformId is used for loading microcode patches, which is also not
>> supported and AMD-based platforms. To mitigate the PlatformId dependency,
>> PcdCpuMicrocodePatchAddress and PcdCpuMicrodePatchRegionSize must be set
>> to 0 (default value), in order to bypass microcode loading code paths.
>>
> I got an error report about the executable bit of MpLib.h while
> packaging ovmf. It turned out that this patch accidentally changed the
> file mode. It would be nice if the mode can be corrected.

Thanks for the report. I've filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=2678> and assigned it to
Leo.

Leo, can you please submit the patch?

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors.
  2020-04-20  9:41     ` Laszlo Ersek
@ 2020-04-21 18:47       ` Duran, Leo
  0 siblings, 0 replies; 8+ messages in thread
From: Duran, Leo @ 2020-04-21 18:47 UTC (permalink / raw)
  To: Laszlo Ersek, Gary Lin, devel@edk2.groups.io; +Cc: Eric Dong, Ray Ni



> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, April 20, 2020 5:41 AM
> To: Gary Lin <glin@suse.com>; devel@edk2.groups.io; Duran, Leo
> <leo.duran@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>; Ray Ni <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Skip reading
> PlatformId on AMD processors.
> 
> On 04/17/20 10:14, Gary Lin wrote:
> > On Sat, Feb 29, 2020 at 09:05:45AM -0600, Leo Duran wrote:
> >> REF:
> >>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug
> >>
> zilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2556&amp;data=02%7C01%7
> Cleo
> >>
> .duran%40amd.com%7C10bcdc9150b24c3c53c108d7e50ef6a9%7C3dd8961fe
> 4884e6
> >>
> 08e11a82d994e183d%7C0%7C0%7C637229724752057989&amp;sdata=fcn7
> g8Tpx97I
> >> U6FBPB%2BZ8wJFMtd9zhQzxJIhOct%2B1Vo%3D&amp;reserved=0
> >>
> >> This patch uses CPUID signature check to skip reading the PlatformId
> >> MSR, which is not implemented on AMD processors.
> >>
> >> The PlatformId is used for loading microcode patches, which is also
> >> not supported and AMD-based platforms. To mitigate the PlatformId
> >> dependency, PcdCpuMicrocodePatchAddress and
> >> PcdCpuMicrodePatchRegionSize must be set to 0 (default value), in order
> to bypass microcode loading code paths.
> >>
> > I got an error report about the executable bit of MpLib.h while
> > packaging ovmf. It turned out that this patch accidentally changed the
> > file mode. It would be nice if the mode can be corrected.
> 
> Thanks for the report. I've filed
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug
> zilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2678&amp;data=02%7C01%7
> Cleo.duran%40amd.com%7C10bcdc9150b24c3c53c108d7e50ef6a9%7C3dd89
> 61fe4884e608e11a82d994e183d%7C0%7C0%7C637229724752057989&amp;
> sdata=wlBDe%2BOTRXpUyuCyJv5aJd1mH7NMbNkzZ%2B4obnhkfx0%3D&am
> p;reserved=0> and assigned it to Leo.
> 
> Leo, can you please submit the patch?
[Duran, Leo] 
Yes, I can do that... Unless the Maintainer prefers to take care of the file permission bits.

> 
> Thanks!
> Laszlo


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-04-21 18:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-29 15:05 [PATCH v4] UefiCpuPkg: Fix bug in MpInitLib Leo Duran
2020-02-29 15:05 ` [PATCH] UefiCpuPkg/MpInitLib: Skip reading PlatformId on AMD processors Leo Duran
2020-02-29 19:50   ` Laszlo Ersek
2020-03-02  1:59   ` Ni, Ray
     [not found]   ` <15F85A23C0D08039.4290@groups.io>
2020-03-02  4:21     ` [edk2-devel] " Ni, Ray
2020-04-17  8:14   ` Gary Lin
2020-04-20  9:41     ` Laszlo Ersek
2020-04-21 18:47       ` Duran, Leo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox