From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC] RFC: extend the xenstore ring with a 'closing' signal Date: Wed, 2 Jul 2014 13:32:18 +0100 Message-ID: <53B3FBD2.8080900@citrix.com> References: <9AAE0902D5BC7E449B7C8E4E778ABCD03B4E3E@AMSPEX01CL01.citrite.net> <1403730912-4964-1-git-send-email-dave.scott@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1X2Ji5-0004is-OJ for xen-devel@lists.xenproject.org; Wed, 02 Jul 2014 12:32:25 +0000 In-Reply-To: <1403730912-4964-1-git-send-email-dave.scott@citrix.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: David Scott , xen-devel@lists.xenproject.org Cc: john.liuqiming@huawei.com, paul.durrant@citrix.com, ian.campbell@citrix.com, yanqiangjun@huawei.com List-Id: xen-devel@lists.xenproject.org On 25/06/14 22:15, David Scott wrote: > Currently hvmloader uses the xenstore ring and then tries to > reset it back to its initial state. This is not part of the > ring protocol and, if xenstored reads the ring while it is > happening, xenstored will conclude it is corrupted. A corrupted > ring will prevent PV drivers from connecting. This seems to > be a rare failure. > > Furthermore, when a VM crashes it may jump to a 'crash kernel' > to create a diagnostic dump. Without the ability to safely > reset the ring the PV drivers won't be able to reliably > establish connections, to (for example) stream a memory dump to > disk. > > This prototype patch contains a simple extension of the > xenstore ring structure, enough to contain version numbers and > a 'closing' flag. This memory is currently unused within the > 4k page and should be zeroed as part of the domain setup > process. The oxenstored server advertises version 1, and > hvmloader detects this and sets the 'closing' flag. The server > then reinitialises the ring, filling it with obviously invalid > data to help debugging (unfortunately blocks of zeroes are > valid xenstore packets) and signals hvmloader by the event > channel that it's safe to boot the guest OS. > > This patch has only been lightly tested. I'd appreciate > feedback on the approach before going further. > > Signed-off-by: David Scott The plan of some version information looks plausible. Some comments below (for the non-ocaml bits). > --- > tools/firmware/hvmloader/xenbus.c | 16 +++++-- > tools/ocaml/libs/xb/xb.ml | 26 ++++++++++- > tools/ocaml/libs/xb/xb.mli | 3 +- > tools/ocaml/libs/xb/xs_ring.ml | 13 ++++++ > tools/ocaml/libs/xb/xs_ring_stubs.c | 81 ++++++++++++++++++++++++++++++++++- > xen/include/public/io/xs_wire.h | 8 ++++ > 6 files changed, 138 insertions(+), 9 deletions(-) > > diff --git a/tools/firmware/hvmloader/xenbus.c b/tools/firmware/hvmloader/xenbus.c > index fe72e97..15d961b 100644 > --- a/tools/firmware/hvmloader/xenbus.c > +++ b/tools/firmware/hvmloader/xenbus.c > @@ -37,6 +37,8 @@ static struct xenstore_domain_interface *rings; /* Shared ring with dom0 */ > static evtchn_port_t event; /* Event-channel to dom0 */ > static char payload[XENSTORE_PAYLOAD_MAX + 1]; /* Unmarshalling area */ > > +static void ring_wait(void); > + Move ring_wait() up, or xenbus_shutdown() down. > /* Connect our xenbus client to the backend. > * Call once, before any other xenbus actions. */ > void xenbus_setup(void) > @@ -68,10 +70,16 @@ void xenbus_shutdown(void) > > ASSERT(rings != NULL); > > - /* We zero out the whole ring -- the backend can handle this, and it's > - * not going to surprise any frontends since it's equivalent to never > - * having used the rings. */ > - memset(rings, 0, sizeof *rings); > + if (rings->server_version > XENSTORE_VERSION_0) { > + rings->closing = 1; > + while (rings->closing == 1) This must be a volatile read of rings->closing, or the compiler is free to optimise this to an infinite loop. > + ring_wait (); Are we guarenteed to receive an event on the xenbus evtchn after the server has cleared rings->closing? I can't see anything in the implementation which would do this. > + } else { > + /* If the backend reads the state while we're erasing it then the > + ring state will become corrupted, preventing guest frontends from > + connecting. This is rare. */ > + memset(rings, 0, sizeof *rings); > + } Brackets optional per Xen style. Could you keep the left-hand column of *'s with the comment? > > /* Clear the event-channel state too. */ > memset(shinfo->vcpu_info, 0, sizeof(shinfo->vcpu_info)); > > diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c > index 8bd1047..4ddf5a7 100644 > --- a/tools/ocaml/libs/xb/xs_ring_stubs.c > +++ b/tools/ocaml/libs/xb/xs_ring_stubs.c > @@ -35,19 +35,28 @@ > > #define GET_C_STRUCT(a) ((struct mmap_interface *) a) > > +#define ERROR_UNKNOWN (-1) > +#define ERROR_CLOSING (-2) > + > static int xs_ring_read(struct mmap_interface *interface, > char *buffer, int len) > { > struct xenstore_domain_interface *intf = interface->addr; > XENSTORE_RING_IDX cons, prod; /* offsets only */ > int to_read; > + uint32_t closing; Spaces in a tabbed file. > > diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h > index 585f0c8..68460cc 100644 > --- a/xen/include/public/io/xs_wire.h > +++ b/xen/include/public/io/xs_wire.h > @@ -104,6 +104,9 @@ enum xs_watch_type > XS_WATCH_TOKEN > }; > > +#define XENSTORE_VERSION_0 0 > +#define XENSTORE_VERSION_1 1 > + Do we really need mnemonics for these? This looks rather peculiar. ~Andrew > /* > * `incontents 150 xenstore_struct XenStore wire protocol. > * > @@ -112,10 +115,15 @@ enum xs_watch_type > typedef uint32_t XENSTORE_RING_IDX; > #define MASK_XENSTORE_IDX(idx) ((idx) & (XENSTORE_RING_SIZE-1)) > struct xenstore_domain_interface { > + /* XENSTORE_VERSION_0 */ > char req[XENSTORE_RING_SIZE]; /* Requests to xenstore daemon. */ > char rsp[XENSTORE_RING_SIZE]; /* Replies and async watch events. */ > XENSTORE_RING_IDX req_cons, req_prod; > XENSTORE_RING_IDX rsp_cons, rsp_prod; > + uint32_t client_version; > + uint32_t server_version; > + /* XENSTORE_VERSION_1 */ > + uint32_t closing; > }; > > /* Violating this is very bad. See docs/misc/xenstore.txt. */