[Xen-devel] [PATCH 4/6] x86/ucode: Rationalise startup and family/model checks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a
switch statement in early_microcode_init() rather than probing each vendor in
turn.  This allows the microcode_ops pointer to become local to core.c.

As there are no external users of microcode_ops, there is no need for
collect_cpu_info() to implement sanity checks.  Move applicable checks to
early_microcode_init() so they are performed once, rather than repeatedly.

Items to note:
 * The AMD ucode driver does have an upper familiy limit of 0x17, as a side
   effect of the logic in verify_patch_size() which does need updating for
   each new model.
 * The Intel logic guarding the read of MSR_PLATFORM_ID is contrary to the
   SDM, which states that the MSR has been architectural since the Pentium
   Pro (06-01-xx), and lists no family/model restrictions in the pseudocode
   for microcode loading.  Either way, Xen's 64bit-only nature already makes
   this check redundant.

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

The MSR_PLATFORM_ID guard was inherited from Linux, and has existed there
since the code's introduction.  My best guess is that the MSR list in the SDM
got altered at some point between then and now.
---
 xen/arch/x86/cpu/microcode/amd.c     | 21 ++------------------
 xen/arch/x86/cpu/microcode/core.c    | 37 ++++++++++++++++++++++--------------
 xen/arch/x86/cpu/microcode/intel.c   | 26 +++----------------------
 xen/arch/x86/cpu/microcode/private.h |  5 +----
 4 files changed, 29 insertions(+), 60 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 9028889813..768fbcf322 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -76,22 +76,12 @@ struct mpbhdr {
 /* See comment in start_update() for cases when this routine fails */
 static int collect_cpu_info(struct cpu_signature *csig)
 {
-    unsigned int cpu = smp_processor_id();
-    struct cpuinfo_x86 *c = &cpu_data[cpu];
-
     memset(csig, 0, sizeof(*csig));
 
-    if ( (c->x86_vendor != X86_VENDOR_AMD) || (c->x86 < 0x10) )
-    {
-        printk(KERN_ERR "microcode: CPU%d not a capable AMD processor\n",
-               cpu);
-        return -EINVAL;
-    }
-
     rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev);
 
     pr_debug("microcode: CPU%d collect_cpu_info: patch_id=%#x\n",
-             cpu, csig->rev);
+             smp_processor_id(), csig->rev);
 
     return 0;
 }
@@ -601,7 +591,7 @@ static int start_update(void)
 }
 #endif
 
-static const struct microcode_ops microcode_amd_ops = {
+const struct microcode_ops amd_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
@@ -613,10 +603,3 @@ static const struct microcode_ops microcode_amd_ops = {
     .compare_patch                    = compare_patch,
     .match_cpu                        = match_cpu,
 };
-
-int __init microcode_init_amd(void)
-{
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
-        microcode_ops = &microcode_amd_ops;
-    return 0;
-}
diff --git a/xen/arch/x86/cpu/microcode/core.c 
b/xen/arch/x86/cpu/microcode/core.c
index ac5da6b2fe..61e4b9b7ab 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -210,7 +210,7 @@ void __init microcode_grab_module(
         microcode_scan_module(module_map, mbi);
 }
 
-const struct microcode_ops *microcode_ops;
+static const struct microcode_ops __read_mostly *microcode_ops;
 
 static DEFINE_SPINLOCK(microcode_mutex);
 
@@ -798,23 +798,32 @@ static int __init early_microcode_update_cpu(void)
 
 int __init early_microcode_init(void)
 {
-    int rc;
-
-    rc = microcode_init_intel();
-    if ( rc )
-        return rc;
-
-    rc = microcode_init_amd();
-    if ( rc )
-        return rc;
+    const struct cpuinfo_x86 *c = &boot_cpu_data;
+    int rc = 0;
 
-    if ( microcode_ops )
+    switch ( c->x86_vendor )
     {
-        microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
+    case X86_VENDOR_AMD:
+        if ( c->x86 >= 0x10 && c->x86 <= 0x17 )
+            microcode_ops = &amd_ucode_ops;
+        break;
+
+    case X86_VENDOR_INTEL:
+        if ( c->x86 >= 6 )
+            microcode_ops = &intel_ucode_ops;
+        break;
+    }
 
-        if ( ucode_mod.mod_end || ucode_blob.size )
-            rc = early_microcode_update_cpu();
+    if ( !microcode_ops )
+    {
+        printk(XENLOG_WARNING "Microcode loading not available\n");
+        return -ENODEV;
     }
 
+    microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
+
+    if ( ucode_mod.mod_end || ucode_blob.size )
+        rc = early_microcode_update_cpu();
+
     return rc;
 }
diff --git a/xen/arch/x86/cpu/microcode/intel.c 
b/xen/arch/x86/cpu/microcode/intel.c
index 90fb006c94..48544e8d6d 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -93,27 +93,14 @@ struct extended_sigtable {
 
 static int collect_cpu_info(struct cpu_signature *csig)
 {
-    unsigned int cpu_num = smp_processor_id();
-    struct cpuinfo_x86 *c = &cpu_data[cpu_num];
     uint64_t msr_content;
 
     memset(csig, 0, sizeof(*csig));
 
-    if ( (c->x86_vendor != X86_VENDOR_INTEL) || (c->x86 < 6) )
-    {
-        printk(KERN_ERR "microcode: CPU%d not a capable Intel "
-               "processor\n", cpu_num);
-        return -1;
-    }
-
     csig->sig = cpuid_eax(0x00000001);
 
-    if ( (c->x86_model >= 5) || (c->x86 > 6) )
-    {
-        /* get processor flags from MSR 0x17 */
-        rdmsrl(MSR_IA32_PLATFORM_ID, msr_content);
-        csig->pf = 1 << ((msr_content >> 50) & 7);
-    }
+    rdmsrl(MSR_IA32_PLATFORM_ID, msr_content);
+    csig->pf = 1 << ((msr_content >> 50) & 7);
 
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
     /* As documented in the SDM: Do a CPUID 1 here */
@@ -405,7 +392,7 @@ static struct microcode_patch *cpu_request_microcode(const 
void *buf,
     return patch;
 }
 
-static const struct microcode_ops microcode_intel_ops = {
+const struct microcode_ops intel_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
@@ -413,10 +400,3 @@ static const struct microcode_ops microcode_intel_ops = {
     .compare_patch                    = compare_patch,
     .match_cpu                        = match_cpu,
 };
-
-int __init microcode_init_intel(void)
-{
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
-        microcode_ops = &microcode_intel_ops;
-    return 0;
-}
diff --git a/xen/arch/x86/cpu/microcode/private.h 
b/xen/arch/x86/cpu/microcode/private.h
index 459b6a4c54..c32ddc8d19 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -32,9 +32,6 @@ struct microcode_ops {
         const struct microcode_patch *new, const struct microcode_patch *old);
 };
 
-extern const struct microcode_ops *microcode_ops;
-
-int microcode_init_intel(void);
-int microcode_init_amd(void);
+extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops;
 
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */
-- 
2.11.0


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