[PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Hi all,

I'm proposing this for 2.6.39.

This series adds a new "Xen" microcode update type, in addition to
Intel and AMD.

The Xen hypervisor is responsible for performing the actual microcode
update (since only it knows what physical CPUs are in the system and
has sufficient privilege to access them), but it requires the dom0
kernel to provide the actual microcode update data.

Xen update mechanism is uniform independent of the CPU type, but the
driver must know where to find the data file, which depends on the CPU
type.  And since the update hypercall updates all CPUs, we only need
to execute it once on any CPU - but for simplicity it just runs it only
on (V)CPU 0.

Thanks,
	J

Jeremy Fitzhardinge (1):
  xen: add CPU microcode update driver

Stephen Tweedie (1):
  xen dom0: Add support for the platform_ops hypercall

 arch/x86/include/asm/microcode.h     |    9 ++
 arch/x86/include/asm/xen/hypercall.h |    8 ++
 arch/x86/kernel/Makefile             |    1 +
 arch/x86/kernel/microcode_core.c     |    5 +-
 arch/x86/kernel/microcode_xen.c      |  198 ++++++++++++++++++++++++++++++
 arch/x86/xen/Kconfig                 |    4 +
 include/xen/interface/platform.h     |  222 ++++++++++++++++++++++++++++++++++
 include/xen/interface/xen.h          |    2 +
 8 files changed, 448 insertions(+), 1 deletions(-)
 create mode 100644 arch/x86/kernel/microcode_xen.c
 create mode 100644 include/xen/interface/platform.h

-- 
1.7.3.5

[PATCH 1/2] xen dom0: Add support for the platform_ops hypercall

[Jeremy Fitzhardinge]

From: Stephen Tweedie <sct@redhat.com>

Minimal changes to get platform ops (renamed dom0_ops on pv_ops) working
on pv_ops builds.  Pulls in upstream linux-2.6.18-xen.hg's platform.h

[ Impact: add Xen hypercall definitions ]

Signed-off-by: Stephen Tweedie <sct@redhat.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 arch/x86/include/asm/xen/hypercall.h |    8 ++
 include/xen/interface/platform.h     |  222 ++++++++++++++++++++++++++++++++++
 include/xen/interface/xen.h          |    2 +
 3 files changed, 232 insertions(+), 0 deletions(-)
 create mode 100644 include/xen/interface/platform.h

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index a3c28ae..3d10d04 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -45,6 +45,7 @@
 #include <xen/interface/xen.h>
 #include <xen/interface/sched.h>
 #include <xen/interface/physdev.h>
+#include <xen/interface/platform.h>
 
 /*
  * The hypercall asms have to meet several constraints:
@@ -299,6 +300,13 @@ HYPERVISOR_set_timer_op(u64 timeout)
 }
 
 static inline int
+HYPERVISOR_dom0_op(struct xen_platform_op *platform_op)
+{
+	platform_op->interface_version = XENPF_INTERFACE_VERSION;
+	return _hypercall1(int, dom0_op, platform_op);
+}
+
+static inline int
 HYPERVISOR_set_debugreg(int reg, unsigned long value)
 {
 	return _hypercall2(int, set_debugreg, reg, value);
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
new file mode 100644
index 0000000..83e4714
--- /dev/null
+++ b/include/xen/interface/platform.h
@@ -0,0 +1,222 @@
+/******************************************************************************
+ * platform.h
+ *
+ * Hardware platform operations. Intended for use by domain-0 kernel.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2002-2006, K Fraser
+ */
+
+#ifndef __XEN_PUBLIC_PLATFORM_H__
+#define __XEN_PUBLIC_PLATFORM_H__
+
+#include "xen.h"
+
+#define XENPF_INTERFACE_VERSION 0x03000001
+
+/*
+ * Set clock such that it would read <secs,nsecs> after 00:00:00 UTC,
+ * 1 January, 1970 if the current system time was <system_time>.
+ */
+#define XENPF_settime             17
+struct xenpf_settime {
+    /* IN variables. */
+    uint32_t secs;
+    uint32_t nsecs;
+    uint64_t system_time;
+};
+typedef struct xenpf_settime xenpf_settime_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_settime_t);
+
+/*
+ * Request memory range (@mfn, @mfn+@nr_mfns-1) to have type @type.
+ * On x86, @type is an architecture-defined MTRR memory type.
+ * On success, returns the MTRR that was used (@reg) and a handle that can
+ * be passed to XENPF_DEL_MEMTYPE to accurately tear down the new setting.
+ * (x86-specific).
+ */
+#define XENPF_add_memtype         31
+struct xenpf_add_memtype {
+    /* IN variables. */
+    unsigned long mfn;
+    uint64_t nr_mfns;
+    uint32_t type;
+    /* OUT variables. */
+    uint32_t handle;
+    uint32_t reg;
+};
+typedef struct xenpf_add_memtype xenpf_add_memtype_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_add_memtype_t);
+
+/*
+ * Tear down an existing memory-range type. If @handle is remembered then it
+ * should be passed in to accurately tear down the correct setting (in case
+ * of overlapping memory regions with differing types). If it is not known
+ * then @handle should be set to zero. In all cases @reg must be set.
+ * (x86-specific).
+ */
+#define XENPF_del_memtype         32
+struct xenpf_del_memtype {
+    /* IN variables. */
+    uint32_t handle;
+    uint32_t reg;
+};
+typedef struct xenpf_del_memtype xenpf_del_memtype_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_del_memtype_t);
+
+/* Read current type of an MTRR (x86-specific). */
+#define XENPF_read_memtype        33
+struct xenpf_read_memtype {
+    /* IN variables. */
+    uint32_t reg;
+    /* OUT variables. */
+    unsigned long mfn;
+    uint64_t nr_mfns;
+    uint32_t type;
+};
+typedef struct xenpf_read_memtype xenpf_read_memtype_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_read_memtype_t);
+
+#define XENPF_microcode_update    35
+struct xenpf_microcode_update {
+    /* IN variables. */
+    GUEST_HANDLE(void) data;          /* Pointer to microcode data */
+    uint32_t length;                  /* Length of microcode data. */
+};
+typedef struct xenpf_microcode_update xenpf_microcode_update_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_microcode_update_t);
+
+#define XENPF_platform_quirk      39
+#define QUIRK_NOIRQBALANCING      1 /* Do not restrict IO-APIC RTE targets */
+#define QUIRK_IOAPIC_BAD_REGSEL   2 /* IO-APIC REGSEL forgets its value    */
+#define QUIRK_IOAPIC_GOOD_REGSEL  3 /* IO-APIC REGSEL behaves properly     */
+struct xenpf_platform_quirk {
+    /* IN variables. */
+    uint32_t quirk_id;
+};
+typedef struct xenpf_platform_quirk xenpf_platform_quirk_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_platform_quirk_t);
+
+#define XENPF_firmware_info       50
+#define XEN_FW_DISK_INFO          1 /* from int 13 AH=08/41/48 */
+#define XEN_FW_DISK_MBR_SIGNATURE 2 /* from MBR offset 0x1b8 */
+#define XEN_FW_VBEDDC_INFO        3 /* from int 10 AX=4f15 */
+struct xenpf_firmware_info {
+	/* IN variables. */
+	uint32_t type;
+	uint32_t index;
+	/* OUT variables. */
+	union {
+		struct {
+			/* Int13, Fn48: Check Extensions Present. */
+			uint8_t device;                   /* %dl: bios device number */
+			uint8_t version;                  /* %ah: major version      */
+			uint16_t interface_support;       /* %cx: support bitmap     */
+			/* Int13, Fn08: Legacy Get Device Parameters. */
+			uint16_t legacy_max_cylinder;     /* %cl[7:6]:%ch: max cyl # */
+			uint8_t legacy_max_head;          /* %dh: max head #         */
+			uint8_t legacy_sectors_per_track; /* %cl[5:0]: max sector #  */
+			/* Int13, Fn41: Get Device Parameters (as filled into %ds:%esi). */
+			/* NB. First uint16_t of buffer must be set to buffer size.      */
+			GUEST_HANDLE(void) edd_params;
+		} disk_info; /* XEN_FW_DISK_INFO */
+		struct {
+			uint8_t device;                   /* bios device number  */
+			uint32_t mbr_signature;           /* offset 0x1b8 in mbr */
+		} disk_mbr_signature; /* XEN_FW_DISK_MBR_SIGNATURE */
+		struct {
+			/* Int10, AX=4F15: Get EDID info. */
+			uint8_t capabilities;
+			uint8_t edid_transfer_time;
+			/* must refer to 128-byte buffer */
+			GUEST_HANDLE(uchar) edid;
+		} vbeddc_info; /* XEN_FW_VBEDDC_INFO */
+	} u;
+};
+typedef struct xenpf_firmware_info xenpf_firmware_info_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_firmware_info_t);
+
+#define XENPF_enter_acpi_sleep    51
+struct xenpf_enter_acpi_sleep {
+	/* IN variables */
+	uint16_t pm1a_cnt_val;      /* PM1a control value. */
+	uint16_t pm1b_cnt_val;      /* PM1b control value. */
+	uint32_t sleep_state;       /* Which state to enter (Sn). */
+	uint32_t flags;             /* Must be zero. */
+};
+typedef struct xenpf_enter_acpi_sleep xenpf_enter_acpi_sleep_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_enter_acpi_sleep_t);
+
+#define XENPF_change_freq         52
+struct xenpf_change_freq {
+	/* IN variables */
+	uint32_t flags; /* Must be zero. */
+	uint32_t cpu;   /* Physical cpu. */
+	uint64_t freq;  /* New frequency (Hz). */
+};
+typedef struct xenpf_change_freq xenpf_change_freq_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_change_freq_t);
+
+/*
+ * Get idle times (nanoseconds since boot) for physical CPUs specified in the
+ * @cpumap_bitmap with range [0..@cpumap_nr_cpus-1]. The @idletime array is
+ * indexed by CPU number; only entries with the corresponding @cpumap_bitmap
+ * bit set are written to. On return, @cpumap_bitmap is modified so that any
+ * non-existent CPUs are cleared. Such CPUs have their @idletime array entry
+ * cleared.
+ */
+#define XENPF_getidletime         53
+struct xenpf_getidletime {
+	/* IN/OUT variables */
+	/* IN: CPUs to interrogate; OUT: subset of IN which are present */
+	GUEST_HANDLE(uchar) cpumap_bitmap;
+	/* IN variables */
+	/* Size of cpumap bitmap. */
+	uint32_t cpumap_nr_cpus;
+	/* Must be indexable for every cpu in cpumap_bitmap. */
+	GUEST_HANDLE(uint64_t) idletime;
+	/* OUT variables */
+	/* System time when the idletime snapshots were taken. */
+	uint64_t now;
+};
+typedef struct xenpf_getidletime xenpf_getidletime_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_getidletime_t);
+
+struct xen_platform_op {
+	uint32_t cmd;
+	uint32_t interface_version; /* XENPF_INTERFACE_VERSION */
+	union {
+		struct xenpf_settime           settime;
+		struct xenpf_add_memtype       add_memtype;
+		struct xenpf_del_memtype       del_memtype;
+		struct xenpf_read_memtype      read_memtype;
+		struct xenpf_microcode_update  microcode;
+		struct xenpf_platform_quirk    platform_quirk;
+		struct xenpf_firmware_info     firmware_info;
+		struct xenpf_enter_acpi_sleep  enter_acpi_sleep;
+		struct xenpf_change_freq       change_freq;
+		struct xenpf_getidletime       getidletime;
+		uint8_t                        pad[128];
+	} u;
+};
+typedef struct xen_platform_op xen_platform_op_t;
+DEFINE_GUEST_HANDLE_STRUCT(xen_platform_op_t);
+
+#endif /* __XEN_PUBLIC_PLATFORM_H__ */
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 2befa3e..18b5599 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -461,6 +461,8 @@ typedef uint8_t xen_domain_handle_t[16];
 #define __mk_unsigned_long(x) x ## UL
 #define mk_unsigned_long(x) __mk_unsigned_long(x)
 
+DEFINE_GUEST_HANDLE(uint64_t);
+
 #else /* __ASSEMBLY__ */
 
 /* In assembly code we cannot use C numeric constant suffixes. */
-- 
1.7.3.5

[PATCH 1/2] xen dom0: Add support for the platform_ops hypercall

[Jeremy Fitzhardinge]

From: Stephen Tweedie <sct@redhat.com>

Minimal changes to get platform ops (renamed dom0_ops on pv_ops) working
on pv_ops builds.  Pulls in upstream linux-2.6.18-xen.hg's platform.h

[ Impact: add Xen hypercall definitions ]

Signed-off-by: Stephen Tweedie <sct@redhat.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 arch/x86/include/asm/xen/hypercall.h |    8 ++
 include/xen/interface/platform.h     |  222 ++++++++++++++++++++++++++++++++++
 include/xen/interface/xen.h          |    2 +
 3 files changed, 232 insertions(+), 0 deletions(-)
 create mode 100644 include/xen/interface/platform.h

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index a3c28ae..3d10d04 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -45,6 +45,7 @@
 #include <xen/interface/xen.h>
 #include <xen/interface/sched.h>
 #include <xen/interface/physdev.h>
+#include <xen/interface/platform.h>
 
 /*
  * The hypercall asms have to meet several constraints:
@@ -299,6 +300,13 @@ HYPERVISOR_set_timer_op(u64 timeout)
 }
 
 static inline int
+HYPERVISOR_dom0_op(struct xen_platform_op *platform_op)
+{
+	platform_op->interface_version = XENPF_INTERFACE_VERSION;
+	return _hypercall1(int, dom0_op, platform_op);
+}
+
+static inline int
 HYPERVISOR_set_debugreg(int reg, unsigned long value)
 {
 	return _hypercall2(int, set_debugreg, reg, value);
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
new file mode 100644
index 0000000..83e4714
--- /dev/null
+++ b/include/xen/interface/platform.h
@@ -0,0 +1,222 @@
+/******************************************************************************
+ * platform.h
+ *
+ * Hardware platform operations. Intended for use by domain-0 kernel.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2002-2006, K Fraser
+ */
+
+#ifndef __XEN_PUBLIC_PLATFORM_H__
+#define __XEN_PUBLIC_PLATFORM_H__
+
+#include "xen.h"
+
+#define XENPF_INTERFACE_VERSION 0x03000001
+
+/*
+ * Set clock such that it would read <secs,nsecs> after 00:00:00 UTC,
+ * 1 January, 1970 if the current system time was <system_time>.
+ */
+#define XENPF_settime             17
+struct xenpf_settime {
+    /* IN variables. */
+    uint32_t secs;
+    uint32_t nsecs;
+    uint64_t system_time;
+};
+typedef struct xenpf_settime xenpf_settime_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_settime_t);
+
+/*
+ * Request memory range (@mfn, @mfn+@nr_mfns-1) to have type @type.
+ * On x86, @type is an architecture-defined MTRR memory type.
+ * On success, returns the MTRR that was used (@reg) and a handle that can
+ * be passed to XENPF_DEL_MEMTYPE to accurately tear down the new setting.
+ * (x86-specific).
+ */
+#define XENPF_add_memtype         31
+struct xenpf_add_memtype {
+    /* IN variables. */
+    unsigned long mfn;
+    uint64_t nr_mfns;
+    uint32_t type;
+    /* OUT variables. */
+    uint32_t handle;
+    uint32_t reg;
+};
+typedef struct xenpf_add_memtype xenpf_add_memtype_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_add_memtype_t);
+
+/*
+ * Tear down an existing memory-range type. If @handle is remembered then it
+ * should be passed in to accurately tear down the correct setting (in case
+ * of overlapping memory regions with differing types). If it is not known
+ * then @handle should be set to zero. In all cases @reg must be set.
+ * (x86-specific).
+ */
+#define XENPF_del_memtype         32
+struct xenpf_del_memtype {
+    /* IN variables. */
+    uint32_t handle;
+    uint32_t reg;
+};
+typedef struct xenpf_del_memtype xenpf_del_memtype_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_del_memtype_t);
+
+/* Read current type of an MTRR (x86-specific). */
+#define XENPF_read_memtype        33
+struct xenpf_read_memtype {
+    /* IN variables. */
+    uint32_t reg;
+    /* OUT variables. */
+    unsigned long mfn;
+    uint64_t nr_mfns;
+    uint32_t type;
+};
+typedef struct xenpf_read_memtype xenpf_read_memtype_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_read_memtype_t);
+
+#define XENPF_microcode_update    35
+struct xenpf_microcode_update {
+    /* IN variables. */
+    GUEST_HANDLE(void) data;          /* Pointer to microcode data */
+    uint32_t length;                  /* Length of microcode data. */
+};
+typedef struct xenpf_microcode_update xenpf_microcode_update_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_microcode_update_t);
+
+#define XENPF_platform_quirk      39
+#define QUIRK_NOIRQBALANCING      1 /* Do not restrict IO-APIC RTE targets */
+#define QUIRK_IOAPIC_BAD_REGSEL   2 /* IO-APIC REGSEL forgets its value    */
+#define QUIRK_IOAPIC_GOOD_REGSEL  3 /* IO-APIC REGSEL behaves properly     */
+struct xenpf_platform_quirk {
+    /* IN variables. */
+    uint32_t quirk_id;
+};
+typedef struct xenpf_platform_quirk xenpf_platform_quirk_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_platform_quirk_t);
+
+#define XENPF_firmware_info       50
+#define XEN_FW_DISK_INFO          1 /* from int 13 AH=08/41/48 */
+#define XEN_FW_DISK_MBR_SIGNATURE 2 /* from MBR offset 0x1b8 */
+#define XEN_FW_VBEDDC_INFO        3 /* from int 10 AX=4f15 */
+struct xenpf_firmware_info {
+	/* IN variables. */
+	uint32_t type;
+	uint32_t index;
+	/* OUT variables. */
+	union {
+		struct {
+			/* Int13, Fn48: Check Extensions Present. */
+			uint8_t device;                   /* %dl: bios device number */
+			uint8_t version;                  /* %ah: major version      */
+			uint16_t interface_support;       /* %cx: support bitmap     */
+			/* Int13, Fn08: Legacy Get Device Parameters. */
+			uint16_t legacy_max_cylinder;     /* %cl[7:6]:%ch: max cyl # */
+			uint8_t legacy_max_head;          /* %dh: max head #         */
+			uint8_t legacy_sectors_per_track; /* %cl[5:0]: max sector #  */
+			/* Int13, Fn41: Get Device Parameters (as filled into %ds:%esi). */
+			/* NB. First uint16_t of buffer must be set to buffer size.      */
+			GUEST_HANDLE(void) edd_params;
+		} disk_info; /* XEN_FW_DISK_INFO */
+		struct {
+			uint8_t device;                   /* bios device number  */
+			uint32_t mbr_signature;           /* offset 0x1b8 in mbr */
+		} disk_mbr_signature; /* XEN_FW_DISK_MBR_SIGNATURE */
+		struct {
+			/* Int10, AX=4F15: Get EDID info. */
+			uint8_t capabilities;
+			uint8_t edid_transfer_time;
+			/* must refer to 128-byte buffer */
+			GUEST_HANDLE(uchar) edid;
+		} vbeddc_info; /* XEN_FW_VBEDDC_INFO */
+	} u;
+};
+typedef struct xenpf_firmware_info xenpf_firmware_info_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_firmware_info_t);
+
+#define XENPF_enter_acpi_sleep    51
+struct xenpf_enter_acpi_sleep {
+	/* IN variables */
+	uint16_t pm1a_cnt_val;      /* PM1a control value. */
+	uint16_t pm1b_cnt_val;      /* PM1b control value. */
+	uint32_t sleep_state;       /* Which state to enter (Sn). */
+	uint32_t flags;             /* Must be zero. */
+};
+typedef struct xenpf_enter_acpi_sleep xenpf_enter_acpi_sleep_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_enter_acpi_sleep_t);
+
+#define XENPF_change_freq         52
+struct xenpf_change_freq {
+	/* IN variables */
+	uint32_t flags; /* Must be zero. */
+	uint32_t cpu;   /* Physical cpu. */
+	uint64_t freq;  /* New frequency (Hz). */
+};
+typedef struct xenpf_change_freq xenpf_change_freq_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_change_freq_t);
+
+/*
+ * Get idle times (nanoseconds since boot) for physical CPUs specified in the
+ * @cpumap_bitmap with range [0..@cpumap_nr_cpus-1]. The @idletime array is
+ * indexed by CPU number; only entries with the corresponding @cpumap_bitmap
+ * bit set are written to. On return, @cpumap_bitmap is modified so that any
+ * non-existent CPUs are cleared. Such CPUs have their @idletime array entry
+ * cleared.
+ */
+#define XENPF_getidletime         53
+struct xenpf_getidletime {
+	/* IN/OUT variables */
+	/* IN: CPUs to interrogate; OUT: subset of IN which are present */
+	GUEST_HANDLE(uchar) cpumap_bitmap;
+	/* IN variables */
+	/* Size of cpumap bitmap. */
+	uint32_t cpumap_nr_cpus;
+	/* Must be indexable for every cpu in cpumap_bitmap. */
+	GUEST_HANDLE(uint64_t) idletime;
+	/* OUT variables */
+	/* System time when the idletime snapshots were taken. */
+	uint64_t now;
+};
+typedef struct xenpf_getidletime xenpf_getidletime_t;
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_getidletime_t);
+
+struct xen_platform_op {
+	uint32_t cmd;
+	uint32_t interface_version; /* XENPF_INTERFACE_VERSION */
+	union {
+		struct xenpf_settime           settime;
+		struct xenpf_add_memtype       add_memtype;
+		struct xenpf_del_memtype       del_memtype;
+		struct xenpf_read_memtype      read_memtype;
+		struct xenpf_microcode_update  microcode;
+		struct xenpf_platform_quirk    platform_quirk;
+		struct xenpf_firmware_info     firmware_info;
+		struct xenpf_enter_acpi_sleep  enter_acpi_sleep;
+		struct xenpf_change_freq       change_freq;
+		struct xenpf_getidletime       getidletime;
+		uint8_t                        pad[128];
+	} u;
+};
+typedef struct xen_platform_op xen_platform_op_t;
+DEFINE_GUEST_HANDLE_STRUCT(xen_platform_op_t);
+
+#endif /* __XEN_PUBLIC_PLATFORM_H__ */
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 2befa3e..18b5599 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -461,6 +461,8 @@ typedef uint8_t xen_domain_handle_t[16];
 #define __mk_unsigned_long(x) x ## UL
 #define mk_unsigned_long(x) __mk_unsigned_long(x)
 
+DEFINE_GUEST_HANDLE(uint64_t);
+
 #else /* __ASSEMBLY__ */
 
 /* In assembly code we cannot use C numeric constant suffixes. */
-- 
1.7.3.5

[PATCH 2/2] xen: add CPU microcode update driver

[Jeremy Fitzhardinge]

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Xen does all the hard work for us, including choosing the right update
method for this cpu type and actually doing it for all cpus.  We just
need to supply it with the firmware blob.

Because Xen updates all CPUs (and the kernel's virtual cpu numbers have
no fixed relationship with the underlying physical cpus), we only bother
doing anything for cpu "0".

[ Impact: allow CPU microcode update in Xen dom0 ]
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 arch/x86/include/asm/microcode.h |    9 ++
 arch/x86/kernel/Makefile         |    1 +
 arch/x86/kernel/microcode_core.c |    5 +-
 arch/x86/kernel/microcode_xen.c  |  198 ++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/Kconfig             |    4 +
 5 files changed, 216 insertions(+), 1 deletions(-)
 create mode 100644 arch/x86/kernel/microcode_xen.c

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 2421507..22677d6 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -61,4 +61,13 @@ static inline struct microcode_ops * __init init_amd_microcode(void)
 }
 #endif
 
+#ifdef CONFIG_MICROCODE_XEN
+extern struct microcode_ops * __init init_xen_microcode(void);
+#else
+static inline struct microcode_ops * __init init_xen_microcode(void)
+{
+	return NULL;
+}
+#endif
+
 #endif /* _ASM_X86_MICROCODE_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 34244b2..8fd7a4e 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_PCSPKR_PLATFORM)	+= pcspeaker.o
 microcode-y				:= microcode_core.o
 microcode-$(CONFIG_MICROCODE_INTEL)	+= microcode_intel.o
 microcode-$(CONFIG_MICROCODE_AMD)	+= microcode_amd.o
+microcode-$(CONFIG_MICROCODE_XEN)	+= microcode_xen.o
 obj-$(CONFIG_MICROCODE)			+= microcode.o
 
 obj-$(CONFIG_X86_CHECK_BIOS_CORRUPTION) += check.o
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 1cca374..6550539 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -83,6 +83,7 @@
 #include <linux/fs.h>
 #include <linux/mm.h>
 
+#include <xen/xen.h>
 #include <asm/microcode.h>
 #include <asm/processor.h>
 
@@ -506,7 +507,9 @@ static int __init microcode_init(void)
 	struct cpuinfo_x86 *c = &cpu_data(0);
 	int error;
 
-	if (c->x86_vendor == X86_VENDOR_INTEL)
+	if (xen_pv_domain())
+		microcode_ops = init_xen_microcode();
+	else if (c->x86_vendor == X86_VENDOR_INTEL)
 		microcode_ops = init_intel_microcode();
 	else if (c->x86_vendor == X86_VENDOR_AMD)
 		microcode_ops = init_amd_microcode();
diff --git a/arch/x86/kernel/microcode_xen.c b/arch/x86/kernel/microcode_xen.c
new file mode 100644
index 0000000..9d2a06b
--- /dev/null
+++ b/arch/x86/kernel/microcode_xen.c
@@ -0,0 +1,198 @@
+/*
+ * Xen microcode update driver
+ *
+ * Xen does most of the work here.  We just pass the whole blob into
+ * Xen, and it will apply it to all CPUs as appropriate.  Xen will
+ * worry about how different CPU models are actually updated.
+ */
+#include <linux/sched.h>
+#include <linux/module.h>
+#include <linux/firmware.h>
+#include <linux/vmalloc.h>
+#include <linux/uaccess.h>
+
+#include <asm/microcode.h>
+
+#include <xen/xen.h>
+#include <xen/interface/platform.h>
+#include <xen/interface/xen.h>
+
+#include <asm/xen/hypercall.h>
+#include <asm/xen/hypervisor.h>
+
+MODULE_DESCRIPTION("Xen microcode update driver");
+MODULE_LICENSE("GPL");
+
+struct xen_microcode {
+	size_t len;
+	char data[0];
+};
+
+static int xen_microcode_update(int cpu)
+{
+	int err;
+	struct xen_platform_op op;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct xen_microcode *uc = uci->mc;
+
+	if (uc == NULL || uc->len == 0) {
+		/*
+		 * We do all cpus at once, so we don't need to do
+		 * other cpus explicitly (besides, these vcpu numbers
+		 * have no relationship to underlying physical cpus).
+		 */
+		return 0;
+	}
+
+	op.cmd = XENPF_microcode_update;
+	set_xen_guest_handle(op.u.microcode.data, uc->data);
+	op.u.microcode.length = uc->len;
+
+	err = HYPERVISOR_dom0_op(&op);
+
+	if (err != 0)
+		printk(KERN_WARNING "microcode_xen: microcode update failed: %d\n", err);
+
+	return err;
+}
+
+static enum ucode_state xen_request_microcode_fw(int cpu, struct device *device)
+{
+	char name[30];
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	const struct firmware *firmware;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	enum ucode_state ret;
+	struct xen_microcode *uc;
+	size_t size;
+	int err;
+
+	switch (c->x86_vendor) {
+	case X86_VENDOR_INTEL:
+		snprintf(name, sizeof(name), "intel-ucode/%02x-%02x-%02x",
+			 c->x86, c->x86_model, c->x86_mask);
+		break;
+
+	case X86_VENDOR_AMD:
+		snprintf(name, sizeof(name), "amd-ucode/microcode_amd.bin");
+		break;
+
+	default:
+		return UCODE_NFOUND;
+	}
+
+	err = request_firmware(&firmware, name, device);
+	if (err) {
+		pr_debug("microcode: data file %s load failed\n", name);
+		return UCODE_NFOUND;
+	}
+
+	/*
+	 * Only bother getting real firmware for cpu 0; the others get
+	 * dummy placeholders.
+	 */
+	if (cpu == 0)
+		size = firmware->size;
+	else
+		size = 0;
+
+	if (uci->mc != NULL) {
+		vfree(uci->mc);
+		uci->mc = NULL;
+	}
+
+	ret = UCODE_ERROR;
+	uc = vmalloc(sizeof(*uc) + size);
+	if (uc == NULL)
+		goto out;
+
+	ret = UCODE_OK;
+	uc->len = size;
+	memcpy(uc->data, firmware->data, uc->len);
+
+	uci->mc = uc;
+
+out:
+	release_firmware(firmware);
+
+	return ret;
+}
+
+static enum ucode_state xen_request_microcode_user(int cpu,
+						   const void __user *buf, size_t size)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct xen_microcode *uc;
+	enum ucode_state ret;
+	size_t unread;
+
+	if (cpu != 0) {
+		/* No real firmware for non-zero cpus; just store a
+		   placeholder */
+		size = 0;
+	}
+
+	if (uci->mc != NULL) {
+		vfree(uci->mc);
+		uci->mc = NULL;
+	}
+
+	ret = UCODE_ERROR;
+	uc = vmalloc(sizeof(*uc) + size);
+	if (uc == NULL)
+		goto out;
+
+	uc->len = size;
+
+	ret = UCODE_NFOUND;
+
+	unread = copy_from_user(uc->data, buf, size);
+
+	if (unread != 0) {
+		printk(KERN_WARNING "failed to read %zd of %zd bytes at %p -> %p\n",
+		       unread, size, buf, uc->data);
+		goto out;
+	}
+
+	ret = UCODE_OK;
+
+out:
+	if (ret == 0)
+		uci->mc = uc;
+	else
+		vfree(uc);
+
+	return ret;
+}
+
+static void xen_microcode_fini_cpu(int cpu)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	vfree(uci->mc);
+	uci->mc = NULL;
+}
+
+static int xen_collect_cpu_info(int cpu, struct cpu_signature *sig)
+{
+	sig->sig = 0;
+	sig->pf = 0;
+	sig->rev = 0;
+
+	return 0;
+}
+
+static struct microcode_ops microcode_xen_ops = {
+	.request_microcode_user		  = xen_request_microcode_user,
+	.request_microcode_fw             = xen_request_microcode_fw,
+	.collect_cpu_info                 = xen_collect_cpu_info,
+	.apply_microcode                  = xen_microcode_update,
+	.microcode_fini_cpu               = xen_microcode_fini_cpu,
+};
+
+struct microcode_ops * __init init_xen_microcode(void)
+{
+	if (!xen_initial_domain())
+		return NULL;
+	return &microcode_xen_ops;
+}
diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index 5b54892..384e0a5 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -48,3 +48,7 @@ config XEN_DEBUG_FS
 	help
 	  Enable statistics output and various tuning options in debugfs.
 	  Enabling this option may incur a significant performance overhead.
+
+config MICROCODE_XEN
+       def_bool y
+       depends on XEN_DOM0 && MICROCODE
\ No newline at end of file
-- 
1.7.3.5

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 
> Hi all,
> 
> I'm proposing this for 2.6.39.
> 
> This series adds a new "Xen" microcode update type, in addition to
> Intel and AMD.
> 
> The Xen hypervisor is responsible for performing the actual microcode
> update (since only it knows what physical CPUs are in the system and
> has sufficient privilege to access them), but it requires the dom0
> kernel to provide the actual microcode update data.
> 
> Xen update mechanism is uniform independent of the CPU type, but the
> driver must know where to find the data file, which depends on the CPU
> type.  And since the update hypercall updates all CPUs, we only need
> to execute it once on any CPU - but for simplicity it just runs it only
> on (V)CPU 0.

I don't like this for several reasons:

- ucode should be loaded as early as possible and the perfect place
for that should be the hypervisor and not the dom0 kernel. But I'm not
that familiar with xen design and I guess it would be pretty hard to
do. (Btw, x86 bare metal could also try to improve the situation there
but that's also hard due to the design of the firmware loader (needs
userspace)).

- you're adding a microcode_xen.c as if this is another hw vendor and
you're figuring out which is the proper firmware image based on the
vendor, then you load it and then do the hypercall. This is duplicating
code and inviting future bugs. Everytime the hw vendors decide to change
something to their fw loading mechanism, xen needs to be updated. Also,
you don't do any sanity checks to the ucode image as the vendor code
does which is inacceptable.

What it should do instead, is call into the hw vendor-specific ucode
loading mechanism and do all the image loading/verification there. The
only thing you'd need to supply is a xen-specific ->apply_microcode()
doing the hypercall _after_ the ucode image has been verified and is
good to go. I'm guessing the XENPF_microcode_update does call into the
hypervisor and calls a xen-specific ucode update mechanism based on the
vendor instead of using the vendor-supplied code...

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/30/2011 03:33 AM, Borislav Petkov wrote:
> On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> Hi all,
>>
>> I'm proposing this for 2.6.39.
>>
>> This series adds a new "Xen" microcode update type, in addition to
>> Intel and AMD.
>>
>> The Xen hypervisor is responsible for performing the actual microcode
>> update (since only it knows what physical CPUs are in the system and
>> has sufficient privilege to access them), but it requires the dom0
>> kernel to provide the actual microcode update data.
>>
>> Xen update mechanism is uniform independent of the CPU type, but the
>> driver must know where to find the data file, which depends on the CPU
>> type.  And since the update hypercall updates all CPUs, we only need
>> to execute it once on any CPU - but for simplicity it just runs it only
>> on (V)CPU 0.
> I don't like this for several reasons:
>
> - ucode should be loaded as early as possible and the perfect place
> for that should be the hypervisor and not the dom0 kernel. But I'm not
> that familiar with xen design and I guess it would be pretty hard to
> do. (Btw, x86 bare metal could also try to improve the situation there
> but that's also hard due to the design of the firmware loader (needs
> userspace)).

Yes, its the same issue with Xen or without.  The Xen hypervisor has no
devices, storage or anything else with which it can get microcode data. 
It depends on the dom0 kernel getting it from somewhere and supplying it
to the hypervisor.  In practice this is no different from relying on
usermode to get the firmware update.

In general firmware updates are not critical and not required very
early, so "as soon as reasonably possible" is OK.  (If the firmware
update is critical, it should be supplied as a BIOS update.)

So I think this is moot with respect to this particular patch.

> - you're adding a microcode_xen.c as if this is another hw vendor and
> you're figuring out which is the proper firmware image based on the
> vendor, then you load it and then do the hypercall. This is duplicating
> code and inviting future bugs. Everytime the hw vendors decide to change
> something to their fw loading mechanism, xen needs to be updated. Also,
> you don't do any sanity checks to the ucode image as the vendor code
> does which is inacceptable.

The code within the hypervisor is more or less the same as the kernel's:
it does all the required vendor-specific checks on the firmware and
loads it on all the CPUs with the appropriate mechanism.  The hypercall
ABI is vendor-agnostic, but it assumes that dom0 will supply suitable
microcode for the current CPU vendor (though if it doesn't, the image
will presumably be rejected).

So from that perspective, it is a distinct microcode loading mechanism,
akin to a vendor-specific one.  The awkward part is getting the filename
to actually request from usermode, which is Intel/AMD/whatever specific,
which results in duplicated code to generate that pathname.

> What it should do instead, is call into the hw vendor-specific ucode
> loading mechanism and do all the image loading/verification there. The
> only thing you'd need to supply is a xen-specific ->apply_microcode()
> doing the hypercall _after_ the ucode image has been verified and is
> good to go. I'm guessing the XENPF_microcode_update does call into the
> hypervisor and calls a xen-specific ucode update mechanism based on the
> vendor instead of using the vendor-supplied code...

Well, I was trying to avoid putting Xen-specific code into the existing
Intel/AMD loaders.  That doesn't seem any cleaner.

I could export "my firmware pathname" functions from them and have the
Xen driver call those, rather than duplicating the pathname construction
code.  Would that help address your concerns?

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Sun, Jan 30, 2011 at 06:34:33PM -0800, Jeremy Fitzhardinge wrote:
> > - ucode should be loaded as early as possible and the perfect place
> > for that should be the hypervisor and not the dom0 kernel. But I'm not
> > that familiar with xen design and I guess it would be pretty hard to
> > do. (Btw, x86 bare metal could also try to improve the situation there
> > but that's also hard due to the design of the firmware loader (needs
> > userspace)).
> 
> Yes, its the same issue with Xen or without.  The Xen hypervisor has no
> devices, storage or anything else with which it can get microcode data. 
> It depends on the dom0 kernel getting it from somewhere and supplying it
> to the hypervisor.  In practice this is no different from relying on
> usermode to get the firmware update.
> 
> In general firmware updates are not critical and not required very
> early, so "as soon as reasonably possible" is OK.

Yep.

I can imagine cases where CPU ucode might need to get applied as early
as possible.

> (If the firmware update is critical, it should be supplied as a BIOS
> update.)

I can also imagine cases where BIOS update is not an option/is not
provided and we'd need the ucode applied by the OS too :). Think BIOS
quirks in the kernel whereas all that can be fixed in the BIOS.

> So I think this is moot with respect to this particular patch.
> 
> > - you're adding a microcode_xen.c as if this is another hw vendor and
> > you're figuring out which is the proper firmware image based on the
> > vendor, then you load it and then do the hypercall. This is duplicating
> > code and inviting future bugs. Everytime the hw vendors decide to change
> > something to their fw loading mechanism, xen needs to be updated. Also,
> > you don't do any sanity checks to the ucode image as the vendor code
> > does which is inacceptable.
> 
> The code within the hypervisor is more or less the same as the kernel's:
> it does all the required vendor-specific checks on the firmware and
> loads it on all the CPUs with the appropriate mechanism.  The hypercall
> ABI is vendor-agnostic, but it assumes that dom0 will supply suitable
> microcode for the current CPU vendor (though if it doesn't, the image
> will presumably be rejected).
> 
> So from that perspective, it is a distinct microcode loading mechanism,
> akin to a vendor-specific one.  The awkward part is getting the filename
> to actually request from usermode, which is Intel/AMD/whatever specific,
> which results in duplicated code to generate that pathname.
> 
> > What it should do instead, is call into the hw vendor-specific ucode
> > loading mechanism and do all the image loading/verification there. The
> > only thing you'd need to supply is a xen-specific ->apply_microcode()
> > doing the hypercall _after_ the ucode image has been verified and is
> > good to go. I'm guessing the XENPF_microcode_update does call into the
> > hypervisor and calls a xen-specific ucode update mechanism based on the
> > vendor instead of using the vendor-supplied code...
> 
> Well, I was trying to avoid putting Xen-specific code into the existing
> Intel/AMD loaders.  That doesn't seem any cleaner.
> 
> I could export "my firmware pathname" functions from them and have the
> Xen driver call those, rather than duplicating the pathname construction
> code.  Would that help address your concerns?

Well, I was thinking even more radically than that. How about

1. microcode_xen.c figures out which struct microcode_ops to use based
on the hw vendor;

2. overwrites the ->apply_microcode ptr with the hypercall wrapper

3. dom0 uses it to load the firmware image and do all checks to it

4. eventually, the hypervisor gets to apply the _verified_ microcode
image (no more checks needed) using the vendor-specific application
method.

This way there's almost no code duplication, you'll be reusing the
vendor-supplied code in baremetal which gets tested and updated
everytime it needs to and will save you a bunch of work everytime
there's change to it needed to replicate it into the hypervisor.

Btw, if the code within the hypervisor is similar to the kernel's, you
could even save the original ->apply_microcode() pointer from step 2 and
call it in the hypervisor when the XENPF_microcode_update hypercall op
gets called. This way you have 0 code duplication.

I think this'll be very cool :).

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Sun, Jan 30, 2011 at 06:34:33PM -0800, Jeremy Fitzhardinge wrote:
> > - ucode should be loaded as early as possible and the perfect place
> > for that should be the hypervisor and not the dom0 kernel. But I'm not
> > that familiar with xen design and I guess it would be pretty hard to
> > do. (Btw, x86 bare metal could also try to improve the situation there
> > but that's also hard due to the design of the firmware loader (needs
> > userspace)).
> 
> Yes, its the same issue with Xen or without.  The Xen hypervisor has no
> devices, storage or anything else with which it can get microcode data. 
> It depends on the dom0 kernel getting it from somewhere and supplying it
> to the hypervisor.  In practice this is no different from relying on
> usermode to get the firmware update.
> 
> In general firmware updates are not critical and not required very
> early, so "as soon as reasonably possible" is OK.

Yep.

I can imagine cases where CPU ucode might need to get applied as early
as possible.

> (If the firmware update is critical, it should be supplied as a BIOS
> update.)

I can also imagine cases where BIOS update is not an option/is not
provided and we'd need the ucode applied by the OS too :). Think BIOS
quirks in the kernel whereas all that can be fixed in the BIOS.

> So I think this is moot with respect to this particular patch.
> 
> > - you're adding a microcode_xen.c as if this is another hw vendor and
> > you're figuring out which is the proper firmware image based on the
> > vendor, then you load it and then do the hypercall. This is duplicating
> > code and inviting future bugs. Everytime the hw vendors decide to change
> > something to their fw loading mechanism, xen needs to be updated. Also,
> > you don't do any sanity checks to the ucode image as the vendor code
> > does which is inacceptable.
> 
> The code within the hypervisor is more or less the same as the kernel's:
> it does all the required vendor-specific checks on the firmware and
> loads it on all the CPUs with the appropriate mechanism.  The hypercall
> ABI is vendor-agnostic, but it assumes that dom0 will supply suitable
> microcode for the current CPU vendor (though if it doesn't, the image
> will presumably be rejected).
> 
> So from that perspective, it is a distinct microcode loading mechanism,
> akin to a vendor-specific one.  The awkward part is getting the filename
> to actually request from usermode, which is Intel/AMD/whatever specific,
> which results in duplicated code to generate that pathname.
> 
> > What it should do instead, is call into the hw vendor-specific ucode
> > loading mechanism and do all the image loading/verification there. The
> > only thing you'd need to supply is a xen-specific ->apply_microcode()
> > doing the hypercall _after_ the ucode image has been verified and is
> > good to go. I'm guessing the XENPF_microcode_update does call into the
> > hypervisor and calls a xen-specific ucode update mechanism based on the
> > vendor instead of using the vendor-supplied code...
> 
> Well, I was trying to avoid putting Xen-specific code into the existing
> Intel/AMD loaders.  That doesn't seem any cleaner.
> 
> I could export "my firmware pathname" functions from them and have the
> Xen driver call those, rather than duplicating the pathname construction
> code.  Would that help address your concerns?

Well, I was thinking even more radically than that. How about

1. microcode_xen.c figures out which struct microcode_ops to use based
on the hw vendor;

2. overwrites the ->apply_microcode ptr with the hypercall wrapper

3. dom0 uses it to load the firmware image and do all checks to it

4. eventually, the hypervisor gets to apply the _verified_ microcode
image (no more checks needed) using the vendor-specific application
method.

This way there's almost no code duplication, you'll be reusing the
vendor-supplied code in baremetal which gets tested and updated
everytime it needs to and will save you a bunch of work everytime
there's change to it needed to replicate it into the hypervisor.

Btw, if the code within the hypervisor is similar to the kernel's, you
could even save the original ->apply_microcode() pointer from step 2 and
call it in the hypervisor when the XENPF_microcode_update hypercall op
gets called. This way you have 0 code duplication.

I think this'll be very cool :).

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/30/2011 11:02 PM, Borislav Petkov wrote:
>> Well, I was trying to avoid putting Xen-specific code into the existing
>> Intel/AMD loaders.  That doesn't seem any cleaner.
>>
>> I could export "my firmware pathname" functions from them and have the
>> Xen driver call those, rather than duplicating the pathname construction
>> code.  Would that help address your concerns?
> Well, I was thinking even more radically than that. How about
>
> 1. microcode_xen.c figures out which struct microcode_ops to use based
> on the hw vendor;
>
> 2. overwrites the ->apply_microcode ptr with the hypercall wrapper
>
> 3. dom0 uses it to load the firmware image and do all checks to it

That could be made to work, but I don't really see it as being an
improvement.  The whole "overwriting bits of other people's ops
structures" thing seems like a pretty bad idea for long term
maintainability.

> 4. eventually, the hypervisor gets to apply the _verified_ microcode
> image (no more checks needed) using the vendor-specific application
> method.
>
> This way there's almost no code duplication, you'll be reusing the
> vendor-supplied code in baremetal which gets tested and updated
> everytime it needs to and will save you a bunch of work everytime
> there's change to it needed to replicate it into the hypervisor.

In general Xen tries to avoid trusting its domains.  Admittedly, dom0 is
special in that it is already somewhat trusted, but even dom0 is
constrained by Xen.  For microcode, Xen just depends on it to provide a
best-possible microcode file, then Xen+the CPU do the work of fully
validating it and installing it.

> Btw, if the code within the hypervisor is similar to the kernel's, you
> could even save the original ->apply_microcode() pointer from step 2 and
> call it in the hypervisor when the XENPF_microcode_update hypercall op
> gets called. This way you have 0 code duplication.

The hypervisor and its domains are completely separate pieces of code. 
This is akin to suggesting the kernel directly jump through a pointer
and to run some usermode code.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/30/2011 11:02 PM, Borislav Petkov wrote:
>> Well, I was trying to avoid putting Xen-specific code into the existing
>> Intel/AMD loaders.  That doesn't seem any cleaner.
>>
>> I could export "my firmware pathname" functions from them and have the
>> Xen driver call those, rather than duplicating the pathname construction
>> code.  Would that help address your concerns?
> Well, I was thinking even more radically than that. How about
>
> 1. microcode_xen.c figures out which struct microcode_ops to use based
> on the hw vendor;
>
> 2. overwrites the ->apply_microcode ptr with the hypercall wrapper
>
> 3. dom0 uses it to load the firmware image and do all checks to it

That could be made to work, but I don't really see it as being an
improvement.  The whole "overwriting bits of other people's ops
structures" thing seems like a pretty bad idea for long term
maintainability.

> 4. eventually, the hypervisor gets to apply the _verified_ microcode
> image (no more checks needed) using the vendor-specific application
> method.
>
> This way there's almost no code duplication, you'll be reusing the
> vendor-supplied code in baremetal which gets tested and updated
> everytime it needs to and will save you a bunch of work everytime
> there's change to it needed to replicate it into the hypervisor.

In general Xen tries to avoid trusting its domains.  Admittedly, dom0 is
special in that it is already somewhat trusted, but even dom0 is
constrained by Xen.  For microcode, Xen just depends on it to provide a
best-possible microcode file, then Xen+the CPU do the work of fully
validating it and installing it.

> Btw, if the code within the hypervisor is similar to the kernel's, you
> could even save the original ->apply_microcode() pointer from step 2 and
> call it in the hypervisor when the XENPF_microcode_update hypercall op
> gets called. This way you have 0 code duplication.

The hypervisor and its domains are completely separate pieces of code. 
This is akin to suggesting the kernel directly jump through a pointer
and to run some usermode code.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Mon, Jan 31, 2011 at 10:17:03AM -0800, Jeremy Fitzhardinge wrote:
> On 01/30/2011 11:02 PM, Borislav Petkov wrote:
> >> Well, I was trying to avoid putting Xen-specific code into the existing
> >> Intel/AMD loaders.  That doesn't seem any cleaner.
> >>
> >> I could export "my firmware pathname" functions from them and have the
> >> Xen driver call those, rather than duplicating the pathname construction
> >> code.  Would that help address your concerns?
> > Well, I was thinking even more radically than that. How about
> >
> > 1. microcode_xen.c figures out which struct microcode_ops to use based
> > on the hw vendor;
> >
> > 2. overwrites the ->apply_microcode ptr with the hypercall wrapper
> >
> > 3. dom0 uses it to load the firmware image and do all checks to it
> 
> That could be made to work, but I don't really see it as being an
> improvement.

WTF? How is

* almost no code duplication
* not adding Xen-specific checks to generic arch code
* relying on already tested codepaths

not an improvement?

> The whole "overwriting bits of other people's ops structures" thing
> seems like a pretty bad idea for long term maintainability.

I don't think that's an issue: you either load microcode_xen in dom0
(x)or the respective vendor driver on baremetal.

> > 4. eventually, the hypervisor gets to apply the _verified_ microcode
> > image (no more checks needed) using the vendor-specific application
> > method.
> >
> > This way there's almost no code duplication, you'll be reusing the
> > vendor-supplied code in baremetal which gets tested and updated
> > everytime it needs to and will save you a bunch of work everytime
> > there's change to it needed to replicate it into the hypervisor.
> 
> In general Xen tries to avoid trusting its domains.  Admittedly, dom0 is
> special in that it is already somewhat trusted, but even dom0 is
> constrained by Xen.  For microcode, Xen just depends on it to provide a
> best-possible microcode file, then Xen+the CPU do the work of fully
> validating it and installing it.

Well, the CPU doesn't trust the microcode provided by the software
either. And why should it?

All I'm saying is, you should try as best as possible to avoid code
duplication and the need for replicating functionality to Xen, thus
doubling - even multiplying - the effort for coding/testing baremetal
and then Xen. Microcode is a perfect example since the vendors do all
their testing/verification on baremetal anyway and the rest should
benefit from that work.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/31/2011 03:41 PM, Borislav Petkov wrote:
> On Mon, Jan 31, 2011 at 10:17:03AM -0800, Jeremy Fitzhardinge wrote:
>> On 01/30/2011 11:02 PM, Borislav Petkov wrote:
>>>> Well, I was trying to avoid putting Xen-specific code into the existing
>>>> Intel/AMD loaders.  That doesn't seem any cleaner.
>>>>
>>>> I could export "my firmware pathname" functions from them and have the
>>>> Xen driver call those, rather than duplicating the pathname construction
>>>> code.  Would that help address your concerns?
>>> Well, I was thinking even more radically than that. How about
>>>
>>> 1. microcode_xen.c figures out which struct microcode_ops to use based
>>> on the hw vendor;
>>>
>>> 2. overwrites the ->apply_microcode ptr with the hypercall wrapper
>>>
>>> 3. dom0 uses it to load the firmware image and do all checks to it
>> That could be made to work, but I don't really see it as being an
>> improvement.
> WTF? How is
>
> * almost no code duplication
> * not adding Xen-specific checks to generic arch code
> * relying on already tested codepaths
>
> not an improvement?

My understanding of what you're proposing is:

   1. the normal CPU-specific microcode driver starts up
   2. an if (xen) test overrides the apply_microcode to call a
      Xen-specific function
   3. the driver requests the firmware, loads and verifies it as normal
   4. the microcode is applied via the Xen apply_microcode function

With (2) needing a Xen-specific change to both the Intel and AMD drivers.

If not, what are you proposing in detail?

Are you instead thinking:

    * Xen-specific microcode driver starts up
    * It calls either the intel or amd request_microcode_fw as needed
    * apply the the loaded data via hypercall

?

But all this is flawed because the microcode_intel/amd.c drivers assume
they can see all the CPUs present in the system, and load suitable
microcode for each specific one.  But a kernel in a Xen domain only has
virtual CPUs - not physical ones - and has no idea how to get
appropriate microcode data for all the physical CPUs in the system.

>>> 4. eventually, the hypervisor gets to apply the _verified_ microcode
>>> image (no more checks needed) using the vendor-specific application
>>> method.
>>>
>>> This way there's almost no code duplication, you'll be reusing the
>>> vendor-supplied code in baremetal which gets tested and updated
>>> everytime it needs to and will save you a bunch of work everytime
>>> there's change to it needed to replicate it into the hypervisor.
>> In general Xen tries to avoid trusting its domains.  Admittedly, dom0 is
>> special in that it is already somewhat trusted, but even dom0 is
>> constrained by Xen.  For microcode, Xen just depends on it to provide a
>> best-possible microcode file, then Xen+the CPU do the work of fully
>> validating it and installing it.
> Well, the CPU doesn't trust the microcode provided by the software
> either. And why should it?

Right.  There's nothing really for the driver to do (kernel or Xen). 
The Intel driver does a bunch of checksum checking, which is presumably
useful for detecting a corrupted firmware update early, and splits out
the chunks specific to the current cpus.  The AMD driver does no
checking per-se, but also looks for processor-specific microcode update
chunks.

In the Xen case, since a domain is not necessarily seeing all the
details of the underlying physical cpus, it makes most sense for
usermode to just pass in the whole microcode file and let Xen do all the
checking/filtering based on complete knowledge of the system.

> All I'm saying is, you should try as best as possible to avoid code
> duplication and the need for replicating functionality to Xen, thus
> doubling - even multiplying - the effort for coding/testing baremetal
> and then Xen. Microcode is a perfect example since the vendors do all
> their testing/verification on baremetal anyway and the rest should
> benefit from that work.

CPU vendors test Xen, and Intel is particularly interested in getting
this microcode driver upstream.  The amount of duplicated code is
trivial, and the basic structure of the microcode updates doesn't seem
set to change.  Since Xen has to have all sorts of other CPU-specific
code which at least somewhat overlaps with what's in the kernel a bit
more doesn't matter.

(It's interesting that nobody seems to have been interested enough in
Via to have ever implemented a microcode driver for it - perhaps they
only ever do it from BIOS.)

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/31/2011 03:41 PM, Borislav Petkov wrote:
> On Mon, Jan 31, 2011 at 10:17:03AM -0800, Jeremy Fitzhardinge wrote:
>> On 01/30/2011 11:02 PM, Borislav Petkov wrote:
>>>> Well, I was trying to avoid putting Xen-specific code into the existing
>>>> Intel/AMD loaders.  That doesn't seem any cleaner.
>>>>
>>>> I could export "my firmware pathname" functions from them and have the
>>>> Xen driver call those, rather than duplicating the pathname construction
>>>> code.  Would that help address your concerns?
>>> Well, I was thinking even more radically than that. How about
>>>
>>> 1. microcode_xen.c figures out which struct microcode_ops to use based
>>> on the hw vendor;
>>>
>>> 2. overwrites the ->apply_microcode ptr with the hypercall wrapper
>>>
>>> 3. dom0 uses it to load the firmware image and do all checks to it
>> That could be made to work, but I don't really see it as being an
>> improvement.
> WTF? How is
>
> * almost no code duplication
> * not adding Xen-specific checks to generic arch code
> * relying on already tested codepaths
>
> not an improvement?

My understanding of what you're proposing is:

   1. the normal CPU-specific microcode driver starts up
   2. an if (xen) test overrides the apply_microcode to call a
      Xen-specific function
   3. the driver requests the firmware, loads and verifies it as normal
   4. the microcode is applied via the Xen apply_microcode function

With (2) needing a Xen-specific change to both the Intel and AMD drivers.

If not, what are you proposing in detail?

Are you instead thinking:

    * Xen-specific microcode driver starts up
    * It calls either the intel or amd request_microcode_fw as needed
    * apply the the loaded data via hypercall

?

But all this is flawed because the microcode_intel/amd.c drivers assume
they can see all the CPUs present in the system, and load suitable
microcode for each specific one.  But a kernel in a Xen domain only has
virtual CPUs - not physical ones - and has no idea how to get
appropriate microcode data for all the physical CPUs in the system.

>>> 4. eventually, the hypervisor gets to apply the _verified_ microcode
>>> image (no more checks needed) using the vendor-specific application
>>> method.
>>>
>>> This way there's almost no code duplication, you'll be reusing the
>>> vendor-supplied code in baremetal which gets tested and updated
>>> everytime it needs to and will save you a bunch of work everytime
>>> there's change to it needed to replicate it into the hypervisor.
>> In general Xen tries to avoid trusting its domains.  Admittedly, dom0 is
>> special in that it is already somewhat trusted, but even dom0 is
>> constrained by Xen.  For microcode, Xen just depends on it to provide a
>> best-possible microcode file, then Xen+the CPU do the work of fully
>> validating it and installing it.
> Well, the CPU doesn't trust the microcode provided by the software
> either. And why should it?

Right.  There's nothing really for the driver to do (kernel or Xen). 
The Intel driver does a bunch of checksum checking, which is presumably
useful for detecting a corrupted firmware update early, and splits out
the chunks specific to the current cpus.  The AMD driver does no
checking per-se, but also looks for processor-specific microcode update
chunks.

In the Xen case, since a domain is not necessarily seeing all the
details of the underlying physical cpus, it makes most sense for
usermode to just pass in the whole microcode file and let Xen do all the
checking/filtering based on complete knowledge of the system.

> All I'm saying is, you should try as best as possible to avoid code
> duplication and the need for replicating functionality to Xen, thus
> doubling - even multiplying - the effort for coding/testing baremetal
> and then Xen. Microcode is a perfect example since the vendors do all
> their testing/verification on baremetal anyway and the rest should
> benefit from that work.

CPU vendors test Xen, and Intel is particularly interested in getting
this microcode driver upstream.  The amount of duplicated code is
trivial, and the basic structure of the microcode updates doesn't seem
set to change.  Since Xen has to have all sorts of other CPU-specific
code which at least somewhat overlaps with what's in the kernel a bit
more doesn't matter.

(It's interesting that nobody seems to have been interested enough in
Via to have ever implemented a microcode driver for it - perhaps they
only ever do it from BIOS.)

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

Note: Xen may not have devices, but it is already using multiboot to 
load multiple modules.  It could load the microcode blob that way.

That would enable an earlier loading of microcode, which is a very good 
thing.

	-hpa

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/31/2011 05:11 PM, H. Peter Anvin wrote:
> Note: Xen may not have devices, but it is already using multiboot to
> load multiple modules.  It could load the microcode blob that way.
>
> That would enable an earlier loading of microcode, which is a very
> good thing.

Yes, that is a thought, but it would require some distro support to make
sure the firmware is copied into /boot, and grub updated appropriately.

The principle advantage of updating the microcode driver is that it Just
Works regardless of whether the system is booting native or Xen.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/31/2011 05:11 PM, H. Peter Anvin wrote:
> Note: Xen may not have devices, but it is already using multiboot to
> load multiple modules.  It could load the microcode blob that way.
>
> That would enable an earlier loading of microcode, which is a very
> good thing.

Yes, that is a thought, but it would require some distro support to make
sure the firmware is copied into /boot, and grub updated appropriately.

The principle advantage of updating the microcode driver is that it Just
Works regardless of whether the system is booting native or Xen.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

On 02/01/2011 02:52 PM, Jeremy Fitzhardinge wrote:
> On 01/31/2011 05:11 PM, H. Peter Anvin wrote:
>> Note: Xen may not have devices, but it is already using multiboot to
>> load multiple modules.  It could load the microcode blob that way.
>>
>> That would enable an earlier loading of microcode, which is a very
>> good thing.
> 
> Yes, that is a thought, but it would require some distro support to make
> sure the firmware is copied into /boot, and grub updated appropriately.
> 
> The principle advantage of updating the microcode driver is that it Just
> Works regardless of whether the system is booting native or Xen.
> 

As I mentioned on IRC...

1. Xen already uses Multiboot, so it's a fairly trivial thing to add
another item to the list for the boot loader to get.

2. The only reason we don't currently install microcode from the boot
loader is because of the considerable complexity in adding SMP support
to boot loaders, as it requires handling the APIC system.

3. Arguably on native hardware we should still load the microcode into
RAM in the boot loader, and install it on the very early CPU bringup
path.  That means locking down some (currently) 400K of RAM to handle
different combinations of CPUs, or the additional complexity of
jettisoning microcode which cannot be used while still be able to deal
with hotplug.  I think there is a strong case for this model, which
would mean moving the microcode into /boot anyway.

	-hpa

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 02/02/2011 11:52 AM, H. Peter Anvin wrote:
> As I mentioned on IRC...
>
> 1. Xen already uses Multiboot, so it's a fairly trivial thing to add
> another item to the list for the boot loader to get.
>
> 2. The only reason we don't currently install microcode from the boot
> loader is because of the considerable complexity in adding SMP support
> to boot loaders, as it requires handling the APIC system.
>
> 3. Arguably on native hardware we should still load the microcode into
> RAM in the boot loader, and install it on the very early CPU bringup
> path.  That means locking down some (currently) 400K of RAM to handle
> different combinations of CPUs, or the additional complexity of
> jettisoning microcode which cannot be used while still be able to deal
> with hotplug.  I think there is a strong case for this model, which
> would mean moving the microcode into /boot anyway.

If we can come up with a scheme that works for both native and Xen (or
at least v. similar) that we can get distros to support, then we can
work with that.  That principally means getting the microcode images
into /boot in a pre-digested form (binary, not text, and maybe pack the
Intel and AMD files together in some extensible way - or at least give
them self-describing headers).

But in the meantime it would be nice to have microcode updates under Xen
within the existing model (or failing that, a little patch to prevent
the spew of spurious errors when the kernel tries and fails - but it
would be strongly preferable to actually update microcode).

My main concern is that I want Xen to Just Work - ideally by not
requiring users/admins to do anything.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Thomas Gleixner]

On Wed, 2 Feb 2011, Jeremy Fitzhardinge wrote:

> On 02/02/2011 11:52 AM, H. Peter Anvin wrote:
> > As I mentioned on IRC...
> >
> > 1. Xen already uses Multiboot, so it's a fairly trivial thing to add
> > another item to the list for the boot loader to get.
> >
> > 2. The only reason we don't currently install microcode from the boot
> > loader is because of the considerable complexity in adding SMP support
> > to boot loaders, as it requires handling the APIC system.
> >
> > 3. Arguably on native hardware we should still load the microcode into
> > RAM in the boot loader, and install it on the very early CPU bringup
> > path.  That means locking down some (currently) 400K of RAM to handle
> > different combinations of CPUs, or the additional complexity of
> > jettisoning microcode which cannot be used while still be able to deal
> > with hotplug.  I think there is a strong case for this model, which
> > would mean moving the microcode into /boot anyway.
> 
> If we can come up with a scheme that works for both native and Xen (or
> at least v. similar) that we can get distros to support, then we can
> work with that.  That principally means getting the microcode images
> into /boot in a pre-digested form (binary, not text, and maybe pack the
> Intel and AMD files together in some extensible way - or at least give
> them self-describing headers).
> 
> But in the meantime it would be nice to have microcode updates under Xen
> within the existing model (or failing that, a little patch to prevent
> the spew of spurious errors when the kernel tries and fails - but it
> would be strongly preferable to actually update microcode).
> 
> My main concern is that I want Xen to Just Work - ideally by not
> requiring users/admins to do anything.

Well, that's a noble goal, but the reality is that Xen is not even
close to the point where it Just Works. So instead of slapping some
weird workaround into the kernel, we really should go for the correct
solution right away.

Thanks,

	tglx

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Thomas Gleixner]

On Wed, 2 Feb 2011, Jeremy Fitzhardinge wrote:

> On 02/02/2011 11:52 AM, H. Peter Anvin wrote:
> > As I mentioned on IRC...
> >
> > 1. Xen already uses Multiboot, so it's a fairly trivial thing to add
> > another item to the list for the boot loader to get.
> >
> > 2. The only reason we don't currently install microcode from the boot
> > loader is because of the considerable complexity in adding SMP support
> > to boot loaders, as it requires handling the APIC system.
> >
> > 3. Arguably on native hardware we should still load the microcode into
> > RAM in the boot loader, and install it on the very early CPU bringup
> > path.  That means locking down some (currently) 400K of RAM to handle
> > different combinations of CPUs, or the additional complexity of
> > jettisoning microcode which cannot be used while still be able to deal
> > with hotplug.  I think there is a strong case for this model, which
> > would mean moving the microcode into /boot anyway.
> 
> If we can come up with a scheme that works for both native and Xen (or
> at least v. similar) that we can get distros to support, then we can
> work with that.  That principally means getting the microcode images
> into /boot in a pre-digested form (binary, not text, and maybe pack the
> Intel and AMD files together in some extensible way - or at least give
> them self-describing headers).
> 
> But in the meantime it would be nice to have microcode updates under Xen
> within the existing model (or failing that, a little patch to prevent
> the spew of spurious errors when the kernel tries and fails - but it
> would be strongly preferable to actually update microcode).
> 
> My main concern is that I want Xen to Just Work - ideally by not
> requiring users/admins to do anything.

Well, that's a noble goal, but the reality is that Xen is not even
close to the point where it Just Works. So instead of slapping some
weird workaround into the kernel, we really should go for the correct
solution right away.

Thanks,

	tglx

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Henrique de Moraes Holschuh]

On Wed, 02 Feb 2011, Jeremy Fitzhardinge wrote:
> work with that.  That principally means getting the microcode images
> into /boot in a pre-digested form (binary, not text, and maybe pack the
> Intel and AMD files together in some extensible way - or at least give
> them self-describing headers).

I can get you a tool to manipulate the Intel microcode packs, and output
them in binary format.  I was planning to eventually switch Debian to it
anyway, as microcode.ctl is slow and unsuitable for initramfs use, and it is
high time we junked it.  The tool is somewhat messy, and I am pretty sure my
messy C is not going to get any good taste awards, but it works.

I will just remove support for /lib/firmware/intel-ucode/* from it before
making it public, because I want that hideously bad idea to DIE and it would
be counter-productive to actually create users for it (AFAIK, nobody ever
used /lib/firmware/intel-ucode/*, so we could still fix it).

But I'd really, really appreciate if someone from Intel [that actually cares
for operating system support of microcode updates] could vouch that we're
allowed to do that (convert their text packs to binary packs, merge
microcodes from older packs with the new to have a single pack with all
microcodes in their most up-to-date revision, and distribute the resulting
binary packs) before I make the tool public.

It would not be much of a problem to add AMD support to it as well (or write
a separate tool), just point me to a friendly AMD engineer that will supply
the docs (or point me to them if they're already public), vouch for the fact
that we're allowed to unpack/merge/strip/repack AMD microcode packs, and
test the tool, because I have no AMD boxes at home or at work to test it.

> My main concern is that I want Xen to Just Work - ideally by not
> requiring users/admins to do anything.

I have no experience with Xen.  What do I get from cpuid(0) and cpuid(1) in
dom0 when the bare metal uses Intel CPUs?  And AMD CPUs?   I'd like to teach
the tool to not do anything idiotic under Xen...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

On 02/02/2011 04:55 PM, Henrique de Moraes Holschuh wrote:
> 
> But I'd really, really appreciate if someone from Intel [that actually cares
> for operating system support of microcode updates] could vouch that we're
> allowed to do that (convert their text packs to binary packs, merge
> microcodes from older packs with the new to have a single pack with all
> microcodes in their most up-to-date revision, and distribute the resulting
> binary packs) before I make the tool public.
> 

I'm trying to figure this stuff out already.

The actual conversion isn't a problem, obviously.

	-hpa

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Wed, Feb 02, 2011 at 10:55:17PM -0200, Henrique de Moraes Holschuh wrote:
> It would not be much of a problem to add AMD support to it as well (or write
> a separate tool), just point me to a friendly AMD engineer that will supply
> the docs (or point me to them if they're already public), vouch for the fact
> that we're allowed to unpack/merge/strip/repack AMD microcode packs, and
> test the tool, because I have no AMD boxes at home or at work to test it.

We already have a single container file with all the ucode patches in
it: http://www.amd64.org/support/microcode.html and the microcode driver
in the kernel can look at it and take out the patches it needs based on
the CPU it is running on. Is that what you had in mind?

> > My main concern is that I want Xen to Just Work - ideally by not
> > requiring users/admins to do anything.
> 
> I have no experience with Xen.  What do I get from cpuid(0) and cpuid(1) in
> dom0 when the bare metal uses Intel CPUs?  And AMD CPUs?   I'd like to teach
> the tool to not do anything idiotic under Xen...

Actually, if the microcode image can be provided to the hypervisor early
using multiboot, it should be easy for it to figure out on what hardware
it is running and apply the correct microcode without the need for dom0
to know anything about microcode, IMHO.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Henrique de Moraes Holschuh]

On Thu, 03 Feb 2011, Borislav Petkov wrote:
> On Wed, Feb 02, 2011 at 10:55:17PM -0200, Henrique de Moraes Holschuh wrote:
> > It would not be much of a problem to add AMD support to it as well (or write
> > a separate tool), just point me to a friendly AMD engineer that will supply
> > the docs (or point me to them if they're already public), vouch for the fact
> > that we're allowed to unpack/merge/strip/repack AMD microcode packs, and
> > test the tool, because I have no AMD boxes at home or at work to test it.
> 
> We already have a single container file with all the ucode patches in
> it: http://www.amd64.org/support/microcode.html and the microcode driver
> in the kernel can look at it and take out the patches it needs based on
> the CPU it is running on. Is that what you had in mind?

Validate the container file in userspace, let the user list available
microcode updates, let the user merge multiple container files into a new
one with just the most up-to-date microcodes for each CPU, optionally
filtered for the CPUs currently online, or to the ones specificed in the
command line.

I have a tool that does that for Intel, based on their documentation and
also on the Linux driver.

However, since AMD has so few microcodes in that file and it is so small,
that's probably not useful at all right now.  Maybe in a few years :-)

> > > My main concern is that I want Xen to Just Work - ideally by not
> > > requiring users/admins to do anything.
> > 
> > I have no experience with Xen.  What do I get from cpuid(0) and cpuid(1) in
> > dom0 when the bare metal uses Intel CPUs?  And AMD CPUs?   I'd like to teach
> > the tool to not do anything idiotic under Xen...
> 
> Actually, if the microcode image can be provided to the hypervisor early
> using multiboot, it should be easy for it to figure out on what hardware
> it is running and apply the correct microcode without the need for dom0
> to know anything about microcode, IMHO.

I'd still appreciate that information.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Henrique de Moraes Holschuh]

On Thu, 03 Feb 2011, Borislav Petkov wrote:
> On Wed, Feb 02, 2011 at 10:55:17PM -0200, Henrique de Moraes Holschuh wrote:
> > It would not be much of a problem to add AMD support to it as well (or write
> > a separate tool), just point me to a friendly AMD engineer that will supply
> > the docs (or point me to them if they're already public), vouch for the fact
> > that we're allowed to unpack/merge/strip/repack AMD microcode packs, and
> > test the tool, because I have no AMD boxes at home or at work to test it.
> 
> We already have a single container file with all the ucode patches in
> it: http://www.amd64.org/support/microcode.html and the microcode driver
> in the kernel can look at it and take out the patches it needs based on
> the CPU it is running on. Is that what you had in mind?

Validate the container file in userspace, let the user list available
microcode updates, let the user merge multiple container files into a new
one with just the most up-to-date microcodes for each CPU, optionally
filtered for the CPUs currently online, or to the ones specificed in the
command line.

I have a tool that does that for Intel, based on their documentation and
also on the Linux driver.

However, since AMD has so few microcodes in that file and it is so small,
that's probably not useful at all right now.  Maybe in a few years :-)

> > > My main concern is that I want Xen to Just Work - ideally by not
> > > requiring users/admins to do anything.
> > 
> > I have no experience with Xen.  What do I get from cpuid(0) and cpuid(1) in
> > dom0 when the bare metal uses Intel CPUs?  And AMD CPUs?   I'd like to teach
> > the tool to not do anything idiotic under Xen...
> 
> Actually, if the microcode image can be provided to the hypervisor early
> using multiboot, it should be easy for it to figure out on what hardware
> it is running and apply the correct microcode without the need for dom0
> to know anything about microcode, IMHO.

I'd still appreciate that information.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Wed, Feb 02, 2011 at 11:52:22AM -0800, H. Peter Anvin wrote:
> 3. Arguably on native hardware we should still load the microcode into
> RAM in the boot loader, and install it on the very early CPU bringup
> path.  That means locking down some (currently) 400K of RAM to handle
> different combinations of CPUs, or the additional complexity of
> jettisoning microcode which cannot be used while still be able to deal
> with hotplug.  I think there is a strong case for this model, which
> would mean moving the microcode into /boot anyway.

/me like it, sounds very nifty. So how do we want to do that, we add
a field to the real-mode kernel header that tells us where to find
the microcode image and we take it and apply the ucode somewhere in
do_boot_cpu() path?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Wed, Feb 02, 2011 at 11:52:22AM -0800, H. Peter Anvin wrote:
> 3. Arguably on native hardware we should still load the microcode into
> RAM in the boot loader, and install it on the very early CPU bringup
> path.  That means locking down some (currently) 400K of RAM to handle
> different combinations of CPUs, or the additional complexity of
> jettisoning microcode which cannot be used while still be able to deal
> with hotplug.  I think there is a strong case for this model, which
> would mean moving the microcode into /boot anyway.

/me like it, sounds very nifty. So how do we want to do that, we add
a field to the real-mode kernel header that tells us where to find
the microcode image and we take it and apply the ucode somewhere in
do_boot_cpu() path?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

On 02/02/2011 12:57 PM, Borislav Petkov wrote:
> On Wed, Feb 02, 2011 at 11:52:22AM -0800, H. Peter Anvin wrote:
>> 3. Arguably on native hardware we should still load the microcode into
>> RAM in the boot loader, and install it on the very early CPU bringup
>> path.  That means locking down some (currently) 400K of RAM to handle
>> different combinations of CPUs, or the additional complexity of
>> jettisoning microcode which cannot be used while still be able to deal
>> with hotplug.  I think there is a strong case for this model, which
>> would mean moving the microcode into /boot anyway.
> 
> /me like it, sounds very nifty. So how do we want to do that, we add
> a field to the real-mode kernel header that tells us where to find
> the microcode image and we take it and apply the ucode somewhere in
> do_boot_cpu() path?
> 

We already have a mechanism for passing arbitrary blobs -- the linked
list -- so we don't have to add a new field at all.

	-hpa

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

On 02/02/2011 12:57 PM, Borislav Petkov wrote:
> On Wed, Feb 02, 2011 at 11:52:22AM -0800, H. Peter Anvin wrote:
>> 3. Arguably on native hardware we should still load the microcode into
>> RAM in the boot loader, and install it on the very early CPU bringup
>> path.  That means locking down some (currently) 400K of RAM to handle
>> different combinations of CPUs, or the additional complexity of
>> jettisoning microcode which cannot be used while still be able to deal
>> with hotplug.  I think there is a strong case for this model, which
>> would mean moving the microcode into /boot anyway.
> 
> /me like it, sounds very nifty. So how do we want to do that, we add
> a field to the real-mode kernel header that tells us where to find
> the microcode image and we take it and apply the ucode somewhere in
> do_boot_cpu() path?
> 

We already have a mechanism for passing arbitrary blobs -- the linked
list -- so we don't have to add a new field at all.

	-hpa

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Wed, Feb 02, 2011 at 01:47:18PM -0800, H. Peter Anvin wrote:
> On 02/02/2011 12:57 PM, Borislav Petkov wrote:
> > On Wed, Feb 02, 2011 at 11:52:22AM -0800, H. Peter Anvin wrote:
> >> 3. Arguably on native hardware we should still load the microcode into
> >> RAM in the boot loader, and install it on the very early CPU bringup
> >> path.  That means locking down some (currently) 400K of RAM to handle
> >> different combinations of CPUs, or the additional complexity of
> >> jettisoning microcode which cannot be used while still be able to deal
> >> with hotplug.  I think there is a strong case for this model, which
> >> would mean moving the microcode into /boot anyway.
> > 
> > /me like it, sounds very nifty. So how do we want to do that, we add
> > a field to the real-mode kernel header that tells us where to find
> > the microcode image and we take it and apply the ucode somewhere in
> > do_boot_cpu() path?
> > 
> 
> We already have a mechanism for passing arbitrary blobs -- the linked
> list -- so we don't have to add a new field at all.

So, after staring at grub legacy sources a bit, we could load the
microcode image using the grub module's mechanism:

kernel /...
module /boot/microcode.gz type=SETUP_MICROCODE # this is looked at by parse_setup_data()

and let grub write the pointer into setup_data passed through the kernel
header.

This would mean that we need to add support to a bunch of boot loaders
used currently, no? Or is there an even better way?

-- 
Regards/Gruss,
Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

On 02/03/2011 10:25 AM, Borislav Petkov wrote:
>>
>> We already have a mechanism for passing arbitrary blobs -- the linked
>> list -- so we don't have to add a new field at all.
> 
> So, after staring at grub legacy sources a bit, we could load the
> microcode image using the grub module's mechanism:
> 
> kernel /...
> module /boot/microcode.gz type=SETUP_MICROCODE # this is looked at by parse_setup_data()
> 
> and let grub write the pointer into setup_data passed through the kernel
> header.
> 
> This would mean that we need to add support to a bunch of boot loaders
> used currently, no? Or is there an even better way?
> 

We would need to add boot loader support, yes.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[H. Peter Anvin]

On 02/03/2011 10:25 AM, Borislav Petkov wrote:
>>
>> We already have a mechanism for passing arbitrary blobs -- the linked
>> list -- so we don't have to add a new field at all.
> 
> So, after staring at grub legacy sources a bit, we could load the
> microcode image using the grub module's mechanism:
> 
> kernel /...
> module /boot/microcode.gz type=SETUP_MICROCODE # this is looked at by parse_setup_data()
> 
> and let grub write the pointer into setup_data passed through the kernel
> header.
> 
> This would mean that we need to add support to a bunch of boot loaders
> used currently, no? Or is there an even better way?
> 

We would need to add boot loader support, yes.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

On Mon, Jan 31, 2011 at 04:15:21PM -0800, Jeremy Fitzhardinge wrote:
> My understanding of what you're proposing is:
> 
>    1. the normal CPU-specific microcode driver starts up
>    2. an if (xen) test overrides the apply_microcode to call a
>       Xen-specific function
>    3. the driver requests the firmware, loads and verifies it as normal
>    4. the microcode is applied via the Xen apply_microcode function
> 
> With (2) needing a Xen-specific change to both the Intel and AMD drivers.
> 
> If not, what are you proposing in detail?
> 
> Are you instead thinking:
> 
>     * Xen-specific microcode driver starts up
>     * It calls either the intel or amd request_microcode_fw as needed
>     * apply the the loaded data via hypercall
> 
> ?

I am thinking something in the sense of the above. For example, in the
AMD case you take

static struct microcode_ops microcode_amd_ops = {
	.request_microcode_user           = request_microcode_user,
	.request_microcode_fw             = request_microcode_fw,
	.collect_cpu_info                 = collect_cpu_info_amd,
	.apply_microcode                  = apply_microcode_amd,
	.microcode_fini_cpu               = microcode_fini_cpu_amd,
};

and reuse the ->request_microcode_fw, ->collect_cpu_info and
->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
to the point where you need to apply the microcode. Then, you use
your supplied ->apply_microcode hypercall wrapper to call into the
hypervisor.

> But all this is flawed because the microcode_intel/amd.c drivers assume
> they can see all the CPUs present in the system, and load suitable
> microcode for each specific one.  But a kernel in a Xen domain only has
> virtual CPUs - not physical ones - and has no idea how to get
> appropriate microcode data for all the physical CPUs in the system.

Well, let me quote you:

On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
> Xen update mechanism is uniform independent of the CPU type, but the
> driver must know where to find the data file, which depends on the CPU
> type. And since the update hypercall updates all CPUs, we only need to
> execute it once on any CPU - but for simplicity it just runs it only
> on (V)CPU 0.

so you only do it once and exit early in the rest of the cases. I
wouldn't worry about performance since ucode is applied only once upon
boot.

[..]

> Right.  There's nothing really for the driver to do (kernel or Xen). 
> The Intel driver does a bunch of checksum checking, which is presumably
> useful for detecting a corrupted firmware update early, and splits out
> the chunks specific to the current cpus.  The AMD driver does no
> checking per-se, but also looks for processor-specific microcode update
> chunks.
> 
> In the Xen case, since a domain is not necessarily seeing all the
> details of the underlying physical cpus, it makes most sense for
> usermode to just pass in the whole microcode file and let Xen do all the
> checking/filtering based on complete knowledge of the system.

This is exactly what I'm talking about - why copy all that
checking/filtering code from baremetal to Xen instead of simply reusing
it? Especially if you'd need to update the copy from time to time when
baremetal changes.

> > All I'm saying is, you should try as best as possible to avoid code
> > duplication and the need for replicating functionality to Xen, thus
> > doubling - even multiplying - the effort for coding/testing baremetal
> > and then Xen. Microcode is a perfect example since the vendors do all
> > their testing/verification on baremetal anyway and the rest should
> > benefit from that work.
> 
> CPU vendors test Xen, and Intel is particularly interested in getting
> this microcode driver upstream.  The amount of duplicated code is
> trivial, and the basic structure of the microcode updates doesn't seem
> set to change.

Uuh, I wouldn't bet on that though :).

> Since Xen has to have all sorts of other CPU-specific code which at
> least somewhat overlaps with what's in the kernel a bit more doesn't
> matter.

Well, I'll let x86 people decide on that but last time I checked they
opposed "if (xen)" sprinkling all over the place.

Btw, hpa has a point, if you can load microcode using multiboot, all
that discussion will become moot since you'll be better at loading
microcode even than baremetal. We need a similar mechanism in x86 too
since the current one loads the microcode definitely too late.

The optimal case for baremetal would be to load it as early as possible
on each CPU's bootstrapping path and if you can do that in the
hypervisor, before even dom0 starts, you're very much fine.

> (It's interesting that nobody seems to have been interested enough in
> Via to have ever implemented a microcode driver for it - perhaps they
> only ever do it from BIOS.)

Yeah, that's one possibility. I bet they'll do it though, when BIOS
cannot be updated/is out of life on some of their platforms and they
want to get a ucode patch applied.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 02/01/2011 03:00 AM, Borislav Petkov wrote:
> I am thinking something in the sense of the above. For example, in the
> AMD case you take
>
> static struct microcode_ops microcode_amd_ops = {
> 	.request_microcode_user           = request_microcode_user,
> 	.request_microcode_fw             = request_microcode_fw,
> 	.collect_cpu_info                 = collect_cpu_info_amd,
> 	.apply_microcode                  = apply_microcode_amd,
> 	.microcode_fini_cpu               = microcode_fini_cpu_amd,
> };
>
> and reuse the ->request_microcode_fw, ->collect_cpu_info and
> ->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
> to the point where you need to apply the microcode. Then, you use
> your supplied ->apply_microcode hypercall wrapper to call into the
> hypervisor.

collect_cpu_info can't work, because the domain doesn't have access to
all the host's physical CPUs.

However, even aside from that, it means exporting a pile of internal
details from microcode_amd and reusing them within microcode_xen.  And
it requires that it be done again for each vendor.

But all that's really needed is a dead simple "request" that loads the
entire file (with a vendor-specific name) and shoves it into Xen.  
There's no need for any vendor-specific code beyond the filename.


>> But all this is flawed because the microcode_intel/amd.c drivers assume
>> they can see all the CPUs present in the system, and load suitable
>> microcode for each specific one.  But a kernel in a Xen domain only has
>> virtual CPUs - not physical ones - and has no idea how to get
>> appropriate microcode data for all the physical CPUs in the system.
> Well, let me quote you:
>
> On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
>> Xen update mechanism is uniform independent of the CPU type, but the
>> driver must know where to find the data file, which depends on the CPU
>> type. And since the update hypercall updates all CPUs, we only need to
>> execute it once on any CPU - but for simplicity it just runs it only
>> on (V)CPU 0.
> so you only do it once and exit early in the rest of the cases. I
> wouldn't worry about performance since ucode is applied only once upon
> boot.

Its not a performance question.  The Intel and AMD microcode drivers
parse the full blob loaded from userspace, and just extract the chunk
needed for each CPU.  It does this for each separate CPU, so in
principle you could have a mixture of models within one machine or
something (the driver certainly assumes that could happen; perhaps it
could on a larger multinode machine).

The point is that if it does this on (what the domain sees as ) "cpu 0",
then it may throw away microcode chunks needed for other CPUs.  That's
why we need to hand Xen the entire microcode file, and let the
hypervisor do the work of splitting it up and installing it on the CPUs.

> This is exactly what I'm talking about - why copy all that
> checking/filtering code from baremetal to Xen instead of simply reusing
> it? Especially if you'd need to update the copy from time to time when
> baremetal changes.

The code in the kernel is in the wrong place.  It has to be done in
Xen.  When Xen is present, the code in the kernel is redundant, not the
other way around.

>> CPU vendors test Xen, and Intel is particularly interested in getting
>> this microcode driver upstream.  The amount of duplicated code is
>> trivial, and the basic structure of the microcode updates doesn't seem
>> set to change.
> Uuh, I wouldn't bet on that though :).

Shrug.  AFAICT the mechanism hasn't changed since it was first
introduced.  If there's a change, then both Linux and Xen will have to
change, and most likely the same CPU vendor engineer will provide a
patch for both.  Xen has a good record for tracking new CPU features.

>> Since Xen has to have all sorts of other CPU-specific code which at
>> least somewhat overlaps with what's in the kernel a bit more doesn't
>> matter.
> Well, I'll let x86 people decide on that but last time I checked they
> opposed "if (xen)" sprinkling all over the place.

Eh?  I'm talking about code within Xen; it doesn't involve any if (xen)s
within the kernel.


> Btw, hpa has a point, if you can load microcode using multiboot, all
> that discussion will become moot since you'll be better at loading
> microcode even than baremetal. We need a similar mechanism in x86 too
> since the current one loads the microcode definitely too late.
>
> The optimal case for baremetal would be to load it as early as possible
> on each CPU's bootstrapping path and if you can do that in the
> hypervisor, before even dom0 starts, you're very much fine.

It is possible, but it requires that vendors install the microcode
updates in /boot and update the grub entries accordingly.  I'd prefer a
solution which works with current distros as-is.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 02/01/2011 03:00 AM, Borislav Petkov wrote:
> I am thinking something in the sense of the above. For example, in the
> AMD case you take
>
> static struct microcode_ops microcode_amd_ops = {
> 	.request_microcode_user           = request_microcode_user,
> 	.request_microcode_fw             = request_microcode_fw,
> 	.collect_cpu_info                 = collect_cpu_info_amd,
> 	.apply_microcode                  = apply_microcode_amd,
> 	.microcode_fini_cpu               = microcode_fini_cpu_amd,
> };
>
> and reuse the ->request_microcode_fw, ->collect_cpu_info and
> ->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
> to the point where you need to apply the microcode. Then, you use
> your supplied ->apply_microcode hypercall wrapper to call into the
> hypervisor.

collect_cpu_info can't work, because the domain doesn't have access to
all the host's physical CPUs.

However, even aside from that, it means exporting a pile of internal
details from microcode_amd and reusing them within microcode_xen.  And
it requires that it be done again for each vendor.

But all that's really needed is a dead simple "request" that loads the
entire file (with a vendor-specific name) and shoves it into Xen.  
There's no need for any vendor-specific code beyond the filename.


>> But all this is flawed because the microcode_intel/amd.c drivers assume
>> they can see all the CPUs present in the system, and load suitable
>> microcode for each specific one.  But a kernel in a Xen domain only has
>> virtual CPUs - not physical ones - and has no idea how to get
>> appropriate microcode data for all the physical CPUs in the system.
> Well, let me quote you:
>
> On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
>> Xen update mechanism is uniform independent of the CPU type, but the
>> driver must know where to find the data file, which depends on the CPU
>> type. And since the update hypercall updates all CPUs, we only need to
>> execute it once on any CPU - but for simplicity it just runs it only
>> on (V)CPU 0.
> so you only do it once and exit early in the rest of the cases. I
> wouldn't worry about performance since ucode is applied only once upon
> boot.

Its not a performance question.  The Intel and AMD microcode drivers
parse the full blob loaded from userspace, and just extract the chunk
needed for each CPU.  It does this for each separate CPU, so in
principle you could have a mixture of models within one machine or
something (the driver certainly assumes that could happen; perhaps it
could on a larger multinode machine).

The point is that if it does this on (what the domain sees as ) "cpu 0",
then it may throw away microcode chunks needed for other CPUs.  That's
why we need to hand Xen the entire microcode file, and let the
hypervisor do the work of splitting it up and installing it on the CPUs.

> This is exactly what I'm talking about - why copy all that
> checking/filtering code from baremetal to Xen instead of simply reusing
> it? Especially if you'd need to update the copy from time to time when
> baremetal changes.

The code in the kernel is in the wrong place.  It has to be done in
Xen.  When Xen is present, the code in the kernel is redundant, not the
other way around.

>> CPU vendors test Xen, and Intel is particularly interested in getting
>> this microcode driver upstream.  The amount of duplicated code is
>> trivial, and the basic structure of the microcode updates doesn't seem
>> set to change.
> Uuh, I wouldn't bet on that though :).

Shrug.  AFAICT the mechanism hasn't changed since it was first
introduced.  If there's a change, then both Linux and Xen will have to
change, and most likely the same CPU vendor engineer will provide a
patch for both.  Xen has a good record for tracking new CPU features.

>> Since Xen has to have all sorts of other CPU-specific code which at
>> least somewhat overlaps with what's in the kernel a bit more doesn't
>> matter.
> Well, I'll let x86 people decide on that but last time I checked they
> opposed "if (xen)" sprinkling all over the place.

Eh?  I'm talking about code within Xen; it doesn't involve any if (xen)s
within the kernel.


> Btw, hpa has a point, if you can load microcode using multiboot, all
> that discussion will become moot since you'll be better at loading
> microcode even than baremetal. We need a similar mechanism in x86 too
> since the current one loads the microcode definitely too late.
>
> The optimal case for baremetal would be to load it as early as possible
> on each CPU's bootstrapping path and if you can do that in the
> hypervisor, before even dom0 starts, you're very much fine.

It is possible, but it requires that vendors install the microcode
updates in /boot and update the grub entries accordingly.  I'd prefer a
solution which works with current distros as-is.

    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

Ok, I'm reading your answers but I keep getting the impression that this
discussion is not moving anywhere. You keep finding reasons why it can't
be done and trying to persuade me that your way is the only way. Why is
that?

And I'm telling you microcode_xen has nothing to do among
vendor-specific code. Since when is xen a hw vendor and deserves special
attention? And don't tell me because people use it. It is absolutely
inacceptable to add a bunch of code to arch/x86 just because you're
telling me it can't be done differently, not intermixed with hw vendor
code and just because a hypervisor vendor needs it. Does that mean that
if every other virtualization booth wants to add their special code to
arch/x86, we just go and slap it in without questioning its design?

I don't think so.

On Tue, Feb 01, 2011 at 03:12:08PM -0800, Jeremy Fitzhardinge wrote:
> On 02/01/2011 03:00 AM, Borislav Petkov wrote:
> > I am thinking something in the sense of the above. For example, in the
> > AMD case you take
> >
> > static struct microcode_ops microcode_amd_ops = {
> > 	.request_microcode_user           = request_microcode_user,
> > 	.request_microcode_fw             = request_microcode_fw,
> > 	.collect_cpu_info                 = collect_cpu_info_amd,
> > 	.apply_microcode                  = apply_microcode_amd,
> > 	.microcode_fini_cpu               = microcode_fini_cpu_amd,
> > };
> >
> > and reuse the ->request_microcode_fw, ->collect_cpu_info and
> > ->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
> > to the point where you need to apply the microcode. Then, you use
> > your supplied ->apply_microcode hypercall wrapper to call into the
> > hypervisor.
> 
> collect_cpu_info can't work, because the domain doesn't have access to
> all the host's physical CPUs.

Why would you need that? You can safely assume that the ucode patch
level on all cores across the system are identical - I've yet to see a
machine running with different patch levels for the same reason that
mixed silicon systems is a large pain in the ass and you're better off
buying yourself a completely new system.

> However, even aside from that, it means exporting a pile of internal
> details from microcode_amd and reusing them within microcode_xen.  And
> it requires that it be done again for each vendor.

Why can't you load the appropriate, unmodified microcode_<vendor> module
in dom0 and let it call the hypercall?

> But all that's really needed is a dead simple "request" that loads the
> entire file (with a vendor-specific name) and shoves it into Xen.  
> There's no need for any vendor-specific code beyond the filename.

But that adds this funny chunk

-       if (c->x86_vendor == X86_VENDOR_INTEL)
+       if (xen_pv_domain())
+               microcode_ops = init_xen_microcode();
+       else if (c->x86_vendor == X86_VENDOR_INTEL)
                microcode_ops = init_intel_microcode();
        else if (c->x86_vendor == X86_VENDOR_AMD)
                microcode_ops = init_amd_microcode();

which can clearly be avoided. Now imagine the if-else thing contained a
dozen or more virt solutions in there, bloat! Now imagine this not only
in the microcode driver but in a bunch of other places across arch/x86.

> >> But all this is flawed because the microcode_intel/amd.c drivers assume
> >> they can see all the CPUs present in the system, and load suitable
> >> microcode for each specific one.  But a kernel in a Xen domain only has
> >> virtual CPUs - not physical ones - and has no idea how to get
> >> appropriate microcode data for all the physical CPUs in the system.
> > Well, let me quote you:
> >
> > On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
> >> Xen update mechanism is uniform independent of the CPU type, but the
> >> driver must know where to find the data file, which depends on the CPU
> >> type. And since the update hypercall updates all CPUs, we only need to
> >> execute it once on any CPU - but for simplicity it just runs it only
> >> on (V)CPU 0.
> > so you only do it once and exit early in the rest of the cases. I
> > wouldn't worry about performance since ucode is applied only once upon
> > boot.
> 
> Its not a performance question.  The Intel and AMD microcode drivers
> parse the full blob loaded from userspace, and just extract the chunk
> needed for each CPU.  It does this for each separate CPU, so in
> principle you could have a mixture of models within one machine or
> something (the driver certainly assumes that could happen; perhaps it
> could on a larger multinode machine).
>
> The point is that if it does this on (what the domain sees as ) "cpu 0",
> then it may throw away microcode chunks needed for other CPUs.  That's
> why we need to hand Xen the entire microcode file, and let the
> hypervisor do the work of splitting it up and installing it on the CPUs.

see comment about mixed silicon above.

> > This is exactly what I'm talking about - why copy all that
> > checking/filtering code from baremetal to Xen instead of simply reusing
> > it? Especially if you'd need to update the copy from time to time when
> > baremetal changes.
> 
> The code in the kernel is in the wrong place.  It has to be done in
> Xen.  When Xen is present, the code in the kernel is redundant, not the
> other way around.

Ok, this says it all. Let's remove the arch code and replace it with
xen-friendly version - we won't need the baremetal one anyway. Jeez!

> >> CPU vendors test Xen, and Intel is particularly interested in getting
> >> this microcode driver upstream.  The amount of duplicated code is
> >> trivial, and the basic structure of the microcode updates doesn't seem
> >> set to change.
> > Uuh, I wouldn't bet on that though :).
> 
> Shrug.  AFAICT the mechanism hasn't changed since it was first
> introduced.  If there's a change, then both Linux and Xen will have to
> change, and most likely the same CPU vendor engineer will provide a
> patch for both.  Xen has a good record for tracking new CPU features.

I don't think that the same CPU vendor engineer will do that, believe me!

> >> Since Xen has to have all sorts of other CPU-specific code which at
> >> least somewhat overlaps with what's in the kernel a bit more doesn't
> >> matter.
> > Well, I'll let x86 people decide on that but last time I checked they
> > opposed "if (xen)" sprinkling all over the place.
> 
> Eh?  I'm talking about code within Xen; it doesn't involve any if (xen)s
> within the kernel.

Ok, I'm getting tired of this. I'll gladly read all your answers if you
have constructive suggestions and better proposals - if the only thing
you have to come back are reasons why xen's is the only way to do it,
then don't even bother to answer - at least I won't. If x86 people want
to take your code, they'll do it since it is their call anyway.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Borislav Petkov]

Ok, I'm reading your answers but I keep getting the impression that this
discussion is not moving anywhere. You keep finding reasons why it can't
be done and trying to persuade me that your way is the only way. Why is
that?

And I'm telling you microcode_xen has nothing to do among
vendor-specific code. Since when is xen a hw vendor and deserves special
attention? And don't tell me because people use it. It is absolutely
inacceptable to add a bunch of code to arch/x86 just because you're
telling me it can't be done differently, not intermixed with hw vendor
code and just because a hypervisor vendor needs it. Does that mean that
if every other virtualization booth wants to add their special code to
arch/x86, we just go and slap it in without questioning its design?

I don't think so.

On Tue, Feb 01, 2011 at 03:12:08PM -0800, Jeremy Fitzhardinge wrote:
> On 02/01/2011 03:00 AM, Borislav Petkov wrote:
> > I am thinking something in the sense of the above. For example, in the
> > AMD case you take
> >
> > static struct microcode_ops microcode_amd_ops = {
> > 	.request_microcode_user           = request_microcode_user,
> > 	.request_microcode_fw             = request_microcode_fw,
> > 	.collect_cpu_info                 = collect_cpu_info_amd,
> > 	.apply_microcode                  = apply_microcode_amd,
> > 	.microcode_fini_cpu               = microcode_fini_cpu_amd,
> > };
> >
> > and reuse the ->request_microcode_fw, ->collect_cpu_info and
> > ->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
> > to the point where you need to apply the microcode. Then, you use
> > your supplied ->apply_microcode hypercall wrapper to call into the
> > hypervisor.
> 
> collect_cpu_info can't work, because the domain doesn't have access to
> all the host's physical CPUs.

Why would you need that? You can safely assume that the ucode patch
level on all cores across the system are identical - I've yet to see a
machine running with different patch levels for the same reason that
mixed silicon systems is a large pain in the ass and you're better off
buying yourself a completely new system.

> However, even aside from that, it means exporting a pile of internal
> details from microcode_amd and reusing them within microcode_xen.  And
> it requires that it be done again for each vendor.

Why can't you load the appropriate, unmodified microcode_<vendor> module
in dom0 and let it call the hypercall?

> But all that's really needed is a dead simple "request" that loads the
> entire file (with a vendor-specific name) and shoves it into Xen.  
> There's no need for any vendor-specific code beyond the filename.

But that adds this funny chunk

-       if (c->x86_vendor == X86_VENDOR_INTEL)
+       if (xen_pv_domain())
+               microcode_ops = init_xen_microcode();
+       else if (c->x86_vendor == X86_VENDOR_INTEL)
                microcode_ops = init_intel_microcode();
        else if (c->x86_vendor == X86_VENDOR_AMD)
                microcode_ops = init_amd_microcode();

which can clearly be avoided. Now imagine the if-else thing contained a
dozen or more virt solutions in there, bloat! Now imagine this not only
in the microcode driver but in a bunch of other places across arch/x86.

> >> But all this is flawed because the microcode_intel/amd.c drivers assume
> >> they can see all the CPUs present in the system, and load suitable
> >> microcode for each specific one.  But a kernel in a Xen domain only has
> >> virtual CPUs - not physical ones - and has no idea how to get
> >> appropriate microcode data for all the physical CPUs in the system.
> > Well, let me quote you:
> >
> > On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
> >> Xen update mechanism is uniform independent of the CPU type, but the
> >> driver must know where to find the data file, which depends on the CPU
> >> type. And since the update hypercall updates all CPUs, we only need to
> >> execute it once on any CPU - but for simplicity it just runs it only
> >> on (V)CPU 0.
> > so you only do it once and exit early in the rest of the cases. I
> > wouldn't worry about performance since ucode is applied only once upon
> > boot.
> 
> Its not a performance question.  The Intel and AMD microcode drivers
> parse the full blob loaded from userspace, and just extract the chunk
> needed for each CPU.  It does this for each separate CPU, so in
> principle you could have a mixture of models within one machine or
> something (the driver certainly assumes that could happen; perhaps it
> could on a larger multinode machine).
>
> The point is that if it does this on (what the domain sees as ) "cpu 0",
> then it may throw away microcode chunks needed for other CPUs.  That's
> why we need to hand Xen the entire microcode file, and let the
> hypervisor do the work of splitting it up and installing it on the CPUs.

see comment about mixed silicon above.

> > This is exactly what I'm talking about - why copy all that
> > checking/filtering code from baremetal to Xen instead of simply reusing
> > it? Especially if you'd need to update the copy from time to time when
> > baremetal changes.
> 
> The code in the kernel is in the wrong place.  It has to be done in
> Xen.  When Xen is present, the code in the kernel is redundant, not the
> other way around.

Ok, this says it all. Let's remove the arch code and replace it with
xen-friendly version - we won't need the baremetal one anyway. Jeez!

> >> CPU vendors test Xen, and Intel is particularly interested in getting
> >> this microcode driver upstream.  The amount of duplicated code is
> >> trivial, and the basic structure of the microcode updates doesn't seem
> >> set to change.
> > Uuh, I wouldn't bet on that though :).
> 
> Shrug.  AFAICT the mechanism hasn't changed since it was first
> introduced.  If there's a change, then both Linux and Xen will have to
> change, and most likely the same CPU vendor engineer will provide a
> patch for both.  Xen has a good record for tracking new CPU features.

I don't think that the same CPU vendor engineer will do that, believe me!

> >> Since Xen has to have all sorts of other CPU-specific code which at
> >> least somewhat overlaps with what's in the kernel a bit more doesn't
> >> matter.
> > Well, I'll let x86 people decide on that but last time I checked they
> > opposed "if (xen)" sprinkling all over the place.
> 
> Eh?  I'm talking about code within Xen; it doesn't involve any if (xen)s
> within the kernel.

Ok, I'm getting tired of this. I'll gladly read all your answers if you
have constructive suggestions and better proposals - if the only thing
you have to come back are reasons why xen's is the only way to do it,
then don't even bother to answer - at least I won't. If x86 people want
to take your code, they'll do it since it is their call anyway.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Henrique de Moraes Holschuh]

On Wed, 02 Feb 2011, Borislav Petkov wrote:
> Why would you need that? You can safely assume that the ucode patch
> level on all cores across the system are identical - I've yet to see a

No.  You can safely assume that the ucode patch level on all cores on a
package are identical, and that's it.  Mixing processors with different
processor flags (and thus potentially different microcode) is not uncommon.
If it boots, it *will* be done, especially later in the product cycle, when
specific spare parts are harder to come by.  Sometimes you can even mix
processors of different steppings.

> mixed silicon systems is a large pain in the ass and you're better off
> buying yourself a completely new system.

Well, I have some of them at work.

Mixed CPU steppings or processor flags happen when you expand the system
later to fill in processor packages (reasonably common), or when you replace
a CPU that is flaky (rare).  This sort of mixing of different processors is
really common on older Xeon systems, and most of them need OS-assisted
microcode updates to get the latest microcode available.

I am not arguing anything about the way Xen decided to go about implementing
the feature, but they got the requirement "must pass through all the
microcodes without removing any" right.  It exists.

> > However, even aside from that, it means exporting a pile of internal
> > details from microcode_amd and reusing them within microcode_xen.  And
> > it requires that it be done again for each vendor.
> 
> Why can't you load the appropriate, unmodified microcode_<vendor> module
> in dom0 and let it call the hypercall?

Or add a microcode_virtual passthrough device, for that matter.  Wouldn't
that be much cleaner and more palatable?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Henrique de Moraes Holschuh]

On Wed, 02 Feb 2011, Borislav Petkov wrote:
> Why would you need that? You can safely assume that the ucode patch
> level on all cores across the system are identical - I've yet to see a

No.  You can safely assume that the ucode patch level on all cores on a
package are identical, and that's it.  Mixing processors with different
processor flags (and thus potentially different microcode) is not uncommon.
If it boots, it *will* be done, especially later in the product cycle, when
specific spare parts are harder to come by.  Sometimes you can even mix
processors of different steppings.

> mixed silicon systems is a large pain in the ass and you're better off
> buying yourself a completely new system.

Well, I have some of them at work.

Mixed CPU steppings or processor flags happen when you expand the system
later to fill in processor packages (reasonably common), or when you replace
a CPU that is flaky (rare).  This sort of mixing of different processors is
really common on older Xeon systems, and most of them need OS-assisted
microcode updates to get the latest microcode available.

I am not arguing anything about the way Xen decided to go about implementing
the feature, but they got the requirement "must pass through all the
microcodes without removing any" right.  It exists.

> > However, even aside from that, it means exporting a pile of internal
> > details from microcode_amd and reusing them within microcode_xen.  And
> > it requires that it be done again for each vendor.
> 
> Why can't you load the appropriate, unmodified microcode_<vendor> module
> in dom0 and let it call the hypercall?

Or add a microcode_virtual passthrough device, for that matter.  Wouldn't
that be much cleaner and more palatable?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 02/02/2011 01:54 AM, Borislav Petkov wrote:
> Ok, I'm reading your answers but I keep getting the impression that this
> discussion is not moving anywhere. You keep finding reasons why it can't
> be done and trying to persuade me that your way is the only way. Why is
> that?

I agree that this conversation has got bogged down.  I'm getting the
impression that you're not really understanding my answers within the
context of how Xen works, and so things are going in circles.  I'm happy
to go into more detail if you're interested.

I'm not trying to be obstructionist here.  We've often changed the way
things work on the Xen side to smooth the path into the kernel, and I'm
perfectly willing to do it again for the microcode driver if it makes
sense to do so.

> And I'm telling you microcode_xen has nothing to do among
> vendor-specific code. Since when is xen a hw vendor and deserves special
> attention? And don't tell me because people use it.

Who's asking for special attention?  I'm just trying to use the existing
microcode infrastructure for handling different methods of microcode
update to add one more.  Why is "because people use it" not a useful
thing to say?  If I said "but nobody uses it", then that would be a
strong argument for not including it.

>  It is absolutely
> inacceptable to add a bunch of code to arch/x86 just because you're
> telling me it can't be done differently, not intermixed with hw vendor
> code and just because a hypervisor vendor needs it.
You seem to have staked out a very... specific... position here, but I
don't agree with your premise that microcode_foo is specifically
microcode_<cpu-vendor>.  If you view it as microcode_<method of loading
microcode> then adding microcode_xen makes perfect sense.  Since it is a
small, self-contained piece of code that doesn't have any effects on any
other code, nor any dependencies aside from the microcode driver's
internal interface, I think it is a clean way to approach the problem.

Or to turn it around, what specific problems do you see arising from
implementing the Xen microcode loader in this way?  Why is adding a
third microcode_foo.c a problem?

>  Does that mean that
> if every other virtualization booth wants to add their special code to
> arch/x86, we just go and slap it in without questioning its design?

Of course not; the whole point of posting code for review is to get
feedback on both general and specific issues, and I appreciate the time
you've spent on this.  But I have to say I don't really understand what
your objections are.  Are you objecting to the very principle of adding
a new microcode driver, or is there something specific about the code I
posted that you have issues with?

Thanks,
    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 02/02/2011 01:54 AM, Borislav Petkov wrote:
> Ok, I'm reading your answers but I keep getting the impression that this
> discussion is not moving anywhere. You keep finding reasons why it can't
> be done and trying to persuade me that your way is the only way. Why is
> that?

I agree that this conversation has got bogged down.  I'm getting the
impression that you're not really understanding my answers within the
context of how Xen works, and so things are going in circles.  I'm happy
to go into more detail if you're interested.

I'm not trying to be obstructionist here.  We've often changed the way
things work on the Xen side to smooth the path into the kernel, and I'm
perfectly willing to do it again for the microcode driver if it makes
sense to do so.

> And I'm telling you microcode_xen has nothing to do among
> vendor-specific code. Since when is xen a hw vendor and deserves special
> attention? And don't tell me because people use it.

Who's asking for special attention?  I'm just trying to use the existing
microcode infrastructure for handling different methods of microcode
update to add one more.  Why is "because people use it" not a useful
thing to say?  If I said "but nobody uses it", then that would be a
strong argument for not including it.

>  It is absolutely
> inacceptable to add a bunch of code to arch/x86 just because you're
> telling me it can't be done differently, not intermixed with hw vendor
> code and just because a hypervisor vendor needs it.
You seem to have staked out a very... specific... position here, but I
don't agree with your premise that microcode_foo is specifically
microcode_<cpu-vendor>.  If you view it as microcode_<method of loading
microcode> then adding microcode_xen makes perfect sense.  Since it is a
small, self-contained piece of code that doesn't have any effects on any
other code, nor any dependencies aside from the microcode driver's
internal interface, I think it is a clean way to approach the problem.

Or to turn it around, what specific problems do you see arising from
implementing the Xen microcode loader in this way?  Why is adding a
third microcode_foo.c a problem?

>  Does that mean that
> if every other virtualization booth wants to add their special code to
> arch/x86, we just go and slap it in without questioning its design?

Of course not; the whole point of posting code for review is to get
feedback on both general and specific issues, and I appreciate the time
you've spent on this.  But I have to say I don't really understand what
your objections are.  Are you objecting to the very principle of adding
a new microcode driver, or is there something specific about the code I
posted that you have issues with?

Thanks,
    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

Sorry, missed your specific comments below.

On 02/02/2011 01:54 AM, Borislav Petkov wrote:
>> collect_cpu_info can't work, because the domain doesn't have access to
>> all the host's physical CPUs.
> Why would you need that? You can safely assume that the ucode patch
> level on all cores across the system are identical - I've yet to see a
> machine running with different patch levels for the same reason that
> mixed silicon systems is a large pain in the ass and you're better off
> buying yourself a completely new system.

I was thinking specifically about a large multi-chassis system with lots
of nodes.  In that case you could well imagine different nodes/chassis
having variations of CPU steppings.

But even on a single board, I don't know what limitations there are on
mixing steppings between sockets, but I suspect it is possible. 
Obviously I wouldn't expect there to be large variations between CPUs
(different models or features) - that would be a problem.

>> However, even aside from that, it means exporting a pile of internal
>> details from microcode_amd and reusing them within microcode_xen.  And
>> it requires that it be done again for each vendor.
> Why can't you load the appropriate, unmodified microcode_<vendor> module
> in dom0 and let it call the hypercall?

Well, if it can call hypercalls, then to some extent it is already
"modified".  Either I change the source to change the wrmsrs into
hypercalls, or overwrite its operations structure to change the
apply_microcode function pointer (which would require - at the very
least - making it non-static) to a hyper-calling function.

Either way it implies that we're delving into the Intel/AMD drivers in
order to implement Xen-specific functionality, which is clearly (to me,
at least) much worse than just making the Xen-specific code completely
self-contained.

>> But all that's really needed is a dead simple "request" that loads the
>> entire file (with a vendor-specific name) and shoves it into Xen.  
>> There's no need for any vendor-specific code beyond the filename.
> But that adds this funny chunk
>
> -       if (c->x86_vendor == X86_VENDOR_INTEL)
> +       if (xen_pv_domain())
> +               microcode_ops = init_xen_microcode();
> +       else if (c->x86_vendor == X86_VENDOR_INTEL)
>                 microcode_ops = init_intel_microcode();
>         else if (c->x86_vendor == X86_VENDOR_AMD)
>                 microcode_ops = init_amd_microcode();
>
> which can clearly be avoided. Now imagine the if-else thing contained a
> dozen or more virt solutions in there, bloat!

This is just the way microcode_core.c is structured.  It doesn't have a
dozen or more virt solutions, but if it did, we could change the way
that microcode loaders are probed for to make it cleaner.  Exactly the
same issue arises if you say "there might be a dozen or more cpu vendors
- bloat!".  True, there might be, and we should fix it in that case.  Or
conversely, there might be a new CPU vendor who decides to implement the
Intel microcode loading interface, in which case the tests on a specific
vendor become wrong.

>  Now imagine this not only
> in the microcode driver but in a bunch of other places across arch/x86.

Why imagine that?  We're not talking about the rest of arch/x86.

>>> This is exactly what I'm talking about - why copy all that
>>> checking/filtering code from baremetal to Xen instead of simply reusing
>>> it? Especially if you'd need to update the copy from time to time when
>>> baremetal changes.
>> The code in the kernel is in the wrong place.  It has to be done in
>> Xen.  When Xen is present, the code in the kernel is redundant, not the
>> other way around.
> Ok, this says it all. Let's remove the arch code and replace it with
> xen-friendly version - we won't need the baremetal one anyway. Jeez!

I didn't say anything about removing the code from Linux.  But the
simple fact is that a kernel running under a hypervisor isn't the
ultimate owner of the machine's hardware - the hypervisor is, and it
controls all access to the hardware.  That's deeply fundamental to the
way any hypervisor works, and Xen is just one example.  And the
consequence of that is that Linux has to use hypervisor facilities to
perform certain operations rather than using its own code.

>> Shrug.  AFAICT the mechanism hasn't changed since it was first
>> introduced.  If there's a change, then both Linux and Xen will have to
>> change, and most likely the same CPU vendor engineer will provide a
>> patch for both.  Xen has a good record for tracking new CPU features.
> I don't think that the same CPU vendor engineer will do that, believe me!

Many individual Intel and AMD engineers contribute code to both Xen and
Linux.  It's not a question of belief.

Thanks,
    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

Sorry, missed your specific comments below.

On 02/02/2011 01:54 AM, Borislav Petkov wrote:
>> collect_cpu_info can't work, because the domain doesn't have access to
>> all the host's physical CPUs.
> Why would you need that? You can safely assume that the ucode patch
> level on all cores across the system are identical - I've yet to see a
> machine running with different patch levels for the same reason that
> mixed silicon systems is a large pain in the ass and you're better off
> buying yourself a completely new system.

I was thinking specifically about a large multi-chassis system with lots
of nodes.  In that case you could well imagine different nodes/chassis
having variations of CPU steppings.

But even on a single board, I don't know what limitations there are on
mixing steppings between sockets, but I suspect it is possible. 
Obviously I wouldn't expect there to be large variations between CPUs
(different models or features) - that would be a problem.

>> However, even aside from that, it means exporting a pile of internal
>> details from microcode_amd and reusing them within microcode_xen.  And
>> it requires that it be done again for each vendor.
> Why can't you load the appropriate, unmodified microcode_<vendor> module
> in dom0 and let it call the hypercall?

Well, if it can call hypercalls, then to some extent it is already
"modified".  Either I change the source to change the wrmsrs into
hypercalls, or overwrite its operations structure to change the
apply_microcode function pointer (which would require - at the very
least - making it non-static) to a hyper-calling function.

Either way it implies that we're delving into the Intel/AMD drivers in
order to implement Xen-specific functionality, which is clearly (to me,
at least) much worse than just making the Xen-specific code completely
self-contained.

>> But all that's really needed is a dead simple "request" that loads the
>> entire file (with a vendor-specific name) and shoves it into Xen.  
>> There's no need for any vendor-specific code beyond the filename.
> But that adds this funny chunk
>
> -       if (c->x86_vendor == X86_VENDOR_INTEL)
> +       if (xen_pv_domain())
> +               microcode_ops = init_xen_microcode();
> +       else if (c->x86_vendor == X86_VENDOR_INTEL)
>                 microcode_ops = init_intel_microcode();
>         else if (c->x86_vendor == X86_VENDOR_AMD)
>                 microcode_ops = init_amd_microcode();
>
> which can clearly be avoided. Now imagine the if-else thing contained a
> dozen or more virt solutions in there, bloat!

This is just the way microcode_core.c is structured.  It doesn't have a
dozen or more virt solutions, but if it did, we could change the way
that microcode loaders are probed for to make it cleaner.  Exactly the
same issue arises if you say "there might be a dozen or more cpu vendors
- bloat!".  True, there might be, and we should fix it in that case.  Or
conversely, there might be a new CPU vendor who decides to implement the
Intel microcode loading interface, in which case the tests on a specific
vendor become wrong.

>  Now imagine this not only
> in the microcode driver but in a bunch of other places across arch/x86.

Why imagine that?  We're not talking about the rest of arch/x86.

>>> This is exactly what I'm talking about - why copy all that
>>> checking/filtering code from baremetal to Xen instead of simply reusing
>>> it? Especially if you'd need to update the copy from time to time when
>>> baremetal changes.
>> The code in the kernel is in the wrong place.  It has to be done in
>> Xen.  When Xen is present, the code in the kernel is redundant, not the
>> other way around.
> Ok, this says it all. Let's remove the arch code and replace it with
> xen-friendly version - we won't need the baremetal one anyway. Jeez!

I didn't say anything about removing the code from Linux.  But the
simple fact is that a kernel running under a hypervisor isn't the
ultimate owner of the machine's hardware - the hypervisor is, and it
controls all access to the hardware.  That's deeply fundamental to the
way any hypervisor works, and Xen is just one example.  And the
consequence of that is that Linux has to use hypervisor facilities to
perform certain operations rather than using its own code.

>> Shrug.  AFAICT the mechanism hasn't changed since it was first
>> introduced.  If there's a change, then both Linux and Xen will have to
>> change, and most likely the same CPU vendor engineer will provide a
>> patch for both.  Xen has a good record for tracking new CPU features.
> I don't think that the same CPU vendor engineer will do that, believe me!

Many individual Intel and AMD engineers contribute code to both Xen and
Linux.  It's not a question of belief.

Thanks,
    J

Re: [PATCH 0/2] x86/microcode: support for microcode update in Xen dom0

[Jeremy Fitzhardinge]

On 01/30/2011 03:33 AM, Borislav Petkov wrote:
> On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
>> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> Hi all,
>>
>> I'm proposing this for 2.6.39.
>>
>> This series adds a new "Xen" microcode update type, in addition to
>> Intel and AMD.
>>
>> The Xen hypervisor is responsible for performing the actual microcode
>> update (since only it knows what physical CPUs are in the system and
>> has sufficient privilege to access them), but it requires the dom0
>> kernel to provide the actual microcode update data.
>>
>> Xen update mechanism is uniform independent of the CPU type, but the
>> driver must know where to find the data file, which depends on the CPU
>> type.  And since the update hypercall updates all CPUs, we only need
>> to execute it once on any CPU - but for simplicity it just runs it only
>> on (V)CPU 0.
> I don't like this for several reasons:
>
> - ucode should be loaded as early as possible and the perfect place
> for that should be the hypervisor and not the dom0 kernel. But I'm not
> that familiar with xen design and I guess it would be pretty hard to
> do. (Btw, x86 bare metal could also try to improve the situation there
> but that's also hard due to the design of the firmware loader (needs
> userspace)).

Yes, its the same issue with Xen or without.  The Xen hypervisor has no
devices, storage or anything else with which it can get microcode data. 
It depends on the dom0 kernel getting it from somewhere and supplying it
to the hypervisor.  In practice this is no different from relying on
usermode to get the firmware update.

In general firmware updates are not critical and not required very
early, so "as soon as reasonably possible" is OK.  (If the firmware
update is critical, it should be supplied as a BIOS update.)

So I think this is moot with respect to this particular patch.

> - you're adding a microcode_xen.c as if this is another hw vendor and
> you're figuring out which is the proper firmware image based on the
> vendor, then you load it and then do the hypercall. This is duplicating
> code and inviting future bugs. Everytime the hw vendors decide to change
> something to their fw loading mechanism, xen needs to be updated. Also,
> you don't do any sanity checks to the ucode image as the vendor code
> does which is inacceptable.

The code within the hypervisor is more or less the same as the kernel's:
it does all the required vendor-specific checks on the firmware and
loads it on all the CPUs with the appropriate mechanism.  The hypercall
ABI is vendor-agnostic, but it assumes that dom0 will supply suitable
microcode for the current CPU vendor (though if it doesn't, the image
will presumably be rejected).

So from that perspective, it is a distinct microcode loading mechanism,
akin to a vendor-specific one.  The awkward part is getting the filename
to actually request from usermode, which is Intel/AMD/whatever specific,
which results in duplicated code to generate that pathname.

> What it should do instead, is call into the hw vendor-specific ucode
> loading mechanism and do all the image loading/verification there. The
> only thing you'd need to supply is a xen-specific ->apply_microcode()
> doing the hypercall _after_ the ucode image has been verified and is
> good to go. I'm guessing the XENPF_microcode_update does call into the
> hypervisor and calls a xen-specific ucode update mechanism based on the
> vendor instead of using the vendor-supplied code...

Well, I was trying to avoid putting Xen-specific code into the existing
Intel/AMD loaders.  That doesn't seem any cleaner.

I could export "my firmware pathname" functions from them and have the
Xen driver call those, rather than duplicating the pathname construction
code.  Would that help address your concerns?

    J