Re: [PATCH] libvchan: interdomain communications library

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 08/22/2011 05:15 AM, Ian Campbell wrote:
> Hi Daniel,
> 
> On Fri, 2011-08-19 at 15:38 +0100, Daniel De Graaf wrote:
> 
> Please could you say a few words about the functionality this new
> library enables and perhaps the design etc? In particular a protocol
> spec would be useful for anyone who wanted to reimplement for another
> guest OS etc. In particular memory barriers are conspicuous in their
> absence.

Agreed, barriers are needed for a robust implementation. Since this is
a shared library and not xen or kernel code, the existing barrier()/wmb()
functions are not available. The only existing implementations in Xen
appear to expand to asm("":::"memory") which does not actually implement
a memory barrier (i.e. MFENCE or similar opcodes). The __sync_synchronize()
compiler intrinsic properly implements these barriers and has been used.

The lack of memory fencing in xen seems to be a topic for another thread.

> I didn't review in detail but it seems like you have invented your own
> ring datastructures rather than using the standard ring.h ones. I think
> this needs some justification

(this has been added as a comment)

The ring.h macros define an asymmetric interface to a shared data structure
that assumes all rings reside in a single contiguous memory space. This is
not suitable for vchan because the interface to the ring is symmetric except
for the setup. Unlike the producer-consumer rings defined in ring.h, the
size of the rings used in vchan are determined at execution time instead of
compile time, so the macros in ring.h cannot be used to access the rings.

> I think it would be appropriate to add protocol.txt at the same time as
> checking in the library.

The comments in the shared header file explain the layout of the shared
memory regions; any other parts of the protocol are application-defined.
 
> In one of the headers it says "The grant reference is expected to be
> shared through some out-of-band mechanism such as XenStore." but the
> library appears to implement the xenstore stuff internally.

Comment has been updated to point to the xenstore path used.

>>  tools/libvchan/gntalloc.h    |   82 ++++++++
>>  tools/libvchan/gntdev.h      |  150 ++++++++++++++
> 
> If these define a Linux ioctl interface then they should go in
> tools/include/xen-sys/Linux/. 
>
>>  tools/libvchan/libvchan.h    |  141 +++++++++++++
> 
> I presume that somewhere in there a shared datastructure is defined. In
> which case that might be better off added to xen/include/public/io/
> instead.
> 
> Cheers,
> Ian.
> 
> 

I'm sending an updated patch with these revisions in a separate email.

-- 
Daniel De Graaf
National Security Agency