Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Chao Gao <chao.gao@intel.com>
To: xen-devel@lists.xenproject.org
Cc: "Ashok Raj" <ashok.raj@intel.com>, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Chao Gao" <chao.gao@intel.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: [Xen-devel] [PATCH v9 10/15] microcode: split out apply_microcode() from cpu_request_microcode()
Date: Mon, 19 Aug 2019 09:25:23 +0800
Message-ID: <1566177928-19114-11-git-send-email-chao.gao@intel.com> (raw)
In-Reply-To: <1566177928-19114-1-git-send-email-chao.gao@intel.com>

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 <chao.gao@intel.com>
---
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        | 177 ++++++++++++++++++++++++++--------------
 xen/arch/x86/microcode_amd.c    |  38 +++++----
 xen/arch/x86/microcode_intel.c  |  66 +++++++--------
 xen/include/asm-x86/microcode.h |   5 +-
 4 files changed, 172 insertions(+), 114 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 0e9322a..a2febc7 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, uint32_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,13 +227,6 @@ 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)
 {
@@ -250,49 +250,71 @@ 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;
 
-    error = microcode_update_cpu(info->buffer, info->buffer_size);
-    if ( error )
-        info->error = error;
+    /* Store the patch after a successful loading */
+    if ( !microcode_update_cpu(patch) && patch )
+    {
+        spin_lock(&microcode_mutex);
+        microcode_update_cache(patch);
+        spin_unlock(&microcode_mutex);
+        patch = NULL;
+    }
 
     if ( microcode_ops->end_update )
         microcode_ops->end_update();
 
-    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);
+    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
+    if ( cpu < nr_cpu_ids )
+        return continue_hypercall_on_cpu(cpu, do_microcode_update, patch);
 
-    error = info->error;
-    xfree(info);
-    return error;
+    /* Free the patch if no CPU has loaded it successfully. */
+    if ( patch )
+        microcode_free_patch(patch);
+
+    return 0;
 }
 
 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;
@@ -300,32 +322,44 @@ 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) )
     {
-        xfree(info);
-        return ret;
+        ret = -EFAULT;
+        goto free;
     }
 
-    info->buffer_size = len;
-    info->error = 0;
-    info->cpu = cpumask_first(&cpu_online_map);
-
     if ( microcode_ops->start_update )
     {
         ret = microcode_ops->start_update();
         if ( ret != 0 )
-        {
-            xfree(info);
-            return ret;
-        }
+            goto free;
     }
 
-    return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+    patch = parse_blob(buffer, len);
+    if ( IS_ERR(patch) )
+    {
+        ret = PTR_ERR(patch);
+        printk(XENLOG_INFO "Parsing microcode blob error %d\n", ret);
+        goto free;
+    }
+
+    if ( !patch )
+    {
+        printk(XENLOG_INFO "No ucode found. Update aborted!\n");
+        ret = -EINVAL;
+        goto free;
+    }
+
+    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)
@@ -372,23 +406,46 @@ 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;
+
+        if ( microcode_ops->start_update )
             rc = microcode_ops->start_update();
 
         if ( rc )
             return rc;
 
-        rc = microcode_update_cpu(data, len);
+        patch = parse_blob(data, len);
+        if ( IS_ERR(patch) )
+        {
+            printk(XENLOG_INFO "Parsing microcode blob error %ld\n",
+                   PTR_ERR(patch));
+            return PTR_ERR(patch);
+        }
+
+        if ( !patch )
+        {
+            printk(XENLOG_INFO "No ucode found. Update aborted!\n");
+            return -EINVAL;
+        }
 
-        if ( microcode_ops->end_update )
-            microcode_ops->end_update();
+        spin_lock(&microcode_mutex);
+        rc = microcode_update_cache(patch);
+        spin_unlock(&microcode_mutex);
 
-        return rc;
+        ASSERT(rc);
     }
-    else
-        return -ENOMEM;
+
+    rc = microcode_update_cpu(NULL);
+
+    if ( microcode_ops->end_update )
+        microcode_ops->end_update();
+
+    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 21cdfe0..6353323 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -447,9 +447,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;
@@ -548,19 +550,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;
 
@@ -593,13 +598,10 @@ static int cpu_request_microcode(const void *buf, size_t bufsize)
     xfree(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;
 }
 
 static int start_update(void)
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 8c0008c..96b38f8 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -282,14 +282,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);
@@ -298,25 +293,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)
@@ -392,26 +374,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);
     }
@@ -420,10 +420,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 8c7de9d..8e71615 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);
@@ -41,8 +42,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 */
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

  parent reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-19  1:25 [Xen-devel] [PATCH v9 00/15] improve late microcode loading Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 01/15] microcode/intel: extend microcode_update_match() Chao Gao
2019-08-28 15:12   ` Jan Beulich
2019-08-29  7:15     ` Chao Gao
2019-08-29  7:14       ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 02/15] microcode/amd: fix memory leak Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 03/15] microcode/amd: distinguish old and mismatched ucode in microcode_fits() Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 04/15] microcode: introduce a global cache of ucode patch Chao Gao
2019-08-22 11:11   ` Roger Pau Monné
2019-08-28 15:21   ` Jan Beulich
2019-08-29 10:18   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 05/15] microcode: clean up microcode_resume_cpu Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 06/15] microcode: remove struct ucode_cpu_info Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 07/15] microcode: remove pointless 'cpu' parameter Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 08/15] microcode/amd: call svm_host_osvw_init() in common code Chao Gao
2019-08-22 13:08   ` Roger Pau Monné
2019-08-28 15:26   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 09/15] microcode: pass a patch pointer to apply_microcode() Chao Gao
2019-08-19  1:25 ` Chao Gao [this message]
2019-08-22 13:59   ` [Xen-devel] [PATCH v9 10/15] microcode: split out apply_microcode() from cpu_request_microcode() Roger Pau Monné
2019-08-29 10:06     ` Jan Beulich
2019-08-30  3:22       ` Chao Gao
2019-08-30  7:25         ` Jan Beulich
2019-08-29 10:19   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 11/15] microcode: unify loading update during CPU resuming and AP wakeup Chao Gao
2019-08-22 14:10   ` Roger Pau Monné
2019-08-22 16:44     ` Chao Gao
2019-08-23  9:09       ` Roger Pau Monné
2019-08-29  7:37         ` Chao Gao
2019-08-29  8:16           ` Roger Pau Monné
2019-08-29 10:26           ` Jan Beulich
2019-08-29 10:29   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch Chao Gao
2019-08-23  8:11   ` Roger Pau Monné
2019-08-26  7:03     ` Chao Gao
2019-08-26  8:11       ` Roger Pau Monné
2019-08-29 10:47   ` Jan Beulich
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 13/15] x86/microcode: Synchronize late microcode loading Chao Gao
2019-08-19 10:27   ` Sergey Dyasli
2019-08-19 14:49     ` Chao Gao
2019-08-29 12:06   ` Jan Beulich
2019-08-30  3:30     ` Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 14/15] microcode: remove microcode_update_lock Chao Gao
2019-08-19  1:25 ` [Xen-devel] [PATCH v9 15/15] microcode: block #NMI handling when loading an ucode Chao Gao
2019-08-23  8:46   ` Sergey Dyasli
2019-08-26  8:07     ` Chao Gao
2019-08-27  4:52       ` Chao Gao
2019-08-28  8:52         ` Sergey Dyasli
2019-08-29 12:11         ` Jan Beulich
2019-08-30  6:35           ` Chao Gao
2019-09-09  5:52             ` Chao Gao
2019-09-09  6:16               ` Jan Beulich
2019-08-29 12:22   ` Jan Beulich
2019-08-30  6:33     ` Chao Gao
2019-08-30  7:30       ` Jan Beulich
2019-08-22  7:51 ` [Xen-devel] [PATCH v9 00/15] improve late microcode loading Sergey Dyasli
2019-08-22 15:39   ` Chao Gao

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1566177928-19114-11-git-send-email-chao.gao@intel.com \
    --to=chao.gao@intel.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=ashok.raj@intel.com \
    --cc=jbeulich@suse.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git