[PATCH 0/3] x86/ucode: Fixes and improvements to ucode revision handling

[PATCH 0/3] x86/ucode: Fixes and improvements to ucode revision handling

[Andrew Cooper]

Patch 2 fixes a bug with the Intel revision handling, which is causing
problems on IceLake SDPs.

Patch 3 adds ucode=allow-same to allow for sensible testing of the late
microcode loading path.

Andrew Cooper (3):
  x86/ucode: Break out compare_revisions() from existing infrastructure
  x86/ucode/intel: Fix handling of microcode revision
  x86/ucode: Introduce ucode=allow-same for testing purposes

 docs/misc/xen-command-line.pandoc    |  7 ++++++-
 xen/arch/x86/cpu/microcode/amd.c     | 25 ++++++++++++++-----------
 xen/arch/x86/cpu/microcode/core.c    |  4 ++++
 xen/arch/x86/cpu/microcode/intel.c   | 31 +++++++++++++++++++++++++++----
 xen/arch/x86/cpu/microcode/private.h |  2 ++
 5 files changed, 53 insertions(+), 16 deletions(-)

-- 
2.11.0

[PATCH 1/3] x86/ucode: Break out compare_revisions() from existing infrastructure

[Andrew Cooper]

Drop some unnecesserily verbose pr_debug()'s on the AMD side.

No functional change.

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

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index e80f7cd3e4..7d2f57c4cb 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -168,6 +168,15 @@ static bool check_final_patch_levels(const struct cpu_signature *sig)
     return false;
 }
 
+static enum microcode_match_result compare_revisions(
+    uint32_t old_rev, uint32_t new_rev)
+{
+    if ( new_rev > old_rev )
+        return NEW_UCODE;
+
+    return OLD_UCODE;
+}
+
 static enum microcode_match_result microcode_fits(
     const struct microcode_patch *patch)
 {
@@ -178,16 +187,7 @@ static enum microcode_match_result microcode_fits(
          equiv.id  != patch->processor_rev_id )
         return MIS_UCODE;
 
-    if ( patch->patch_id <= sig->rev )
-    {
-        pr_debug("microcode: patch is already at required level or greater.\n");
-        return OLD_UCODE;
-    }
-
-    pr_debug("microcode: CPU%d found a matching microcode update with version %#x (current=%#x)\n",
-             cpu, patch->patch_id, sig->rev);
-
-    return NEW_UCODE;
+    return compare_revisions(sig->rev, patch->patch_id);
 }
 
 static enum microcode_match_result compare_header(
@@ -196,7 +196,7 @@ static enum microcode_match_result compare_header(
     if ( new->processor_rev_id != old->processor_rev_id )
         return MIS_UCODE;
 
-    return new->patch_id > old->patch_id ? NEW_UCODE : OLD_UCODE;
+    return compare_revisions(old->patch_id, new->patch_id);
 }
 
 static enum microcode_match_result compare_patch(
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 72c07fcd1d..e1ccb5d232 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -222,6 +222,15 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
     return 0;
 }
 
+static enum microcode_match_result compare_revisions(
+    uint32_t old_rev, uint32_t new_rev)
+{
+    if ( new_rev > old_rev )
+        return NEW_UCODE;
+
+    return OLD_UCODE;
+}
+
 /* Check an update against the CPU signature and current update revision */
 static enum microcode_match_result microcode_update_match(
     const struct microcode_patch *mc)
@@ -245,7 +254,7 @@ static enum microcode_match_result microcode_update_match(
     return MIS_UCODE;
 
  found:
-    return mc->rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
+    return compare_revisions(cpu_sig->rev, mc->rev);
 }
 
 static enum microcode_match_result compare_patch(
@@ -258,7 +267,7 @@ static enum microcode_match_result compare_patch(
     ASSERT(microcode_update_match(old) != MIS_UCODE);
     ASSERT(microcode_update_match(new) != MIS_UCODE);
 
-    return new->rev > old->rev ? NEW_UCODE : OLD_UCODE;
+    return compare_revisions(old->rev, new->rev);
 }
 
 static int apply_microcode(const struct microcode_patch *patch)
@@ -332,7 +341,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
-             (!saved || (mc->rev > saved->rev)) )
+             (!saved || compare_revisions(saved->rev, mc->rev) == NEW_UCODE) )
             saved = mc;
 
         buf  += blob_size;
-- 
2.11.0

Re: [PATCH 1/3] x86/ucode: Break out compare_revisions() from existing infrastructure

[Jan Beulich]

On 26.10.2020 18:25, Andrew Cooper wrote:
> Drop some unnecesserily verbose pr_debug()'s on the AMD side.

To be honest I'm not entirely convinced of this part of the change:
For one, pr_debug() expands to nothing by default. And then you
delete 2/3 of all pr_debug() instances, putting under question why
there's a pr_debug() in the first place.

> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Only after having looked at the subsequent patches to understand
how this is going to be useful:
Acked-by: Jan Beulich <jbeulich@suse.com>
However, ...

> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -168,6 +168,15 @@ static bool check_final_patch_levels(const struct cpu_signature *sig)
>      return false;
>  }
>  
> +static enum microcode_match_result compare_revisions(
> +    uint32_t old_rev, uint32_t new_rev)

... this (and the respective Intel code) is another good example
where, by our present guide lines, fixed width types aren't
appropriate to use. "unsigned int" (and in the later patch plain
"int" or "signed int") will fulfill the purpose, and hence ought
to be preferred.

Jan

[PATCH 2/3] x86/ucode/intel: Fix handling of microcode revision

[Andrew Cooper]

For Intel microcodes, the revision field is signed (as documented in the SDM)
and negative revisions are used for pre-production/test microcode (not
documented publicly anywhere I can spot).

Adjust the revision checking to match the algorithm presented here:

  https://software.intel.com/security-software-guidance/best-practices/microcode-update-guidance

This treats pre-production microcode as always applicable, but also production
microcode having higher precident than pre-production.  It is expected that
anyone using pre-production microcode knows what they are doing.

This is necessary to load production microcode on an SDP with pre-production
microcode embedded in firmware.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Igor Druzhinin <igor.druzhinin@citrix.com>

"signed" is somewhat of a problem.  The actual numbers only make sense as
sign-magnitude, and not as twos-compliement, which is why the rule is "any
debug microcode goes".

The actual upgrade/downgrade rules are quite complicated, but in general, any
change is permitted so long as the Security Version Number (embedded inside
the patch) doesn't go backwards.

---
 xen/arch/x86/cpu/microcode/intel.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index e1ccb5d232..5fa2821cdb 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -33,7 +33,7 @@
 
 struct microcode_patch {
     uint32_t hdrver;
-    uint32_t rev;
+    int32_t rev;
     uint16_t year;
     uint8_t  day;
     uint8_t  month;
@@ -222,12 +222,23 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
     return 0;
 }
 
+/*
+ * Production microcode has a positive revision.  Pre-production microcode has
+ * a negative revision.
+ */
 static enum microcode_match_result compare_revisions(
-    uint32_t old_rev, uint32_t new_rev)
+    int32_t old_rev, int32_t new_rev)
 {
     if ( new_rev > old_rev )
         return NEW_UCODE;
 
+    /*
+     * Treat pre-production as always applicable - anyone using pre-production
+     * microcode knows what they are doing, and can keep any resulting pieces.
+     */
+    if ( new_rev < 0 )
+        return NEW_UCODE;
+
     return OLD_UCODE;
 }
 
-- 
2.11.0

Re: [PATCH 2/3] x86/ucode/intel: Fix handling of microcode revision

[Jan Beulich]

On 26.10.2020 18:25, Andrew Cooper wrote:
> For Intel microcodes, the revision field is signed (as documented in the SDM)
> and negative revisions are used for pre-production/test microcode (not
> documented publicly anywhere I can spot).
> 
> Adjust the revision checking to match the algorithm presented here:
> 
>   https://software.intel.com/security-software-guidance/best-practices/microcode-update-guidance
> 
> This treats pre-production microcode as always applicable, but also production
> microcode having higher precident than pre-production.  It is expected that

Nit: "precedence" I guess?

> anyone using pre-production microcode knows what they are doing.
> 
> This is necessary to load production microcode on an SDP with pre-production
> microcode embedded in firmware.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH 3/3] x86/ucode: Introduce ucode=allow-same for testing purposes

[Andrew Cooper]

Many CPUs will actually reload microcode when offered the same version as
currently loaded.  This allows for easy testing of the late microcode loading
path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Igor Druzhinin <igor.druzhinin@citrix.com>

I was hoping to make this a runtime parameter, but I honestly can't figure out
the new HYPFS-only infrastructure is supposed to work.
---
 docs/misc/xen-command-line.pandoc    | 7 ++++++-
 xen/arch/x86/cpu/microcode/amd.c     | 3 +++
 xen/arch/x86/cpu/microcode/core.c    | 4 ++++
 xen/arch/x86/cpu/microcode/intel.c   | 3 +++
 xen/arch/x86/cpu/microcode/private.h | 2 ++
 5 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 4ae9391fcd..97b1cc67a4 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2216,7 +2216,7 @@ logic applies:
    active by default.
 
 ### ucode
-> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
+> `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
 
     Applicability: x86
     Default: `nmi`
@@ -2248,6 +2248,11 @@ precedence over `scan`.
 stop_machine context. In NMI handler, even NMIs are blocked, which is
 considered safer. The default value is `true`.
 
+'allow-same' alters the default acceptance policy for new microcode to permit
+trying to reload the same version.  Many CPUs will actually reload microcode
+of the same version, and this allows for easily testing of the late microcode
+loading path.
+
 ### unrestricted_guest (Intel)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 7d2f57c4cb..5255028af7 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -174,6 +174,9 @@ static enum microcode_match_result compare_revisions(
     if ( new_rev > old_rev )
         return NEW_UCODE;
 
+    if ( opt_ucode_allow_same && new_rev == old_rev )
+        return NEW_UCODE;
+
     return OLD_UCODE;
 }
 
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 18ebc07b13..ac3ceb567c 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -95,6 +95,8 @@ static bool_t __initdata ucode_scan;
 /* By default, ucode loading is done in NMI handler */
 static bool ucode_in_nmi = true;
 
+bool __read_mostly opt_ucode_allow_same;
+
 /* Protected by microcode_mutex */
 static struct microcode_patch *microcode_cache;
 
@@ -121,6 +123,8 @@ static int __init parse_ucode(const char *s)
 
         if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
             ucode_in_nmi = val;
+        else if ( (val = parse_boolean("allow-same", s, ss)) >= 0 )
+            opt_ucode_allow_same = val;
         else if ( !ucode_mod_forced ) /* Not forced by EFI */
         {
             if ( (val = parse_boolean("scan", s, ss)) >= 0 )
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 5fa2821cdb..f6d01490e0 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -232,6 +232,9 @@ static enum microcode_match_result compare_revisions(
     if ( new_rev > old_rev )
         return NEW_UCODE;
 
+    if ( opt_ucode_allow_same && new_rev == old_rev )
+        return NEW_UCODE;
+
     /*
      * Treat pre-production as always applicable - anyone using pre-production
      * microcode knows what they are doing, and can keep any resulting pieces.
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 9a15cdc879..c085a10268 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -3,6 +3,8 @@
 
 #include <asm/microcode.h>
 
+extern bool opt_ucode_allow_same;
+
 enum microcode_match_result {
     OLD_UCODE, /* signature matched, but revision id is older or equal */
     NEW_UCODE, /* signature matched, but revision id is newer */
-- 
2.11.0

Re: [PATCH 3/3] x86/ucode: Introduce ucode=allow-same for testing purposes

[Jan Beulich]

On 26.10.2020 18:25, Andrew Cooper wrote:
> Many CPUs will actually reload microcode when offered the same version as
> currently loaded.  This allows for easy testing of the late microcode loading
> path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one nit:

> @@ -2248,6 +2248,11 @@ precedence over `scan`.
>  stop_machine context. In NMI handler, even NMIs are blocked, which is
>  considered safer. The default value is `true`.
>  
> +'allow-same' alters the default acceptance policy for new microcode to permit
> +trying to reload the same version.  Many CPUs will actually reload microcode
> +of the same version, and this allows for easily testing of the late microcode
> +loading path.

Either "easy" or drop "of"?

Jan

Re: [PATCH 3/3] x86/ucode: Introduce ucode=allow-same for testing purposes

[Jürgen Groß]

On 26.10.20 18:25, Andrew Cooper wrote:
> Many CPUs will actually reload microcode when offered the same version as
> currently loaded.  This allows for easy testing of the late microcode loading
> path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Juergen Gross <jgross@suse.com>
> CC: Igor Druzhinin <igor.druzhinin@citrix.com>
> 
> I was hoping to make this a runtime parameter, but I honestly can't figure out
> the new HYPFS-only infrastructure is supposed to work.

For your use case have a look into xen/arch/x86/hvm/vmx/vmcs.c how the
"ept" runtime parameter is handled.

This is a similar case where one sub-option can be modified at runtime.


Juergen