[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
Hi Andy, Thanks for the feedback! On 2 Jul 2014, at 13:32, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote: > 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. OK > >> /* 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. Aha, good spot. Is it sufficient to do something like: - while (rings->closing == 1) + while ( *(volatile uint32_t*)&rings->closing == 1) ring_wait (); > >> + 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. Unfortunately the code is split between the OCaml and the C functions. The C functions take care of manipulating the flags, pointers and data, while the OCaml code manages the event channel. The OCaml ‘handle_exception’ function calls ‘Xs_ring.close’ (the C stub) and then calls ‘backend.eventchn_notify’. This is the only way ‘Xs_ring.close' is called, so I believe it’s ok. > >> + } 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? Sure > >> >> /* 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. Oops > >> >> 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. Yeah those are probably a bit OTT. I’ll remove them. Cheers, Dave > > ~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 |