Re: [edk2] [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode signature not matched

[Mike Maslenkin]

On Sat, 2016-07-02 at 02:58 +, Fan, Jeff wrote:
> Mike,
> 
> Parameter - PeiCpuMpData is added for MicrocodeDetect() in this patch.
> PeiCpuMpData->MpLock is used to avoid debug message corruption when Aps 
> output debug message in parallel mode. 
> 
Thank you. This is a thing I wanted to understand.

> I don't understand why the patch showed in your mail has not the following 
> part of patch. It's maybe the reason of your concern.
> 
I just left the hunk I asked a question about. A quote of complete patch
was not required there.

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode signature not matched

[Fan, Jeff]

Mike,

Parameter - PeiCpuMpData is added for MicrocodeDetect() in this patch.
PeiCpuMpData->MpLock is used to avoid debug message corruption when Aps output 
debug message in parallel mode. 

I don't understand why the patch showed in your mail has not the following part 
of patch. It's maybe the reason of your concern.

diff --git a/UefiCpuPkg/CpuMpPei/Microcode.c b/UefiCpuPkg/CpuMpPei/Microcode.c 
index 8e7f3b0..51a0737 100644
--- a/UefiCpuPkg/CpuMpPei/Microcode.c
+++ b/UefiCpuPkg/CpuMpPei/Microcode.c
@@ -36,10 +36,11 @@ GetCurrentMicrocodeSignature (
 /**
   Detect whether specified processor can find matching microcode patch and 
load it.
 
+  @param PeiCpuMpDataPointer to PEI CPU MP Data
 **/
 VOID
 MicrocodeDetect (
-  VOID
+  IN PEI_CPU_MP_DATA*PeiCpuMpData
   )
 {
   UINT64  MicrocodePatchAddress;
@@ -187,25 +188,29 @@ MicrocodeDetect (

Thanks!
Jeff
-Original Message-
From: Mike Maslenkin [mailto:mike.maslen...@gmail.com] 
Sent: Saturday, July 02, 2016 6:42 AM
To: Fan, Jeff
Cc: edk2-de...@ml01.01.org; Kinney, Michael D; Tian, Feng
Subject: Re: [edk2] [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode 
signature not matched

Just curious why locking is required here?
Looks like some operator missed.

On Fri, 2016-07-01 at 15:02 +0800, Jeff Fan wrote:
> diff --git a/UefiCpuPkg/CpuMpPei/Microcode.c 
> b/UefiCpuPkg/CpuMpPei/Microcode.c index 8e7f3b0..51a0737 100644
> --- a/UefiCpuPkg/CpuMpPei/Microcode.c
> +++ b/UefiCpuPkg/CpuMpPei/Microcode.c
> @@ -187,25 +188,29 @@ MicrocodeDetect (
>  MicrocodeEntryPoint = (EFI_CPU_MICROCODE_HEADER *) (((UINTN) 
> MicrocodeEntryPoint) + TotalSize);
>} while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
>  
> -  if (LatestRevision > 0) {
> +  if (LatestRevision > CurrentRevision) {
>  //
>  // BIOS only authenticate updates that contain a numerically larger 
> revision
>  // than the currently loaded revision, where Current Signature < New 
> Update
>  // Revision. A processor with no loaded update is considered to have a
>  // revision equal to zero.
>  //
> -if (LatestRevision > GetCurrentMicrocodeSignature ()) {
> -  AsmWriteMsr64 (
> -EFI_MSR_IA32_BIOS_UPDT_TRIG,
> -(UINT64) (UINTN) MicrocodeInfo.MicrocodeData
> -);
> -  //
> -  // Get and verify new microcode signature
> -  //
> -  ASSERT (LatestRevision == GetCurrentMicrocodeSignature ());
> -  MicrocodeInfo.Load = TRUE;
> -} else {
> -  MicrocodeInfo.Load = FALSE;
> +AsmWriteMsr64 (
> +  EFI_MSR_IA32_BIOS_UPDT_TRIG,
> +  (UINT64) (UINTN) MicrocodeInfo.MicrocodeData
> +  );
> +//
> +// Get and check new microcode signature
> +//
> +CurrentRevision = GetCurrentMicrocodeSignature ();
> +if (CurrentRevision != LatestRevision) {
> +  AcquireSpinLock(>MpLock);
> +  DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not 
> match \
> +loaded microcode signature [0x%08x]\n", CurrentRevision, 
> LatestRevision));
> +  ReleaseSpinLock(>MpLock);
>  }
> +MicrocodeInfo.Load = TRUE;
> +  } else {
> +MicrocodeInfo.Load = FALSE;
>}
>  }


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode signature not matched

[Mike Maslenkin]

Just curious why locking is required here?
Looks like some operator missed.

On Fri, 2016-07-01 at 15:02 +0800, Jeff Fan wrote:
> diff --git a/UefiCpuPkg/CpuMpPei/Microcode.c b/UefiCpuPkg/CpuMpPei/Microcode.c
> index 8e7f3b0..51a0737 100644
> --- a/UefiCpuPkg/CpuMpPei/Microcode.c
> +++ b/UefiCpuPkg/CpuMpPei/Microcode.c
> @@ -187,25 +188,29 @@ MicrocodeDetect (
>  MicrocodeEntryPoint = (EFI_CPU_MICROCODE_HEADER *) (((UINTN) 
> MicrocodeEntryPoint) + TotalSize);
>} while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
>  
> -  if (LatestRevision > 0) {
> +  if (LatestRevision > CurrentRevision) {
>  //
>  // BIOS only authenticate updates that contain a numerically larger 
> revision
>  // than the currently loaded revision, where Current Signature < New 
> Update
>  // Revision. A processor with no loaded update is considered to have a
>  // revision equal to zero.
>  //
> -if (LatestRevision > GetCurrentMicrocodeSignature ()) {
> -  AsmWriteMsr64 (
> -EFI_MSR_IA32_BIOS_UPDT_TRIG,
> -(UINT64) (UINTN) MicrocodeInfo.MicrocodeData
> -);
> -  //
> -  // Get and verify new microcode signature
> -  //
> -  ASSERT (LatestRevision == GetCurrentMicrocodeSignature ());
> -  MicrocodeInfo.Load = TRUE;
> -} else {
> -  MicrocodeInfo.Load = FALSE;
> +AsmWriteMsr64 (
> +  EFI_MSR_IA32_BIOS_UPDT_TRIG,
> +  (UINT64) (UINTN) MicrocodeInfo.MicrocodeData
> +  );
> +//
> +// Get and check new microcode signature
> +//
> +CurrentRevision = GetCurrentMicrocodeSignature ();
> +if (CurrentRevision != LatestRevision) {
> +  AcquireSpinLock(>MpLock);
> +  DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not 
> match \
> +loaded microcode signature [0x%08x]\n", CurrentRevision, 
> LatestRevision));
> +  ReleaseSpinLock(>MpLock);
>  }
> +MicrocodeInfo.Load = TRUE;
> +  } else {
> +MicrocodeInfo.Load = FALSE;
>}
>  }


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode signature not matched

[Mudusuru, Giri P]

Thanks Jeff, 
Please use @param[in] instead of @param.

Reviewed-by: Giri P Mudusuru  

> -Original Message-
> From: Fan, Jeff
> Sent: Friday, July 1, 2016 12:02 AM
> To: edk2-de...@ml01.01.org
> Cc: Kinney, Michael D ; Tian, Feng
> ; Mudusuru, Giri P 
> Subject: [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode
> signature not matched
> 
> Verification microcode signature is one enhancement and not one
> requirement from
> IA32 SDM. This update is just to dump debug message instead of ASSERT() if
> the
> updated microcode signature does not match the loaded microcode
> signature.
> 
> Cc: Michael Kinney 
> Cc: Feng Tian 
> Cc: Giri P Mudusuru 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan 
> ---
>  UefiCpuPkg/CpuMpPei/CpuMpPei.c  |  4 ++--
>  UefiCpuPkg/CpuMpPei/CpuMpPei.h  | 10 ++
>  UefiCpuPkg/CpuMpPei/Microcode.c | 33 +++
> --
>  UefiCpuPkg/CpuMpPei/Microcode.h |  9 -
>  4 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> index 4ed1da9..bccff24 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> @@ -302,7 +302,7 @@ ApCFunction (
>// Sync BSP's Mtrr table to all wakeup APs and load microcode on APs.
>//
>MtrrSetAllMtrrs (>MtrrTable);
> -  MicrocodeDetect ();
> +  MicrocodeDetect (PeiCpuMpData);
>PeiCpuMpData->CpuData[ProcessorNumber].State = CpuStateIdle;
>  } else {
>//
> @@ -624,7 +624,7 @@ CountProcessorNumber (
>//
>// Load Microcode on BSP
>//
> -  MicrocodeDetect ();
> +  MicrocodeDetect (PeiCpuMpData);
>//
>// Store BSP's MTRR setting
>//
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> index afbcb6e..5e56934 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> @@ -321,4 +321,14 @@ SecPlatformInformation2 (
>   OUT EFI_SEC_PLATFORM_INFORMATION_RECORD2
> *PlatformInformationRecord2
>);
> 
> +/**
> +  Detect whether specified processor can find matching microcode patch
> and load it.
> +
> +  @param PeiCpuMpDataPointer to PEI CPU MP Data
> +**/
> +VOID
> +MicrocodeDetect (
> +  IN PEI_CPU_MP_DATA*PeiCpuMpData
> +  );
> +
>  #endif
> diff --git a/UefiCpuPkg/CpuMpPei/Microcode.c
> b/UefiCpuPkg/CpuMpPei/Microcode.c
> index 8e7f3b0..51a0737 100644
> --- a/UefiCpuPkg/CpuMpPei/Microcode.c
> +++ b/UefiCpuPkg/CpuMpPei/Microcode.c
> @@ -36,10 +36,11 @@ GetCurrentMicrocodeSignature (
>  /**
>Detect whether specified processor can find matching microcode patch and
> load it.
> 
> +  @param PeiCpuMpDataPointer to PEI CPU MP Data
>  **/
>  VOID
>  MicrocodeDetect (
> -  VOID
> +  IN PEI_CPU_MP_DATA*PeiCpuMpData
>)
>  {
>UINT64  MicrocodePatchAddress;
> @@ -187,25 +188,29 @@ MicrocodeDetect (
>  MicrocodeEntryPoint = (EFI_CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + TotalSize);
>} while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> 
> -  if (LatestRevision > 0) {
> +  if (LatestRevision > CurrentRevision) {
>  //
>  // BIOS only authenticate updates that contain a numerically larger 
> revision
>  // than the currently loaded revision, where Current Signature < New
> Update
>  // Revision. A processor with no loaded update is considered to have a
>  // revision equal to zero.
>  //
> -if (LatestRevision > GetCurrentMicrocodeSignature ()) {
> -  AsmWriteMsr64 (
> -EFI_MSR_IA32_BIOS_UPDT_TRIG,
> -(UINT64) (UINTN) MicrocodeInfo.MicrocodeData
> -);
> -  //
> -  // Get and verify new microcode signature
> -  //
> -  ASSERT (LatestRevision == GetCurrentMicrocodeSignature ());
> -  MicrocodeInfo.Load = TRUE;
> -} else {
> -  MicrocodeInfo.Load = FALSE;
> +AsmWriteMsr64 (
> +  EFI_MSR_IA32_BIOS_UPDT_TRIG,
> +  (UINT64) (UINTN) MicrocodeInfo.MicrocodeData
> +  );
> +//
> +// Get and check new microcode signature
> +//
> +CurrentRevision = GetCurrentMicrocodeSignature ();
> +if (CurrentRevision != LatestRevision) {
> +  AcquireSpinLock(>MpLock);
> +  DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does
> not match \
> +loaded microcode signature [0x%08x]\n", CurrentRevision,
> LatestRevision));
> +  ReleaseSpinLock(>MpLock);
>  }
> +MicrocodeInfo.Load = TRUE;
> +  } else {
> +MicrocodeInfo.Load = FALSE;
>}
>  }
> diff --git a/UefiCpuPkg/CpuMpPei/Microcode.h
> b/UefiCpuPkg/CpuMpPei/Microcode.h
> index ea68669..f7d23a0 100644
> --- a/UefiCpuPkg/CpuMpPei/Microcode.h
> +++ 

[edk2] [Patch 2/3] UefiCpuPkg/CpuMpPei: Dump message if microcode signature not matched

[Jeff Fan]

Verification microcode signature is one enhancement and not one requirement from
IA32 SDM. This update is just to dump debug message instead of ASSERT() if the
updated microcode signature does not match the loaded microcode signature.

Cc: Michael Kinney 
Cc: Feng Tian 
Cc: Giri P Mudusuru 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c  |  4 ++--
 UefiCpuPkg/CpuMpPei/CpuMpPei.h  | 10 ++
 UefiCpuPkg/CpuMpPei/Microcode.c | 33 +++--
 UefiCpuPkg/CpuMpPei/Microcode.h |  9 -
 4 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index 4ed1da9..bccff24 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -302,7 +302,7 @@ ApCFunction (
   // Sync BSP's Mtrr table to all wakeup APs and load microcode on APs.
   //
   MtrrSetAllMtrrs (>MtrrTable);
-  MicrocodeDetect ();
+  MicrocodeDetect (PeiCpuMpData);
   PeiCpuMpData->CpuData[ProcessorNumber].State = CpuStateIdle;
 } else {
   //
@@ -624,7 +624,7 @@ CountProcessorNumber (
   //
   // Load Microcode on BSP
   //
-  MicrocodeDetect ();
+  MicrocodeDetect (PeiCpuMpData);
   //
   // Store BSP's MTRR setting
   //
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
index afbcb6e..5e56934 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
@@ -321,4 +321,14 @@ SecPlatformInformation2 (
  OUT EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2
   );
 
+/**
+  Detect whether specified processor can find matching microcode patch and 
load it.
+
+  @param PeiCpuMpDataPointer to PEI CPU MP Data
+**/
+VOID
+MicrocodeDetect (
+  IN PEI_CPU_MP_DATA*PeiCpuMpData
+  );
+
 #endif
diff --git a/UefiCpuPkg/CpuMpPei/Microcode.c b/UefiCpuPkg/CpuMpPei/Microcode.c
index 8e7f3b0..51a0737 100644
--- a/UefiCpuPkg/CpuMpPei/Microcode.c
+++ b/UefiCpuPkg/CpuMpPei/Microcode.c
@@ -36,10 +36,11 @@ GetCurrentMicrocodeSignature (
 /**
   Detect whether specified processor can find matching microcode patch and 
load it.
 
+  @param PeiCpuMpDataPointer to PEI CPU MP Data
 **/
 VOID
 MicrocodeDetect (
-  VOID
+  IN PEI_CPU_MP_DATA*PeiCpuMpData
   )
 {
   UINT64  MicrocodePatchAddress;
@@ -187,25 +188,29 @@ MicrocodeDetect (
 MicrocodeEntryPoint = (EFI_CPU_MICROCODE_HEADER *) (((UINTN) 
MicrocodeEntryPoint) + TotalSize);
   } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
 
-  if (LatestRevision > 0) {
+  if (LatestRevision > CurrentRevision) {
 //
 // BIOS only authenticate updates that contain a numerically larger 
revision
 // than the currently loaded revision, where Current Signature < New Update
 // Revision. A processor with no loaded update is considered to have a
 // revision equal to zero.
 //
-if (LatestRevision > GetCurrentMicrocodeSignature ()) {
-  AsmWriteMsr64 (
-EFI_MSR_IA32_BIOS_UPDT_TRIG,
-(UINT64) (UINTN) MicrocodeInfo.MicrocodeData
-);
-  //
-  // Get and verify new microcode signature
-  //
-  ASSERT (LatestRevision == GetCurrentMicrocodeSignature ());
-  MicrocodeInfo.Load = TRUE;
-} else {
-  MicrocodeInfo.Load = FALSE;
+AsmWriteMsr64 (
+  EFI_MSR_IA32_BIOS_UPDT_TRIG,
+  (UINT64) (UINTN) MicrocodeInfo.MicrocodeData
+  );
+//
+// Get and check new microcode signature
+//
+CurrentRevision = GetCurrentMicrocodeSignature ();
+if (CurrentRevision != LatestRevision) {
+  AcquireSpinLock(>MpLock);
+  DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not 
match \
+loaded microcode signature [0x%08x]\n", CurrentRevision, 
LatestRevision));
+  ReleaseSpinLock(>MpLock);
 }
+MicrocodeInfo.Load = TRUE;
+  } else {
+MicrocodeInfo.Load = FALSE;
   }
 }
diff --git a/UefiCpuPkg/CpuMpPei/Microcode.h b/UefiCpuPkg/CpuMpPei/Microcode.h
index ea68669..f7d23a0 100644
--- a/UefiCpuPkg/CpuMpPei/Microcode.h
+++ b/UefiCpuPkg/CpuMpPei/Microcode.h
@@ -56,13 +56,4 @@ typedef struct {
   UINT32  ProcessorChecksum;
 } EFI_CPU_MICROCODE_EXTENDED_TABLE;
 
-/**
-  Detect whether specified processor can find matching microcode patch and 
load it.
-
-**/
-VOID
-MicrocodeDetect (
-  VOID
-  );
-
 #endif
-- 
2.7.4.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel