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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

On 4 Jul 2014, at 10:59, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

> On Thu, 2014-07-03 at 16:15 +0100, David Scott wrote:
> 
> This all looks broadly sensible to me.

Thanks!

> 
>> 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.
>> 
>> Updates since v2 (thanks to Andy Cooper for the review):
> 
> Please can you put these updates after the "---" marker. This will cause
> the to be omitted from the final commit by the git am tool.
> 
>> * hvmloader: use volatile for read of closing flag
>> * style improvements
>> * remove xenstore version #defines
>> 
>> Updates since v1 (thanks to Paul Durrant for the review):
>> * remove unused client version and associated dead code
>> * treat 'closing' as a flag by using "!=0" rather than "==1"
>> * hvmloader: move function up and remove forward decl
>> * document the existing xenstore ring and the extention in misc/
>> 
>> Signed-off-by: David Scott <dave.scott@xxxxxxxxxx>
>> ---
>> docs/misc/xenstore-ring.txt         |   79 
>> +++++++++++++++++++++++++++++++++++
>> tools/firmware/hvmloader/xenbus.c   |   39 +++++++++--------
>> tools/ocaml/libs/xb/xb.ml           |   26 +++++++++++-
>> tools/ocaml/libs/xb/xb.mli          |    1 +
>> tools/ocaml/libs/xb/xs_ring.ml      |   11 +++++
>> tools/ocaml/libs/xb/xs_ring_stubs.c |   63 +++++++++++++++++++++++++++-
>> xen/include/public/io/xs_wire.h     |    5 +++
>> 7 files changed, 203 insertions(+), 21 deletions(-)
>> create mode 100644 docs/misc/xenstore-ring.txt
>> 
>> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
>> new file mode 100644
>> index 0000000..df2a09f
>> --- /dev/null
>> +++ b/docs/misc/xenstore-ring.txt
>> @@ -0,0 +1,79 @@
>> +
>> +The domain builder allocates a single page and shares it with the xenstore
>> +daemon. This document describes the ring protocol used to communicate via
>> +this page which is used to transmit and receive
>> +[xenstore protocol packets](xenstore.txt).
>> +
>> +In the original version (we call this "version 0"), the shared page has the
>> +following contents (all offsets and lengths are in octets):
>> +
>> +Offset  Length  Description
>> +-----------------------------------------------------------------
>> +0       1024    Requests: data sent to the xenstore daemon
>> +1024    1024    Replies: data sent to the domain
>> +2048    4       Request consumer offset
>> +2052    4       Request producer offset
>> +2056    4       Response consumer offset
>> +2060    4       Response producer offset
>> +
>> +When the page is allocated it is guaranteed to be full of zeroes.
>> +
>> +The "Requests" and "Replies" are treated as circular buffers, one for
>> +each direction. Each circular buffer is associated wth a producer and
>> +consumer offset, which are free-running counters starting from 0. A 
>> "producer"
>> +offset is the offset in the byte stream of the next byte to be written; a
>> +"consumer" offset is the offset in the byte stream of the next byte to be
>> +read. The byte at offset 'x' in the byte stream will be stored in
>> +offset 'x mod 1024' in the circular buffer. "producer - consumer" gives
>> +the number of bytes still to be read, and "1024 - (producer - consumer)"
>> +therefore gives the amount of space currently available for writing,
>> +where we must avoid overwriting unread data.
>> +
>> +The circular buffers are only used to send sequences of bytes between 
>> domains.
>> +It is the responsibility of the layer above to segment these bytes into
>> +packets, as described in [xenstore.txt](xenstore.txt).
>> +
>> +The client and server domains can run concurrently on different cores and
>> +different sockets. We must therefore take care to avoid corruption by:
>> +
>> +  1. using atomic loads and stores when reading and writing the producer
>> +     and consumer offsets. If an offset were to be updated by a non-atomic
>> +     store then the other domain may read an invalid offset value.
>> +  2. ensuring request and reply data is fully read or written before
>> +     updating the offsets by issuing a memory barrier.
>> +
>> +If we wish to read data but find the circular buffer empty, or wish to write
>> +data and find the circular buffer full, we wait on a pre-arranged event
>> +channel. When the other party next reads or writes data to the ring, it
>> +will notify() this event channel and we will wake.
>> +
>> +Protocol extension for reconnection
>> +-----------------------------------
>> +
>> +In version 0 of the protocol it is not possible to close and reopen the
>> +connection. This means that if the ring is corrupted, it can never be used
>> +(safely) again. Extending the protocol to allow reconnection would allow
>> +us to:
> 
> The use of past tense here is a bit odd given that this patch implements
> and enables all of this stuff.
> 
>> +
>> +  1. use the ring in the firmware (hvmloader) and safely reset it for use
>> +     by the guest
>> +  2. re-establish a ring in a 'crash kernel' environment, allowing us to
>> +     write crash dumps to PV disks or network devices.
>> +
>> +In version 1 of the protocol the ring is extended with the following
>> +fields:
>> +
>> +Offset  Length  Description
>> +-----------------------------------------------------------------
>> +2064    4       Xenstore daemon supported protocol version
>> +2068    4       Close request flag
>> +
>> +In a system supporting only version 0, both these fields are guaranteed
>> +to contain 0 by the domain builder.
>> +
>> +In a system supporting version 1, the xenstore daemon will write "1" into
>> +the support protocol version field. The guest xenstore client (eg in
>> +hvmloader) can query the version, and if it is set to "1" it can write
>> +"1" to the close request flag and call notify(). The supporting xenstore
>> +daemon can reset the ring to an empty state and signal completion by
>> +clearing the flag and calling notify() again.
> 
> I agree with Paul WRT s/calling notify()/kick the evtchn/.
> 
>> diff --git a/tools/firmware/hvmloader/xenbus.c 
>> b/tools/firmware/hvmloader/xenbus.c
>> index fe72e97..f85832c 100644
>> --- a/tools/firmware/hvmloader/xenbus.c
>> +++ b/tools/firmware/hvmloader/xenbus.c
>> @@ -37,6 +37,19 @@ 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)
>> +{
>> +    struct shared_info *shinfo = get_shared_info();
>> +    struct sched_poll poll;
>> +
>> +    memset(&poll, 0, sizeof(poll));
>> +    set_xen_guest_handle(poll.ports, &event);
>> +    poll.nr_ports = 1;
>> +
>> +    while ( !test_and_clear_bit(event, shinfo->evtchn_pending) )
>> +        hypercall_sched_op(SCHEDOP_poll, &poll);
>> +}
>> +
>> /* Connect our xenbus client to the backend.
>>  * Call once, before any other xenbus actions. */
>> void xenbus_setup(void)
>> @@ -68,10 +81,15 @@ 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 > 0) {
> 
> hvmloader is part of the Xen release and is therefore entitled to assume
> that it is running against the xenstore(s) which shipped with that
> release of Xen.
> 
> IOW you can omit this check or replace it with an assertion if you
> prefer.
> 
> (Later: OIC, I thought you'd patched the cxenstored but that was
> actually just the ocaml stubs. This is fine then)

OK

> 
>> diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
>> index 29d354d..d5cd776 100644
>> --- a/tools/ocaml/libs/xb/xb.ml
>> +++ b/tools/ocaml/libs/xb/xb.ml
> 
> I'll trust you've got all this ocaml stuff right ;-)
> 
> Is there someone else you can CC to get a pair of savvy eyes on it?

I’ve cc:d Anil and Jon who are always good at this kind of thing.

> 
>> diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c 
>> b/tools/ocaml/libs/xb/xs_ring_stubs.c
>> index 8bd1047..27c98cd 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;
>> 
>>      cons = *(volatile uint32*)&intf->req_cons;
>>      prod = *(volatile uint32*)&intf->req_prod;
>> +    closing = *(volatile uint32*)&intf->closing;
>> +
>> +    if (closing != 0)
>> +            return ERROR_CLOSING;
> 
> It's not possible to just raise the relevant exception here rather than
> propagating this to all the callers?

Hm, looking at the code in context I don’t know why both read and write have 
been decomposed into two functions each— I might as well merge them back 
together and raise the exceptions inline as you suggest.

> 
>> +CAMLprim value ml_interface_close(value interface)
>> +{
>> +    CAMLparam1(interface);
>> +    const char invalid_data[] = { 'd', 'e', 'a', 'd', 'b', 'e', 'e', 'f' };
>> +    struct xenstore_domain_interface *intf = GET_C_STRUCT(interface)->addr;
>> +    int i;
>> +
>> +    intf->req_cons = intf->req_prod = intf->rsp_cons = intf->rsp_prod = 0;
>> +    /* Ensure the unused space is full of invalid xenstore packets. */
> 
> Is filling with ones or zeroes not sufficient?
> 
> Also, is this strictly necessary of just for debugging purposes?

It’s not strictly necessary so it could be removed.

My preference i to fill the unused space within the ring buffers with obviously 
invalid packets so that if someone peeks at it at the wrong time, they get 
obviously bad data and hopefully stop immediately. If both ends are operating 
correctly then they should never notice. It’s unfortunate that all zeroes looks 
like a valid sequence of DEBUG packets. Perhaps I could fill it with ‘0xff’ and 
explicitly declare 0xff as an invalid message type xs_wire.h — that would do it.

What do you think?

Dave


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel