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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 01/27/2011 02:20 PM, Konrad Rzeszutek Wilk wrote:
> 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.".

OK, will do. I'm just using a bitfield here to save memory, so noting 
the exact bits didn't seem necessary.

> 
>> +	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.

It's not doing anything else besides adding/removing the notification,
but it does do the notify only on unmap or delete, so that's a good name.

>> +{
>> +	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?

I'm not sure how the user would mess up without notification: either it will be
a valid offset, or find_grefs will fail and return -ENOENT.

>> +
>> +	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?

Yes, they're related enough. I'll do that in the next revision.

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

It's not an absolute address within the device, just within the multi-page
structure pointed to by map. notify_addr is also a good name, if you think
that it makes it clearer.

>> +			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?
 
If op.index = 0xdeadbeef and map->index = 0xdead9, then we want 0x2eef
to be the offset.

> 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).

If we get here, the offset is within the multi-page entry pointed to by
map. It is valid to have notify_offset > PAGE_SIZE if map->count > 1; in
this case, the notify points to a byte on the 2nd or later page.

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