[edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()

Wu, Hao A posted 1 patch 4 years, 1 month ago
Failed in applying to current master (apply log)
UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
[edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Wu, Hao A 4 years, 1 month ago
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498

Commit fd30b00707 updated the logic in function MicrocodeDetect() that
will directly use the CPUID and PlatformID information from the 'CpuData'
field in the CPU_MP_DATA structure, instead of collecting these
information for each processor via AsmCpuid() and AsmReadMsr64() calls
respectively.

At that moment, this approach worked fine for APs. Since:
a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
   entry at PEI phase), the function InitializeApData() will be called for
   each AP and the CPUID and PlatformID information will be collected.

b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
   APs are waken up again, the function InitializeApData() will not be
   called, which means the CPUID and PlatformID information will not be
   collected. However, the below logics in MicrocodeDetect() function:

  CurrentRevision = GetCurrentMicrocodeSignature ();
  IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
  if (CurrentRevision != 0 && !IsBspCallIn) {
    //
    // Skip loading microcode if it has been loaded successfully
    //
    return;
  }

   will ensure that the microcode detection and application will be
   skipped due to the fact that such process has already been done in the
   PEI phase.

But after commit 396e791059, which removes the above skip loading logic,
the CPUID and PlatformID information on APs will be used upon the 2nd
entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
information has not been collected, it will bring issue to the microcode
detection process.

This commit will update the logic in MicrocodeDetect() back to always
collecting the CPUID and PlatformID information explicitly.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index b6675b9a60..67e214d463 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -93,6 +93,7 @@ MicrocodeDetect (
   UINT32                                  InCompleteCheckSum32;
   BOOLEAN                                 CorrectMicrocode;
   VOID                                    *MicrocodeData;
+  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
   UINT32                                  ThreadId;
   BOOLEAN                                 IsBspCallIn;
 
@@ -115,8 +116,18 @@ MicrocodeDetect (
   }
 
   ExtendedTableLength = 0;
-  Eax.Uint32 = CpuMpData->CpuData[ProcessorNumber].ProcessorSignature;
-  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
+  //
+  // Here data of CPUID leafs have not been collected into context buffer, so
+  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
+  //
+  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
+
+  //
+  // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID
+  //
+  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
+  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
+
 
   //
   // Check whether AP has same processor with BSP.
-- 
2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53633): https://edk2.groups.io/g/devel/message/53633
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Siyuan, Fu 4 years, 1 month ago
Hi, Hao

If CPUID and PlatformID will always be collected in MicrocodeDetect(), is the change in 999463 redundant now? Should we remove that code?

Best Regards
Siyuan 

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: 2020年2月3日 8:35
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan
> <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID &
> PlatformID in MicrocodeDetect()
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> 
> Commit fd30b00707 updated the logic in function MicrocodeDetect() that
> will directly use the CPUID and PlatformID information from the 'CpuData'
> field in the CPU_MP_DATA structure, instead of collecting these
> information for each processor via AsmCpuid() and AsmReadMsr64() calls
> respectively.
> 
> At that moment, this approach worked fine for APs. Since:
> a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
>    entry at PEI phase), the function InitializeApData() will be called for
>    each AP and the CPUID and PlatformID information will be collected.
> 
> b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
>    APs are waken up again, the function InitializeApData() will not be
>    called, which means the CPUID and PlatformID information will not be
>    collected. However, the below logics in MicrocodeDetect() function:
> 
>   CurrentRevision = GetCurrentMicrocodeSignature ();
>   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> TRUE : FALSE;
>   if (CurrentRevision != 0 && !IsBspCallIn) {
>     //
>     // Skip loading microcode if it has been loaded successfully
>     //
>     return;
>   }
> 
>    will ensure that the microcode detection and application will be
>    skipped due to the fact that such process has already been done in the
>    PEI phase.
> 
> But after commit 396e791059, which removes the above skip loading logic,
> the CPUID and PlatformID information on APs will be used upon the 2nd
> entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> information has not been collected, it will bring issue to the microcode
> detection process.
> 
> This commit will update the logic in MicrocodeDetect() back to always
> collecting the CPUID and PlatformID information explicitly.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index b6675b9a60..67e214d463 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -93,6 +93,7 @@ MicrocodeDetect (
>    UINT32                                  InCompleteCheckSum32;
>    BOOLEAN                                 CorrectMicrocode;
>    VOID                                    *MicrocodeData;
> +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
>    UINT32                                  ThreadId;
>    BOOLEAN                                 IsBspCallIn;
> 
> @@ -115,8 +116,18 @@ MicrocodeDetect (
>    }
> 
>    ExtendedTableLength = 0;
> -  Eax.Uint32 = CpuMpData->CpuData[ProcessorNumber].ProcessorSignature;
> -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> +  //
> +  // Here data of CPUID leafs have not been collected into context buffer, so
> +  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
> +  //
> +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +  //
> +  // The index of platform information resides in bits 50:52 of MSR
> IA32_PLATFORM_ID
> +  //
> +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> +  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> +
> 
>    //
>    // Check whether AP has same processor with BSP.
> --
> 2.12.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53634): https://edk2.groups.io/g/devel/message/53634
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Wu, Hao A 4 years, 1 month ago
> -----Original Message-----
> From: Fu, Siyuan
> Sent: Monday, February 03, 2020 8:55 AM
> To: devel@edk2.groups.io; Wu, Hao A
> Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID
> & PlatformID in MicrocodeDetect()
> 
> Hi, Hao
> 
> If CPUID and PlatformID will always be collected in MicrocodeDetect(), is the
> change in 999463 redundant now? Should we remove that code?


I think the changes are still needed.

The CPUID and PlatformID information stored in the 'CPU_AP_DATA' structure are
needed during the pre-process of the microcode patch headers for deciding
whether the microcode patch data should be loaded into memory.

Best Regards,
Hao Wu


> 
> Best Regards
> Siyuan
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> > A
> > Sent: 2020年2月3日 8:35
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID &
> > PlatformID in MicrocodeDetect()
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> >
> > Commit fd30b00707 updated the logic in function MicrocodeDetect() that
> > will directly use the CPUID and PlatformID information from the 'CpuData'
> > field in the CPU_MP_DATA structure, instead of collecting these
> > information for each processor via AsmCpuid() and AsmReadMsr64() calls
> > respectively.
> >
> > At that moment, this approach worked fine for APs. Since:
> > a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
> >    entry at PEI phase), the function InitializeApData() will be called for
> >    each AP and the CPUID and PlatformID information will be collected.
> >
> > b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
> >    APs are waken up again, the function InitializeApData() will not be
> >    called, which means the CPUID and PlatformID information will not be
> >    collected. However, the below logics in MicrocodeDetect() function:
> >
> >   CurrentRevision = GetCurrentMicrocodeSignature ();
> >   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> > TRUE : FALSE;
> >   if (CurrentRevision != 0 && !IsBspCallIn) {
> >     //
> >     // Skip loading microcode if it has been loaded successfully
> >     //
> >     return;
> >   }
> >
> >    will ensure that the microcode detection and application will be
> >    skipped due to the fact that such process has already been done in the
> >    PEI phase.
> >
> > But after commit 396e791059, which removes the above skip loading logic,
> > the CPUID and PlatformID information on APs will be used upon the 2nd
> > entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> > information has not been collected, it will bring issue to the microcode
> > detection process.
> >
> > This commit will update the logic in MicrocodeDetect() back to always
> > collecting the CPUID and PlatformID information explicitly.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index b6675b9a60..67e214d463 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -93,6 +93,7 @@ MicrocodeDetect (
> >    UINT32                                  InCompleteCheckSum32;
> >    BOOLEAN                                 CorrectMicrocode;
> >    VOID                                    *MicrocodeData;
> > +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
> >    UINT32                                  ThreadId;
> >    BOOLEAN                                 IsBspCallIn;
> >
> > @@ -115,8 +116,18 @@ MicrocodeDetect (
> >    }
> >
> >    ExtendedTableLength = 0;
> > -  Eax.Uint32 = CpuMpData->CpuData[ProcessorNumber].ProcessorSignature;
> > -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> > +  //
> > +  // Here data of CPUID leafs have not been collected into context buffer,
> so
> > +  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
> > +  //
> > +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> > +
> > +  //
> > +  // The index of platform information resides in bits 50:52 of MSR
> > IA32_PLATFORM_ID
> > +  //
> > +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > +  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> > +
> >
> >    //
> >    // Check whether AP has same processor with BSP.
> > --
> > 2.12.0.windows.1
> >
> >
> > 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53636): https://edk2.groups.io/g/devel/message/53636
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Siyuan, Fu 4 years, 1 month ago
Got it.

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

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: 2020年2月3日 9:03
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get
> CPUID & PlatformID in MicrocodeDetect()
> 
> > -----Original Message-----
> > From: Fu, Siyuan
> > Sent: Monday, February 03, 2020 8:55 AM
> > To: devel@edk2.groups.io; Wu, Hao A
> > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D
> > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get
> CPUID
> > & PlatformID in MicrocodeDetect()
> >
> > Hi, Hao
> >
> > If CPUID and PlatformID will always be collected in MicrocodeDetect(), is
> the
> > change in 999463 redundant now? Should we remove that code?
> 
> 
> I think the changes are still needed.
> 
> The CPUID and PlatformID information stored in the 'CPU_AP_DATA'
> structure are
> needed during the pre-process of the microcode patch headers for deciding
> whether the microcode patch data should be loaded into memory.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Best Regards
> > Siyuan
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Hao
> > > A
> > > Sent: 2020年2月3日 8:35
> > > To: devel@edk2.groups.io
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ni,
> > > Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan
> > > <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get
> CPUID &
> > > PlatformID in MicrocodeDetect()
> > >
> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> > >
> > > Commit fd30b00707 updated the logic in function MicrocodeDetect()
> that
> > > will directly use the CPUID and PlatformID information from the
> 'CpuData'
> > > field in the CPU_MP_DATA structure, instead of collecting these
> > > information for each processor via AsmCpuid() and AsmReadMsr64() calls
> > > respectively.
> > >
> > > At that moment, this approach worked fine for APs. Since:
> > > a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
> > >    entry at PEI phase), the function InitializeApData() will be called for
> > >    each AP and the CPUID and PlatformID information will be collected.
> > >
> > > b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
> > >    APs are waken up again, the function InitializeApData() will not be
> > >    called, which means the CPUID and PlatformID information will not be
> > >    collected. However, the below logics in MicrocodeDetect() function:
> > >
> > >   CurrentRevision = GetCurrentMicrocodeSignature ();
> > >   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData-
> >BspNumber) ?
> > > TRUE : FALSE;
> > >   if (CurrentRevision != 0 && !IsBspCallIn) {
> > >     //
> > >     // Skip loading microcode if it has been loaded successfully
> > >     //
> > >     return;
> > >   }
> > >
> > >    will ensure that the microcode detection and application will be
> > >    skipped due to the fact that such process has already been done in the
> > >    PEI phase.
> > >
> > > But after commit 396e791059, which removes the above skip loading
> logic,
> > > the CPUID and PlatformID information on APs will be used upon the 2nd
> > > entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> > > information has not been collected, it will bring issue to the microcode
> > > detection process.
> > >
> > > This commit will update the logic in MicrocodeDetect() back to always
> > > collecting the CPUID and PlatformID information explicitly.
> > >
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > > ---
> > >  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > > index b6675b9a60..67e214d463 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > > @@ -93,6 +93,7 @@ MicrocodeDetect (
> > >    UINT32                                  InCompleteCheckSum32;
> > >    BOOLEAN                                 CorrectMicrocode;
> > >    VOID                                    *MicrocodeData;
> > > +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
> > >    UINT32                                  ThreadId;
> > >    BOOLEAN                                 IsBspCallIn;
> > >
> > > @@ -115,8 +116,18 @@ MicrocodeDetect (
> > >    }
> > >
> > >    ExtendedTableLength = 0;
> > > -  Eax.Uint32 = CpuMpData-
> >CpuData[ProcessorNumber].ProcessorSignature;
> > > -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> > > +  //
> > > +  // Here data of CPUID leafs have not been collected into context buffer,
> > so
> > > +  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
> > > +  //
> > > +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> > > +
> > > +  //
> > > +  // The index of platform information resides in bits 50:52 of MSR
> > > IA32_PLATFORM_ID
> > > +  //
> > > +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > > +  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> > > +
> > >
> > >    //
> > >    // Check whether AP has same processor with BSP.
> > > --
> > > 2.12.0.windows.1
> > >
> > >
> > > 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53637): https://edk2.groups.io/g/devel/message/53637
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Laszlo Ersek 4 years, 1 month ago
Hello Hao,

On 02/03/20 01:34, Hao A Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> 
> Commit fd30b00707 updated the logic in function MicrocodeDetect() that
> will directly use the CPUID and PlatformID information from the 'CpuData'
> field in the CPU_MP_DATA structure, instead of collecting these
> information for each processor via AsmCpuid() and AsmReadMsr64() calls
> respectively.
> 
> At that moment, this approach worked fine for APs. Since:
> a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
>    entry at PEI phase), the function InitializeApData() will be called for
>    each AP and the CPUID and PlatformID information will be collected.
> 
> b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
>    APs are waken up again, the function InitializeApData() will not be
>    called, which means the CPUID and PlatformID information will not be
>    collected. However, the below logics in MicrocodeDetect() function:
> 
>   CurrentRevision = GetCurrentMicrocodeSignature ();
>   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
>   if (CurrentRevision != 0 && !IsBspCallIn) {
>     //
>     // Skip loading microcode if it has been loaded successfully
>     //
>     return;
>   }
> 
>    will ensure that the microcode detection and application will be
>    skipped due to the fact that such process has already been done in the
>    PEI phase.
> 
> But after commit 396e791059, which removes the above skip loading logic,
> the CPUID and PlatformID information on APs will be used upon the 2nd
> entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> information has not been collected, it will bring issue to the microcode
> detection process.
> 
> This commit will update the logic in MicrocodeDetect() back to always
> collecting the CPUID and PlatformID information explicitly.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

It seems like I haven't been involved in reviewing either commit
fd30b0070773 ("UefiCpuPkg/MpInitLib: Remove redundant microcode fields
in CPU_MP_DATA", 2020-01-02) or commit 396e791059f3 ("UefiCpuPkg: Always
load microcode patch on AP processor.", 2020-01-08), so I'd like to
defer on this patch to the other UefiCpuPkg reviewers.

Thanks!
Laszlo

> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index b6675b9a60..67e214d463 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -93,6 +93,7 @@ MicrocodeDetect (
>    UINT32                                  InCompleteCheckSum32;
>    BOOLEAN                                 CorrectMicrocode;
>    VOID                                    *MicrocodeData;
> +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
>    UINT32                                  ThreadId;
>    BOOLEAN                                 IsBspCallIn;
>  
> @@ -115,8 +116,18 @@ MicrocodeDetect (
>    }
>  
>    ExtendedTableLength = 0;
> -  Eax.Uint32 = CpuMpData->CpuData[ProcessorNumber].ProcessorSignature;
> -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> +  //
> +  // Here data of CPUID leafs have not been collected into context buffer, so
> +  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
> +  //
> +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +  //
> +  // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID
> +  //
> +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> +  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> +
>  
>    //
>    // Check whether AP has same processor with BSP.
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53652): https://edk2.groups.io/g/devel/message/53652
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Dong, Eric 4 years, 1 month ago
Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Monday, February 3, 2020 8:35 AM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()

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

Commit fd30b00707 updated the logic in function MicrocodeDetect() that will directly use the CPUID and PlatformID information from the 'CpuData'
field in the CPU_MP_DATA structure, instead of collecting these information for each processor via AsmCpuid() and AsmReadMsr64() calls respectively.

At that moment, this approach worked fine for APs. Since:
a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
   entry at PEI phase), the function InitializeApData() will be called for
   each AP and the CPUID and PlatformID information will be collected.

b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
   APs are waken up again, the function InitializeApData() will not be
   called, which means the CPUID and PlatformID information will not be
   collected. However, the below logics in MicrocodeDetect() function:

  CurrentRevision = GetCurrentMicrocodeSignature ();
  IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
  if (CurrentRevision != 0 && !IsBspCallIn) {
    //
    // Skip loading microcode if it has been loaded successfully
    //
    return;
  }

   will ensure that the microcode detection and application will be
   skipped due to the fact that such process has already been done in the
   PEI phase.

But after commit 396e791059, which removes the above skip loading logic, the CPUID and PlatformID information on APs will be used upon the 2nd entry of the MpInitLibInitialize(). But since the CPUID and PlatformID information has not been collected, it will bring issue to the microcode detection process.

This commit will update the logic in MicrocodeDetect() back to always collecting the CPUID and PlatformID information explicitly.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index b6675b9a60..67e214d463 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -93,6 +93,7 @@ MicrocodeDetect (
   UINT32                                  InCompleteCheckSum32;
   BOOLEAN                                 CorrectMicrocode;
   VOID                                    *MicrocodeData;
+  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
   UINT32                                  ThreadId;
   BOOLEAN                                 IsBspCallIn;
 
@@ -115,8 +116,18 @@ MicrocodeDetect (
   }
 
   ExtendedTableLength = 0;
-  Eax.Uint32 = CpuMpData->CpuData[ProcessorNumber].ProcessorSignature;
-  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
+  //
+  // Here data of CPUID leafs have not been collected into context 
+ buffer, so  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
+  //
+  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
+
+  //
+  // The index of platform information resides in bits 50:52 of MSR 
+ IA32_PLATFORM_ID  //
+  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
+ PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
+
 
   //
   // Check whether AP has same processor with BSP.
--
2.12.0.windows.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53723): https://edk2.groups.io/g/devel/message/53723
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Wu, Hao A 4 years, 1 month ago
Thanks Siyuan and Eric,

The patch has been pushed via commit a9e3458ba7.

Best Regards,
Hao Wu


> -----Original Message-----
> From: Dong, Eric
> Sent: Tuesday, February 04, 2020 9:47 PM
> To: devel@edk2.groups.io; Wu, Hao A
> Cc: Ni, Ray; Laszlo Ersek; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get
> CPUID & PlatformID in MicrocodeDetect()
> 
> Reviewed-by: Eric Dong <eric.dong@intel.com>
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Monday, February 3, 2020 8:35 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan
> <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID &
> PlatformID in MicrocodeDetect()
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> 
> Commit fd30b00707 updated the logic in function MicrocodeDetect() that
> will directly use the CPUID and PlatformID information from the 'CpuData'
> field in the CPU_MP_DATA structure, instead of collecting these information
> for each processor via AsmCpuid() and AsmReadMsr64() calls respectively.
> 
> At that moment, this approach worked fine for APs. Since:
> a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
>    entry at PEI phase), the function InitializeApData() will be called for
>    each AP and the CPUID and PlatformID information will be collected.
> 
> b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
>    APs are waken up again, the function InitializeApData() will not be
>    called, which means the CPUID and PlatformID information will not be
>    collected. However, the below logics in MicrocodeDetect() function:
> 
>   CurrentRevision = GetCurrentMicrocodeSignature ();
>   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> TRUE : FALSE;
>   if (CurrentRevision != 0 && !IsBspCallIn) {
>     //
>     // Skip loading microcode if it has been loaded successfully
>     //
>     return;
>   }
> 
>    will ensure that the microcode detection and application will be
>    skipped due to the fact that such process has already been done in the
>    PEI phase.
> 
> But after commit 396e791059, which removes the above skip loading logic,
> the CPUID and PlatformID information on APs will be used upon the 2nd
> entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> information has not been collected, it will bring issue to the microcode
> detection process.
> 
> This commit will update the logic in MicrocodeDetect() back to always
> collecting the CPUID and PlatformID information explicitly.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index b6675b9a60..67e214d463 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -93,6 +93,7 @@ MicrocodeDetect (
>    UINT32                                  InCompleteCheckSum32;
>    BOOLEAN                                 CorrectMicrocode;
>    VOID                                    *MicrocodeData;
> +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
>    UINT32                                  ThreadId;
>    BOOLEAN                                 IsBspCallIn;
> 
> @@ -115,8 +116,18 @@ MicrocodeDetect (
>    }
> 
>    ExtendedTableLength = 0;
> -  Eax.Uint32 = CpuMpData-
> >CpuData[ProcessorNumber].ProcessorSignature;
> -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> +  //
> +  // Here data of CPUID leafs have not been collected into context
> + buffer, so  // GetProcessorCpuid() cannot be used here to retrieve CPUID
> data.
> +  //
> +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +  //
> +  // The index of platform information resides in bits 50:52 of MSR
> + IA32_PLATFORM_ID  //
> +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> + PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> +
> 
>    //
>    // Check whether AP has same processor with BSP.
> --
> 2.12.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53836): https://edk2.groups.io/g/devel/message/53836
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
Posted by Ni, Ray 4 years, 1 month ago
Hao,
I submitted a new Bugzilla to capture some potential enhancement to this patch.
https://bugzilla.tianocore.org/show_bug.cgi?id=2519

Your patch can fix the issue, but I think we need some cleanup to avoid potential bugs in future.

Thanks,
Ray

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, February 6, 2020 9:40 AM
> To: Fu, Siyuan <siyuan.fu@intel.com>; Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
> 
> Thanks Siyuan and Eric,
> 
> The patch has been pushed via commit a9e3458ba7.
> 
> Best Regards,
> Hao Wu
> 
> 
> > -----Original Message-----
> > From: Dong, Eric
> > Sent: Tuesday, February 04, 2020 9:47 PM
> > To: devel@edk2.groups.io; Wu, Hao A
> > Cc: Ni, Ray; Laszlo Ersek; Fu, Siyuan; Kinney, Michael D
> > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get
> > CPUID & PlatformID in MicrocodeDetect()
> >
> > Reviewed-by: Eric Dong <eric.dong@intel.com>
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> > A
> > Sent: Monday, February 3, 2020 8:35 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID &
> > PlatformID in MicrocodeDetect()
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> >
> > Commit fd30b00707 updated the logic in function MicrocodeDetect() that
> > will directly use the CPUID and PlatformID information from the 'CpuData'
> > field in the CPU_MP_DATA structure, instead of collecting these information
> > for each processor via AsmCpuid() and AsmReadMsr64() calls respectively.
> >
> > At that moment, this approach worked fine for APs. Since:
> > a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
> >    entry at PEI phase), the function InitializeApData() will be called for
> >    each AP and the CPUID and PlatformID information will be collected.
> >
> > b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
> >    APs are waken up again, the function InitializeApData() will not be
> >    called, which means the CPUID and PlatformID information will not be
> >    collected. However, the below logics in MicrocodeDetect() function:
> >
> >   CurrentRevision = GetCurrentMicrocodeSignature ();
> >   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> > TRUE : FALSE;
> >   if (CurrentRevision != 0 && !IsBspCallIn) {
> >     //
> >     // Skip loading microcode if it has been loaded successfully
> >     //
> >     return;
> >   }
> >
> >    will ensure that the microcode detection and application will be
> >    skipped due to the fact that such process has already been done in the
> >    PEI phase.
> >
> > But after commit 396e791059, which removes the above skip loading logic,
> > the CPUID and PlatformID information on APs will be used upon the 2nd
> > entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> > information has not been collected, it will bring issue to the microcode
> > detection process.
> >
> > This commit will update the logic in MicrocodeDetect() back to always
> > collecting the CPUID and PlatformID information explicitly.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index b6675b9a60..67e214d463 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -93,6 +93,7 @@ MicrocodeDetect (
> >    UINT32                                  InCompleteCheckSum32;
> >    BOOLEAN                                 CorrectMicrocode;
> >    VOID                                    *MicrocodeData;
> > +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
> >    UINT32                                  ThreadId;
> >    BOOLEAN                                 IsBspCallIn;
> >
> > @@ -115,8 +116,18 @@ MicrocodeDetect (
> >    }
> >
> >    ExtendedTableLength = 0;
> > -  Eax.Uint32 = CpuMpData-
> > >CpuData[ProcessorNumber].ProcessorSignature;
> > -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> > +  //
> > +  // Here data of CPUID leafs have not been collected into context
> > + buffer, so  // GetProcessorCpuid() cannot be used here to retrieve CPUID
> > data.
> > +  //
> > +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> > +
> > +  //
> > +  // The index of platform information resides in bits 50:52 of MSR
> > + IA32_PLATFORM_ID  //
> > +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> > + PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> > +
> >
> >    //
> >    // Check whether AP has same processor with BSP.
> > --
> > 2.12.0.windows.1
> >
> >
> > 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54194): https://edk2.groups.io/g/devel/message/54194
Mute This Topic: https://groups.io/mt/70934372/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-