[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC] RFC: extend the xenstore ring with a 'closing' signal
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 <dave.scott@xxxxxxxxxx> 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. */ _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |