From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [RFC v2] misc/xenmicrocode: Upload /lib/firmware/ to the hypervisor Date: Fri, 30 Jan 2015 09:44:56 -0500 Message-ID: <54CB98E8.2060701@oracle.com> References: <1422580462-26324-1-git-send-email-mcgrof@do-not-panic.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YHCp6-00039f-0J for xen-devel@lists.xenproject.org; Fri, 30 Jan 2015 14:45:28 +0000 In-Reply-To: <1422580462-26324-1-git-send-email-mcgrof@do-not-panic.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Luis R. Rodriguez" , stefano.stabellini@eu.citrix.com Cc: Juergen Gross , Michal Marek , Jason Douglas , Takashi Iwai , Andrew Cooper , mcgrof@suse.com, Henrique de Moraes Holschuh , david.vrabel@citrix.com, Jan Beulich , xen-devel@lists.xenproject.org, Borislav Petkov , Olaf Hering List-Id: xen-devel@lists.xenproject.org On 01/29/2015 08:14 PM, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > 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 > Cc: Konrad Rzeszutek Wilk > Cc: Andrew Cooper > Cc: Borislav Petkov > Cc: Takashi Iwai > Cc: Olaf Hering > Cc: Jan Beulich > Cc: Jason Douglas > Cc: Juergen Gross > Cc: Michal Marek > Cc: Henrique de Moraes Holschuh > Signed-off-by: Konrad Rzeszutek Wilk > Signed-off-by: Luis R. Rodriguez > --- > > 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 > #include > #include > +#include > #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 > +#include > > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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 \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; > +} >