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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wl@xxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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