[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 02/07/14 17:47, Dave Scott wrote: > 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 (); Yeah - that looks better (albeit with the leading space removed). > >>> + 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. Ok. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |