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: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 6/6] xen/gntalloc, gntdev: Add unmap notify ioctl
From: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Date: Thu, 27 Jan 2011 15:09:49 -0500
Cc: jeremy@xxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, Ian.Campbell@xxxxxxxxxx
Delivery-date: Thu, 27 Jan 2011 12:10:32 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110127192038.GD27853@xxxxxxxxxxxx>
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>
Organization: National Security Agency
References: <1295625548-22069-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1295625548-22069-7-git-send-email-dgdegra@xxxxxxxxxxxxx> <20110127192038.GD27853@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101209 Fedora/3.1.7-0.35.b3pre.fc13 Thunderbird/3.1.7
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

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