[Xen-devel] Re: [PATCH 1/3] libxc: add xc_gnttab_map_grant_ref_notify

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2011-09-19 at 23:43 +0100, Daniel De Graaf wrote:
> @@ -116,4 +116,35 @@ struct ioctl_gntdev_set_max_grants {
>       uint32_t count;
>  };
>  
> +/*
> + * Sets up an unmap notification within the page, so that the other side can 
> do
> + * cleanup if this side crashes. Required to implement cross-domain robust
> + * mutexes or close notification on communication channels.
> + *
> + * Each mapped page only supports one notification; multiple calls referring 
> to
> + * the same page overwrite the previous notification. You must clear the
> + * notification prior to the IOCTL_GNTALLOC_DEALLOC_GREF if you do not want 
> it
> + * to occur.
> + */
> +#define IOCTL_GNTDEV_SET_UNMAP_NOTIFY \
> +_IOC(_IOC_NONE, 'G', 7, sizeof(struct ioctl_gntdev_unmap_notify))
> +struct ioctl_gntdev_unmap_notify {
> +     /* IN parameters */
> +     /* Offset in the file descriptor for a byte within the page (same as
> +      * used in mmap).

I'm probably being thick but I don't understand what this means, i.e.
what this thing is relative to.

>  If using UNMAP_NOTIFY_CLEAR_BYTE, this is the byte to
> +      * be cleared. Otherwise, it can be any byte in the page whose
> +      * notification we are adjusting.
> +      */
> +     uint64_t index;
> +     /* Action(s) to take on unmap */
> +     uint32_t action;
> +     /* Event channel to notify */
> +     uint32_t event_channel_port;

evtchn_port_t ?

> +};
> +
> +/* Clear (set to zero) the byte specified by index */
> +#define UNMAP_NOTIFY_CLEAR_BYTE 0x1
> +/* Send an interrupt on the indicated event channel */
> +#define UNMAP_NOTIFY_SEND_EVENT 0x2
> +
>  #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
> diff --git a/tools/libxc/xc_gnttab.c b/tools/libxc/xc_gnttab.c
> index 4f55fce..3d3c63b 100644
> --- a/tools/libxc/xc_gnttab.c
> +++ b/tools/libxc/xc_gnttab.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "xc_private.h"
> +#include <errno.h>
>  
>  int xc_gnttab_op(xc_interface *xch, int cmd, void * op, int op_size, int 
> count)
>  {
> @@ -174,6 +175,28 @@ void *xc_gnttab_map_domain_grant_refs(xc_gnttab *xcg,
>                                                       count, domid, refs, 
> prot);
>  }
>  
> +void *xc_gnttab_map_grant_ref_notify(xc_gnttab *xcg,
> +                                     uint32_t domid,
> +                                     uint32_t ref,
> +                                     uint32_t notify_offset,
> +                                     evtchn_port_t notify_port,
> +                                     int *notify_result)
> +{
> +     if (xcg->ops->u.gnttab.map_grant_ref_notify)
> +             return xcg->ops->u.gnttab.map_grant_ref_notify(xcg, 
> xcg->ops_handle,
> +                     domid, ref, notify_offset, notify_port, notify_result);
> +     else {
> +             void* area = xcg->ops->u.gnttab.map_grant_ref(xcg, 
> xcg->ops_handle,
> +                     domid, ref, PROT_READ|PROT_WRITE);
> +             if (area && notify_result) {
> +                     *notify_result = -1;
> +                     errno = ENOSYS;
> +             }
> +             return area;
> +     }

I think the new public interface is fine but do we really need a new
internal interface here?

I think you can just add the notify_* arguments to the existing OSDEP
function and have those OS backends which don't implement that feature
return ENOSYS if notify_offset != 0 (or ~0 or whatever invalid value
works).

Why doesn't the *_notify variant take a prot argument?

I'd be tempted to do away with notify_result too -- if the caller asked
for notification and we fail to give that then we can cleanup and return
an error. If they want to try again without the notification then that's
up to them.

> +}
> +
> +
>  int xc_gnttab_munmap(xc_gnttab *xcg,
>                       void *start_address,
>                       uint32_t count)
> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> index dca6718..3040cb6 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -613,6 +613,62 @@ static void 
> *linux_gnttab_map_domain_grant_refs(xc_gnttab *xcg, xc_osdep_handle
>      return do_gnttab_map_grant_refs(xcg, h, count, &domid, 0, refs, prot);
>  }
>  
> +static void *linux_gnttab_map_grant_ref_notify(xc_gnttab *xch, 
> xc_osdep_handle h,
> +                                               uint32_t domid, uint32_t ref,
> +                                               uint32_t notify_offset,
> +                                               evtchn_port_t notify_port,
> +                                               int *notify_result)
> +{
> +    int fd = (int)h;
> +    int rv = 0;
> +    struct ioctl_gntdev_map_grant_ref map;
> +    struct ioctl_gntdev_unmap_notify notify;
> +    void *addr;
> +
> +    map.count = 1;
> +    map.refs[0].domid = domid;
> +    map.refs[0].ref = ref;
> +
> +    if ( ioctl(fd, IOCTL_GNTDEV_MAP_GRANT_REF, &map) ) {
> +        PERROR("xc_gnttab_map_grant_ref: ioctl MAP_GRANT_REF failed");
> +        return NULL;
> +    }
> +
> +    addr = mmap(NULL, XC_PAGE_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 
> map.index);
> +    if ( addr == MAP_FAILED )
> +    {
> +        int saved_errno = errno;
> +        struct ioctl_gntdev_unmap_grant_ref unmap_grant;
> +
> +        PERROR("xc_gnttab_map_grant_ref: mmap failed");
> +        unmap_grant.index = map.index;
> +        unmap_grant.count = 1;
> +        ioctl(fd, IOCTL_GNTDEV_UNMAP_GRANT_REF, &unmap_grant);
> +        errno = saved_errno;
> +        return NULL;
> +    }

The non-notify variant handles EAGAIN, why doesn't this one need to do
so?

> +
> +    notify.index = map.index;
> +    notify.action = 0;
> +    if (notify_offset >= 0) {
> +        notify.index += notify_offset;
> +        notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
> +    }
> +    if (notify_port >= 0) {
> +        notify.event_channel_port = notify_port;
> +        notify.action |= UNMAP_NOTIFY_SEND_EVENT;
> +    }
> +    if (notify.action)
> +        rv = ioctl(fd, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, &notify);

Is there a race if the other end (or this process) dies between the MAP
ioctl and here?

Ian.


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