Re: [PATCH v3] libvchan: interdomain communications library

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

On 09/01/2011 12:28 PM, Ian Campbell wrote:
> On Wed, 2011-08-31 at 20:17 +0100, Daniel De Graaf wrote:
>>> [...]
>>>> +static int init_gnt_srv(struct libvchan *ctrl)
>>>> +{
>>>> +       int pages_left = ctrl->read.order >= PAGE_SHIFT ? 1 << (ctrl->read.order - PAGE_SHIFT) : 0;
>>>> +       int pages_right = ctrl->write.order >= PAGE_SHIFT ? 1 << (ctrl->write.order - PAGE_SHIFT) : 0;
>>>> +       struct ioctl_gntalloc_alloc_gref *gref_info = NULL;
>>>> +       int ring_fd = open("/dev/xen/gntalloc", O_RDWR);
>>>> +       int ring_ref = -1;
>>>> +       int err;
>>>> +       void *ring, *area;
>>>> +
>>>> +       if (ring_fd < 0)
>>>> +               return -1;
>>>> +
>>>> +       gref_info = malloc(sizeof(*gref_info) + max(pages_left, pages_right)*sizeof(uint32_t));
>>>> +
>>>> +       gref_info->domid = ctrl->other_domain_id;
>>>> +       gref_info->flags = GNTALLOC_FLAG_WRITABLE;
>>>> +       gref_info->count = 1;
>>>> +
>>>> +       err = ioctl(ring_fd, IOCTL_GNTALLOC_ALLOC_GREF, gref_info);
>>>
>>> Unless libvchan is going to be the only user of this interface we should
>>> add helpful wrappers to libxc, like we do for gntdev and evtchn.
>>
>> Adding the wrappers made the library more complex with no other gains when
>> it was out-of-tree; for upstreaming, this does make sense. This will result
>> in a vchan consuming two file descriptors while it is active because the libxc
>> API does not expose the ability to close descriptors without unmapping memory.
>> Since that ability is likely to be linux-specific, it's reasonable to stop
>> relying on it for portability reasons.
> 
> I'm not sure I understand (wouldn't you just add a gntalloc fd to
> libvchan and reuse it everywhere?) but if you need a new variant of a
> particular libxc interface with different semantics feel free to add it
> (or convert an existing function to it if that seems more appropriate).
 
The previous version of libvchan closed the gntalloc file descriptor during
the initialization. This is unlikely to be portable when abstracted to close
the entire gntshr interface.

Making this change has exposed an interesting ordering dependency in the
notify API under Linux: the file descriptor for gntdev or gntalloc must be
less than the file descriptor for evtchn in order for the event channel to
still be active when the unmap occurs on a crash. The init functions of
libvchan do open the files in the proper order for this to happen.

>>>> +#ifdef IOCTL_GNTALLOC_SET_UNMAP_NOTIFY
>>>> +       {
>>>> +               struct ioctl_gntalloc_unmap_notify arg;
>>>> +               arg.index = gref_info->index + offsetof(struct vchan_interface, srv_live);
>>>> +               arg.action = UNMAP_NOTIFY_CLEAR_BYTE | UNMAP_NOTIFY_SEND_EVENT;
>>>> +               arg.event_channel_port = ctrl->event_port;
>>>> +               ioctl(ring_fd, IOCTL_GNTALLOC_SET_UNMAP_NOTIFY, &arg);
>>>> +       }
>>>> +#endif
>>>
>>> What is the fallback if this isn't available?
>>
>> The fallback is that the notify is not sent, and the peer cannot detect when
>> its peer crashes or is killed instead of executing a graceful shutdown.
>>
>> Adding this functionality to libxc requires yet another wrapper on the grant
>> mapping functionality. Instead of attempting to pass back the index as is
>> done in the current version, I am considering adding the functions
>> xc_gnttab_map_grant_ref_notify(xcg, domid, ref, notify_offset, notify_port) and
>> xc_gntshr_share_page_notify(xcs, domid, &ref, notify_offset, notify_port);
>> these would fall back to xc_gnttab_map_grant_ref if notify is not present.
> 
> You can't just add the xc_gnttab_notify() as a function which just
> registers the notify and use xc_gnttab_map_grant_ref + that new
> function?

This is possible, but you would need to pass back the index used to mmap
or keep metadata within the file descriptor to allow this to be determined.
Since the current xc_* mapping interfaces do not expose this index, it would
require a larger change to expose this mostly-useless index just for the
purpose of passing it to the notify call.

>>> [...]
>>>> static int init_xs_srv(struct libvchan *ctrl, int ring_ref)
>>>> +{
>>>> +       int ret = -1;
>>>> +       struct xs_handle *xs;
>>>> +       struct xs_permissions perms[2];
>>>> +       char buf[64];
>>>> +       char ref[16];
>>>> +       char* domid_str = NULL;
>>>> +       xs = xs_domain_open();
>>>> +       if (!xs)
>>>> +               goto fail;
>>>> +       domid_str = xs_read(xs, 0, "domid", NULL);
>>>> +       if (!domid_str)
>>>> +               goto fail_xs_open;
>>>> +
>>>> +       // owner domain is us
>>>> +       perms[0].id = atoi(domid_str);
>>>
>>> It sucks a bit that xenstore doesn't appear to allow DOMNID_SELF here
>>> but oh well.
>>
>> On the client side, we need to look up our own domid to find the path
>> (if the changes to follow usual xenstore convention are made) so it's
>> required either way.
> 
> How do you mean? relative xenstore accesses are relative
> to /local/domain/<domid> so you shouldn't need to know domid to access
> e.g. /local/domain/<domid>/foo/bar -- just access foo/bar instead.
> 

Yes, but the client doesn't use a path relative to its own domid. It uses
/local/domain/<server-id>/data/vchan/<client-id>/<vchan-id>/...

Devices work around this problem by having xl or xm fill in paths under
both /local/domain/<client-id> and /local/domain/<server-id> pointing to
each other; using this style of path is not possible without some side
knowing its own domain ID.

Is reading "domid" the best method for determining the domain ID of the
local domain? I noticed in testing that it may need to be set for dom0
if only the xl tools are used in domain creation.

>>>> +       // permissions for domains not listed = none
>>>> +       perms[0].perms = XS_PERM_NONE;
>>>> +       // other domains
>>>> +       perms[1].id = ctrl->other_domain_id;
>>>> +       perms[1].perms = XS_PERM_READ;
>>>> +
>>>> +       snprintf(ref, sizeof ref, "%d", ring_ref);
>>>> +       snprintf(buf, sizeof buf, "data/vchan/%d/ring-ref", ctrl->device_number);
>>>> +       if (!xs_write(xs, 0, buf, ref, strlen(ref)))
>>>> +               goto fail_xs_open;
>>>> +       if (!xs_set_permissions(xs, 0, buf, perms, 2))
>>>> +               goto fail_xs_open;
>>>> +
>>>> +       snprintf(ref, sizeof ref, "%d", ctrl->event_port);
>>>> +       snprintf(buf, sizeof buf, "data/vchan/%d/event-channel", ctrl->device_number);
>>>> +       if (!xs_write(xs, 0, buf, ref, strlen(ref)))
>>>> +               goto fail_xs_open;
>>>> +       if (!xs_set_permissions(xs, 0, buf, perms, 2))
>>>> +               goto fail_xs_open;
>>>
>>> Am I right that the intended usage model is that two domains can decide
>>> to setup a connection without admin or toolstack involvement?
>>>
>>> Do we need to arrange on the toolstack side that a suitable
>>> vchan-specific directory (or directories) in xenstore exists with
>>> suitable permissions to allow this to happen exists or do we think data
>>> is an appropriate location? 
>>
>> Yes, the intended use is to avoid needing to have management tools involved
>> in the setup. Of course, that doesn't mean that vchan can't have help from
>> management tools - but since this help isn't required, adding an unneeded
>> dependency was pointless and might also imply a level of control that is not
>> actually present (i.e. restricting the management tools does not actually
>> restrict the ability to set up a vchan; that requires something like an XSM
>> policy blocking the grant or event channels). I picked data because it does
>> not require toolstack modification to use, and no other location jumped out
>> at me - vchan isn't really a device.
> 
> OK. I'm a bit fearful that data may become a bit of a dumping ground
> (I'm not sure what its intended use/semantics actually are) but that's
> not the fault of this patch.
> 
> Ian.
> 

-- 
Daniel De Graaf
National Security Agency