WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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

<Prev in Thread] Current Thread [Next in Thread>