Re: [Xen-devel] [PATCH RFC] RFC: extend the xenstore ring with a 'closing' signal

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

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