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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/29/2015 08:14 PM, 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.
>
>   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);

You are not handling errors returned by xc_domain_pause(). So then you 
will try to unpause a domain that may not have been paused and (I think 
more importantly) may proceed with microcode update while not all 
domains are paused.

> +
> +       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]);

Same here --- you may fail unpausing a domain and noone will know about it.


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

Not sure what tools coding convention is but you may not need {} here 
(and a few more places)

> +
> +    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
>   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/ */

On Linux but not necessarily on other OSs. You code doesn't require it 
to be there so you probably want to avoid referring to this path in 
comments and commit subject/body.

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

Do you need this? (probably leftover from debugging?)


-boris

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

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