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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@citrix.com>

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.

>  /* 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.

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

> +    } 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?

>  
>      /* 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.

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

~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. */