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

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>