[v10,09/16] microcode: split out apply_microcode() from cpu_request_microcode()

Commit Message

Chao Gao Sept. 12, 2019, 7:22 a.m. UTC

During late microcode loading, apply_microcode() is invoked in
cpu_request_microcode(). To make late microcode update more reliable,
we want to put the apply_microcode() into stop_machine context. So
we split out it from cpu_request_microcode(). In general, for both
early loading on BSP and late loading, cpu_request_microcode() is
called first to get the matching microcode update contained by
the blob and then apply_microcode() is invoked explicitly on each
cpu in common code.

Given that all CPUs are supposed to have the same signature, parsing
microcode only needs to be done once. So cpu_request_microcode() is
also moved out of microcode_update_cpu().

In some cases (e.g. a broken bios), the system may have multiple
revisions of microcode update. So we would try to load a microcode
update as long as it covers current cpu. And if a cpu loads this patch
successfully, the patch would be stored into the patch cache.

Signed-off-by: Chao Gao <[e-mail]>
Reviewed-by: Roger Pau Monné <[e-mail]>
---
Changes in v10:
 - make microcode_update_cache static
 - raise an error if loading ucode failed with -EIO
 - ensure end_update_percpu() is called following a successful call of
 start_update()

Changes in v9:
 - remove the calling of ->compare_patch in microcode_update_cpu().
 - drop "microcode_" prefix for static function - microcode_parse_blob().
 - rebase and fix conflict

Changes in v8:
 - divide the original patch into three patches to improve readability
 - load an update on each cpu as long as the update covers current cpu
 - store an update after the first successful loading on a CPU
 - Make sure the current CPU (especially pf value) is covered
 by updates.

changes in v7:
 - to handle load failure, unvalidated patches won't be cached. They
 are passed as function arguments. So if update failed, we needn't
 any cleanup to microcode cache.
---
 xen/arch/x86/microcode.c        | 182 +++++++++++++++++++++++++++-------------
 xen/arch/x86/microcode_amd.c    |  38 +++++----
 xen/arch/x86/microcode_intel.c  |  66 +++++++--------
 xen/include/asm-x86/microcode.h |   5 +-
 4 files changed, 178 insertions(+), 113 deletions(-)

Comments

Jan Beulich Sept. 12, 2019, 2:07 p.m. UTC | #1

On 12.09.2019 09:22, Chao Gao wrote:
> @@ -249,49 +249,80 @@ bool microcode_update_cache(struct microcode_patch *patch)
>      return true;
>  }
>  
> -static int microcode_update_cpu(const void *buf, size_t size)
> +/*
> + * Load a microcode update to current CPU.
> + *
> + * If no patch is provided, the cached patch will be loaded. Microcode update
> + * during APs bringup and CPU resuming falls into this case.
> + */
> +static int microcode_update_cpu(const struct microcode_patch *patch)
>  {
> -    int err;
> -    unsigned int cpu = smp_processor_id();
> -    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
> +    int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
>  
> -    spin_lock(&microcode_mutex);
> +    if ( unlikely(err) )
> +        return err;
>  
> -    err = microcode_ops->collect_cpu_info(sig);
> -    if ( likely(!err) )
> -        err = microcode_ops->cpu_request_microcode(buf, size);
> -    spin_unlock(&microcode_mutex);
> +    if ( patch )
> +        err = microcode_ops->apply_microcode(patch);
> +    else if ( microcode_cache )
> +    {
> +        spin_lock(&microcode_mutex);
> +        err = microcode_ops->apply_microcode(microcode_cache);
> +        if ( err == -EIO )
> +        {
> +            microcode_free_patch(microcode_cache);
> +            microcode_cache = NULL;
> +        }
> +        spin_unlock(&microcode_mutex);
> +    }

I'm having trouble understanding the locking discipline here: Why
do you call ->apply_microcode() once with the lock held and once
without? If this is to guard against microcode_cache changing,
then (a) the check of it being non-NULL would need to be done with
the lock held as well and (b) you'd need to explain why the non-
locked call to ->apply_microcode() is okay.

It certainly wasn't this way in v8, yet the v9 revision log also
doesn't mention such a (not insignificant) change (which is part
of the reason why I didn't spot it in v9).

> +    else
> +        /* No patch to update */
> +        err = -ENOENT;
>  
>      return err;
>  }
>  
> -static long do_microcode_update(void *_info)
> +static long do_microcode_update(void *patch)
>  {
> -    struct microcode_info *info = _info;
> -    int error;
> -
> -    BUG_ON(info->cpu != smp_processor_id());
> +    unsigned int cpu;
> +    int ret = microcode_update_cpu(patch);
>  
> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
> -    if ( error )
> -        info->error = error;
> +    /* Store the patch after a successful loading */
> +    if ( !ret && patch )
> +    {
> +        spin_lock(&microcode_mutex);
> +        microcode_update_cache(patch);
> +        spin_unlock(&microcode_mutex);
> +        patch = NULL;
> +    }
>  
>      if ( microcode_ops->end_update_percpu )
>          microcode_ops->end_update_percpu();
>  
> -    info->cpu = cpumask_next(info->cpu, &cpu_online_map);
> -    if ( info->cpu < nr_cpu_ids )
> -        return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> +    /*
> +     * Each thread tries to load ucode and only the first thread of a core
> +     * would succeed. Ignore error other than -EIO.
> +     */
> +    if ( ret != -EIO )
> +        ret = 0;

I don't think this is a good idea. Ignoring a _specific_ error
code (e.g. indicating "already loaded" or "newer patch already
loaded") is fine, but here you also ignore things like -ENOMEM
or -EINVAL.

> +    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
> +    if ( cpu < nr_cpu_ids )
> +        return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?
> +                                                                          : ret;

When there's no middle operand I don't think ? and : should be on
separate lines.

> @@ -299,32 +330,46 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>      if ( microcode_ops == NULL )
>          return -EINVAL;
>  
> -    info = xmalloc_bytes(sizeof(*info) + len);
> -    if ( info == NULL )
> +    buffer = xmalloc_bytes(len);
> +    if ( !buffer )
>          return -ENOMEM;
>  
> -    ret = copy_from_guest(info->buffer, buf, len);
> -    if ( ret != 0 )
> +    if ( copy_from_guest(buffer, buf, len) )
> +    {
> +        ret = -EFAULT;
> +        goto free;
> +    }
> +
> +    patch = parse_blob(buffer, len);

You don't look to be using buffer anymore below this point. Why don't
you free it right here, avoiding the need for the "free" label below
and also further reducing the overall code churn as it seems.

Jan

Chao Gao Sept. 13, 2019, 6:47 a.m. UTC | #2

On Thu, Sep 12, 2019 at 04:07:16PM +0200, Jan Beulich wrote:
>On 12.09.2019 09:22, Chao Gao wrote:
>> @@ -249,49 +249,80 @@ bool microcode_update_cache(struct microcode_patch *patch)
>>      return true;
>>  }
>>  
>> -static int microcode_update_cpu(const void *buf, size_t size)
>> +/*
>> + * Load a microcode update to current CPU.
>> + *
>> + * If no patch is provided, the cached patch will be loaded. Microcode update
>> + * during APs bringup and CPU resuming falls into this case.
>> + */
>> +static int microcode_update_cpu(const struct microcode_patch *patch)
>>  {
>> -    int err;
>> -    unsigned int cpu = smp_processor_id();
>> -    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>> +    int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
>>  
>> -    spin_lock(&microcode_mutex);
>> +    if ( unlikely(err) )
>> +        return err;
>>  
>> -    err = microcode_ops->collect_cpu_info(sig);
>> -    if ( likely(!err) )
>> -        err = microcode_ops->cpu_request_microcode(buf, size);
>> -    spin_unlock(&microcode_mutex);
>> +    if ( patch )
>> +        err = microcode_ops->apply_microcode(patch);
>> +    else if ( microcode_cache )
>> +    {
>> +        spin_lock(&microcode_mutex);
>> +        err = microcode_ops->apply_microcode(microcode_cache);
>> +        if ( err == -EIO )
>> +        {
>> +            microcode_free_patch(microcode_cache);
>> +            microcode_cache = NULL;
>> +        }
>> +        spin_unlock(&microcode_mutex);
>> +    }
>
>I'm having trouble understanding the locking discipline here: Why
>do you call ->apply_microcode() once with the lock held and once
>without? If this is to guard against microcode_cache changing,

Yes. microcode_cache is protected by microcode_mutex;

>then (a) the check of it being non-NULL would need to be done with
>the lock held as well and

Will do.

>(b) you'd need to explain why the non-
>locked call to ->apply_microcode() is okay.

->apply_microcode() was always called with this lock held was because
it always read the old per-cpu cache which was protected by the lock.
It gave us an impression that ->apply_microcode() was protected by the
lock.

The patch before this one makes ->apply_microcode() accept a patch
pointer. With this change, if the patch being passed should be accessed
with some lock held (like the secondary call site above), we acquire
the lock. Otherwise, no lock is taken and the caller of
microcode_update_cpu() is supposed to guarantee the patch won't be
changed by others.

>
>It certainly wasn't this way in v8, yet the v9 revision log also
>doesn't mention such a (not insignificant) change (which is part
>of the reason why I didn't spot it in v9).

It is my bad.

>
>> +    else
>> +        /* No patch to update */
>> +        err = -ENOENT;
>>  
>>      return err;
>>  }
>>  
>> -static long do_microcode_update(void *_info)
>> +static long do_microcode_update(void *patch)
>>  {
>> -    struct microcode_info *info = _info;
>> -    int error;
>> -
>> -    BUG_ON(info->cpu != smp_processor_id());
>> +    unsigned int cpu;
>> +    int ret = microcode_update_cpu(patch);
>>  
>> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
>> -    if ( error )
>> -        info->error = error;
>> +    /* Store the patch after a successful loading */
>> +    if ( !ret && patch )
>> +    {
>> +        spin_lock(&microcode_mutex);
>> +        microcode_update_cache(patch);
>> +        spin_unlock(&microcode_mutex);
>> +        patch = NULL;
>> +    }
>>  
>>      if ( microcode_ops->end_update_percpu )
>>          microcode_ops->end_update_percpu();
>>  
>> -    info->cpu = cpumask_next(info->cpu, &cpu_online_map);
>> -    if ( info->cpu < nr_cpu_ids )
>> -        return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>> +    /*
>> +     * Each thread tries to load ucode and only the first thread of a core
>> +     * would succeed. Ignore error other than -EIO.
>> +     */
>> +    if ( ret != -EIO )
>> +        ret = 0;
>
>I don't think this is a good idea. Ignoring a _specific_ error
>code (e.g. indicating "already loaded" or "newer patch already
>loaded") is fine, but here you also ignore things like -ENOMEM
>or -EINVAL.

will do.

>
>> +    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
>> +    if ( cpu < nr_cpu_ids )
>> +        return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?
>> +                                                                          : ret;
>
>When there's no middle operand I don't think ? and : should be on
>separate lines.
>
>> @@ -299,32 +330,46 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>      if ( microcode_ops == NULL )
>>          return -EINVAL;
>>  
>> -    info = xmalloc_bytes(sizeof(*info) + len);
>> -    if ( info == NULL )
>> +    buffer = xmalloc_bytes(len);
>> +    if ( !buffer )
>>          return -ENOMEM;
>>  
>> -    ret = copy_from_guest(info->buffer, buf, len);
>> -    if ( ret != 0 )
>> +    if ( copy_from_guest(buffer, buf, len) )
>> +    {
>> +        ret = -EFAULT;
>> +        goto free;
>> +    }
>> +
>> +    patch = parse_blob(buffer, len);
>
>You don't look to be using buffer anymore below this point. Why don't
>you free it right here, avoiding the need for the "free" label below
>and also further reducing the overall code churn as it seems.

Yes. Good idea.

Thanks
Chao

Patch

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index b44e4d7..d4738f6 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -189,12 +189,19 @@  static DEFINE_SPINLOCK(microcode_mutex);
 
 DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
 
-struct microcode_info {
-    unsigned int cpu;
-    uint32_t buffer_size;
-    int error;
-    char buffer[1];
-};
+/*
+ * Return a patch that covers current CPU. If there are multiple patches,
+ * return the one with the highest revision number. Return error If no
+ * patch is found and an error occurs during the parsing process. Otherwise
+ * return NULL.
+ */
+static struct microcode_patch *parse_blob(const char *buf, size_t len)
+{
+    if ( likely(!microcode_ops->collect_cpu_info(&this_cpu(cpu_sig))) )
+        return microcode_ops->cpu_request_microcode(buf, len);
+
+    return NULL;
+}
 
 int microcode_resume_cpu(void)
 {
@@ -220,15 +227,8 @@  void microcode_free_patch(struct microcode_patch *microcode_patch)
     xfree(microcode_patch);
 }
 
-const struct microcode_patch *microcode_get_cache(void)
-{
-    ASSERT(spin_is_locked(&microcode_mutex));
-
-    return microcode_cache;
-}
-
 /* Return true if cache gets updated. Otherwise, return false */
-bool microcode_update_cache(struct microcode_patch *patch)
+static bool microcode_update_cache(struct microcode_patch *patch)
 {
     ASSERT(spin_is_locked(&microcode_mutex));
 
@@ -249,49 +249,80 @@  bool microcode_update_cache(struct microcode_patch *patch)
     return true;
 }
 
-static int microcode_update_cpu(const void *buf, size_t size)
+/*
+ * Load a microcode update to current CPU.
+ *
+ * If no patch is provided, the cached patch will be loaded. Microcode update
+ * during APs bringup and CPU resuming falls into this case.
+ */
+static int microcode_update_cpu(const struct microcode_patch *patch)
 {
-    int err;
-    unsigned int cpu = smp_processor_id();
-    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+    int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
 
-    spin_lock(&microcode_mutex);
+    if ( unlikely(err) )
+        return err;
 
-    err = microcode_ops->collect_cpu_info(sig);
-    if ( likely(!err) )
-        err = microcode_ops->cpu_request_microcode(buf, size);
-    spin_unlock(&microcode_mutex);
+    if ( patch )
+        err = microcode_ops->apply_microcode(patch);
+    else if ( microcode_cache )
+    {
+        spin_lock(&microcode_mutex);
+        err = microcode_ops->apply_microcode(microcode_cache);
+        if ( err == -EIO )
+        {
+            microcode_free_patch(microcode_cache);
+            microcode_cache = NULL;
+        }
+        spin_unlock(&microcode_mutex);
+    }
+    else
+        /* No patch to update */
+        err = -ENOENT;
 
     return err;
 }
 
-static long do_microcode_update(void *_info)
+static long do_microcode_update(void *patch)
 {
-    struct microcode_info *info = _info;
-    int error;
-
-    BUG_ON(info->cpu != smp_processor_id());
+    unsigned int cpu;
+    int ret = microcode_update_cpu(patch);
 
-    error = microcode_update_cpu(info->buffer, info->buffer_size);
-    if ( error )
-        info->error = error;
+    /* Store the patch after a successful loading */
+    if ( !ret && patch )
+    {
+        spin_lock(&microcode_mutex);
+        microcode_update_cache(patch);
+        spin_unlock(&microcode_mutex);
+        patch = NULL;
+    }
 
     if ( microcode_ops->end_update_percpu )
         microcode_ops->end_update_percpu();
 
-    info->cpu = cpumask_next(info->cpu, &cpu_online_map);
-    if ( info->cpu < nr_cpu_ids )
-        return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+    /*
+     * Each thread tries to load ucode and only the first thread of a core
+     * would succeed. Ignore error other than -EIO.
+     */
+    if ( ret != -EIO )
+        ret = 0;
+
+    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
+    if ( cpu < nr_cpu_ids )
+        return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?
+                                                                          : ret;
 
-    error = info->error;
-    xfree(info);
-    return error;
+    /* Free the patch if no CPU has loaded it successfully. */
+    if ( patch )
+        microcode_free_patch(patch);
+
+    return ret;
 }
 
 int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
 {
     int ret;
-    struct microcode_info *info;
+    void *buffer;
+    struct microcode_patch *patch;
 
     if ( len != (uint32_t)len )
         return -E2BIG;
@@ -299,32 +330,46 @@  int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
     if ( microcode_ops == NULL )
         return -EINVAL;
 
-    info = xmalloc_bytes(sizeof(*info) + len);
-    if ( info == NULL )
+    buffer = xmalloc_bytes(len);
+    if ( !buffer )
         return -ENOMEM;
 
-    ret = copy_from_guest(info->buffer, buf, len);
-    if ( ret != 0 )
+    if ( copy_from_guest(buffer, buf, len) )
+    {
+        ret = -EFAULT;
+        goto free;
+    }
+
+    patch = parse_blob(buffer, len);
+    if ( IS_ERR(patch) )
     {
-        xfree(info);
-        return ret;
+        ret = PTR_ERR(patch);
+        printk(XENLOG_WARNING "Parsing microcode blob error %d\n", ret);
+        goto free;
     }
 
-    info->buffer_size = len;
-    info->error = 0;
-    info->cpu = cpumask_first(&cpu_online_map);
+    if ( !patch )
+    {
+        ret = -ENOENT;
+        goto free;
+    }
 
     if ( microcode_ops->start_update )
     {
         ret = microcode_ops->start_update();
         if ( ret != 0 )
         {
-            xfree(info);
-            return ret;
+            microcode_free_patch(patch);
+            goto free;
         }
     }
 
-    return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+    ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
+                                    do_microcode_update, patch);
+
+ free:
+    xfree(buffer);
+    return ret;
 }
 
 static int __init microcode_init(void)
@@ -371,23 +416,42 @@  int __init early_microcode_update_cpu(bool start_update)
 
     microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
 
-    if ( data )
+    if ( !data )
+        return -ENOMEM;
+
+    if ( start_update )
     {
-        if ( start_update && microcode_ops->start_update )
+        struct microcode_patch *patch;
+
+        patch = parse_blob(data, len);
+        if ( IS_ERR(patch) )
+        {
+            printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
+                   PTR_ERR(patch));
+            return PTR_ERR(patch);
+        }
+
+        if ( !patch )
+            return -ENOENT;
+
+        spin_lock(&microcode_mutex);
+        rc = microcode_update_cache(patch);
+        spin_unlock(&microcode_mutex);
+        ASSERT(rc);
+
+        if ( microcode_ops->start_update )
             rc = microcode_ops->start_update();
 
         if ( rc )
             return rc;
+    }
 
-        rc = microcode_update_cpu(data, len);
+    rc = microcode_update_cpu(NULL);
 
-        if ( microcode_ops->end_update_percpu )
-            microcode_ops->end_update_percpu();
+    if ( microcode_ops->end_update_percpu )
+        microcode_ops->end_update_percpu();
 
-        return rc;
-    }
-    else
-        return -ENOMEM;
+    return rc;
 }
 
 int __init early_microcode_init(void)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index c6d2ea3..1d1bea4 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -455,9 +455,11 @@  static bool_t check_final_patch_levels(unsigned int cpu)
     return 0;
 }
 
-static int cpu_request_microcode(const void *buf, size_t bufsize)
+static struct microcode_patch *cpu_request_microcode(const void *buf,
+                                                     size_t bufsize)
 {
     struct microcode_amd *mc_amd;
+    struct microcode_patch *patch = NULL;
     size_t offset = 0;
     int error = 0;
     unsigned int current_cpu_id;
@@ -556,19 +558,22 @@  static int cpu_request_microcode(const void *buf, size_t bufsize)
             break;
         }
 
-        /* Update cache if this patch covers current CPU */
-        if ( microcode_fits(new_patch->mc_amd) != MIS_UCODE )
-            microcode_update_cache(new_patch);
-        else
-            microcode_free_patch(new_patch);
-
-        if ( match_cpu(microcode_get_cache()) )
+        /*
+         * If the new patch covers current CPU, compare patches and store the
+         * one with higher revision.
+         */
+        if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) &&
+             (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
         {
-            error = apply_microcode(microcode_get_cache());
-            if ( error )
-                break;
+            struct microcode_patch *tmp = patch;
+
+            patch = new_patch;
+            new_patch = tmp;
         }
 
+        if ( new_patch )
+            microcode_free_patch(new_patch);
+
         if ( offset >= bufsize )
             break;
 
@@ -599,13 +604,10 @@  static int cpu_request_microcode(const void *buf, size_t bufsize)
     free_patch(mc_amd);
 
   out:
-    /*
-     * In some cases we may return an error even if processor's microcode has
-     * been updated. For example, the first patch in a container file is loaded
-     * successfully but subsequent container file processing encounters a
-     * failure.
-     */
-    return error;
+    if ( error && !patch )
+        patch = ERR_PTR(error);
+
+    return patch;
 }
 
 #ifdef CONFIG_HVM
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index b1ec81d..c3083d7 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -286,14 +286,9 @@  static enum microcode_match_result compare_patch(
                                                              : OLD_UCODE;
 }
 
-/*
- * return 0 - no update found
- * return 1 - found update
- * return < 0 - error
- */
-static int get_matching_microcode(const void *mc)
+static struct microcode_patch *alloc_microcode_patch(
+    const struct microcode_header_intel *mc_header)
 {
-    const struct microcode_header_intel *mc_header = mc;
     unsigned long total_size = get_totalsize(mc_header);
     void *new_mc = xmalloc_bytes(total_size);
     struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
@@ -302,25 +297,12 @@  static int get_matching_microcode(const void *mc)
     {
         xfree(new_patch);
         xfree(new_mc);
-        return -ENOMEM;
+        return ERR_PTR(-ENOMEM);
     }
-    memcpy(new_mc, mc, total_size);
+    memcpy(new_mc, mc_header, total_size);
     new_patch->mc_intel = new_mc;
 
-    /* Make sure that this patch covers current CPU */
-    if ( microcode_update_match(mc) == MIS_UCODE )
-    {
-        microcode_free_patch(new_patch);
-        return 0;
-    }
-
-    microcode_update_cache(new_patch);
-
-    pr_debug("microcode: CPU%d found a matching microcode update with"
-             " version %#x (current=%#x)\n",
-             smp_processor_id(), mc_header->rev, this_cpu(cpu_sig).rev);
-
-    return 1;
+    return new_patch;
 }
 
 static int apply_microcode(const struct microcode_patch *patch)
@@ -399,26 +381,44 @@  static long get_next_ucode_from_buffer(void **mc, const u8 *buf,
     return offset + total_size;
 }
 
-static int cpu_request_microcode(const void *buf, size_t size)
+static struct microcode_patch *cpu_request_microcode(const void *buf,
+                                                     size_t size)
 {
     long offset = 0;
     int error = 0;
     void *mc;
+    struct microcode_patch *patch = NULL;
 
     while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
     {
+        struct microcode_patch *new_patch;
+
         error = microcode_sanity_check(mc);
         if ( error )
             break;
-        error = get_matching_microcode(mc);
-        if ( error < 0 )
+
+        new_patch = alloc_microcode_patch(mc);
+        if ( IS_ERR(new_patch) )
+        {
+            error = PTR_ERR(new_patch);
             break;
+        }
+
         /*
-         * It's possible the data file has multiple matching ucode,
-         * lets keep searching till the latest version
+         * If the new patch covers current CPU, compare patches and store the
+         * one with higher revision.
          */
-        if ( error == 1 )
-            error = 0;
+        if ( (microcode_update_match(&new_patch->mc_intel->hdr) != MIS_UCODE) &&
+             (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
+        {
+            struct microcode_patch *tmp = patch;
+
+            patch = new_patch;
+            new_patch = tmp;
+        }
+
+        if ( new_patch )
+            microcode_free_patch(new_patch);
 
         xfree(mc);
     }
@@ -427,10 +427,10 @@  static int cpu_request_microcode(const void *buf, size_t size)
     if ( offset < 0 )
         error = offset;
 
-    if ( !error && match_cpu(microcode_get_cache()) )
-        error = apply_microcode(microcode_get_cache());
+    if ( error && !patch )
+        patch = ERR_PTR(error);
 
-    return error;
+    return patch;
 }
 
 static const struct microcode_ops microcode_intel_ops = {
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 02feb09..7d5a1f8 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -20,7 +20,8 @@  struct microcode_patch {
 };
 
 struct microcode_ops {
-    int (*cpu_request_microcode)(const void *buf, size_t size);
+    struct microcode_patch *(*cpu_request_microcode)(const void *buf,
+                                                     size_t size);
     int (*collect_cpu_info)(struct cpu_signature *csig);
     int (*apply_microcode)(const struct microcode_patch *patch);
     int (*start_update)(void);
@@ -40,8 +41,6 @@  struct cpu_signature {
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 extern const struct microcode_ops *microcode_ops;
 
-const struct microcode_patch *microcode_get_cache(void);
-bool microcode_update_cache(struct microcode_patch *patch);
 void microcode_free_patch(struct microcode_patch *patch);
 
 #endif /* ASM_X86__MICROCODE_H */