Grant access to more than one dom

Grant access to more than one dom

[Simon Martin]

Hi all,

  The   system   I  am implementing has 2 domU: Windows, and realtime,
  and  I  need  to  share  memory  between  them. Originally I had the
  realtime  domU  produce  a  page  and which was then consumed by the
  Windows  domU,  however  given the way that the realtime domU works,
  this  can  terminate  and restart, which will destroy and invalidate
  the  shared  memory page.

  To  simplify  things I have written a simple little app that runs in
  dom0,  creates  the  shared  memory page(s) and then starts both the
  domUs.  If  either domU stops, it is automatically restarted, but as
  the shared memory belongs to dom0 it stays stable across restarts.

  However  I  now  need to share this grant between multiple domUs. As
  far as I can see the grant ref only allows access from a single dom.
  How do I work around this? It must be possible, otherwise all driver
  implementations would require a memcpy by dom0.

-- 
Best regards,
 Simon                          mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Ian Campbell]

On Thu, 2014-04-17 at 11:58 -0300, Simon Martin wrote:
> Hi all,
> 
>   The   system   I  am implementing has 2 domU: Windows, and realtime,
>   and  I  need  to  share  memory  between  them. Originally I had the
>   realtime  domU  produce  a  page  and which was then consumed by the
>   Windows  domU,  however  given the way that the realtime domU works,
>   this  can  terminate  and restart, which will destroy and invalidate
>   the  shared  memory page.
> 
>   To  simplify  things I have written a simple little app that runs in
>   dom0,  creates  the  shared  memory page(s) and then starts both the
>   domUs.  If  either domU stops, it is automatically restarted, but as
>   the shared memory belongs to dom0 it stays stable across restarts.
> 
>   However  I  now  need to share this grant between multiple domUs. As
>   far as I can see the grant ref only allows access from a single dom.
>   How do I work around this? It must be possible, otherwise all driver
>   implementations would require a memcpy by dom0.

Dom0 should create two grant refs, one for each of the other domains.

Ian.

Re: Grant access to more than one dom

[Simon Martin]

Hello Ian,

> Dom0 should create two grant refs, one for each of the other domains.

Perfect. Thanks Ian. New there had to be a simple way of doing it!

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Simon Martin]

Hello Ian,

> Dom0 should create two grant refs, one for each of the other domains.

Now comes the next question. The dom0 app is running in userspace, so
I am using the /dev/xen/gntalloc device to allocate the pages. Looking
at the IOCTLs defined for this I don't see a function that will take
an existing map index and share it again.

I've had a quick look at Linux/drivers/xen/gntalloc.c and it doesn't
look too complicated to implement this, however I don't want to tread
on anybody's toes, also any chance that this could make it's way back
into kernel source?

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Ian Campbell]

On Thu, 2014-04-17 at 12:38 -0300, Simon Martin wrote:
> Hello Ian,
> 
> > Dom0 should create two grant refs, one for each of the other domains.
> 
> Now comes the next question. The dom0 app is running in userspace, so
> I am using the /dev/xen/gntalloc device to allocate the pages. Looking
> at the IOCTLs defined for this I don't see a function that will take
> an existing map index and share it again.
> 
> I've had a quick look at Linux/drivers/xen/gntalloc.c and it doesn't
> look too complicated to implement this, however I don't want to tread
> on anybody's toes,

I shouldn't think any one is working on this, Daniel De Graaf (cc) is
the original author.

>  also any chance that this could make it's way back
> into kernel source?

I'm not a kernel maintainer, but I don't see why not.

Ian.

Re: Grant access to more than one dom

[Simon Martin]

Hi Daniel,

>> I've had a quick look at Linux/drivers/xen/gntalloc.c and it doesn't
>> look too complicated to implement this, however I don't want to tread
>> on anybody's toes,

> I shouldn't think any one is working on this, Daniel De Graaf (cc) is
> the original author.

>>  also any chance that this could make it's way back
>> into kernel source?

> I'm not a kernel maintainer, but I don't see why not.

Do  you  mind  if  I wade in here? Once I have my mods, would you mind
checking them, signing them off and pushing then up stream?


-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Daniel De Graaf]

On 04/17/2014 12:57 PM, Simon Martin wrote:
> Hi Daniel,
>
>>> I've had a quick look at Linux/drivers/xen/gntalloc.c and it doesn't
>>> look too complicated to implement this, however I don't want to tread
>>> on anybody's toes,
>
>> I shouldn't think any one is working on this, Daniel De Graaf (cc) is
>> the original author.
>
>>>   also any chance that this could make it's way back
>>> into kernel source?
>
>> I'm not a kernel maintainer, but I don't see why not.
>
> Do  you  mind  if  I wade in here? Once I have my mods, would you mind
> checking them, signing them off and pushing then up stream?

Go ahead - the only reason that I did not implement gntalloc with this
ability is because I did not have a use case for it (and only sharing the
pages once was a bit simpler).  I think this would be a useful extension.

I'll look at the modifications, but I'm not the maintainer for this file,
so you'll need to send your changes to LKML and the Xen kernel maintainers
to get it upstream.

-- 
Daniel De Graaf
National Security Agency

Re: Grant access to more than one dom

[Simon Martin]

Thanks Daniel,

>> Do  you  mind  if  I wade in here? Once I have my mods, would you mind
>> checking them, signing them off and pushing then up stream?

> Go ahead - the only reason that I did not implement gntalloc with this
> ability is because I did not have a use case for it (and only sharing the
> pages once was a bit simpler).  I think this would be a useful extension.

> I'll look at the modifications, but I'm not the maintainer for this file,
> so you'll need to send your changes to LKML and the Xen kernel maintainers
> to get it upstream.

I'll  carry on then. When I have it working and tested I'll send it to
you  so  you  can  have  a look. Once everyone is happy I'll see about
pushing it to a kernel maintainer.

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Simon Martin]

Hello Daniel,

>> I'll look at the modifications, but I'm not the maintainer for this file,
>> so you'll need to send your changes to LKML and the Xen kernel maintainers
>> to get it upstream.

> I'll  carry on then. When I have it working and tested I'll send it to
> you  so  you  can  have  a look. Once everyone is happy I'll see about
> pushing it to a kernel maintainer.

I'm browsing the kernel git repositories, and wondering which to
clone. There seem to be a few that are Xen related, or should I just
clone the current stable release?

Regards.


-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Ian Campbell]

On Fri, 2014-04-25 at 09:12 -0300, Simon Martin wrote:
> Hello Daniel,
> 
> >> I'll look at the modifications, but I'm not the maintainer for this file,
> >> so you'll need to send your changes to LKML and the Xen kernel maintainers
> >> to get it upstream.
> 
> > I'll  carry on then. When I have it working and tested I'll send it to
> > you  so  you  can  have  a look. Once everyone is happy I'll see about
> > pushing it to a kernel maintainer.
> 
> I'm browsing the kernel git repositories, and wondering which to
> clone. There seem to be a few that are Xen related, or should I just
> clone the current stable release?

Development should be based on the Linus Torvald's development branch by
default.

Ian.

Re: Grant access to more than one dom

[Simon Martin]

Thanks Ian,

>> I'm browsing the kernel git repositories, and wondering which to
>> clone. There seem to be a few that are Xen related, or should I just
>> clone the current stable release?

> Development should be based on the Linus Torvald's development branch by
> default.

I assume that should be
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/ can
you confirm please.

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Ian Campbell]

On Fri, 2014-04-25 at 09:18 -0300, Simon Martin wrote:
> Thanks Ian,
> 
> >> I'm browsing the kernel git repositories, and wondering which to
> >> clone. There seem to be a few that are Xen related, or should I just
> >> clone the current stable release?
> 
> > Development should be based on the Linus Torvald's development branch by
> > default.
> 
> I assume that should be
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/ can
> you confirm please.

Yes, the latest rc tag is probably a good bet. v3.15-rc2.

Ian.

Re: Grant access to more than one dom

[Simon Martin]

Hi Daniel.

> I'll  carry on then. When I have it working and tested I'll send it to
> you  so  you  can  have  a look. Once everyone is happy I'll see about
> pushing it to a kernel maintainer.

After  being  sidetracked  into  other more boring projects I have now
almost  completed  this  change,  however  I  have some questions. I'm
pretty certain that I must be missing something here.

For  the  moment  I  am  only looking at gntalloc.c as it is the one I
need.

In  the  add_grefs  function  you  create  a couple of list structures
(queue_gref  and queue_file) and then splice these onto the end of the
gref_list  and  gntalloc_file_private_data.list lists. However I'm not
really  sure  what  you  are  doing  here. Can you explain please. The
problems I see are:

1.- queue_gref  and queue_file are declared on the stack, however they
are  then  appended to the lists with no memory allocation whatsoever.
What are you expecting to happen here?

2.-  queue_gref   and queue_file are just list structs, i.e. they have
no  payload,  they  are not part of an encapsulating struct, so again,
what are you expecting to happen here?

Regards.


-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Daniel De Graaf]

On 05/08/2014 11:47 AM, Simon Martin wrote:
> Hi Daniel.
>
>> I'll  carry on then. When I have it working and tested I'll send it to
>> you  so  you  can  have  a look. Once everyone is happy I'll see about
>> pushing it to a kernel maintainer.
>
> After  being  sidetracked  into  other more boring projects I have now
> almost  completed  this  change,  however  I  have some questions. I'm
> pretty certain that I must be missing something here.
>
> For  the  moment  I  am  only looking at gntalloc.c as it is the one I
> need.
>
> In  the  add_grefs  function  you  create  a couple of list structures
> (queue_gref  and queue_file) and then splice these onto the end of the
> gref_list  and  gntalloc_file_private_data.list lists. However I'm not
> really  sure  what  you  are  doing  here. Can you explain please. The
> problems I see are:
>
> 1.- queue_gref  and queue_file are declared on the stack, however they
> are  then  appended to the lists with no memory allocation whatsoever.
> What are you expecting to happen here?
>
> 2.-  queue_gref   and queue_file are just list structs, i.e. they have
> no  payload,  they  are not part of an encapsulating struct, so again,
> what are you expecting to happen here?
>
> Regards.

The add_grefs function strings the allocated gntalloc_gref structures onto
two linked lists: the list heads are local, but are only used temporarily.
If the operation is successful, the list_splice_tail calls wire up the
linked lists to permanent structures on the heap; if unsuccessful, they
are used to clean up.

-- 
Daniel De Graaf
National Security Agency

Re: Grant access to more than one dom

[Simon Martin]

Thanks Daniel,


> The add_grefs function strings the allocated gntalloc_gref structures onto
> two linked lists: the list heads are local, but are only used temporarily.
> If the operation is successful, the list_splice_tail calls wire up the
> linked lists to permanent structures on the heap; if unsuccessful, they
> are used to clean up.

After sending the mail I kind of worked out the what, however the how
still bugs me. The function list_splice_tail does pointer juggling,
not allocation, so as far as I can see you just end up with pointers to
stack variables...

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Daniel De Graaf]

On 05/08/2014 04:03 PM, Simon Martin wrote:
> Thanks Daniel,
>
>
>> The add_grefs function strings the allocated gntalloc_gref structures onto
>> two linked lists: the list heads are local, but are only used temporarily.
>> If the operation is successful, the list_splice_tail calls wire up the
>> linked lists to permanent structures on the heap; if unsuccessful, they
>> are used to clean up.
>
> After sending the mail I kind of worked out the what, however the how
> still bugs me. The function list_splice_tail does pointer juggling,
> not allocation, so as far as I can see you just end up with pointers to
> stack variables...
>

The allocation is done ingref = kzalloc(sizeof(*gref), GFP_KERNEL).  Any
pointers to stack variables are overwritten during the splice.

-- 
Daniel De Graaf
National Security Agency

Re: Grant access to more than one dom

[Simon Martin]

Hello Daniel,

> The allocation is done ingref = kzalloc(sizeof(*gref), GFP_KERNEL). Any
> pointers to stack variables are overwritten during the splice.

Spotted  my mistake, list_splice_tail doesn't include the root node of
the appended list, so stack locations disappear.


-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

Re: Grant access to more than one dom

[Simon Martin]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

Hello Daniel,

I  have  been using the modified kernel driver for a few weeks now and
it seems to be doing everything I want it to. I attach the patch file,
this patches gntalloc.c and gntalloc.h.

Can  you  check  that  you are happy with the changes and then tell me
what to do to push then upstream?

Regards.

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

[-- Attachment #2: gntalloc.patch --]
[-- Type: application/octet-stream, Size: 7301 bytes --]

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..60998d1 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -85,6 +85,12 @@ struct notify_info {
 	int event;            /* Port (event channel) to notify */
 };
 
+/* list of foreign grants associated to a given grant reference */
+struct gntalloc_share_list {
+	grant_ref_t gref_id;
+	struct list_head next_share;
+};
+
 /* Metadata on a grant reference. */
 struct gntalloc_gref {
 	struct list_head next_gref;  /* list entry gref_list */
@@ -92,7 +98,7 @@ struct gntalloc_gref {
 	struct page *page;           /* The shared page */
 	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 */
+	struct gntalloc_share_list share_list;  /* The list of grant reference numbers associated with this page */
 	struct notify_info notify;   /* Unmap notification */
 };
 
@@ -125,6 +131,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	LIST_HEAD(queue_gref);
 	LIST_HEAD(queue_file);
 	struct gntalloc_gref *gref;
+	grant_ref_t gref_id;
 
 	readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE);
 	rc = -ENOMEM;
@@ -141,13 +148,19 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 			goto undo;
 
 		/* Grant foreign access to the page. */
-		gref->gref_id = gnttab_grant_foreign_access(op->domid,
+		gref_id = gnttab_grant_foreign_access(op->domid,
 			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
-		if ((int)gref->gref_id < 0) {
-			rc = gref->gref_id;
+		if ((int)gref_id < 0) {
+			rc = gref_id;
 			goto undo;
 		}
-		gref_ids[i] = gref->gref_id;
+
+		// store the gred_id to be returned to the user so they can use it later
+		gref_ids[i] = gref_id;
+
+		// add this gref to the list for this page
+		INIT_LIST_HEAD(&gref->share_list.next_share);
+		gref->share_list.gref_id = gref_id;
 	}
 
 	/* Add to gref lists. */
@@ -179,8 +192,21 @@ undo:
 	return rc;
 }
 
+static void __release_gref(grant_ref_t *gref_id)
+{
+	if ((*gref_id > 0) &&
+		!gnttab_query_foreign_access(*gref_id) &&
+		gnttab_end_foreign_access_ref(*gref_id, 0))
+	{
+		gnttab_free_grant_reference(*gref_id);
+		*gref_id = 0;
+	}
+}
+
 static void __del_gref(struct gntalloc_gref *gref)
 {
+	struct gntalloc_share_list *pos = NULL;
+
 	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		uint8_t *tmp = kmap(gref->page);
 		tmp[gref->notify.pgoff] = 0;
@@ -192,16 +218,16 @@ static void __del_gref(struct gntalloc_gref *gref)
 	}
 
 	gref->notify.flags = 0;
-
-	if (gref->gref_id > 0) {
-		if (gnttab_query_foreign_access(gref->gref_id))
-			return;
-
-		if (!gnttab_end_foreign_access_ref(gref->gref_id, 0))
-			return;
-
-		gnttab_free_grant_reference(gref->gref_id);
+	while (!list_empty(&gref->share_list.next_share))
+	{
+		pos = list_first_entry(&gref->share_list.next_share,
+						 struct gntalloc_share_list,
+						 next_share);
+		list_del(&pos->next_share);
+		__release_gref(&pos->gref_id);
+		kfree(pos);
 	}
+	__release_gref(&gref->share_list.gref_id);
 
 	gref_size--;
 	list_del(&gref->next_gref);
@@ -337,6 +363,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
 
 out_free:
 	kfree(gref_ids);
+
 out:
 	return rc;
 }
@@ -435,6 +462,97 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	return rc;
 }
 
+static long gntalloc_ioctl_share(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_share_gref op;
+	struct gntalloc_gref *gref;
+	int rc;
+	int readonly;
+	grant_ref_t gref_id;
+	struct gntalloc_share_list *share_list;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	readonly = !(op.flags & GNTALLOC_FLAG_WRITABLE);
+	rc = -ENOMEM;
+
+	/* get the grant structure */
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref) {
+		rc = -ENOENT;
+		goto share_out;
+	}
+
+	/* Grant foreign access to the page. */
+	gref_id = gnttab_grant_foreign_access(op.domid,
+		pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+	if ((signed)gref_id < 0) {
+		rc = gref_id;
+		goto share_out;
+	}
+
+	// add this gref to the list for this page
+	share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);
+	INIT_LIST_HEAD(&share_list->next_share);
+	share_list->gref_id = gref_id;
+	list_add_tail(&share_list->next_share, &gref->share_list.next_share);
+
+	// set output data
+	op.gref_id = gref_id;
+	rc = 0;
+
+share_out:
+	mutex_unlock(&gref_mutex);
+
+	if (!rc && copy_to_user(arg, &op, sizeof(op)))
+		rc = -EFAULT;
+
+	return rc;
+}
+
+static long gntalloc_ioctl_unshare(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_unshare_gref op;
+	struct gntalloc_gref *gref;
+	int rc = -ENOENT;
+	struct gntalloc_share_list *pos;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	/* get the grant structure */
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref)
+		goto unshare_out;
+
+	// search the share list for this grant ref
+	list_for_each_entry(pos, &gref->share_list.next_share, next_share) {
+		// if this is our grant ref then delete it
+		if (pos->gref_id == op.gref_id) {
+			// delete
+			list_del(&pos->next_share);
+			__release_gref(&pos->gref_id);
+			kfree(pos);
+
+			// success
+			rc = 0;
+
+			// done
+			break;
+		}
+	}
+
+unshare_out:
+	mutex_unlock(&gref_mutex);
+
+	return rc;
+}
+
 static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -450,6 +568,12 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 	case IOCTL_GNTALLOC_SET_UNMAP_NOTIFY:
 		return gntalloc_ioctl_unmap_notify(priv, (void __user *)arg);
 
+	case IOCTL_GNTALLOC_SHARE_GREF:
+		return gntalloc_ioctl_share(priv, (void __user *)arg);
+
+	case IOCTL_GNTALLOC_UNSHARE_GREF:
+		return gntalloc_ioctl_unshare(priv, (void __user *)arg);
+
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
index 76bd580..20dc973 100644
--- a/include/uapi/xen/gntalloc.h
+++ b/include/uapi/xen/gntalloc.h
@@ -79,4 +79,36 @@ struct ioctl_gntalloc_unmap_notify {
 /* Send an interrupt on the indicated event channel */
 #define UNMAP_NOTIFY_SEND_EVENT 0x2
 
+/*
+ * Shares a page that has already been allocated (so we can share the smae page 
+ * between more than one domain)
+ */
+#define IOCTL_GNTALLOC_SHARE_GREF \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntalloc_share_gref))
+struct ioctl_gntalloc_share_gref {
+	/* IN parameters */
+	/* The ID of the domain to be given access to the grants. */
+	uint16_t domid;
+	/* Flags for this mapping */
+	uint16_t flags;
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* OUT parameters */
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
+/*
+ * Unhares a page that has already been shared
+ */
+#define IOCTL_GNTALLOC_UNSHARE_GREF \
+_IOC(_IOC_NONE, 'G', 9, sizeof(struct ioctl_gntalloc_unshare_gref))
+struct ioctl_gntalloc_unshare_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
 #endif /* __LINUX_PUBLIC_GNTALLOC_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Grant access to more than one dom

[Ian Campbell]

On Thu, 2014-06-05 at 12:10 -0400, Simon Martin wrote:
> Hello Daniel,
> 
> I  have  been using the modified kernel driver for a few weeks now and
> it seems to be doing everything I want it to. I attach the patch file,
> this patches gntalloc.c and gntalloc.h.
> 
> Can  you  check  that  you are happy with the changes and then tell me
> what to do to push then upstream?

FYI See Documentation/SubmittingPatches for the Linux process here (it's
not dissimilar to Xen's)

The normal way to get feedback on a change is to just post it formally
following that procedure and then rinse/repeat as you get comments,
rather than asking for comments before submitting.

Ian.

Re: Grant access to more than one dom

[Daniel De Graaf]

On 06/05/2014 12:10 PM, Simon Martin wrote:
> Hello Daniel,
>
> I  have  been using the modified kernel driver for a few weeks now and
> it seems to be doing everything I want it to. I attach the patch file,
> this patches gntalloc.c and gntalloc.h.
>
> Can  you  check  that  you are happy with the changes and then tell me
> what to do to push then upstream?
>
> Regards.
>

Ian has already mentioned the preferred method to submit for upstream. I
have a few comments on this version below.

> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index 787d179..60998d1 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -85,6 +85,12 @@ struct notify_info {
>  	int event;            /* Port (event channel) to notify */
>  };
>
> +/* list of foreign grants associated to a given grant reference */
> +struct gntalloc_share_list {
> +	grant_ref_t gref_id;
> +	struct list_head next_share;
> +};
> +
>  /* Metadata on a grant reference. */
>  struct gntalloc_gref {
>  	struct list_head next_gref;  /* list entry gref_list */
> @@ -92,7 +98,7 @@ struct gntalloc_gref {
>  	struct page *page;           /* The shared page */
>  	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 */
> +	struct gntalloc_share_list share_list;  /* The list of grant reference numbers associated with this page */
>  	struct notify_info notify;   /* Unmap notification */
>  };

While I think that including the full structure here is correct since it
avoids unneeded allocations for the common case where only one gref_id is
used, this does end up complicating some of the logic slightly.  Naming
the variable share_list is a bit misleading, since it contains a gref_id
in addition to the list.

[...]
> @@ -179,8 +192,21 @@ undo:
>  	return rc;
>  }
>
> +static void __release_gref(grant_ref_t *gref_id)
> +{
> +	if ((*gref_id > 0) &&
> +		!gnttab_query_foreign_access(*gref_id) &&
> +		gnttab_end_foreign_access_ref(*gref_id, 0))
> +	{
> +		gnttab_free_grant_reference(*gref_id);
> +		*gref_id = 0;
> +	}
> +}

This function needs to return success/failure.

>  static void __del_gref(struct gntalloc_gref *gref)
>  {
> +	struct gntalloc_share_list *pos = NULL;
> +
>  	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
>  		uint8_t *tmp = kmap(gref->page);
>  		tmp[gref->notify.pgoff] = 0;
> @@ -192,16 +218,16 @@ static void __del_gref(struct gntalloc_gref *gref)
>  	}
>
>  	gref->notify.flags = 0;
> -
> -	if (gref->gref_id > 0) {
> -		if (gnttab_query_foreign_access(gref->gref_id))
> -			return;
> -
> -		if (!gnttab_end_foreign_access_ref(gref->gref_id, 0))
> -			return;
> -
> -		gnttab_free_grant_reference(gref->gref_id);
> +	while (!list_empty(&gref->share_list.next_share))
> +	{
> +		pos = list_first_entry(&gref->share_list.next_share,
> +						 struct gntalloc_share_list,
> +						 next_share);
> +		list_del(&pos->next_share);
> +		__release_gref(&pos->gref_id);
> +		kfree(pos);
>  	}
> +	__release_gref(&gref->share_list.gref_id);

This will leak grant references if __release_gref fails, both inside and
outside the loop.  You need to remove the references that can be freed
and return before this line if any of the grant references still have a
foreign mapping.

>  	gref_size--;
>  	list_del(&gref->next_gref);
> @@ -337,6 +363,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
>
>  out_free:
>  	kfree(gref_ids);
> +

Unneeded whitespace change.

>  out:
>  	return rc;
>  }
> @@ -435,6 +462,97 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
>  	return rc;
>  }
>
> +static long gntalloc_ioctl_share(struct gntalloc_file_private_data *priv, void __user *arg)
> +{
> +	struct ioctl_gntalloc_share_gref op;
> +	struct gntalloc_gref *gref;
> +	int rc;
> +	int readonly;
> +	grant_ref_t gref_id;
> +	struct gntalloc_share_list *share_list;
> +
> +	if (copy_from_user(&op, arg, sizeof(op)))
> +		return -EFAULT;
> +
> +	readonly = !(op.flags & GNTALLOC_FLAG_WRITABLE);
> +	rc = -ENOMEM;
> +
> +	/* get the grant structure */
> +	mutex_lock(&gref_mutex);
> +	gref = find_grefs(priv, op.index, 1);
> +
> +	if (!gref) {
> +		rc = -ENOENT;
> +		goto share_out;
> +	}
> +
> +	/* Grant foreign access to the page. */
> +	gref_id = gnttab_grant_foreign_access(op.domid,
> +		pfn_to_mfn(page_to_pfn(gref->page)), readonly);
> +	if ((signed)gref_id < 0) {
> +		rc = gref_id;
> +		goto share_out;
> +	}
> +
> +	// add this gref to the list for this page
> +	share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);

You need to check for kzalloc returning NULL, and for proper error recovery,
the allocation needs to be done before calling gnttab_grant_foreign_access.
Otherwise, it is possible to end up in a situation where you have a grant
that cannot be freed due to a foreign mapping, but with no data structure to
put the ID in so it can be freed later.

> +	INIT_LIST_HEAD(&share_list->next_share);

This is not needed since you are immediately adding it to a list.

> +	share_list->gref_id = gref_id;
> +	list_add_tail(&share_list->next_share, &gref->share_list.next_share);
> +
> +	// set output data
> +	op.gref_id = gref_id;
> +	rc = 0;
> +
> +share_out:
> +	mutex_unlock(&gref_mutex);
> +
> +	if (!rc && copy_to_user(arg, &op, sizeof(op)))
> +		rc = -EFAULT;
> +
> +	return rc;
> +}
> +
> +static long gntalloc_ioctl_unshare(struct gntalloc_file_private_data *priv, void __user *arg)
> +{
> +	struct ioctl_gntalloc_unshare_gref op;
> +	struct gntalloc_gref *gref;
> +	int rc = -ENOENT;
> +	struct gntalloc_share_list *pos;
> +
> +	if (copy_from_user(&op, arg, sizeof(op)))
> +		return -EFAULT;
> +
> +	/* get the grant structure */
> +	mutex_lock(&gref_mutex);
> +	gref = find_grefs(priv, op.index, 1);
> +
> +	if (!gref)
> +		goto unshare_out;
> +
> +	// search the share list for this grant ref
> +	list_for_each_entry(pos, &gref->share_list.next_share, next_share) {
> +		// if this is our grant ref then delete it
> +		if (pos->gref_id == op.gref_id) {
> +			// delete
> +			list_del(&pos->next_share);
> +			__release_gref(&pos->gref_id);

This needs to return an error if the page is still shared instead of leaking
the reference.

> +			kfree(pos);
> +
> +			// success
> +			rc = 0;
> +
> +			// done
> +			break;
> +		}
> +	}

It would be nice if this function also supported freeing the initial grant
reference, so that it can be used to change the domain ID a page is being
shared with.

> +unshare_out:
> +	mutex_unlock(&gref_mutex);
> +
> +	return rc;
> +}
> +
>  static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
>  		unsigned long arg)
>  {
> @@ -450,6 +568,12 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
>  	case IOCTL_GNTALLOC_SET_UNMAP_NOTIFY:
>  		return gntalloc_ioctl_unmap_notify(priv, (void __user *)arg);
>
> +	case IOCTL_GNTALLOC_SHARE_GREF:
> +		return gntalloc_ioctl_share(priv, (void __user *)arg);
> +
> +	case IOCTL_GNTALLOC_UNSHARE_GREF:
> +		return gntalloc_ioctl_unshare(priv, (void __user *)arg);
> +
>  	default:
>  		return -ENOIOCTLCMD;
>  	}
> diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
> index 76bd580..20dc973 100644
> --- a/include/uapi/xen/gntalloc.h
> +++ b/include/uapi/xen/gntalloc.h
> @@ -79,4 +79,36 @@ struct ioctl_gntalloc_unmap_notify {
>  /* Send an interrupt on the indicated event channel */
>  #define UNMAP_NOTIFY_SEND_EVENT 0x2
>
> +/*
> + * Shares a page that has already been allocated (so we can share the smae page

Typo (smae=>same)

> + * between more than one domain)
> + */
> +#define IOCTL_GNTALLOC_SHARE_GREF \
> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntalloc_share_gref))
> +struct ioctl_gntalloc_share_gref {
> +	/* IN parameters */
> +	/* The ID of the domain to be given access to the grants. */
> +	uint16_t domid;
> +	/* Flags for this mapping */
> +	uint16_t flags;
> +	/* The offset used in call to mmap(). */
> +	uint64_t index;
> +	/* OUT parameters */
> +	/* The grant references of the newly created grant */
> +	uint32_t gref_id;
> +};

This structure should have uint64_t index as the first member; otherwise,
it will differ in size depending on the alignment of 64-bit types and a
compat_ioctl will be required for 32-bit userspace.

> +
> +/*
> + * Unhares a page that has already been shared

Typo  ^^^^^^^

> + */
> +#define IOCTL_GNTALLOC_UNSHARE_GREF \
> +_IOC(_IOC_NONE, 'G', 9, sizeof(struct ioctl_gntalloc_unshare_gref))
> +struct ioctl_gntalloc_unshare_gref {
> +	/* IN parameters */
> +	/* The offset used in call to mmap(). */
> +	uint64_t index;
> +	/* The grant references of the newly created grant */
> +	uint32_t gref_id;
> +};
> +
>  #endif /* __LINUX_PUBLIC_GNTALLOC_H__ */
>


-- 
Daniel De Graaf
National Security Agency

Re: Grant access to more than one dom

[Simon Martin]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

Hi Daniel,

Thanks for the revision. I attach an updated patch. With regards your
comments:

1.- Renamed share_list field of gntalloc_gref. Now called shares.

2.- As you said, I think the increased logic is worth the effort of
not requiring dynamic memory for the usual case of only one share.

3.- Analyzing the return value of __release_gref I found there is a
possible (unlikely) infinite loop in the original __del_gref
implementation. If gref_id <= 0 then the list element will never be
released. If we have this situation then a memory leak is the the
least of our worries as there is either a problem in the logic or
memory corruption. I propose that under this situation we should just
return OK and carry on. What do you think?

4.- Fixed possible leak in __del_gref

5.- Fixed whitespace changes.

6.- Checked kzalloc return value in gntalloc_ioctl_share.

7.- Removed superfluous INIT_LIST_HEAD in gntalloc_ioctl_share.

8.- Fixed possible gref leak in gntalloc_ioctl_unshare

9.- Re your comment about gntalloc_ioctl_unshare:

> It would be nice if this function also supported freeing the initial grant
> reference, so that it can be used to change the domain ID a page is being
> shared with.

The idea of gntalloc_gref.shares is that there is 1 or more grefs
associated to any allocation. This would mean changing that to 0 or
more, i.e. gntalloc_gref.shares should be changed directly to a list
head and we will require a memory allocation for the original
allocation. The change is simple, shall I do it?

10.- gntalloc.h typos fixed.

11.- Fixed struct ioctl_gntalloc_share_gref member order.

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

[-- Attachment #2: gntalloc.patch --]
[-- Type: application/octet-stream, Size: 8016 bytes --]

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..db33a89 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -85,6 +85,12 @@ struct notify_info {
 	int event;            /* Port (event channel) to notify */
 };
 
+/* list of foreign grants associated to a given grant reference */
+struct gntalloc_share_list {
+	grant_ref_t gref_id;
+	struct list_head next_share;
+};
+
 /* Metadata on a grant reference. */
 struct gntalloc_gref {
 	struct list_head next_gref;  /* list entry gref_list */
@@ -92,7 +98,7 @@ struct gntalloc_gref {
 	struct page *page;	     /* The shared page */
 	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 */
+	struct gntalloc_share_list shares;  /* The list of grant reference numbers associated with this page */
 	struct notify_info notify;   /* Unmap notification */
 };
 
@@ -125,6 +131,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	LIST_HEAD(queue_gref);
 	LIST_HEAD(queue_file);
 	struct gntalloc_gref *gref;
+	grant_ref_t gref_id;
 
 	readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE);
 	rc = -ENOMEM;
@@ -141,13 +148,19 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 			goto undo;
 
 		/* Grant foreign access to the page. */
-		gref->gref_id = gnttab_grant_foreign_access(op->domid,
+		gref_id = gnttab_grant_foreign_access(op->domid,
 			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
-		if ((int)gref->gref_id < 0) {
-			rc = gref->gref_id;
+		if ((int)gref_id < 0) {
+			rc = gref_id;
 			goto undo;
 		}
-		gref_ids[i] = gref->gref_id;
+
+		// store the gred_id to be returned to the user so they can use it later
+		gref_ids[i] = gref_id;
+
+		// add this gref to the list for this page
+		INIT_LIST_HEAD(&gref->shares.next_share);
+		gref->shares.gref_id = gref_id;
 	}
 
 	/* Add to gref lists. */
@@ -179,8 +192,31 @@ undo:
 	return rc;
 }
 
+static int __release_gref(grant_ref_t *gref_id)
+{
+	/* if we have an invalid gref_id then we have a real problem, either a
+	   problem in logic or memory corruption, so a leaking a page is the
+	   least of our worries */
+	if (*gref_id <= 0)
+		return 0;
+	/* if this gref is still mapped by the foreign dom then we can't release
+	   so return error and the caller must retry */
+	else if (gnttab_query_foreign_access(*gref_id))
+		return -EAGAIN;
+	/* if we can't release the foreign access to this gref then we can't
+	   release and the caller must retry */
+	else if (!gnttab_end_foreign_access_ref(*gref_id, 0))
+		return -EAGAIN;
+
+	gnttab_free_grant_reference(*gref_id);
+	*gref_id = 0;
+	return 0;
+}
+
 static void __del_gref(struct gntalloc_gref *gref)
 {
+	struct gntalloc_share_list *pos = NULL;
+
 	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		uint8_t *tmp = kmap(gref->page);
 		tmp[gref->notify.pgoff] = 0;
@@ -192,16 +228,18 @@ static void __del_gref(struct gntalloc_gref *gref)
 	}
 
 	gref->notify.flags = 0;
-
-	if (gref->gref_id > 0) {
-		if (gnttab_query_foreign_access(gref->gref_id))
-			return;
-
-		if (!gnttab_end_foreign_access_ref(gref->gref_id, 0))
-			return;
-
-		gnttab_free_grant_reference(gref->gref_id);
+	while (!list_empty(&gref->shares.next_share))
+	{
+		pos = list_first_entry(&gref->shares.next_share,
+						 struct gntalloc_share_list,
+						 next_share);
+		if (!__release_gref(&pos->gref_id))
+		{
+			list_del(&pos->next_share);
+			kfree(pos);
+		}
 	}
+	while (__release_gref(&gref->shares.gref_id));
 
 	gref_size--;
 	list_del(&gref->next_gref);
@@ -435,6 +473,104 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	return rc;
 }
 
+static long gntalloc_ioctl_share(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_share_gref op;
+	struct gntalloc_gref *gref;
+	int rc;
+	int readonly;
+	struct gntalloc_share_list *share_list;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	readonly = !(op.flags & GNTALLOC_FLAG_WRITABLE);
+	rc = -ENOMEM;
+
+	/* get the grant structure */
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref) {
+		rc = -ENOENT;
+		goto share_out;
+	}
+
+	// create share list node
+	share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);
+	if (!share_list) {
+		rc = -ENOMEM;
+		goto share_out;
+	}
+
+	/* Grant foreign access to the page. */
+	share_list->gref_id = gnttab_grant_foreign_access(op.domid,
+		pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+	if ((signed)share_list->gref_id < 0) {
+		kzfree(share_list);
+		rc = share_list->gref_id;
+		goto share_out;
+	}
+
+	// add this gref to the list for this page
+	list_add_tail(&share_list->next_share, &gref->shares.next_share);
+
+	// set output data
+	op.gref_id = share_list->gref_id;
+	rc = 0;
+
+share_out:
+	mutex_unlock(&gref_mutex);
+
+	if (!rc && copy_to_user(arg, &op, sizeof(op)))
+		rc = -EFAULT;
+
+	return rc;
+}
+
+static long gntalloc_ioctl_unshare(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_unshare_gref op;
+	struct gntalloc_gref *gref;
+	int rc = -ENOENT;
+	struct gntalloc_share_list *pos;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	/* get the grant structure */
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref)
+		goto unshare_out;
+
+	// search the share list for this grant ref
+	list_for_each_entry(pos, &gref->shares.next_share, next_share) {
+		// if this is our grant ref then delete it
+		if (pos->gref_id == op.gref_id) {
+			// if we cannot release then request again
+			if (__release_gref(&pos->gref_id))
+				rc = -EAGAIN;
+			else {
+				// delete
+				list_del(&pos->next_share);
+				kfree(pos);
+
+				// success
+				rc = 0;
+			}
+
+			// done
+			break;
+		}
+	}
+
+unshare_out:
+	mutex_unlock(&gref_mutex);
+	return rc;
+}
+
 static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -450,6 +586,12 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 	case IOCTL_GNTALLOC_SET_UNMAP_NOTIFY:
 		return gntalloc_ioctl_unmap_notify(priv, (void __user *)arg);
 
+	case IOCTL_GNTALLOC_SHARE_GREF:
+		return gntalloc_ioctl_share(priv, (void __user *)arg);
+
+	case IOCTL_GNTALLOC_UNSHARE_GREF:
+		return gntalloc_ioctl_unshare(priv, (void __user *)arg);
+
 	default:
 		return -ENOIOCTLCMD;
 	}
@@ -573,7 +715,7 @@ static const struct file_operations gntalloc_fops = {
  */
 static struct miscdevice gntalloc_miscdev = {
 	.minor	= MISC_DYNAMIC_MINOR,
-	.name	= "xen/gntalloc",
+	.name	= "xen/gntalloc_trio",
 	.fops	= &gntalloc_fops,
 };
 
diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
index 76bd580..31911da 100644
--- a/include/uapi/xen/gntalloc.h
+++ b/include/uapi/xen/gntalloc.h
@@ -79,4 +79,36 @@ struct ioctl_gntalloc_unmap_notify {
 /* Send an interrupt on the indicated event channel */
 #define UNMAP_NOTIFY_SEND_EVENT 0x2
 
+/*
+ * Shares a page that has already been allocated (so we can share the same page 
+ * between more than one domain)
+ */
+#define IOCTL_GNTALLOC_SHARE_GREF \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntalloc_share_gref))
+struct ioctl_gntalloc_share_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The ID of the domain to be given access to the grants. */
+	uint16_t domid;
+	/* Flags for this mapping */
+	uint16_t flags;
+	/* OUT parameters */
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
+/*
+ * Unshares a page that has already been shared
+ */
+#define IOCTL_GNTALLOC_UNSHARE_GREF \
+_IOC(_IOC_NONE, 'G', 9, sizeof(struct ioctl_gntalloc_unshare_gref))
+struct ioctl_gntalloc_unshare_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
 #endif /* __LINUX_PUBLIC_GNTALLOC_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Grant access to more than one dom

[Daniel De Graaf]

On 06/09/2014 11:53 AM, Simon Martin wrote:
> Hi Daniel,
>
> Thanks for the revision. I attach an updated patch. With regards your
> comments:
>
> 1.- Renamed share_list field of gntalloc_gref. Now called shares.
>
> 2.- As you said, I think the increased logic is worth the effort of
> not requiring dynamic memory for the usual case of only one share.
>
> 3.- Analyzing the return value of __release_gref I found there is a
> possible (unlikely) infinite loop in the original __del_gref
> implementation. If gref_id <= 0 then the list element will never be
> released. If we have this situation then a memory leak is the the
> least of our worries as there is either a problem in the logic or
> memory corruption. I propose that under this situation we should just
> return OK and carry on. What do you think?

The infinite loop you added is incorrect and is actually quite likely to
be triggered if the other domain is not responding to whatever unmap
request has been set up.  This does not have to be an error; it could be
triggered because the other domain has not yet been scheduled after the
notify was queued.  If __release_gref fails, then you need to return from
__del_gref without actually deleting the gref object.  This postpones the
actual deletion until it is retried by do_cleanup.

Encountering gref <= 0 in this loop should not happen; that indicates a
significantproblem and deserves a WARN_ON if you want to check for it.

> 4.- Fixed possible leak in __del_gref
>
> 5.- Fixed whitespace changes.
>
> 6.- Checked kzalloc return value in gntalloc_ioctl_share.
>
> 7.- Removed superfluous INIT_LIST_HEAD in gntalloc_ioctl_share.
>
> 8.- Fixed possible gref leak in gntalloc_ioctl_unshare
>
> 9.- Re your comment about gntalloc_ioctl_unshare:
>
>> It would be nice if this function also supported freeing the initial grant
>> reference, so that it can be used to change the domain ID a page is being
>> shared with.
>
> The idea of gntalloc_gref.shares is that there is 1 or more grefs
> associated to any allocation. This would mean changing that to 0 or
> more, i.e. gntalloc_gref.shares should be changed directly to a list
> head and we will require a memory allocation for the original
> allocation. The change is simple, shall I do it?

I would instead change the logic to something like:
If removing the primary reference and there are secondary references, promote
the first secondary reference (and remove/free its gntalloc_share_list). If
removing the last reference, either error, act as DEALLOC_GREF, or change all
other uses of gref_id to handle gref_id==0 meaning "not shared".

> 10.- gntalloc.h typos fixed.
>
> 11.- Fixed struct ioctl_gntalloc_share_gref member order.

The name change to xen/gntalloc_trio should not be in the submitted patch;
I assume this was for testing.


-- 
Daniel De Graaf
National Security Agency

Re: Grant access to more than one dom

[Simon Martin]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

Hello Daniel,

>>
>> 3.- Analyzing the return value of __release_gref I found there is a
>> possible (unlikely) infinite loop in the original __del_gref
>> implementation. If gref_id <= 0 then the list element will never be
>> released. If we have this situation then a memory leak is the the
>> least of our worries as there is either a problem in the logic or
>> memory corruption. I propose that under this situation we should just
>> return OK and carry on. What do you think?

> The infinite loop you added is incorrect and is actually quite likely to
> be triggered if the other domain is not responding to whatever unmap
> request has been set up.  This does not have to be an error; it could be
> triggered because the other domain has not yet been scheduled after the
> notify was queued.  If __release_gref fails, then you need to return from
> __del_gref without actually deleting the gref object.  This postpones the
> actual deletion until it is retried by do_cleanup.

> Encountering gref <= 0 in this loop should not happen; that indicates a
> significantproblem and deserves a WARN_ON if you want to check for it.

I have added WARN_ON(gref <= 0)

Removed loop waiting for foreign dom to release page and added
do_cleanup to end of share/unshare IOCTL.

>>> It would be nice if this function also supported freeing the initial grant
>>> reference, so that it can be used to change the domain ID a page is being
>>> shared with.
>>
>> The idea of gntalloc_gref.shares is that there is 1 or more grefs
>> associated to any allocation. This would mean changing that to 0 or
>> more, i.e. gntalloc_gref.shares should be changed directly to a list
>> head and we will require a memory allocation for the original
>> allocation. The change is simple, shall I do it?

> I would instead change the logic to something like:
> If removing the primary reference and there are secondary references, promote
> the first secondary reference (and remove/free its gntalloc_share_list). If
> removing the last reference, either error, act as DEALLOC_GREF, or change all
> other uses of gref_id to handle gref_id==0 meaning "not shared".

Implemented this. Not as clean as changing the semantics but avoids
dynamic allocation for the most common case.

> The name change to xen/gntalloc_trio should not be in the submitted patch;
> I assume this was for testing.

Sorry. I do this so I can have the original driver loaded at the same
time. I realized this after I'd sent the patch, but I'd already sent
it.

-- 
Best regards,
 Simon                            mailto:furryfuttock@gmail.com

[-- Attachment #2: gntalloc.patch --]
[-- Type: application/octet-stream, Size: 8691 bytes --]

diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 787d179..f15d8b6 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -85,6 +85,12 @@ struct notify_info {
 	int event;            /* Port (event channel) to notify */
 };
 
+/* list of foreign grants associated to a given grant reference */
+struct gntalloc_share_list {
+	grant_ref_t gref_id;
+	struct list_head next_share;
+};
+
 /* Metadata on a grant reference. */
 struct gntalloc_gref {
 	struct list_head next_gref;  /* list entry gref_list */
@@ -92,7 +98,7 @@ struct gntalloc_gref {
 	struct page *page;	     /* The shared page */
 	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 */
+	struct gntalloc_share_list shares;  /* The list of grant reference numbers associated with this page */
 	struct notify_info notify;   /* Unmap notification */
 };
 
@@ -125,6 +131,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 	LIST_HEAD(queue_gref);
 	LIST_HEAD(queue_file);
 	struct gntalloc_gref *gref;
+	grant_ref_t gref_id;
 
 	readonly = !(op->flags & GNTALLOC_FLAG_WRITABLE);
 	rc = -ENOMEM;
@@ -141,13 +148,19 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
 			goto undo;
 
 		/* Grant foreign access to the page. */
-		gref->gref_id = gnttab_grant_foreign_access(op->domid,
+		gref_id = gnttab_grant_foreign_access(op->domid,
 			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
-		if ((int)gref->gref_id < 0) {
-			rc = gref->gref_id;
+		if ((int)gref_id < 0) {
+			rc = gref_id;
 			goto undo;
 		}
-		gref_ids[i] = gref->gref_id;
+
+		/* store the gred_id to be returned to the user so they can use it later */
+		gref_ids[i] = gref_id;
+
+		/* add this gref to the list for this page */
+		INIT_LIST_HEAD(&gref->shares.next_share);
+		gref->shares.gref_id = gref_id;
 	}
 
 	/* Add to gref lists. */
@@ -179,8 +192,32 @@ undo:
 	return rc;
 }
 
-static void __del_gref(struct gntalloc_gref *gref)
-{
+static int __release_gref(grant_ref_t *gref_id) {
+	WARN_ON(*gref_id < 0);
+
+	/* if we have an invalid gref_id then we have a real problem, either a
+	   problem in logic or memory corruption, so a leaking a page is the
+	   least of our worries */
+	if (*gref_id <= 0)
+		return 0;
+	/* if this gref is still mapped by the foreign dom then we can't release
+	   so return error and the caller must retry */
+	else if (gnttab_query_foreign_access(*gref_id))
+		return -EAGAIN;
+	/* if we can't release the foreign access to this gref then we can't
+	   release and the caller must retry */
+	else if (!gnttab_end_foreign_access_ref(*gref_id, 0))
+		return -EAGAIN;
+
+	gnttab_free_grant_reference(*gref_id);
+	*gref_id = 0;
+	return 0;
+}
+
+static void __del_gref(struct gntalloc_gref *gref) {
+	struct gntalloc_share_list *pos, *n;
+	int pending = 0;
+
 	if (gref->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		uint8_t *tmp = kmap(gref->page);
 		tmp[gref->notify.pgoff] = 0;
@@ -190,18 +227,23 @@ static void __del_gref(struct gntalloc_gref *gref)
 		notify_remote_via_evtchn(gref->notify.event);
 		evtchn_put(gref->notify.event);
 	}
-
 	gref->notify.flags = 0;
 
-	if (gref->gref_id > 0) {
-		if (gnttab_query_foreign_access(gref->gref_id))
-			return;
-
-		if (!gnttab_end_foreign_access_ref(gref->gref_id, 0))
-			return;
-
-		gnttab_free_grant_reference(gref->gref_id);
+	list_for_each_entry_safe(pos, n, &gref->shares.next_share, next_share) {
+		if (!__release_gref(&pos->gref_id))
+		{
+			list_del(&pos->next_share);
+			kfree(pos);
+		}
+		else
+			pending++;
 	}
+	if (gref->shares.gref_id > 0)
+		pending += !!__release_gref(&gref->shares.gref_id);
+
+	/* if there are shares that are still mapped by foreign doms then we postpone page removal to do_cleanup */
+	if (pending)
+		return;
 
 	gref_size--;
 	list_del(&gref->next_gref);
@@ -435,6 +477,126 @@ static long gntalloc_ioctl_unmap_notify(struct gntalloc_file_private_data *priv,
 	return rc;
 }
 
+static long gntalloc_ioctl_share(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_share_gref op;
+	struct gntalloc_gref *gref;
+	int rc;
+	int readonly;
+	struct gntalloc_share_list *share_list;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	readonly = !(op.flags & GNTALLOC_FLAG_WRITABLE);
+	rc = -ENOMEM;
+
+	mutex_lock(&gref_mutex);
+
+	/* see if we can free up any pending page releases */
+	do_cleanup();
+
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref) {
+		rc = -ENOENT;
+		goto share_out;
+	}
+
+	/* if the initial share is assigned then create a new share list node */
+	if (gref->shares.gref_id) {
+		share_list = kzalloc(sizeof(*share_list), GFP_KERNEL);
+		if (!share_list) {
+			rc = -ENOMEM;
+			goto share_out;
+		}
+
+		share_list->gref_id = gnttab_grant_foreign_access(op.domid,
+			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+		if ((signed)share_list->gref_id < 0) {
+			kzfree(share_list);
+			rc = share_list->gref_id;
+			goto share_out;
+		}
+
+		list_add_tail(&share_list->next_share, &gref->shares.next_share);
+	}
+	else {
+		share_list = &gref->shares;
+		share_list->gref_id = gnttab_grant_foreign_access(op.domid,
+			pfn_to_mfn(page_to_pfn(gref->page)), readonly);
+		if ((signed)share_list->gref_id < 0) {
+			rc = share_list->gref_id;
+			goto share_out;
+		}
+	}
+
+	op.gref_id = share_list->gref_id;
+	rc = 0;
+
+share_out:
+	mutex_unlock(&gref_mutex);
+
+	if (!rc && copy_to_user(arg, &op, sizeof(op)))
+		rc = -EFAULT;
+
+	return rc;
+}
+
+static long gntalloc_ioctl_unshare(struct gntalloc_file_private_data *priv, void __user *arg)
+{
+	struct ioctl_gntalloc_unshare_gref op;
+	struct gntalloc_gref *gref;
+	int rc = -ENOENT;
+	struct gntalloc_share_list *pos;
+
+	if (copy_from_user(&op, arg, sizeof(op)))
+		return -EFAULT;
+
+	mutex_lock(&gref_mutex);
+	gref = find_grefs(priv, op.index, 1);
+
+	if (!gref)
+		goto unshare_out;
+
+	/* if we are releasing the initial share then replace it with the next available share.
+	   if there are no more shares then just leave the gref_id as 0 */
+	if (gref->shares.gref_id == op.gref_id) {
+		if (__release_gref(&gref->shares.gref_id))
+			rc = -EAGAIN;
+		else {
+			pos = list_first_entry(&gref->shares.next_share,
+							 struct gntalloc_share_list,
+							 next_share);
+			if (pos) {
+				gref->shares.gref_id = pos->gref_id;
+				list_del(&pos->next_share);
+				kfree(pos);
+			}
+		}
+	}
+	else {
+		list_for_each_entry(pos, &gref->shares.next_share, next_share) {
+			if (pos->gref_id == op.gref_id) {
+				if (__release_gref(&pos->gref_id))
+					rc = -EAGAIN;
+				else {
+					list_del(&pos->next_share);
+					kfree(pos);
+					rc = 0;
+				}
+				break;
+			}
+		}
+	}
+
+	do_cleanup();
+
+unshare_out:
+	mutex_unlock(&gref_mutex);
+	return rc;
+}
+
 static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -450,6 +612,12 @@ static long gntalloc_ioctl(struct file *filp, unsigned int cmd,
 	case IOCTL_GNTALLOC_SET_UNMAP_NOTIFY:
 		return gntalloc_ioctl_unmap_notify(priv, (void __user *)arg);
 
+	case IOCTL_GNTALLOC_SHARE_GREF:
+		return gntalloc_ioctl_share(priv, (void __user *)arg);
+
+	case IOCTL_GNTALLOC_UNSHARE_GREF:
+		return gntalloc_ioctl_unshare(priv, (void __user *)arg);
+
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
index 76bd580..31911da 100644
--- a/include/uapi/xen/gntalloc.h
+++ b/include/uapi/xen/gntalloc.h
@@ -79,4 +79,36 @@ struct ioctl_gntalloc_unmap_notify {
 /* Send an interrupt on the indicated event channel */
 #define UNMAP_NOTIFY_SEND_EVENT 0x2
 
+/*
+ * Shares a page that has already been allocated (so we can share the same page 
+ * between more than one domain)
+ */
+#define IOCTL_GNTALLOC_SHARE_GREF \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntalloc_share_gref))
+struct ioctl_gntalloc_share_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The ID of the domain to be given access to the grants. */
+	uint16_t domid;
+	/* Flags for this mapping */
+	uint16_t flags;
+	/* OUT parameters */
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
+/*
+ * Unshares a page that has already been shared
+ */
+#define IOCTL_GNTALLOC_UNSHARE_GREF \
+_IOC(_IOC_NONE, 'G', 9, sizeof(struct ioctl_gntalloc_unshare_gref))
+struct ioctl_gntalloc_unshare_gref {
+	/* IN parameters */
+	/* The offset used in call to mmap(). */
+	uint64_t index;
+	/* The grant references of the newly created grant */
+	uint32_t gref_id;
+};
+
 #endif /* __LINUX_PUBLIC_GNTALLOC_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel