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

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

On 02/07/14 17:47, Dave Scott wrote:
> Hi Andy,
>
> Thanks for the feedback!
>
> On 2 Jul 2014, at 13:32, Andrew Cooper <andrew.cooper3@citrix.com> 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@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.
> 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 ();

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

Ok.

~Andrew