[Xen-devel] [PATCH 1/2] x86: consolidate microcode loading code

["Jan Beulich" <JBeulich@xxxxxxxx>]

- memory was leaked on a CPU offline/online cycle (including S3)
- memory was leaked on AMD systems when microcode_update() ran a 2nd
  time with the same data that was used on the first run
- microcode never got restored on APs during S3 resume (or post-boot
  onlining of a CPU that was also online when microcode_update() first
  ran [in the event the prior microcode update got lost intermediately,
  which supposedly shouldn't happen]); this will still be the case when
  no other online CPU has an identical signature (which however is now
  consistent with bringing up such a CPU the very first time)
- resume was unimplemented in the AMD case
- there was a race between microcode_update_cpu() and
  microcode_resume_cpu()

This also moves vendor specific type declarations to the vendor source
file and sets the stage for boot time microcode loading (i.e. without
Dom0 involvement).

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -30,9 +30,10 @@ obj-y += io_apic.o
 obj-y += msi.o
 obj-y += ioport_emulate.o
 obj-y += irq.o
-obj-y += microcode.o
 obj-y += microcode_amd.o
 obj-y += microcode_intel.o
+# This must come after the vendor specific files.
+obj-y += microcode.o
 obj-y += mm.o
 obj-y += mpparse.o
 obj-y += nmi.o
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -22,17 +22,17 @@
  */
 
 #include <xen/config.h>
+#include <xen/cpu.h>
 #include <xen/lib.h>
 #include <xen/kernel.h>
 #include <xen/init.h>
+#include <xen/notifier.h>
 #include <xen/sched.h>
 #include <xen/smp.h>
 #include <xen/spinlock.h>
 #include <xen/guest_access.h>
 
-#include <asm/current.h>
 #include <asm/msr.h>
-#include <asm/uaccess.h>
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
@@ -69,30 +69,50 @@ int microcode_resume_cpu(int cpu)
     int err;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     struct cpu_signature nsig;
+    unsigned int cpu2;
 
-    if ( !uci->mc.mc_valid )
-        return -EIO;
+    spin_lock(&microcode_mutex);
 
-    /*
-     * Let's verify that the 'cached' ucode does belong
-     * to this cpu (a bit of paranoia):
-     */
-    err = microcode_ops->collect_cpu_info(cpu, &nsig);
+    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
     if ( err )
     {
-        microcode_fini_cpu(cpu);
+        __microcode_fini_cpu(cpu);
+        spin_unlock(&microcode_mutex);
         return err;
     }
 
-    if ( microcode_ops->microcode_resume_match(cpu, &nsig) )
+    if ( uci->mc.mc_valid )
     {
-        return microcode_ops->apply_microcode(cpu);
+        err = microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid);
+        if ( err >= 0 )
+        {
+            if ( err )
+                err = microcode_ops->apply_microcode(cpu);
+            spin_unlock(&microcode_mutex);
+            return err;
+        }
     }
-    else
+
+    nsig = uci->cpu_sig;
+    __microcode_fini_cpu(cpu);
+    uci->cpu_sig = nsig;
+
+    err = -EIO;
+    for_each_online_cpu ( cpu2 )
     {
-        microcode_fini_cpu(cpu);
-        return -EIO;
+        uci = &per_cpu(ucode_cpu_info, cpu2);
+        if ( uci->mc.mc_valid &&
+             microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid) > 0 )
+        {
+            err = microcode_ops->apply_microcode(cpu);
+            break;
+        }
     }
+
+    __microcode_fini_cpu(cpu);
+    spin_unlock(&microcode_mutex);
+
+    return err;
 }
 
 static int microcode_update_cpu(const void *buf, size_t size)
@@ -162,3 +182,30 @@ int microcode_update(XEN_GUEST_HANDLE(co
 
     return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
 }
+
+static int microcode_percpu_callback(
+    struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+    unsigned int cpu = (unsigned long)hcpu;
+
+    switch ( action )
+    {
+    case CPU_DEAD:
+        microcode_fini_cpu(cpu);
+        break;
+    }
+
+    return NOTIFY_DONE;
+}
+
+static struct notifier_block microcode_percpu_nfb = {
+    .notifier_call = microcode_percpu_callback,
+};
+
+static int __init microcode_presmp_init(void)
+{
+    if ( microcode_ops )
+        register_cpu_notifier(&microcode_percpu_nfb);
+    return 0;
+}
+presmp_initcall(microcode_presmp_init);
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -23,27 +23,53 @@
 #include <xen/spinlock.h>
 
 #include <asm/msr.h>
-#include <asm/uaccess.h>
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
 #define pr_debug(x...) ((void)0)
 
+struct equiv_cpu_entry {
+    uint32_t installed_cpu;
+    uint32_t fixed_errata_mask;
+    uint32_t fixed_errata_compare;
+    uint16_t equiv_cpu;
+    uint16_t reserved;
+} __attribute__((packed));
+
+struct microcode_header_amd {
+    uint32_t data_code;
+    uint32_t patch_id;
+    uint8_t  mc_patch_data_id[2];
+    uint8_t  mc_patch_data_len;
+    uint8_t  init_flag;
+    uint32_t mc_patch_data_checksum;
+    uint32_t nb_dev_id;
+    uint32_t sb_dev_id;
+    uint16_t processor_rev_id;
+    uint8_t  nb_rev_id;
+    uint8_t  sb_rev_id;
+    uint8_t  bios_api_rev;
+    uint8_t  reserved1[3];
+    uint32_t match_reg[8];
+} __attribute__((packed));
+
 #define UCODE_MAGIC                0x00414d44
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
 #define UCODE_MAX_SIZE          (2048)
-#define DEFAULT_UCODE_DATASIZE  (896)
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_amd))
-#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
-#define DWSIZE                  (sizeof(uint32_t))
+
+struct microcode_amd {
+    struct microcode_header_amd hdr;
+    unsigned int mpb[(UCODE_MAX_SIZE - MC_HEADER_SIZE) / 4];
+    unsigned int equiv_cpu_table_size;
+    struct equiv_cpu_entry equiv_cpu_table[];
+};
 
 /* serialize access to the physical write */
 static DEFINE_SPINLOCK(microcode_update_lock);
 
-struct equiv_cpu_entry *equiv_cpu_table;
-
 static int collect_cpu_info(int cpu, struct cpu_signature *csig)
 {
     struct cpuinfo_x86 *c = &cpu_data[cpu];
@@ -65,10 +91,11 @@ static int collect_cpu_info(int cpu, str
     return 0;
 }
 
-static int microcode_fits(void *mc, int cpu)
+static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    struct microcode_header_amd *mc_header = mc;
+    const struct microcode_header_amd *mc_header = &mc_amd->hdr;
+    const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
     unsigned int current_cpu_id;
     unsigned int equiv_cpu_id = 0x0;
     unsigned int i;
@@ -99,7 +126,7 @@ static int microcode_fits(void *mc, int 
     }
 
     if ( mc_header->patch_id <= uci->cpu_sig.rev )
-        return -EINVAL;
+        return 0;
 
     printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
            "update with version 0x%x (current=0x%x)\n",
@@ -186,17 +213,15 @@ static int get_next_ucode_from_buffer_am
     return 0;
 }
 
-static int install_equiv_cpu_table(const void *buf, uint32_t size,
-                                   unsigned long *offset)
+static int install_equiv_cpu_table(
+    struct microcode_amd *mc_amd,
+    const uint32_t *buf_pos,
+    unsigned long *offset)
 {
-    const uint32_t *buf_pos = buf;
-    unsigned long off;
-
-    off = *offset;
-    *offset = 0;
+    uint32_t size = buf_pos[2];
 
     /* No more data */
-    if ( off >= size )
+    if ( size + 12 >= *offset )
         return -EINVAL;
 
     if ( buf_pos[1] != UCODE_EQUIV_CPU_TABLE_TYPE )
@@ -213,15 +238,8 @@ static int install_equiv_cpu_table(const
         return -EINVAL;
     }
 
-    equiv_cpu_table = xmalloc_bytes(size);
-    if ( equiv_cpu_table == NULL )
-    {
-        printk(KERN_ERR "microcode: error, can't allocate "
-               "memory for equiv CPU table\n");
-        return -ENOMEM;
-    }
-
-    memcpy(equiv_cpu_table, (const void *)&buf_pos[3], size);
+    memcpy(mc_amd->equiv_cpu_table, &buf_pos[3], size);
+    mc_amd->equiv_cpu_table_size = size;
 
     *offset = size + 12;       /* add header length */
 
@@ -231,11 +249,11 @@ static int install_equiv_cpu_table(const
 static int cpu_request_microcode(int cpu, const void *buf, size_t size)
 {
     const uint32_t *buf_pos;
-    unsigned long offset = 0;
+    struct microcode_amd *mc_amd, *mc_old;
+    unsigned long offset = size;
     int error = 0;
     int ret;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    void *mc;
 
     /* We should bind the task to the CPU */
     BUG_ON(cpu != raw_smp_processor_id());
@@ -249,59 +267,85 @@ static int cpu_request_microcode(int cpu
         return -EINVAL;
     }
 
-    error = install_equiv_cpu_table(buf, (uint32_t)(buf_pos[2]), &offset);
-    if ( error )
+    mc_amd = xmalloc_bytes(sizeof(*mc_amd) + buf_pos[2]);
+    if ( !mc_amd )
     {
-        printk(KERN_ERR "microcode: installing equivalent cpu table failed\n");
-        return -EINVAL;
+        printk(KERN_ERR "microcode: error! "
+               "Can not allocate memory for microcode patch\n");
+        return -ENOMEM;
     }
 
-    mc = xmalloc_bytes(UCODE_MAX_SIZE);
-    if ( mc == NULL )
+    error = install_equiv_cpu_table(mc_amd, buf, &offset);
+    if ( error )
     {
-        printk(KERN_ERR "microcode: error! "
-               "Can not allocate memory for microcode patch\n");
-        error = -ENOMEM;
-        goto out;
+        xfree(mc_amd);
+        printk(KERN_ERR "microcode: installing equivalent cpu table failed\n");
+        return -EINVAL;
     }
 
+    mc_old = uci->mc.mc_amd;
     /* implicitely validates uci->mc.mc_valid */
-    uci->mc.mc_amd = mc;
+    uci->mc.mc_amd = mc_amd;
 
     /*
      * It's possible the data file has multiple matching ucode,
      * lets keep searching till the latest version
      */
-    while ( (ret = get_next_ucode_from_buffer_amd(mc, buf, size, &offset)) == 
0)
+    while ( (ret = get_next_ucode_from_buffer_amd(&mc_amd->hdr, buf, size,
+                                                  &offset)) == 0 )
     {
-        error = microcode_fits(mc, cpu);
+        error = microcode_fits(mc_amd, cpu);
         if (error <= 0)
             continue;
 
         error = apply_microcode(cpu);
         if (error == 0)
+        {
+            error = 1;
             break;
+        }
     }
 
+    if ( ret < 0 )
+        error = ret;
+
     /* On success keep the microcode patch for
      * re-apply on resume.
      */
-    if (error) {
-        xfree(mc);
-        mc = NULL;
-    }
-    uci->mc.mc_amd = mc;
-
-out:
-    xfree(equiv_cpu_table);
-    equiv_cpu_table = NULL;
+    if (error == 1)
+    {
+        xfree(mc_old);
+        return 0;
+    }
+    xfree(mc_amd);
+    uci->mc.mc_amd = mc_old;
 
     return error;
 }
 
-static int microcode_resume_match(int cpu, struct cpu_signature *nsig)
+static int microcode_resume_match(int cpu, const void *mc)
 {
-    return 0;
+    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+    struct microcode_amd *mc_amd = uci->mc.mc_amd;
+    const struct microcode_amd *src = mc;
+    int res = microcode_fits(src, cpu);
+
+    if ( res <= 0 )
+        return res;
+
+    if ( src != mc_amd )
+    {
+        xfree(mc_amd);
+        mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size);
+        uci->mc.mc_amd = mc_amd;
+        if ( !mc_amd )
+            return -ENOMEM;
+        memcpy(mc_amd, src, UCODE_MAX_SIZE);
+        memcpy(mc_amd->equiv_cpu_table, src->equiv_cpu_table,
+               src->equiv_cpu_table_size);
+    }
+
+    return 1;
 }
 
 static const struct microcode_ops microcode_amd_ops = {
@@ -317,4 +361,4 @@ static __init int microcode_init_amd(voi
         microcode_ops = &microcode_amd_ops;
     return 0;
 }
-__initcall(microcode_init_amd);
+presmp_initcall(microcode_init_amd);
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -30,12 +30,43 @@
 #include <xen/spinlock.h>
 
 #include <asm/msr.h>
-#include <asm/uaccess.h>
 #include <asm/processor.h>
 #include <asm/microcode.h>
 
 #define pr_debug(x...) ((void)0)
 
+struct microcode_header_intel {
+    unsigned int hdrver;
+    unsigned int rev;
+    unsigned int date;
+    unsigned int sig;
+    unsigned int cksum;
+    unsigned int ldrver;
+    unsigned int pf;
+    unsigned int datasize;
+    unsigned int totalsize;
+    unsigned int reserved[3];
+};
+
+struct microcode_intel {
+    struct microcode_header_intel hdr;
+    unsigned int bits[0];
+};
+
+/* 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];
+};
+
 #define DEFAULT_UCODE_DATASIZE  (2000)
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
 #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
@@ -98,7 +129,8 @@ static int collect_cpu_info(int cpu_num,
 }
 
 static inline int microcode_update_match(
-    int cpu_num, struct microcode_header_intel *mc_header, int sig, int pf)
+    int cpu_num, const struct microcode_header_intel *mc_header,
+    int sig, int pf)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
 
@@ -200,11 +232,11 @@ static int microcode_sanity_check(void *
  * return 1 - found update
  * return < 0 - error
  */
-static int get_matching_microcode(void *mc, int cpu)
+static int get_matching_microcode(const void *mc, int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    struct microcode_header_intel *mc_header = mc;
-    struct extended_sigtable *ext_header;
+    const struct microcode_header_intel *mc_header = mc;
+    const struct extended_sigtable *ext_header;
     unsigned long total_size = get_totalsize(mc_header);
     int ext_sigcount, i;
     struct extended_signature *ext_sig;
@@ -229,6 +261,8 @@ static int get_matching_microcode(void *
     }
     return 0;
  find:
+    if ( uci->mc.mc_intel && uci->mc.mc_intel->hdr.rev >= mc_header->rev )
+        return 0;
     pr_debug("microcode: CPU%d found a matching microcode update with"
              " version 0x%x (current=0x%x)\n",
              cpu, mc_header->rev, uci->cpu_sig.rev);
@@ -239,10 +273,8 @@ static int get_matching_microcode(void *
         return -ENOMEM;
     }
 
-    /* free previous update file */
-    xfree(uci->mc.mc_intel);
-
     memcpy(new_mc, mc, total_size);
+    xfree(uci->mc.mc_intel);
     uci->mc.mc_intel = new_mc;
     return 1;
 }
@@ -361,12 +393,9 @@ static int cpu_request_microcode(int cpu
     return error;
 }
 
-static int microcode_resume_match(int cpu, struct cpu_signature *nsig)
+static int microcode_resume_match(int cpu, const void *mc)
 {
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-
-    return (sigmatch(nsig->sig, uci->cpu_sig.sig, nsig->pf, uci->cpu_sig.pf) &&
-            (uci->cpu_sig.rev > nsig->rev));
+    return get_matching_microcode(mc, cpu);
 }
 
 static const struct microcode_ops microcode_intel_ops = {
@@ -382,4 +411,4 @@ static __init int microcode_init_intel(v
         microcode_ops = &microcode_intel_ops;
     return 0;
 }
-__initcall(microcode_init_intel);
+presmp_initcall(microcode_init_intel);
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -7,74 +7,12 @@ struct cpu_signature;
 struct ucode_cpu_info;
 
 struct microcode_ops {
-    int (*microcode_resume_match)(int cpu, struct cpu_signature *nsig);
+    int (*microcode_resume_match)(int cpu, const void *mc);
     int (*cpu_request_microcode)(int cpu, const void *buf, size_t size);
     int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
     int (*apply_microcode)(int cpu);
 };
 
-struct microcode_header_intel {
-    unsigned int hdrver;
-    unsigned int rev;
-    unsigned int date;
-    unsigned int sig;
-    unsigned int cksum;
-    unsigned int ldrver;
-    unsigned int pf;
-    unsigned int datasize;
-    unsigned int totalsize;
-    unsigned int reserved[3];
-};
-
-struct microcode_intel {
-    struct microcode_header_intel hdr;
-    unsigned int bits[0];
-};
-
-/* 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 equiv_cpu_entry {
-    uint32_t installed_cpu;
-    uint32_t fixed_errata_mask;
-    uint32_t fixed_errata_compare;
-    uint16_t equiv_cpu;
-    uint16_t reserved;
-} __attribute__((packed));
-
-struct microcode_header_amd {
-    uint32_t data_code;
-    uint32_t patch_id;
-    uint8_t  mc_patch_data_id[2];
-    uint8_t  mc_patch_data_len;
-    uint8_t  init_flag;
-    uint32_t mc_patch_data_checksum;
-    uint32_t nb_dev_id;
-    uint32_t sb_dev_id;
-    uint16_t processor_rev_id;
-    uint8_t  nb_rev_id;
-    uint8_t  sb_rev_id;
-    uint8_t  bios_api_rev;
-    uint8_t  reserved1[3];
-    uint32_t match_reg[8];
-} __attribute__((packed));
-
-struct microcode_amd {
-    struct microcode_header_amd hdr;
-    unsigned int mpb[0];
-};
-
 struct cpu_signature {
     unsigned int sig;
     unsigned int pf;

Attachment: x86-ucode-consolidation.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel