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 17:52:57 +0100 Message-ID: <53B438E9.90409@citrix.com> References: <9AAE0902D5BC7E449B7C8E4E778ABCD03B4E3E@AMSPEX01CL01.citrite.net> <1403730912-4964-1-git-send-email-dave.scott@citrix.com> <53B3FBD2.8080900@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1X2NmI-0001Mc-JT for xen-devel@lists.xenproject.org; Wed, 02 Jul 2014 16:53:05 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dave Scott Cc: "xen-devel@lists.xenproject.org" , Paul Durrant , Ian Campbell , "yanqiangjun@huawei.com" , "john.liuqiming@huawei.com" List-Id: xen-devel@lists.xenproject.org On 02/07/14 17:47, Dave Scott wrote: > Hi Andy, > > Thanks for the feedback! > > On 2 Jul 2014, at 13:32, Andrew Cooper 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 >> 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/hvmload= er/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; /* Sh= ared ring with dom0 */ >>> static evtchn_port_t event; /* Event-channel to dom= 0 */ >>> 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 !=3D 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 ne= ver = >>> - * having used the rings. */ >>> - memset(rings, 0, sizeof *rings); >>> + if (rings->server_version > XENSTORE_VERSION_0) { >>> + rings->closing =3D 1; >>> + while (rings->closing =3D=3D 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 =3D=3D 1) > + while ( *(volatile uint32_t*)&rings->closing =3D=3D 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. Th= e C functions take care of manipulating the flags, pointers and data, while= the OCaml code manages the event channel. The OCaml =91handle_exception=92= function calls =91Xs_ring.close=92 (the C stub) and then calls =91backend.= eventchn_notify=92. This is the only way =91Xs_ring.close' is called, so I = believe it=92s ok. Ok. ~Andrew