[Xen-devel] Re: [PATCH v3] libvchan: interdomain communications library

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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

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

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


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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel