diff mbox series

[2/4] x86/ucode: Fix error paths in apply_microcode()

Message ID 20200320212453.21685-3-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/ucode: Cleanup - Part 2/n | expand

Commit Message

Andrew Cooper March 20, 2020, 9:24 p.m. UTC 
In the unlikley case that patch application completes, but the resutling
revision isn't expected, sig->rev doesn't get updated to match reality.

It will get adjusted the next time collect_cpu_info() gets called, but in the
meantime Xen might operate on a state value.  Nothing good will come of this.

Rewrite the logic to always update the stashed revision, before worring about
whether the attempt was a success or failure.

Take the opportunity to make the printk() messages as consistent as possible.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
-CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/amd.c   | 14 +++++++-------
 xen/arch/x86/cpu/microcode/intel.c | 22 +++++++++++-----------
 2 files changed, 18 insertions(+), 18 deletions(-)

Comments

Wei Liu March 21, 2020, 4:49 p.m. UTC | #1 
Actually CC Jan

On Fri, Mar 20, 2020 at 09:24:50PM +0000, Andrew Cooper wrote:
> In the unlikley case that patch application completes, but the resutling
> revision isn't expected, sig->rev doesn't get updated to match reality.
> 
> It will get adjusted the next time collect_cpu_info() gets called, but in the
> meantime Xen might operate on a state value.  Nothing good will come of this.

state -> stale

> 
> Rewrite the logic to always update the stashed revision, before worring about

worring -> worrying

> whether the attempt was a success or failure.
> 
> Take the opportunity to make the printk() messages as consistent as possible.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

> ---
> -CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpu/microcode/amd.c   | 14 +++++++-------
>  xen/arch/x86/cpu/microcode/intel.c | 22 +++++++++++-----------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> index d4b2874de6..a053e43923 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -229,11 +229,11 @@ static enum microcode_match_result compare_patch(
>  
>  static int apply_microcode(const struct microcode_patch *patch)
>  {
> -    uint32_t rev;
>      int hw_err;
>      unsigned int cpu = smp_processor_id();
>      struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>      const struct microcode_header_amd *hdr;
> +    uint32_t rev, old_rev = sig->rev;
>  
>      if ( !patch )
>          return -ENOENT;
> @@ -249,6 +249,7 @@ static int apply_microcode(const struct microcode_patch *patch)
>  
>      /* get patch id after patching */
>      rdmsrl(MSR_AMD_PATCHLEVEL, rev);
> +    sig->rev = rev;
>  
>      /*
>       * Some processors leave the ucode blob mapping as UC after the update.
> @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch)
>      /* check current patch id and patch's id for match */
>      if ( hw_err || (rev != hdr->patch_id) )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
> +        printk(XENLOG_ERR
> +               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
> +               cpu, old_rev, hdr->patch_id, rev);
>          return -EIO;
>      }
>  
> -    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
> -           cpu, sig->rev, hdr->patch_id);
> -
> -    sig->rev = rev;
> +    printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n",
> +           cpu, old_rev, hdr->patch_id);
>  
>      return 0;
>  }
> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
> index 5e9c2a9c7f..6ac5f98694 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -278,10 +278,10 @@ static enum microcode_match_result compare_patch(
>  static int apply_microcode(const struct microcode_patch *patch)
>  {
>      uint64_t msr_content;
> -    unsigned int val[2];
> -    unsigned int cpu_num = raw_smp_processor_id();
> +    unsigned int cpu = smp_processor_id();
>      struct cpu_signature *sig = &this_cpu(cpu_sig);
>      const struct microcode_intel *mc_intel;
> +    uint32_t rev, old_rev = sig->rev;
>  
>      if ( !patch )
>          return -ENOENT;
> @@ -302,20 +302,20 @@ static int apply_microcode(const struct microcode_patch *patch)
>  
>      /* get the current revision from MSR 0x8B */
>      rdmsrl(MSR_IA32_UCODE_REV, msr_content);
> -    val[1] = (uint32_t)(msr_content >> 32);
> +    sig->rev = rev = msr_content >> 32;
>  
> -    if ( val[1] != mc_intel->hdr.rev )
> +    if ( rev != mc_intel->hdr.rev )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
> -               sig->rev, mc_intel->hdr.rev, val[1]);
> +        printk(XENLOG_ERR
> +               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
> +               cpu, old_rev, mc_intel->hdr.rev, rev);
>          return -EIO;
>      }
> -    printk(KERN_INFO "microcode: CPU%d updated from revision "
> -           "%#x to %#x, date = %04x-%02x-%02x\n",
> -           cpu_num, sig->rev, val[1], mc_intel->hdr.year,
> +
> +    printk(XENLOG_WARNING
> +           "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n",
> +           cpu, old_rev, rev, mc_intel->hdr.year,
>             mc_intel->hdr.month, mc_intel->hdr.day);
> -    sig->rev = val[1];
>  
>      return 0;
>  }
> -- 
> 2.11.0
>
Jan Beulich March 23, 2020, 8:32 a.m. UTC | #2 
On 20.03.2020 22:24, Andrew Cooper wrote:
> @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch)
>      /* check current patch id and patch's id for match */
>      if ( hw_err || (rev != hdr->patch_id) )
>      {
> -        printk(KERN_ERR "microcode: CPU%d update from revision "
> -               "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
> +        printk(XENLOG_ERR
> +               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
> +               cpu, old_rev, hdr->patch_id, rev);
>          return -EIO;
>      }
>  
> -    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
> -           cpu, sig->rev, hdr->patch_id);
> -
> -    sig->rev = rev;
> +    printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n",
> +           cpu, old_rev, hdr->patch_id);

Prefer the local variable here over hdr->patch_id, just like you do
on the Intel side?

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index d4b2874de6..a053e43923 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -229,11 +229,11 @@  static enum microcode_match_result compare_patch(
 
 static int apply_microcode(const struct microcode_patch *patch)
 {
-    uint32_t rev;
     int hw_err;
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     const struct microcode_header_amd *hdr;
+    uint32_t rev, old_rev = sig->rev;
 
     if ( !patch )
         return -ENOENT;
@@ -249,6 +249,7 @@  static int apply_microcode(const struct microcode_patch *patch)
 
     /* get patch id after patching */
     rdmsrl(MSR_AMD_PATCHLEVEL, rev);
+    sig->rev = rev;
 
     /*
      * Some processors leave the ucode blob mapping as UC after the update.
@@ -259,15 +260,14 @@  static int apply_microcode(const struct microcode_patch *patch)
     /* check current patch id and patch's id for match */
     if ( hw_err || (rev != hdr->patch_id) )
     {
-        printk(KERN_ERR "microcode: CPU%d update from revision "
-               "%#x to %#x failed\n", cpu, rev, hdr->patch_id);
+        printk(XENLOG_ERR
+               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
+               cpu, old_rev, hdr->patch_id, rev);
         return -EIO;
     }
 
-    printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
-           cpu, sig->rev, hdr->patch_id);
-
-    sig->rev = rev;
+    printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n",
+           cpu, old_rev, hdr->patch_id);
 
     return 0;
 }
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 5e9c2a9c7f..6ac5f98694 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -278,10 +278,10 @@  static enum microcode_match_result compare_patch(
 static int apply_microcode(const struct microcode_patch *patch)
 {
     uint64_t msr_content;
-    unsigned int val[2];
-    unsigned int cpu_num = raw_smp_processor_id();
+    unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &this_cpu(cpu_sig);
     const struct microcode_intel *mc_intel;
+    uint32_t rev, old_rev = sig->rev;
 
     if ( !patch )
         return -ENOENT;
@@ -302,20 +302,20 @@  static int apply_microcode(const struct microcode_patch *patch)
 
     /* get the current revision from MSR 0x8B */
     rdmsrl(MSR_IA32_UCODE_REV, msr_content);
-    val[1] = (uint32_t)(msr_content >> 32);
+    sig->rev = rev = msr_content >> 32;
 
-    if ( val[1] != mc_intel->hdr.rev )
+    if ( rev != mc_intel->hdr.rev )
     {
-        printk(KERN_ERR "microcode: CPU%d update from revision "
-               "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
-               sig->rev, mc_intel->hdr.rev, val[1]);
+        printk(XENLOG_ERR
+               "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
+               cpu, old_rev, mc_intel->hdr.rev, rev);
         return -EIO;
     }
-    printk(KERN_INFO "microcode: CPU%d updated from revision "
-           "%#x to %#x, date = %04x-%02x-%02x\n",
-           cpu_num, sig->rev, val[1], mc_intel->hdr.year,
+
+    printk(XENLOG_WARNING
+           "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n",
+           cpu, old_rev, rev, mc_intel->hdr.year,
            mc_intel->hdr.month, mc_intel->hdr.day);
-    sig->rev = val[1];
 
     return 0;
 }