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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

David Scott writes ("[Xen-devel] [PATCH v2] RFC: extend the xenstore ring with 
a 'closing' signal"):
> 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.

This seems like a sensible extension in principle.  I have some
comments.  I'll concentrate on the protocol specification:

Firstly, thanks a lot for actually writing this down properly.

> 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).

This document should explain things mainly from the point of view of
the guest.  The rest are internal implementation details (which might
be moved into comments in the implementation).

> +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.

For example, this belongs somewhere else.  Guests should not assume
that the page starts out full of zeroes.

Instead, you should say something like:

  When the guest starts, the ring is in a valid state with an empty
  queue in each direction.  (I.e., each consumer offset is the same as
  the corresponding producer offset.)

> +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.

It is surely not necessary to specify that these counters start at 0.
Both ends need to cope with wraparound anyway.

On the other hand if old guests have assumed that the counters start
at 0 then that should be documented.  Have they, do we know ?

> +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:

I generally favour feature bits rather than version numbers.  This
makes handling divergence and feature obsolecence much easier.  There
is plenty of room in this page for one bit per feature.

(Also, using feature bits makes it clearer what the intended semantics
are.  It is implicit in your design that an implementation which seems
a later version number advertised by its peer should not refuse to
operate, but you didn't actually say so.  The corresponding statement
for feature bits is simpler: "unknown feature bits should be
ignored".)

> +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.

Implicitly there is a rule here that the daemon may only write to this
field if it is currently nonzero, and that the client may only write
to it if it is currently zero.  This (or something like it) should be
stated explicitly.

You should also say whether this has any effect on the higher
layers of the xenstored protocol.  Does a ring reset have the effect
of a RESET_WATCHES call ?  What happens to oustanding commands and
responses ?

> index 585f0c8..2c934ca 100644
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -108,14 +108,22 @@ enum xs_watch_type
>   * `incontents 150 xenstore_struct XenStore wire protocol.
>   *
>   * Inter-domain shared memory communications. */

This should gain a reference to your new document.

Thanks,
Ian.

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