[PATCH v2] Add grant references for fbfront/kbdfront

[PATCH v2] Add grant references for fbfront/kbdfront

[Daniel De Graaf]

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.

[PATCH 1/3] xen-fbfront: Read width/height from backend
[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

[Daniel De Graaf]

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 a20218c..56d6061 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

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

[Daniel De Graaf]

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 56d6061..b3107e4 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, "feature-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;
@@ -599,8 +636,12 @@ static int xenfb_connect_backend(struct xenbus_device *dev,
 		xenbus_dev_fatal(dev, ret, "starting transaction");
 		goto unbind_irq;
 	}
-	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

[Konrad Rzeszutek Wilk]

> +	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, "feature-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;
> +	}

Shouldn't we in xenfb_remove also cleanup (unclaim and wholesale free the
grant reference lot?)

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

[Daniel De Graaf]

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 |   96 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index 56d6061..b43c5c9 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,8 @@ struct xenfb_info {
 	int			irq;
 	struct xenfb_page	*page;
 	unsigned long 		*mfns;
+	grant_ref_t             *mfn_grefs;
+	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 +62,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 *);
 
@@ -412,6 +409,7 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
 	info->x1 = info->y1 = INT_MAX;
 	spin_lock_init(&info->dirty_lock);
 	spin_lock_init(&info->resize_lock);
+	info->page_gref = -1;
 
 	info->fb = vmalloc(fb_size);
 	if (info->fb == NULL)
@@ -474,7 +472,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 +526,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);
 }
 
@@ -543,9 +541,25 @@ static int xenfb_remove(struct xenbus_device *dev)
 		fb_dealloc_cmap(&info->fb_info->cmap);
 		framebuffer_release(info->fb_info);
 	}
+	if (info->page_gref >= 0) {
+		int epd = PAGE_SIZE / sizeof(info->mfns[0]);
+		int pdpages = (info->nr_pages + epd - 1) / epd;
+		int i;
+		gnttab_end_foreign_access_ref(info->page_gref, 0);
+		gnttab_free_grant_reference(info->page_gref);
+		for (i = 0; i < pdpages; i++) {
+			gnttab_end_foreign_access_ref(info->mfn_grefs[i], 1);
+			gnttab_free_grant_reference(info->mfn_grefs[i]);
+		}
+		for (i = 0; i < info->nr_pages; i++) {
+			gnttab_end_foreign_access_ref(info->mfns[i], 1);
+			gnttab_free_grant_reference(info->mfns[i]);
+		}
+	}
 	free_page((unsigned long)info->page);
 	vfree(info->mfns);
 	vfree(info->fb);
+	kfree(info->mfn_grefs);
 	kfree(info);
 
 	return 0;
@@ -556,17 +570,63 @@ 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 pdpages = (info->nr_pages + epd - 1) / epd;
+	int allpages = info->nr_pages + pdpages + 1;
+
+	int grants = 0;
+	xenbus_scanf(XBT_NIL, dev->otherend, "feature-grants", "%d", &grants);
+
+	if (grants) {
+		int err = gnttab_alloc_grant_references(allpages, &gref_head);
+		info->mfn_grefs = kzalloc(pdpages*sizeof(grant_ref_t), GFP_KERNEL);
+		if (!info->mfn_grefs || err < 0) {
+			info->page_gref = -ENOSPC;
+			if (err >= 0)
+				gnttab_free_grant_references(gref_head);
+			grants = 0;
+		} 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->mfn_grefs[i] = ref;
+			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;
@@ -599,8 +659,12 @@ static int xenfb_connect_backend(struct xenbus_device *dev,
 		xenbus_dev_fatal(dev, ret, "starting transaction");
 		goto unbind_irq;
 	}
-	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

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

[Daniel De Graaf]

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 7f85a86..a5c064e 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.
- */
-
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/kernel.h>
@@ -30,6 +24,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>
@@ -38,6 +34,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];
@@ -122,6 +119,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);
@@ -218,15 +216,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;
 
@@ -234,12 +237,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)
@@ -249,7 +255,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);
@@ -258,6 +264,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;
 }
 
@@ -266,6 +280,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 v2] Add grant references for fbfront/kbdfront

[Ian Campbell]

On Mon, 2011-03-07 at 20:11 +0000, Daniel De Graaf 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.

Looks good to me. I presume this was all tested with the existing
backends as well as the separate display server backend?

Under that assumption all 3:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I think it would be good to get the backend patches into the tree as
well, to be used even when running in domain 0. Not just for good form
but because it should help avoid this stuff from bit-rotting etc which
seems like a danger if the only user is your display server.

Ian.

> 
> Changes since v1:
> 	Updated xenstore key to "feature-grants"
> 
> Updated frontend patch has been sent to qemu-devel.
> 
> [PATCH 1/3] xen-fbfront: Read width/height from backend
> [PATCH 2/3] xen-fbfront: Use grant references when requested
> [PATCH 3/3] xen-kbdfront: Add grant reference for shared page

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

[Daniel De Graaf]

On 03/08/2011 05:44 AM, Ian Campbell wrote:
> On Mon, 2011-03-07 at 20:11 +0000, Daniel De Graaf 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.
> 
> Looks good to me. I presume this was all tested with the existing
> backends as well as the separate display server backend?
> 
> Under that assumption all 3:
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I think it would be good to get the backend patches into the tree as
> well, to be used even when running in domain 0. Not just for good form
> but because it should help avoid this stuff from bit-rotting etc which
> seems like a danger if the only user is your display server.
> 
> Ian.
> 

The kernel changes have been tested using both qemu-dm (for PV guest; the
emulated device is still used for HVM) and a custom display server (for
both PV and HVM). I have not tested using a qemu-dm on HVM guests (dom0 or
stub-domain), since the default on HVM is to use qemu's own emulated
graphics card rather than the Xen framebuffer.

Testing on HVM guests requires another patch (below) that I did not submit
because it will cause HVM domains to delay boot for the 300-second device
timeout when vfb entries are placed in xenstore without actually providing
a vfb backend. Unfortunately, this is how XM stores the VNC parameters for
HVM guests so that they are in the same location as for PV guests
(tools/python/xen/xend/XendConfig.py line 942), so providing the xenstore
entries without a backend is a common configuration.

---

diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
index a5c064e..54b48df 100644
--- a/drivers/input/xen-kbdfront.c
+++ b/drivers/input/xen-kbdfront.c
@@ -358,7 +358,7 @@ static struct xenbus_driver xenkbd_driver = {
 
 static int __init xenkbd_init(void)
 {
-	if (!xen_pv_domain())
+	if (!xen_domain())
 		return -ENODEV;
 
 	/* Nothing to do if running in dom0. */
diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index b3107e4..03d7ef3 100644
--- a/drivers/video/xen-fbfront.c
+++ b/drivers/video/xen-fbfront.c
@@ -748,7 +748,7 @@ static struct xenbus_driver xenfb_driver = {
 
 static int __init xenfb_init(void)
 {
-	if (!xen_pv_domain())
+	if (!xen_domain())
 		return -ENODEV;
 
 	/* Nothing to do if running in dom0. */

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

[Ian Campbell]

On Tue, 2011-03-08 at 21:03 +0000, Daniel De Graaf wrote:
> On 03/08/2011 05:44 AM, Ian Campbell wrote:
> > On Mon, 2011-03-07 at 20:11 +0000, Daniel De Graaf 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.
> > 
> > Looks good to me. I presume this was all tested with the existing
> > backends as well as the separate display server backend?
> > 
> > Under that assumption all 3:
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > I think it would be good to get the backend patches into the tree as
> > well, to be used even when running in domain 0. Not just for good form
> > but because it should help avoid this stuff from bit-rotting etc which
> > seems like a danger if the only user is your display server.
> > 
> > Ian.
> > 
> 
> The kernel changes have been tested using both qemu-dm (for PV guest; the
> emulated device is still used for HVM) and a custom display server (for
> both PV and HVM). I have not tested using a qemu-dm on HVM guests (dom0 or
> stub-domain), since the default on HVM is to use qemu's own emulated
> graphics card rather than the Xen framebuffer.

OK, thanks.

> Testing on HVM guests requires another patch (below) that I did not submit
> because it will cause HVM domains to delay boot for the 300-second device
> timeout when vfb entries are placed in xenstore without actually providing
> a vfb backend. Unfortunately, this is how XM stores the VNC parameters for
> HVM guests so that they are in the same location as for PV guests
> (tools/python/xen/xend/XendConfig.py line 942), so providing the xenstore
> entries without a backend is a common configuration.

Hrm, I guess that something we'll have to figure out some other time.

Ian.

> ---
> 
> diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
> index a5c064e..54b48df 100644
> --- a/drivers/input/xen-kbdfront.c
> +++ b/drivers/input/xen-kbdfront.c
> @@ -358,7 +358,7 @@ static struct xenbus_driver xenkbd_driver = {
>  
>  static int __init xenkbd_init(void)
>  {
> -	if (!xen_pv_domain())
> +	if (!xen_domain())
>  		return -ENODEV;
>  
>  	/* Nothing to do if running in dom0. */
> diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
> index b3107e4..03d7ef3 100644
> --- a/drivers/video/xen-fbfront.c
> +++ b/drivers/video/xen-fbfront.c
> @@ -748,7 +748,7 @@ static struct xenbus_driver xenfb_driver = {
>  
>  static int __init xenfb_init(void)
>  {
> -	if (!xen_pv_domain())
> +	if (!xen_domain())
>  		return -ENODEV;
>  
>  	/* Nothing to do if running in dom0. */

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