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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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;
> +}