[PATCH] Add grant references for fbfront/kbdfront

[PATCH] Add grant references for fbfront/kbdfront

[dgdegra]

This series fixes the interface for the fbfront and kbdfront devices,
which were storing MFNs in xenstore rather than creating grant table
entries. To maintain backwards compatibility, a different xenstore key
is used (page-gref instead of page-ref) and the use of grants must be
requested for fbfront (because two levels of page references are
embedded within the shared page). This makes it possible to move a
display server out of dom0 without giving the display domain full
access to other domain's memory.

[PATCH 1/3] xen-fbfront: Read width/height from backend
	Note: Not directly related, just used by display server.
[PATCH 2/3] xen-fbfront: Use grant references when requested
[PATCH 3/3] xen-kbdfront: Add grant reference for shared page

[PATCH 1/3] xen-fbfront: Read width/height from backend

[dgdegra]

From: Daniel De Graaf <dgdegra@tycho.nsa.gov>

This allows the backend driver to specify the size of the framebuffer
instead of requiring it to be on the kernel command line.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/video/xen-fbfront.c |   30 ++++++++++++++++++++++++------
 1 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index 428d273..341c919 100644
--- a/drivers/video/xen-fbfront.c
+++ b/drivers/video/xen-fbfront.c
@@ -58,7 +58,7 @@ struct xenfb_info {
 #define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8)
 
 enum { KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT };
-static int video[KPARAM_CNT] = { 2, XENFB_WIDTH, XENFB_HEIGHT };
+static int video[KPARAM_CNT] = { 0, 0, 0 };
 module_param_array(video, int, NULL, 0);
 MODULE_PARM_DESC(video,
 	"Video memory size in MB, width, height in pixels (default 2,800,600)");
@@ -363,7 +363,7 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
 {
 	struct xenfb_info *info;
 	struct fb_info *fb_info;
-	int fb_size;
+	int fb_size, fb_need;
 	int val;
 	int ret;
 
@@ -375,14 +375,32 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
 
 	/* Limit kernel param videoram amount to what is in xenstore */
 	if (xenbus_scanf(XBT_NIL, dev->otherend, "videoram", "%d", &val) == 1) {
-		if (val < video[KPARAM_MEM])
+		if (!video[KPARAM_MEM] || val < video[KPARAM_MEM])
 			video[KPARAM_MEM] = val;
 	}
 
+	/* Take width/height from xenstore if not set on command line */
+	if (!video[KPARAM_WIDTH]) {
+		if (xenbus_scanf(XBT_NIL, dev->otherend, "width", "%d", &val) == 1)
+			video[KPARAM_WIDTH] = val;
+		else
+			video[KPARAM_WIDTH] = XENFB_WIDTH;
+	}
+	if (!video[KPARAM_HEIGHT]) {
+		if (xenbus_scanf(XBT_NIL, dev->otherend, "height", "%d", &val) == 1)
+			video[KPARAM_HEIGHT] = val;
+		else
+			video[KPARAM_HEIGHT] = XENFB_HEIGHT;
+	}
+
+	fb_need = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH / 8;
+	if (video[KPARAM_MEM])
+		fb_size = video[KPARAM_MEM] * 1024 * 1024;
+	else
+		fb_size = video[KPARAM_MEM] = fb_need;
+
 	/* If requested res does not fit in available memory, use default */
-	fb_size = video[KPARAM_MEM] * 1024 * 1024;
-	if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH / 8
-	    > fb_size) {
+	if (fb_need > fb_size) {
 		video[KPARAM_WIDTH] = XENFB_WIDTH;
 		video[KPARAM_HEIGHT] = XENFB_HEIGHT;
 		fb_size = XENFB_DEFAULT_FB_LEN;
-- 
1.7.3.4

Re: [PATCH 1/3] xen-fbfront: Read width/height from backend

[Ian Campbell]

On Fri, 2011-01-07 at 16:12 +0000, dgdegra@tycho.nsa.gov wrote: 
> From: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> This allows the backend driver to specify the size of the framebuffer
> instead of requiring it to be on the kernel command line.

Do you plan to plumb this up through the toolstack so it can be defined
guest configuration file?

I wonder if it might be useful to allow the backend to specify a maximum
size but to allow the frontend to pick a resolution within that
constraint and communicate the choice to the backend.

Ian.

Re: [PATCH 1/3] xen-fbfront: Read width/height from backend

[Daniel De Graaf]

On 01/07/2011 03:47 PM, Ian Campbell wrote:
> On Fri, 2011-01-07 at 16:12 +0000, dgdegra@tycho.nsa.gov wrote: 
>> From: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>
>> This allows the backend driver to specify the size of the framebuffer
>> instead of requiring it to be on the kernel command line.
> 
> Do you plan to plumb this up through the toolstack so it can be defined
> guest configuration file?
>

Possibly, although it may end up being redundant with the kernel parameter
in the domU command line. My current use for this is to communicate a target
resolution for guests from my fixed-size display server (derived from the
monitor by the display server); this doesn't really translate for VNC,
although it would for fullscreen SDL.

> I wonder if it might be useful to allow the backend to specify a maximum
> size but to allow the frontend to pick a resolution within that
> constraint and communicate the choice to the backend.
> 
> Ian.
> 

I'm not sure I see a use for the frontend to choose a smaller resolution
than the backend suggests in some automatic way; it's certainly possible by
specifying whatever you want on the command line of the domU kernel.

-- 
Daniel De Graaf
National Security Agency

[PATCH 2/3] xen-fbfront: Use grant references when requested

[dgdegra]

From: Daniel De Graaf <dgdegra@tycho.nsa.gov>

The current version of the Xen framebuffer API passes MFNs directly
to the backend driver, which requires the backend to have full access
to this domain's memory. Add a parameter in xenbus to request the use of
grant entries instead, which are slightly slower to map but provide
inter-domain isolation.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/video/xen-fbfront.c |   73 +++++++++++++++++++++++++++++++++---------
 1 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index 341c919..bb99bbd 100644
--- a/drivers/video/xen-fbfront.c
+++ b/drivers/video/xen-fbfront.c
@@ -11,13 +11,6 @@
  *  more details.
  */
 
-/*
- * TODO:
- *
- * Switch to grant tables when they become capable of dealing with the
- * frame buffer.
- */
-
 #include <linux/console.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -32,6 +25,8 @@
 #include <xen/xen.h>
 #include <xen/events.h>
 #include <xen/page.h>
+#include <xen/grant_table.h>
+#include <xen/interface/grant_table.h>
 #include <xen/interface/io/fbif.h>
 #include <xen/interface/io/protocols.h>
 #include <xen/xenbus.h>
@@ -46,6 +41,7 @@ struct xenfb_info {
 	int			irq;
 	struct xenfb_page	*page;
 	unsigned long 		*mfns;
+	int                     page_gref;
 	int			update_wanted; /* XENFB_TYPE_UPDATE wanted */
 	int			feature_resize; /* XENFB_TYPE_RESIZE ok */
 	struct xenfb_resize	resize;		/* protected by resize_lock */
@@ -65,7 +61,7 @@ MODULE_PARM_DESC(video,
 
 static void xenfb_make_preferred_console(void);
 static int xenfb_remove(struct xenbus_device *);
-static void xenfb_init_shared_page(struct xenfb_info *, struct fb_info *);
+static void xenfb_init_shared_page(struct xenbus_device *, struct xenfb_info *, struct fb_info *);
 static int xenfb_connect_backend(struct xenbus_device *, struct xenfb_info *);
 static void xenfb_disconnect_backend(struct xenfb_info *);
 
@@ -474,7 +470,7 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
 	fb_info->fbdefio = &xenfb_defio;
 	fb_deferred_io_init(fb_info);
 
-	xenfb_init_shared_page(info, fb_info);
+	xenfb_init_shared_page(dev, info, fb_info);
 
 	ret = xenfb_connect_backend(dev, info);
 	if (ret < 0)
@@ -528,7 +524,7 @@ static int xenfb_resume(struct xenbus_device *dev)
 	struct xenfb_info *info = dev_get_drvdata(&dev->dev);
 
 	xenfb_disconnect_backend(info);
-	xenfb_init_shared_page(info, info->fb_info);
+	xenfb_init_shared_page(dev, info, info->fb_info);
 	return xenfb_connect_backend(dev, info);
 }
 
@@ -556,17 +552,58 @@ static unsigned long vmalloc_to_mfn(void *address)
 	return pfn_to_mfn(vmalloc_to_pfn(address));
 }
 
-static void xenfb_init_shared_page(struct xenfb_info *info,
+static void xenfb_init_shared_page(struct xenbus_device *dev,
+                                   struct xenfb_info *info,
 				   struct fb_info *fb_info)
 {
-	int i;
 	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
+	int be_id = dev->otherend_id;
+	int i, ref;
+	unsigned long mfn;
+	grant_ref_t gref_head;
+	int allpages = info->nr_pages + ((info->nr_pages + epd - 1) / epd) + 1;
+
+	int grants = 0;
+	xenbus_scanf(XBT_NIL, dev->otherend, "use-grants", "%d", &grants);
+
+	if (grants) {
+		int err = gnttab_alloc_grant_references(allpages, &gref_head);
+		if (err < 0) {
+			xenbus_dev_fatal(dev, err, "fbdev grant refs");
+			info->page_gref = -ENOSPC;
+		} else {
+			ref = gnttab_claim_grant_reference(&gref_head);
+			mfn = virt_to_mfn(info->page);
+			BUG_ON(ref == -ENOSPC);
+			gnttab_grant_foreign_access_ref(ref, be_id, mfn, 0);
+			info->page_gref = ref;
+		}
+	} else
+		info->page_gref = -ENOENT;
 
 	for (i = 0; i < info->nr_pages; i++)
-		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
+	{
+		mfn = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
+		if (grants) {
+			ref = gnttab_claim_grant_reference(&gref_head);
+			BUG_ON(ref == -ENOSPC);
+			gnttab_grant_foreign_access_ref(ref, be_id, mfn, 1);
+			info->mfns[i] = ref;
+		} else
+			info->mfns[i] = mfn;
+	}
 
 	for (i = 0; i * epd < info->nr_pages; i++)
-		info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i * epd]);
+	{
+		mfn = vmalloc_to_mfn(&info->mfns[i * epd]);
+		if (grants) {
+			ref = gnttab_claim_grant_reference(&gref_head);
+			BUG_ON(ref == -ENOSPC);
+			gnttab_grant_foreign_access_ref(ref, be_id, mfn, 1);
+			info->page->pd[i] = ref;
+		} else
+			info->page->pd[i] = mfn;
+	}
 
 	info->page->width = fb_info->var.xres;
 	info->page->height = fb_info->var.yres;
@@ -601,8 +638,12 @@ static int xenfb_connect_backend(struct xenbus_device *dev,
 		xenbus_dev_fatal(dev, ret, "starting transaction");
 		return ret;
 	}
-	ret = xenbus_printf(xbt, dev->nodename, "page-ref", "%lu",
-			    virt_to_mfn(info->page));
+	if (info->page_gref < 0)
+		ret = xenbus_printf(xbt, dev->nodename, "page-ref", "%lu",
+			            virt_to_mfn(info->page));
+	else
+		ret = xenbus_printf(xbt, dev->nodename, "page-gref", "%u",
+				    info->page_gref);
 	if (ret)
 		goto error_xenbus;
 	ret = xenbus_printf(xbt, dev->nodename, "event-channel", "%u",
-- 
1.7.3.4

Re: [PATCH 2/3] xen-fbfront: Use grant references when requested

[Ian Campbell]

On Fri, 2011-01-07 at 16:12 +0000, dgdegra@tycho.nsa.gov wrote: 
> @@ -556,17 +552,58 @@ static unsigned long vmalloc_to_mfn(void *address)
>  	return pfn_to_mfn(vmalloc_to_pfn(address));
>  }
>  
> -static void xenfb_init_shared_page(struct xenfb_info *info,
> +static void xenfb_init_shared_page(struct xenbus_device *dev,
> +                                   struct xenfb_info *info,
>  				   struct fb_info *fb_info)
>  {
> -	int i;
>  	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
> +	int be_id = dev->otherend_id;
> +	int i, ref;
> +	unsigned long mfn;
> +	grant_ref_t gref_head;
> +	int allpages = info->nr_pages + ((info->nr_pages + epd - 1) / epd) + 1;
> +
> +	int grants = 0;
> +	xenbus_scanf(XBT_NIL, dev->otherend, "use-grants", "%d", &grants);

There doesn't seem to be any negotiation with the backend about whether
or not grants should be used so there is no way for a backend to know if
it can choose to set this flag or not, granted not all backends will
have a choice due to their privilege level.

More importantly there is also no way for the backend to figure out is
the frontend is going to obey the request if it does write it (at least
until it tries to map a gref and fails because its really got an mfn).

Usually both front and backend would write a feature-foo node to their
respective directory in xenstore and then figure out what to do based on
what the other end wrote.

In the kbdfront patch you simply write both the mfn and the grant
reference to xenstore, presumably the backend then just picks for itself
which access method to use, could that approach be applicable here?

There seems to be slack in xenfb_page which could accommodate a second
pd array containing grefs for the pages. The presence of a page-gref
node in xenstore would indicate that the larger structure with the grefs
is in use.

Ian.

Re: [PATCH 2/3] xen-fbfront: Use grant references when requested

[Daniel De Graaf]

On 01/07/2011 03:44 PM, Ian Campbell wrote:
> On Fri, 2011-01-07 at 16:12 +0000, dgdegra@tycho.nsa.gov wrote: 
>> @@ -556,17 +552,58 @@ static unsigned long vmalloc_to_mfn(void *address)
>>  	return pfn_to_mfn(vmalloc_to_pfn(address));
>>  }
>>  
>> -static void xenfb_init_shared_page(struct xenfb_info *info,
>> +static void xenfb_init_shared_page(struct xenbus_device *dev,
>> +                                   struct xenfb_info *info,
>>  				   struct fb_info *fb_info)
>>  {
>> -	int i;
>>  	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
>> +	int be_id = dev->otherend_id;
>> +	int i, ref;
>> +	unsigned long mfn;
>> +	grant_ref_t gref_head;
>> +	int allpages = info->nr_pages + ((info->nr_pages + epd - 1) / epd) + 1;
>> +
>> +	int grants = 0;
>> +	xenbus_scanf(XBT_NIL, dev->otherend, "use-grants", "%d", &grants);
> 
> There doesn't seem to be any negotiation with the backend about whether
> or not grants should be used so there is no way for a backend to know if
> it can choose to set this flag or not, granted not all backends will
> have a choice due to their privilege level.

The "use-grants" flag is a request by the backend; older versions will ignore
it, newer versions (may) honor it.
 
> More importantly there is also no way for the backend to figure out is
> the frontend is going to obey the request if it does write it (at least
> until it tries to map a gref and fails because its really got an mfn).

The key used in xenstore (page-gref versus page-ref) tells the backend if
the request was honored. If the backend is unable to map MFNs, then it will
fail if page-gref is not present; otherwise, it can fall back to the old
MFN mapping.
 
> Usually both front and backend would write a feature-foo node to their
> respective directory in xenstore and then figure out what to do based on
> what the other end wrote.

Perhaps "use-grants" should be renamed to "feature-grants"? I thought the
frontend writing a feature-grants node would be redundant since the
page-gref node already communicated what was chosen.
 
> In the kbdfront patch you simply write both the mfn and the grant
> reference to xenstore, presumably the backend then just picks for itself
> which access method to use, could that approach be applicable here?

Possible, but it would waste a bit of memory (a few extra pages for the
MFN tables in addition to the grant-ref tables). If this isn't an issue,
then fbfront can expose both and allow the backend to choose without any
negotiation. Is this preferred?
 
> There seems to be slack in xenfb_page which could accommodate a second
> pd array containing grefs for the pages. The presence of a page-gref
> node in xenstore would indicate that the larger structure with the grefs
> is in use.
> 
> Ian.
> 

Yes, there is space in xenfb_page, since grants are always 32-bit numbers.
The offset will be different depending on sizeof(unsigned long) in the guest,
due to bad design of the shared page, but that's not hard to work around.
Alternatively, we could put in some padding to fix the offset.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 2/3] xen-fbfront: Use grant references when requested

[Daniel De Graaf]

On 01/07/2011 04:02 PM, Daniel De Graaf wrote:
> On 01/07/2011 03:44 PM, Ian Campbell wrote:
>> On Fri, 2011-01-07 at 16:12 +0000, dgdegra@tycho.nsa.gov wrote: 
>>> @@ -556,17 +552,58 @@ static unsigned long vmalloc_to_mfn(void *address)
>>>  	return pfn_to_mfn(vmalloc_to_pfn(address));
>>>  }
>>>  
>>> -static void xenfb_init_shared_page(struct xenfb_info *info,
>>> +static void xenfb_init_shared_page(struct xenbus_device *dev,
>>> +                                   struct xenfb_info *info,
>>>  				   struct fb_info *fb_info)
>>>  {
>>> -	int i;
>>>  	int epd = PAGE_SIZE / sizeof(info->mfns[0]);
>>> +	int be_id = dev->otherend_id;
>>> +	int i, ref;
>>> +	unsigned long mfn;
>>> +	grant_ref_t gref_head;
>>> +	int allpages = info->nr_pages + ((info->nr_pages + epd - 1) / epd) + 1;
>>> +
>>> +	int grants = 0;
>>> +	xenbus_scanf(XBT_NIL, dev->otherend, "use-grants", "%d", &grants);
>>
>> There doesn't seem to be any negotiation with the backend about whether
>> or not grants should be used so there is no way for a backend to know if
>> it can choose to set this flag or not, granted not all backends will
>> have a choice due to their privilege level.
> 
> The "use-grants" flag is a request by the backend; older versions will ignore
> it, newer versions (may) honor it.
>  
>> More importantly there is also no way for the backend to figure out is
>> the frontend is going to obey the request if it does write it (at least
>> until it tries to map a gref and fails because its really got an mfn).
> 
> The key used in xenstore (page-gref versus page-ref) tells the backend if
> the request was honored. If the backend is unable to map MFNs, then it will
> fail if page-gref is not present; otherwise, it can fall back to the old
> MFN mapping.
>  
>> Usually both front and backend would write a feature-foo node to their
>> respective directory in xenstore and then figure out what to do based on
>> what the other end wrote.
> 
> Perhaps "use-grants" should be renamed to "feature-grants"? I thought the
> frontend writing a feature-grants node would be redundant since the
> page-gref node already communicated what was chosen.
>  
>> In the kbdfront patch you simply write both the mfn and the grant
>> reference to xenstore, presumably the backend then just picks for itself
>> which access method to use, could that approach be applicable here?
> 
> Possible, but it would waste a bit of memory (a few extra pages for the
> MFN tables in addition to the grant-ref tables). If this isn't an issue,
> then fbfront can expose both and allow the backend to choose without any
> negotiation. Is this preferred?
>  
>> There seems to be slack in xenfb_page which could accommodate a second
>> pd array containing grefs for the pages. The presence of a page-gref
>> node in xenstore would indicate that the larger structure with the grefs
>> is in use.
>>
>> Ian.
>>
> 
> Yes, there is space in xenfb_page, since grants are always 32-bit numbers.
> The offset will be different depending on sizeof(unsigned long) in the guest,
> due to bad design of the shared page, but that's not hard to work around.
> Alternatively, we could put in some padding to fix the offset.
> 

Actually, after looking at xenfb_page again, there isn't room, and the current
structure in the linux kernel is very misleading. Offset 1024 in the structure
is the start of the incoming ring buffer, and offset 2048 is the outgoing ring
buffer. The existing pd array overlaps both of these ring buffers (on 64-bit;
on 32-bit it only overlaps the incoming ring).

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH 2/3] xen-fbfront: Use grant references when requested

[Ian Campbell]

On Fri, 2011-01-07 at 21:02 +0000, Daniel De Graaf wrote: 
> On 01/07/2011 03:44 PM, Ian Campbell wrote:
> > More importantly there is also no way for the backend to figure out is
> > the frontend is going to obey the request if it does write it (at least
> > until it tries to map a gref and fails because its really got an mfn).
> 
> The key used in xenstore (page-gref versus page-ref) tells the backend if
> the request was honored.

Oh yes, of course. I even suggested the same thing myself later on...

> > Usually both front and backend would write a feature-foo node to their
> > respective directory in xenstore and then figure out what to do based on
> > what the other end wrote.
> 
> Perhaps "use-grants" should be renamed to "feature-grants"?

I think it would be more consistent with the other protocols (and the
existing feature-update in the fb protocol) to do so. Also use-grants
sounds a bit like a command rather than a suggestion to me.

> I thought the
> frontend writing a feature-grants node would be redundant since the
> page-gref node already communicated what was chosen.

Agreed.

> > In the kbdfront patch you simply write both the mfn and the grant
> > reference to xenstore, presumably the backend then just picks for itself
> > which access method to use, could that approach be applicable here?
> 
> Possible, but it would waste a bit of memory (a few extra pages for the
> MFN tables in addition to the grant-ref tables). If this isn't an issue,
> then fbfront can expose both and allow the backend to choose without any
> negotiation. Is this preferred?

Since you've already got the negotiation aspect covered I don't think
there is any need go down this path.

Ian.

[PATCH 3/3] xen-kbdfront: Add grant reference for shared page

[dgdegra]

From: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Without a grant reference, full access to the domain's memory is
required to use the shared page. Add an additional parameter in
xenstore to allow grant mapping to be used.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 drivers/input/xen-kbdfront.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
index e0c024d..a5d0656 100644
--- a/drivers/input/xen-kbdfront.c
+++ b/drivers/input/xen-kbdfront.c
@@ -11,12 +11,6 @@
  *  more details.
  */
 
-/*
- * TODO:
- *
- * Switch to grant tables together with xen-fbfront.c.
- */
-
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/module.h>
@@ -28,6 +22,8 @@
 #include <xen/xen.h>
 #include <xen/events.h>
 #include <xen/page.h>
+#include <xen/grant_table.h>
+#include <xen/interface/grant_table.h>
 #include <xen/interface/io/fbif.h>
 #include <xen/interface/io/kbdif.h>
 #include <xen/xenbus.h>
@@ -36,6 +32,7 @@ struct xenkbd_info {
 	struct input_dev *kbd;
 	struct input_dev *ptr;
 	struct xenkbd_page *page;
+	int gref;
 	int irq;
 	struct xenbus_device *xbdev;
 	char phys[32];
@@ -121,6 +118,7 @@ static int __devinit xenkbd_probe(struct xenbus_device *dev,
 	dev_set_drvdata(&dev->dev, info);
 	info->xbdev = dev;
 	info->irq = -1;
+	info->gref = -1;
 	snprintf(info->phys, sizeof(info->phys), "xenbus/%s", dev->nodename);
 
 	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
@@ -217,15 +215,20 @@ static int xenkbd_connect_backend(struct xenbus_device *dev,
 	int ret, evtchn;
 	struct xenbus_transaction xbt;
 
+	ret = gnttab_grant_foreign_access(dev->otherend_id,
+	                                  virt_to_mfn(info->page), 0);
+	if (ret < 0)
+		return ret;
+	info->gref = ret;
+
 	ret = xenbus_alloc_evtchn(dev, &evtchn);
 	if (ret)
-		return ret;
+		goto error_grant;
 	ret = bind_evtchn_to_irqhandler(evtchn, input_handler,
 					0, dev->devicetype, info);
 	if (ret < 0) {
-		xenbus_free_evtchn(dev, evtchn);
 		xenbus_dev_fatal(dev, ret, "bind_evtchn_to_irqhandler");
-		return ret;
+		goto error_evtchan;
 	}
 	info->irq = ret;
 
@@ -233,12 +236,15 @@ static int xenkbd_connect_backend(struct xenbus_device *dev,
 	ret = xenbus_transaction_start(&xbt);
 	if (ret) {
 		xenbus_dev_fatal(dev, ret, "starting transaction");
-		return ret;
+		goto error_irqh;
 	}
 	ret = xenbus_printf(xbt, dev->nodename, "page-ref", "%lu",
 			    virt_to_mfn(info->page));
 	if (ret)
 		goto error_xenbus;
+	ret = xenbus_printf(xbt, dev->nodename, "page-gref", "%u", info->gref);
+	if (ret)
+		goto error_xenbus;
 	ret = xenbus_printf(xbt, dev->nodename, "event-channel", "%u",
 			    evtchn);
 	if (ret)
@@ -248,7 +254,7 @@ static int xenkbd_connect_backend(struct xenbus_device *dev,
 		if (ret == -EAGAIN)
 			goto again;
 		xenbus_dev_fatal(dev, ret, "completing transaction");
-		return ret;
+		goto error_irqh;
 	}
 
 	xenbus_switch_state(dev, XenbusStateInitialised);
@@ -257,6 +263,14 @@ static int xenkbd_connect_backend(struct xenbus_device *dev,
  error_xenbus:
 	xenbus_transaction_end(xbt, 1);
 	xenbus_dev_fatal(dev, ret, "writing xenstore");
+ error_irqh:
+	unbind_from_irqhandler(info->irq, info);
+	info->irq = -1;
+ error_evtchan:
+	xenbus_free_evtchn(dev, evtchn);
+ error_grant:
+	gnttab_end_foreign_access_ref(info->gref, 0);
+	info->gref = -1;
 	return ret;
 }
 
@@ -265,6 +279,9 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *info)
 	if (info->irq >= 0)
 		unbind_from_irqhandler(info->irq, info);
 	info->irq = -1;
+	if (info->gref >= 0)
+		gnttab_end_foreign_access_ref(info->gref, 0);
+	info->gref = -1;
 }
 
 static void xenkbd_backend_changed(struct xenbus_device *dev,
-- 
1.7.3.4

Re: [PATCH] Add grant references for fbfront/kbdfront

[Konrad Rzeszutek Wilk]

On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
> This series fixes the interface for the fbfront and kbdfront devices,
> which were storing MFNs in xenstore rather than creating grant table
> entries. To maintain backwards compatibility, a different xenstore key
> is used (page-gref instead of page-ref) and the use of grants must be
> requested for fbfront (because two levels of page references are
> embedded within the shared page). This makes it possible to move a
> display server out of dom0 without giving the display domain full
> access to other domain's memory.

So these patches go on top of  xen/stable-2.6.32 tree right?

> 
> [PATCH 1/3] xen-fbfront: Read width/height from backend
> 	Note: Not directly related, just used by display server.
> [PATCH 2/3] xen-fbfront: Use grant references when requested
> [PATCH 3/3] xen-kbdfront: Add grant reference for shared page
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] Add grant references for fbfront/kbdfront

[Daniel De Graaf]

On 01/07/2011 11:51 AM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
>> This series fixes the interface for the fbfront and kbdfront devices,
>> which were storing MFNs in xenstore rather than creating grant table
>> entries. To maintain backwards compatibility, a different xenstore key
>> is used (page-gref instead of page-ref) and the use of grants must be
>> requested for fbfront (because two levels of page references are
>> embedded within the shared page). This makes it possible to move a
>> display server out of dom0 without giving the display domain full
>> access to other domain's memory.
> 
> So these patches go on top of  xen/stable-2.6.32 tree right?
>

They apply to either 2.6.32 or 2.6.37; I have tested them on both.

>>
>> [PATCH 1/3] xen-fbfront: Read width/height from backend
>> 	Note: Not directly related, just used by display server.
>> [PATCH 2/3] xen-fbfront: Use grant references when requested
>> [PATCH 3/3] xen-kbdfront: Add grant reference for shared page
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 


-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] Add grant references for fbfront/kbdfront

[Ian Campbell]

On Fri, 2011-01-07 at 17:55 +0000, Daniel De Graaf wrote: 
> On 01/07/2011 11:51 AM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
> >> This series fixes the interface for the fbfront and kbdfront devices,
> >> which were storing MFNs in xenstore rather than creating grant table
> >> entries. To maintain backwards compatibility, a different xenstore key
> >> is used (page-gref instead of page-ref) and the use of grants must be
> >> requested for fbfront (because two levels of page references are
> >> embedded within the shared page). This makes it possible to move a
> >> display server out of dom0 without giving the display domain full
> >> access to other domain's memory.
> > 
> > So these patches go on top of  xen/stable-2.6.32 tree right?
> >
> 
> They apply to either 2.6.32 or 2.6.37; I have tested them on both.

Are there matching backend patches somewhere?

Ian.

> 
> >>
> >> [PATCH 1/3] xen-fbfront: Read width/height from backend
> >> 	Note: Not directly related, just used by display server.
> >> [PATCH 2/3] xen-fbfront: Use grant references when requested
> >> [PATCH 3/3] xen-kbdfront: Add grant reference for shared page
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xensource.com
> >> http://lists.xensource.com/xen-devel
> > 
> 
> 

Re: [PATCH] Add grant references for fbfront/kbdfront

[Daniel De Graaf]

On 01/07/2011 01:28 PM, Ian Campbell wrote:
> On Fri, 2011-01-07 at 17:55 +0000, Daniel De Graaf wrote: 
>> On 01/07/2011 11:51 AM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
>>>> This series fixes the interface for the fbfront and kbdfront devices,
>>>> which were storing MFNs in xenstore rather than creating grant table
>>>> entries. To maintain backwards compatibility, a different xenstore key
>>>> is used (page-gref instead of page-ref) and the use of grants must be
>>>> requested for fbfront (because two levels of page references are
>>>> embedded within the shared page). This makes it possible to move a
>>>> display server out of dom0 without giving the display domain full
>>>> access to other domain's memory.
>>>
>>> So these patches go on top of  xen/stable-2.6.32 tree right?
>>>
>>
>> They apply to either 2.6.32 or 2.6.37; I have tested them on both.
> 
> Are there matching backend patches somewhere?
> 
> Ian.
> 

Not yet, although it shouldn't be hard to modify the Xen backend
to use grants. This has been tested using a custom display server.

>>>>
>>>> [PATCH 1/3] xen-fbfront: Read width/height from backend
>>>> 	Note: Not directly related, just used by display server.
>>>> [PATCH 2/3] xen-fbfront: Use grant references when requested
>>>> [PATCH 3/3] xen-kbdfront: Add grant reference for shared page
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xensource.com
>>>> http://lists.xensource.com/xen-devel
>>>
>>

Re: [PATCH] Add grant references for fbfront/kbdfront

[Ian Campbell]

On Fri, 2011-01-07 at 19:36 +0000, Daniel De Graaf wrote: 
> On 01/07/2011 01:28 PM, Ian Campbell wrote:
> > Are there matching backend patches somewhere?

> Not yet, although it shouldn't be hard to modify the Xen backend
> to use grants.

Is this something you are working on? It's a bit hard to evaluate the
frontend changes without the equivalent backend patches.

Ian.

Re: [PATCH] Add grant references for fbfront/kbdfront

[Daniel De Graaf]

On 01/07/2011 03:44 PM, Ian Campbell wrote:
> On Fri, 2011-01-07 at 19:36 +0000, Daniel De Graaf wrote: 
>> On 01/07/2011 01:28 PM, Ian Campbell wrote:
>>> Are there matching backend patches somewhere?
> 
>> Not yet, although it shouldn't be hard to modify the Xen backend
>> to use grants.
> 
> Is this something you are working on? It's a bit hard to evaluate the
> frontend changes without the equivalent backend patches.
> 
> Ian.
> 

Yes, I just need to port my changes to xenfb.c and friends from the
display server that I'm using.

-- 
Daniel De Graaf
National Security Agency

[PATCH qemu] Support grant references in mapping

[Daniel De Graaf]

This is untested in qemu, but is the same patch as was applied to the
server I have tested. It applies to the ioemu-remote that I am using.

---

Request and support using grant references in backends for
the keyboard and framebuffer.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

---

diff --git a/hw/xenfb.c b/hw/xenfb.c
index 97960ba..c8d7a3e 100644
--- a/hw/xenfb.c
+++ b/hw/xenfb.c
@@ -61,6 +61,7 @@ struct common {
     struct XenDevice  xendev;  /* must be first */
     void              *page;
     DisplayState      *ds;
+    int               uses_gref;
 };
 
 struct XenInput {
@@ -100,22 +101,27 @@ struct XenFB {
 
 static int common_bind(struct common *c)
 {
-    int mfn;
+    int ref;
 
-    if (xenstore_read_fe_int(&c->xendev, "page-ref", &mfn) == -1)
-	return -1;
     if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
 	return -1;
 
-    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
-				   XC_PAGE_SIZE,
-				   PROT_READ | PROT_WRITE, mfn);
+    if (xenstore_read_fe_int(&c->xendev, "page-gref", &ref) == 0) {
+	c->page = xc_gnttab_map_grant_ref(xen_xc, c->xendev.gnttabdev, c->xendev.dom, ref, PROT_READ | PROT_WRITE);
+	c->uses_gref = 1;
+    } else if (xenstore_read_fe_int(&c->xendev, "page-ref", &ref) == 0) {
+	xen_pfn_t pfn = ref;
+	c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom, PROT_READ | PROT_WRITE, &pfn, 1);
+	c->uses_gref = 0;
+    } else
+	return -1;
+
     if (c->page == NULL)
 	return -1;
 
     xen_be_bind_evtchn(&c->xendev);
-    xen_be_printf(&c->xendev, 1, "ring mfn %d, remote-port %d, local-port %d\n",
-		  mfn, c->xendev.remote_port, c->xendev.local_port);
+    xen_be_printf(&c->xendev, 1, "ring ref %d, remote-port %d, local-port %d\n",
+		  ref, c->xendev.remote_port, c->xendev.local_port);
 
     return 0;
 }
@@ -123,10 +129,12 @@ static int common_bind(struct common *c)
 static void common_unbind(struct common *c)
 {
     xen_be_unbind_evtchn(&c->xendev);
-    if (c->page) {
+    if (c->page && c->uses_gref) {
+	xc_gnttab_munmap(xen_xc, c->xendev.gnttabdev, c->page, 1);
+    } else if (c->page) {
 	munmap(c->page, XC_PAGE_SIZE);
-	c->page = NULL;
     }
+    c->page = NULL;
 }
 
 /* -------------------------------------------------------------------- */
@@ -422,8 +430,6 @@ static int xenfb_map_fb(struct XenFB *xenfb)
     struct xenfb_page *page = xenfb->c.page;
     char *protocol = xenfb->c.xendev.protocol;
     int n_fbdirs;
-    unsigned long *pgmfns = NULL;
-    unsigned long *fbmfns = NULL;
     void *map, *pd;
     int mode, ret = -1;
 
@@ -472,36 +478,72 @@ static int xenfb_map_fb(struct XenFB *xenfb)
 #endif
     }
 
-    if (xenfb->pixels) {
+    if (xenfb->pixels && xenfb->c.uses_gref) {
+	xc_gnttab_munmap(xen_xc, xenfb->c.xendev.gnttabdev, xenfb->pixels, xenfb->fbpages);
+    } else if (xenfb->pixels) {
         munmap(xenfb->pixels, xenfb->fbpages * XC_PAGE_SIZE);
-        xenfb->pixels = NULL;
     }
+    xenfb->pixels = NULL;
 
     xenfb->fbpages = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
     n_fbdirs = xenfb->fbpages * mode / 8;
     n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
 
-    pgmfns = qemu_mallocz(sizeof(unsigned long) * n_fbdirs);
-    fbmfns = qemu_mallocz(sizeof(unsigned long) * xenfb->fbpages);
+    if (xenfb->c.uses_gref) {
+	uint32_t* domids = qemu_mallocz(sizeof(uint32_t)*n_fbdirs);
+	uint32_t* refs = qemu_mallocz(sizeof(uint32_t)*n_fbdirs);
+	int i;
+	for(i=0; i < n_fbdirs; i++)
+	{
+	    domids[i] = xenfb->c.xendev.dom;
+	    refs[i] = (mode == 32) ? ((uint32_t*)pd)[i] : ((uint64_t*)pd)[i];
+	}
 
-    xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
-    map = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
-			       PROT_READ, pgmfns, n_fbdirs);
-    if (map == NULL)
-	goto out;
-    xenfb_copy_mfns(mode, xenfb->fbpages, fbmfns, map);
-    munmap(map, n_fbdirs * XC_PAGE_SIZE);
+	map = xc_gnttab_map_grant_refs(xen_xc, xenfb->c.xendev.gnttabdev,
+	                               n_fbdirs, domids, refs, PROT_READ);
+	qemu_free(domids);
+	qemu_free(refs);
+
+	if (map == NULL)
+	    goto out;
+
+	domids = qemu_mallocz(sizeof(uint32_t)*xenfb->fbpages);
+	refs = qemu_mallocz(sizeof(uint32_t)*xenfb->fbpages);
+	for(i=0; i < xenfb->fbpages; i++)
+	{
+	    domids[i] = xenfb->c.xendev.dom;
+	    refs[i] = (mode == 32) ? ((uint32_t*)map)[i] : ((uint64_t*)map)[i];
+	}
+
+	xc_gnttab_munmap(xen_xc, xenfb->c.xendev.gnttabdev, map, n_fbdirs);
+	xenfb->pixels = xc_gnttab_map_grant_refs(xen_xc, xenfb->c.xendev.gnttabdev,
+				xenfb->fbpages, domids, refs, PROT_READ);
+	qemu_free(domids);
+	qemu_free(refs);
+    } else {
+	unsigned long *pgmfns = qemu_mallocz(sizeof(unsigned long) * n_fbdirs);
+	xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
+	map = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
+				   PROT_READ, pgmfns, n_fbdirs);
+	qemu_free(pgmfns);
+
+	if (map == NULL)
+	    goto out;
+
+	unsigned long *fbmfns = qemu_mallocz(sizeof(unsigned long) * xenfb->fbpages);
+	xenfb_copy_mfns(mode, xenfb->fbpages, fbmfns, map);
+	munmap(map, n_fbdirs * XC_PAGE_SIZE);
+	xenfb->pixels = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
+					     PROT_READ | PROT_WRITE, fbmfns, xenfb->fbpages);
+	qemu_free(fbmfns);
+    }
 
-    xenfb->pixels = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
-					 PROT_READ | PROT_WRITE, fbmfns, xenfb->fbpages);
     if (xenfb->pixels == NULL)
 	goto out;
 
     ret = 0; /* all is fine */
 
 out:
-    qemu_free(pgmfns);
-    qemu_free(fbmfns);
     return ret;
 }
 
@@ -860,6 +902,8 @@ static int fb_init(struct XenDevice *xendev)
 #ifdef XENFB_TYPE_RESIZE
     xenstore_write_be_int(xendev, "feature-resize", 1);
 #endif
+    xenstore_write_be_int(xendev, "use-grants", 1);
+
     return 0;
 }
 
@@ -913,6 +957,13 @@ static void fb_disconnect(struct XenDevice *xendev)
 {
     struct XenFB *fb = container_of(xendev, struct XenFB, c.xendev);
 
+    if (fb->pixels && fb->c.uses_gref) {
+	xc_gnttab_munmap(xen_xc, fb->c.xendev.gnttabdev, fb->pixels, fb->fbpages);
+    } else if (fb->pixels) {
+	// Note: not needed if we are doing the mmap() below
+	// munmap(fb->pixels, fb->fbpages * XC_PAGE_SIZE);
+    }
+
     /*
      * FIXME: qemu can't un-init gfx display (yet?).
      *   Replacing the framebuffer with anonymous shared memory

Re: [PATCH qemu] Support grant references in mapping

[Ian Jackson]

Daniel De Graaf writes ("[Xen-devel] [PATCH qemu] Support grant references in mapping"):
> This is untested in qemu, but is the same patch as was applied to the
> server I have tested. It applies to the ioemu-remote that I am using.

Thanks for this.  I'll be looking for an ack from Stefano at least.

However, as a new feature this comes too late for the Xen 4.1 release.
If the conversation isn't still going on then, can you make sure you
repost it when we make the 4.1 release branch, at which point we'll be
able to apply it to the xen-unstable which will become 4.2.

Ian.

Re: [PATCH qemu] Support grant references in mapping

[Konrad Rzeszutek Wilk]

On Fri, Jan 07, 2011 at 05:09:40PM -0500, Daniel De Graaf wrote:
> This is untested in qemu, but is the same patch as was applied to the
> server I have tested. It applies to the ioemu-remote that I am using.

I lost track of this patchset. I think we need to first have somebody
familiar with QEMU review these patches.

CC-ing Stefano, Ian, Anthony
> 
> ---
> 
> Request and support using grant references in backends for
> the keyboard and framebuffer.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> ---
> 
> diff --git a/hw/xenfb.c b/hw/xenfb.c
> index 97960ba..c8d7a3e 100644
> --- a/hw/xenfb.c
> +++ b/hw/xenfb.c
> @@ -61,6 +61,7 @@ struct common {
>      struct XenDevice  xendev;  /* must be first */
>      void              *page;
>      DisplayState      *ds;
> +    int               uses_gref;
>  };
>  
>  struct XenInput {
> @@ -100,22 +101,27 @@ struct XenFB {
>  
>  static int common_bind(struct common *c)
>  {
> -    int mfn;
> +    int ref;
>  
> -    if (xenstore_read_fe_int(&c->xendev, "page-ref", &mfn) == -1)
> -	return -1;
>      if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
>  	return -1;
>  
> -    c->page = xc_map_foreign_range(xen_xc, c->xendev.dom,
> -				   XC_PAGE_SIZE,
> -				   PROT_READ | PROT_WRITE, mfn);
> +    if (xenstore_read_fe_int(&c->xendev, "page-gref", &ref) == 0) {
> +	c->page = xc_gnttab_map_grant_ref(xen_xc, c->xendev.gnttabdev, c->xendev.dom, ref, PROT_READ | PROT_WRITE);
> +	c->uses_gref = 1;
> +    } else if (xenstore_read_fe_int(&c->xendev, "page-ref", &ref) == 0) {
> +	xen_pfn_t pfn = ref;
> +	c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom, PROT_READ | PROT_WRITE, &pfn, 1);
> +	c->uses_gref = 0;
> +    } else
> +	return -1;
> +
>      if (c->page == NULL)
>  	return -1;
>  
>      xen_be_bind_evtchn(&c->xendev);
> -    xen_be_printf(&c->xendev, 1, "ring mfn %d, remote-port %d, local-port %d\n",
> -		  mfn, c->xendev.remote_port, c->xendev.local_port);
> +    xen_be_printf(&c->xendev, 1, "ring ref %d, remote-port %d, local-port %d\n",
> +		  ref, c->xendev.remote_port, c->xendev.local_port);
>  
>      return 0;
>  }
> @@ -123,10 +129,12 @@ static int common_bind(struct common *c)
>  static void common_unbind(struct common *c)
>  {
>      xen_be_unbind_evtchn(&c->xendev);
> -    if (c->page) {
> +    if (c->page && c->uses_gref) {
> +	xc_gnttab_munmap(xen_xc, c->xendev.gnttabdev, c->page, 1);
> +    } else if (c->page) {
>  	munmap(c->page, XC_PAGE_SIZE);
> -	c->page = NULL;
>      }
> +    c->page = NULL;
>  }
>  
>  /* -------------------------------------------------------------------- */
> @@ -422,8 +430,6 @@ static int xenfb_map_fb(struct XenFB *xenfb)
>      struct xenfb_page *page = xenfb->c.page;
>      char *protocol = xenfb->c.xendev.protocol;
>      int n_fbdirs;
> -    unsigned long *pgmfns = NULL;
> -    unsigned long *fbmfns = NULL;
>      void *map, *pd;
>      int mode, ret = -1;
>  
> @@ -472,36 +478,72 @@ static int xenfb_map_fb(struct XenFB *xenfb)
>  #endif
>      }
>  
> -    if (xenfb->pixels) {
> +    if (xenfb->pixels && xenfb->c.uses_gref) {
> +	xc_gnttab_munmap(xen_xc, xenfb->c.xendev.gnttabdev, xenfb->pixels, xenfb->fbpages);
> +    } else if (xenfb->pixels) {
>          munmap(xenfb->pixels, xenfb->fbpages * XC_PAGE_SIZE);
> -        xenfb->pixels = NULL;
>      }
> +    xenfb->pixels = NULL;
>  
>      xenfb->fbpages = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>      n_fbdirs = xenfb->fbpages * mode / 8;
>      n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>  
> -    pgmfns = qemu_mallocz(sizeof(unsigned long) * n_fbdirs);
> -    fbmfns = qemu_mallocz(sizeof(unsigned long) * xenfb->fbpages);
> +    if (xenfb->c.uses_gref) {
> +	uint32_t* domids = qemu_mallocz(sizeof(uint32_t)*n_fbdirs);
> +	uint32_t* refs = qemu_mallocz(sizeof(uint32_t)*n_fbdirs);
> +	int i;
> +	for(i=0; i < n_fbdirs; i++)
> +	{
> +	    domids[i] = xenfb->c.xendev.dom;
> +	    refs[i] = (mode == 32) ? ((uint32_t*)pd)[i] : ((uint64_t*)pd)[i];
> +	}
>  
> -    xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
> -    map = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
> -			       PROT_READ, pgmfns, n_fbdirs);
> -    if (map == NULL)
> -	goto out;
> -    xenfb_copy_mfns(mode, xenfb->fbpages, fbmfns, map);
> -    munmap(map, n_fbdirs * XC_PAGE_SIZE);
> +	map = xc_gnttab_map_grant_refs(xen_xc, xenfb->c.xendev.gnttabdev,
> +	                               n_fbdirs, domids, refs, PROT_READ);
> +	qemu_free(domids);
> +	qemu_free(refs);
> +
> +	if (map == NULL)
> +	    goto out;
> +
> +	domids = qemu_mallocz(sizeof(uint32_t)*xenfb->fbpages);
> +	refs = qemu_mallocz(sizeof(uint32_t)*xenfb->fbpages);
> +	for(i=0; i < xenfb->fbpages; i++)
> +	{
> +	    domids[i] = xenfb->c.xendev.dom;
> +	    refs[i] = (mode == 32) ? ((uint32_t*)map)[i] : ((uint64_t*)map)[i];
> +	}
> +
> +	xc_gnttab_munmap(xen_xc, xenfb->c.xendev.gnttabdev, map, n_fbdirs);
> +	xenfb->pixels = xc_gnttab_map_grant_refs(xen_xc, xenfb->c.xendev.gnttabdev,
> +				xenfb->fbpages, domids, refs, PROT_READ);
> +	qemu_free(domids);
> +	qemu_free(refs);
> +    } else {
> +	unsigned long *pgmfns = qemu_mallocz(sizeof(unsigned long) * n_fbdirs);
> +	xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
> +	map = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
> +				   PROT_READ, pgmfns, n_fbdirs);
> +	qemu_free(pgmfns);
> +
> +	if (map == NULL)
> +	    goto out;
> +
> +	unsigned long *fbmfns = qemu_mallocz(sizeof(unsigned long) * xenfb->fbpages);
> +	xenfb_copy_mfns(mode, xenfb->fbpages, fbmfns, map);
> +	munmap(map, n_fbdirs * XC_PAGE_SIZE);
> +	xenfb->pixels = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
> +					     PROT_READ | PROT_WRITE, fbmfns, xenfb->fbpages);
> +	qemu_free(fbmfns);
> +    }
>  
> -    xenfb->pixels = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
> -					 PROT_READ | PROT_WRITE, fbmfns, xenfb->fbpages);
>      if (xenfb->pixels == NULL)
>  	goto out;
>  
>      ret = 0; /* all is fine */
>  
>  out:
> -    qemu_free(pgmfns);
> -    qemu_free(fbmfns);
>      return ret;
>  }
>  
> @@ -860,6 +902,8 @@ static int fb_init(struct XenDevice *xendev)
>  #ifdef XENFB_TYPE_RESIZE
>      xenstore_write_be_int(xendev, "feature-resize", 1);
>  #endif
> +    xenstore_write_be_int(xendev, "use-grants", 1);
> +
>      return 0;
>  }
>  
> @@ -913,6 +957,13 @@ static void fb_disconnect(struct XenDevice *xendev)
>  {
>      struct XenFB *fb = container_of(xendev, struct XenFB, c.xendev);
>  
> +    if (fb->pixels && fb->c.uses_gref) {
> +	xc_gnttab_munmap(xen_xc, fb->c.xendev.gnttabdev, fb->pixels, fb->fbpages);
> +    } else if (fb->pixels) {
> +	// Note: not needed if we are doing the mmap() below
> +	// munmap(fb->pixels, fb->fbpages * XC_PAGE_SIZE);
> +    }
> +
>      /*
>       * FIXME: qemu can't un-init gfx display (yet?).
>       *   Replacing the framebuffer with anonymous shared memory
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH qemu] Support grant references in mapping

[Stefano Stabellini]

On Fri, 25 Feb 2011, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 07, 2011 at 05:09:40PM -0500, Daniel De Graaf wrote:
> > This is untested in qemu, but is the same patch as was applied to the
> > server I have tested. It applies to the ioemu-remote that I am using.
> 
> I lost track of this patchset. I think we need to first have somebody
> familiar with QEMU review these patches.
> 
> CC-ing Stefano, Ian, Anthony

It looks OK in principle. It has some code style issues, in particular
qemu uses spaces instead of tabs.

This patch needs to be sent to qemu-devel too (in fact considering that
it is not going to be applied to qemu-xen 4.1 and that we are going to
move to upstream qemu in the 4.2 release cycle, it only matters if
upstream qemu applies it).

Re: [PATCH] Add grant references for fbfront/kbdfront

[Konrad Rzeszutek Wilk]

On Fri, Jan 07, 2011 at 12:55:24PM -0500, Daniel De Graaf wrote:
> On 01/07/2011 11:51 AM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
> >> This series fixes the interface for the fbfront and kbdfront devices,
> >> which were storing MFNs in xenstore rather than creating grant table
> >> entries. To maintain backwards compatibility, a different xenstore key
> >> is used (page-gref instead of page-ref) and the use of grants must be
> >> requested for fbfront (because two levels of page references are
> >> embedded within the shared page). This makes it possible to move a
> >> display server out of dom0 without giving the display domain full
> >> access to other domain's memory.
> > 
> > So these patches go on top of  xen/stable-2.6.32 tree right?
> >
> 
> They apply to either 2.6.32 or 2.6.37; I have tested them on both.

What is the 2.6.37 branch you applied it to? Can you point me to the
git tree? Or a set of patches that need to be applied on top of ftp.kernel.org
virgin 2.6.37?

Re: [PATCH] Add grant references for fbfront/kbdfront

[Daniel De Graaf]

On 01/07/2011 02:21 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 07, 2011 at 12:55:24PM -0500, Daniel De Graaf wrote:
>> On 01/07/2011 11:51 AM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
>>>> This series fixes the interface for the fbfront and kbdfront devices,
>>>> which were storing MFNs in xenstore rather than creating grant table
>>>> entries. To maintain backwards compatibility, a different xenstore key
>>>> is used (page-gref instead of page-ref) and the use of grants must be
>>>> requested for fbfront (because two levels of page references are
>>>> embedded within the shared page). This makes it possible to move a
>>>> display server out of dom0 without giving the display domain full
>>>> access to other domain's memory.
>>>
>>> So these patches go on top of  xen/stable-2.6.32 tree right?
>>>
>>
>> They apply to either 2.6.32 or 2.6.37; I have tested them on both.
> 
> What is the 2.6.37 branch you applied it to? Can you point me to the
> git tree? Or a set of patches that need to be applied on top of ftp.kernel.org
> virgin 2.6.37?
> 

It should apply on top of any 2.6.37 tree; I sent the patches from a
rebase on top of the v2.6.37 tag on ftp.kernel.org. They should apply
cleanly on any branch unless I missed other pending changes to fbfront.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] Add grant references for fbfront/kbdfront

[Konrad Rzeszutek Wilk]

On Fri, Jan 07, 2011 at 02:32:13PM -0500, Daniel De Graaf wrote:
> On 01/07/2011 02:21 PM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 07, 2011 at 12:55:24PM -0500, Daniel De Graaf wrote:
> >> On 01/07/2011 11:51 AM, Konrad Rzeszutek Wilk wrote:
> >>> On Fri, Jan 07, 2011 at 11:12:56AM -0500, dgdegra@tycho.nsa.gov wrote:
> >>>> This series fixes the interface for the fbfront and kbdfront devices,
> >>>> which were storing MFNs in xenstore rather than creating grant table
> >>>> entries. To maintain backwards compatibility, a different xenstore key
> >>>> is used (page-gref instead of page-ref) and the use of grants must be
> >>>> requested for fbfront (because two levels of page references are
> >>>> embedded within the shared page). This makes it possible to move a
> >>>> display server out of dom0 without giving the display domain full
> >>>> access to other domain's memory.
> >>>
> >>> So these patches go on top of  xen/stable-2.6.32 tree right?
> >>>
> >>
> >> They apply to either 2.6.32 or 2.6.37; I have tested them on both.
> > 
> > What is the 2.6.37 branch you applied it to? Can you point me to the
> > git tree? Or a set of patches that need to be applied on top of ftp.kernel.org
> > virgin 2.6.37?
> > 
> 
> It should apply on top of any 2.6.37 tree; I sent the patches from a

They do. The mail file I had with your name had patches for 2.6.32 which
did not apply on 2.6.37.

Sorry about the noise.

[PATCH] Add grant references for fbfront/kbdfront

[Konrad Rzeszutek Wilk]

This patchset is proposed for 2.6.39.

The patchset description from:
http://lists.colo.xensource.com/archives/html/xen-devel/2011-03/msg00381.html

This series fixes the interface for the fbfront and kbdfront devices,
which were storing MFNs in xenstore rather than creating grant table
entries. To maintain backwards compatibility, a different xenstore key
is used (page-gref instead of page-ref) and the use of grants must be
requested for fbfront (because two levels of page references are
embedded within the shared page). This makes it possible to move a
display server out of dom0 without giving the display domain full access
to other domain's memory.

Changes since v1:
        Updated xenstore key to "feature-grants"

Updated frontend patch has been sent to qemu-devel.


 drivers/input/xen-kbdfront.c |   39 +++++++++----
 drivers/video/xen-fbfront.c  |  126 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 132 insertions(+), 33 deletions(-)

Daniel De Graaf (3):
      xen-fbfront: Read width/height from backend
      xen-kbdfront: Add grant reference for shared page
      xen-fbfront: Use grant references when requested