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 6/6] xen/gntalloc, gntdev: Add unmap notify ioctl

To: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 6/6] xen/gntalloc, gntdev: Add unmap notify ioctl
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Thu, 27 Jan 2011 14:20:38 -0500
Cc: jeremy@xxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, Ian.Campbell@xxxxxxxxxx
Delivery-date: Thu, 27 Jan 2011 11:21:52 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1295625548-22069-7-git-send-email-dgdegra@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1295625548-22069-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1295625548-22069-7-git-send-email-dgdegra@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Jan 21, 2011 at 10:59:08AM -0500, Daniel De Graaf wrote:
> This ioctl allows the users of a shared page to be notified when
> the other end exits abnormally.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
>  drivers/xen/gntalloc.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/xen/gntdev.c   |   56 
> +++++++++++++++++++++++++++++++++++++++++++++++-
>  include/xen/gntalloc.h |   28 ++++++++++++++++++++++++
>  include/xen/gntdev.h   |   27 +++++++++++++++++++++++
>  4 files changed, 165 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index a230dc4..8ac0dfc 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -60,11 +60,13 @@
>  #include <linux/uaccess.h>
>  #include <linux/types.h>
>  #include <linux/list.h>
> +#include <linux/highmem.h>
>  
>  #include <xen/xen.h>
>  #include <xen/page.h>
>  #include <xen/grant_table.h>
>  #include <xen/gntalloc.h>
> +#include <xen/events.h>
>  
>  static int limit = 1024;
>  module_param(limit, int, 0644);
> @@ -83,6 +85,9 @@ struct gntalloc_gref {
>       uint64_t file_index;         /* File offset for mmap() */
>       unsigned int users;          /* Use count - when zero, waiting on Xen */
>       grant_ref_t gref_id;         /* The grant reference number */
> +     unsigned notify_flags:2;     /* Unmap notification flags */

Can you add to the comment: bits 0-1
> +     unsigned notify_pgoff:12;    /* Offset of the byte to clear */
                                              ^^ in the page
And "bits 2-14.".


> +     int notify_event;            /* Port (event channel) to notify */
>  };
>  
>  struct gntalloc_file_private_data {
> @@ -169,6 +174,16 @@ undo:
>  
>  static void __del_gref(struct gntalloc_gref *gref)
>  {
> +     if (gref->notify_flags & UNMAP_NOTIFY_CLEAR_BYTE) {
> +             uint8_t *tmp = kmap(gref->page);
> +             tmp[gref->notify_pgoff] = 0;
> +             kunmap(gref->page);
> +     }
> +     if (gref->notify_flags & UNMAP_NOTIFY_SEND_EVENT)
> +             notify_remote_via_evtchn(gref->notify_event);
> +
> +     gref->notify_flags = 0;
> +
>       if (gnttab_query_foreign_access(gref->gref_id))
>               return;
>  
> @@ -339,6 +354,43 @@ dealloc_grant_out:
>       return rc;
>  }
>  
> +static long gntalloc_ioctl_notify(struct gntalloc_file_private_data *priv,
> +             void __user *arg)

Call it '..ioctl_unmap_notify'. When I read it first time I thought this
would do just notify.. while in actuality it can do both.

> +{
> +     struct ioctl_gntalloc_unmap_notify op;
> +     struct gntalloc_gref *gref;
> +     uint64_t index;
> +     int pgoff;
> +     int rc;
> +
> +     if (copy_from_user(&op, arg, sizeof(op)))
> +             return -EFAULT;
> +
> +     index = op.index & ~(PAGE_SIZE - 1);
> +     pgoff = op.index & (PAGE_SIZE - 1);

You don't want to tell the user if the messed up? Say 0xdeadf00d?
> +
> +     spin_lock(&gref_lock);
> +
> +     gref = find_grefs(priv, index, 1);
> +     if (!gref) {
> +             rc = -ENOENT;
> +             goto unlock_out;
> +     }
> +
> +     if (op.action & ~(UNMAP_NOTIFY_CLEAR_BYTE|UNMAP_NOTIFY_SEND_EVENT)) {
> +             rc = -EINVAL;
> +             goto unlock_out;
> +     }
> +
> +     gref->notify_flags = op.action;
> +     gref->notify_pgoff = pgoff;
> +     gref->notify_event = op.event_channel_port;
> +     rc = 0;
> + unlock_out:
> +     spin_unlock(&gref_lock);
> +     return rc;
> +}
> +
>  static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
>               unsigned long arg)
>  {
> @@ -351,6 +403,9 @@ static long gntalloc_ioctl(struct file *filp, unsigned 
> int cmd,
>       case IOCTL_GNTALLOC_DEALLOC_GREF:
>               return gntalloc_ioctl_dealloc(priv, (void __user *)arg);
>  
> +     case IOCTL_GNTALLOC_SET_UNMAP_NOTIFY:
> +             return gntalloc_ioctl_notify(priv, (void __user *)arg);
> +
>       default:
>               return -ENOIOCTLCMD;
>       }
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index a5710e8..e360d0a 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -37,6 +37,7 @@
>  #include <xen/xen.h>
>  #include <xen/grant_table.h>
>  #include <xen/gntdev.h>
> +#include <xen/events.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/page.h>
> @@ -70,6 +71,9 @@ struct grant_map {
>       int count;
>       int flags;
>       int is_mapped;
> +     int notify_flags;
> +     int notify_offset;
> +     int notify_event;

Would it make sense to put these inside a struct?

>       atomic_t users;
>       struct ioctl_gntdev_grant_ref *grants;
>       struct gnttab_map_grant_ref   *map_ops;
> @@ -165,7 +169,7 @@ static struct grant_map *gntdev_find_map_index(struct 
> gntdev_priv *priv,
>       list_for_each_entry(map, &priv->maps, next) {
>               if (map->index != index)
>                       continue;
> -             if (map->count != count)
> +             if (count && map->count != count)
>                       continue;
>               return map;
>       }
> @@ -184,6 +188,10 @@ static void gntdev_put_map(struct grant_map *map)
>  
>       atomic_sub(map->count, &pages_mapped);
>  
> +     if (map->notify_flags & UNMAP_NOTIFY_SEND_EVENT) {
> +             notify_remote_via_evtchn(map->notify_event);
> +     }
> +
>       if (map->pages) {
>               if (!use_ptemod)
>                       unmap_grant_pages(map, 0, map->count);
> @@ -272,6 +280,16 @@ static int unmap_grant_pages(struct grant_map *map, int 
> offset, int pages)
>  {
>       int i, err = 0;
>  
> +     if (map->notify_flags & UNMAP_NOTIFY_CLEAR_BYTE) {
> +             int pgno = (map->notify_offset >> PAGE_SHIFT);
> +             if (pgno >= offset && pgno < pages + offset) {

Whoa, that looks to violate what 'notify_offset' actually means.
Perhaps a better name for that variable? notify_addr?

> +                     uint8_t *tmp = kmap(map->pages[pgno]);
> +                     tmp[map->notify_offset & (PAGE_SIZE-1)] = 0;
> +                     kunmap(map->pages[pgno]);
> +                     map->notify_flags &= ~UNMAP_NOTIFY_CLEAR_BYTE;
> +             }
> +     }
> +
>       pr_debug("map %d+%d [%d+%d]\n", map->index, map->count, offset, pages);
>       err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages, pages);
>       if (err)
> @@ -517,6 +535,39 @@ static long gntdev_ioctl_get_offset_for_vaddr(struct 
> gntdev_priv *priv,
>       return 0;
>  }
>  
> +static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)

> +{
> +     struct ioctl_gntdev_unmap_notify op;
> +     struct grant_map *map;
> +     int rc;
> +
> +     if (copy_from_user(&op, u, sizeof(op)))
> +             return -EFAULT;
> +
> +     if (op.action & ~(UNMAP_NOTIFY_CLEAR_BYTE|UNMAP_NOTIFY_SEND_EVENT))
> +             return -EINVAL;
> +
> +     spin_lock(&priv->lock);
> +
> +     list_for_each_entry(map, &priv->maps, next) {
> +             uint64_t begin = map->index << PAGE_SHIFT;
> +             uint64_t end = (map->index + map->count) << PAGE_SHIFT;
> +             if (op.index >= begin && op.index < end)
> +                     goto found;
> +     }
> +     rc = -ENOENT;
> +     goto unlock_out;
> +
> + found:
> +     map->notify_flags = op.action;
> +     map->notify_offset = op.index - (map->index << PAGE_SHIFT);

Minus? So say the index is 0xdeadbeef, wouldn't this throw this off?

Should you also check the index to make sure it is within a PAGE_SIZE
offset? (the ioctl accepts 64-bit values for the index).

> +     map->notify_event = op.event_channel_port;
> +     rc = 0;
> + unlock_out:
> +     spin_unlock(&priv->lock);
> +     return rc;
> +}
> +
>  static long gntdev_ioctl(struct file *flip,
>                        unsigned int cmd, unsigned long arg)
>  {
> @@ -533,6 +584,9 @@ static long gntdev_ioctl(struct file *flip,
>       case IOCTL_GNTDEV_GET_OFFSET_FOR_VADDR:
>               return gntdev_ioctl_get_offset_for_vaddr(priv, ptr);
>  
> +     case IOCTL_GNTDEV_SET_UNMAP_NOTIFY:
> +             return gntdev_ioctl_notify(priv, ptr);
> +
>       default:
>               pr_debug("priv %p, unknown cmd %x\n", priv, cmd);
>               return -ENOIOCTLCMD;
> diff --git a/include/xen/gntalloc.h b/include/xen/gntalloc.h
> index e1d6d0f..92339f5 100644
> --- a/include/xen/gntalloc.h
> +++ b/include/xen/gntalloc.h
> @@ -67,4 +67,32 @@ struct ioctl_gntalloc_dealloc_gref {
>       /* Number of references to unmap */
>       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_GNTALLOC_SET_UNMAP_NOTIFY \
> +_IOC(_IOC_NONE, 'G', 7, sizeof(struct ioctl_gntalloc_unmap_notify))
> +struct ioctl_gntalloc_unmap_notify {
> +     /* IN parameters */
> +     /* Index of a byte in the page */
> +     uint64_t index;
> +     /* Action(s) to take on unmap */
> +     uint32_t action;
> +     /* Event channel to notify */
> +     uint32_t event_channel_port;
> +};
> +
> +/* 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_GNTALLOC_H__ */
> diff --git a/include/xen/gntdev.h b/include/xen/gntdev.h
> index eb23f41..5d9b9b4 100644
> --- a/include/xen/gntdev.h
> +++ b/include/xen/gntdev.h
> @@ -116,4 +116,31 @@ 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 */
> +     /* Index of a byte in the page */
> +     uint64_t index;
> +     /* Action(s) to take on unmap */
> +     uint32_t action;
> +     /* Event channel to notify */
> +     uint32_t event_channel_port;
> +};
> +
> +/* 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__ */
> -- 
> 1.7.3.4

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

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