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