[PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Luis R. Rodriguez]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

There are several ways that a Xen system can update the
CPU microcode on a pvops kernel [0] now, the preferred way
is through the early microcode update mechanism. At run
time folks should use this new xenmicrocode tool and use
the same CPU microcode file as present on /lib/firmware.

Some distributions may use the historic sysfs rescan interface,
users of that mechanism should be aware that the interface will
not be available when using Xen and as such should first check
the presence of the interface before usage, as an alternative
this xenmicrocode tool can be used on priviledged domains.

Folks wishing to update CPU microcode at run time should be
aware that not all CPU microcode can be updated on a system
and should take care to ensure that only known-to-work and
supported CPU microcode updates are used [0].

[0] http://wiki.xenproject.org/wiki/XenParavirtOps/microcode_update

Cc: Borislav Petkov <bp@suse.de>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Olaf Hering <ohering@suse.de>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Jason Douglas <jdouglas@suse.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Henrique de Moraes Holschuh <hmh@debian.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 tools/libxc/xc_misc.c     | 19 +++++++++++
 tools/libxc/xenctrl.h     |  2 ++
 tools/misc/Makefile       |  7 ++--
 tools/misc/xenmicrocode.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 2 deletions(-)
 create mode 100644 tools/misc/xenmicrocode.c

diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index e253a58..3ef2664 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -250,6 +250,25 @@ int xc_mca_op(xc_interface *xch, struct xen_mc *mc)
     xc_hypercall_bounce_post(xch, mc);
     return ret;
 }
+int xc_platform_op(xc_interface *xch, struct xen_platform_op *op)
+{
+    int ret = 0;
+    DECLARE_HYPERCALL;
+    DECLARE_HYPERCALL_BOUNCE(op, sizeof(*op), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+    if ( xc_hypercall_bounce_pre(xch, op) )
+    {
+        PERROR("Could not bounce xen_platform_op memory buffer");
+        return -1;
+    }
+    op->interface_version = XENPF_INTERFACE_VERSION;
+
+    hypercall.op = __HYPERVISOR_platform_op;
+    hypercall.arg[0] = HYPERCALL_BUFFER_AS_ARG(op);
+    ret = do_xen_hypercall(xch, &hypercall);
+    xc_hypercall_bounce_post(xch, op);
+    return ret;
+}
 #endif
 
 int xc_perfc_reset(xc_interface *xch)
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 514b241..5085e50 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -54,6 +54,7 @@
 #include <xen/foreign/x86_32.h>
 #include <xen/foreign/x86_64.h>
 #include <xen/arch-x86/xen-mca.h>
+#include <xen/platform.h>
 #endif
 
 #define XC_PAGE_SHIFT           12
@@ -2131,6 +2132,7 @@ int xc_cpuid_apply_policy(xc_interface *xch,
 void xc_cpuid_to_str(const unsigned int *regs,
                      char **strs); /* some strs[] may be NULL if ENOMEM */
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
+int xc_platform_op(xc_interface *xch, struct xen_platform_op *platform_op);
 #endif
 
 struct xc_px_val {
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 69b1817..bb838d0 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -10,7 +10,7 @@ CFLAGS += $(CFLAGS_libxenstore)
 HDRS     = $(wildcard *.h)
 
 TARGETS-y := xenperf xenpm xen-tmem-list-parse gtraceview gtracestat xenlockprof xenwatchdogd xencov
-TARGETS-$(CONFIG_X86) += xen-detect xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump
+TARGETS-$(CONFIG_X86) += xen-detect xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump xenmicrocode
 TARGETS-$(CONFIG_MIGRATE) += xen-hptool
 TARGETS := $(TARGETS-y)
 
@@ -22,7 +22,7 @@ INSTALL_BIN := $(INSTALL_BIN-y)
 
 INSTALL_SBIN-y := xen-bugtool xen-python-path xenperf xenpm xen-tmem-list-parse gtraceview \
 	gtracestat xenlockprof xenwatchdogd xen-ringwatch xencov
-INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump
+INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump xenmicrocode
 INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
 INSTALL_SBIN := $(INSTALL_SBIN-y)
 
@@ -66,6 +66,9 @@ xenperf: xenperf.o
 xenpm: xenpm.o
 	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
+xenmicrocode: xenmicrocode.o
+	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
 gtracestat: gtracestat.o
 	$(CC) $(LDFLAGS) -o $@ $< $(APPEND_LDFLAGS)
 
diff --git a/tools/misc/xenmicrocode.c b/tools/misc/xenmicrocode.c
new file mode 100644
index 0000000..5c58a1b
--- /dev/null
+++ b/tools/misc/xenmicrocode.c
@@ -0,0 +1,81 @@
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <errno.h>
+#include <string.h>
+#include <inttypes.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <xenctrl.h>
+
+int main(int argc, char *argv[])
+{
+    int fd = 0;
+    unsigned char *fbuf;
+    int len;
+    xc_interface *xc_handle;
+    char *filename;
+    struct stat buf;
+    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
+    struct xen_platform_op op;
+
+    filename = argv[1];
+    fd = open(filename, O_RDONLY);
+
+    if (fd <= 0)
+    {
+        printf("Could not open; err: %d(%s)\n", errno, strerror(errno));
+        return errno;
+    }
+
+    if (stat(filename, &buf) != 0)
+    {
+        printf("Could not open; err: %d(%s)\n", errno, strerror(errno));
+        return errno;
+    }
+
+    printf("%s: %ld\n", filename, buf.st_size);
+
+    len = buf.st_size;
+    fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
+
+    if ((xc_handle = xc_interface_open(0,0,0)) == 0)
+    {
+        fprintf(stderr, "Error opening xc interface: %d (%s)\n",
+                errno, strerror(errno));
+        return 1;
+    }
+
+    if (fbuf == MAP_FAILED)
+    {
+        printf("Could not map: error: %d(%s)\n", errno,
+               strerror(errno));
+        return errno;
+    }
+
+    uc = xc_hypercall_buffer_alloc(xc_handle, uc, len);
+    memcpy(uc, fbuf, len);
+
+    set_xen_guest_handle(op.u.microcode.data, uc);
+    op.cmd = XENPF_microcode_update;
+    op.interface_version = XENPF_INTERFACE_VERSION;
+    op.u.microcode.length = len;
+    xc_platform_op(xc_handle, &op);
+
+    xc_hypercall_buffer_free(xc_handle, uc);
+    xc_interface_close(xc_handle);
+
+    if (munmap(fbuf, len))
+    {
+        printf("Could not unmap: %d(%s)\n", errno, strerror(errno));
+        return errno;
+    }
+
+    close(fd);
+
+    return 0;
+}
-- 
2.1.1

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Boris Ostrovsky]

On 01/27/2015 03:11 PM, Luis R. Rodriguez wrote:
> +    fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
> +
> +    if ((xc_handle = xc_interface_open(0,0,0)) == 0)
> +    {
> +        fprintf(stderr, "Error opening xc interface: %d (%s)\n",
> +                errno, strerror(errno));
> +        return 1;
> +    }
> +
> +    if (fbuf == MAP_FAILED)
> +    {
> +        printf("Could not map: error: %d(%s)\n", errno,
> +               strerror(errno));
> +        return errno;
> +    }

Shouldn't this 'if' block directly follow the mmap()?

> +
> +    uc = xc_hypercall_buffer_alloc(xc_handle, uc, len);
> +    memcpy(uc, fbuf, len);
> +
> +    set_xen_guest_handle(op.u.microcode.data, uc);
> +    op.cmd = XENPF_microcode_update;
> +    op.interface_version = XENPF_INTERFACE_VERSION;
> +    op.u.microcode.length = len;
> +    xc_platform_op(xc_handle, &op);
> +
> +    xc_hypercall_buffer_free(xc_handle, uc);
> +    xc_interface_close(xc_handle);
> +
> +    if (munmap(fbuf, len))
> +    {
> +        printf("Could not unmap: %d(%s)\n", errno, strerror(errno));
> +        return errno;
> +    }
> +
> +    close(fd);

Given that you never close the file on errors this is not really 
necessary. Or you should close it on errors for consistency.


-boris

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Luis R. Rodriguez]

On Tue, Jan 27, 2015 at 04:25:09PM -0500, Boris Ostrovsky wrote:
> On 01/27/2015 03:11 PM, Luis R. Rodriguez wrote:
>> +    fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
>> +
>> +    if ((xc_handle = xc_interface_open(0,0,0)) == 0)
>> +    {
>> +        fprintf(stderr, "Error opening xc interface: %d (%s)\n",
>> +                errno, strerror(errno));
>> +        return 1;
>> +    }
>> +
>> +    if (fbuf == MAP_FAILED)
>> +    {
>> +        printf("Could not map: error: %d(%s)\n", errno,
>> +               strerror(errno));
>> +        return errno;
>> +    }
>
> Shouldn't this 'if' block directly follow the mmap()?

Sure.

>> +
>> +    uc = xc_hypercall_buffer_alloc(xc_handle, uc, len);
>> +    memcpy(uc, fbuf, len);
>> +
>> +    set_xen_guest_handle(op.u.microcode.data, uc);
>> +    op.cmd = XENPF_microcode_update;
>> +    op.interface_version = XENPF_INTERFACE_VERSION;
>> +    op.u.microcode.length = len;
>> +    xc_platform_op(xc_handle, &op);
>> +
>> +    xc_hypercall_buffer_free(xc_handle, uc);
>> +    xc_interface_close(xc_handle);
>> +
>> +    if (munmap(fbuf, len))
>> +    {
>> +        printf("Could not unmap: %d(%s)\n", errno, strerror(errno));
>> +        return errno;
>> +    }
>> +
>> +    close(fd);
>
> Given that you never close the file on errors this is not really necessary. 
> Or you should close it on errors for consistency.

Its not required but I'll do this as I think its better, and while
at it I'll also add a check for xc_hypercall_buffer_alloc() failure
as no check is there for it now. Will send out a v2.

  Luis

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Andrew Cooper]

On 27/01/2015 20:11, Luis R. Rodriguez wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> There are several ways that a Xen system can update the
> CPU microcode on a pvops kernel [0] now, the preferred way
> is through the early microcode update mechanism. At run
> time folks should use this new xenmicrocode tool and use
> the same CPU microcode file as present on /lib/firmware.
>
> Some distributions may use the historic sysfs rescan interface,
> users of that mechanism should be aware that the interface will
> not be available when using Xen and as such should first check
> the presence of the interface before usage, as an alternative
> this xenmicrocode tool can be used on priviledged domains.
>
> Folks wishing to update CPU microcode at run time should be
> aware that not all CPU microcode can be updated on a system
> and should take care to ensure that only known-to-work and
> supported CPU microcode updates are used [0].
>
> [0] http://wiki.xenproject.org/wiki/XenParavirtOps/microcode_update
>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Olaf Hering <ohering@suse.de>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Jason Douglas <jdouglas@suse.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Henrique de Moraes Holschuh <hmh@debian.org>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>

I am not convinced of the safely of permitting microcode updates at
runtime.  We have seen in the past that having mismatched microcode on
different halfs of an AMD cluster causes system crashes when non-root
mode is in use.  There is no protection against this in the hypercall,
although it could be argued that this could be Xen's problem to solve
rather than userspace.

Either way, updating micrcode is probably not something an admin would
wish to do with running VMs.

> ---
>  tools/libxc/xc_misc.c     | 19 +++++++++++
>  tools/libxc/xenctrl.h     |  2 ++
>  tools/misc/Makefile       |  7 ++--
>  tools/misc/xenmicrocode.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 107 insertions(+), 2 deletions(-)
>  create mode 100644 tools/misc/xenmicrocode.c
>
> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> index e253a58..3ef2664 100644
> --- a/tools/libxc/xc_misc.c
> +++ b/tools/libxc/xc_misc.c
> @@ -250,6 +250,25 @@ int xc_mca_op(xc_interface *xch, struct xen_mc *mc)
>      xc_hypercall_bounce_post(xch, mc);
>      return ret;
>  }

Newlines and coding style please.

> +int xc_platform_op(xc_interface *xch, struct xen_platform_op *op)
> +{
> +    int ret = 0;
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BOUNCE(op, sizeof(*op), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +
> +    if ( xc_hypercall_bounce_pre(xch, op) )
> +    {
> +        PERROR("Could not bounce xen_platform_op memory buffer");
> +        return -1;
> +    }
> +    op->interface_version = XENPF_INTERFACE_VERSION;
> +
> +    hypercall.op = __HYPERVISOR_platform_op;
> +    hypercall.arg[0] = HYPERCALL_BUFFER_AS_ARG(op);
> +    ret = do_xen_hypercall(xch, &hypercall);
> +    xc_hypercall_bounce_post(xch, op);
> +    return ret;
> +}
>  #endif
>  
>  int xc_perfc_reset(xc_interface *xch)
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 514b241..5085e50 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -54,6 +54,7 @@
>  #include <xen/foreign/x86_32.h>
>  #include <xen/foreign/x86_64.h>
>  #include <xen/arch-x86/xen-mca.h>
> +#include <xen/platform.h>
>  #endif
>  
>  #define XC_PAGE_SHIFT           12
> @@ -2131,6 +2132,7 @@ int xc_cpuid_apply_policy(xc_interface *xch,
>  void xc_cpuid_to_str(const unsigned int *regs,
>                       char **strs); /* some strs[] may be NULL if ENOMEM */
>  int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
> +int xc_platform_op(xc_interface *xch, struct xen_platform_op *platform_op);
>  #endif
>  
>  struct xc_px_val {
> diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> index 69b1817..bb838d0 100644
> --- a/tools/misc/Makefile
> +++ b/tools/misc/Makefile

You are going to have to rebase this on top of master.  I have altered
this makefile quite substantially.

> @@ -10,7 +10,7 @@ CFLAGS += $(CFLAGS_libxenstore)
>  HDRS     = $(wildcard *.h)
>  
>  TARGETS-y := xenperf xenpm xen-tmem-list-parse gtraceview gtracestat xenlockprof xenwatchdogd xencov
> -TARGETS-$(CONFIG_X86) += xen-detect xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump
> +TARGETS-$(CONFIG_X86) += xen-detect xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump xenmicrocode
>  TARGETS-$(CONFIG_MIGRATE) += xen-hptool
>  TARGETS := $(TARGETS-y)
>  
> @@ -22,7 +22,7 @@ INSTALL_BIN := $(INSTALL_BIN-y)
>  
>  INSTALL_SBIN-y := xen-bugtool xen-python-path xenperf xenpm xen-tmem-list-parse gtraceview \
>  	gtracestat xenlockprof xenwatchdogd xen-ringwatch xencov
> -INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump
> +INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx xen-hvmcrash xen-lowmemd xen-mfndump xenmicrocode
>  INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
>  INSTALL_SBIN := $(INSTALL_SBIN-y)
>  
> @@ -66,6 +66,9 @@ xenperf: xenperf.o
>  xenpm: xenpm.o
>  	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
>  
> +xenmicrocode: xenmicrocode.o
> +	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
> +
>  gtracestat: gtracestat.o
>  	$(CC) $(LDFLAGS) -o $@ $< $(APPEND_LDFLAGS)
>  
> diff --git a/tools/misc/xenmicrocode.c b/tools/misc/xenmicrocode.c
> new file mode 100644
> index 0000000..5c58a1b
> --- /dev/null
> +++ b/tools/misc/xenmicrocode.c
> @@ -0,0 +1,81 @@
> +#define _GNU_SOURCE
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <inttypes.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <xenctrl.h>
> +
> +int main(int argc, char *argv[])
> +{
> +    int fd = 0;
> +    unsigned char *fbuf;
> +    int len;
> +    xc_interface *xc_handle;
> +    char *filename;
> +    struct stat buf;
> +    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);

I am of the opinion that hypercall buffers should be an internal detail
to libxc, and not exposed to consumers.  I suggest introducing an
xc_microcode_update() function which takes a plain uint8_t buffer and
does the bouncing itself.

> +    struct xen_platform_op op;
> +
> +    filename = argv[1];

what happens if someone calls this without any parameters, or with
--help perhaps?

> +    fd = open(filename, O_RDONLY);
> +
> +    if (fd <= 0)
> +    {
> +        printf("Could not open; err: %d(%s)\n", errno, strerror(errno));
> +        return errno;
> +    }
> +
> +    if (stat(filename, &buf) != 0)
> +    {
> +        printf("Could not open; err: %d(%s)\n", errno, strerror(errno));

A little too much copy/paste?

> +        return errno;
> +    }
> +
> +    printf("%s: %ld\n", filename, buf.st_size);
> +
> +    len = buf.st_size;
> +    fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
> +
> +    if ((xc_handle = xc_interface_open(0,0,0)) == 0)

NULL and 0 are two very different things.  Please use each appropriately.

~Andrew

> +    {
> +        fprintf(stderr, "Error opening xc interface: %d (%s)\n",
> +                errno, strerror(errno));
> +        return 1;
> +    }
> +
> +    if (fbuf == MAP_FAILED)
> +    {
> +        printf("Could not map: error: %d(%s)\n", errno,
> +               strerror(errno));
> +        return errno;
> +    }
> +
> +    uc = xc_hypercall_buffer_alloc(xc_handle, uc, len);
> +    memcpy(uc, fbuf, len);
> +
> +    set_xen_guest_handle(op.u.microcode.data, uc);
> +    op.cmd = XENPF_microcode_update;
> +    op.interface_version = XENPF_INTERFACE_VERSION;
> +    op.u.microcode.length = len;
> +    xc_platform_op(xc_handle, &op);
> +
> +    xc_hypercall_buffer_free(xc_handle, uc);
> +    xc_interface_close(xc_handle);
> +
> +    if (munmap(fbuf, len))
> +    {
> +        printf("Could not unmap: %d(%s)\n", errno, strerror(errno));
> +        return errno;
> +    }
> +
> +    close(fd);
> +
> +    return 0;
> +}

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Borislav Petkov]

On Tue, Jan 27, 2015 at 10:26:00PM +0000, Andrew Cooper wrote:
> I am not convinced of the safely of permitting microcode updates at
> runtime.  We have seen in the past that having mismatched microcode on
> different halfs of an AMD cluster causes system crashes when non-root

What kind of CPU mix are we talking about here?

And how is microcode on those machines supposed to be updated,
regardless of OS, i.e. how does the platform vendor do those updates?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Andrew Cooper]

On 27/01/2015 23:17, Borislav Petkov wrote:
> On Tue, Jan 27, 2015 at 10:26:00PM +0000, Andrew Cooper wrote:
>> I am not convinced of the safely of permitting microcode updates at
>> runtime.  We have seen in the past that having mismatched microcode on
>> different halfs of an AMD cluster causes system crashes when non-root
> What kind of CPU mix are we talking about here?
>
> And how is microcode on those machines supposed to be updated,
> regardless of OS, i.e. how does the platform vendor do those updates?
>

There was a thread on xen-devel but I cant currently find it in the
archives.

To the best of my memory,  it was a 4 core APU system where the BIOS had
updated the microcode on cpu 0 but left 1-3 at a lower patch level. 
Every time the reporter tried creating an HVM guest (i.e. entering SVM
non-root mode), the system reset.

The instability was sorted by ensuring each core was at the same
microcode level.

As Xen updates microcode one cpu at a time from 0, it could easily
create a similar situation if microcode is updated after VMs have been
started.  Come to think of it, this is also an impending problem for PVH
dom0 systems.

~Andrew

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Borislav Petkov]

On Wed, Jan 28, 2015 at 12:10:43AM +0000, Andrew Cooper wrote:
> There was a thread on xen-devel but I cant currently find it in the
> archives.
> 
> To the best of my memory,  it was a 4 core APU system where the BIOS had
> updated the microcode on cpu 0 but left 1-3 at a lower patch level. 
> Every time the reporter tried creating an HVM guest (i.e. entering SVM
> non-root mode), the system reset.
> 
> The instability was sorted by ensuring each core was at the same
> microcode level.

That sounds like a BIOS bug to me, frankly.

> As Xen updates microcode one cpu at a time from 0, it could easily
> create a similar situation if microcode is updated after VMs have been
> started.  Come to think of it, this is also an impending problem for PVH
> dom0 systems.

The common way for doing microcode updates is to update all cores at
the same time, possibly. Or at least as close to one another in time as
possible.

Now, we do two methods:

* the early update which should be done as early as possible during
boot. I don't think that should be a problem wrt to guests if you do it
early enough.

* the late update is an addition to the early one to cover the cases of
long running systems where a reboot is prohibitively painful. With that,
as with the early method, you would want to update all hardware cores in
one go.

Now, this is where it becomes tricky for virt: you need to stop guests,
do the update and then resume them. Even worse, if all of a sudden you
want to hide hardware features and/or instructions like HSW TSX for
example, you most likely want to even avoid the late update and warn the
admin that she has to reboot that machine and apply microcode with the
early method.

So this should be the gist of it...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Luis R. Rodriguez]

On Wed, Jan 28, 2015 at 09:39:24AM +0100, Borislav Petkov wrote:
> On Wed, Jan 28, 2015 at 12:10:43AM +0000, Andrew Cooper wrote:
> > There was a thread on xen-devel but I cant currently find it in the
> > archives.
> > 
> > To the best of my memory,  it was a 4 core APU system where the BIOS had
> > updated the microcode on cpu 0 but left 1-3 at a lower patch level. 
> > Every time the reporter tried creating an HVM guest (i.e. entering SVM
> > non-root mode), the system reset.
> > 
> > The instability was sorted by ensuring each core was at the same
> > microcode level.
> 
> That sounds like a BIOS bug to me, frankly.
> 
> > As Xen updates microcode one cpu at a time from 0, it could easily
> > create a similar situation if microcode is updated after VMs have been
> > started.  Come to think of it, this is also an impending problem for PVH
> > dom0 systems.
> 
> The common way for doing microcode updates is to update all cores at
> the same time, possibly. Or at least as close to one another in time as
> possible.

How close?

> Now, we do two methods:
> 
> * the early update which should be done as early as possible during
> boot. I don't think that should be a problem wrt to guests if you do it
> early enough.
> 
> * the late update is an addition to the early one to cover the cases of
> long running systems where a reboot is prohibitively painful. With that,
> as with the early method, you would want to update all hardware cores in
> one go.
> 
> Now, this is where it becomes tricky for virt: you need to stop guests,
> do the update and then resume them. Even worse, if all of a sudden you
> want to hide hardware features and/or instructions like HSW TSX for
> example, you most likely want to even avoid the late update and warn the
> admin that she has to reboot that machine and apply microcode with the
> early method.
> 
> So this should be the gist of it...

I've reviewed the implmentation a bit more on the Xen side. For early boot
things look similar to what is done upstream on the kernel. For the run time
update here's what Xen does in detail, elaborating a bit more on Andrew's
summary of how it works.

The XENPF_microcode_update hypercall calls the general Xen microcode_update() which
will do microcode_ops->start_update() (only AMD has this op for and it does
svm_host_osvw_reset()) and finally it continues the hypercall by calling
do_microcode_update() on the cpumask_first(&cpu_online_map) *always*. The
mechanism that Xen uses to continue the hypercall is by using
continue_hypercall_on_cpu(), if this returns 0 then it is guaranteed to run
*at some in the future* on the given CPU. If preemption is enabled this
could also mean the hypercall was preempted, and can be preempted later
on the other CPU. This will in turn will do the same call but on the
next CPU using continuation until it reaches the end of the CPU mask.
The do_microcode_update() call itself calls ops->cpu_request_microcode()
on each iteration which in turn should also do the ops->apply_microcode()
once a microcode buffer on the file that fits is found. The buffers are
kept in case of suspend / resume.

There is no tight loop here or locking of what other CPUs do while one is running
work to update microcode. Tons of things can happen in between so some refinements
seem desirable and likely this implementation does differ quite signifantly
over the Linux kernel's legacy 'rescan' interface.

Given this review, it seems folks should use xenmicrocode keeping in mind the
above algorithm, and support wise folks should be ready to consider upgrades on
microcode and possible issues / caveats from vendors on a case by case basis.

>From what I gather some folks have even considered tainting kernels when the
sysfs rescan interface is used, I do wonder if this is worthy on Xen for this
tool given the possible issues here... or am I just paranoid about this?
It seems like this might be more severe of an issue for Xen as-is.

  Luis

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Henrique de Moraes Holschuh]

On Wed, Jan 28, 2015, at 06:39, Borislav Petkov wrote:
> On Wed, Jan 28, 2015 at 12:10:43AM +0000, Andrew Cooper wrote:
> > There was a thread on xen-devel but I cant currently find it in the
> > archives.
> > 
> > To the best of my memory,  it was a 4 core APU system where the BIOS had
> > updated the microcode on cpu 0 but left 1-3 at a lower patch level. 

Which is a situation that will *always* happen  when the late microcode
update driver is used, both on Intel and AMD.  You will always have a
time window where the processor is running with mismatched microcode
between some of the hardware threads/cores/modules/whatever.

This is not an issue when using the bare-metal early microcode update
driver, because it updates the BSP while still in uniprocessor mode, and
it updates the APs very early on the processor bootstrap code, well
before they are on-lined.  We can control what machien code runs in a
hardware thread that might be running mismatched microcode (i.e. that
runs before the processor bootstrap code attempts to update the
microcode) and keep it simple and away from anything that would heavly
object to mismatched microcode.

Likewise, it is not an issue for a non-broken BIOS/UEFI, as it is
*supposed* to update everything to the same microcode well before it
attempts to do anything complex.

> > Every time the reporter tried creating an HVM guest (i.e. entering SVM
> > non-root mode), the system reset.
> > 
> > The instability was sorted by ensuring each core was at the same
> > microcode level.
> 
> That sounds like a BIOS bug to me, frankly.

Sort of.  The extremely wide time window of mismatched microcode in that
computer was a BIOS bug, of course.

But the fact that you cannot trust a system with mismatched microcode to
be stable is the hard truth: neither AMD nor Intel are really enforcing
that late microcode updates will be always safe in all conditions.

What we can do about it in the Linux kernel late microcode driver is to
shorten that window as much as possible, and try to quiesce the system
as much as possible during the microcode update until all cores have
been updated.

It still looks like Xen should *never* trigger a late microcode update,
unless it freezes all VMs first.

> > As Xen updates microcode one cpu at a time from 0, it could easily
> > create a similar situation if microcode is updated after VMs have been
> > started.  Come to think of it, this is also an impending problem for PVH
> > dom0 systems.
> 
> The common way for doing microcode updates is to update all cores at
> the same time, possibly. Or at least as close to one another in time as
> possible.

The later.  We serialize microcode updates across CPUs, and doing them
all at the same time is neither trivial (unforeseen side effects on a
running system) nor future-proof.

For example, on Intel you must *never* have two CPUs attempt to update
the same "microcode store" at the same time, which requires that you
actually know how the microcode is partitioned relative to
packages/cores/threads (so far, this is easy: HT siblings share
microcode, nothing else does.  But what about future processors?).

> * the late update is an addition to the early one to cover the cases of
> long running systems where a reboot is prohibitively painful. With that,
> as with the early method, you would want to update all hardware cores in
> one go.

And, unfortunately, you have a time window of mismatched microcode
during the "one go", which is not something we can fix.

So we would have to try to limit what happens during that time window,
instead.

> Now, this is where it becomes tricky for virt: you need to stop guests,
> do the update and then resume them. Even worse, if all of a sudden you
> want to hide hardware features and/or instructions like HSW TSX for
> example, you most likely want to even avoid the late update and warn the
> admin that she has to reboot that machine and apply microcode with the
> early method.

Exactly.  But it goes further: we likely should freeze the entire kernel
and run nothing (not even interrupt handling) on non-up-to-date cores. 
I.e. offline every CPU but one, switch to the last online CPU, update
its microcode, then update the other ones one-at-a-time, onlining them
after they are up-to-date (and leaving them offline if something wrong
happens).

Or something to that effect.

It is no wonder we currently "hope for the best" as far as late
microcode update mode goes, and also that Linux distros are switching to
"early updates only" by default.  BTW, most datacenter people I know
have a policy of never updating *any* firmware at all outside of
maintenance downtime, so they're actually quite fine with the idea of a
reboot being required to update processor microcode.

-- 
  "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 de Moraes Holschuh <hmh@debian.org>

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Borislav Petkov]

On Thu, Jan 29, 2015 at 09:36:49AM -0200, Henrique de Moraes Holschuh wrote:
> But the fact that you cannot trust a system with mismatched microcode to
> be stable is the hard truth: neither AMD nor Intel are really enforcing
> that late microcode updates will be always safe in all conditions.

How do you know? Have you talked to anyone?

Can you imagine that someone might have done that and asked exactly that
question and got an assurance that CPU vendors actually do try to make
microcode updates self-contained?

So let me stop you right there with that "hard truth" drama. Just stick
to the facts. If you don't have any facts, don't create them out of thin
air. Ok?

The HSW TSX stuff had to be done that way and I'm sure Intel had their
good reasons. And I'm sure they bought the pain of the late update
explosions.

And software should support even the use case of late updates. And that
use case is very simple - if a microcode patch is self-contained, then
it should get applied. If not - and here we rely on the CPU vendors to
let us know; if they don't, we learn it the hard way anyway - we warn
the user and disallow the update. This is a very simple thing to solve.

> What we can do about it in the Linux kernel late microcode driver is to
> shorten that window as much as possible, and try to quiesce the system
> as much as possible during the microcode update until all cores have
> been updated.
> 
> It still looks like Xen should *never* trigger a late microcode update,
> unless it freezes all VMs first.

Now this is better.

> For example, on Intel you must *never* have two CPUs attempt to update
> the same "microcode store" at the same time, which requires that you

Interesting - this is the first time I hear about such restriction.
Details?

> actually know how the microcode is partitioned relative to
> packages/cores/threads (so far, this is easy: HT siblings share
> microcode, nothing else does.  But what about future processors?).

What about it? Software gets adjusted like for anything else.

> ... so they're actually quite fine with the idea of a reboot being
> required to update processor microcode.

And then there's the other group who can't afford to reboot long running
machines for whatever reason. As long as we can support both, we should
support both.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Henrique de Moraes Holschuh]

On Thu, Jan 29, 2015, at 10:17, Borislav Petkov wrote:
> On Thu, Jan 29, 2015 at 09:36:49AM -0200, Henrique de Moraes Holschuh
> wrote:
> > But the fact that you cannot trust a system with mismatched microcode to
> > be stable is the hard truth: neither AMD nor Intel are really enforcing
> > that late microcode updates will be always safe in all conditions.
> 
> How do you know? Have you talked to anyone?

No, I have not.  It was a direct observation based on the fact that
there is a report of a system misbehaving because of mismatched AMD
microcode in this very thread.  Now, if someone from AMD did state that
they are enforcing that the processor should work with mismatched
microcode at all times, so that it won't happen again, I stand
corrected.

That said, I don't see how anyone could support stable operation in all
cases on a system with mismatched microcode (time window where not all
cores have been updated yet) without adding quite important constraints,
either to what you can do in a microcode update, or on what the system
can do while it has not yet finished updating all cores (probably both).

I recall some AMD microcode updates did add functionality, so you could
have issues if a task migrates from a more capable microcode (up-to-date
core) to a less capable microcode (non-up-to-date core), for example.

The same reasoning is valid for Intel.

> Can you imagine that someone might have done that and asked exactly that
> question and got an assurance that CPU vendors actually do try to make
> microcode updates self-contained?

If that happened, I'd like to see the list of assumptions made by the
hardware vendor side, though.  When is that assurance valid?  Are we
meeting all their expectations?

> So let me stop you right there with that "hard truth" drama. Just stick
> to the facts. If you don't have any facts, don't create them out of thin
> air. Ok?

See above.  I am not sure we'd be even asking for something that is
possible/desireable [as currently implemented in Linux].  Currently we
run the late microcode update with the system as-is, in full production
mode...

Maybe if we could ensure (in Linux) that there will be no task migration
between CPUs (including due to clone/fork/whatever) and no global system
state change (i.e. visible to tasks on other processors) that could
depend on microcode-provided capabilities until the late microcode
update system finished the microcode update run for all processors...

> > For example, on Intel you must *never* have two CPUs attempt to update
> > the same "microcode store" at the same time, which requires that you
> 
> Interesting - this is the first time I hear about such restriction.
> Details?

The restriction is in the Intel SDM, section 9.11.6.2, (vol. 3A, page
9-35):

"9.11.6.3   Update in a System Supporting Intel Hyper-Threading
Technology:
Intel Hyper-Threading Technology has implications on the loading of the
microcode update. The update must be
loaded for each core in a physical processor. Thus, for a processor
supporting Intel Hyper-Threading Technology,
only one logical processor per core is required to load the microcode
update. Each individual logical processor can
independently load the update. However, MP initialization must provide
some mechanism (e.g. a software sema-
phore) to force serialization of microcode update loads and to prevent
simultaneous load attempts to the same
core."

I recall (from my code archeology efforts a few months ago on the intel
microcode driver) that the serialization across CPUs during a microcode
update was added to the ancient Intel microcode driver more than a
decade ago because of that requirement.

I don't know what would happen should you violate that restriction,
though.  It could be something that modern Intel 64 processors simply
don't care about, just like that 16-byte alignment restriction.

> And then there's the other group who can't afford to reboot long running
> machines for whatever reason. As long as we can support both, we should
> support both.

You know my position on this: I consider it a valid advanced use-case,
for those who really know what they're doing. But it shouldn't be the
default mode of operation for Linux distros, and one shouldn't assume it
is going to be safe without extra data (thus the "for those who really
know what they're doing").

-- 
  "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 de Moraes Holschuh <hmh@debian.org>

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Borislav Petkov]

On Thu, Jan 29, 2015 at 03:01:22PM -0200, Henrique de Moraes Holschuh wrote:
> No, I have not.  It was a direct observation based on the fact that
> there is a report of a system misbehaving because of mismatched AMD
> microcode in this very thread.  Now, if someone from AMD did state that
> they are enforcing that the processor should work with mismatched
> microcode at all times,

It was a dumb BIOS bug, if at all. Drop it already.

<snip drivel about mismatched ucode which is completely beside the point
  here>

<snip more hypothetical drivel which I don't even know what's supposed
 to mean>

> The restriction is in the Intel SDM, section 9.11.6.2, (vol. 3A, page
> 9-35):
> 
> "9.11.6.3   Update in a System Supporting Intel Hyper-Threading
> Technology:

Ah that.

We have that serialization from the design of the late reloading by
iterating over all cores in succession. After we update the first
logical core of two-threads core which share the microcode engine, the
updated version is visible on the second hyperthread and we don't do the
update there.

If you're seeing actual bug reports about that, feel free to CC me.

> You know my position on this: I consider it a valid advanced use-case,
> for those who really know what they're doing. But it shouldn't be the
> default mode of operation for Linux distros, and one shouldn't assume
> it is going to be safe without extra data (thus the "for those who
> really know what they're doing").

I don't see the reason to force people who update the software on their
machines to reboot them just because a new microcode patch was released
for their hw.

So, for those cases, we can simply use the late method. During the
update, the initrd gets regenerated with the new patch anyway so when
they reboot next time, they get the update.

But forcing them to reboot specifically just because they updated their
microcode patch and that microcode patch is self-contained is completely
unnecessary.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Andrew Cooper]

<snip>
Getting this conversation back on topic.

The current state of play in Xen is this:

* Boot time microcode loading exists (by scanning uncompressed cpio
multiboot modules) and should be safe to use.

* The facility for runtime microcode loading exists (via privileged
hypercall), but is unsafe to use at present, especially if virtual
machines are running.  There are several steps which can be taken to
make it safer to use.


There is a plausible usecase for runtime microcode loading for people
who wish to take that risk, and as such, xenmicrocode is useful utility
to have, but it should probably not be available by default until we
believe the hypervisor side of the interface avoids the known potholes.

~Andrew

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Borislav Petkov]

On Thu, Jan 29, 2015 at 04:21:05AM +0100, Luis R. Rodriguez wrote:
> How close?

As close as we can get but not closer - see the thing about updating
microcode on Intel hyperthreaded logical cores in the other mail.

We probably can do it in parallel if needed. But it hasn't been needed
until now.

> I've reviewed the implmentation a bit more on the Xen side. For early boot
> things look similar to what is done upstream on the kernel. For the run time
> update here's what Xen does in detail, elaborating a bit more on Andrew's
> summary of how it works.
> 
> The XENPF_microcode_update hypercall calls the general Xen microcode_update() which
> will do microcode_ops->start_update() (only AMD has this op for and it does
> svm_host_osvw_reset()) and finally it continues the hypercall by calling
> do_microcode_update() on the cpumask_first(&cpu_online_map) *always*. The
> mechanism that Xen uses to continue the hypercall is by using
> continue_hypercall_on_cpu(), if this returns 0 then it is guaranteed to run
> *at some in the future* on the given CPU. If preemption is enabled this
> could also mean the hypercall was preempted, and can be preempted later
> on the other CPU. This will in turn will do the same call but on the
> next CPU using continuation until it reaches the end of the CPU mask.
> The do_microcode_update() call itself calls ops->cpu_request_microcode()
> on each iteration which in turn should also do the ops->apply_microcode()
> once a microcode buffer on the file that fits is found. The buffers are
> kept in case of suspend / resume.

Yah, this is mostly fine except the preemption thing. If the guests get
to see an inconsistent state with a subset of the cores updated and the
rest not, then that is bad.

Not to mention the case when we have to late-update problematic
microcode which has to happen in parallel on each core. I haven't seen
one so far but we should be prepared.

> There is no tight loop here or locking of what other CPUs do while one is running
> work to update microcode. Tons of things can happen in between so some refinements
> seem desirable and likely this implementation does differ quite signifantly
> over the Linux kernel's legacy 'rescan' interface.

Well, we're not very strict there either but that works so far. We'll
change it if the need arises.

> Given this review, it seems folks should use xenmicrocode keeping in mind the
> above algorithm, and support wise folks should be ready to consider upgrades on
> microcode and possible issues / caveats from vendors on a case by case basis.

Right.

> From what I gather some folks have even considered tainting kernels when the
> sysfs rescan interface is used, I do wonder if this is worthy on Xen for this
> tool given the possible issues here... or am I just paranoid about this?
> It seems like this might be more severe of an issue for Xen as-is.

That would not be unnecesary.

So, I would try to do the application of the microcode in the hypervisor
as tight as possible. Maybe the hypercall could hand in the microcode
blob only and the hypervisor can "schedule" an update later, after
having frozen the guests.

In any case, we should strive for a parallel late update, simultaneously
on each core with no interruption. The kernel doesn't do that either but
it'll probably have to, one day.

I don't know whether this is possible at all in xen and whether doing
a simple sequential update method now and improving it later is easier
than doing it right and in parallel from the get-go. I'm talking
hypothetically here, I have no idea what actually is possible and doable
in xen.

HTH.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Konrad Rzeszutek Wilk]

On Thu, Jan 29, 2015 at 06:34:23PM +0000, Andrew Cooper wrote:
> <snip>
> Getting this conversation back on topic.
> 
> The current state of play in Xen is this:
> 
> * Boot time microcode loading exists (by scanning uncompressed cpio
> multiboot modules) and should be safe to use.

Please note that it does require passing in 'ucode=scan' on the Xen
command line and does not do it automatically. It would be nice
if that was automatic..
> 
> * The facility for runtime microcode loading exists (via privileged
> hypercall), but is unsafe to use at present, especially if virtual
> machines are running.  There are several steps which can be taken to
> make it safer to use.
> 
> 
> There is a plausible usecase for runtime microcode loading for people
> who wish to take that risk, and as such, xenmicrocode is useful utility
> to have, but it should probably not be available by default until we
> believe the hypervisor side of the interface avoids the known potholes.

Aren't these issues the same if we had an runtime microcode
implementation (I am referring to the xen-microcode driver that
Jeremy wrote once and some distros have in their kernel). The loading
of microcode is done the same was as baremetal via 'rescan' interface.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Andrew Cooper]

On 29/01/2015 20:12, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 29, 2015 at 06:34:23PM +0000, Andrew Cooper wrote:
>> <snip>
>> Getting this conversation back on topic.
>>
>> The current state of play in Xen is this:
>>
>> * Boot time microcode loading exists (by scanning uncompressed cpio
>> multiboot modules) and should be safe to use.
> Please note that it does require passing in 'ucode=scan' on the Xen
> command line and does not do it automatically. It would be nice
> if that was automatic..

If there is an efficent way for Xen to identify compressed or non-cpio
boot modules and skip them (I have not inspected the code sufficiently
to know), it is perhaps the kind of option we should consider enabling
by default.

>> * The facility for runtime microcode loading exists (via privileged
>> hypercall), but is unsafe to use at present, especially if virtual
>> machines are running.  There are several steps which can be taken to
>> make it safer to use.
>>
>>
>> There is a plausible usecase for runtime microcode loading for people
>> who wish to take that risk, and as such, xenmicrocode is useful utility
>> to have, but it should probably not be available by default until we
>> believe the hypervisor side of the interface avoids the known potholes.
> Aren't these issues the same if we had an runtime microcode
> implementation (I am referring to the xen-microcode driver that
> Jeremy wrote once and some distros have in their kernel). The loading
> of microcode is done the same was as baremetal via 'rescan' interface.

Correct.  Any use of the hypercall mechanism is currently susceptible to
the potholes identified previously in this thread, and therefore unsafe
for use in practice.  This includes the classic-xen kernel driver, and
this new userspace utility.  Having the dom0 kernel issue the hypercall
as a result of its own boot time microcode patching has fewer potholes
to trip over, than later when domUs have been booted.

~Andrew

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Luis R. Rodriguez]

On Thu, Jan 29, 2015 at 08:15:16PM +0100, Borislav Petkov wrote:
> On Thu, Jan 29, 2015 at 04:21:05AM +0100, Luis R. Rodriguez wrote:
> > How close?
> 
> As close as we can get but not closer - see the thing about updating
> microcode on Intel hyperthreaded logical cores in the other mail.
> 
> We probably can do it in parallel if needed. But it hasn't been needed
> until now.
> 
> > I've reviewed the implmentation a bit more on the Xen side. For early boot
> > things look similar to what is done upstream on the kernel. For the run time
> > update here's what Xen does in detail, elaborating a bit more on Andrew's
> > summary of how it works.
> > 
> > The XENPF_microcode_update hypercall calls the general Xen microcode_update() which
> > will do microcode_ops->start_update() (only AMD has this op for and it does
> > svm_host_osvw_reset()) and finally it continues the hypercall by calling
> > do_microcode_update() on the cpumask_first(&cpu_online_map) *always*. The
> > mechanism that Xen uses to continue the hypercall is by using
> > continue_hypercall_on_cpu(), if this returns 0 then it is guaranteed to run
> > *at some in the future* on the given CPU. If preemption is enabled this
> > could also mean the hypercall was preempted, and can be preempted later
> > on the other CPU. This will in turn will do the same call but on the
> > next CPU using continuation until it reaches the end of the CPU mask.
> > The do_microcode_update() call itself calls ops->cpu_request_microcode()
> > on each iteration which in turn should also do the ops->apply_microcode()
> > once a microcode buffer on the file that fits is found. The buffers are
> > kept in case of suspend / resume.
> 
> Yah, this is mostly fine except the preemption thing. If the guests get
> to see an inconsistent state with a subset of the cores updated and the
> rest not, then that is bad.
> 
> Not to mention the case when we have to late-update problematic
> microcode which has to happen in parallel on each core. I haven't seen
> one so far but we should be prepared.
> 
> > There is no tight loop here or locking of what other CPUs do while one is running
> > work to update microcode. Tons of things can happen in between so some refinements
> > seem desirable and likely this implementation does differ quite signifantly
> > over the Linux kernel's legacy 'rescan' interface.
> 
> Well, we're not very strict there either but that works so far. We'll
> change it if the need arises.
> 
> > Given this review, it seems folks should use xenmicrocode keeping in mind the
> > above algorithm, and support wise folks should be ready to consider upgrades on
> > microcode and possible issues / caveats from vendors on a case by case basis.
> 
> Right.
> 
> > From what I gather some folks have even considered tainting kernels when the
> > sysfs rescan interface is used, I do wonder if this is worthy on Xen for this
> > tool given the possible issues here... or am I just paranoid about this?
> > It seems like this might be more severe of an issue for Xen as-is.
> 
> That would not be unnecesary.
> 
> So, I would try to do the application of the microcode in the hypervisor
> as tight as possible. Maybe the hypercall could hand in the microcode
> blob only and the hypervisor can "schedule" an update later, after
> having frozen the guests.

OK I like this for now.

> In any case, we should strive for a parallel late update, simultaneously
> on each core with no interruption. The kernel doesn't do that either but
> it'll probably have to, one day.

OK.

> I don't know whether this is possible at all in xen and whether doing
> a simple sequential update method now and improving it later is easier
> than doing it right and in parallel from the get-go. I'm talking
> hypothetically here, I have no idea what actually is possible and doable
> in xen.

The implementation of the microcode update right now is done serially
by continuing the hypercall on each CPU. To do what you say seems
possible but would obviously require quite a bit of changes from
the existing solution.

For now I've just given a try to quiesce the domains before doing
the microcode update, will send that out next for RFC.

  Luis

Re: [PATCH] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Konrad Rzeszutek Wilk]

On Fri, Jan 30, 2015 at 12:29:15AM +0000, Andrew Cooper wrote:
> On 29/01/2015 20:12, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jan 29, 2015 at 06:34:23PM +0000, Andrew Cooper wrote:
> >> <snip>
> >> Getting this conversation back on topic.
> >>
> >> The current state of play in Xen is this:
> >>
> >> * Boot time microcode loading exists (by scanning uncompressed cpio
> >> multiboot modules) and should be safe to use.
> > Please note that it does require passing in 'ucode=scan' on the Xen
> > command line and does not do it automatically. It would be nice
> > if that was automatic..
> 
> If there is an efficent way for Xen to identify compressed or non-cpio
> boot modules and skip them (I have not inspected the code sufficiently
> to know), it is perhaps the kind of option we should consider enabling
> by default.

It scans only for cpio archive and then searches for a specific string
- and if it finds that then it will slurp the binary up and use that
as an candidate for microcode patching.

In that sense anything that is not not-cpio is skipped (compressed, binary,
non-cpio, etc).
> 
> >> * The facility for runtime microcode loading exists (via privileged
> >> hypercall), but is unsafe to use at present, especially if virtual
> >> machines are running.  There are several steps which can be taken to
> >> make it safer to use.
> >>
> >>
> >> There is a plausible usecase for runtime microcode loading for people
> >> who wish to take that risk, and as such, xenmicrocode is useful utility
> >> to have, but it should probably not be available by default until we
> >> believe the hypervisor side of the interface avoids the known potholes.
> > Aren't these issues the same if we had an runtime microcode
> > implementation (I am referring to the xen-microcode driver that
> > Jeremy wrote once and some distros have in their kernel). The loading
> > of microcode is done the same was as baremetal via 'rescan' interface.
> 
> Correct.  Any use of the hypercall mechanism is currently susceptible to
> the potholes identified previously in this thread, and therefore unsafe
> for use in practice.  This includes the classic-xen kernel driver, and
> this new userspace utility.  Having the dom0 kernel issue the hypercall
> as a result of its own boot time microcode patching has fewer potholes
> to trip over, than later when domUs have been booted.

I am surprised you haven't volunteered to put it on your 'free-time' todo
list to fix this :-) <chuckles>
> 
> ~Andrew