Re: [PATCH 1/3] libxc: add xc_gnttab_map_grant_ref_notify

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Sep 01, 2011 at 12:22:16PM -0400, Daniel De Graaf wrote:
> Normally, when a userspace process mapping a grant crashes, the domain
> providing the reference receives no indication that its peer has
> crashed, possibly leading to unexpected freezes or timeouts. This
> function provides a notification of the unmap by signalling an event
> channel and/or clearing a specific byte in the page.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  tools/libxc/xc_gnttab.c      |   15 ++++++++++++
>  tools/libxc/xc_linux_osdep.c |   52 ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h        |   21 +++++++++++++++++
>  tools/libxc/xenctrlosdep.h   |    5 ++++
>  4 files changed, 93 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxc/xc_gnttab.c b/tools/libxc/xc_gnttab.c
> index 4f55fce..dc7aa0c 100644
> --- a/tools/libxc/xc_gnttab.c
> +++ b/tools/libxc/xc_gnttab.c
> @@ -174,6 +174,21 @@ 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)
> +{
> +	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);
> +	else
> +		return xcg->ops->u.gnttab.map_grant_ref(xcg, xcg->ops_handle,
> +			domid, ref, PROT_READ|PROT_WRITE);
> +}
> +
> +
>  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..8f7718f 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -613,6 +613,57 @@ 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 fd = (int)h;
> +    struct ioctl_gntdev_map_grant_ref map;
> +	struct ioctl_gntdev_unmap_notify notify;

That looks a bit odd. Like the formatting is off?

> +    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;
> +    }
> +
> +    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 && ioctl(fd, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, &notify)) {
> +        PERROR("linux_gnttab_map_grant_ref_notify: ioctl SET_UNMAP_NOTIFY failed");

Perhaps reporting via an argument that we failed at doing the notify would
be useful? That way at least you know you need to do polling.

> +    }
> +
> +    return addr;
> +}
> +
> +
>  static int linux_gnttab_munmap(xc_gnttab *xcg, xc_osdep_handle h,
>                                 void *start_address, uint32_t count)
>  {
> @@ -662,6 +713,7 @@ static struct xc_osdep_ops linux_gnttab_ops = {
>          .map_grant_ref = &linux_gnttab_map_grant_ref,
>          .map_grant_refs = &linux_gnttab_map_grant_refs,
>          .map_domain_grant_refs = &linux_gnttab_map_domain_grant_refs,
> +	.map_grant_ref_notify = &linux_gnttab_map_grant_ref_notify,
>          .munmap = &linux_gnttab_munmap,
>      },
>  };
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 1b82ee0..7859571 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1349,6 +1349,27 @@ void *xc_gnttab_map_domain_grant_refs(xc_gnttab *xcg,
>                                        int prot);
>  
>  /*
> + * Memory maps a grant reference from one domain to a local address range.
> + * Mappings should be unmapped with xc_gnttab_munmap.  Logs errors.
                                                          ^^^^^^^^^^^^ .. that looks odd?

> + * This version always maps writable pages, and will attempt to set up
> + * an unmap notification at the given offset and event channel. When the
> + * page is unmapped, the byte at the given offset will be zeroed and a
> + * wakeup will be sent to the given event channel.
> + *
> + * @parm xcg a handle on an open grant table interface
> + * @parm domid the domain to map memory from
> + * @parm ref the grant reference ID to map
> + * @parm notify_offset The byte offset in the page to use for unmap
> + *                     notification; -1 for none.
> + * @parm notify_port The event channel port to use for unmap notify, or -1
> + */
> +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);
> +
> +/*
>   * Unmaps the @count pages starting at @start_address, which were mapped by a
>   * call to xc_gnttab_map_grant_ref or xc_gnttab_map_grant_refs. Never logs.
>   */
> diff --git a/tools/libxc/xenctrlosdep.h b/tools/libxc/xenctrlosdep.h
> index bfe46e0..01969c5 100644
> --- a/tools/libxc/xenctrlosdep.h
> +++ b/tools/libxc/xenctrlosdep.h
> @@ -119,6 +119,11 @@ struct xc_osdep_ops
>                                             uint32_t domid,
>                                             uint32_t *refs,
>                                             int prot);
> +            void *(*map_grant_ref_notify)(xc_gnttab *xcg, xc_osdep_handle h,
> +                                          uint32_t domid,
> +                                          uint32_t ref,
> +                                          uint32_t notify_offset,
> +                                          evtchn_port_t notify_port);
>              int (*munmap)(xc_gnttab *xcg, xc_osdep_handle h,
>                            void *start_address,
>                            uint32_t count);
> -- 
> 1.7.6