|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH 6/6] xen/gntalloc, gntdev: Add unmap notify ioctl
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@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.".
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |