Re: [Xen-devel] [RFC v2] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/01/15 01:14, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
>
> 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]. To avoid issues
> with delays on the hypercall / microcode update this
> implementation will quiesce all domains prior to updating
> the microcode, and then only queisce'd domains will be
> unpaused once done.
>
> [0] http://wiki.xenproject.org/wiki/XenParavirtOps/microcode_update
>
> Based on original work by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: Takashi Iwai <tiwai@xxxxxxx>
> Cc: Olaf Hering <ohering@xxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Jason Douglas <jdouglas@xxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Michal Marek <mmarek@xxxxxxx>
> Cc: Henrique de Moraes Holschuh <hmh@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx>
> ---
>
> Just wrote this, haven't tested it. This does some queiscing prior
> to doing the microcode update. The quiescing is done by pausing
> all domains. Once the microcode update is done we only unpause
> domains which we queisced as part of our work. Let me know if this
> is on the right track to help avoid issues noted on the list.

There is also a TOCTOU race with your paused check, which itself is
buggy as it you should unconditionally pause all VMs (userspace pause
refcounting has been fixed for a little while now).

However, xenmicrocode (even indirectly via xc_microcode_update()) is not
in a position to safely pause all domains as there is no interlock to
preventing a new domain being created.  Even if all domains do get
successfully paused, the idle loops are substantially less trivial than
ideal.

The toolstack should not hack around hypervisor bugs, and indeed is not
capable of doing so.

>
>  tools/libxc/include/xenctrl.h |   2 +
>  tools/libxc/xc_misc.c         | 102 
> ++++++++++++++++++++++++++++++++++++++++++
>  tools/misc/Makefile           |   4 ++
>  tools/misc/xenmicrocode.c     | 101 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 209 insertions(+)
>  create mode 100644 tools/misc/xenmicrocode.c
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 0ad8b8d..d5db0b8 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -55,6 +55,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
> @@ -2145,6 +2146,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_microcode_update(xc_interface *xch, uint8_t *fbuf, size_t len);
>  #endif
>  
>  struct xc_px_val {
> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> index e253a58..6137e24 100644
> --- a/tools/libxc/xc_misc.c
> +++ b/tools/libxc/xc_misc.c
> @@ -20,6 +20,7 @@
>  
>  #include "xc_private.h"
>  #include <xen/hvm/hvm_op.h>
> +#include <xen/platform.h>
>  
>  int xc_get_max_cpus(xc_interface *xch)
>  {
> @@ -250,6 +251,107 @@ int xc_mca_op(xc_interface *xch, struct xen_mc *mc)
>      xc_hypercall_bounce_post(xch, mc);
>      return ret;
>  }
> +
> +struct xc_quiesce_request {
> +    int num_quiesced;
> +    int domids[1024]; /* never domid 0 */
> +};
> +
> +/*
> + * Do not pause already paused domains, and allow us to
> + * unpause only quiesced domains.
> + */
> +static int quiesce_all_domains(xc_interface *xch,
> +                            struct xc_quiesce_request *quiesce_request)
> +{
> +    xc_domaininfo_t info[1024];
> +    int i, nb_domain;
> +
> +    nb_domain = xc_domain_getinfolist(xch, 0, 1024, info);
> +    if ( nb_domain < 0 )
> +    {
> +     return -1;
> +    }
> +
> +    for ( i = 0; i < nb_domain; i++ )
> +    {
> +        if ( info[i].domain == 0 )
> +             continue;
> +     if ( info[i].flags & XEN_DOMINF_paused )
> +             continue;
> +
> +        xc_domain_pause(xch, info[i].domain);
> +
> +     quiesce_request->domids[quiesce_request->num_quiesced] = info[i].domain;
> +     quiesce_request->num_quiesced++;
> +    }
> +
> +    return 0;
> +}
> +
> +static void unquiesce_all_domains(xc_interface *xch,
> +                                  struct xc_quiesce_request *quiesce_request)
> +{
> +    int i;
> +
> +    for ( i = 0; i < quiesce_request->num_quiesced; i++ )
> +    {
> +     xc_domain_unpause(xch, quiesce_request->domids[i]);
> +    }
> +}
> +
> +int xc_microcode_update(xc_interface *xch, uint8_t *fbuf, size_t len)
> +{
> +    int ret = 0;
> +    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
> +    DECLARE_HYPERCALL;
> +    struct xen_platform_op op_s;
> +    struct xen_platform_op *op = &op_s;
> +    DECLARE_HYPERCALL_BOUNCE(op, sizeof(*op), 
> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +    struct xc_quiesce_request quiesce_request;
> +
> +    memset(&quiesce_request, 0, sizeof(struct xc_quiesce_request));
> +
> +    if ( !xch )
> +    {
> +        return -1;
> +    }
> +
> +    uc = xc_hypercall_buffer_alloc(xch, uc, len);
> +    if ( uc == NULL )
> +    {
> +        PERROR("Could not allocate memory for xc_microcode_update 
> hypercall");
> +        return -1;
> +    }
> +
> +    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;
> +
> +    if ( xc_hypercall_bounce_pre(xch, op) )
> +    {
> +     xc_hypercall_buffer_free(xch, uc);
> +        PERROR("Could not bounce memory for xc_microcode_update hypercall");
> +        return -1;
> +    }
> +
> +    hypercall.op = __HYPERVISOR_platform_op;
> +    hypercall.arg[0] = HYPERCALL_BUFFER_AS_ARG(op);
> +
> +    quiesce_all_domains(xch, &quiesce_request);
> +    ret = do_xen_hypercall(xch, &hypercall);
> +    unquiesce_all_domains(xch, &quiesce_request);
> +
> +    xc_hypercall_bounce_post(xch, op);
> +    xc_hypercall_buffer_free(xch, uc);
> +    xc_interface_close(xch);
> +
> +    return ret;
> +}
> +
>  #endif
>  
>  int xc_perfc_reset(xc_interface *xch)
> diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> index a255c22..ba4779a 100644
> --- a/tools/misc/Makefile
> +++ b/tools/misc/Makefile
> @@ -21,6 +21,7 @@ INSTALL_SBIN-$(CONFIG_X86)     += xen-hvmcrash
>  INSTALL_SBIN-$(CONFIG_X86)     += xen-hvmctx
>  INSTALL_SBIN-$(CONFIG_X86)     += xen-lowmemd
>  INSTALL_SBIN-$(CONFIG_X86)     += xen-mfndump
> +INSTALL_SBIN-$(CONFIG_X86)     += xenmicrocode

"xen-microcode" perhaps?

>  INSTALL_SBIN                   += xen-ringwatch
>  INSTALL_SBIN                   += xen-tmem-list-parse
>  INSTALL_SBIN                   += xencov
> @@ -74,6 +75,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..d027b13
> --- /dev/null
> +++ b/tools/misc/xenmicrocode.c
> @@ -0,0 +1,101 @@
> +#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>
> +
> +/*
> + * Documentation:
> + *
> + * http://wiki.xenproject.org/wiki/XenParavirtOps/microcode_update
> + */
> +
> +void show_help(void)
> +{
> +    fprintf(stderr,
> +            "xenmicrocode: Xen runtime CPU microcode update tool\n"
> +            "Usage: xenmicrocode </lib/firmware/microcode-file>\n"
> +           );
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    int fd = 0;
> +    uint8_t *fbuf;
> +    char *filename;
> +    struct stat buf;
> +    static xc_interface *xch;
> +    int ret;
> +
> +    if ( argc != 2 )
> +    {
> +     show_help();
> +     return 1;
> +    }
> +
> +    if ( !strcmp("--help", argv[1]) ||
> +         !strcmp("-h", argv[1]) )
> +    {
> +     show_help();
> +        return 1;
> +    }
> +
> +    /* microcode file as present on /lib/firmware/ */
> +    filename = argv[1];
> +
> +    fd = open(filename, O_RDONLY);
> +    if ( fd <= 0 )
> +    {
> +     show_help();
> +        fprintf(stderr,
> +                "Could not open; err: %d(%s)\n", errno, strerror(errno));

You have mixed tabs and spaces.  Please adjust for spaces only and put a
local variable block at the bottom of the file.

> +        return errno;
> +    }
> +
> +    if ( stat(filename, &buf) != 0 )
> +    {
> +        fprintf(stderr, "Could not open; err: %d(%s)\n", errno, 
> strerror(errno));
> +        close(fd);
> +        return errno;
> +    }
> +
> +    xch = xc_interface_open(0,0,0);
> +    if ( !xch )
> +    {
> +        close(fd);
> +        fprintf(stderr, "failed to get the handler\n");
> +        return 1;
> +    }
> +
> +    printf("%s: %ld\n", filename, buf.st_size);
> +
> +    fbuf = mmap(0, buf.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +    if ( fbuf == MAP_FAILED )
> +    {
> +        fprintf(stderr, "Could not map: error: %d(%s)\n", errno,
> +                strerror(errno));
> +        close(fd);
> +        return errno;
> +    }
> +
> +    ret = xc_microcode_update(xch, fbuf, buf.st_size);
> +
> +    if ( munmap(fbuf, buf.st_size) )
> +    {
> +        fprintf(stderr, "Could not unmap: %d(%s)\n", errno, strerror(errno));
> +        close(fd);
> +        return errno;
> +    }
> +
> +    close(fd);
> +
> +    return ret;
> +}

The content of xenmicrocode.c is looking ok now, but as indicated, I do
not believe the xc_microcode_update() call should be pausing domains.

~Andrew


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel