[Xen-devel] [PATCH 0/6] x86/ucode: Cleanup - Part 1/n

[Xen-devel] [PATCH 0/6] x86/ucode: Cleanup - Part 1/n

[Andrew Cooper]

I got sad at the state of microcode handling while investigating an issue.
This is some preliminary cleanup, with the only practical changes in patch 4.

Andrew Cooper (6):
  x86/ucode: Remove declarations for non-external functions
  x86/ucode: Move microcode into its own directory
  x86/ucode: Move interface from processor.h to microcode.h
  x86/ucode: Rationalise startup and family/model checks
  x86/ucode: Alter ops->free_patch() to free the entire patch
  x86/ucode: Make struct microcode_patch opaque

 xen/arch/x86/Makefile                              |  3 --
 xen/arch/x86/acpi/power.c                          |  1 +
 xen/arch/x86/cpu/Makefile                          |  1 +
 xen/arch/x86/cpu/microcode/Makefile                |  3 ++
 .../x86/{microcode_amd.c => cpu/microcode/amd.c}   | 54 ++++++++-----------
 xen/arch/x86/{microcode.c => cpu/microcode/core.c} | 62 +++++++++++-----------
 .../{microcode_intel.c => cpu/microcode/intel.c}   | 47 ++++++----------
 .../x86/cpu/microcode/private.h}                   | 33 ++++--------
 xen/arch/x86/efi/efi-boot.h                        |  2 +-
 xen/arch/x86/platform_hypercall.c                  |  1 +
 xen/arch/x86/setup.c                               |  1 +
 xen/arch/x86/smpboot.c                             |  1 +
 xen/arch/x86/spec_ctrl.c                           |  1 -
 xen/include/asm-x86/microcode.h                    | 36 +++----------
 xen/include/asm-x86/processor.h                    |  9 ----
 15 files changed, 93 insertions(+), 162 deletions(-)
 create mode 100644 xen/arch/x86/cpu/microcode/Makefile
 rename xen/arch/x86/{microcode_amd.c => cpu/microcode/amd.c} (95%)
 rename xen/arch/x86/{microcode.c => cpu/microcode/core.c} (96%)
 rename xen/arch/x86/{microcode_intel.c => cpu/microcode/intel.c} (92%)
 copy xen/{include/asm-x86/microcode.h => arch/x86/cpu/microcode/private.h} (57%)

-- 
2.11.0


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

[Xen-devel] [PATCH 1/6] x86/ucode: Remove declarations for non-external functions

[Andrew Cooper]

Neither microcode_free_patch() nor early_microcode_update_cpu() have external
callers.  Make them static.

early_microcode_update_cpu()'s sole caller is following a use of
microcode_ops, making the error path dead.  Drop it as well.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/microcode.c        | 7 ++-----
 xen/include/asm-x86/microcode.h | 2 --
 xen/include/asm-x86/processor.h | 1 -
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 6907b312cf..27a88c6826 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -250,7 +250,7 @@ static struct microcode_patch *parse_blob(const char *buf, size_t len)
     return NULL;
 }
 
-void microcode_free_patch(struct microcode_patch *microcode_patch)
+static void microcode_free_patch(struct microcode_patch *microcode_patch)
 {
     microcode_ops->free_patch(microcode_patch->mc);
     xfree(microcode_patch);
@@ -763,16 +763,13 @@ int microcode_update_one(bool start_update)
 }
 
 /* BSP calls this function to parse ucode blob and then apply an update. */
-int __init early_microcode_update_cpu(void)
+static int __init early_microcode_update_cpu(void)
 {
     int rc = 0;
     const void *data = NULL;
     size_t len;
     struct microcode_patch *patch;
 
-    if ( !microcode_ops )
-        return -ENOSYS;
-
     if ( ucode_blob.size )
     {
         len = ucode_blob.size;
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 7d5a1f8e8a..1a2bbacc6c 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -41,6 +41,4 @@ struct cpu_signature {
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 extern const struct microcode_ops *microcode_ops;
 
-void microcode_free_patch(struct microcode_patch *patch);
-
 #endif /* ASM_X86__MICROCODE_H */
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index b2b19a02cd..895c7032b9 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -581,7 +581,6 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val);
 
 void microcode_set_module(unsigned int);
 int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
-int early_microcode_update_cpu(void);
 int early_microcode_init(void);
 int microcode_update_one(bool start_update);
 int microcode_init_intel(void);
-- 
2.11.0


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

[Xen-devel] [PATCH 2/6] x86/ucode: Move microcode into its own directory

[Andrew Cooper]

Split the existing asm/microcode.h in half, keeping the per-cpu cpu_sig
available to external users, and moving everything else into private.h

Take the opportunity to trim and clean up the include lists for all 3 source
files, all of which include rather more than necessary.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

Inclusion of asm/flushtlb.h in isolation was broken by c/s 80943aa40e, and the
commit message even states this breakage.  I'm surprised it got accepted.

Either this needs fixing, or the 23(!) other files including asm/flushtlb.h
should be adjusted.  Personally I don't think it is reasonable to require
including xen/mm.h just to get at tlb flushing functionality, but I also can't
spot an obvious way to untangle the dependencies (hence the TODO).
---
 xen/arch/x86/Makefile                              |  3 ---
 xen/arch/x86/cpu/Makefile                          |  1 +
 xen/arch/x86/cpu/microcode/Makefile                |  3 +++
 .../x86/{microcode_amd.c => cpu/microcode/amd.c}   | 12 ++++-----
 xen/arch/x86/{microcode.c => cpu/microcode/core.c} | 15 +++--------
 .../{microcode_intel.c => cpu/microcode/intel.c}   |  9 +++----
 .../x86/cpu/microcode/private.h}                   | 19 +++++---------
 xen/include/asm-x86/microcode.h                    | 30 ----------------------
 8 files changed, 22 insertions(+), 70 deletions(-)
 create mode 100644 xen/arch/x86/cpu/microcode/Makefile
 rename xen/arch/x86/{microcode_amd.c => cpu/microcode/amd.c} (99%)
 rename xen/arch/x86/{microcode.c => cpu/microcode/core.c} (99%)
 rename xen/arch/x86/{microcode_intel.c => cpu/microcode/intel.c} (98%)
 copy xen/{include/asm-x86/microcode.h => arch/x86/cpu/microcode/private.h} (78%)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index ed709e2373..e954edbc2e 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -43,9 +43,6 @@ obj-$(CONFIG_INDIRECT_THUNK) += indirect-thunk.o
 obj-y += ioport_emulate.o
 obj-y += irq.o
 obj-$(CONFIG_KEXEC) += machine_kexec.o
-obj-y += microcode_amd.o
-obj-y += microcode_intel.o
-obj-y += microcode.o
 obj-y += mm.o x86_64/mm.o
 obj-$(CONFIG_HVM) += monitor.o
 obj-y += mpparse.o
diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index de983006a1..35561fe51d 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -1,4 +1,5 @@
 obj-y += mcheck/
+obj-y += microcode/
 obj-y += mtrr/
 
 obj-y += amd.o
diff --git a/xen/arch/x86/cpu/microcode/Makefile b/xen/arch/x86/cpu/microcode/Makefile
new file mode 100644
index 0000000000..aae235245b
--- /dev/null
+++ b/xen/arch/x86/cpu/microcode/Makefile
@@ -0,0 +1,3 @@
+obj-y += amd.o
+obj-y += core.o
+obj-y += intel.o
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/cpu/microcode/amd.c
similarity index 99%
rename from xen/arch/x86/microcode_amd.c
rename to xen/arch/x86/cpu/microcode/amd.c
index bc7459416c..9028889813 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -16,16 +16,14 @@
 
 #include <xen/err.h>
 #include <xen/init.h>
-#include <xen/kernel.h>
-#include <xen/lib.h>
-#include <xen/sched.h>
-#include <xen/smp.h>
-#include <xen/spinlock.h>
+#include <xen/mm.h> /* TODO: Fix asm/tlbflush.h breakage */
 
+#include <asm/hvm/svm/svm.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
-#include <asm/microcode.h>
-#include <asm/hvm/svm/svm.h>
+#include <asm/system.h>
+
+#include "private.h"
 
 #define pr_debug(x...) ((void)0)
 
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/cpu/microcode/core.c
similarity index 99%
rename from xen/arch/x86/microcode.c
rename to xen/arch/x86/cpu/microcode/core.c
index 27a88c6826..ac5da6b2fe 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -22,29 +22,22 @@
  */
 
 #include <xen/cpu.h>
+#include <xen/earlycpio.h>
 #include <xen/err.h>
+#include <xen/guest_access.h>
 #include <xen/init.h>
-#include <xen/kernel.h>
-#include <xen/lib.h>
-#include <xen/notifier.h>
 #include <xen/param.h>
-#include <xen/sched.h>
-#include <xen/smp.h>
-#include <xen/softirq.h>
 #include <xen/spinlock.h>
 #include <xen/stop_machine.h>
-#include <xen/tasklet.h>
-#include <xen/guest_access.h>
-#include <xen/earlycpio.h>
 #include <xen/watchdog.h>
 
 #include <asm/apic.h>
 #include <asm/delay.h>
-#include <asm/msr.h>
 #include <asm/nmi.h>
 #include <asm/processor.h>
 #include <asm/setup.h>
-#include <asm/microcode.h>
+
+#include "private.h"
 
 /*
  * Before performing a late microcode update on any thread, we
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/cpu/microcode/intel.c
similarity index 98%
rename from xen/arch/x86/microcode_intel.c
rename to xen/arch/x86/cpu/microcode/intel.c
index 91b7d473f7..90fb006c94 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -23,15 +23,12 @@
 
 #include <xen/err.h>
 #include <xen/init.h>
-#include <xen/kernel.h>
-#include <xen/lib.h>
-#include <xen/sched.h>
-#include <xen/smp.h>
-#include <xen/spinlock.h>
 
 #include <asm/msr.h>
 #include <asm/processor.h>
-#include <asm/microcode.h>
+#include <asm/system.h>
+
+#include "private.h"
 
 #define pr_debug(x...) ((void)0)
 
diff --git a/xen/include/asm-x86/microcode.h b/xen/arch/x86/cpu/microcode/private.h
similarity index 78%
copy from xen/include/asm-x86/microcode.h
copy to xen/arch/x86/cpu/microcode/private.h
index 1a2bbacc6c..2e3be79eaf 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -1,7 +1,9 @@
-#ifndef ASM_X86__MICROCODE_H
-#define ASM_X86__MICROCODE_H
+#ifndef ASM_X86_MICROCODE_PRIVATE_H
+#define ASM_X86_MICROCODE_PRIVATE_H
 
-#include <xen/percpu.h>
+#include <xen/types.h>
+
+#include <asm/microcode.h>
 
 enum microcode_match_result {
     OLD_UCODE, /* signature matched, but revision id is older or equal */
@@ -9,8 +11,6 @@ enum microcode_match_result {
     MIS_UCODE, /* signature mismatched */
 };
 
-struct cpu_signature;
-
 struct microcode_patch {
     union {
         struct microcode_intel *mc_intel;
@@ -32,13 +32,6 @@ struct microcode_ops {
         const struct microcode_patch *new, const struct microcode_patch *old);
 };
 
-struct cpu_signature {
-    unsigned int sig;
-    unsigned int pf;
-    unsigned int rev;
-};
-
-DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 extern const struct microcode_ops *microcode_ops;
 
-#endif /* ASM_X86__MICROCODE_H */
+#endif /* ASM_X86_MICROCODE_PRIVATE_H */
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 1a2bbacc6c..9b6ff7db08 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -3,35 +3,6 @@
 
 #include <xen/percpu.h>
 
-enum microcode_match_result {
-    OLD_UCODE, /* signature matched, but revision id is older or equal */
-    NEW_UCODE, /* signature matched, but revision id is newer */
-    MIS_UCODE, /* signature mismatched */
-};
-
-struct cpu_signature;
-
-struct microcode_patch {
-    union {
-        struct microcode_intel *mc_intel;
-        struct microcode_amd *mc_amd;
-        void *mc;
-    };
-};
-
-struct microcode_ops {
-    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);
-    void (*end_update_percpu)(void);
-    void (*free_patch)(void *mc);
-    bool (*match_cpu)(const struct microcode_patch *patch);
-    enum microcode_match_result (*compare_patch)(
-        const struct microcode_patch *new, const struct microcode_patch *old);
-};
-
 struct cpu_signature {
     unsigned int sig;
     unsigned int pf;
@@ -39,6 +10,5 @@ struct cpu_signature {
 };
 
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
-extern const struct microcode_ops *microcode_ops;
 
 #endif /* ASM_X86__MICROCODE_H */
-- 
2.11.0


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

[Xen-devel] [PATCH 3/6] x86/ucode: Move interface from processor.h to microcode.h

[Andrew Cooper]

This reduces the complexity of processor.h, particularly the need to include
public/xen.h.  Substitute processor.h includes for microcode.h in some
sources, and add microcode.h includes in others.

Only 4 of the function declarations are actually called externally.  Move the
avendor init declarations to private.h

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/power.c            | 1 +
 xen/arch/x86/cpu/microcode/private.h | 3 +++
 xen/arch/x86/efi/efi-boot.h          | 2 +-
 xen/arch/x86/platform_hypercall.c    | 1 +
 xen/arch/x86/setup.c                 | 1 +
 xen/arch/x86/smpboot.c               | 1 +
 xen/arch/x86/spec_ctrl.c             | 1 -
 xen/include/asm-x86/microcode.h      | 8 ++++++++
 xen/include/asm-x86/processor.h      | 8 --------
 9 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index b5df00b22c..e3d6eefe65 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -30,6 +30,7 @@
 #include <asm/tboot.h>
 #include <asm/apic.h>
 #include <asm/io_apic.h>
+#include <asm/microcode.h>
 #include <asm/spec_ctrl.h>
 #include <acpi/cpufreq/cpufreq.h>
 
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 2e3be79eaf..459b6a4c54 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -34,4 +34,7 @@ struct microcode_ops {
 
 extern const struct microcode_ops *microcode_ops;
 
+int microcode_init_intel(void);
+int microcode_init_amd(void);
+
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index bf7b0a61dc..7bfb96875c 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -6,8 +6,8 @@
 #include <xen/vga.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
+#include <asm/microcode.h>
 #include <asm/msr.h>
-#include <asm/processor.h>
 
 static struct file __initdata ucode;
 static multiboot_info_t __initdata mbi = {
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 80efb84328..ee2efdd875 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -27,6 +27,7 @@
 #include <public/platform.h>
 #include <acpi/cpufreq/processor_perf.h>
 #include <asm/edd.h>
+#include <asm/microcode.h>
 #include <asm/mtrr.h>
 #include <asm/io_apic.h>
 #include <asm/setup.h>
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c87040c890..885919d5c3 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -52,6 +52,7 @@
 #include <asm/cpuid.h>
 #include <asm/spec_ctrl.h>
 #include <asm/guest.h>
+#include <asm/microcode.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 0e54bd14f3..09264b02d1 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -39,6 +39,7 @@
 #include <asm/div64.h>
 #include <asm/flushtlb.h>
 #include <asm/guest.h>
+#include <asm/microcode.h>
 #include <asm/msr.h>
 #include <asm/mtrr.h>
 #include <asm/spec_ctrl.h>
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index aed2c6613a..c5d8e587a8 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -24,7 +24,6 @@
 
 #include <asm/microcode.h>
 #include <asm/msr.h>
-#include <asm/processor.h>
 #include <asm/pv/domain.h>
 #include <asm/pv/shim.h>
 #include <asm/setup.h>
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 9b6ff7db08..89b9aaa02d 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -1,8 +1,11 @@
 #ifndef ASM_X86__MICROCODE_H
 #define ASM_X86__MICROCODE_H
 
+#include <xen/types.h>
 #include <xen/percpu.h>
 
+#include <public/xen.h>
+
 struct cpu_signature {
     unsigned int sig;
     unsigned int pf;
@@ -11,4 +14,9 @@ struct cpu_signature {
 
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 
+void microcode_set_module(unsigned int idx);
+int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
+int early_microcode_init(void);
+int microcode_update_one(bool start_update);
+
 #endif /* ASM_X86__MICROCODE_H */
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 895c7032b9..fe231c5072 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -9,7 +9,6 @@
 #include <xen/types.h>
 #include <xen/smp.h>
 #include <xen/percpu.h>
-#include <public/xen.h>
 #include <asm/types.h>
 #include <asm/cpufeature.h>
 #include <asm/desc.h>
@@ -579,13 +578,6 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
 int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val);
 int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val);
 
-void microcode_set_module(unsigned int);
-int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
-int early_microcode_init(void);
-int microcode_update_one(bool start_update);
-int microcode_init_intel(void);
-int microcode_init_amd(void);
-
 static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
                                      uint8_t *stepping)
 {
-- 
2.11.0


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

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

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Andrew Cooper]

The data layout for struct microcode_patch is extremely poor, and
unnecessarily complicated.  Almost all of it is opaque to core.c, with the
exception of free_patch().

Move the responsibility for freeing the patch into the free_patch() hook,
which will allow each driver to do a better job.  Take the opportunity to make
the hooks idempotent.

No practical change in behaviour.

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/amd.c     | 17 ++++++++++++-----
 xen/arch/x86/cpu/microcode/core.c    |  3 +--
 xen/arch/x86/cpu/microcode/intel.c   |  8 ++++++--
 xen/arch/x86/cpu/microcode/private.h |  2 +-
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 768fbcf322..77e582c8e1 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -180,10 +180,8 @@ static bool match_cpu(const struct microcode_patch *patch)
     return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE);
 }
 
-static void free_patch(void *mc)
+static void free_mc_amd(struct microcode_amd *mc_amd)
 {
-    struct microcode_amd *mc_amd = mc;
-
     if ( mc_amd )
     {
         xfree(mc_amd->equiv_cpu_table);
@@ -192,6 +190,15 @@ static void free_patch(void *mc)
     }
 }
 
+static void free_patch(struct microcode_patch *patch)
+{
+    if ( patch )
+    {
+        free_mc_amd(patch->mc_amd);
+        xfree(patch);
+    }
+}
+
 static enum microcode_match_result compare_header(
     const struct microcode_header_amd *new_header,
     const struct microcode_header_amd *old_header)
@@ -564,12 +571,12 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
             patch->mc_amd = mc_amd;
         else
         {
-            free_patch(mc_amd);
+            free_mc_amd(mc_amd);
             error = -ENOMEM;
         }
     }
     else
-        free_patch(mc_amd);
+        free_mc_amd(mc_amd);
 
   out:
     if ( error && !patch )
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 61e4b9b7ab..30017e3e0f 100644
--- 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_blob(const char *buf, size_t len)
 
 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 */
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 48544e8d6d..0e6ba50048 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -248,9 +248,13 @@ static bool match_cpu(const struct microcode_patch *patch)
     return microcode_update_match(&patch->mc_intel->hdr) == NEW_UCODE;
 }
 
-static void free_patch(void *mc)
+static void free_patch(struct microcode_patch *patch)
 {
-    xfree(mc);
+    if ( patch )
+    {
+        xfree(patch->mc_intel);
+        xfree(patch);
+    }
 }
 
 static enum microcode_match_result compare_patch(
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index c32ddc8d19..897d32a8e9 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -26,7 +26,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);
-- 
2.11.0


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

[Xen-devel] [PATCH 6/6] x86/ucode: Make struct microcode_patch opaque

[Andrew Cooper]

This will enforce proper interface discipline in core.c, and allow each driver
to choose its own (better) data layout.

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/amd.c     | 4 ++++
 xen/arch/x86/cpu/microcode/intel.c   | 4 ++++
 xen/arch/x86/cpu/microcode/private.h | 8 +-------
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 77e582c8e1..99e2449eee 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -73,6 +73,10 @@ struct mpbhdr {
     uint8_t data[];
 };
 
+struct microcode_patch {
+    struct microcode_amd *mc_amd;
+};
+
 /* See comment in start_update() for cases when this routine fails */
 static int collect_cpu_info(struct cpu_signature *csig)
 {
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 0e6ba50048..5e9c2a9c7f 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -71,6 +71,10 @@ struct extended_sigtable {
     struct extended_signature sigs[0];
 };
 
+struct microcode_patch {
+    struct microcode_intel *mc_intel;
+};
+
 #define DEFAULT_UCODE_DATASIZE  (2000)
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
 #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 897d32a8e9..e64168a502 100644
--- 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; /* Opaque */
 
 struct microcode_ops {
     struct microcode_patch *(*cpu_request_microcode)(const void *buf,
-- 
2.11.0


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

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

[Jan Beulich]

On 19.03.2020 16:26, Andrew Cooper wrote:
> 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.

I don't see this being the case, and hence I think it is this patch
which introduces such a restriction. As long a patches are less
than 2k, all unspecified families are supported by verify_patch_size()
through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE
doesn't really fit how it is being used.)

I'm happy about all other changes made here.

Jan

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

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

[Andrew Cooper]

On 20/03/2020 13:37, Jan Beulich wrote:
> On 19.03.2020 16:26, Andrew Cooper wrote:
>> 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.
> I don't see this being the case, and hence I think it is this patch
> which introduces such a restriction. As long a patches are less
> than 2k, all unspecified families are supported by verify_patch_size()
> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE
> doesn't really fit how it is being used.)
>
> I'm happy about all other changes made here.

Linux actually has a different algorithm which drops length restrictions
on Fam15h and later, so they get forward compatibility that way.

Would you be happy if I dropped just this aspect of the patch, and defer
AMD adjustments to a later set of changes?

~Andrew

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Jan Beulich]

On 19.03.2020 16:26, Andrew Cooper wrote:
> The data layout for struct microcode_patch is extremely poor, and
> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
> exception of free_patch().
> 
> Move the responsibility for freeing the patch into the free_patch() hook,
> which will allow each driver to do a better job.

But that wrapper structure is something common, i.e. to be
allocated as well as to be freed (preferably) by common code.
We did specifically move there during review of the most
recent re-work.

However, having taken a look also at the next patch I wonder
why you even retain that wrapper structure containing just
a single pointer? Why can't what is now
struct microcode_{amd,intel} become struct microcode_patch,
with - as you say there - different per-vendor layout which
is opaque to common code?

> Take the opportunity to make the hooks idempotent.

I'm having difficulty seeing what part of the patch this is
about.

Jan

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

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

[Jan Beulich]

On 20.03.2020 14:40, Andrew Cooper wrote:
> On 20/03/2020 13:37, Jan Beulich wrote:
>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>> 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.
>> I don't see this being the case, and hence I think it is this patch
>> which introduces such a restriction. As long a patches are less
>> than 2k, all unspecified families are supported by verify_patch_size()
>> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE
>> doesn't really fit how it is being used.)
>>
>> I'm happy about all other changes made here.
> 
> Linux actually has a different algorithm which drops length restrictions
> on Fam15h and later, so they get forward compatibility that way.

If that's what AMD mandates/suggests, we {c,sh}ould consider doing
so too. I thought though that these length restrictions were actually
put in by AMD folks.

> Would you be happy if I dropped just this aspect of the patch, and defer
> AMD adjustments to a later set of changes?

Yes, as said - everything else looked good to me.

Jan

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

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

[Andrew Cooper]

On 20/03/2020 13:56, Jan Beulich wrote:
> On 20.03.2020 14:40, Andrew Cooper wrote:
>> On 20/03/2020 13:37, Jan Beulich wrote:
>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>> 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.
>>> I don't see this being the case, and hence I think it is this patch
>>> which introduces such a restriction. As long a patches are less
>>> than 2k, all unspecified families are supported by verify_patch_size()
>>> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE
>>> doesn't really fit how it is being used.)
>>>
>>> I'm happy about all other changes made here.
>> Linux actually has a different algorithm which drops length restrictions
>> on Fam15h and later, so they get forward compatibility that way.
> If that's what AMD mandates/suggests, we {c,sh}ould consider doing
> so too. I thought though that these length restrictions were actually
> put in by AMD folks.

Its on the list of questions...

>> Would you be happy if I dropped just this aspect of the patch, and defer
>> AMD adjustments to a later set of changes?
> Yes, as said - everything else looked good to me.

Can I take that as an A-by then, to save posting the patch again?

~Andrew

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

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

[Jan Beulich]

On 20.03.2020 15:27, Andrew Cooper wrote:
> On 20/03/2020 13:56, Jan Beulich wrote:
>> On 20.03.2020 14:40, Andrew Cooper wrote:
>>> On 20/03/2020 13:37, Jan Beulich wrote:
>>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>>> 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.
>>>> I don't see this being the case, and hence I think it is this patch
>>>> which introduces such a restriction. As long a patches are less
>>>> than 2k, all unspecified families are supported by verify_patch_size()
>>>> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE
>>>> doesn't really fit how it is being used.)
>>>>
>>>> I'm happy about all other changes made here.
>>> Linux actually has a different algorithm which drops length restrictions
>>> on Fam15h and later, so they get forward compatibility that way.
>> If that's what AMD mandates/suggests, we {c,sh}ould consider doing
>> so too. I thought though that these length restrictions were actually
>> put in by AMD folks.
> 
> Its on the list of questions...
> 
>>> Would you be happy if I dropped just this aspect of the patch, and defer
>>> AMD adjustments to a later set of changes?
>> Yes, as said - everything else looked good to me.
> 
> Can I take that as an A-by then, to save posting the patch again?

If it's just taking out the fam <= 0x17 check - yes.

Jan

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Andrew Cooper]

On 20/03/2020 13:51, Jan Beulich wrote:
> On 19.03.2020 16:26, Andrew Cooper wrote:
>> The data layout for struct microcode_patch is extremely poor, and
>> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
>> exception of free_patch().
>>
>> Move the responsibility for freeing the patch into the free_patch() hook,
>> which will allow each driver to do a better job.
> But that wrapper structure is something common, i.e. to be
> allocated as well as to be freed (preferably) by common code.
> We did specifically move there during review of the most
> recent re-work.

The current behaviour of having it allocated by the request() hook, but
"freed" in a mix of common code and a free() hook, cannot possibly have
been an intended consequence from moving it.

The free() hook is currently necessary, as is the vendor-specific
allocation logic, so splitting freeing responsibility with the common
code is wrong.

> However, having taken a look also at the next patch I wonder
> why you even retain that wrapper structure containing just
> a single pointer? Why can't what is now
> struct microcode_{amd,intel} become struct microcode_patch,
> with - as you say there - different per-vendor layout which
> is opaque to common code?

Various fixes along these lines are pending (but having the resulting
change not be "rewrite the entire file from scratch" is proving harder
than I'd anticipated).

Both Intel and AMD make pointless intermediate memory allocations /
frees for every individual ucode they find in the containers.  Fixing
this is moderately easy and an obvious win.


However, I was also thinking further forwards, perhaps with some
different changes.

We've currently got some awkward hoops to jump through for accessing the
initrd/ucode module, and the dependency on memory allocations forces us
to load microcode much later than ideal on boot.

I was considering whether we could rearrange things so all allocations
were done in core.c, with the vendor specific logic simply identifying a
subset of the provided buffer if an applicable patch is found.

This way, very early boot can load straight out of the initrd/ucode
module (or builtin firmware, for which there is a patch outstanding),
and setting up the ucode cash can happen later when dynamic memory
allocations are available.

This is easy to do for Intel, and not so easy for AMD, given the second
allocation for the equivalence table.

For AMD, the ucode patches don't have the processor signature in them,
and the use of the equivalence table is necessary to turn the processor
signature into the opaque signature in the ucode header.   After
parsing, it is only used for sanity checks, and given the other
restrictions we have on assuming a heterogeneous system, I think we can
get away with dropping the allocation.

OTOH, if we do go down these lines (and specifically, shift the
allocation reponsibility into core.c), I can't see a way of
reintroducing heterogeneous support (on AMD.  Again, Intel is easy, and
we're going to need it eventually for Lakefield support).

Thoughts?

>
>> Take the opportunity to make the hooks idempotent.
> I'm having difficulty seeing what part of the patch this is
> about.

The "if ( patch )" clauses in free_patch().

but I realise that what I meant to write was "tolerate NULL".  Sorry.

We have a weird mix where some of the functions tolerate a NULL patch
(where they can reasonably expect never to be given NULL), but the free
hook doesn't (where it would be most useful for caller simplicity).

~Andrew

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Jan Beulich]

On 20.03.2020 15:50, Andrew Cooper wrote:
> On 20/03/2020 13:51, Jan Beulich wrote:
>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>> The data layout for struct microcode_patch is extremely poor, and
>>> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
>>> exception of free_patch().
>>>
>>> Move the responsibility for freeing the patch into the free_patch() hook,
>>> which will allow each driver to do a better job.
>> But that wrapper structure is something common, i.e. to be
>> allocated as well as to be freed (preferably) by common code.
>> We did specifically move there during review of the most
>> recent re-work.
> 
> The current behaviour of having it allocated by the request() hook, but
> "freed" in a mix of common code and a free() hook, cannot possibly have
> been an intended consequence from moving it.
> 
> The free() hook is currently necessary, as is the vendor-specific
> allocation logic, so splitting freeing responsibility with the common
> code is wrong.

Hmm, yes, with the allocation done in vendor code, the freeing
could be, too. But the wrapper struct gets allocated last in
cpu_request_microcode() (for both Intel and AMD), and hence ought
to be relatively easy to get rid of, instead of moving around
the freeing (the common code part of the freeing would then
simply go away).

>> However, having taken a look also at the next patch I wonder
>> why you even retain that wrapper structure containing just
>> a single pointer? Why can't what is now
>> struct microcode_{amd,intel} become struct microcode_patch,
>> with - as you say there - different per-vendor layout which
>> is opaque to common code?
> 
> Various fixes along these lines are pending (but having the resulting
> change not be "rewrite the entire file from scratch" is proving harder
> than I'd anticipated).
> 
> Both Intel and AMD make pointless intermediate memory allocations /
> frees for every individual ucode they find in the containers.  Fixing
> this is moderately easy and an obvious win.
> 
> 
> However, I was also thinking further forwards, perhaps with some
> different changes.
> 
> We've currently got some awkward hoops to jump through for accessing the
> initrd/ucode module, and the dependency on memory allocations forces us
> to load microcode much later than ideal on boot.
> 
> I was considering whether we could rearrange things so all allocations
> were done in core.c, with the vendor specific logic simply identifying a
> subset of the provided buffer if an applicable patch is found.
> 
> This way, very early boot can load straight out of the initrd/ucode
> module (or builtin firmware, for which there is a patch outstanding),
> and setting up the ucode cash can happen later when dynamic memory
> allocations are available.
> 
> This is easy to do for Intel, and not so easy for AMD, given the second
> allocation for the equivalence table.

During early boot the equiv table could live right in initrd / ucode
module as well, couldn't it?

> For AMD, the ucode patches don't have the processor signature in them,
> and the use of the equivalence table is necessary to turn the processor
> signature into the opaque signature in the ucode header.   After
> parsing, it is only used for sanity checks, and given the other
> restrictions we have on assuming a heterogeneous system, I think we can
> get away with dropping the allocation.
> 
> OTOH, if we do go down these lines (and specifically, shift the
> allocation reponsibility into core.c), I can't see a way of
> reintroducing heterogeneous support (on AMD.  Again, Intel is easy, and
> we're going to need it eventually for Lakefield support).

I think we can worry about re-introduction of this when we actually
learn we'll need it. (Besides shifting allocation responsibility, I
wonder whether e.g. struct microcode_amd couldn't have a static, i.e.
build time instance to be used to track early boot data.)

Jan

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Andrew Cooper]

On 20/03/2020 15:15, Jan Beulich wrote:
> On 20.03.2020 15:50, Andrew Cooper wrote:
>> On 20/03/2020 13:51, Jan Beulich wrote:
>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>> The data layout for struct microcode_patch is extremely poor, and
>>>> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
>>>> exception of free_patch().
>>>>
>>>> Move the responsibility for freeing the patch into the free_patch() hook,
>>>> which will allow each driver to do a better job.
>>> But that wrapper structure is something common, i.e. to be
>>> allocated as well as to be freed (preferably) by common code.
>>> We did specifically move there during review of the most
>>> recent re-work.
>> The current behaviour of having it allocated by the request() hook, but
>> "freed" in a mix of common code and a free() hook, cannot possibly have
>> been an intended consequence from moving it.
>>
>> The free() hook is currently necessary, as is the vendor-specific
>> allocation logic, so splitting freeing responsibility with the common
>> code is wrong.
> Hmm, yes, with the allocation done in vendor code, the freeing
> could be, too. But the wrapper struct gets allocated last in
> cpu_request_microcode() (for both Intel and AMD), and hence ought
> to be relatively easy to get rid of, instead of moving around
> the freeing (the common code part of the freeing would then
> simply go away).

I am working on removing all unnecessary allocations, including folding
microcode_{intel,amd} into microcode_patch, but I'm still confident this
wants to be done with microcode_patch being properly opaque to core.c

>
>>> However, having taken a look also at the next patch I wonder
>>> why you even retain that wrapper structure containing just
>>> a single pointer? Why can't what is now
>>> struct microcode_{amd,intel} become struct microcode_patch,
>>> with - as you say there - different per-vendor layout which
>>> is opaque to common code?
>> Various fixes along these lines are pending (but having the resulting
>> change not be "rewrite the entire file from scratch" is proving harder
>> than I'd anticipated).
>>
>> Both Intel and AMD make pointless intermediate memory allocations /
>> frees for every individual ucode they find in the containers.  Fixing
>> this is moderately easy and an obvious win.
>>
>>
>> However, I was also thinking further forwards, perhaps with some
>> different changes.
>>
>> We've currently got some awkward hoops to jump through for accessing the
>> initrd/ucode module, and the dependency on memory allocations forces us
>> to load microcode much later than ideal on boot.
>>
>> I was considering whether we could rearrange things so all allocations
>> were done in core.c, with the vendor specific logic simply identifying a
>> subset of the provided buffer if an applicable patch is found.
>>
>> This way, very early boot can load straight out of the initrd/ucode
>> module (or builtin firmware, for which there is a patch outstanding),
>> and setting up the ucode cash can happen later when dynamic memory
>> allocations are available.
>>
>> This is easy to do for Intel, and not so easy for AMD, given the second
>> allocation for the equivalence table.
> During early boot the equiv table could live right in initrd / ucode
> module as well, couldn't it?

Thinking about it, with a bit of internal refactoring and one new "void
(*bsp_early_load_something_suitable)(buf, size)" hook, we could get all
of the early handling sorted without complicating the "do we cache it,
do we not?" in core.c

In which case I'm now quite convinced that the opaque microcode_patch is
the best way forwards.

>> For AMD, the ucode patches don't have the processor signature in them,
>> and the use of the equivalence table is necessary to turn the processor
>> signature into the opaque signature in the ucode header.   After
>> parsing, it is only used for sanity checks, and given the other
>> restrictions we have on assuming a heterogeneous system, I think we can
>> get away with dropping the allocation.
>>
>> OTOH, if we do go down these lines (and specifically, shift the
>> allocation reponsibility into core.c), I can't see a way of
>> reintroducing heterogeneous support (on AMD.  Again, Intel is easy, and
>> we're going to need it eventually for Lakefield support).
> I think we can worry about re-introduction of this when we actually
> learn we'll need it.

Agreed, but prefer not to design myself into a corner by not considering
the implications.

> (Besides shifting allocation responsibility, I
> wonder whether e.g. struct microcode_amd couldn't have a static, i.e.
> build time instance to be used to track early boot data.)

The equivalence table is specific to a single ucode container
(microcode_amd_*.bin), and maps cpu_sig (CPUID.1.EAX) => ucode_sig
(uint16_t, which looks like a compression of CPUID.1 but isn't in some
cases).

In the latest microcode from linux-firmware.git, I see one aliasing
update, where both (cpu) 00100f22 and 00100f23 use (ucode) 1022.  All
others look to be unique blobs.

Thinking about it, the cpu_sig => ucode_sig mapping is almost certainly
fixed for specific cpu, because the latter is the identifying
information in the ucode header.

Therefore, given our homogeneous assumptions, there is a single
applicable ucode_sig globally.  A theoretical heterogeneous case could
maintain a small list (4/8 entries?) of applicable mappings for the
appropriate family, which gets augmented by each equiv table we see.

This might be a very helpful simplification for the AMD driver...

~Andrew

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Jan Beulich]

On 20.03.2020 17:10, Andrew Cooper wrote:
> On 20/03/2020 15:15, Jan Beulich wrote:
>> On 20.03.2020 15:50, Andrew Cooper wrote:
>>> On 20/03/2020 13:51, Jan Beulich wrote:
>>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>>> The data layout for struct microcode_patch is extremely poor, and
>>>>> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
>>>>> exception of free_patch().
>>>>>
>>>>> Move the responsibility for freeing the patch into the free_patch() hook,
>>>>> which will allow each driver to do a better job.
>>>> But that wrapper structure is something common, i.e. to be
>>>> allocated as well as to be freed (preferably) by common code.
>>>> We did specifically move there during review of the most
>>>> recent re-work.
>>> The current behaviour of having it allocated by the request() hook, but
>>> "freed" in a mix of common code and a free() hook, cannot possibly have
>>> been an intended consequence from moving it.
>>>
>>> The free() hook is currently necessary, as is the vendor-specific
>>> allocation logic, so splitting freeing responsibility with the common
>>> code is wrong.
>> Hmm, yes, with the allocation done in vendor code, the freeing
>> could be, too. But the wrapper struct gets allocated last in
>> cpu_request_microcode() (for both Intel and AMD), and hence ought
>> to be relatively easy to get rid of, instead of moving around
>> the freeing (the common code part of the freeing would then
>> simply go away).
> 
> I am working on removing all unnecessary allocations, including folding
> microcode_{intel,amd} into microcode_patch, but I'm still confident this
> wants to be done with microcode_patch being properly opaque to core.c

Oh, sure - I didn't mean to put this under question. It just seems
to me the the route there may better be somewhat different from this
and the following patch.

Jan

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Andrew Cooper]

On 20/03/2020 16:16, Jan Beulich wrote:
> On 20.03.2020 17:10, Andrew Cooper wrote:
>> On 20/03/2020 15:15, Jan Beulich wrote:
>>> On 20.03.2020 15:50, Andrew Cooper wrote:
>>>> On 20/03/2020 13:51, Jan Beulich wrote:
>>>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>>>> The data layout for struct microcode_patch is extremely poor, and
>>>>>> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
>>>>>> exception of free_patch().
>>>>>>
>>>>>> Move the responsibility for freeing the patch into the free_patch() hook,
>>>>>> which will allow each driver to do a better job.
>>>>> But that wrapper structure is something common, i.e. to be
>>>>> allocated as well as to be freed (preferably) by common code.
>>>>> We did specifically move there during review of the most
>>>>> recent re-work.
>>>> The current behaviour of having it allocated by the request() hook, but
>>>> "freed" in a mix of common code and a free() hook, cannot possibly have
>>>> been an intended consequence from moving it.
>>>>
>>>> The free() hook is currently necessary, as is the vendor-specific
>>>> allocation logic, so splitting freeing responsibility with the common
>>>> code is wrong.
>>> Hmm, yes, with the allocation done in vendor code, the freeing
>>> could be, too. But the wrapper struct gets allocated last in
>>> cpu_request_microcode() (for both Intel and AMD), and hence ought
>>> to be relatively easy to get rid of, instead of moving around
>>> the freeing (the common code part of the freeing would then
>>> simply go away).
>> I am working on removing all unnecessary allocations, including folding
>> microcode_{intel,amd} into microcode_patch, but I'm still confident this
>> wants to be done with microcode_patch being properly opaque to core.c
> Oh, sure - I didn't mean to put this under question. It just seems
> to me the the route there may better be somewhat different from this
> and the following patch.

How?

We want to remove the pointer from microcode_patch, and don't want the
current contents of microcode_{intel,amd} escaping from their current
source files.

I don't see any option but to rearrange it to be opaque.

~Andrew

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

Re: [Xen-devel] [PATCH 5/6] x86/ucode: Alter ops->free_patch() to free the entire patch

[Jan Beulich]

On 20.03.2020 17:48, Andrew Cooper wrote:
> On 20/03/2020 16:16, Jan Beulich wrote:
>> On 20.03.2020 17:10, Andrew Cooper wrote:
>>> On 20/03/2020 15:15, Jan Beulich wrote:
>>>> On 20.03.2020 15:50, Andrew Cooper wrote:
>>>>> On 20/03/2020 13:51, Jan Beulich wrote:
>>>>>> On 19.03.2020 16:26, Andrew Cooper wrote:
>>>>>>> The data layout for struct microcode_patch is extremely poor, and
>>>>>>> unnecessarily complicated.  Almost all of it is opaque to core.c, with the
>>>>>>> exception of free_patch().
>>>>>>>
>>>>>>> Move the responsibility for freeing the patch into the free_patch() hook,
>>>>>>> which will allow each driver to do a better job.
>>>>>> But that wrapper structure is something common, i.e. to be
>>>>>> allocated as well as to be freed (preferably) by common code.
>>>>>> We did specifically move there during review of the most
>>>>>> recent re-work.
>>>>> The current behaviour of having it allocated by the request() hook, but
>>>>> "freed" in a mix of common code and a free() hook, cannot possibly have
>>>>> been an intended consequence from moving it.
>>>>>
>>>>> The free() hook is currently necessary, as is the vendor-specific
>>>>> allocation logic, so splitting freeing responsibility with the common
>>>>> code is wrong.
>>>> Hmm, yes, with the allocation done in vendor code, the freeing
>>>> could be, too. But the wrapper struct gets allocated last in
>>>> cpu_request_microcode() (for both Intel and AMD), and hence ought
>>>> to be relatively easy to get rid of, instead of moving around
>>>> the freeing (the common code part of the freeing would then
>>>> simply go away).
>>> I am working on removing all unnecessary allocations, including folding
>>> microcode_{intel,amd} into microcode_patch, but I'm still confident this
>>> wants to be done with microcode_patch being properly opaque to core.c
>> Oh, sure - I didn't mean to put this under question. It just seems
>> to me the the route there may better be somewhat different from this
>> and the following patch.
> 
> How?
> 
> We want to remove the pointer from microcode_patch, and don't want the
> current contents of microcode_{intel,amd} escaping from their current
> source files.
> 
> I don't see any option but to rearrange it to be opaque.

I agree. But why do you need to first re-arrange freeing, if then you
drop the wrapper struct (which really is a union) anyway? By dropping
it right away, the split freeing will go away as a side effect.

Jan

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