[Xen-devel] [PATCH v2 0/7] x86/ucode: Cleanup and fixes - Part 3/n (Intel)

[Xen-devel] [PATCH v2 0/7] x86/ucode: Cleanup and fixes - Part 3/n (Intel)

[Andrew Cooper]

This supercedes the remnants of the Part 1 series, using Jan's suggested
alternative for making struct microcode_patch opaque.

Andrew Cooper (7):
  x86/ucode: Remove unnecessary indirection in struct microcode_patch
  x86/ucode/intel: Adjust microcode_sanity_check() to not take void *
  x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()
  x86/ucode/intel: Reimplement get_{data,total}size() helpers
  x86/ucode/intel: Clean up microcode_update_match()
  x86/ucode/intel: Clean up microcode_sanity_check()
  x86/ucode/intel: Fold structures together

 xen/arch/x86/cpu/microcode/amd.c     |  30 ++-
 xen/arch/x86/cpu/microcode/core.c    |   3 +-
 xen/arch/x86/cpu/microcode/intel.c   | 362 +++++++++++++++++------------------
 xen/arch/x86/cpu/microcode/private.h |  11 +-
 4 files changed, 187 insertions(+), 219 deletions(-)

-- 
2.11.0

[Xen-devel] [PATCH v2 1/7] x86/ucode: Remove unnecessary indirection in struct microcode_patch

[Andrew Cooper]

Currently, each cpu_request_microcode() allocates a struct microcode_patch,
which is a single pointer to a separate allocated structure.  This is
wasteful.

Fixing this is complicated because the common microcode_free_patch() code is
responsible for freeing struct microcode_patch, despite this being asymmetric
with how it is allocated.

Make struct microcode_patch fully opaque to the common logic.  This involves
moving the responsibility for freeing struct microcode_patch fully into the
free_patch() hook.

In each vendor logic, use some temporary ifdef-ary (cleaned up in subsequent
changes) to reduce the churn as much as possible, and forgo allocating the
intermediate pointer in cpu_request_microcode().

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
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>

v2:
 * New
---
 xen/arch/x86/cpu/microcode/amd.c     | 30 ++++++++++++------------------
 xen/arch/x86/cpu/microcode/core.c    |  3 +--
 xen/arch/x86/cpu/microcode/intel.c   | 31 ++++++++++++-------------------
 xen/arch/x86/cpu/microcode/private.h | 11 +++--------
 4 files changed, 28 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 122b8309af..1b9373f0d9 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -60,13 +60,16 @@ struct __packed microcode_header_amd {
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
-struct microcode_amd {
+struct microcode_patch {
     void *mpb;
     size_t mpb_size;
     struct equiv_cpu_entry *equiv_cpu_table;
     size_t equiv_cpu_table_size;
 };
 
+/* Temporary, until the microcode_* structure are disentangled. */
+#define microcode_amd microcode_patch
+
 struct mpbhdr {
     uint32_t type;
     uint32_t len;
@@ -177,13 +180,11 @@ static enum microcode_match_result microcode_fits(
 
 static bool match_cpu(const struct microcode_patch *patch)
 {
-    return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE);
+    return patch && (microcode_fits(patch) == NEW_UCODE);
 }
 
-static void free_patch(void *mc)
+static void free_patch(struct microcode_patch *mc_amd)
 {
-    struct microcode_amd *mc_amd = mc;
-
     if ( mc_amd )
     {
         xfree(mc_amd->equiv_cpu_table);
@@ -206,12 +207,12 @@ static enum microcode_match_result compare_header(
 static enum microcode_match_result compare_patch(
     const struct microcode_patch *new, const struct microcode_patch *old)
 {
-    const struct microcode_header_amd *new_header = new->mc_amd->mpb;
-    const struct microcode_header_amd *old_header = old->mc_amd->mpb;
+    const struct microcode_header_amd *new_header = new->mpb;
+    const struct microcode_header_amd *old_header = old->mpb;
 
     /* Both patches to compare are supposed to be applicable to local CPU. */
-    ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
-    ASSERT(microcode_fits(old->mc_amd) != MIS_UCODE);
+    ASSERT(microcode_fits(new) != MIS_UCODE);
+    ASSERT(microcode_fits(old) != MIS_UCODE);
 
     return compare_header(new_header, old_header);
 }
@@ -230,7 +231,7 @@ static int apply_microcode(const struct microcode_patch *patch)
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    hdr = patch->mc_amd->mpb;
+    hdr = patch->mpb;
 
     BUG_ON(local_irq_is_enabled());
 
@@ -554,14 +555,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
     {
         mc_amd->mpb = saved;
         mc_amd->mpb_size = saved_size;
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-            patch->mc_amd = mc_amd;
-        else
-        {
-            free_patch(mc_amd);
-            error = -ENOMEM;
-        }
+        patch = mc_amd;
     }
     else
         free_patch(mc_amd);
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 61150e04c8..b3e5913d49 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -245,8 +245,7 @@ static struct microcode_patch *parse_blob(const char *buf, size_t len)
 
 static void microcode_free_patch(struct microcode_patch *microcode_patch)
 {
-    microcode_ops->free_patch(microcode_patch->mc);
-    xfree(microcode_patch);
+    microcode_ops->free_patch(microcode_patch);
 }
 
 /* Return true if cache gets updated. Otherwise, return false */
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 78455aa0ae..a69f7fe1de 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -52,11 +52,14 @@ struct microcode_header_intel {
     unsigned int reserved[3];
 };
 
-struct microcode_intel {
+struct microcode_patch {
     struct microcode_header_intel hdr;
     unsigned int bits[0];
 };
 
+/* Temporary, until the microcode_* structure are disentangled. */
+#define microcode_intel microcode_patch
+
 /* microcode format is extended from prescott processors */
 struct extended_signature {
     unsigned int sig;
@@ -245,12 +248,12 @@ static bool match_cpu(const struct microcode_patch *patch)
     if ( !patch )
         return false;
 
-    return microcode_update_match(&patch->mc_intel->hdr) == NEW_UCODE;
+    return microcode_update_match(&patch->hdr) == NEW_UCODE;
 }
 
-static void free_patch(void *mc)
+static void free_patch(struct microcode_patch *patch)
 {
-    xfree(mc);
+    xfree(patch);
 }
 
 static enum microcode_match_result compare_patch(
@@ -260,11 +263,10 @@ static enum microcode_match_result compare_patch(
      * Both patches to compare are supposed to be applicable to local CPU.
      * Just compare the revision number.
      */
-    ASSERT(microcode_update_match(&old->mc_intel->hdr) != MIS_UCODE);
-    ASSERT(microcode_update_match(&new->mc_intel->hdr) != MIS_UCODE);
+    ASSERT(microcode_update_match(&old->hdr) != MIS_UCODE);
+    ASSERT(microcode_update_match(&new->hdr) != MIS_UCODE);
 
-    return (new->mc_intel->hdr.rev > old->mc_intel->hdr.rev) ? NEW_UCODE
-                                                             : OLD_UCODE;
+    return (new->hdr.rev > old->hdr.rev) ? NEW_UCODE : OLD_UCODE;
 }
 
 static int apply_microcode(const struct microcode_patch *patch)
@@ -281,7 +283,7 @@ static int apply_microcode(const struct microcode_patch *patch)
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    mc_intel = patch->mc_intel;
+    mc_intel = patch;
 
     BUG_ON(local_irq_is_enabled());
 
@@ -372,16 +374,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
         error = offset;
 
     if ( saved )
-    {
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-            patch->mc_intel = saved;
-        else
-        {
-            xfree(saved);
-            error = -ENOMEM;
-        }
-    }
+        patch = saved;
 
     if ( error && !patch )
         patch = ERR_PTR(error);
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 230b935c94..df0d0852cd 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -11,13 +11,8 @@ enum microcode_match_result {
     MIS_UCODE, /* signature mismatched */
 };
 
-struct microcode_patch {
-    union {
-        struct microcode_intel *mc_intel;
-        struct microcode_amd *mc_amd;
-        void *mc;
-    };
-};
+/* Opaque.  Internals are vendor-specific. */
+struct microcode_patch;
 
 struct microcode_ops {
     /*
@@ -62,7 +57,7 @@ struct microcode_ops {
     void (*end_update_percpu)(void);
 
     /* Free a patch previously allocated by cpu_request_microcode(). */
-    void (*free_patch)(void *mc);
+    void (*free_patch)(struct microcode_patch *patch);
 
     /*
      * Is the microcode patch applicable for the current CPU, and newer than
-- 
2.11.0

[Xen-devel] [PATCH v2 2/7] x86/ucode/intel: Adjust microcode_sanity_check() to not take void *

[Andrew Cooper]

microcode_sanity_check()'s callers actually call it with a mixture of
microcode_intel(/patch) and microcode_header_intel pointers, which is fragile.

Rework it to take struct microcode_patch *, which in turn requires
microcode_update_match()'s type to be altered.

No functional change - compiled binary is identical.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over struct microcode_patch re-work
---
 xen/arch/x86/cpu/microcode/intel.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index a69f7fe1de..77539a00be 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -118,9 +118,9 @@ static int collect_cpu_info(struct cpu_signature *csig)
     return 0;
 }
 
-static int microcode_sanity_check(const void *mc)
+static int microcode_sanity_check(const struct microcode_patch *mc)
 {
-    const struct microcode_header_intel *mc_header = mc;
+    const struct microcode_header_intel *mc_header = &mc->hdr;
     const struct extended_sigtable *ext_header = NULL;
     const struct extended_signature *ext_sig;
     unsigned long total_size, data_size, ext_table_size;
@@ -152,7 +152,7 @@ static int microcode_sanity_check(const void *mc)
                    "Small exttable size in microcode data file\n");
             return -EINVAL;
         }
-        ext_header = mc + MC_HEADER_SIZE + data_size;
+        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
         if ( ext_table_size != exttable_size(ext_header) )
         {
             printk(KERN_ERR "microcode: error! "
@@ -210,8 +210,9 @@ static int microcode_sanity_check(const void *mc)
 
 /* Check an update against the CPU signature and current update revision */
 static enum microcode_match_result microcode_update_match(
-    const struct microcode_header_intel *mc_header)
+    const struct microcode_patch *mc)
 {
+    const struct microcode_header_intel *mc_header = &mc->hdr;
     const struct extended_sigtable *ext_header;
     const struct extended_signature *ext_sig;
     unsigned int i;
@@ -222,7 +223,7 @@ static enum microcode_match_result microcode_update_match(
     unsigned long data_size = get_datasize(mc_header);
     const void *end = (const void *)mc_header + get_totalsize(mc_header);
 
-    ASSERT(!microcode_sanity_check(mc_header));
+    ASSERT(!microcode_sanity_check(mc));
     if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
         return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
@@ -248,7 +249,7 @@ static bool match_cpu(const struct microcode_patch *patch)
     if ( !patch )
         return false;
 
-    return microcode_update_match(&patch->hdr) == NEW_UCODE;
+    return microcode_update_match(patch) == NEW_UCODE;
 }
 
 static void free_patch(struct microcode_patch *patch)
@@ -263,8 +264,8 @@ static enum microcode_match_result compare_patch(
      * Both patches to compare are supposed to be applicable to local CPU.
      * Just compare the revision number.
      */
-    ASSERT(microcode_update_match(&old->hdr) != MIS_UCODE);
-    ASSERT(microcode_update_match(&new->hdr) != MIS_UCODE);
+    ASSERT(microcode_update_match(old) != MIS_UCODE);
+    ASSERT(microcode_update_match(new) != MIS_UCODE);
 
     return (new->hdr.rev > old->hdr.rev) ? NEW_UCODE : OLD_UCODE;
 }
@@ -361,7 +362,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
          * If the new update covers current CPU, compare updates and store the
          * one with higher revision.
          */
-        if ( (microcode_update_match(&mc->hdr) != MIS_UCODE) &&
+        if ( (microcode_update_match(mc) != MIS_UCODE) &&
              (!saved || (mc->hdr.rev > saved->hdr.rev)) )
         {
             xfree(saved);
-- 
2.11.0

[Xen-devel] [PATCH v2 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()

[Andrew Cooper]

cpu_request_microcode() needs to scan its container and duplicate one blob,
but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
Furthermore, the length checking is only safe from overflow in 64bit builds.

Delete get_next_ucode_from_buffer() and alter the purpose of the saved
variable to simply point somewhere in buf until we're ready to return.

This is only a modest reduction in absolute code size (-144), but avoids
making memory allocations for every blob in the container.

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>

v2:
 * Rebase over struct microcode_patch re-work
 * Reinstate printk() for bad data
---
 xen/arch/x86/cpu/microcode/intel.c | 65 +++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 77539a00be..2b48959573 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -315,67 +315,52 @@ static int apply_microcode(const struct microcode_patch *patch)
     return 0;
 }
 
-static long get_next_ucode_from_buffer(struct microcode_intel **mc,
-                                       const uint8_t *buf, unsigned long size,
-                                       unsigned long offset)
-{
-    struct microcode_header_intel *mc_header;
-    unsigned long total_size;
-
-    /* No more data */
-    if ( offset >= size )
-        return 0;
-    mc_header = (struct microcode_header_intel *)(buf + offset);
-    total_size = get_totalsize(mc_header);
-
-    if ( (offset + total_size) > size )
-    {
-        printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
-        return -EINVAL;
-    }
-
-    *mc = xmemdup_bytes(mc_header, total_size);
-    if ( *mc == NULL )
-        return -ENOMEM;
-
-    return offset + total_size;
-}
-
 static struct microcode_patch *cpu_request_microcode(const void *buf,
                                                      size_t size)
 {
-    long offset = 0;
     int error = 0;
-    struct microcode_intel *mc, *saved = NULL;
+    const struct microcode_patch *saved = NULL;
     struct microcode_patch *patch = NULL;
 
-    while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
+    while ( size )
     {
-        error = microcode_sanity_check(mc);
-        if ( error )
+        const struct microcode_patch *mc;
+        unsigned int blob_size;
+
+        if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
+             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
+             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
+             size < (blob_size =            /* Insufficient space for patch?  */
+                     get_totalsize(&mc->hdr)) )
         {
-            xfree(mc);
+            error = -EINVAL;
+            printk(XENLOG_WARNING "microcode: Bad data in container\n");
             break;
         }
 
+        error = microcode_sanity_check(mc);
+        if ( error )
+            break;
+
         /*
          * If the new update covers current CPU, compare updates and store the
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
              (!saved || (mc->hdr.rev > saved->hdr.rev)) )
-        {
-            xfree(saved);
             saved = mc;
-        }
-        else
-            xfree(mc);
+
+        buf  += blob_size;
+        size -= blob_size;
     }
-    if ( offset < 0 )
-        error = offset;
 
     if ( saved )
-        patch = saved;
+    {
+        patch = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
+
+        if ( !patch )
+            error = -ENOMEM;
+    }
 
     if ( error && !patch )
         patch = ERR_PTR(error);
-- 
2.11.0

[Xen-devel] [PATCH v2 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Andrew Cooper]

Every caller actually passes a struct microcode_header_intel *, but it is more
helpful to us longterm to take struct microcode_patch *.  Implement the
helpers with proper types, and leave a comment explaining the Pentium Pro/II
behaviour with empty {data,total}size fields.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over struct microcode_patch re-work
 * Drop leading underscore
---
 xen/arch/x86/cpu/microcode/intel.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 2b48959573..be2f4871dc 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -46,6 +46,12 @@ struct microcode_header_intel {
     unsigned int sig;
     unsigned int cksum;
     unsigned int ldrver;
+
+    /*
+     * Microcode for the Pentium Pro and II had all further fields in the
+     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
+     * and didn't use platform flags despite the availability of the MSR.
+     */
     unsigned int pf;
     unsigned int datasize;
     unsigned int totalsize;
@@ -74,20 +80,21 @@ struct extended_sigtable {
     struct extended_signature sigs[0];
 };
 
-#define DEFAULT_UCODE_DATASIZE  (2000)
+#define PPRO_UCODE_DATASIZE     2000
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
-#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
 #define EXT_HEADER_SIZE         (sizeof(struct extended_sigtable))
 #define EXT_SIGNATURE_SIZE      (sizeof(struct extended_signature))
 #define DWSIZE                  (sizeof(u32))
-#define get_totalsize(mc) \
-        (((struct microcode_intel *)mc)->hdr.totalsize ? \
-         ((struct microcode_intel *)mc)->hdr.totalsize : \
-         DEFAULT_UCODE_TOTALSIZE)
 
-#define get_datasize(mc) \
-        (((struct microcode_intel *)mc)->hdr.datasize ? \
-         ((struct microcode_intel *)mc)->hdr.datasize : DEFAULT_UCODE_DATASIZE)
+static uint32_t get_datasize(const struct microcode_patch *patch)
+{
+    return patch->hdr.datasize ?: PPRO_UCODE_DATASIZE;
+}
+
+static uint32_t get_totalsize(const struct microcode_patch *patch)
+{
+    return patch->hdr.totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
+}
 
 #define sigmatch(s1, s2, p1, p2) \
         (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
@@ -127,8 +134,8 @@ static int microcode_sanity_check(const struct microcode_patch *mc)
     unsigned int ext_sigcount = 0, i;
     uint32_t sum, orig_sum;
 
-    total_size = get_totalsize(mc_header);
-    data_size = get_datasize(mc_header);
+    total_size = get_totalsize(mc);
+    data_size = get_datasize(mc);
     if ( (data_size + MC_HEADER_SIZE) > total_size )
     {
         printk(KERN_ERR "microcode: error! "
@@ -220,8 +227,8 @@ static enum microcode_match_result microcode_update_match(
     unsigned int sig = cpu_sig->sig;
     unsigned int pf = cpu_sig->pf;
     unsigned int rev = cpu_sig->rev;
-    unsigned long data_size = get_datasize(mc_header);
-    const void *end = (const void *)mc_header + get_totalsize(mc_header);
+    unsigned long data_size = get_datasize(mc);
+    const void *end = (const void *)mc_header + get_totalsize(mc);
 
     ASSERT(!microcode_sanity_check(mc));
     if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
@@ -331,7 +338,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
              (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
              mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
              size < (blob_size =            /* Insufficient space for patch?  */
-                     get_totalsize(&mc->hdr)) )
+                     get_totalsize(mc)) )
         {
             error = -EINVAL;
             printk(XENLOG_WARNING "microcode: Bad data in container\n");
@@ -356,7 +363,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
 
     if ( saved )
     {
-        patch = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
+        patch = xmemdup_bytes(saved, get_totalsize(saved));
 
         if ( !patch )
             error = -ENOMEM;
-- 
2.11.0

[Xen-devel] [PATCH v2 5/7] x86/ucode/intel: Clean up microcode_update_match()

[Andrew Cooper]

Implement a new get_ext_sigtable() helper to abstract the logic for
identifying whether an extended signature table exists.  As part of this,
rename microcode_intel.bits to data and change its type so it can be usefully
used in combination with the datasize header field.

Also, replace the sigmatch() macro with a static inline with a more useful
API, and an explanation of why it is safe to drop one of the previous
conditionals.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over struct microcode_patch re-work
 * Fix maches => matches typo
 * Retain constness on cast through void *
---
 xen/arch/x86/cpu/microcode/intel.c | 77 +++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index be2f4871dc..9d8d5bfc6e 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -60,7 +60,9 @@ struct microcode_header_intel {
 
 struct microcode_patch {
     struct microcode_header_intel hdr;
-    unsigned int bits[0];
+
+    /* Microcode payload.  Format is propriety and encrypted. */
+    uint8_t data[];
 };
 
 /* Temporary, until the microcode_* structure are disentangled. */
@@ -96,8 +98,41 @@ static uint32_t get_totalsize(const struct microcode_patch *patch)
     return patch->hdr.totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
 }
 
-#define sigmatch(s1, s2, p1, p2) \
-        (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
+/*
+ * A piece of microcode has an extended signature table if there is space
+ * between the end of data[] and the total size.  (This logic also works
+ * appropriately for Pentium Pro/II microcode, which has 0 for both size
+ * fields, and no extended signature table.)
+ */
+static const struct extended_sigtable *get_ext_sigtable(
+    const struct microcode_patch *patch)
+{
+    if ( patch->hdr.totalsize > (MC_HEADER_SIZE + patch->hdr.datasize) )
+        return (const void *)&patch->data[patch->hdr.datasize];
+
+    return NULL;
+}
+
+/*
+ * A piece of microcode is applicable for a CPU if:
+ *  1) the signatures (CPUID.1.EAX - Family/Model/Stepping) match, and
+ *  2) The Platform Flags bitmap intersect.
+ *
+ * A CPU will have a single Platform Flag bit, while the microcode may be
+ * common to multiple platforms and have multiple bits set.
+ *
+ * Note: The Pentium Pro/II microcode didn't use platform flags, and should
+ * treat 0 as a match.  However, Xen being 64bit means that the CPU signature
+ * won't match, allowing us to simplify the logic.
+ */
+static bool signature_matches(const struct cpu_signature *cpu_sig,
+                              unsigned int ucode_sig, unsigned int ucode_pf)
+{
+    if ( cpu_sig->sig != ucode_sig )
+        return false;
+
+    return cpu_sig->pf & ucode_pf;
+}
 
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
@@ -219,36 +254,26 @@ static int microcode_sanity_check(const struct microcode_patch *mc)
 static enum microcode_match_result microcode_update_match(
     const struct microcode_patch *mc)
 {
-    const struct microcode_header_intel *mc_header = &mc->hdr;
-    const struct extended_sigtable *ext_header;
-    const struct extended_signature *ext_sig;
+    const struct extended_sigtable *ext;
     unsigned int i;
     struct cpu_signature *cpu_sig = &this_cpu(cpu_sig);
-    unsigned int sig = cpu_sig->sig;
-    unsigned int pf = cpu_sig->pf;
-    unsigned int rev = cpu_sig->rev;
-    unsigned long data_size = get_datasize(mc);
-    const void *end = (const void *)mc_header + get_totalsize(mc);
 
     ASSERT(!microcode_sanity_check(mc));
-    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
-        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
-    ext_header = (const void *)(mc_header + 1) + data_size;
-    ext_sig = (const void *)(ext_header + 1);
+    /* Check the main microcode signature. */
+    if ( signature_matches(cpu_sig, mc->hdr.sig, mc->hdr.pf) )
+        goto found;
 
-    /*
-     * Make sure there is enough space to hold an extended header and enough
-     * array elements.
-     */
-    if ( end <= (const void *)ext_sig )
-        return MIS_UCODE;
-
-    for ( i = 0; i < ext_header->count; i++ )
-        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
-            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+    /* If there is an extended signature table, check each of them. */
+    if ( (ext = get_ext_sigtable(mc)) != NULL )
+        for ( i = 0; i < ext->count; ++i )
+            if ( signature_matches(cpu_sig, ext->sigs[i].sig, ext->sigs[i].pf) )
+                goto found;
 
     return MIS_UCODE;
+
+ found:
+    return mc->hdr.rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static bool match_cpu(const struct microcode_patch *patch)
@@ -296,7 +321,7 @@ static int apply_microcode(const struct microcode_patch *patch)
     BUG_ON(local_irq_is_enabled());
 
     /* write microcode via MSR 0x79 */
-    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
+    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->data);
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
 
     /* As documented in the SDM: Do a CPUID 1 here */
-- 
2.11.0

[Xen-devel] [PATCH v2 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Andrew Cooper]

Rewrite the size checks in a way which doesn't depend on Xen being compiled as
64bit.

Introduce a check missing from the old code, that total_size is a multiple of
1024 bytes, and drop unnecessary defines/macros/structures.

No practical change in behaviour.

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>

v2:
 * Rebase over struct microcode_patch re-work
 * Retain constness on cast through void *
 * Reinstate printk()s for bad data
---
 xen/arch/x86/cpu/microcode/intel.c | 147 +++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 81 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 9d8d5bfc6e..1358a25032 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -69,24 +69,19 @@ struct microcode_patch {
 #define microcode_intel microcode_patch
 
 /* microcode format is extended from prescott processors */
-struct extended_signature {
-    unsigned int sig;
-    unsigned int pf;
-    unsigned int cksum;
-};
-
 struct extended_sigtable {
     unsigned int count;
     unsigned int cksum;
     unsigned int reserved[3];
-    struct extended_signature sigs[0];
+    struct {
+        unsigned int sig;
+        unsigned int pf;
+        unsigned int cksum;
+    } sigs[];
 };
 
 #define PPRO_UCODE_DATASIZE     2000
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
-#define EXT_HEADER_SIZE         (sizeof(struct extended_sigtable))
-#define EXT_SIGNATURE_SIZE      (sizeof(struct extended_signature))
-#define DWSIZE                  (sizeof(u32))
 
 static uint32_t get_datasize(const struct microcode_patch *patch)
 {
@@ -134,8 +129,6 @@ static bool signature_matches(const struct cpu_signature *cpu_sig,
     return cpu_sig->pf & ucode_pf;
 }
 
-#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
-
 static int collect_cpu_info(struct cpu_signature *csig)
 {
     uint64_t msr_content;
@@ -160,93 +153,85 @@ static int collect_cpu_info(struct cpu_signature *csig)
     return 0;
 }
 
-static int microcode_sanity_check(const struct microcode_patch *mc)
+/*
+ * Sanity check a blob which is expected to be a microcode patch.  The 48 byte
+ * header is of a known format, and together with totalsize are within the
+ * bounds of the container.  Everything else is unchecked.
+ */
+static int microcode_sanity_check(const struct microcode_patch *patch)
 {
-    const struct microcode_header_intel *mc_header = &mc->hdr;
-    const struct extended_sigtable *ext_header = NULL;
-    const struct extended_signature *ext_sig;
-    unsigned long total_size, data_size, ext_table_size;
-    unsigned int ext_sigcount = 0, i;
-    uint32_t sum, orig_sum;
-
-    total_size = get_totalsize(mc);
-    data_size = get_datasize(mc);
-    if ( (data_size + MC_HEADER_SIZE) > total_size )
+    const struct extended_sigtable *ext;
+    const uint32_t *ptr;
+    unsigned int total_size = get_totalsize(patch);
+    unsigned int data_size = get_datasize(patch);
+    unsigned int i, ext_size;
+    uint32_t sum;
+
+    /*
+     * Total size must be a multiple of 1024 bytes.  Data size and the header
+     * must fit within it.
+     */
+    if ( (total_size & 1023) ||
+         data_size > (total_size - MC_HEADER_SIZE) )
     {
-        printk(KERN_ERR "microcode: error! "
-               "Bad data size in microcode data file\n");
+        printk(XENLOG_WARNING "microcode: Bad size\n");
         return -EINVAL;
     }
 
-    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
-    {
-        printk(KERN_ERR "microcode: error! "
-               "Unknown microcode update format\n");
+    /* Checksum the main header and data. */
+    for ( sum = 0, ptr = (const uint32_t *)patch;
+          ptr < (const uint32_t *)&patch->data[data_size]; ++ptr )
+        sum += *ptr;
+
+    if ( sum != 0 )
         return -EINVAL;
-    }
-    ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
-    if ( ext_table_size )
+
+    /* Look to see if there is an extended signature table. */
+    ext_size = total_size - data_size - MC_HEADER_SIZE;
+
+    /* No extended signature table?  All done. */
+    if ( ext_size == 0 )
     {
-        if ( (ext_table_size < EXT_HEADER_SIZE) ||
-             ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) )
-        {
-            printk(KERN_ERR "microcode: error! "
-                   "Small exttable size in microcode data file\n");
-            return -EINVAL;
-        }
-        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
-        if ( ext_table_size != exttable_size(ext_header) )
-        {
-            printk(KERN_ERR "microcode: error! "
-                   "Bad exttable size in microcode data file\n");
-            return -EFAULT;
-        }
-        ext_sigcount = ext_header->count;
+        printk(XENLOG_WARNING "microcode: Bad checksum\n");
+        return 0;
     }
 
-    /* check extended table checksum */
-    if ( ext_table_size )
+    /*
+     * Check the structure of the extended signature table, ensuring that it
+     * fits exactly in the remaining space.
+     */
+    ext = (const void *)&patch->data[data_size];
+    if ( ext_size < sizeof(*ext) ||
+         (ext_size - sizeof(*ext)) % sizeof(ext->sigs[0]) ||
+         (ext_size - sizeof(*ext)) / sizeof(ext->sigs[0]) != ext->count )
     {
-        uint32_t ext_table_sum = 0;
-        uint32_t *ext_tablep = (uint32_t *)ext_header;
-
-        i = ext_table_size / DWSIZE;
-        while ( i-- )
-            ext_table_sum += ext_tablep[i];
-        if ( ext_table_sum )
-        {
-            printk(KERN_WARNING "microcode: aborting, "
-                   "bad extended signature table checksum\n");
-            return -EINVAL;
-        }
+        printk(XENLOG_WARNING "microcode: Bad sigtable size\n");
+        return -EINVAL;
     }
 
-    /* calculate the checksum */
-    orig_sum = 0;
-    i = (MC_HEADER_SIZE + data_size) / DWSIZE;
-    while ( i-- )
-        orig_sum += ((uint32_t *)mc)[i];
-    if ( orig_sum )
+    /* Checksum the whole extended signature table. */
+    for ( sum = 0, ptr = (const uint32_t *)ext;
+          ptr < (const uint32_t *)&ext->sigs[ext->count]; ++ptr )
+        sum += *ptr;
+
+    if ( sum != 0 )
     {
-        printk(KERN_ERR "microcode: aborting, bad checksum\n");
+        printk(XENLOG_WARNING "microcode: Bad sigtable checksum\n");
         return -EINVAL;
     }
-    if ( !ext_table_size )
-        return 0;
-    /* check extended signature checksum */
-    for ( i = 0; i < ext_sigcount; i++ )
-    {
-        ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
-            EXT_SIGNATURE_SIZE * i;
-        sum = orig_sum
-            - (mc_header->sig + mc_header->pf + mc_header->cksum)
-            + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
-        if ( sum )
+
+    /*
+     * Checksum each indiviudal extended signature as if it had been in the
+     * main header.
+     */
+    sum = patch->hdr.sig + patch->hdr.pf + patch->hdr.cksum;
+    for ( i = 0; i < ext->count; ++i )
+        if ( sum != (ext->sigs[i].sig + ext->sigs[i].pf + ext->sigs[i].cksum) )
         {
-            printk(KERN_ERR "microcode: aborting, bad checksum\n");
+            printk(XENLOG_WARNING "microcode: Bad sigtable checksum\n");
             return -EINVAL;
         }
-    }
+
     return 0;
 }
 
-- 
2.11.0

[Xen-devel] [PATCH v2 7/7] x86/ucode/intel: Fold structures together

[Andrew Cooper]

With all the necessary cleanup now in place, fold struct
microcode_header_intel into struct microcode_patch and drop the struct
microcode_intel temporary ifdef-ary.

No functional change.

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>

v2:
 * Rebase over struct microcode_patch re-work
---
 xen/arch/x86/cpu/microcode/intel.c | 56 ++++++++++++++------------------------
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 1358a25032..9a8ef62e2b 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -32,17 +32,12 @@
 
 #define pr_debug(x...) ((void)0)
 
-struct microcode_header_intel {
+struct microcode_patch {
     unsigned int hdrver;
     unsigned int rev;
-    union {
-        struct {
-            uint16_t year;
-            uint8_t day;
-            uint8_t month;
-        };
-        unsigned int date;
-    };
+    uint16_t year;
+    uint8_t  day;
+    uint8_t  month;
     unsigned int sig;
     unsigned int cksum;
     unsigned int ldrver;
@@ -56,18 +51,11 @@ struct microcode_header_intel {
     unsigned int datasize;
     unsigned int totalsize;
     unsigned int reserved[3];
-};
-
-struct microcode_patch {
-    struct microcode_header_intel hdr;
 
     /* Microcode payload.  Format is propriety and encrypted. */
     uint8_t data[];
 };
 
-/* Temporary, until the microcode_* structure are disentangled. */
-#define microcode_intel microcode_patch
-
 /* microcode format is extended from prescott processors */
 struct extended_sigtable {
     unsigned int count;
@@ -81,16 +69,16 @@ struct extended_sigtable {
 };
 
 #define PPRO_UCODE_DATASIZE     2000
-#define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
+#define MC_HEADER_SIZE          offsetof(struct microcode_patch, data)
 
 static uint32_t get_datasize(const struct microcode_patch *patch)
 {
-    return patch->hdr.datasize ?: PPRO_UCODE_DATASIZE;
+    return patch->datasize ?: PPRO_UCODE_DATASIZE;
 }
 
 static uint32_t get_totalsize(const struct microcode_patch *patch)
 {
-    return patch->hdr.totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
+    return patch->totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
 }
 
 /*
@@ -102,8 +90,8 @@ static uint32_t get_totalsize(const struct microcode_patch *patch)
 static const struct extended_sigtable *get_ext_sigtable(
     const struct microcode_patch *patch)
 {
-    if ( patch->hdr.totalsize > (MC_HEADER_SIZE + patch->hdr.datasize) )
-        return (const void *)&patch->data[patch->hdr.datasize];
+    if ( patch->totalsize > (MC_HEADER_SIZE + patch->datasize) )
+        return (const void *)&patch->data[patch->datasize];
 
     return NULL;
 }
@@ -224,7 +212,7 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
      * Checksum each indiviudal extended signature as if it had been in the
      * main header.
      */
-    sum = patch->hdr.sig + patch->hdr.pf + patch->hdr.cksum;
+    sum = patch->sig + patch->pf + patch->cksum;
     for ( i = 0; i < ext->count; ++i )
         if ( sum != (ext->sigs[i].sig + ext->sigs[i].pf + ext->sigs[i].cksum) )
         {
@@ -246,7 +234,7 @@ static enum microcode_match_result microcode_update_match(
     ASSERT(!microcode_sanity_check(mc));
 
     /* Check the main microcode signature. */
-    if ( signature_matches(cpu_sig, mc->hdr.sig, mc->hdr.pf) )
+    if ( signature_matches(cpu_sig, mc->sig, mc->pf) )
         goto found;
 
     /* If there is an extended signature table, check each of them. */
@@ -258,7 +246,7 @@ static enum microcode_match_result microcode_update_match(
     return MIS_UCODE;
 
  found:
-    return mc->hdr.rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
+    return mc->rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static bool match_cpu(const struct microcode_patch *patch)
@@ -284,7 +272,7 @@ static enum microcode_match_result compare_patch(
     ASSERT(microcode_update_match(old) != MIS_UCODE);
     ASSERT(microcode_update_match(new) != MIS_UCODE);
 
-    return (new->hdr.rev > old->hdr.rev) ? NEW_UCODE : OLD_UCODE;
+    return new->rev > old->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static int apply_microcode(const struct microcode_patch *patch)
@@ -292,7 +280,6 @@ static int apply_microcode(const struct microcode_patch *patch)
     uint64_t msr_content;
     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 )
@@ -301,12 +288,10 @@ static int apply_microcode(const struct microcode_patch *patch)
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    mc_intel = patch;
-
     BUG_ON(local_irq_is_enabled());
 
     /* write microcode via MSR 0x79 */
-    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->data);
+    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)patch->data);
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
 
     /* As documented in the SDM: Do a CPUID 1 here */
@@ -316,18 +301,17 @@ static int apply_microcode(const struct microcode_patch *patch)
     rdmsrl(MSR_IA32_UCODE_REV, msr_content);
     sig->rev = rev = msr_content >> 32;
 
-    if ( rev != mc_intel->hdr.rev )
+    if ( rev != patch->rev )
     {
         printk(XENLOG_ERR
                "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
-               cpu, old_rev, mc_intel->hdr.rev, rev);
+               cpu, old_rev, patch->rev, rev);
         return -EIO;
     }
 
     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);
+           cpu, old_rev, rev, patch->year, patch->month, patch->day);
 
     return 0;
 }
@@ -345,8 +329,8 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
         unsigned int blob_size;
 
         if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
-             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
-             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
+             (mc = buf)->hdrver != 1 ||     /* Unrecognised header version?   */
+             mc->ldrver != 1 ||             /* Unrecognised loader version?   */
              size < (blob_size =            /* Insufficient space for patch?  */
                      get_totalsize(mc)) )
         {
@@ -364,7 +348,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
-             (!saved || (mc->hdr.rev > saved->hdr.rev)) )
+             (!saved || (mc->rev > saved->rev)) )
             saved = mc;
 
         buf  += blob_size;
-- 
2.11.0

Re: [PATCH v2 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()

[Jan Beulich]

On 27.03.2020 13:28, Andrew Cooper wrote:
> cpu_request_microcode() needs to scan its container and duplicate one blob,
> but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
> Furthermore, the length checking is only safe from overflow in 64bit builds.
> 
> Delete get_next_ucode_from_buffer() and alter the purpose of the saved
> variable to simply point somewhere in buf until we're ready to return.
> 
> This is only a modest reduction in absolute code size (-144), but avoids
> making memory allocations for every blob in the container.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> v2:
>  * Rebase over struct microcode_patch re-work
>  * Reinstate printk() for bad data

Ooi, did the number mentioned above indeed no change with this?
(I don't mean you to adjust it, as it's precise value is not
really meaningful anyway without also knowing compiler version
etc.)

Jan

Re: [PATCH v2 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()

[Andrew Cooper]

On 31/03/2020 15:09, Jan Beulich wrote:
> On 27.03.2020 13:28, Andrew Cooper wrote:
>> cpu_request_microcode() needs to scan its container and duplicate one blob,
>> but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
>> Furthermore, the length checking is only safe from overflow in 64bit builds.
>>
>> Delete get_next_ucode_from_buffer() and alter the purpose of the saved
>> variable to simply point somewhere in buf until we're ready to return.
>>
>> This is only a modest reduction in absolute code size (-144), but avoids
>> making memory allocations for every blob in the container.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> v2:
>>  * Rebase over struct microcode_patch re-work
>>  * Reinstate printk() for bad data
> Ooi, did the number mentioned above indeed no change with this?
> (I don't mean you to adjust it, as it's precise value is not
> really meaningful anyway without also knowing compiler version
> etc.)

I actually stripped the number after re-reading this on xen-devel.  I
didn't go back to check, but it almost certainly isn't the same.

~Andrew

Re: [PATCH v2 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Jan Beulich]

On 27.03.2020 13:29, Andrew Cooper wrote:
> @@ -160,93 +153,85 @@ static int collect_cpu_info(struct cpu_signature *csig)
>      return 0;
>  }
>  
> -static int microcode_sanity_check(const struct microcode_patch *mc)
> +/*
> + * Sanity check a blob which is expected to be a microcode patch.  The 48 byte
> + * header is of a known format, and together with totalsize are within the
> + * bounds of the container.  Everything else is unchecked.
> + */
> +static int microcode_sanity_check(const struct microcode_patch *patch)
>  {
> -    const struct microcode_header_intel *mc_header = &mc->hdr;
> -    const struct extended_sigtable *ext_header = NULL;
> -    const struct extended_signature *ext_sig;
> -    unsigned long total_size, data_size, ext_table_size;
> -    unsigned int ext_sigcount = 0, i;
> -    uint32_t sum, orig_sum;
> -
> -    total_size = get_totalsize(mc);
> -    data_size = get_datasize(mc);
> -    if ( (data_size + MC_HEADER_SIZE) > total_size )
> +    const struct extended_sigtable *ext;
> +    const uint32_t *ptr;
> +    unsigned int total_size = get_totalsize(patch);
> +    unsigned int data_size = get_datasize(patch);
> +    unsigned int i, ext_size;
> +    uint32_t sum;
> +
> +    /*
> +     * Total size must be a multiple of 1024 bytes.  Data size and the header
> +     * must fit within it.
> +     */
> +    if ( (total_size & 1023) ||
> +         data_size > (total_size - MC_HEADER_SIZE) )
>      {
> -        printk(KERN_ERR "microcode: error! "
> -               "Bad data size in microcode data file\n");
> +        printk(XENLOG_WARNING "microcode: Bad size\n");
>          return -EINVAL;
>      }
>  
> -    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
> -    {
> -        printk(KERN_ERR "microcode: error! "
> -               "Unknown microcode update format\n");
> +    /* Checksum the main header and data. */
> +    for ( sum = 0, ptr = (const uint32_t *)patch;
> +          ptr < (const uint32_t *)&patch->data[data_size]; ++ptr )
> +        sum += *ptr;
> +
> +    if ( sum != 0 )
>          return -EINVAL;

The error message for this looks to have been lost, or ...

> -    }
> -    ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
> -    if ( ext_table_size )
> +
> +    /* Look to see if there is an extended signature table. */
> +    ext_size = total_size - data_size - MC_HEADER_SIZE;
> +
> +    /* No extended signature table?  All done. */
> +    if ( ext_size == 0 )
>      {
> -        if ( (ext_table_size < EXT_HEADER_SIZE) ||
> -             ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) )
> -        {
> -            printk(KERN_ERR "microcode: error! "
> -                   "Small exttable size in microcode data file\n");
> -            return -EINVAL;
> -        }
> -        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
> -        if ( ext_table_size != exttable_size(ext_header) )
> -        {
> -            printk(KERN_ERR "microcode: error! "
> -                   "Bad exttable size in microcode data file\n");
> -            return -EFAULT;
> -        }
> -        ext_sigcount = ext_header->count;
> +        printk(XENLOG_WARNING "microcode: Bad checksum\n");
> +        return 0;

... to have got mistakenly moved here. With this addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 7/7] x86/ucode/intel: Fold structures together

[Jan Beulich]

On 27.03.2020 13:29, Andrew Cooper wrote:
> With all the necessary cleanup now in place, fold struct
> microcode_header_intel into struct microcode_patch and drop the struct
> microcode_intel temporary ifdef-ary.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Andrew Cooper]

On 31/03/2020 15:18, Jan Beulich wrote:
> On 27.03.2020 13:29, Andrew Cooper wrote:
>> @@ -160,93 +153,85 @@ static int collect_cpu_info(struct cpu_signature *csig)
>>      return 0;
>>  }
>>  
>> -static int microcode_sanity_check(const struct microcode_patch *mc)
>> +/*
>> + * Sanity check a blob which is expected to be a microcode patch.  The 48 byte
>> + * header is of a known format, and together with totalsize are within the
>> + * bounds of the container.  Everything else is unchecked.
>> + */
>> +static int microcode_sanity_check(const struct microcode_patch *patch)
>>  {
>> -    const struct microcode_header_intel *mc_header = &mc->hdr;
>> -    const struct extended_sigtable *ext_header = NULL;
>> -    const struct extended_signature *ext_sig;
>> -    unsigned long total_size, data_size, ext_table_size;
>> -    unsigned int ext_sigcount = 0, i;
>> -    uint32_t sum, orig_sum;
>> -
>> -    total_size = get_totalsize(mc);
>> -    data_size = get_datasize(mc);
>> -    if ( (data_size + MC_HEADER_SIZE) > total_size )
>> +    const struct extended_sigtable *ext;
>> +    const uint32_t *ptr;
>> +    unsigned int total_size = get_totalsize(patch);
>> +    unsigned int data_size = get_datasize(patch);
>> +    unsigned int i, ext_size;
>> +    uint32_t sum;
>> +
>> +    /*
>> +     * Total size must be a multiple of 1024 bytes.  Data size and the header
>> +     * must fit within it.
>> +     */
>> +    if ( (total_size & 1023) ||
>> +         data_size > (total_size - MC_HEADER_SIZE) )
>>      {
>> -        printk(KERN_ERR "microcode: error! "
>> -               "Bad data size in microcode data file\n");
>> +        printk(XENLOG_WARNING "microcode: Bad size\n");
>>          return -EINVAL;
>>      }
>>  
>> -    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
>> -    {
>> -        printk(KERN_ERR "microcode: error! "
>> -               "Unknown microcode update format\n");
>> +    /* Checksum the main header and data. */
>> +    for ( sum = 0, ptr = (const uint32_t *)patch;
>> +          ptr < (const uint32_t *)&patch->data[data_size]; ++ptr )
>> +        sum += *ptr;
>> +
>> +    if ( sum != 0 )
>>          return -EINVAL;
> The error message for this looks to have been lost, or ...
>
>> -    }
>> -    ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
>> -    if ( ext_table_size )
>> +
>> +    /* Look to see if there is an extended signature table. */
>> +    ext_size = total_size - data_size - MC_HEADER_SIZE;
>> +
>> +    /* No extended signature table?  All done. */
>> +    if ( ext_size == 0 )
>>      {
>> -        if ( (ext_table_size < EXT_HEADER_SIZE) ||
>> -             ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) )
>> -        {
>> -            printk(KERN_ERR "microcode: error! "
>> -                   "Small exttable size in microcode data file\n");
>> -            return -EINVAL;
>> -        }
>> -        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
>> -        if ( ext_table_size != exttable_size(ext_header) )
>> -        {
>> -            printk(KERN_ERR "microcode: error! "
>> -                   "Bad exttable size in microcode data file\n");
>> -            return -EFAULT;
>> -        }
>> -        ext_sigcount = ext_header->count;
>> +        printk(XENLOG_WARNING "microcode: Bad checksum\n");
>> +        return 0;
> ... to have got mistakenly moved here.

It was mistakenly moved.  I found and fixed that at some point after
sending this series.

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

Thanks,

~Andrew