Re: [PATCH 6/6] xen/gntalloc, gntdev: Add unmap notify ioctl

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

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@tycho.nsa.gov>
> ---
>  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