diff mbox series

[7/7] x86/ucode/intel: Fold structures together

Message ID 20200323101724.15655-8-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series x86/ucode: Cleanup and fixes - Part 3/n (Intel) | expand

Commit Message

Andrew Cooper March 23, 2020, 10:17 a.m. UTC
Currently, we allocate an 8 byte struct microcode_patch to point at a
separately allocated struct microcode_intel.  This is wasteful.

Fold struct microcode_header_intel and microcode_intel into microcode_patch to
simplify the code and remove a level of indirection.

The two semantic changes are in free_patch() and cpu_request_microcode() which
deal with the memory allocation aspects.  Everything else is 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>
---
 xen/arch/x86/cpu/microcode/intel.c | 103 +++++++++++++------------------------
 1 file changed, 37 insertions(+), 66 deletions(-)

Comments

Jan Beulich March 25, 2020, 2:16 p.m. UTC | #1
On 23.03.2020 11:17, Andrew Cooper wrote:
> Currently, we allocate an 8 byte struct microcode_patch to point at a
> separately allocated struct microcode_intel.  This is wasteful.

As indicated elsewhere I'm very much in favor of this, but I think it
wants doing in one of the earlier series, and then for AMD at the same
time. Possibly, to limit code churn there, ...

> --- 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 {

... accompanying this with

#define microcode_header_intel microcode_patch

or even ...

> -    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;
> @@ -57,10 +52,7 @@ struct microcode_header_intel {
>      unsigned int _datasize;
>      unsigned int _totalsize;
>      unsigned int reserved[3];
> -};
>  
> -struct microcode_intel {
> -    struct microcode_header_intel hdr;
>      uint8_t data[];
>  };

... keeping the two structures separate until here, which would
make this one what would initially become struct microcode_patch.
This is in particular because ...

>  static void free_patch(struct microcode_patch *patch)
>  {
> -    if ( patch )
> -    {
> -        xfree(patch->mc_intel);
> -        xfree(patch);
> -    }
> +    xfree(patch);
>  }

... in that earlier series you've moved the 2nd xfree() here just
to now delete it again.

Jan
Andrew Cooper March 25, 2020, 2:32 p.m. UTC | #2
On 25/03/2020 14:16, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>> separately allocated struct microcode_intel.  This is wasteful.
> As indicated elsewhere I'm very much in favor of this, but I think it
> wants doing in one of the earlier series, and then for AMD at the same
> time.

I've got some ideas for an AMD series given the replies I got, and will
be able to do an equivalent microcode_amd => microcode_patch folding on
that side.  There is also further work to do, including unbreaking the
OSVW logic (which has been totally clobbered by the start/end_update
debacle).

However, given that it taken this whole series to make the transition
look safe on the Intel side, I really don't see a way of doing this
"earlier".

In particular, no amount of ifdefary suggested below can AFAICT make it
safe to do this transform without having microcode_patch opaque to being
with.

Yes - there is a bit of churn, but I can't see a safe alternative.

~Andrew

>  Possibly, to limit code churn there, ...
>
>> --- 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 {
> ... accompanying this with
>
> #define microcode_header_intel microcode_patch
>
> or even ...
>
>> -    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;
>> @@ -57,10 +52,7 @@ struct microcode_header_intel {
>>      unsigned int _datasize;
>>      unsigned int _totalsize;
>>      unsigned int reserved[3];
>> -};
>>  
>> -struct microcode_intel {
>> -    struct microcode_header_intel hdr;
>>      uint8_t data[];
>>  };
> ... keeping the two structures separate until here, which would
> make this one what would initially become struct microcode_patch.
> This is in particular because ...
>
>>  static void free_patch(struct microcode_patch *patch)
>>  {
>> -    if ( patch )
>> -    {
>> -        xfree(patch->mc_intel);
>> -        xfree(patch);
>> -    }
>> +    xfree(patch);
>>  }
> ... in that earlier series you've moved the 2nd xfree() here just
> to now delete it again.
>
> Jan
Jan Beulich March 26, 2020, 12:24 p.m. UTC | #3
On 25.03.2020 15:32, Andrew Cooper wrote:
> On 25/03/2020 14:16, Jan Beulich wrote:
>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>>> separately allocated struct microcode_intel.  This is wasteful.
>> As indicated elsewhere I'm very much in favor of this, but I think it
>> wants doing in one of the earlier series, and then for AMD at the same
>> time.
> 
> I've got some ideas for an AMD series given the replies I got, and will
> be able to do an equivalent microcode_amd => microcode_patch folding on
> that side.  There is also further work to do, including unbreaking the
> OSVW logic (which has been totally clobbered by the start/end_update
> debacle).
> 
> However, given that it taken this whole series to make the transition
> look safe on the Intel side, I really don't see a way of doing this
> "earlier".
> 
> In particular, no amount of ifdefary suggested below can AFAICT make it
> safe to do this transform without having microcode_patch opaque to being
> with.
> 
> Yes - there is a bit of churn, but I can't see a safe alternative.

Something like the one below (compile tested only, and not really
cleaned up in any way)?

Jan

--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -60,13 +60,15 @@ 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;
 };
 
+#define microcode_amd microcode_patch
+
 struct mpbhdr {
     uint32_t type;
     uint32_t len;
@@ -177,13 +179,11 @@ static enum microcode_match_result micro
 
 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 +206,12 @@ static enum microcode_match_result compa
 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(new->mc_amd) != MIS_UCODE);
+    ASSERT(microcode_fits(new) != MIS_UCODE);
+    ASSERT(microcode_fits(new) != MIS_UCODE);
 
     return compare_header(new_header, old_header);
 }
@@ -230,7 +230,7 @@ static int apply_microcode(const struct
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    hdr = patch->mc_amd->mpb;
+    hdr = patch->mpb;
 
     BUG_ON(local_irq_is_enabled());
 
@@ -412,7 +412,6 @@ static struct microcode_patch *cpu_reque
 {
     struct microcode_amd *mc_amd;
     struct microcode_header_amd *saved = NULL;
-    struct microcode_patch *patch = NULL;
     size_t offset = 0, saved_size = 0;
     int error = 0;
     unsigned int current_cpu_id;
@@ -559,23 +558,18 @@ static struct microcode_patch *cpu_reque
     {
         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;
-        }
     }
     else
+    {
         free_patch(mc_amd);
+        mc_amd = NULL;
+    }
 
   out:
-    if ( error && !patch )
-        patch = ERR_PTR(error);
+    if ( error && !mc_amd )
+        mc_amd = ERR_PTR(error);
 
-    return patch;
+    return mc_amd;
 }
 
 #ifdef CONFIG_HVM
--- 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_blo
 
 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 */
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -52,11 +52,13 @@ struct microcode_header_intel {
     unsigned int reserved[3];
 };
 
-struct microcode_intel {
+struct microcode_patch {
     struct microcode_header_intel hdr;
     unsigned int bits[0];
 };
 
+#define microcode_intel microcode_patch
+
 /* microcode format is extended from prescott processors */
 struct extended_signature {
     unsigned int sig;
@@ -245,12 +247,12 @@ static bool match_cpu(const struct micro
     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,10 +262,10 @@ static enum microcode_match_result compa
      * 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
+    return (new->hdr.rev > old->hdr.rev) ? NEW_UCODE
                                                              : OLD_UCODE;
 }
 
@@ -281,7 +283,7 @@ static int apply_microcode(const struct
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    mc_intel = patch->mc_intel;
+    mc_intel = patch;
 
     BUG_ON(local_irq_is_enabled());
 
@@ -347,7 +349,6 @@ static struct microcode_patch *cpu_reque
     long offset = 0;
     int error = 0;
     struct microcode_intel *mc, *saved = NULL;
-    struct microcode_patch *patch = NULL;
 
     while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
     {
@@ -374,22 +375,10 @@ static struct microcode_patch *cpu_reque
     if ( offset < 0 )
         error = offset;
 
-    if ( saved )
-    {
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-            patch->mc_intel = saved;
-        else
-        {
-            xfree(saved);
-            error = -ENOMEM;
-        }
-    }
-
-    if ( error && !patch )
-        patch = ERR_PTR(error);
+    if ( error && !saved )
+        saved = ERR_PTR(error);
 
-    return patch;
+    return saved;
 }
 
 const struct microcode_ops intel_ucode_ops = {
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -11,13 +11,7 @@ enum microcode_match_result {
     MIS_UCODE, /* signature mismatched */
 };
 
-struct microcode_patch {
-    union {
-        struct microcode_intel *mc_intel;
-        struct microcode_amd *mc_amd;
-        void *mc;
-    };
-};
+struct microcode_patch;
 
 struct microcode_ops {
     struct microcode_patch *(*cpu_request_microcode)(const void *buf,
@@ -26,7 +20,7 @@ struct microcode_ops {
     int (*apply_microcode)(const struct microcode_patch *patch);
     int (*start_update)(void);
     void (*end_update_percpu)(void);
-    void (*free_patch)(void *mc);
+    void (*free_patch)(struct microcode_patch *patch);
     bool (*match_cpu)(const struct microcode_patch *patch);
     enum microcode_match_result (*compare_patch)(
         const struct microcode_patch *new, const struct microcode_patch *old);
Andrew Cooper March 26, 2020, 2:50 p.m. UTC | #4
On 26/03/2020 12:24, Jan Beulich wrote:
> On 25.03.2020 15:32, Andrew Cooper wrote:
>> On 25/03/2020 14:16, Jan Beulich wrote:
>>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>>>> separately allocated struct microcode_intel.  This is wasteful.
>>> As indicated elsewhere I'm very much in favor of this, but I think it
>>> wants doing in one of the earlier series, and then for AMD at the same
>>> time.
>> I've got some ideas for an AMD series given the replies I got, and will
>> be able to do an equivalent microcode_amd => microcode_patch folding on
>> that side.  There is also further work to do, including unbreaking the
>> OSVW logic (which has been totally clobbered by the start/end_update
>> debacle).
>>
>> However, given that it taken this whole series to make the transition
>> look safe on the Intel side, I really don't see a way of doing this
>> "earlier".
>>
>> In particular, no amount of ifdefary suggested below can AFAICT make it
>> safe to do this transform without having microcode_patch opaque to being
>> with.
>>
>> Yes - there is a bit of churn, but I can't see a safe alternative.
> Something like the one below (compile tested only, and not really
> cleaned up in any way)?
>
> Jan

Thanks.  I'll experiment with this approach.

On a perhaps tangential note, what (if anything) are you plans regarding
backport here?

These defines are ok for a transitional period across a series (and
probably means I'll need to get the AMD side ready to be committed at
the same time), but I don't think we'd want them in the code for the
longterm.

I personally wasn't overly concerned about backports, but if you are, we
should probably take this into consideration for the fixes.

~Andrew
Jan Beulich March 26, 2020, 3:05 p.m. UTC | #5
On 26.03.2020 15:50, Andrew Cooper wrote:
> On a perhaps tangential note, what (if anything) are you plans regarding
> backport here?
> 
> These defines are ok for a transitional period across a series (and
> probably means I'll need to get the AMD side ready to be committed at
> the same time), but I don't think we'd want them in the code for the
> longterm.
> 
> I personally wasn't overly concerned about backports, but if you are, we
> should probably take this into consideration for the fixes.

Till now I didn't see a strong reason why backporting might be
needed (or even just wanted). If you think there is one,
arranging for backport material to come first would of course
be nice. And indeed, the #define-s you mention are meant to be
there just to limit the churn of this immediate patch; I'd be
happy to see them go away in another patch immediately after.

Jan
Andrew Cooper March 27, 2020, 12:40 p.m. UTC | #6
On 26/03/2020 15:05, Jan Beulich wrote:
> On 26.03.2020 15:50, Andrew Cooper wrote:
>> On a perhaps tangential note, what (if anything) are you plans regarding
>> backport here?
>>
>> These defines are ok for a transitional period across a series (and
>> probably means I'll need to get the AMD side ready to be committed at
>> the same time), but I don't think we'd want them in the code for the
>> longterm.
>>
>> I personally wasn't overly concerned about backports, but if you are, we
>> should probably take this into consideration for the fixes.
> Till now I didn't see a strong reason why backporting might be
> needed (or even just wanted).

The gratuitous memory allocation fixes would be the most compelling (and
even then, not massively so), but they're sufficiently interlaced with
the rest of the cleanup that I wasn't expecting backports to be a
pleasant idea.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 2cccf9c26d..8f4ebbd759 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;
@@ -57,10 +52,7 @@  struct microcode_header_intel {
     unsigned int _datasize;
     unsigned int _totalsize;
     unsigned int reserved[3];
-};
 
-struct microcode_intel {
-    struct microcode_header_intel hdr;
     uint8_t data[];
 };
 
@@ -76,21 +68,17 @@  struct extended_sigtable {
     } sigs[];
 };
 
-struct microcode_patch {
-    struct microcode_intel *mc_intel;
-};
-
 #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_header_intel *hdr)
+static uint32_t get_datasize(const struct microcode_patch *mc)
 {
-    return hdr->_datasize ?: PPRO_UCODE_DATASIZE;
+    return mc->_datasize ?: PPRO_UCODE_DATASIZE;
 }
 
-static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
+static uint32_t get_totalsize(const struct microcode_patch *mc)
 {
-    return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
+    return mc->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
 }
 
 /*
@@ -100,10 +88,10 @@  static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
  * fields, and no extended signature table.)
  */
 static const struct extended_sigtable *get_ext_sigtable(
-    const struct microcode_intel *mc)
+    const struct microcode_patch *mc)
 {
-    if ( mc->hdr._totalsize > (MC_HEADER_SIZE + mc->hdr._datasize) )
-        return (void *)&mc->data[mc->hdr._datasize];
+    if ( mc->_totalsize > (MC_HEADER_SIZE + mc->_datasize) )
+        return (void *)&mc->data[mc->_datasize];
 
     return NULL;
 }
@@ -158,11 +146,11 @@  static int collect_cpu_info(struct cpu_signature *csig)
  * 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_intel *mc)
+static int microcode_sanity_check(const struct microcode_patch *mc)
 {
     const struct extended_sigtable *ext;
-    unsigned int total_size = get_totalsize(&mc->hdr);
-    unsigned int data_size = get_datasize(&mc->hdr);
+    unsigned int total_size = get_totalsize(mc);
+    unsigned int data_size = get_datasize(mc);
     unsigned int i, ext_size;
     uint32_t sum, *ptr;
 
@@ -211,7 +199,7 @@  static int microcode_sanity_check(const struct microcode_intel *mc)
      * Checksum each indiviudal extended signature as if it had been in the
      * main header.
      */
-    sum = mc->hdr.sig + mc->hdr.pf + mc->hdr.cksum;
+    sum = mc->sig + mc->pf + mc->cksum;
     for ( i = 0; i < ext->count; ++i )
         if ( sum != (ext->sigs[i].sig + ext->sigs[i].pf + ext->sigs[i].cksum) )
             return -EINVAL;
@@ -221,7 +209,7 @@  static int microcode_sanity_check(const struct microcode_intel *mc)
 
 /* Check an update against the CPU signature and current update revision */
 static enum microcode_match_result microcode_update_match(
-    const struct microcode_intel *mc)
+    const struct microcode_patch *mc)
 {
     const struct extended_sigtable *ext;
     unsigned int i;
@@ -230,7 +218,7 @@  static enum microcode_match_result microcode_update_match(
     ASSERT(!microcode_sanity_check(mc));
 
     /* Check the main microcode signature. */
-    if ( signature_maches(cpu_sig, mc->hdr.sig, mc->hdr.pf) )
+    if ( signature_maches(cpu_sig, mc->sig, mc->pf) )
         goto found;
 
     /* If there is an extended signature table, check each of them. */
@@ -242,7 +230,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)
@@ -250,16 +238,12 @@  static bool match_cpu(const struct microcode_patch *patch)
     if ( !patch )
         return false;
 
-    return microcode_update_match(patch->mc_intel) == NEW_UCODE;
+    return microcode_update_match(patch) == NEW_UCODE;
 }
 
 static void free_patch(struct microcode_patch *patch)
 {
-    if ( patch )
-    {
-        xfree(patch->mc_intel);
-        xfree(patch);
-    }
+    xfree(patch);
 }
 
 static enum microcode_match_result compare_patch(
@@ -269,11 +253,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) != MIS_UCODE);
-    ASSERT(microcode_update_match(new->mc_intel) != MIS_UCODE);
+    ASSERT(microcode_update_match(old) != MIS_UCODE);
+    ASSERT(microcode_update_match(new) != MIS_UCODE);
 
-    return (new->mc_intel->hdr.rev > old->mc_intel->hdr.rev) ? NEW_UCODE
-                                                             : OLD_UCODE;
+    return new->rev > old->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static int apply_microcode(const struct microcode_patch *patch)
@@ -281,7 +264,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 )
@@ -290,12 +272,10 @@  static int apply_microcode(const struct microcode_patch *patch)
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    mc_intel = patch->mc_intel;
-
     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 */
@@ -305,18 +285,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;
 }
@@ -325,19 +304,19 @@  static struct microcode_patch *cpu_request_microcode(const void *buf,
                                                      size_t size)
 {
     int error = 0;
-    const struct microcode_intel *saved = NULL;
+    const struct microcode_patch *saved = NULL;
     struct microcode_patch *patch = NULL;
 
     while ( size )
     {
-        const struct microcode_intel *mc;
+        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?   */
+             (mc = buf)->hdrver != 1 ||     /* Unrecognised header version?   */
+             mc->ldrver != 1 ||             /* Unrecognised loader version?   */
              size < (blob_size =            /* Insufficient space for patch?  */
-                     get_totalsize(&mc->hdr)) )
+                     get_totalsize(mc)) )
         {
             error = -EINVAL;
             break;
@@ -352,7 +331,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;
@@ -361,17 +340,9 @@  static struct microcode_patch *cpu_request_microcode(const void *buf,
 
     if ( saved )
     {
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-        {
-            patch->mc_intel = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
-            if ( !patch->mc_intel )
-            {
-                XFREE(patch);
-                error = -ENOMEM;
-            }
-        }
-        else
+        patch = xmemdup_bytes(saved, get_totalsize(saved));
+
+        if ( !patch )
             error = -ENOMEM;
     }