32-on-64: pvfb issue

32-on-64: pvfb issue

[Gerd Hoffmann]

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

  Hi,

There is a problem with the virtual framebuffer: The page directory in
the shared page (xenfb_page->pd[]) is unsigned long and thus has
different sizes on 32bit and 64bit.  The alignment is different too. And
on top of that the frontend driver doesn't clear the shared page, which
makes it difficult to autodetect the bitsize.  I've tried nevertheless,
patch (untested!) attached for comments.  In the long run this code is
supposed to be replaced by grant tables anyway, so it is probably okay
to live with the hack for the time being.  We could of course also fix
the struct if we can afford breaking the interface.  As it is quite new
and probably (hmm, does fc6 ship it?) not widely used yet that might be
an option.

cheers,
  Gerd

[-- Attachment #2: bimodal-fb --]
[-- Type: text/plain, Size: 2944 bytes --]

--- build-32-unstable-13401/tools/xenfb/xenfb.c-13401	2007-01-18 11:35:53.000000000 +0100
+++ build-32-unstable-13401/tools/xenfb/xenfb.c	2007-01-18 12:30:44.000000000 +0100
@@ -329,12 +329,59 @@
 	struct xenfb_page *page = xenfb->fb.page;
 	int n_fbmfns;
 	int n_fbdirs;
+	unsigned long pgmfns[2];
 	unsigned long *fbmfns;
+	uint32_t *ptr32;
+	uint64_t *ptr64;
+	int i,mode;
+
+/*
+ * bimodal guess work.
+ * page->depth is at a 64bit border, thus we get for page->pg[] ...
+ *
+ *  i386    | depth | 3b pad |  pg0  |  pg1  |  pg2  |  pg3  |  pg4  |
+ *  x86_64  | depth | 7b pad         |      pg0      |      pg1      |
+ *
+ * frontend does *not* clear the page.  pg0 and pg1 are initialized.
+ * at the moment pg0 is used only (800x600x32 needs less than 512 pages,
+ * which fits into one page directory even with 64bit), pg1 is initialized
+ * to zero.  We also assume the machine has less than 16 TB main memory,
+ * thus 32bit are enougth for the page frame numbers.  By the time this
+ * assumtion is rendered invalid the code should have been converted to
+ * grant tables anyway.
+ */
+#if defined(__i386__)
+	mode = 32;
+	ptr32 = (void*)page->pd;
+	ptr64 = ((void*)ptr32) + 4;
+	if (ptr32[1] > 0 &&
+	    ptr32[2] == 0 && 
+	    ptr32[3] == 0 &&
+	    ptr32[4] == 0)
+		mode = 64;
+#elif defined(__x86_64__)
+	mode = 64;
+	ptr64 = (void*)page->pd;
+	ptr32 = ((void*)ptr64) - 4;
+	if (ptr32[0] > 0 &&
+	    ptr32[1] == 0)
+		mode = 32;
+#else
+# error unknown arch
+#endif
 
 	n_fbmfns = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
-	n_fbdirs = n_fbmfns * sizeof(unsigned long);
+	n_fbdirs = n_fbmfns * mode / 4;
 	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
 
+	if (32 == mode) {
+		for (i = 0; i < n_fbdirs; i++)
+			pgmfns[i] = ptr32[i];
+	} else {
+		for (i = 0; i < n_fbdirs; i++)
+			pgmfns[i] = ptr64[i];
+	}
+
 	/*
 	 * Bug alert: xc_map_foreign_batch() can fail partly and
 	 * return a non-null value.  This is a design flaw.  When it
@@ -342,18 +389,31 @@
 	 * access.
 	 */
 	fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
-			PROT_READ, page->pd, n_fbdirs);
+			PROT_READ, pgmfns, n_fbdirs);
 	if (fbmfns == NULL)
 		return -1;
 
+	if (32 == mode) {
+		ptr32 = (void*)fbmfns;
+		fbmfns = malloc(n_fbmfns * sizeof(*fbmfns));
+		for (i = 0; i < n_fbmfns; i++)
+			fbmfns[i] = ptr32[i];
+		munmap(ptr32, n_fbdirs * XC_PAGE_SIZE);
+	} else {
+		ptr64 = (void*)fbmfns;
+		fbmfns = malloc(n_fbmfns * sizeof(*fbmfns));
+		for (i = 0; i < n_fbmfns; i++)
+			fbmfns[i] = ptr64[i];
+		munmap(ptr64, n_fbdirs * XC_PAGE_SIZE);
+	}
+
 	xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
 				PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
-	if (xenfb->pub.pixels == NULL) {
-		munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+	if (xenfb->pub.pixels == NULL)
 		return -1;
-	}
 
-	return munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+	free(fbmfns);
+	return 0;
 }
 
 static int xenfb_bind(struct xenfb_device *dev)

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

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

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 18/1/07 13:57, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> As it is quite new
> and probably (hmm, does fc6 ship it?) not widely used yet that might be
> an option.

This would be preferable but possibly not too popular with Red Hat. :-)
Otherwise using aligned uint64_t will obviously save some pain.

 -- Keir

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Gerd Hoffmann <kraxel@suse.de> writes:

>   Hi,
>
> There is a problem with the virtual framebuffer: The page directory in
> the shared page (xenfb_page->pd[]) is unsigned long and thus has
> different sizes on 32bit and 64bit.  The alignment is different too. And
> on top of that the frontend driver doesn't clear the shared page, which
> makes it difficult to autodetect the bitsize.  I've tried nevertheless,
> patch (untested!) attached for comments.  In the long run this code is
> supposed to be replaced by grant tables anyway, so it is probably okay
> to live with the hack for the time being.  We could of course also fix
> the struct if we can afford breaking the interface.  As it is quite new
> and probably (hmm, does fc6 ship it?) not widely used yet that might be
> an option.

Breaking the API now is right out of the question, I fear :)

You can evolve the API.  Let the frontend put something in xenstore[*]
that lets the backend detect which page layout to use.  Make sure the
backend can deal with old and new frontend.  I doubt it's worthwhile
here.

Excuse my ignorance, but why do you have to guess the guest's size?
Doesn't dom0 know?


[*] I suggested to put version ID right into the page, but that was
shot down in favor of xenstore.

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@suse.de> writes:
> 
>> and probably (hmm, does fc6 ship it?) not widely used yet that might be
>> an option.
> 
> Breaking the API now is right out of the question, I fear :)

Yep, I've seen in the release notes fc6 ships pvfb, so it is used in the
wild now and breaking the API is clearly out of question.  Damn.  Should
have reviewed the patches earlier ...

> You can evolve the API.  Let the frontend put something in xenstore[*]
> that lets the backend detect which page layout to use.  Make sure the
> backend can deal with old and new frontend.  I doubt it's worthwhile
> here.

Well, the problem are the *existing* guests, we already have two
different versions *without* indication out there:  The 32bit and the
64bit ones.  And with the upcoming 32-on-64 support the backend suddenly
must be able to deal with both of them.  Ideally it should work
automatically, without updating the frontends, and without manual
configuration.

> Excuse my ignorance, but why do you have to guess the guest's size?
> Doesn't dom0 know?

No, right now there is no hypercall to get that information.  Which
brings the discussion back onto the table:  Should we add one?  pvfb
isn't the only driver with that kind of problems.

And it better shouldn't be a dom0-only hypercall, otherwise driver
domains will not be able to use it ...

> [*] I suggested to put version ID right into the page, but that was
> shot down in favor of xenstore.

Too bad.  The configuration is in the shared page anyway, so what is the
point of *not* placing the version info there as well?

cheers,
  Gerd

Re: 32-on-64: pvfb issue

[Daniel P. Berrange]

On Thu, Jan 18, 2007 at 04:35:03PM +0100, Gerd Hoffmann wrote:
> Markus Armbruster wrote:
> > Gerd Hoffmann <kraxel@suse.de> writes:
> > 
> >> and probably (hmm, does fc6 ship it?) not widely used yet that might be
> >> an option.
> > 
> > Breaking the API now is right out of the question, I fear :)
> 
> Yep, I've seen in the release notes fc6 ships pvfb, so it is used in the
> wild now and breaking the API is clearly out of question.  Damn.  Should
> have reviewed the patches earlier ...

Actually FC6 isn't a problem - the ABI already changed between the time we 
shipped it in FC6, and the time it got merged in xen-devel. For RHEL-5 though
we have synced to the ABI currently in xen-devel/xen-3.0.4. We plan to update
FC-6 to use this ABI too, to remove the incompatability. The downside is that
in FC-6 Xen userspace we now have to maintain some non-upstream back-compat
code to let both old &new kernels run on new userspace - that's the price
paid for shipping before upstream. Now it is upstream though we really don't
want to see incompatible changes again because it will screw over not just
Fedora, but RHEL too, and indeed any users of Xen 3.0.4 release.

BTW, the way we do the back-compat support is basically to ship two versions
of the xen-sdlfb & xen-vncfb  daemons - one speaking the old protocol, one
speaking the new. We try to launch the new versions & if they fail, we 
launch the old versions. A nasty hack, but it works.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Daniel P. Berrange wrote:
> Actually FC6 isn't a problem - the ABI already changed between the time we 
> shipped it in FC6, and the time it got merged in xen-devel.

Ok.

> For RHEL-5 though
> we have synced to the ABI currently in xen-devel/xen-3.0.4.

Hmm, assuming we'll fix up the ABI now to be binary compatible between
32 and 64 bit and put that one into both 3.0.4 and unstable, could that
still make it into RHEL-5 or is that too late now?

cheers,
  Gerd

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

  Hi,

> Hmm, assuming we'll fix up the ABI now to be binary compatible between
> 32 and 64 bit and put that one into both 3.0.4 and unstable, could that
> still make it into RHEL-5 or is that too late now?

i.e. something along the lines of the attached patch (compile tested
only) ...

cheers,
  Gerd

[-- Attachment #2: abi-fb --]
[-- Type: text/plain, Size: 3713 bytes --]

--- build-32-unstable-13401/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c.abi	2007-01-18 17:43:07.000000000 +0100
+++ build-32-unstable-13401/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c	2007-01-18 17:46:29.000000000 +0100
@@ -58,7 +58,7 @@
 
 	int			irq;
 	struct xenfb_page	*page;
-	unsigned long 		*mfns;
+	u64                     *mfns;
 	int			update_wanted; /* XENFB_TYPE_UPDATE wanted */
 
 	struct xenbus_device	*xbdev;
@@ -474,12 +474,12 @@
 	if (info->pages == NULL)
 		goto error_nomem;
 
-	info->mfns = vmalloc(sizeof(unsigned long) * info->nr_pages);
+	info->mfns = vmalloc(sizeof(u64) * info->nr_pages);
 	if (!info->mfns)
 		goto error_nomem;
 
 	/* set up shared page */
-	info->page = (void *)__get_free_page(GFP_KERNEL);
+	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
 
@@ -600,6 +600,7 @@
 	for (i = 0; i < info->nr_pages; i++)
 		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
 
+	info->page->protocol = 1;
 	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
 	info->page->pd[1] = 0;
 	info->page->width = XENFB_WIDTH;
--- build-32-unstable-13401/tools/xenfb/xenfb.c.abi	2007-01-18 17:48:57.000000000 +0100
+++ build-32-unstable-13401/tools/xenfb/xenfb.c	2007-01-18 17:50:40.000000000 +0100
@@ -329,12 +329,20 @@
 	struct xenfb_page *page = xenfb->fb.page;
 	int n_fbmfns;
 	int n_fbdirs;
+	unsigned long pgmfns[2];
 	unsigned long *fbmfns;
+	uint64_t *ptr64;
+	int i;
+
+	ptr64 = page->pd;
 
 	n_fbmfns = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
-	n_fbdirs = n_fbmfns * sizeof(unsigned long);
+	n_fbdirs = n_fbmfns * sizeof(uint64_t);
 	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
 
+	for (i = 0; i < n_fbdirs; i++)
+		pgmfns[i] = ptr64[i];
+
 	/*
 	 * Bug alert: xc_map_foreign_batch() can fail partly and
 	 * return a non-null value.  This is a design flaw.  When it
@@ -342,18 +350,23 @@
 	 * access.
 	 */
 	fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
-			PROT_READ, page->pd, n_fbdirs);
+			PROT_READ, pgmfns, n_fbdirs);
 	if (fbmfns == NULL)
 		return -1;
 
+	ptr64 = (void*)fbmfns;
+	fbmfns = malloc(n_fbmfns * sizeof(*fbmfns));
+	for (i = 0; i < n_fbmfns; i++)
+		fbmfns[i] = ptr64[i];
+	munmap(ptr64, n_fbdirs * XC_PAGE_SIZE);
+
 	xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
 				PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
-	if (xenfb->pub.pixels == NULL) {
-		munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+	if (xenfb->pub.pixels == NULL)
 		return -1;
-	}
 
-	return munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+	free(fbmfns);
+	return 0;
 }
 
 static int xenfb_bind(struct xenfb_device *dev)
--- build-32-unstable-13401/xen/include/public/io/fbif.h.abi	2007-01-18 17:34:49.000000000 +0100
+++ build-32-unstable-13401/xen/include/public/io/fbif.h	2007-01-18 17:38:39.000000000 +0100
@@ -102,6 +102,8 @@
     uint32_t line_length;   /* the length of a row of pixels (in bytes) */
     uint32_t mem_length;    /* the length of the framebuffer (in bytes) */
     uint8_t depth;          /* the depth of a pixel (in bits) */
+    uint8_t protocol;       /* protocol version, will bump when we switch to grant tables */
+    uint8_t pad[6];         /* align next field to 64bit */
 
     /*
      * Framebuffer page directory
@@ -109,10 +111,10 @@
      * Each directory page holds PAGE_SIZE / sizeof(*pd)
      * framebuffer pages, and can thus map up to PAGE_SIZE *
      * PAGE_SIZE / sizeof(*pd) bytes.  With PAGE_SIZE == 4096 and
-     * sizeof(unsigned long) == 4, that's 4 Megs.  Two directory
+     * sizeof(uint64_t) == 8, that's 2 Megs.  Four directory
      * pages should be enough for a while.
      */
-    unsigned long pd[2];
+    uint64_t pd[4];
 };
 
 /*

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

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

Re: 32-on-64: pvfb issue

[Daniel P. Berrange]

On Thu, Jan 18, 2007 at 05:34:00PM +0100, Gerd Hoffmann wrote:
> Daniel P. Berrange wrote:
> > Actually FC6 isn't a problem - the ABI already changed between the time we 
> > shipped it in FC6, and the time it got merged in xen-devel.
> 
> Ok.
> 
> > For RHEL-5 though
> > we have synced to the ABI currently in xen-devel/xen-3.0.4.
> 
> Hmm, assuming we'll fix up the ABI now to be binary compatible between
> 32 and 64 bit and put that one into both 3.0.4 and unstable, could that
> still make it into RHEL-5 or is that too late now?

Not any chance whatsoever I'm afraid :-( We're way too far into the release
process to make any functional changes now.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 18/1/07 15:35, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> Excuse my ignorance, but why do you have to guess the guest's size?
>> Doesn't dom0 know?
> 
> No, right now there is no hypercall to get that information.  Which
> brings the discussion back onto the table:  Should we add one?  pvfb
> isn't the only driver with that kind of problems.
> 
> And it better shouldn't be a dom0-only hypercall, otherwise driver
> domains will not be able to use it ...

Do the same as you did with blkback. Add a protocol version or bitwidth
field to xenstore. Update frontend driver to write that field. Make tools
write a default value for pv guests for backward compat. This is even better
than with blkfront/blkback because there are definitely no PV-on-HVM pvfb
driver frontends out in the wild (and it's HVM that makes things harder).

Speaking of which, we should add the protocol/bitwidth field to blkfront
asap...

 -- Keir

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Keir Fraser wrote:
> Do the same as you did with blkback. Add a protocol version or bitwidth
> field to xenstore. Update frontend driver to write that field. Make tools
> write a default value for pv guests for backward compat. This is even better
> than with blkfront/blkback because there are definitely no PV-on-HVM pvfb
> driver frontends out in the wild (and it's HVM that makes things harder).

Ok, point taken, forgot about the pv-on-hvm issue.

Why put the protocol into xenstore btw?  The framebuffer configuration
is in the shared page instead of xenstore anyway.  I'd place protocol
next to the configuration, it is the natural location IMHO.  We can
still sneak it in without breaking the struct layout as we have
alignment holes in there anyway.

> Speaking of which, we should add the protocol/bitwidth field to blkfront
> asap...

It is sitting in my 32-on-64 patch queue emmanuel is looking at.  The
blkback bits shouldn't have any dependencies to the other patches
though, so I can submit it right away if you are willing to take it now.
 Patch for unstable attached.  I can prepare a minimal one with just the
frontend bits for 3.0.4 if you want.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: blkif-bimodal.diff --]
[-- Type: text/x-patch, Size: 25463 bytes --]

multiprotocol blkback drivers.

This is a patch for the block interface, frontend drivers, backend
drivers and tools to support multiple ring protocols.  Right there are
now just two: the 32bit and the 64bit one.  If needed it can be
extended.

Interface changes (io/blkif.h)
 * Have both request structs there, with "v1" and "v2" added to the
   name.  The old name is aliased to the native protocol of the
   architecture.
 * Add helper functions to convert v1/v2 requests to native.

Frontend changes:
 * Create a new node "protocol", add the protocol number it speaks
   there.

Backend changes:
 * Look at the "protocol" number of the frontend and switch ring
   handling accordingly.  If the protocol node isn't present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn't need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

Tools changes:
 * Add one more option to the disk configuration, so one can specify the
   protocol the frontend speaks in the config file.  This is needed for
   old frontends which don't advertise the protocol they are speaking
   themself.
   I'm not that happy with this approach, but it works for now and I'm
   kida lost in the stack of python classes doing domain and device
   handling ...

---
 linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c   |   64 ++++++++----
 linux-2.6-xen-sparse/drivers/xen/blkback/common.h    |    6 -
 linux-2.6-xen-sparse/drivers/xen/blkback/interface.c |   25 +++-
 linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c    |   14 ++
 linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c |    7 +
 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c     |   63 ++++++++----
 linux-2.6-xen-sparse/drivers/xen/blktap/common.h     |    6 -
 linux-2.6-xen-sparse/drivers/xen/blktap/interface.c  |   25 +++-
 linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c     |   14 ++
 linux-2.6-xen-sparse/include/xen/blkif.h             |   99 +++++++++++++++++++
 tools/python/xen/xend/server/blkif.py                |    3 
 tools/python/xen/xm/create.py                        |    7 +
 xen/include/public/io/blkif.h                        |   14 +-
 13 files changed, 282 insertions(+), 65 deletions(-)

Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
@@ -298,17 +298,20 @@ irqreturn_t blkif_be_int(int irq, void *
 
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->co.req_cons;
+	rp = blk_rings->co.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
-	while ((rc != rp) && !RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+	while ((rc != rp)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->co, rc))
+			break;
 
 		pending_req = alloc_req();
 		if (NULL == pending_req) {
@@ -317,8 +320,17 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */
+		switch (blkif->blk_protocol) {
+		case 1:
+			blkif_get_v1_req(&req, RING_GET_REQUEST(&blk_rings->v1, rc));
+			break;
+		case 2:
+			blkif_get_v2_req(&req, RING_GET_REQUEST(&blk_rings->v2, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->co.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -498,34 +510,44 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
 			  unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
-	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case 1:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v1, blk_rings->v1.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case 2:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v2, blk_rings->v2.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->co.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->co, notify);
+	if (blk_rings->co.rsp_prod_pvt == blk_rings->co.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->co, more_to_do);
 
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->co)) {
 		more_to_do = 1;
-
 	}
+
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 
 	if (more_to_do)
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
@@ -40,8 +40,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 #include <xen/xenbus.h>
@@ -67,7 +66,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	int               blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* The VBD attached to this interface. */
 	struct vbd        vbd;
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
@@ -95,7 +95,6 @@ static void unmap_frontend_page(blkif_t 
 
 int blkif_map(blkif_t *blkif, unsigned long shared_page, unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -111,8 +110,24 @@ int blkif_map(blkif_t *blkif, unsigned l
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case 1:
+	{
+		blkif_v1_sring_t *sring_v1;
+		sring_v1 = (blkif_v1_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v1, sring_v1, PAGE_SIZE);
+		break;
+	}
+	case 2:
+	{
+		blkif_v2_sring_t *sring_v2;
+		sring_v2 = (blkif_v2_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v2, sring_v2, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, blkif_be_int, 0, "blkif-backend", blkif);
@@ -143,10 +158,10 @@ void blkif_disconnect(blkif_t *blkif)
 		blkif->irq = 0;
 	}
 
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.co.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.co.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
@@ -459,6 +459,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	unsigned int protocol;
 	int err;
 
 	DPRINTK("%s", dev->otherend);
@@ -472,6 +473,19 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%u", &protocol, NULL);
+	if (err)
+		protocol = BLKIF_NATIVE_PROTOCOL;
+	if (protocol < 1 || protocol > 2) {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %d", protocol);
+		return -1;
+	}
+	be->blkif->blk_protocol = protocol;
+
+	printk("blkback: ring-ref %ld, event-channel %d, protocol %d\n",
+	       ring_ref, evtchn, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
@@ -45,6 +45,7 @@
 #include <xen/xenbus.h>
 #include <xen/interface/grant_table.h>
 #include <xen/gnttab.h>
+#include <xen/blkif.h>
 #include <asm/hypervisor.h>
 #include <asm/maddr.h>
 
@@ -180,6 +181,12 @@ again:
 		message = "writing event-channel";
 		goto abort_transaction;
 	}
+	err = xenbus_printf(xbt, dev->nodename,
+			    "protocol", "%u", BLKIF_NATIVE_PROTOCOL);
+	if (err) {
+		message = "writing protocol";
+		goto abort_transaction;
+	}
 
 	err = xenbus_transaction_end(xbt, 0);
 	if (err) {
Index: build-32-unstable-13495/xen/include/public/io/blkif.h
===================================================================
--- build-32-unstable-13495.orig/xen/include/public/io/blkif.h
+++ build-32-unstable-13495/xen/include/public/io/blkif.h
@@ -71,18 +71,20 @@
  */
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
+struct blkif_request_segment {
+    grant_ref_t gref;        /* reference to I/O buffer frame        */
+    /* @first_sect: first sector in frame to transfer (inclusive).   */
+    /* @last_sect: last sector in frame to transfer (inclusive).     */
+    uint8_t     first_sect, last_sect;
+};
+
 struct blkif_request {
     uint8_t        operation;    /* BLKIF_OP_???                         */
     uint8_t        nr_segments;  /* number of segments                   */
     blkif_vdev_t   handle;       /* only for read/write requests         */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
-    struct blkif_request_segment {
-        grant_ref_t gref;        /* reference to I/O buffer frame        */
-        /* @first_sect: first sector in frame to transfer (inclusive).   */
-        /* @last_sect: last sector in frame to transfer (inclusive).     */
-        uint8_t     first_sect, last_sect;
-    } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 };
 typedef struct blkif_request blkif_request_t;
 
Index: build-32-unstable-13495/tools/python/xen/xend/server/blkif.py
===================================================================
--- build-32-unstable-13495.orig/tools/python/xen/xend/server/blkif.py
+++ build-32-unstable-13495/tools/python/xen/xend/server/blkif.py
@@ -38,6 +38,7 @@ class BlkifController(DevController):
         """@see DevController.getDeviceDetails"""
         uname = config.get('uname', '')
         dev = config.get('dev', '')
+        protocol = config.get('protocol')
         
         if 'ioemu:' in dev:
             (_, dev) = string.split(dev, ':', 1)
@@ -85,6 +86,8 @@ class BlkifController(DevController):
         front = { 'virtual-device' : "%i" % devid,
                   'device-type' : dev_type
                 }
+	if protocol:
+	    front.update({ 'protocol' : protocol });
 
         return (devid, back, front)
 
Index: build-32-unstable-13495/tools/python/xen/xm/create.py
===================================================================
--- build-32-unstable-13495.orig/tools/python/xen/xm/create.py
+++ build-32-unstable-13495/tools/python/xen/xm/create.py
@@ -537,7 +537,7 @@ def configure_image(vals):
 def configure_disks(config_devs, vals):
     """Create the config for disks (virtual block devices).
     """
-    for (uname, dev, mode, backend) in vals.disk:
+    for (uname, dev, mode, backend, protocol) in vals.disk:
         if uname.startswith('tap:'):
             cls = 'tap'
         else:
@@ -549,6 +549,8 @@ def configure_disks(config_devs, vals):
                       ['mode', mode ] ]
         if backend:
             config_vbd.append(['backend', backend])
+        if protocol:
+            config_vbd.append(['protocol', protocol])
         config_devs.append(['device', config_vbd])
 
 def configure_pci(config_devs, vals):
@@ -803,7 +805,10 @@ def preprocess_disk(vals):
         n = len(d)
         if n == 3:
             d.append(None)
+            d.append(None)
         elif n == 4:
+            d.append(None)
+        elif n == 5:
             pass
         else:
             err('Invalid disk specifier: ' + v)
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
@@ -1094,15 +1094,15 @@ irqreturn_t tap_blkif_be_int(int irq, vo
 static int print_dbug = 1;
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 	tap_blkif_t *info;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->co.req_cons;
+	rp = blk_rings->co.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
 	/*Check blkif has corresponding UE ring*/
@@ -1133,8 +1133,8 @@ static int do_block_io_op(blkif_t *blkif
 			more_to_do = 1;
 			break;
 		}
-		
-		if (RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->co, rc)) {
 			WPRINTK("RING_REQUEST_CONS_OVERFLOW!"
 			       " More to do\n");
 			more_to_do = 1;
@@ -1148,8 +1148,17 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */	
+		switch (blkif->blk_protocol) {
+		case 1:
+			blkif_get_v1_req(&req, RING_GET_REQUEST(&blk_rings->v1, rc));
+			break;
+		case 2:
+			blkif_get_v2_req(&req, RING_GET_REQUEST(&blk_rings->v2, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->co.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -1225,7 +1234,7 @@ static void dispatch_rw_block_io(blkif_t
 		WPRINTK("blktap: fe_ring is full, can't add "
 			"IO Request will be dropped. %d %d\n",
 			RING_SIZE(&info->ufe_ring),
-			RING_SIZE(&blkif->blk_ring));
+			RING_SIZE(&blkif->blk_rings.co));
 		goto fail_response;
 	}
 
@@ -1413,32 +1422,44 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
                           unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
+
 	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case 1:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v1, blk_rings->v1.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case 2:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v2, blk_rings->v2.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->co.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->co, notify);
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	if (blk_rings->co.rsp_prod_pvt == blk_rings->co.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->co, more_to_do);
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->co)) {
 		more_to_do = 1;
+	}
 
-	}	
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 	if (more_to_do)
 		blkif_notify_work(blkif);
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
@@ -39,8 +39,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 
@@ -58,7 +57,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	int               blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* Back pointer to the backend_info. */
 	struct backend_info *be;
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
@@ -96,7 +96,6 @@ static void unmap_frontend_page(blkif_t 
 int tap_blkif_map(blkif_t *blkif, unsigned long shared_page, 
 		  unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -112,8 +111,24 @@ int tap_blkif_map(blkif_t *blkif, unsign
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case 1:
+	{
+		blkif_v1_sring_t *sring_v1;
+		sring_v1 = (blkif_v1_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v1, sring_v1, PAGE_SIZE);
+		break;
+	}
+	case 2:
+	{
+		blkif_v2_sring_t *sring_v2;
+		sring_v2 = (blkif_v2_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v2, sring_v2, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, tap_blkif_be_int,
@@ -134,10 +149,10 @@ void tap_blkif_unmap(blkif_t *blkif)
 		unbind_from_irqhandler(blkif->irq, blkif);
 		blkif->irq = 0;
 	}
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.co.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.co.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
@@ -340,6 +340,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	unsigned int protocol;
 	int err;
 
 	DPRINTK("%s\n", dev->otherend);
@@ -353,6 +354,19 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%u", &protocol, NULL);
+	if (err)
+		protocol = BLKIF_NATIVE_PROTOCOL;
+	if (protocol < 1 || protocol > 2) {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %d", protocol);
+		return -1;
+	}
+	be->blkif->blk_protocol = protocol;
+
+	printk("blktap: ring-ref %ld, event-channel %d, protocol %d\n",
+	       ring_ref, evtchn, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = tap_blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13495/linux-2.6-xen-sparse/include/xen/blkif.h
===================================================================
--- /dev/null
+++ build-32-unstable-13495/linux-2.6-xen-sparse/include/xen/blkif.h
@@ -0,0 +1,99 @@
+#ifndef __XEN_BLKIF_H__
+#define __XEN_BLKIF_H__
+
+#include <xen/interface/io/ring.h>
+#include <xen/interface/io/blkif.h>
+
+/* Not a real protocol.  Used to generate ring structs which contain
+ * the elements common to all protocols only.  This way we get a
+ * compiler-checkable way to use common struct elements, so we can
+ * avoid using switch(protocol) in a number of places.  */
+struct blkif_co_request {
+    char dummy;
+};
+struct blkif_co_response {
+    char dummy;
+};
+
+/* i386 protocol version */
+#pragma pack(push, 4)
+struct blkif_v1_request {
+    uint8_t        operation;    /* BLKIF_OP_???                         */
+    uint8_t        nr_segments;  /* number of segments                   */
+    blkif_vdev_t   handle;       /* only for read/write requests         */
+    uint64_t       id;           /* private guest value, echoed in resp  */
+    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_v1_response {
+    uint64_t        id;              /* copied from request */
+    uint8_t         operation;       /* copied from request */
+    int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_v1_request blkif_v1_request_t;
+typedef struct blkif_v1_response blkif_v1_response_t;
+#pragma pack(pop)
+
+/* x86_64 protocol version */
+struct blkif_v2_request {
+    uint8_t        operation;    /* BLKIF_OP_???                         */
+    uint8_t        nr_segments;  /* number of segments                   */
+    blkif_vdev_t   handle;       /* only for read/write requests         */
+    uint64_t       __attribute__((__aligned__(8))) id;
+    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_v2_response {
+    uint64_t       __attribute__((__aligned__(8))) id;
+    uint8_t         operation;       /* copied from request */
+    int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_v2_request blkif_v2_request_t;
+typedef struct blkif_v2_response blkif_v2_response_t;
+
+DEFINE_RING_TYPES(blkif_co, struct blkif_co_request, struct blkif_co_response);
+DEFINE_RING_TYPES(blkif_v1, struct blkif_v1_request, struct blkif_v1_response);
+DEFINE_RING_TYPES(blkif_v2, struct blkif_v2_request, struct blkif_v2_response);
+
+union blkif_back_rings {
+    blkif_co_back_ring_t co;
+    blkif_v1_back_ring_t v1;
+    blkif_v2_back_ring_t v2;
+};
+typedef union blkif_back_rings blkif_back_rings_t;
+
+#if defined(__i386__)
+# define BLKIF_NATIVE_PROTOCOL 1
+#elif defined(__x86_64__) || defined(__ia64__)
+# define BLKIF_NATIVE_PROTOCOL 2
+#else
+# error arch fixup needed here
+#endif
+
+/* translate requests: v1/v2 to native */
+#if 1 == BLKIF_NATIVE_PROTOCOL
+static void inline blkif_get_v1_req(blkif_request_t *dst, blkif_v1_request_t *src)
+#else
+static void inline blkif_get_v2_req(blkif_request_t *dst, blkif_v2_request_t *src)
+#endif
+{
+	memcpy(dst, src, sizeof(*dst));
+}
+
+#if 1 == BLKIF_NATIVE_PROTOCOL
+static void inline blkif_get_v2_req(blkif_request_t *dst, blkif_v2_request_t *src)
+#else
+static void inline blkif_get_v1_req(blkif_request_t *dst, blkif_v1_request_t *src)
+#endif
+{
+	int i;
+	dst->operation = src->operation;
+	dst->nr_segments = src->nr_segments;
+	dst->handle = src->handle;
+	dst->id = src->id;
+	dst->sector_number = src->sector_number;
+	for (i = 0; i < src->nr_segments; i++)
+		dst->seg[i] = src->seg[i];
+}
+
+#endif /* __XEN_BLKIF_H__ */

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

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

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Keir Fraser wrote:
> Make tools
> write a default value for pv guests for backward compat.

Here is a one-line fix to make the pvfb frontend clear the shared page.
 Can this go into both 3.0.4 and unstable please?

thanks,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: fbif-bimodal.diff --]
[-- Type: text/x-patch, Size: 689 bytes --]

---
 linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
@@ -479,7 +479,7 @@ static int __devinit xenfb_probe(struct 
 		goto error_nomem;
 
 	/* set up shared page */
-	info->page = (void *)__get_free_page(GFP_KERNEL);
+	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
 

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

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

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Gerd Hoffmann <kraxel@suse.de> writes:

[...]
> Here is a one-line fix to make the pvfb frontend clear the shared page.
>  Can this go into both 3.0.4 and unstable please?

No objections to clearing the shared page, just keep in mind that any
scheme to guess the page layout that relies on the page being cleared
is playing a dicey game with 3.0.3 frontends.

Re: 32-on-64: pvfb issue

[Ian Campbell]

On Thu, 2007-01-18 at 18:31 +0000, Keir Fraser wrote:
> This is even better than with blkfront/blkback because there are definitely
> no PV-on-HVM pvfb driver frontends out in the wild (and it's HVM that makes
> things harder).

PV on HVM for fbfront is pretty hard since the kernel.org tree does not
export zap_page_range to modules, neither do many of the older distro
kernels I've looked at.

I tried doing a compat version like with other such functions but it
ended up pulling half of mm/memory.c into the compat layer which I don't
like.

Do I vaguely recall plans to get rid of the use of zap_page_range or is
my memory playing tricks?

Ian.

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@suse.de> writes:
> 
> [...]
>> Here is a one-line fix to make the pvfb frontend clear the shared page.
>>  Can this go into both 3.0.4 and unstable please?
> 
> No objections to clearing the shared page, just keep in mind that any
> scheme to guess the page layout that relies on the page being cleared
> is playing a dicey game with 3.0.3 frontends.

3.0.3?  Wasn't pvfb merged in the 3.0.4 cycle?  Or do you talk about
what FC6 ships?

As the 3.0.4 frontends use hard-coded 800x600-32 which needs one page
directory only I think it is possible to keep them apart even in case
they don't carry the protocol info ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 19/1/07 09:54, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> Make tools
>> write a default value for pv guests for backward compat.
> 
> Here is a one-line fix to make the pvfb frontend clear the shared page.
>  Can this go into both 3.0.4 and unstable please?

How about a patch to clear the page *and* write a newly-defined protocol
field? Or are you still planning to kludge the bitwidth check in the
backend?

 -- Keir

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Keir Fraser wrote:
> 
> 
> On 19/1/07 09:54, "Gerd Hoffmann" <kraxel@suse.de> wrote:
> 
>>> Make tools
>>> write a default value for pv guests for backward compat.
>> Here is a one-line fix to make the pvfb frontend clear the shared page.
>>  Can this go into both 3.0.4 and unstable please?
> 
> How about a patch to clear the page *and* write a newly-defined protocol
> field? Or are you still planning to kludge the bitwidth check in the
> backend?

That is better, yes.  Minimum patch attached.  For unstable I'll brew a
nicer version with defines and so on when adjusting the backend code to
be able to deal with both protocols.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: fbif-bimodal.diff --]
[-- Type: text/x-patch, Size: 1996 bytes --]

---
 linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c |    8 +++++++-
 xen/include/public/io/fbif.h                     |    5 +++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
@@ -479,7 +479,7 @@ static int __devinit xenfb_probe(struct 
 		goto error_nomem;
 
 	/* set up shared page */
-	info->page = (void *)__get_free_page(GFP_KERNEL);
+	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
 
@@ -600,6 +600,12 @@ static void xenfb_init_shared_page(struc
 	for (i = 0; i < info->nr_pages; i++)
 		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
 
+#if defined(__i386__)
+	info->page->protocol = 1;
+#elif defined(__x86_64__) || defined(__ia64__)
+	info->page->protocol = 2;
+#endif
+
 	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
 	info->page->pd[1] = 0;
 	info->page->width = XENFB_WIDTH;
Index: build-32-unstable-13495/xen/include/public/io/fbif.h
===================================================================
--- build-32-unstable-13495.orig/xen/include/public/io/fbif.h
+++ build-32-unstable-13495/xen/include/public/io/fbif.h
@@ -102,6 +102,11 @@ struct xenfb_page
     uint32_t line_length;   /* the length of a row of pixels (in bytes) */
     uint32_t mem_length;    /* the length of the framebuffer (in bytes) */
     uint8_t depth;          /* the depth of a pixel (in bits) */
+    uint8_t protocol;       /* protocol version
+                             *    1 -- page directory: i386
+                             *    2 -- page directory: x86_64, ia64
+                             *    3 -- grant tables [not yet]
+                             */
 
     /*
      * Framebuffer page directory

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

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

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Gerd Hoffmann <kraxel@suse.de> writes:

> Markus Armbruster wrote:
>> Gerd Hoffmann <kraxel@suse.de> writes:
>> 
>> [...]
>>> Here is a one-line fix to make the pvfb frontend clear the shared page.
>>>  Can this go into both 3.0.4 and unstable please?
>> 
>> No objections to clearing the shared page, just keep in mind that any
>> scheme to guess the page layout that relies on the page being cleared
>> is playing a dicey game with 3.0.3 frontends.
>
> 3.0.3?  Wasn't pvfb merged in the 3.0.4 cycle?  Or do you talk about
> what FC6 ships?

Sorry, I meant 3.0.4.

> As the 3.0.4 frontends use hard-coded 800x600-32 which needs one page
> directory only I think it is possible to keep them apart even in case
> they don't carry the protocol info ...

Okay.

If that turns out not to work, consider turning depth into your
version number, starting with version 32.  Evil, ugly, but no guessing
games.  New frontends won't work with old backends, but that's okay.

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 19/1/07 11:43, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> How about a patch to clear the page *and* write a newly-defined protocol
>> field? Or are you still planning to kludge the bitwidth check in the
>> backend?
> 
> That is better, yes.  Minimum patch attached.  For unstable I'll brew a
> nicer version with defines and so on when adjusting the backend code to
> be able to deal with both protocols.

Thinking about this some more -- isn't it a bit skank to have the protocol
field embedded in the middle of the structure which it effectively defines?
Pvfb does interact with xenbus already, so poking a protocol field in there
seems reasonable and makes the scheme the same as blkfront/blkback.
Furthermore, as I already pointed out, this allows tools to poke a default
value and hence we can support e.g., old 32-bit frontends on new 64-bit
backend.

Furthermore, if we use a xenstore node then we are not restricted to a
protocol number. This is rather nice because it allows us to give more
meaningful names to our supported protocols. Right now, since we make no
effort to ensure protocol compat across machine architectures (for example
we use native endianness) I suggest that we define a per-architecture
protocol name: 'x86_32', 'x86_64', 'ia64', 'powerpc64', etc. Yes, some of
these protocols are actually identical but I don't think this redundancy in
the definition of the protocol field matters at all -- a little bit more
information can sometimes be useful, and this definition of the protocol
field is imo clearer than a simple enumeration.

 -- Keir

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Ian Campbell <Ian.Campbell@xensource.com> writes:

> On Thu, 2007-01-18 at 18:31 +0000, Keir Fraser wrote:
>> This is even better than with blkfront/blkback because there are definitely
>> no PV-on-HVM pvfb driver frontends out in the wild (and it's HVM that makes
>> things harder).
>
> PV on HVM for fbfront is pretty hard since the kernel.org tree does not
> export zap_page_range to modules, neither do many of the older distro
> kernels I've looked at.
>
> I tried doing a compat version like with other such functions but it
> ended up pulling half of mm/memory.c into the compat layer which I don't
> like.
>
> Do I vaguely recall plans to get rid of the use of zap_page_range or is
> my memory playing tricks?
>
> Ian.

We'd like to replace zap_page_range() with unmap_mapping_range(), for
several good reasons:

* it is already fully exported,

* it deals with locking automatically via the address_space
  i_mmap_lock spinlock,

* it automatically iterates over all the vmas on the address_space
  without us having to loop over them ourselves.

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Keir Fraser wrote:
> On 19/1/07 11:43, "Gerd Hoffmann" <kraxel@suse.de> wrote:
> 
>>> How about a patch to clear the page *and* write a newly-defined protocol
>>> field? Or are you still planning to kludge the bitwidth check in the
>>> backend?
>> That is better, yes.  Minimum patch attached.  For unstable I'll brew a
>> nicer version with defines and so on when adjusting the backend code to
>> be able to deal with both protocols.
> 
> Thinking about this some more -- isn't it a bit skank to have the protocol
> field embedded in the middle of the structure which it effectively defines?

When writing something new I would have placed it at the start of the
struct, but with the situation we have now it is more important not
shuffle the fields around.  All the fields before the new protocol field
have a fixed size and no alignment problems, so you can be sure the
protocol field has a fixed place everythere.  Changing the struct is
only possible after the protocol field of course.  The only pending
change is switching from the page directory to grant tables, so this
isn't too bad.

> Pvfb does interact with xenbus already, so poking a protocol field in there
> seems reasonable and makes the scheme the same as blkfront/blkback.

Would be more consistent across drivers, yes.  I still think it would be
placed better in the struct next to the fb config, but if you insist on
having it in xenstore I can do it that way too.

> Right now, since we make no
> effort to ensure protocol compat across machine architectures (for example
> we use native endianness) I suggest that we define a per-architecture
> protocol name: 'x86_32', 'x86_64', 'ia64', 'powerpc64', etc.

Hmm, not sure I like that idea, especially for pvfb as there certainly
will come the protocol switch to grant tables, so using the arch names
doesn't sound like a good idea to me.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 19/1/07 12:59, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> Right now, since we make no
>> effort to ensure protocol compat across machine architectures (for example
>> we use native endianness) I suggest that we define a per-architecture
>> protocol name: 'x86_32', 'x86_64', 'ia64', 'powerpc64', etc.
> 
> Hmm, not sure I like that idea, especially for pvfb as there certainly
> will come the protocol switch to grant tables, so using the arch names
> doesn't sound like a good idea to me.

Then we would make the protocol name structural: '<arch>-v2' rather than the
extending an enumeration to 3 and 4 (in your scheme). Or take advantage of
the stringness and call it '<arch>-grant'. Personally I'm of the opinion
that the architectural ABI is a fundamental component of our protocol (in
fact, the very component that is tripping us up here!). And magic numbers
suck compared with intelligible strings for this kind of thing imo. However,
I am open to persuasive arguments on this point. I'm not quite as stuck on
it as I am regarding use of xenbus for this field: it just occurred to me as
a seemingly neat extensible technique. :-)

 -- Keir

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Keir Fraser wrote:

> Then we would make the protocol name structural: '<arch>-v2' rather than the
> extending an enumeration to 3 and 4 (in your scheme). Or take advantage of
> the stringness and call it '<arch>-grant'.

Hmm, well, I think we never ever should need "<arch>-v2".  It would suck
big time if we manage to layout the structs for some future protocol
version in a way which is abi-dependent *again*.

> Personally I'm of the opinion
> that the architectural ABI is a fundamental component of our protocol (in
> fact, the very component that is tripping us up here!).

Yep.

> And magic numbers
> suck compared with intelligible strings for this kind of thing imo.

We'll need both then.  Strings are certainly nice for human-visible
stuff, but you don't want to strcmp() all the time in the backend.
Wouldn't be a problem for pvfb, but for blkfront/back where the ring
protocol is abi-dependent and thus the backend has to check often.

> I'm not quite as stuck on
> it as I am regarding use of xenbus for this field: it just occurred to me as
> a seemingly neat extensible technique. :-)

How about the patch below?  It adds both names and enums for the
protocols, the enums to be used internally in the backend, the names for
xenbus, plus conversion helpers (not used yet, want to collect feedback
on the idea first).

Location most likely isn't final, not sure where to place that best so
all parties involved can see it, probably xen/include/public/io/

comments?

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 19/1/07 15:08, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> And magic numbers
>> suck compared with intelligible strings for this kind of thing imo.
> 
> We'll need both then.  Strings are certainly nice for human-visible
> stuff, but you don't want to strcmp() all the time in the backend.
> Wouldn't be a problem for pvfb, but for blkfront/back where the ring
> protocol is abi-dependent and thus the backend has to check often.

You missed out the patch. I'm sure however that I'll argue you should make
the enumeration local to the backend. It will always support his native
architecture. Where it supports cross-architecture (i386-on-x64) he can
*privately* have a numeric assignment for that situation which it uses on
data paths. Then we don't have redundant info in xenstore and we don't get
tied to particular magic numbers.

But I definitely agree that a private enumeration, or sets of accessor hook
functions, makes sense. We'll certainly need one or the other for
efficiency.

 -- Keir

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Keir Fraser wrote:
> You missed out the patch.

Oops, attached.

> I'm sure however that I'll argue you should make
> the enumeration local to the backend. It will always support his native
> architecture. Where it supports cross-architecture (i386-on-x64) he can
> *privately* have a numeric assignment for that situation which it uses on
> data paths. Then we don't have redundant info in xenstore and we don't get
> tied to particular magic numbers.

I don't want to put numbers into xenstore.  But there are multiple
backends affected (pvfb, blktab, blkback, tpm, maybe more) and thus it
would be useful to share the infrastructure IMHO ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: protocol-bimodal.diff --]
[-- Type: text/x-patch, Size: 1924 bytes --]

---
 linux-2.6-xen-sparse/include/xen/protocols.h |   57 +++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

Index: build-32-unstable-13495/linux-2.6-xen-sparse/include/xen/protocols.h
===================================================================
--- /dev/null
+++ build-32-unstable-13495/linux-2.6-xen-sparse/include/xen/protocols.h
@@ -0,0 +1,57 @@
+#ifndef __XEN_PROTOCOLS_H__
+#define __XEN_PROTOCOLS_H__
+
+enum xen_io_proto_id {
+	/* For all front/backends */
+	XEN_IO_PROTO_ABI_X86_32    = 1,
+	XEN_IO_PROTO_ABI_X86_64    = 2,
+	XEN_IO_PROTO_ABI_IA64      = 3,
+	XEN_IO_PROTO_ABI_POWERPC64 = 4,
+
+	/* framebuffer via grant tables on little endian machines */
+	XEN_IO_PROTO_FB_LE_GRANT   = 5,
+};
+
+static const char *xen_io_proto_names[] = {
+	[ XEN_IO_PROTO_ABI_X86_32 ]    = "x86_32-abi",
+	[ XEN_IO_PROTO_ABI_X86_64 ]    = "x86_64-abi",
+	[ XEN_IO_PROTO_ABI_IA64 ]      = "ia64-abi",
+	[ XEN_IO_PROTO_ABI_POWERPC64 ] = "powerpc64-abi",
+
+	[ XEN_IO_PROTO_FB_LE_GRANT ]   = "fb-le-grant",
+};
+#define XEN_IO_PROTO_COUNT (sizeof(xen_io_proto_names)/sizeof(xen_io_proto_names[0]))
+
+static enum xen_io_proto_id xen_io_proto_name2id(const char *name)
+{
+	int id;
+
+	for (id = 0; id < XEN_IO_PROTO_COUNT; id++) {
+		if (!xen_io_proto_names[id])
+			continue;
+		if (0 != strcmp(name, xen_io_proto_names[id]))
+			continue;
+		return i;
+	}
+}
+
+static const char *xen_io_proto_id2name(enum xen_io_proto_id id)
+{
+	if (id >= XEN_IO_PROTO_COUNT)
+		return NULL;
+	return xen_io_proto_names[id];
+}
+
+#if defined(__i386__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_32
+#elif defined(__x86_64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_64
+#elif defined(__ia64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_IA64
+#elif defined(__powerpc64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_POWERPC64
+#else
+# error arch fixup needed here
+#endif
+
+#endif

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

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

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 19/1/07 15:31, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> I'm sure however that I'll argue you should make
>> the enumeration local to the backend. It will always support his native
>> architecture. Where it supports cross-architecture (i386-on-x64) he can
>> *privately* have a numeric assignment for that situation which it uses on
>> data paths. Then we don't have redundant info in xenstore and we don't get
>> tied to particular magic numbers.
> 
> I don't want to put numbers into xenstore.  But there are multiple
> backends affected (pvfb, blktab, blkback, tpm, maybe more) and thus it
> would be useful to share the infrastructure IMHO ...

And we can do so. xenbus_get_native_protocol()? Frontends can write the
returned string; backends can strcmp with the returned string (and usually
fail on mismatch). The few mismatches we do care about will result in us
executing driver-specific code anyway: drivers can declare 'native' ABI to
be 0 and have special-case driver-specific non-zero values for the
non-native protocols they care about. Would that actually be more code than
the potentially-knows-about-every-driver-in-the-world approach of
protocols.h?

If we can agree on a location for the protocol field (same directory as the
xenbus state field?), and a set of names (yours are fine, including the
'-abi' suffix), and a time in xenbus state machine to write the protocol (as
early as possible I guess!) then let's get the frontend machinery in place
first. Then we can continue to thrash out the backend details.

 -- Keir

Re: 32-on-64: pvfb issue

[Markus Armbruster]

I really don't understand why we need this level of generality and
complexity, in particular when a simple hypercall to query a domain's
width would do.  Or a simple, stupid version number in the shared
page.  We'll hardly end up with an unmanageable number of versions.

[Patch] [VTPM_TOOLS] Add HVM support to vtpm_manager

[Scarlata, Vincent R]

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

VTPM_TOOLS: Added support for QEMU to communicate with vTPM over UNIX
socket for HVM guests.

Signed-off-by: Vinnie Scarlata <vincent.r.scarlata@intel.com>

[-- Attachment #2: vtpm_tools-01182007.patch --]
[-- Type: application/octet-stream, Size: 27759 bytes --]

# HG changeset patch
# User root@TPRL-Linux-D2.jf.intel.com
# Node ID 1be602b84f6dba3ea10dabb5da88859410728194
# Parent  003d56dec2ea9ec9c3e2120cabc244d66d56bac6

VTPM_TOOLS: Added support for QEMU to communicate with vTPM over UNIX socket for HVM guests.

diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/manager/vtpm_manager.h
--- a/tools/vtpm_manager/manager/vtpm_manager.h	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/manager/vtpm_manager.h	Fri Jan 19 00:19:24 2007
@@ -70,6 +70,10 @@
 #define VTPM_ORD_MIGRATE_OUT  (VTPM_PRIV_BASE + 5) // migrate VTPM to dest 
 
 //************************ Return Codes ****************************
+#define VTPM_TYPE_PVM 1 // Paravirtualized Domain
+#define VTPM_TYPE_HVM 2 // HVM Domain
+
+//************************ Return Codes ****************************
 #define VTPM_SUCCESS               0
 #define VTPM_FAIL                  1
 #define VTPM_UNSUPPORTED           2
@@ -104,8 +108,9 @@
 
 VTPM_Open:
   Input Parameters:
-    Domain_type: 1 byte 
+    mig_type: 1 byte 
     startup_mode: 1 byte // Cold Boot = 1, resume = 2, deactive = 3
+    domain type: 1 byte
     instance_id: 4 bytes
   Output Parameters:
     None
diff -r 003d56dec2ea -r 1be602b84f6d tools/examples/vtpm-impl
--- a/tools/examples/vtpm-impl	Wed Jan 10 00:29:32 2007
+++ b/tools/examples/vtpm-impl	Fri Jan 19 00:19:24 2007
@@ -32,11 +32,14 @@
 # OF THE POSSIBILITY OF SUCH DAMAGE.
 # ===================================================================
 
-#            |        SRC        |    TAG  |      CMD SIZE     |        ORD       | type| mode
-TPM_CMD_OPEN=\\x00\\x00\\x00\\x00\\x01\\xc1\\x00\\x00\\x00\\x10\\x01\\x00\\x00\\x01\\x01\\x01
-TPM_CMD_RESM=\\x00\\x00\\x00\\x00\\x01\\xc1\\x00\\x00\\x00\\x10\\x01\\x00\\x00\\x01\\x01\\x02
+#            |        SRC        |    TAG  |      CMD SIZE     |        ORD       |mtype|strt
+TPM_CMD_OPEN=\\x00\\x00\\x00\\x00\\x01\\xc1\\x00\\x00\\x00\\x11\\x01\\x00\\x00\\x01\\x01\\x01
+TPM_CMD_RESM=\\x00\\x00\\x00\\x00\\x01\\xc1\\x00\\x00\\x00\\x11\\x01\\x00\\x00\\x01\\x01\\x02
 TPM_CMD_CLOS=\\x00\\x00\\x00\\x00\\x01\\xc1\\x00\\x00\\x00\\x0e\\x01\\x00\\x00\\x02
 TPM_CMD_DELE=\\x00\\x00\\x00\\x00\\x01\\xc1\\x00\\x00\\x00\\x0e\\x01\\x00\\x00\\x03
+
+TPM_TYPE_PVM=\\x01
+TPM_TYPE_HVM=\\x02
 
 TPM_SUCCESS=00000000
 
@@ -80,11 +83,25 @@
  release_lock vtpm_mgr
 
  #return whether the command was successful
- if [ $resp_hex != $TPM_SUCCESS ]; then
+ if [ $resp_hex ne $TPM_SUCCESS ]; then
    vtpm_fatal_error=1
    false
   else
    true
+ fi
+}
+
+# Helper to get vm type to pass to vtpm_manager open/resume
+function vtpm_get_type() {
+ local inst=$(xenstore_read $XENBUS_PATH/frontend-id)
+ local vm=$(xenstore_read /local/domain/$inst/vm)
+ if [ "$vm" != "" ]; then
+  local ostype=$(xenstore-read $vm/image/ostype)
+  if [ "$ostype" == "hvm" ]; then
+   echo $TPM_TYPE_HVM;
+  else
+   echo $TPM_TYPE_PVM;
+  fi
  fi
 }
 
@@ -99,11 +116,13 @@
 
 # Setup vtpm instance for use.
 function vtpm_start() {
- $(vtpm_manager_cmd $TPM_CMD_OPEN $1)
+ local vmtype=$(vtpm_get_type);
+ $(vtpm_manager_cmd $TPM_CMD_OPEN$vmtype $1)
 }
 
 function vtpm_resume() {
- $(vtpm_manager_cmd $TPM_CMD_RESM $1)
+ local vmtype=$(vtpm_get_type);
+ $(vtpm_manager_cmd $TPM_CMD_RESM$vmtype $1)
 }
 
 # Reset the vtpm AKA clear PCRs
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/manager/vtpm_manager_handler.c
--- a/tools/vtpm_manager/manager/vtpm_manager_handler.c	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/manager/vtpm_manager_handler.c	Fri Jan 19 00:19:24 2007
@@ -40,6 +40,7 @@
 #include <stdio.h>
 #include <unistd.h>
 #include <string.h>
+#include <errno.h>
 
 #include "vtpm_manager.h"
 #include "vtpmpriv.h"
@@ -105,7 +106,7 @@
       for (i=0; i<size_read; i++) 
 	vtpmhandlerloginfomore(VTPM_LOG_VTPM_DEEP, "%x ", cmd_header[i]);
     } else {
-      vtpmhandlerlogerror(VTPM_LOG_VTPM, "%s can't read from ipc. Aborting... \n", thread_name);
+      vtpmhandlerlogerror(VTPM_LOG_VTPM, "%s can't read from ipc. Errono = %d. Aborting... \n", thread_name, errno);
       goto abort_command;
     }
 
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm/vtpm.patch
--- a/tools/vtpm/vtpm.patch	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm/vtpm.patch	Fri Jan 19 00:19:24 2007
@@ -1,14 +1,14 @@
 diff -uprN tpm_emulator/AUTHORS vtpm/AUTHORS
---- tpm_emulator/AUTHORS	2006-07-24 14:35:35.000000000 -0700
-+++ vtpm/AUTHORS	2006-07-24 14:35:35.000000000 -0700
+--- tpm_emulator/AUTHORS	2006-12-08 12:51:29.000000000 -0800
++++ vtpm/AUTHORS	2006-12-13 16:38:52.000000000 -0800
 @@ -1,3 +1,3 @@
  Mario Strasser <mast@gmx.net>
  Heiko Stamer <stamer@gaos.org> [DAA]
 -INTEL Corp <> [Dropped to Ring3]
 +INTEL Corp <> [VTPM Extensions]
 diff -uprN tpm_emulator/ChangeLog vtpm/ChangeLog
---- tpm_emulator/ChangeLog	2006-07-24 14:35:35.000000000 -0700
-+++ vtpm/ChangeLog	2006-07-24 14:35:35.000000000 -0700
+--- tpm_emulator/ChangeLog	2006-12-08 12:51:29.000000000 -0800
++++ vtpm/ChangeLog	2006-12-13 16:38:52.000000000 -0800
 @@ -1,5 +1,6 @@
  ????-??-?? Intel Corp
  	* Moved module out of kernel to run as a ring 3 app
@@ -17,9 +17,9 @@
  2006-06-23  Mario Strasser <mast@gmx.net>
  	* tpm_startup.c: behaviour of ST_CLEAR and storage of
 diff -uprN tpm_emulator/linux_module.h vtpm/linux_module.h
---- tpm_emulator/linux_module.h	2006-07-24 14:35:35.000000000 -0700
-+++ vtpm/linux_module.h	2006-07-24 14:35:35.000000000 -0700
-@@ -44,18 +44,21 @@
+--- tpm_emulator/linux_module.h	2006-12-08 12:51:29.000000000 -0800
++++ vtpm/linux_module.h	2007-01-09 14:49:06.000000000 -0800
+@@ -44,18 +44,26 @@
  #define TPM_DEVICE_NAME   "tpm"
  #define TPM_MODULE_NAME   "tpm_emulator"
  
@@ -31,8 +31,13 @@
 -                        __FILE__, __LINE__, ## __VA_ARGS__)
 +#define debug(fmt, ...) printf("TPMD[%d]: %s:%d: Debug: " fmt "\n", \
 +                        dmi_id, __FILE__, __LINE__, ## __VA_ARGS__)
++#define debug_nostop(fmt, ...) printf("TPMD[%d]: %s:%d: Debug: " fmt, \
++                        dmi_id, __FILE__, __LINE__, ## __VA_ARGS__)
++#define debug_more(fmt, ...) printf( fmt, ## __VA_ARGS__ )
  #else
  #define debug(fmt, ...) 
++#define debug_nostop(fmt, ...) 
++#define debug_more(fmt, ...)
  #endif
 -#define info(fmt, ...)  printf("TPMD: %s:%d: Info: " fmt "\n", \
 -                        __FILE__, __LINE__, ## __VA_ARGS__)
@@ -50,8 +55,8 @@
  /* memory allocation */
  
 diff -uprN tpm_emulator/Makefile vtpm/Makefile
---- tpm_emulator/Makefile	2006-07-24 14:35:35.000000000 -0700
-+++ vtpm/Makefile	2006-07-24 14:35:35.000000000 -0700
+--- tpm_emulator/Makefile	2006-12-08 12:51:29.000000000 -0800
++++ vtpm/Makefile	2006-12-13 16:38:52.000000000 -0800
 @@ -7,7 +7,7 @@
  COMPILE_ARCH    ?= $(shell uname -m | sed -e s/i.86/x86_32/)
  
@@ -83,9 +88,46 @@
  
  .PHONY: all install clean dist gmp version
 +
+diff -uprN tpm_emulator/tpm/tpm_capability.c vtpm/tpm/tpm_capability.c
+--- tpm_emulator/tpm/tpm_capability.c	2006-06-23 03:37:07.000000000 -0700
++++ vtpm/tpm/tpm_capability.c	2007-01-10 10:00:49.000000000 -0800
+@@ -136,8 +136,18 @@ static TPM_RESULT cap_property(UINT32 su
+ 
+     case TPM_CAP_PROP_TIS_TIMEOUT:
+       debug("[TPM_CAP_PROP_TIS_TIMEOUT]");
+-      /* TODO: TPM_CAP_PROP_TIS_TIMEOUT */
+-      return TPM_FAIL;
++      /* TODO: TPM_CAP_PROP_TIS_TIMEOUT: Measure these values and determine correct ones */
++      UINT32 len = *respSize = 16;
++      BYTE *ptr = *resp = tpm_malloc(*respSize);
++      if (ptr == NULL || 
++          tpm_marshal_UINT32(&ptr, &len, 200000) ||
++          tpm_marshal_UINT32(&ptr, &len, 200000) ||
++          tpm_marshal_UINT32(&ptr, &len, 200000) ||
++          tpm_marshal_UINT32(&ptr, &len, 200000)) {
++        tpm_free(*resp);
++        return TPM_FAIL;
++      }
++      return TPM_SUCCESS;
+ 
+     case TPM_CAP_PROP_STARTUP_EFFECT:
+       debug("[TPM_CAP_PROP_STARTUP_EFFECT]");
+@@ -190,7 +200,11 @@ static TPM_RESULT cap_property(UINT32 su
+ 
+     case TPM_CAP_PROP_DURATION:
+       debug("[TPM_CAP_PROP_DURATION]");
+-      /* TODO: TPM_CAP_PROP_DURATION */
++      /* TODO: TPM_CAP_PROP_DURATION: Measure these values and return accurate ones */
++      BYTE dur[]= {0x0,0x0,0x0,0xc,0x0,0x7,0xa1,0x20,0x0,0x1e,0x84,0x80,0x11,0xe1,0xa3,0x0}; 
++      *respSize = 16;
++      *resp = tpm_malloc(*respSize);
++      memcpy(*resp,dur,16); 
+       return TPM_FAIL;
+ 
+     case TPM_CAP_PROP_ACTIVE_COUNTER:
 diff -uprN tpm_emulator/tpm/tpm_data.c vtpm/tpm/tpm_data.c
---- tpm_emulator/tpm/tpm_data.c	2006-07-24 14:35:35.000000000 -0700
-+++ vtpm/tpm/tpm_data.c	2006-07-24 14:35:35.000000000 -0700
+--- tpm_emulator/tpm/tpm_data.c	2006-12-08 12:51:29.000000000 -0800
++++ vtpm/tpm/tpm_data.c	2006-12-13 16:38:52.000000000 -0800
 @@ -1,6 +1,7 @@
  /* Software-Based Trusted Platform Module (TPM) Emulator for Linux
   * Copyright (C) 2004 Mario Strasser <mast@gmx.net>,
@@ -371,10 +413,15 @@
  
  #else
 diff -uprN tpm_emulator/tpmd.c vtpm/tpmd.c
---- tpm_emulator/tpmd.c	2006-07-24 14:35:35.000000000 -0700
-+++ vtpm/tpmd.c	2006-07-24 14:35:35.000000000 -0700
-@@ -23,13 +23,27 @@
+--- tpm_emulator/tpmd.c	2006-12-08 12:51:29.000000000 -0800
++++ vtpm/tpmd.c	2007-01-09 14:48:56.000000000 -0800
+@@ -21,12 +21,24 @@
+ #include <sys/stat.h>
+ #include <fcntl.h>
  #include <sys/time.h>
++#include <sys/socket.h>
++#include <sys/un.h>
++#include <errno.h>
  
  #include "tpm_emulator.h"
 +#include "vtpm_manager.h"
@@ -384,61 +431,115 @@
 +#ifdef VTPM_MULTI_VM
 + #define DEV_BE "/dev/vtpm"
 +#else
-+ #define GUEST_RX_FIFO_D "/var/vtpm/fifos/tpm_cmd_to_%d.fifo"
-+ #define GUEST_TX_FIFO "/var/vtpm/fifos/tpm_rsp_from_all.fifo"
-+#endif
- 
++ #define PVM_RX_FIFO_D "/var/vtpm/fifos/tpm_cmd_to_%d.fifo"
++ #define PVM_TX_FIFO "/var/vtpm/fifos/tpm_rsp_from_all.fifo"
+ 
++ #define HVM_RX_FIFO_D "/var/vtpm/socks/%d.socket"
++#endif
++
 + int dmi_id;
 +						
  #define BUFFER_SIZE 2048
  
-+static uint8_t ctrl_msg[] = { 0, 0, 0, 0,   // destination
-+                              1, 193,       // VTPM_TAG
-+                              0, 0, 0, 10,  // Size
-+                              0, 0, 0, 0};  // TPM_SUCCESS
-+                            
-+
  static int devurandom=0;
-+
- 	  
- void get_random_bytes(void *buf, int nbytes) {
-   
-@@ -52,18 +66,26 @@ uint64_t tpm_get_ticks(void)
+@@ -38,7 +50,7 @@ void get_random_bytes(void *buf, int nby
+   }
+ 
+   if (read(devurandom, buf, nbytes) != nbytes) {
+-      printf("Can't get random number.\n");
++      error("Can't get random number.\n");
+       exit(-1);
+   }
+ }
+@@ -52,105 +64,182 @@ uint64_t tpm_get_ticks(void)
  
  int main(int argc, char **argv)
  {
 -  uint8_t in[BUFFER_SIZE], *out;
-+  uint8_t in[BUFFER_SIZE], *out, *addressed_out;
++  uint8_t type, in[BUFFER_SIZE], *out, *addressed_out;
++  char *vtpm_rx_file=NULL;
    uint32_t out_size;
    int in_size, written;
 -  int i;
 -  struct stat file_info;
--
++  int i, guest_id=-1;
+ 
 -  int tpm_tx_fh=-1, tpm_rx_fh=-1;
-+  int i, guest_id=-1;
++#ifndef VTPM_MULTI_VM
++  int sockfd = -1;
++  struct sockaddr_un addr;
++  struct sockaddr_un client_addr;
++  unsigned int client_length;
++
++#endif
 + 
 +  int vtpm_tx_fh=-1, vtpm_rx_fh=-1;
 +#ifdef VTPM_MULTI_VM
    if (argc < 2) {
-     printf("Usage: tpmd clear|save|deactivated\n" );
-+#else
-+  if (argc < 3) {
-+    printf("Usage: tpmd clear|save|deactivated vtpmid\n" );
+-    printf("Usage: tpmd clear|save|deactivated\n" );
++    error("Usage: tpmd clear|save|deactivated\n" );
++#else
++  if (argc < 4) {
++    error("Usage: tpmd clear|save|deactivated pvm|hvm vtpmid\n" );
 +#endif
  	  return -1;
    }
  
 +#ifndef VTPM_MULTI_VM
-+  dmi_id = atoi(argv[2]);
++  /* setup type of vm */
++  if (!strcmp(argv[2], "pvm")) {
++    type = VTPM_TYPE_PVM; // Get commands from vTPM Manager through fifo
++  } else if (!strcmp(argv[2], "hvm")) {
++    type = VTPM_TYPE_HVM; // Get commands from qemu via socket
++  } else {
++    error("invalid vTPM type '%s'.\n", argv[2]);
++  }
++
++  dmi_id = atoi(argv[3]);
++
++  if (type == VTPM_TYPE_PVM) {
++    vtpm_rx_file = malloc(10 + strlen(PVM_RX_FIFO_D));
++    sprintf(vtpm_rx_file, PVM_RX_FIFO_D, (uint32_t) dmi_id);
++  } else {
++    vtpm_rx_file = malloc(10 + strlen(HVM_RX_FIFO_D));
++    sprintf(vtpm_rx_file, HVM_RX_FIFO_D, (uint32_t) dmi_id);
++
++    if ( (sockfd = socket(PF_UNIX,SOCK_STREAM,0)) < 0) {
++          error("Unable to create socket. errno = %d\n", errno);
++      exit (-1);
++    }
++
++    memset(&addr, 0, sizeof(addr));
++    addr.sun_family = AF_UNIX;
++    strcpy(addr.sun_path,vtpm_rx_file );
++    unlink(addr.sun_path);
++  }
++#endif
++
++#ifdef VTPM_MULTI_VM
++  info("Initializing tpm state: %s\n", argv[1]);
++#else
++  info("Initializing tpm state: %s, type: %s, id: %d\n", argv[1], argv[2], dmi_id);
 +#endif
 +
    /* initialize TPM emulator */
    if (!strcmp(argv[1], "clear")) {
-     printf("Initializing tpm: %s\n", argv[1]);
-@@ -80,46 +102,30 @@ int main(int argc, char **argv)
+-    printf("Initializing tpm: %s\n", argv[1]);
+     tpm_emulator_init(1);
+-  } else if (!strcmp(argv[1], "save")) { 
+-    printf("Initializing tpm: %s\n", argv[1]);
++  } else if (!strcmp(argv[1], "save")) {
+     tpm_emulator_init(2);
+   } else if (!strcmp(argv[1], "deactivated")) {
+-    printf("Initializing tpm: %s\n", argv[1]);
+     tpm_emulator_init(3);
+   } else {
+-    printf("invalid startup mode '%s'; must be 'clear', "
++    error("invalid startup mode '%s'; must be 'clear', "
+       "'save' (default) or 'deactivated", argv[1]);
      return -1;
    }
- 
+-
 -  if ( stat(TPM_RX_FNAME, &file_info) == -1) {
 -    if ( mkfifo(TPM_RX_FNAME, S_IWUSR | S_IRUSR ) ) {
 -      printf("Failed to create fifo %s.\n", TPM_RX_FNAME);
@@ -453,8 +554,6 @@
 -    }
 -  }
 -
-+  char *guest_rx_file = malloc(10 + strlen(GUEST_RX_FIFO_D));
-+  sprintf(guest_rx_file, GUEST_RX_FIFO_D, (uint32_t) dmi_id);
 +  
    while (1) {
  abort_command:
@@ -462,15 +561,33 @@
 -      tpm_rx_fh = open(TPM_RX_FNAME, O_RDONLY);
 +    if (vtpm_rx_fh < 0) {
 +#ifdef VTPM_MUTLI_VM
-+	  vtpm_rx_fh = open(DEV_BE, O_RDWR);
-+#else
-+      vtpm_rx_fh = open(guest_rx_file, O_RDONLY);
++      vtpm_rx_fh = open(DEV_BE, O_RDWR);
++#else
++      if (type == VTPM_TYPE_PVM) {
++        vtpm_rx_fh = open(vtpm_rx_file, O_RDONLY);
++      } else {
++        if (bind(sockfd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
++          error("Unable to bind(). errno = %d\n", errno);
++          exit (-1);
++        }
++
++        if (listen(sockfd, 10) <0) {
++          error("Unable to listen(). errno = %d\n", errno);
++          exit (-1);
++        }
++
++        memset(&client_addr, 0, sizeof(client_addr));
++        client_length = sizeof(client_addr);
++
++        vtpm_rx_fh = vtpm_tx_fh = accept(sockfd, &client_addr, &client_length);
++      }
 +#endif
      }
      
 -    if (tpm_rx_fh < 0) {
+-      printf("ERROR: failed to open devices to listen to guest.\n");
 +    if (vtpm_rx_fh < 0) {
-       printf("ERROR: failed to open devices to listen to guest.\n");
++      error("Failed to open devices to listen to guest.\n");
        return -1;
      }
      
@@ -486,7 +603,8 @@
 -    in_size = read(tpm_rx_fh, in, BUFFER_SIZE);
 +    in_size = read(vtpm_rx_fh, in, BUFFER_SIZE);
      if (in_size < 6) { // Magic size of minium TPM command
-       printf("Recv[%d] to small: 0x", in_size);
+-      printf("Recv[%d] to small: 0x", in_size);
++      info("Recv incomplete command of %d bytes.", in_size);
        if (in_size <= 0) {
 -          close(tpm_rx_fh);
 -          tpm_rx_fh = -1;
@@ -495,8 +613,13 @@
            goto abort_command;
        }
      } else { 
-@@ -129,28 +135,73 @@ abort_command:
-       printf("\n");
+-      printf("Recv[%d]: 0x", in_size);
++      debug_nostop("Recv[%d]: 0x", in_size);
+       for (i=0; i< in_size; i++) 
+-        printf("%x ", in[i]);
+-      printf("\n");
++        debug_more("%x ", in[i]);
++      debug_more("\n");
      }
  
 -    
@@ -504,71 +627,56 @@
 -        printf("ERROR: Handler Failed.\n");
 +    if (guest_id == -1) {
 +        guest_id = *((uint32_t *) in);
-+        *((uint32_t *) ctrl_msg) = *((uint32_t *) in);
 +    } else {
 +        if (guest_id != *((uint32_t *) in) ) {
-+            printf("WARNING: More than one guest attached\n");
++            error("WARNING: More than one guest attached\n");
 +        }
++    }
++
++    if (vtpm_tx_fh < 0) {
++#ifdef VTPM_MUTLI_VM
++      vtpm_tx_fh = open(DEV_BE, O_RDWR);
++      vtpm_rx_fh = vtpm_tx_fh;
++#else
++      if (type == VTPM_TYPE_PVM) {
++        vtpm_tx_fh = open(PVM_TX_FIFO, O_WRONLY);
++      } // No need to open the other direction for HVM
++#endif
++    }
++
++    if (vtpm_tx_fh < 0) {
++      error("Failed to open devices to respond to guest.\n");
++      return -1;
++    }
++
++    // Handle the command, but skip the domain id header    
++    if (tpm_handle_command(in + sizeof(uint32_t), in_size - sizeof(uint32_t), &out, &out_size) != 0) { 
++      error("Handler Failed.\n");
      }
  
 -    written = write(tpm_tx_fh, out, out_size);
-+    if (vtpm_tx_fh < 0) {
-+#ifdef VTPM_MUTLI_VM
-+	  vtpm_tx_fh = open(DEV_BE, O_RDWR);
-+	  vtpm_rx_fh = vtpm_tx_fh;
-+#else
-+      vtpm_tx_fh = open(GUEST_TX_FIFO, O_WRONLY);
-+#endif
-+    }
++    addressed_out = (uint8_t *) tpm_malloc(sizeof(uint32_t) + out_size);
++    *(uint32_t *) addressed_out = *(uint32_t *) in;
++    memcpy(addressed_out + sizeof(uint32_t), out, out_size);
++
++    written = write(vtpm_tx_fh, addressed_out, out_size + sizeof(uint32_t));
  
 -    if (written != out_size ) {
 -      printf("ERROR: Part of response not written %d/%d.\nAttempt: ", written, out_size);
--    } else {
++    if (written != out_size + sizeof(uint32_t)) {
++      error("Part of response not written %d/%d.\n", written, out_size);
+     } else {
 -      printf("Sent[%Zu]: ", out_size);
-+    if (vtpm_tx_fh < 0) {
-+      printf("ERROR: failed to open devices to respond to guest.\n");
-+      return -1;
-+    }
-+    
-+    // Handle command, but we need to skip the identifier
-+    if (  BE16_TO_CPU( ((uint16_t *) in)[2] ) == VTPM_TAG_REQ ) { // Control message from xend
-+      // This DM doesn't really care about ctrl messages. Just ACK the message
-+      written = write(vtpm_tx_fh, ctrl_msg, sizeof(ctrl_msg));
-+
-+      if (written != sizeof(ctrl_msg)) {
-+        printf("ERROR: Part of response not written %d/%Zu.\n", written, sizeof(ctrl_msg));
-+      } else {
-+        printf("Send Ctrl Message confermation\n");
-+      }
-+    } else { // Message from Guest
-+      if (tpm_handle_command(in + sizeof(uint32_t), in_size - sizeof(uint32_t), &out, &out_size) != 0) { 
-+        printf("ERROR: Handler Failed.\n");
-+      }
-+
-+      addressed_out = (uint8_t *) tpm_malloc(sizeof(uint32_t) + out_size);
-+      *(uint32_t *) addressed_out = *(uint32_t *) in;
-+      memcpy(addressed_out + sizeof(uint32_t), out, out_size);
-+
-+      written = write(vtpm_tx_fh, addressed_out, out_size + sizeof(uint32_t));
-+
-+      if (written != out_size + sizeof(uint32_t)) {
-+        printf("ERROR: Part of response not written %d/%d.\n", written, out_size);
-+        for (i=0; i< out_size+ sizeof(uint32_t); i++)
-+          printf("%x ", addressed_out[i]);
-+        printf("\n");
-+      } else {
-+        printf("Sent[%Zu]: ", out_size + sizeof(uint32_t));
-+        for (i=0; i< out_size+ sizeof(uint32_t); i++)
-+          printf("%x ", addressed_out[i]);
-+        printf("\n");
-+      }
-+      tpm_free(out);
-+      tpm_free(addressed_out);
++      debug_nostop("Sent[%Zu]: ", out_size + sizeof(uint32_t));
++      for (i=0; i< out_size+ sizeof(uint32_t); i++)
++        debug_more("%x ", addressed_out[i]);
++      debug_more("\n");
      }
 -    for (i=0; i< out_size; i++)
 -      printf("%x ", out[i]);
 -    printf("\n");
--    tpm_free(out);
+     tpm_free(out);
++    tpm_free(addressed_out);
  
    } // loop
  
@@ -579,19 +687,7 @@
 +  close(vtpm_tx_fh);
 +#ifndef VTPM_MUTLI_VM
 +  close(vtpm_rx_fh);
-+  free (guest_rx_file);
++  free (vtpm_rx_file);
 +#endif
  
  }
-Binary files tpm_emulator/tpm_emulator and vtpm/tpm_emulator differ
-diff -uprN tpm_emulator/tpm_version.h vtpm/tpm_version.h
---- tpm_emulator/tpm_version.h	2006-07-24 14:35:41.000000000 -0700
-+++ vtpm/tpm_version.h	2006-07-24 14:35:35.000000000 -0700
-@@ -2,5 +2,5 @@
- #define _TPM_VERSION_H_
- #define VERSION_MAJOR 0
- #define VERSION_MINOR 4
--#define VERSION_BUILD 1153776940
-+#define VERSION_BUILD 1153776935
- #endif /* _TPM_VERSION_H_ */
-Binary files tpm_emulator/vtpmd and vtpm/vtpmd differ
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/manager/dmictl.c
--- a/tools/vtpm_manager/manager/dmictl.c	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/manager/dmictl.c	Fri Jan 19 00:19:24 2007
@@ -54,7 +54,7 @@
 // if dmi_res is non-null, then return a pointer to new object.
 // Also, this does not fill in the measurements. They should be filled by
 // design dependent code or saveNVM
-TPM_RESULT init_dmi(UINT32 dmi_id, BYTE type,  VTPM_DMI_RESOURCE **dmi_res) {
+TPM_RESULT init_dmi(UINT32 dmi_id, BYTE dmi_type, VTPM_DMI_RESOURCE **dmi_res) {
 
   TPM_RESULT status=TPM_SUCCESS;
   VTPM_DMI_RESOURCE *new_dmi=NULL;
@@ -66,6 +66,7 @@
   }
   memset(new_dmi, 0, sizeof(VTPM_DMI_RESOURCE));
   new_dmi->dmi_id = dmi_id;
+  new_dmi->dmi_type = dmi_type;
   new_dmi->connected = FALSE;
   new_dmi->TCSContext = 0;
 
@@ -120,47 +121,46 @@
   
   VTPM_DMI_RESOURCE *new_dmi=NULL;
   TPM_RESULT status=TPM_FAIL;
-  BYTE type, startup_mode;
+  BYTE dmi_type, vm_type, startup_mode;
   UINT32 dmi_id; 
 
   if (param_buf == NULL) { // Assume creation of Dom 0 control
-    type = VTPM_TYPE_NON_MIGRATABLE;
+    dmi_type = VTPM_TYPE_NON_MIGRATABLE;
     dmi_id = VTPM_CTL_DM;
-  } else if (buffer_len(param_buf) != sizeof(BYTE) + sizeof(BYTE) + sizeof(UINT32)) {
+  } else if (buffer_len(param_buf) != sizeof(BYTE) * 3  + sizeof(UINT32)) {
     vtpmloginfo(VTPM_LOG_VTPM, "New DMI command wrong length: %d.\n", buffer_len(param_buf));
     status = TPM_BAD_PARAMETER;
     goto abort_egress;
   } else {
     vtpm_globals->connected_dmis++; // Put this here so we don't count Dom0
-    BSG_UnpackList( param_buf->bytes, 3,
-		    BSG_TYPE_BYTE, &type,
+    BSG_UnpackList( param_buf->bytes, 4,
+		    BSG_TYPE_BYTE, &dmi_type,
 		    BSG_TYPE_BYTE, &startup_mode,
+		    BSG_TYPE_BYTE, &vm_type,
 		    BSG_TYPE_UINT32,  &dmi_id);
+  }
+
+  if ((dmi_type != VTPM_TYPE_NON_MIGRATABLE) && (dmi_type != VTPM_TYPE_MIGRATABLE)) {
+    vtpmlogerror(VTPM_LOG_VTPM, "Creation of VTPM with illegal type.\n");
+    status = TPM_BAD_PARAMETER;
+    goto abort_egress;
   }
 
   new_dmi = (VTPM_DMI_RESOURCE *) hashtable_search(vtpm_globals->dmi_map, &dmi_id);
   if (new_dmi == NULL) { 
     vtpmloginfo(VTPM_LOG_VTPM, "Creating new DMI instance %d attached.\n", dmi_id );
     // Brand New DMI. Initialize the persistent pieces
-    TPMTRYRETURN(init_dmi(dmi_id, type, &new_dmi) );  
+    TPMTRYRETURN(init_dmi(dmi_id, dmi_type, &new_dmi) );  
   } else 
     vtpmloginfo(VTPM_LOG_VTPM, "Re-attaching DMI instance %d.\n", dmi_id);
 
-  if (type != VTPM_TYPE_MIGRATED) {
-    new_dmi->dmi_type = type;
-  } else {
-    vtpmlogerror(VTPM_LOG_VTPM, "Creation of VTPM with illegal type.\n");
-    status = TPM_BAD_PARAMETER;
-    goto abort_egress;
-  }
-  
   if (new_dmi->connected) {
     vtpmlogerror(VTPM_LOG_VTPM, "Attempt to re-attach, currently attached instance %d. Ignoring\n", dmi_id);
     status = TPM_BAD_PARAMETER;
     goto abort_egress;
   }
   
-  if (type == VTPM_TYPE_MIGRATED) {
+  if (new_dmi->dmi_type == VTPM_TYPE_MIGRATED) {
     vtpmlogerror(VTPM_LOG_VTPM, "Attempt to re-attach previously migrated instance %d without recovering first. Ignoring\n", dmi_id);
     status = TPM_BAD_PARAMETER;
     goto abort_egress;
@@ -173,7 +173,7 @@
 
   // Design specific new DMI code. 
   // Includes: create IPCs, Measuring DMI, and maybe launching DMI
-  status = VTPM_New_DMI_Extra(new_dmi, startup_mode);
+  TPMTRYRETURN(VTPM_New_DMI_Extra(new_dmi, vm_type, startup_mode) );
   goto egress;
   
  abort_egress:
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/manager/vtpmd.c
--- a/tools/vtpm_manager/manager/vtpmd.c	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/manager/vtpmd.c	Fri Jan 19 00:19:24 2007
@@ -63,6 +63,9 @@
 #define VTPM_TX_HP_FNAME       "/var/vtpm/fifos/to_console.fifo"
 #define VTPM_RX_HP_FNAME       "/var/vtpm/fifos/from_console.fifo"
 
+#define VTPM_TYPE_PVM_STRING "pvm"
+#define VTPM_TYPE_HVM_STRING "hvm"
+
 struct vtpm_thread_params_s {
   vtpm_ipc_handle_t *tx_ipc_h;
   vtpm_ipc_handle_t *rx_ipc_h;
@@ -104,12 +107,12 @@
 
 struct sigaction ctl_c_handler;
 
-TPM_RESULT VTPM_New_DMI_Extra(VTPM_DMI_RESOURCE *dmi_res, BYTE startup_mode) {
+TPM_RESULT VTPM_New_DMI_Extra(VTPM_DMI_RESOURCE *dmi_res, BYTE vm_type, BYTE startup_mode) {
 
   TPM_RESULT status = TPM_SUCCESS;
   int fh;
   char dmi_id_str[11]; // UINT32s are up to 10 digits + NULL
-  char *tx_vtpm_name, *tx_tpm_name;
+  char *tx_vtpm_name, *tx_tpm_name, *vm_type_string;
   struct stat file_info;
 
   if (dmi_res->dmi_id == VTPM_CTL_DM) {
@@ -156,6 +159,10 @@
     */
     memset(&dmi_res->DMI_measurement, 0xcc, sizeof(TPM_DIGEST));
 
+    if (vm_type == VTPM_TYPE_PVM)
+      vm_type_string = (BYTE *)&VTPM_TYPE_PVM_STRING;
+    else
+      vm_type_string = (BYTE *)&VTPM_TYPE_HVM_STRING;
 
     // Launch DMI
     sprintf(dmi_id_str, "%d", (int) dmi_res->dmi_id);
@@ -172,13 +179,13 @@
     } else if (pid == 0) {
       switch (startup_mode) {
       case TPM_ST_CLEAR:
-        execl (TPM_EMULATOR_PATH, "vtpmd", "clear", dmi_id_str, NULL);
+        execl (TPM_EMULATOR_PATH, "vtpmd", "clear", vm_type_string, dmi_id_str, NULL);
         break;
       case TPM_ST_STATE:
-        execl (TPM_EMULATOR_PATH, "vtpmd", "save", dmi_id_str, NULL);
+        execl (TPM_EMULATOR_PATH, "vtpmd", "save", vm_type_string, dmi_id_str, NULL);
         break;
       case TPM_ST_DEACTIVATED:
-        execl (TPM_EMULATOR_PATH, "vtpmd", "deactivated", dmi_id_str, NULL);
+        execl (TPM_EMULATOR_PATH, "vtpmd", "deactivated", vm_type_string, dmi_id_str, NULL);
         break;
       default:
         status = TPM_BAD_PARAMETER;
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/Rules.mk
--- a/tools/vtpm_manager/Rules.mk	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/Rules.mk	Fri Jan 19 00:19:24 2007
@@ -39,7 +39,7 @@
 CFLAGS += -D_GNU_SOURCE
 
 # Logging Level. See utils/tools.h for usage
-CFLAGS += -DLOGGING_MODULES="(BITMASK(VTPM_LOG_TCS)|BITMASK(VTPM_LOG_VTSP)|BITMASK(VTPM_LOG_VTPM)|BITMASK(VTPM_LOG_VTPM_DEEP))"
+CFLAGS += -DLOGGING_MODULES="(BITMASK(VTPM_LOG_TCS)|BITMASK(VTPM_LOG_VTSP)|BITMASK(VTPM_LOG_VTPM))"
 
 # Silent Mode
 #CFLAGS += -DLOGGING_MODULES=0x0
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/manager/vtpmpriv.h
--- a/tools/vtpm_manager/manager/vtpmpriv.h	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/manager/vtpmpriv.h	Fri Jan 19 00:19:24 2007
@@ -165,7 +165,7 @@
 TPM_RESULT VTPM_SaveManagerData(void);
 TPM_RESULT VTPM_LoadManagerData(void);
 
-TPM_RESULT VTPM_New_DMI_Extra(VTPM_DMI_RESOURCE *dmi_res, BYTE startup_mode);
+TPM_RESULT VTPM_New_DMI_Extra(VTPM_DMI_RESOURCE *dmi_res, BYTE vm_type, BYTE startup_mode);
 
 TPM_RESULT VTPM_Close_DMI_Extra(VTPM_DMI_RESOURCE *dmi_res);
 
diff -r 003d56dec2ea -r 1be602b84f6d tools/vtpm_manager/manager/Makefile
--- a/tools/vtpm_manager/manager/Makefile	Wed Jan 10 00:29:32 2007
+++ b/tools/vtpm_manager/manager/Makefile	Fri Jan 19 00:19:24 2007
@@ -13,6 +13,9 @@
 install: build
 	if [ ! -d "$(DESTDIR)/var/vtpm/fifos" ]; \
 		then mkdir -p $(DESTDIR)/var/vtpm/fifos; \
+	fi
+	if [ ! -d "$(DESTDIR)/var/vtpm/socks" ]; \
+		then mkdir -p $(DESTDIR)/var/vtpm/socks; \
 	fi
 	$(INSTALL_PROG) $(BIN) $(TOOLS_INSTALL_DIR)
 

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

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

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

  Hi,

> And we can do so. xenbus_get_native_protocol()? Frontends can write the
> returned string; backends can strcmp with the returned string (and usually
> fail on mismatch). The few mismatches we do care about will result in us
> executing driver-specific code anyway: drivers can declare 'native' ABI to
> be 0 and have special-case driver-specific non-zero values for the
> non-native protocols they care about. Would that actually be more code than
> the potentially-knows-about-every-driver-in-the-world approach of
> protocols.h?

I'll go code up both front and back bits for block and pvfb to see how
it works out in practice, I think I'll have patches later today or
tomorrow ...

> If we can agree on a location for the protocol field (same directory as the
> xenbus state field?),

Yes.

> and a set of names (yours are fine, including the
> '-abi' suffix),

Yep, those I'll plan to put into a common header file so they can be
shared in any case.  Not sure any more how useful sharing the enum for
the protocol actually is, I'll see when coding up things.

> and a time in xenbus state machine to write the protocol

Same transaction event-channel is written.

stay tuned,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Markus Armbruster wrote:
> I really don't understand why we need this level of generality and
> complexity, in particular when a simple hypercall to query a domain's
> width would do.  Or a simple, stupid version number in the shared
> page.  We'll hardly end up with an unmanageable number of versions.

A simple hypercall will *not* do for paravirtualized drivers in fully
virtualized domains.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Gerd Hoffmann wrote:

> I'll go code up both front and back bits for block and pvfb to see how
> it works out in practice, I think I'll have patches later today or
> tomorrow ...

Here we go.  Compile-tested on 32bit, more tests coming, full rebuild
still in progress ...

Attached are:

protocol-bimodal.diff
  short header file with the protocol names.

{blk,fb}front-bimodal.diff
  small frontend driver patches, just add the protocol name to xenstore.

{blk,fb}back-bimodal.diff
  backend patches (for unstable only).

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: protocol-bimodal.diff --]
[-- Type: text/x-patch, Size: 987 bytes --]

---
 xen/include/public/io/protocols.h |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: build-32-unstable-13495/xen/include/public/io/protocols.h
===================================================================
--- /dev/null
+++ build-32-unstable-13495/xen/include/public/io/protocols.h
@@ -0,0 +1,21 @@
+#ifndef __XEN_PROTOCOLS_H__
+#define __XEN_PROTOCOLS_H__
+
+#define XEN_IO_PROTO_ABI_X86_32     "x86_32-abi"
+#define XEN_IO_PROTO_ABI_X86_64     "x86_64-abi"
+#define XEN_IO_PROTO_ABI_IA64       "ia64-abi"
+#define XEN_IO_PROTO_ABI_POWERPC64  "powerpc64-abi"
+
+#if defined(__i386__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_32
+#elif defined(__x86_64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_64
+#elif defined(__ia64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_IA64
+#elif defined(__powerpc64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_POWERPC64
+#else
+# error arch fixup needed here
+#endif
+
+#endif

[-- Attachment #3: fbfront-bimodal.diff --]
[-- Type: text/x-patch, Size: 1260 bytes --]

---
 linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
@@ -27,6 +27,7 @@
 #include <asm/hypervisor.h>
 #include <xen/evtchn.h>
 #include <xen/interface/io/fbif.h>
+#include <xen/interface/io/protocols.h>
 #include <xen/xenbus.h>
 #include <linux/kthread.h>
 
@@ -479,7 +480,7 @@ static int __devinit xenfb_probe(struct 
 		goto error_nomem;
 
 	/* set up shared page */
-	info->page = (void *)__get_free_page(GFP_KERNEL);
+	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
 
@@ -640,6 +641,10 @@ static int xenfb_connect_backend(struct 
 			    irq_to_evtchn_port(info->irq));
 	if (ret)
 		goto error_xenbus;
+	ret = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
+			    XEN_IO_PROTO_ABI_NATIVE);
+	if (ret)
+		goto error_xenbus;
 	ret = xenbus_printf(xbt, dev->nodename, "feature-update", "1");
 	if (ret)
 		goto error_xenbus;

[-- Attachment #4: blkfront-bimodal.diff --]
[-- Type: text/x-patch, Size: 976 bytes --]

---
 linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
@@ -44,6 +44,7 @@
 #include <xen/evtchn.h>
 #include <xen/xenbus.h>
 #include <xen/interface/grant_table.h>
+#include <xen/interface/io/protocols.h>
 #include <xen/gnttab.h>
 #include <asm/hypervisor.h>
 #include <asm/maddr.h>
@@ -180,6 +181,12 @@ again:
 		message = "writing event-channel";
 		goto abort_transaction;
 	}
+	err = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
+			    XEN_IO_PROTO_ABI_NATIVE);
+	if (err) {
+		message = "writing protocol";
+		goto abort_transaction;
+	}
 
 	err = xenbus_transaction_end(xbt, 0);
 	if (err) {

[-- Attachment #5: fbback-bimodal.diff --]
[-- Type: text/x-patch, Size: 4460 bytes --]

---
 tools/xenfb/xenfb.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 11 deletions(-)

Index: build-32-unstable-13495/tools/xenfb/xenfb.c
===================================================================
--- build-32-unstable-13495.orig/tools/xenfb/xenfb.c
+++ build-32-unstable-13495/tools/xenfb/xenfb.c
@@ -7,6 +7,7 @@
 #include <xen/io/xenbus.h>
 #include <xen/io/fbif.h>
 #include <xen/io/kbdif.h>
+#include <xen/io/protocols.h>
 #include <sys/select.h>
 #include <stdbool.h>
 #include <xen/linux/evtchn.h>
@@ -40,6 +41,7 @@ struct xenfb_private {
 	struct xs_handle *xsh;	/* xs daemon handle */
 	struct xenfb_device fb, kbd;
 	size_t fb_len;		/* size of framebuffer */
+	char protocol[64];	/* frontend protocol */
 };
 
 static void xenfb_detach_dom(struct xenfb_private *);
@@ -324,36 +326,112 @@ static int xenfb_wait_for_frontend_initi
 	return 0;
 }
 
+static void xenfb_copy_mfns(int mode, int count, unsigned long *dst, void *src)
+{
+	uint32_t *src32 = src;
+	uint64_t *src64 = src;
+	int i;
+
+	if (32 == mode) {
+		for (i = 0; i < count; i++)
+			dst[i] = src32[i];
+	} else {
+		for (i = 0; i < count; i++)
+			dst[i] = src64[i];
+	}
+}
+
 static int xenfb_map_fb(struct xenfb_private *xenfb, int domid)
 {
 	struct xenfb_page *page = xenfb->fb.page;
 	int n_fbmfns;
 	int n_fbdirs;
-	unsigned long *fbmfns;
+	unsigned long *pgmfns = NULL;
+	unsigned long *fbmfns = NULL;
+	void *map, *pd;
+	int mode, ret = -1;
+
+	/* default to native */
+	pd = page->pd;
+	mode = sizeof(unsigned long) * 8;
+
+	if (0 == strlen(xenfb->protocol)) {
+		/*
+		 * Undefined protocol, some guesswork needed.
+		 *
+		 * Old frontends which don't set the protocol use
+		 * one page directory only, thus pd[1] must be zero.
+		 * pd[1] of the 32bit struct layout and the lower
+		 * 32 bits of pd[0] of the 64bit struct layout have
+		 * the same location, so we can check that ...
+		 */
+		uint32_t *ptr32 = NULL;
+		uint32_t *ptr64 = NULL;
+#if defined(__i386_)
+		ptr32 = page->pd;
+		ptr64 = ((void*)page->pd) + 4;
+#elif defined(__x86_64__)
+		ptr32 = ((void*)page->pd) - 4;
+		ptr64 = page->pd;
+#endif
+		if (ptr32) {
+			if (0 == ptr32[1]) {
+				mode = 32;
+				pd   = ptr32;
+			} else {
+				mode = 64;
+				pd   = ptr64;
+			}
+		}
+#if defined(__x86_64__)
+	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_32)) {
+		/* 64bit dom0, 32bit domU */
+		mode = 32;
+		pd   = ((void*)page->pd) - 4;
+#elif defined(__i386_)
+	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_64)) {
+		/* 32bit dom0, 64bit domU */
+		mode = 64;
+		pd   = ((void*)page->pd) + 4;
+#endif
+	}
 
 	n_fbmfns = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
-	n_fbdirs = n_fbmfns * sizeof(unsigned long);
+	n_fbdirs = n_fbmfns * mode / 8;
 	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
 
+	pgmfns = malloc(sizeof(unsigned long) * n_fbdirs);
+	fbmfns = malloc(sizeof(unsigned long) * n_fbmfns);
+	if (!pgmfns || !fbmfns)
+		goto out;
+
 	/*
 	 * Bug alert: xc_map_foreign_batch() can fail partly and
 	 * return a non-null value.  This is a design flaw.  When it
 	 * happens, we happily continue here, and later crash on
 	 * access.
 	 */
-	fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
-			PROT_READ, page->pd, n_fbdirs);
-	if (fbmfns == NULL)
-		return -1;
+	xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
+	map = xc_map_foreign_batch(xenfb->xc, domid,
+				   PROT_READ, pgmfns, n_fbdirs);
+	if (map == NULL)
+		goto out;
+	xenfb_copy_mfns(mode, n_fbmfns, fbmfns, map);
+	munmap(map, n_fbdirs * XC_PAGE_SIZE);
 
 	xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
 				PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
-	if (xenfb->pub.pixels == NULL) {
-		munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
-		return -1;
-	}
+	if (xenfb->pub.pixels == NULL)
+		goto out;
 
-	return munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+	ret = 0; /* all is fine */
+
+ out:
+	if (pgmfns)
+		free(pgmfns);
+	if (fbmfns)
+		free(fbmfns);
+	return ret;
 }
 
 static int xenfb_bind(struct xenfb_device *dev)
@@ -368,6 +446,9 @@ static int xenfb_bind(struct xenfb_devic
 	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "event-channel", "%u",
 			    &evtchn) < 0)
 		return -1;
+	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "protocol", "%63s",
+			    xenfb->protocol) < 0)
+		xenfb->protocol[0] = '\0';
 
 	dev->port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
 					       dev->otherend_id, evtchn);

[-- Attachment #6: blkback-bimodal.diff --]
[-- Type: text/x-patch, Size: 23028 bytes --]

multiprotocol blkback drivers.

This is a patch for the block interface, frontend drivers, backend
drivers and tools to support multiple ring protocols.  Right there are
now just two: the 32bit and the 64bit one.  If needed it can be
extended.

Interface changes (io/blkif.h)
 * Have both request structs there, with "v1" and "v2" added to the
   name.  The old name is aliased to the native protocol of the
   architecture.
 * Add helper functions to convert v1/v2 requests to native.

Frontend changes:
 * Create a new node "protocol", add the protocol number it speaks
   there.

Backend changes:
 * Look at the "protocol" number of the frontend and switch ring
   handling accordingly.  If the protocol node isn't present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn't need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

Tools changes:
 * Add one more option to the disk configuration, so one can specify the
   protocol the frontend speaks in the config file.  This is needed for
   old frontends which don't advertise the protocol they are speaking
   themself.
   I'm not that happy with this approach, but it works for now and I'm
   kida lost in the stack of python classes doing domain and device
   handling ...

---
 linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c   |   64 ++++++++----
 linux-2.6-xen-sparse/drivers/xen/blkback/common.h    |    6 -
 linux-2.6-xen-sparse/drivers/xen/blkback/interface.c |   25 +++-
 linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c    |   20 +++
 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c     |   63 +++++++----
 linux-2.6-xen-sparse/drivers/xen/blktap/common.h     |    6 -
 linux-2.6-xen-sparse/drivers/xen/blktap/interface.c  |   25 +++-
 linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c     |   19 +++
 linux-2.6-xen-sparse/include/xen/blkif.h             |  100 +++++++++++++++++++
 xen/include/public/io/blkif.h                        |   14 +-
 10 files changed, 278 insertions(+), 64 deletions(-)

Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
@@ -298,17 +298,20 @@ irqreturn_t blkif_be_int(int irq, void *
 
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->co.req_cons;
+	rp = blk_rings->co.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
-	while ((rc != rp) && !RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+	while ((rc != rp)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->co, rc))
+			break;
 
 		pending_req = alloc_req();
 		if (NULL == pending_req) {
@@ -317,8 +320,17 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */
+		switch (blkif->blk_protocol) {
+		case 1:
+			blkif_get_v1_req(&req, RING_GET_REQUEST(&blk_rings->v1, rc));
+			break;
+		case 2:
+			blkif_get_v2_req(&req, RING_GET_REQUEST(&blk_rings->v2, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->co.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -498,34 +510,44 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
 			  unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
-	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case 1:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v1, blk_rings->v1.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case 2:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v2, blk_rings->v2.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->co.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->co, notify);
+	if (blk_rings->co.rsp_prod_pvt == blk_rings->co.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->co, more_to_do);
 
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->co)) {
 		more_to_do = 1;
-
 	}
+
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 
 	if (more_to_do)
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
@@ -40,8 +40,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 #include <xen/xenbus.h>
@@ -67,7 +66,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	int               blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* The VBD attached to this interface. */
 	struct vbd        vbd;
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
@@ -95,7 +95,6 @@ static void unmap_frontend_page(blkif_t 
 
 int blkif_map(blkif_t *blkif, unsigned long shared_page, unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -111,8 +110,24 @@ int blkif_map(blkif_t *blkif, unsigned l
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case 1:
+	{
+		blkif_v1_sring_t *sring_v1;
+		sring_v1 = (blkif_v1_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v1, sring_v1, PAGE_SIZE);
+		break;
+	}
+	case 2:
+	{
+		blkif_v2_sring_t *sring_v2;
+		sring_v2 = (blkif_v2_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v2, sring_v2, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, blkif_be_int, 0, "blkif-backend", blkif);
@@ -143,10 +158,10 @@ void blkif_disconnect(blkif_t *blkif)
 		blkif->irq = 0;
 	}
 
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.co.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.co.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
@@ -459,6 +459,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	char protocol[64] = "";
 	int err;
 
 	DPRINTK("%s", dev->otherend);
@@ -472,6 +473,25 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	be->blkif->blk_protocol = BLKIF_NATIVE_PROTOCOL;
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%63s", protocol, NULL);
+	if (err)
+		strcpy(protocol, "unspecified, assuming native");
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
+		be->blkif->blk_protocol = BLKIF_NATIVE_PROTOCOL;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
+		be->blkif->blk_protocol = 1;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
+		be->blkif->blk_protocol = 2;
+	else {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
+		return -1;
+	}
+
+	printk("blkback: ring-ref %ld, event-channel %d, protocol %d (%s)\n",
+	       ring_ref, evtchn, be->blkif->blk_protocol, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13534/xen/include/public/io/blkif.h
===================================================================
--- build-32-unstable-13534.orig/xen/include/public/io/blkif.h
+++ build-32-unstable-13534/xen/include/public/io/blkif.h
@@ -71,18 +71,20 @@
  */
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
+struct blkif_request_segment {
+    grant_ref_t gref;        /* reference to I/O buffer frame        */
+    /* @first_sect: first sector in frame to transfer (inclusive).   */
+    /* @last_sect: last sector in frame to transfer (inclusive).     */
+    uint8_t     first_sect, last_sect;
+};
+
 struct blkif_request {
     uint8_t        operation;    /* BLKIF_OP_???                         */
     uint8_t        nr_segments;  /* number of segments                   */
     blkif_vdev_t   handle;       /* only for read/write requests         */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
-    struct blkif_request_segment {
-        grant_ref_t gref;        /* reference to I/O buffer frame        */
-        /* @first_sect: first sector in frame to transfer (inclusive).   */
-        /* @last_sect: last sector in frame to transfer (inclusive).     */
-        uint8_t     first_sect, last_sect;
-    } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 };
 typedef struct blkif_request blkif_request_t;
 
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
@@ -1091,15 +1091,15 @@ irqreturn_t tap_blkif_be_int(int irq, vo
 static int print_dbug = 1;
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 	tap_blkif_t *info;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->co.req_cons;
+	rp = blk_rings->co.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
 	/*Check blkif has corresponding UE ring*/
@@ -1130,8 +1130,8 @@ static int do_block_io_op(blkif_t *blkif
 			more_to_do = 1;
 			break;
 		}
-		
-		if (RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->co, rc)) {
 			WPRINTK("RING_REQUEST_CONS_OVERFLOW!"
 			       " More to do\n");
 			more_to_do = 1;
@@ -1145,8 +1145,17 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */	
+		switch (blkif->blk_protocol) {
+		case 1:
+			blkif_get_v1_req(&req, RING_GET_REQUEST(&blk_rings->v1, rc));
+			break;
+		case 2:
+			blkif_get_v2_req(&req, RING_GET_REQUEST(&blk_rings->v2, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->co.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -1222,7 +1231,7 @@ static void dispatch_rw_block_io(blkif_t
 		WPRINTK("blktap: fe_ring is full, can't add "
 			"IO Request will be dropped. %d %d\n",
 			RING_SIZE(&info->ufe_ring),
-			RING_SIZE(&blkif->blk_ring));
+			RING_SIZE(&blkif->blk_rings.co));
 		goto fail_response;
 	}
 
@@ -1410,32 +1419,44 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
                           unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
+
 	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case 1:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v1, blk_rings->v1.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case 2:
+		memcpy(RING_GET_RESPONSE(&blk_rings->v2, blk_rings->v2.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->co.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->co, notify);
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	if (blk_rings->co.rsp_prod_pvt == blk_rings->co.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->co, more_to_do);
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->co)) {
 		more_to_do = 1;
+	}
 
-	}	
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 	if (more_to_do)
 		blkif_notify_work(blkif);
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
@@ -39,8 +39,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 
@@ -58,7 +57,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	int               blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* Back pointer to the backend_info. */
 	struct backend_info *be;
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
@@ -96,7 +96,6 @@ static void unmap_frontend_page(blkif_t 
 int tap_blkif_map(blkif_t *blkif, unsigned long shared_page, 
 		  unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -112,8 +111,24 @@ int tap_blkif_map(blkif_t *blkif, unsign
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case 1:
+	{
+		blkif_v1_sring_t *sring_v1;
+		sring_v1 = (blkif_v1_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v1, sring_v1, PAGE_SIZE);
+		break;
+	}
+	case 2:
+	{
+		blkif_v2_sring_t *sring_v2;
+		sring_v2 = (blkif_v2_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.v2, sring_v2, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, tap_blkif_be_int,
@@ -134,10 +149,10 @@ void tap_blkif_unmap(blkif_t *blkif)
 		unbind_from_irqhandler(blkif->irq, blkif);
 		blkif->irq = 0;
 	}
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.co.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.co.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
@@ -340,6 +340,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	char protocol[64];
 	int err;
 
 	DPRINTK("%s\n", dev->otherend);
@@ -353,6 +354,24 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	be->blkif->blk_protocol = BLKIF_NATIVE_PROTOCOL;
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%63s", protocol, NULL);
+	if (err)
+		strcpy(protocol, "unspecified, assuming native");
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
+		be->blkif->blk_protocol = BLKIF_NATIVE_PROTOCOL;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
+		be->blkif->blk_protocol = 1;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
+		be->blkif->blk_protocol = 2;
+	else {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
+		return -1;
+	}
+	printk("blktap: ring-ref %ld, event-channel %d, protocol %d (%s)\n",
+	       ring_ref, evtchn, be->blkif->blk_protocol, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = tap_blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13534/linux-2.6-xen-sparse/include/xen/blkif.h
===================================================================
--- /dev/null
+++ build-32-unstable-13534/linux-2.6-xen-sparse/include/xen/blkif.h
@@ -0,0 +1,100 @@
+#ifndef __XEN_BLKIF_H__
+#define __XEN_BLKIF_H__
+
+#include <xen/interface/io/ring.h>
+#include <xen/interface/io/blkif.h>
+#include <xen/interface/io/protocols.h>
+
+/* Not a real protocol.  Used to generate ring structs which contain
+ * the elements common to all protocols only.  This way we get a
+ * compiler-checkable way to use common struct elements, so we can
+ * avoid using switch(protocol) in a number of places.  */
+struct blkif_co_request {
+    char dummy;
+};
+struct blkif_co_response {
+    char dummy;
+};
+
+/* i386 protocol version */
+#pragma pack(push, 4)
+struct blkif_v1_request {
+    uint8_t        operation;    /* BLKIF_OP_???                         */
+    uint8_t        nr_segments;  /* number of segments                   */
+    blkif_vdev_t   handle;       /* only for read/write requests         */
+    uint64_t       id;           /* private guest value, echoed in resp  */
+    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_v1_response {
+    uint64_t        id;              /* copied from request */
+    uint8_t         operation;       /* copied from request */
+    int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_v1_request blkif_v1_request_t;
+typedef struct blkif_v1_response blkif_v1_response_t;
+#pragma pack(pop)
+
+/* x86_64 protocol version */
+struct blkif_v2_request {
+    uint8_t        operation;    /* BLKIF_OP_???                         */
+    uint8_t        nr_segments;  /* number of segments                   */
+    blkif_vdev_t   handle;       /* only for read/write requests         */
+    uint64_t       __attribute__((__aligned__(8))) id;
+    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_v2_response {
+    uint64_t       __attribute__((__aligned__(8))) id;
+    uint8_t         operation;       /* copied from request */
+    int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_v2_request blkif_v2_request_t;
+typedef struct blkif_v2_response blkif_v2_response_t;
+
+DEFINE_RING_TYPES(blkif_co, struct blkif_co_request, struct blkif_co_response);
+DEFINE_RING_TYPES(blkif_v1, struct blkif_v1_request, struct blkif_v1_response);
+DEFINE_RING_TYPES(blkif_v2, struct blkif_v2_request, struct blkif_v2_response);
+
+union blkif_back_rings {
+    blkif_co_back_ring_t co;
+    blkif_v1_back_ring_t v1;
+    blkif_v2_back_ring_t v2;
+};
+typedef union blkif_back_rings blkif_back_rings_t;
+
+#if defined(__i386__)
+# define BLKIF_NATIVE_PROTOCOL 1
+#elif defined(__x86_64__) || defined(__ia64__)
+# define BLKIF_NATIVE_PROTOCOL 2
+#else
+# error arch fixup needed here
+#endif
+
+/* translate requests: v1/v2 to native */
+#if 1 == BLKIF_NATIVE_PROTOCOL
+static void inline blkif_get_v1_req(blkif_request_t *dst, blkif_v1_request_t *src)
+#else
+static void inline blkif_get_v2_req(blkif_request_t *dst, blkif_v2_request_t *src)
+#endif
+{
+	memcpy(dst, src, sizeof(*dst));
+}
+
+#if 1 == BLKIF_NATIVE_PROTOCOL
+static void inline blkif_get_v2_req(blkif_request_t *dst, blkif_v2_request_t *src)
+#else
+static void inline blkif_get_v1_req(blkif_request_t *dst, blkif_v1_request_t *src)
+#endif
+{
+	int i;
+	dst->operation = src->operation;
+	dst->nr_segments = src->nr_segments;
+	dst->handle = src->handle;
+	dst->id = src->id;
+	dst->sector_number = src->sector_number;
+	for (i = 0; i < src->nr_segments; i++)
+		dst->seg[i] = src->seg[i];
+}
+
+#endif /* __XEN_BLKIF_H__ */

[-- Attachment #7: blktools-bimodal.diff --]
[-- Type: text/x-patch, Size: 2264 bytes --]

---
 tools/python/xen/xend/server/blkif.py |    3 +++
 tools/python/xen/xm/create.py         |    7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Index: build-32-unstable-13534/tools/python/xen/xend/server/blkif.py
===================================================================
--- build-32-unstable-13534.orig/tools/python/xen/xend/server/blkif.py
+++ build-32-unstable-13534/tools/python/xen/xend/server/blkif.py
@@ -38,6 +38,7 @@ class BlkifController(DevController):
         """@see DevController.getDeviceDetails"""
         uname = config.get('uname', '')
         dev = config.get('dev', '')
+        protocol = config.get('protocol')
         
         if 'ioemu:' in dev:
             (_, dev) = string.split(dev, ':', 1)
@@ -85,6 +86,8 @@ class BlkifController(DevController):
         front = { 'virtual-device' : "%i" % devid,
                   'device-type' : dev_type
                 }
+	if protocol:
+	    front.update({ 'protocol' : protocol });
 
         return (devid, back, front)
 
Index: build-32-unstable-13534/tools/python/xen/xm/create.py
===================================================================
--- build-32-unstable-13534.orig/tools/python/xen/xm/create.py
+++ build-32-unstable-13534/tools/python/xen/xm/create.py
@@ -537,7 +537,7 @@ def configure_image(vals):
 def configure_disks(config_devs, vals):
     """Create the config for disks (virtual block devices).
     """
-    for (uname, dev, mode, backend) in vals.disk:
+    for (uname, dev, mode, backend, protocol) in vals.disk:
         if uname.startswith('tap:'):
             cls = 'tap'
         else:
@@ -549,6 +549,8 @@ def configure_disks(config_devs, vals):
                       ['mode', mode ] ]
         if backend:
             config_vbd.append(['backend', backend])
+        if protocol:
+            config_vbd.append(['protocol', protocol])
         config_devs.append(['device', config_vbd])
 
 def configure_pci(config_devs, vals):
@@ -803,7 +805,10 @@ def preprocess_disk(vals):
         n = len(d)
         if n == 3:
             d.append(None)
+            d.append(None)
         elif n == 4:
+            d.append(None)
+        elif n == 5:
             pass
         else:
             err('Invalid disk specifier: ' + v)

[-- Attachment #8: Type: text/plain, Size: 138 bytes --]

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

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 22/1/07 14:01, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> Here we go.  Compile-tested on 32bit, more tests coming, full rebuild
> still in progress ...

Yeah, I like these. They can go in as soon as you're happy with them. One
exception is blkback -- I'm not keen on the v1/v2 thing as it is. I think we
should continue to include public/io/blkif.h and use the struct definition
there when protocol==XEN_IO_PROTO_ABI_NATIVE. The exceptions can be
XEN_IO_PROTO_ABI_X86_32 and XEN_IO_PROTO_ABI_X86_64 which can use the v1/v2
definitions (but I suggest renamed to include x86_32/x86_64 in the names).
We can generalise the names if they turn out to be useful in future for
other architectures (or provide typedef/#define equivalents). So for now we
end up with three useful enumerations for blk protocols: native, x86_32,
x86_64.

 -- Keir

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Gerd Hoffmann <kraxel@suse.de> writes:

> Gerd Hoffmann wrote:
>
>> I'll go code up both front and back bits for block and pvfb to see how
>> it works out in practice, I think I'll have patches later today or
>> tomorrow ...
>
> Here we go.  Compile-tested on 32bit, more tests coming, full rebuild
> still in progress ...
>
> Attached are:
>
> protocol-bimodal.diff
>   short header file with the protocol names.
>
> {blk,fb}front-bimodal.diff
>   small frontend driver patches, just add the protocol name to xenstore.
>
> {blk,fb}back-bimodal.diff
>   backend patches (for unstable only).
>
> cheers,
>   Gerd
>
> -- 
> Gerd Hoffmann <kraxel@suse.de>
> ---
>  xen/include/public/io/protocols.h |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> Index: build-32-unstable-13495/xen/include/public/io/protocols.h
> ===================================================================
> --- /dev/null
> +++ build-32-unstable-13495/xen/include/public/io/protocols.h
> @@ -0,0 +1,21 @@
> +#ifndef __XEN_PROTOCOLS_H__
> +#define __XEN_PROTOCOLS_H__
> +
> +#define XEN_IO_PROTO_ABI_X86_32     "x86_32-abi"
> +#define XEN_IO_PROTO_ABI_X86_64     "x86_64-abi"
> +#define XEN_IO_PROTO_ABI_IA64       "ia64-abi"
> +#define XEN_IO_PROTO_ABI_POWERPC64  "powerpc64-abi"
> +
> +#if defined(__i386__)
> +# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_32
> +#elif defined(__x86_64__)
> +# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_64
> +#elif defined(__ia64__)
> +# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_IA64
> +#elif defined(__powerpc64__)
> +# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_POWERPC64
> +#else
> +# error arch fixup needed here
> +#endif
> +
> +#endif
> ---
>  linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
> ===================================================================
> --- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
> +++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
> @@ -27,6 +27,7 @@
>  #include <asm/hypervisor.h>
>  #include <xen/evtchn.h>
>  #include <xen/interface/io/fbif.h>
> +#include <xen/interface/io/protocols.h>
>  #include <xen/xenbus.h>
>  #include <linux/kthread.h>
>  
> @@ -479,7 +480,7 @@ static int __devinit xenfb_probe(struct 
>  		goto error_nomem;
>  
>  	/* set up shared page */
> -	info->page = (void *)__get_free_page(GFP_KERNEL);
> +	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
>  	if (!info->page)
>  		goto error_nomem;
>  
> @@ -640,6 +641,10 @@ static int xenfb_connect_backend(struct 
>  			    irq_to_evtchn_port(info->irq));
>  	if (ret)
>  		goto error_xenbus;
> +	ret = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
> +			    XEN_IO_PROTO_ABI_NATIVE);
> +	if (ret)
> +		goto error_xenbus;

This identifies the ABI, but how does it provide versioning?

>  	ret = xenbus_printf(xbt, dev->nodename, "feature-update", "1");
>  	if (ret)
>  		goto error_xenbus;
> ---
>  linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
> ===================================================================
> --- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
> +++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
> @@ -44,6 +44,7 @@
>  #include <xen/evtchn.h>
>  #include <xen/xenbus.h>
>  #include <xen/interface/grant_table.h>
> +#include <xen/interface/io/protocols.h>
>  #include <xen/gnttab.h>
>  #include <asm/hypervisor.h>
>  #include <asm/maddr.h>
> @@ -180,6 +181,12 @@ again:
>  		message = "writing event-channel";
>  		goto abort_transaction;
>  	}
> +	err = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
> +			    XEN_IO_PROTO_ABI_NATIVE);
> +	if (err) {
> +		message = "writing protocol";
> +		goto abort_transaction;
> +	}
>  
>  	err = xenbus_transaction_end(xbt, 0);
>  	if (err) {
> ---
>  tools/xenfb/xenfb.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 92 insertions(+), 11 deletions(-)
>
> Index: build-32-unstable-13495/tools/xenfb/xenfb.c
> ===================================================================
> --- build-32-unstable-13495.orig/tools/xenfb/xenfb.c
> +++ build-32-unstable-13495/tools/xenfb/xenfb.c
> @@ -7,6 +7,7 @@
>  #include <xen/io/xenbus.h>
>  #include <xen/io/fbif.h>
>  #include <xen/io/kbdif.h>
> +#include <xen/io/protocols.h>
>  #include <sys/select.h>
>  #include <stdbool.h>
>  #include <xen/linux/evtchn.h>
> @@ -40,6 +41,7 @@ struct xenfb_private {
>  	struct xs_handle *xsh;	/* xs daemon handle */
>  	struct xenfb_device fb, kbd;
>  	size_t fb_len;		/* size of framebuffer */
> +	char protocol[64];	/* frontend protocol */
>  };
>  
>  static void xenfb_detach_dom(struct xenfb_private *);
> @@ -324,36 +326,112 @@ static int xenfb_wait_for_frontend_initi
>  	return 0;
>  }
>  
> +static void xenfb_copy_mfns(int mode, int count, unsigned long *dst, void *src)

Nitpick: the argument order dst, src, count got burned into my
synapses; other orders tend to confuse me.

> +{
> +	uint32_t *src32 = src;
> +	uint64_t *src64 = src;
> +	int i;
> +
> +	if (32 == mode) {
> +		for (i = 0; i < count; i++)
> +			dst[i] = src32[i];
> +	} else {
> +		for (i = 0; i < count; i++)
> +			dst[i] = src64[i];
> +	}
> +}
> +
>  static int xenfb_map_fb(struct xenfb_private *xenfb, int domid)
>  {
>  	struct xenfb_page *page = xenfb->fb.page;
>  	int n_fbmfns;
>  	int n_fbdirs;
> -	unsigned long *fbmfns;
> +	unsigned long *pgmfns = NULL;
> +	unsigned long *fbmfns = NULL;
> +	void *map, *pd;
> +	int mode, ret = -1;
> +
> +	/* default to native */
> +	pd = page->pd;
> +	mode = sizeof(unsigned long) * 8;

Nitick: encoding modes as 4, 8 and so forth rather than 32, 64, ...
would be slightly simpler.

> +
> +	if (0 == strlen(xenfb->protocol)) {
> +		/*
> +		 * Undefined protocol, some guesswork needed.

Guesswork is only required if we want to support old domU with
different width than dom0.  Do we?

> +		 *
> +		 * Old frontends which don't set the protocol use
> +		 * one page directory only, thus pd[1] must be zero.
> +		 * pd[1] of the 32bit struct layout and the lower
> +		 * 32 bits of pd[0] of the 64bit struct layout have
> +		 * the same location, so we can check that ...
> +		 */
> +		uint32_t *ptr32 = NULL;
> +		uint32_t *ptr64 = NULL;
> +#if defined(__i386_)
> +		ptr32 = page->pd;
> +		ptr64 = ((void*)page->pd) + 4;
> +#elif defined(__x86_64__)
> +		ptr32 = ((void*)page->pd) - 4;
> +		ptr64 = page->pd;
> +#endif
> +		if (ptr32) {
> +			if (0 == ptr32[1]) {
> +				mode = 32;
> +				pd   = ptr32;
> +			} else {
> +				mode = 64;
> +				pd   = ptr64;
> +			}

This clever hack tests those 32 pg bits that are guaranteed to be
initialized for both modes.  In other words, it uses all the
information there is.

In 32-bit mode, ptr32[1] is pd[1], and that is certainly zero for old
frontends.

In 64-bit mode, ptr32[1] is pd[0] & 0xffffffff.  Non-zero unless we
get very unlucky with the address of mfns.  Why can't that happen?

> +		}
> +#if defined(__x86_64__)
> +	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_32)) {
> +		/* 64bit dom0, 32bit domU */
> +		mode = 32;
> +		pd   = ((void*)page->pd) - 4;
> +#elif defined(__i386_)
> +	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_64)) {
> +		/* 32bit dom0, 64bit domU */
> +		mode = 64;
> +		pd   = ((void*)page->pd) + 4;
> +#endif

Hackery so that we can keep the old page layout.  Could perhaps be
done in a cleaner way, but I don't care.

> +	}
>  
>  	n_fbmfns = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
> -	n_fbdirs = n_fbmfns * sizeof(unsigned long);
> +	n_fbdirs = n_fbmfns * mode / 8;
>  	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>  
> +	pgmfns = malloc(sizeof(unsigned long) * n_fbdirs);
> +	fbmfns = malloc(sizeof(unsigned long) * n_fbmfns);
> +	if (!pgmfns || !fbmfns)
> +		goto out;
> +
>  	/*
>  	 * Bug alert: xc_map_foreign_batch() can fail partly and
>  	 * return a non-null value.  This is a design flaw.  When it
>  	 * happens, we happily continue here, and later crash on
>  	 * access.
>  	 */
> -	fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
> -			PROT_READ, page->pd, n_fbdirs);
> -	if (fbmfns == NULL)
> -		return -1;
> +	xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
> +	map = xc_map_foreign_batch(xenfb->xc, domid,
> +				   PROT_READ, pgmfns, n_fbdirs);
> +	if (map == NULL)
> +		goto out;
> +	xenfb_copy_mfns(mode, n_fbmfns, fbmfns, map);
> +	munmap(map, n_fbdirs * XC_PAGE_SIZE);
>  
>  	xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
>  				PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
> -	if (xenfb->pub.pixels == NULL) {
> -		munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
> -		return -1;
> -	}
> +	if (xenfb->pub.pixels == NULL)
> +		goto out;
>  
> -	return munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
> +	ret = 0; /* all is fine */
> +
> + out:
> +	if (pgmfns)
> +		free(pgmfns);
> +	if (fbmfns)
> +		free(fbmfns);
> +	return ret;
>  }
>  
>  static int xenfb_bind(struct xenfb_device *dev)
> @@ -368,6 +446,9 @@ static int xenfb_bind(struct xenfb_devic
>  	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "event-channel", "%u",
>  			    &evtchn) < 0)
>  		return -1;
> +	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "protocol", "%63s",
> +			    xenfb->protocol) < 0)
> +		xenfb->protocol[0] = '\0';
>  
>  	dev->port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
>  					       dev->otherend_id, evtchn);
[...]

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@suse.de> writes:
> 
>> +	ret = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
>> +			    XEN_IO_PROTO_ABI_NATIVE);
>> +	if (ret)
>> +		goto error_xenbus;
> 
> This identifies the ABI, but how does it provide versioning?

#define XEN_IO_PROTO_FB_GRANT_TABLES "fb-grant-tables" ?

Just create a descriptive name you like.  Assuming we don't create
abi-dependent structs *again* when we change the protocol.

We can of course also just use XEN_IO_PROTO_ABI_NATIVE "-v2".  With the
protocol identifier being a string there lots of ways ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 22/1/07 15:33, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> 
>> This identifies the ABI, but how does it provide versioning?
> 
> #define XEN_IO_PROTO_FB_GRANT_TABLES "fb-grant-tables" ?
> 
> Just create a descriptive name you like.  Assuming we don't create
> abi-dependent structs *again* when we change the protocol.
> 
> We can of course also just use XEN_IO_PROTO_ABI_NATIVE "-v2".  With the
> protocol identifier being a string there lots of ways ...

I'd suggest we add a suffix after the ABI part of the protocol name unless
we're really after full portability across any arbitrary architectures (and
so deal with e.g. endianness issues too). Whatever, clearly this approach
has the flexibility to be extended in future without adding all that much
code.

 -- Keir

Does vt-x itself have perf. impact on Hypervisor w/o considering HVM?

[Liang Yang]

Hello,

Suppose I have two different kinds of CPUs which have exactly the same 
configuration except one supports VT-X while the other does not. If I want 
to test the I/O performance (or other perf. testing which is not 
particularly related to I/O) of the both domain0 and Para-Virtualized Guest 
Domain (HVM domain is not considered), shall I expect to get the same 
performance results on these two CPUs?

Thanks,

Liang

RE: [Xen-users] Does vt-x itself have perf. impact on Hypervisor w/o considering HVM?

[Petersson, Mats]

> -----Original Message-----
> From: xen-users-bounces@lists.xensource.com 
> [mailto:xen-users-bounces@lists.xensource.com] On Behalf Of Liang Yang
> Sent: 22 January 2007 18:33
> To: Xen devel list; xen-users@lists.xensource.com
> Subject: [Xen-users] Does vt-x itself have perf. impact on 
> Hypervisor w/o considering HVM?
> 
> Hello,
> 
> Suppose I have two different kinds of CPUs which have exactly 
> the same 
> configuration except one supports VT-X while the other does 
> not. If I want 
> to test the I/O performance (or other perf. testing which is not 
> particularly related to I/O) of the both domain0 and 
> Para-Virtualized Guest 
> Domain (HVM domain is not considered), shall I expect to get the same 
> performance results on these two CPUs?

Assuming ALL other aspects are the same, when you're not using HVM,
there should be absolutely zero impact from it (aside from it using up a
few kilobytes of memory, to be precise, HVM (including both VMX and SVM)
takes up 129459 bytes when not used - more memory is allocated
dynamically when it's being used for obvious reasons. For modern
systems, that's so small that it doesn't matter). 

--
Mats
> 
> Thanks,
> 
> Liang
> 
> 
> _______________________________________________
> Xen-users mailing list
> Xen-users@lists.xensource.com
> http://lists.xensource.com/xen-users
> 
> 
> 

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Keir Fraser wrote:
> On 22/1/07 14:01, "Gerd Hoffmann" <kraxel@suse.de> wrote:
> 
>> Here we go.  Compile-tested on 32bit, more tests coming, full rebuild
>> still in progress ...
> 
> Yeah, I like these. They can go in as soon as you're happy with them. One
> exception is blkback -- I'm not keen on the v1/v2 thing as it is. I think we
> should continue to include public/io/blkif.h and use the struct definition
> there when protocol==XEN_IO_PROTO_ABI_NATIVE.

Fixed.  New versions attached, with comments added/updated and
signed-off-by.  They are partly tested only though, 32-on-64 seems to be
broken in current unstable, mixing 32bit and 64bit domains doesn't work.
 Time to get the domain builder bits merged too, so this can be added to
the regression tests ...

please apply,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: protocol-bimodal.diff --]
[-- Type: text/x-patch, Size: 1132 bytes --]

bimodal: header file with protocol names.

This patch adds a header file with the protocol names.

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
---
 xen/include/public/io/protocols.h |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: build-32-unstable-13495/xen/include/public/io/protocols.h
===================================================================
--- /dev/null
+++ build-32-unstable-13495/xen/include/public/io/protocols.h
@@ -0,0 +1,21 @@
+#ifndef __XEN_PROTOCOLS_H__
+#define __XEN_PROTOCOLS_H__
+
+#define XEN_IO_PROTO_ABI_X86_32     "x86_32-abi"
+#define XEN_IO_PROTO_ABI_X86_64     "x86_64-abi"
+#define XEN_IO_PROTO_ABI_IA64       "ia64-abi"
+#define XEN_IO_PROTO_ABI_POWERPC64  "powerpc64-abi"
+
+#if defined(__i386__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_32
+#elif defined(__x86_64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_X86_64
+#elif defined(__ia64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_IA64
+#elif defined(__powerpc64__)
+# define XEN_IO_PROTO_ABI_NATIVE XEN_IO_PROTO_ABI_POWERPC64
+#else
+# error arch fixup needed here
+#endif
+
+#endif

[-- Attachment #3: blkfront-bimodal.diff --]
[-- Type: text/x-patch, Size: 1123 bytes --]

bimodal: blkfront

Create a new node "protocol" in xenstore, add the protocol name it speaks there.

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
---
 linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
@@ -44,6 +44,7 @@
 #include <xen/evtchn.h>
 #include <xen/xenbus.h>
 #include <xen/interface/grant_table.h>
+#include <xen/interface/io/protocols.h>
 #include <xen/gnttab.h>
 #include <asm/hypervisor.h>
 #include <asm/maddr.h>
@@ -180,6 +181,12 @@ again:
 		message = "writing event-channel";
 		goto abort_transaction;
 	}
+	err = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
+			    XEN_IO_PROTO_ABI_NATIVE);
+	if (err) {
+		message = "writing protocol";
+		goto abort_transaction;
+	}
 
 	err = xenbus_transaction_end(xbt, 0);
 	if (err) {

[-- Attachment #4: fbfront-bimodal.diff --]
[-- Type: text/x-patch, Size: 1412 bytes --]

bimodal: pvfb frontend

Create a new node "protocol" in xenstore, add the protocol name it
speaks there.

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
---
 linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
===================================================================
--- build-32-unstable-13495.orig/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
+++ build-32-unstable-13495/linux-2.6-xen-sparse/drivers/xen/fbfront/xenfb.c
@@ -27,6 +27,7 @@
 #include <asm/hypervisor.h>
 #include <xen/evtchn.h>
 #include <xen/interface/io/fbif.h>
+#include <xen/interface/io/protocols.h>
 #include <xen/xenbus.h>
 #include <linux/kthread.h>
 
@@ -479,7 +480,7 @@ static int __devinit xenfb_probe(struct 
 		goto error_nomem;
 
 	/* set up shared page */
-	info->page = (void *)__get_free_page(GFP_KERNEL);
+	info->page = (void *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
 	if (!info->page)
 		goto error_nomem;
 
@@ -640,6 +641,10 @@ static int xenfb_connect_backend(struct 
 			    irq_to_evtchn_port(info->irq));
 	if (ret)
 		goto error_xenbus;
+	ret = xenbus_printf(xbt, dev->nodename, "protocol", "%s",
+			    XEN_IO_PROTO_ABI_NATIVE);
+	if (ret)
+		goto error_xenbus;
 	ret = xenbus_printf(xbt, dev->nodename, "feature-update", "1");
 	if (ret)
 		goto error_xenbus;

[-- Attachment #5: blkback-bimodal.diff --]
[-- Type: text/x-patch, Size: 23522 bytes --]

multiprotocol blkback drivers.

This is a patch for the block interface, frontend drivers, backend
drivers and tools to support multiple ring protocols.  Right there are
now just two: the 32bit and the 64bit one.  If needed it can be
extended.

Interface changes (io/blkif.h)
 * Have both request structs there, with "v1" and "v2" added to the
   name.  The old name is aliased to the native protocol of the
   architecture.
 * Add helper functions to convert v1/v2 requests to native.

Backend changes:
 * Look at the "protocol" name of the frontend and switch ring
   handling accordingly.  If the protocol node isn't present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn't need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

---
 linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c   |   71 ++++++++++----
 linux-2.6-xen-sparse/drivers/xen/blkback/common.h    |    6 -
 linux-2.6-xen-sparse/drivers/xen/blkback/interface.c |   25 ++++-
 linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c    |   19 +++
 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c     |   74 ++++++++++----
 linux-2.6-xen-sparse/drivers/xen/blktap/common.h     |    6 -
 linux-2.6-xen-sparse/drivers/xen/blktap/interface.c  |   25 ++++-
 linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c     |   19 +++
 linux-2.6-xen-sparse/include/xen/blkif.h             |   95 +++++++++++++++++++
 xen/include/public/io/blkif.h                        |   14 +-
 10 files changed, 290 insertions(+), 64 deletions(-)

Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
@@ -298,17 +298,20 @@ irqreturn_t blkif_be_int(int irq, void *
 
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->common.req_cons;
+	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
-	while ((rc != rp) && !RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+	while ((rc != rp)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))
+			break;
 
 		pending_req = alloc_req();
 		if (NULL == pending_req) {
@@ -317,8 +320,20 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */
+		switch (blkif->blk_protocol) {
+		case BLKIF_PROTOCOL_NATIVE:
+			memcpy(&req, RING_GET_REQUEST(&blk_rings->native, rc), sizeof(req));
+			break;
+		case BLKIF_PROTOCOL_X86_32:
+			blkif_get_x86_32_req(&req, RING_GET_REQUEST(&blk_rings->x86_32, rc));
+			break;
+		case BLKIF_PROTOCOL_X86_64:
+			blkif_get_x86_64_req(&req, RING_GET_REQUEST(&blk_rings->x86_64, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->common.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -498,34 +513,48 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
 			  unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
-	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case BLKIF_PROTOCOL_NATIVE:
+		memcpy(RING_GET_RESPONSE(&blk_rings->native, blk_rings->native.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_32:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_32, blk_rings->x86_32.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_64:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_64, blk_rings->x86_64.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->common.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify);
+	if (blk_rings->common.rsp_prod_pvt == blk_rings->common.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do);
 
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->common)) {
 		more_to_do = 1;
-
 	}
+
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 
 	if (more_to_do)
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
@@ -40,8 +40,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 #include <xen/xenbus.h>
@@ -67,7 +66,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	int               blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* The VBD attached to this interface. */
 	struct vbd        vbd;
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
@@ -95,7 +95,6 @@ static void unmap_frontend_page(blkif_t 
 
 int blkif_map(blkif_t *blkif, unsigned long shared_page, unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -111,8 +110,24 @@ int blkif_map(blkif_t *blkif, unsigned l
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case 1:
+	{
+		blkif_x86_32_sring_t *sring_x86_32;
+		sring_x86_32 = (blkif_x86_32_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_32, sring_x86_32, PAGE_SIZE);
+		break;
+	}
+	case 2:
+	{
+		blkif_x86_64_sring_t *sring_x86_64;
+		sring_x86_64 = (blkif_x86_64_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_64, sring_x86_64, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, blkif_be_int, 0, "blkif-backend", blkif);
@@ -143,10 +158,10 @@ void blkif_disconnect(blkif_t *blkif)
 		blkif->irq = 0;
 	}
 
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.common.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.common.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
@@ -459,6 +459,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	char protocol[64] = "";
 	int err;
 
 	DPRINTK("%s", dev->otherend);
@@ -472,6 +473,24 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%63s", protocol, NULL);
+	if (err)
+		strcpy(protocol, "unspecified, assuming native");
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
+	else {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
+		return -1;
+	}
+	printk("blkback: ring-ref %ld, event-channel %d, protocol %d (%s)\n",
+	       ring_ref, evtchn, be->blkif->blk_protocol, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13534/xen/include/public/io/blkif.h
===================================================================
--- build-32-unstable-13534.orig/xen/include/public/io/blkif.h
+++ build-32-unstable-13534/xen/include/public/io/blkif.h
@@ -71,18 +71,20 @@
  */
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
+struct blkif_request_segment {
+    grant_ref_t gref;        /* reference to I/O buffer frame        */
+    /* @first_sect: first sector in frame to transfer (inclusive).   */
+    /* @last_sect: last sector in frame to transfer (inclusive).     */
+    uint8_t     first_sect, last_sect;
+};
+
 struct blkif_request {
     uint8_t        operation;    /* BLKIF_OP_???                         */
     uint8_t        nr_segments;  /* number of segments                   */
     blkif_vdev_t   handle;       /* only for read/write requests         */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
-    struct blkif_request_segment {
-        grant_ref_t gref;        /* reference to I/O buffer frame        */
-        /* @first_sect: first sector in frame to transfer (inclusive).   */
-        /* @last_sect: last sector in frame to transfer (inclusive).     */
-        uint8_t     first_sect, last_sect;
-    } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 };
 typedef struct blkif_request blkif_request_t;
 
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
@@ -1091,15 +1091,15 @@ irqreturn_t tap_blkif_be_int(int irq, vo
 static int print_dbug = 1;
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 	tap_blkif_t *info;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->common.req_cons;
+	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
 	/*Check blkif has corresponding UE ring*/
@@ -1130,8 +1130,8 @@ static int do_block_io_op(blkif_t *blkif
 			more_to_do = 1;
 			break;
 		}
-		
-		if (RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc)) {
 			WPRINTK("RING_REQUEST_CONS_OVERFLOW!"
 			       " More to do\n");
 			more_to_do = 1;
@@ -1145,8 +1145,21 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */	
+		switch (blkif->blk_protocol) {
+		case BLKIF_PROTOCOL_NATIVE:
+			memcpy(&req, RING_GET_REQUEST(&blk_rings->native, rc),
+			       sizeof(req));
+			break;
+		case BLKIF_PROTOCOL_X86_32:
+			blkif_get_x86_32_req(&req, RING_GET_REQUEST(&blk_rings->x86_32, rc));
+			break;
+		case BLKIF_PROTOCOL_X86_64:
+			blkif_get_x86_64_req(&req, RING_GET_REQUEST(&blk_rings->x86_64, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->common.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -1222,7 +1235,7 @@ static void dispatch_rw_block_io(blkif_t
 		WPRINTK("blktap: fe_ring is full, can't add "
 			"IO Request will be dropped. %d %d\n",
 			RING_SIZE(&info->ufe_ring),
-			RING_SIZE(&blkif->blk_ring));
+			RING_SIZE(&blkif->blk_rings.common));
 		goto fail_response;
 	}
 
@@ -1410,32 +1423,51 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
                           unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
+
 	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case BLKIF_PROTOCOL_NATIVE:
+		memcpy(RING_GET_RESPONSE(&blk_rings->native,
+					 blk_rings->native.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_32:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_32,
+					 blk_rings->x86_32.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_64:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_64,
+					 blk_rings->x86_64.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->common.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify);
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	if (blk_rings->common.rsp_prod_pvt == blk_rings->common.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do);
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->common)) {
 		more_to_do = 1;
+	}
 
-	}	
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 	if (more_to_do)
 		blkif_notify_work(blkif);
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
@@ -39,8 +39,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 
@@ -58,7 +57,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	int               blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* Back pointer to the backend_info. */
 	struct backend_info *be;
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
@@ -96,7 +96,6 @@ static void unmap_frontend_page(blkif_t 
 int tap_blkif_map(blkif_t *blkif, unsigned long shared_page, 
 		  unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -112,8 +111,24 @@ int tap_blkif_map(blkif_t *blkif, unsign
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case 1:
+	{
+		blkif_x86_32_sring_t *sring_x86_32;
+		sring_x86_32 = (blkif_x86_32_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_32, sring_x86_32, PAGE_SIZE);
+		break;
+	}
+	case 2:
+	{
+		blkif_x86_64_sring_t *sring_x86_64;
+		sring_x86_64 = (blkif_x86_64_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_64, sring_x86_64, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, tap_blkif_be_int,
@@ -134,10 +149,10 @@ void tap_blkif_unmap(blkif_t *blkif)
 		unbind_from_irqhandler(blkif->irq, blkif);
 		blkif->irq = 0;
 	}
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.common.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.common.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
===================================================================
--- build-32-unstable-13534.orig/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
+++ build-32-unstable-13534/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
@@ -340,6 +340,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	char protocol[64];
 	int err;
 
 	DPRINTK("%s\n", dev->otherend);
@@ -353,6 +354,24 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%63s", protocol, NULL);
+	if (err)
+		strcpy(protocol, "unspecified, assuming native");
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
+	else {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
+		return -1;
+	}
+	printk("blktap: ring-ref %ld, event-channel %d, protocol %d (%s)\n",
+	       ring_ref, evtchn, be->blkif->blk_protocol, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = tap_blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13534/linux-2.6-xen-sparse/include/xen/blkif.h
===================================================================
--- /dev/null
+++ build-32-unstable-13534/linux-2.6-xen-sparse/include/xen/blkif.h
@@ -0,0 +1,95 @@
+#ifndef __XEN_BLKIF_H__
+#define __XEN_BLKIF_H__
+
+#include <xen/interface/io/ring.h>
+#include <xen/interface/io/blkif.h>
+#include <xen/interface/io/protocols.h>
+
+/* Not a real protocol.  Used to generate ring structs which contain
+ * the elements common to all protocols only.  This way we get a
+ * compiler-checkable way to use common struct elements, so we can
+ * avoid using switch(protocol) in a number of places.  */
+struct blkif_common_request {
+	char dummy;
+};
+struct blkif_common_response {
+	char dummy;
+};
+
+/* i386 protocol version */
+#pragma pack(push, 4)
+struct blkif_x86_32_request {
+	uint8_t        operation;    /* BLKIF_OP_???                         */
+	uint8_t        nr_segments;  /* number of segments                   */
+	blkif_vdev_t   handle;       /* only for read/write requests         */
+	uint64_t       id;           /* private guest value, echoed in resp  */
+	blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+	struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_x86_32_response {
+	uint64_t        id;              /* copied from request */
+	uint8_t         operation;       /* copied from request */
+	int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_x86_32_request blkif_x86_32_request_t;
+typedef struct blkif_x86_32_response blkif_x86_32_response_t;
+#pragma pack(pop)
+
+/* x86_64 protocol version */
+struct blkif_x86_64_request {
+	uint8_t        operation;    /* BLKIF_OP_???                         */
+	uint8_t        nr_segments;  /* number of segments                   */
+	blkif_vdev_t   handle;       /* only for read/write requests         */
+	uint64_t       __attribute__((__aligned__(8))) id;
+	blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+	struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_x86_64_response {
+	uint64_t       __attribute__((__aligned__(8))) id;
+	uint8_t         operation;       /* copied from request */
+	int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_x86_64_request blkif_x86_64_request_t;
+typedef struct blkif_x86_64_response blkif_x86_64_response_t;
+
+DEFINE_RING_TYPES(blkif_common, struct blkif_common_request, struct blkif_common_response);
+DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request, struct blkif_x86_32_response);
+DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request, struct blkif_x86_64_response);
+
+union blkif_back_rings {
+	blkif_back_ring_t        native;
+	blkif_common_back_ring_t common;
+	blkif_x86_32_back_ring_t x86_32;
+	blkif_x86_64_back_ring_t x86_64;
+};
+typedef union blkif_back_rings blkif_back_rings_t;
+
+#define BLKIF_PROTOCOL_NATIVE 1
+#define BLKIF_PROTOCOL_X86_32 2
+#define BLKIF_PROTOCOL_X86_64 3
+
+static void inline blkif_get_x86_32_req(blkif_request_t *dst, blkif_x86_32_request_t *src)
+{
+	int i;
+	dst->operation = src->operation;
+	dst->nr_segments = src->nr_segments;
+	dst->handle = src->handle;
+	dst->id = src->id;
+	dst->sector_number = src->sector_number;
+	for (i = 0; i < src->nr_segments; i++)
+		dst->seg[i] = src->seg[i];
+}
+
+static void inline blkif_get_x86_64_req(blkif_request_t *dst, blkif_x86_64_request_t *src)
+{
+	int i;
+	dst->operation = src->operation;
+	dst->nr_segments = src->nr_segments;
+	dst->handle = src->handle;
+	dst->id = src->id;
+	dst->sector_number = src->sector_number;
+	for (i = 0; i < src->nr_segments; i++)
+		dst->seg[i] = src->seg[i];
+}
+
+#endif /* __XEN_BLKIF_H__ */

[-- Attachment #6: fbback-bimodal.diff --]
[-- Type: text/x-patch, Size: 4592 bytes --]

bimodal: pvfb backend

Teach pvfb backend to deal with bith 32 and 64 bit frontends.

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
---
 tools/xenfb/xenfb.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 11 deletions(-)

Index: build-32-unstable-13495/tools/xenfb/xenfb.c
===================================================================
--- build-32-unstable-13495.orig/tools/xenfb/xenfb.c
+++ build-32-unstable-13495/tools/xenfb/xenfb.c
@@ -7,6 +7,7 @@
 #include <xen/io/xenbus.h>
 #include <xen/io/fbif.h>
 #include <xen/io/kbdif.h>
+#include <xen/io/protocols.h>
 #include <sys/select.h>
 #include <stdbool.h>
 #include <xen/linux/evtchn.h>
@@ -40,6 +41,7 @@ struct xenfb_private {
 	struct xs_handle *xsh;	/* xs daemon handle */
 	struct xenfb_device fb, kbd;
 	size_t fb_len;		/* size of framebuffer */
+	char protocol[64];	/* frontend protocol */
 };
 
 static void xenfb_detach_dom(struct xenfb_private *);
@@ -324,36 +326,112 @@ static int xenfb_wait_for_frontend_initi
 	return 0;
 }
 
+static void xenfb_copy_mfns(int mode, int count, unsigned long *dst, void *src)
+{
+	uint32_t *src32 = src;
+	uint64_t *src64 = src;
+	int i;
+
+	if (32 == mode) {
+		for (i = 0; i < count; i++)
+			dst[i] = src32[i];
+	} else {
+		for (i = 0; i < count; i++)
+			dst[i] = src64[i];
+	}
+}
+
 static int xenfb_map_fb(struct xenfb_private *xenfb, int domid)
 {
 	struct xenfb_page *page = xenfb->fb.page;
 	int n_fbmfns;
 	int n_fbdirs;
-	unsigned long *fbmfns;
+	unsigned long *pgmfns = NULL;
+	unsigned long *fbmfns = NULL;
+	void *map, *pd;
+	int mode, ret = -1;
+
+	/* default to native */
+	pd = page->pd;
+	mode = sizeof(unsigned long) * 8;
+
+	if (0 == strlen(xenfb->protocol)) {
+		/*
+		 * Undefined protocol, some guesswork needed.
+		 *
+		 * Old frontends which don't set the protocol use
+		 * one page directory only, thus pd[1] must be zero.
+		 * pd[1] of the 32bit struct layout and the lower
+		 * 32 bits of pd[0] of the 64bit struct layout have
+		 * the same location, so we can check that ...
+		 */
+		uint32_t *ptr32 = NULL;
+		uint32_t *ptr64 = NULL;
+#if defined(__i386_)
+		ptr32 = page->pd;
+		ptr64 = ((void*)page->pd) + 4;
+#elif defined(__x86_64__)
+		ptr32 = ((void*)page->pd) - 4;
+		ptr64 = page->pd;
+#endif
+		if (ptr32) {
+			if (0 == ptr32[1]) {
+				mode = 32;
+				pd   = ptr32;
+			} else {
+				mode = 64;
+				pd   = ptr64;
+			}
+		}
+#if defined(__x86_64__)
+	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_32)) {
+		/* 64bit dom0, 32bit domU */
+		mode = 32;
+		pd   = ((void*)page->pd) - 4;
+#elif defined(__i386_)
+	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_64)) {
+		/* 32bit dom0, 64bit domU */
+		mode = 64;
+		pd   = ((void*)page->pd) + 4;
+#endif
+	}
 
 	n_fbmfns = (xenfb->fb_len + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
-	n_fbdirs = n_fbmfns * sizeof(unsigned long);
+	n_fbdirs = n_fbmfns * mode / 8;
 	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
 
+	pgmfns = malloc(sizeof(unsigned long) * n_fbdirs);
+	fbmfns = malloc(sizeof(unsigned long) * n_fbmfns);
+	if (!pgmfns || !fbmfns)
+		goto out;
+
 	/*
 	 * Bug alert: xc_map_foreign_batch() can fail partly and
 	 * return a non-null value.  This is a design flaw.  When it
 	 * happens, we happily continue here, and later crash on
 	 * access.
 	 */
-	fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
-			PROT_READ, page->pd, n_fbdirs);
-	if (fbmfns == NULL)
-		return -1;
+	xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
+	map = xc_map_foreign_batch(xenfb->xc, domid,
+				   PROT_READ, pgmfns, n_fbdirs);
+	if (map == NULL)
+		goto out;
+	xenfb_copy_mfns(mode, n_fbmfns, fbmfns, map);
+	munmap(map, n_fbdirs * XC_PAGE_SIZE);
 
 	xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
 				PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
-	if (xenfb->pub.pixels == NULL) {
-		munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
-		return -1;
-	}
+	if (xenfb->pub.pixels == NULL)
+		goto out;
 
-	return munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+	ret = 0; /* all is fine */
+
+ out:
+	if (pgmfns)
+		free(pgmfns);
+	if (fbmfns)
+		free(fbmfns);
+	return ret;
 }
 
 static int xenfb_bind(struct xenfb_device *dev)
@@ -368,6 +446,9 @@ static int xenfb_bind(struct xenfb_devic
 	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "event-channel", "%u",
 			    &evtchn) < 0)
 		return -1;
+	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "protocol", "%63s",
+			    xenfb->protocol) < 0)
+		xenfb->protocol[0] = '\0';
 
 	dev->port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
 					       dev->otherend_id, evtchn);

[-- Attachment #7: blktools-bimodal.diff --]
[-- Type: text/x-patch, Size: 2694 bytes --]

bimodal: blk tools

Add one more option to the disk configuration, so one can specify the
protocol the frontend speaks in the config file.  This is needed for
old frontends which don't advertise the protocol they are speaking
themself.

I'm not that happy with this approach, but it works for now and I'm
kida lost in the stack of python classes doing domain and device
handling ...

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
---
 tools/python/xen/xend/server/blkif.py |    3 +++
 tools/python/xen/xm/create.py         |    7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Index: build-32-unstable-13534/tools/python/xen/xend/server/blkif.py
===================================================================
--- build-32-unstable-13534.orig/tools/python/xen/xend/server/blkif.py
+++ build-32-unstable-13534/tools/python/xen/xend/server/blkif.py
@@ -38,6 +38,7 @@ class BlkifController(DevController):
         """@see DevController.getDeviceDetails"""
         uname = config.get('uname', '')
         dev = config.get('dev', '')
+        protocol = config.get('protocol')
         
         if 'ioemu:' in dev:
             (_, dev) = string.split(dev, ':', 1)
@@ -85,6 +86,8 @@ class BlkifController(DevController):
         front = { 'virtual-device' : "%i" % devid,
                   'device-type' : dev_type
                 }
+	if protocol:
+	    front.update({ 'protocol' : protocol });
 
         return (devid, back, front)
 
Index: build-32-unstable-13534/tools/python/xen/xm/create.py
===================================================================
--- build-32-unstable-13534.orig/tools/python/xen/xm/create.py
+++ build-32-unstable-13534/tools/python/xen/xm/create.py
@@ -537,7 +537,7 @@ def configure_image(vals):
 def configure_disks(config_devs, vals):
     """Create the config for disks (virtual block devices).
     """
-    for (uname, dev, mode, backend) in vals.disk:
+    for (uname, dev, mode, backend, protocol) in vals.disk:
         if uname.startswith('tap:'):
             cls = 'tap'
         else:
@@ -549,6 +549,8 @@ def configure_disks(config_devs, vals):
                       ['mode', mode ] ]
         if backend:
             config_vbd.append(['backend', backend])
+        if protocol:
+            config_vbd.append(['protocol', protocol])
         config_devs.append(['device', config_vbd])
 
 def configure_pci(config_devs, vals):
@@ -803,7 +805,10 @@ def preprocess_disk(vals):
         n = len(d)
         if n == 3:
             d.append(None)
+            d.append(None)
         elif n == 4:
+            d.append(None)
+        elif n == 5:
             pass
         else:
             err('Invalid disk specifier: ' + v)

[-- Attachment #8: Type: text/plain, Size: 138 bytes --]

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

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 23/1/07 12:53, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> Fixed.  New versions attached, with comments added/updated and
> signed-off-by.  They are partly tested only though, 32-on-64 seems to be
> broken in current unstable, mixing 32bit and 64bit domains doesn't work.
>  Time to get the domain builder bits merged too, so this can be added to
> the regression tests ...

I've checked all in except blkback and blktools:

 * blktools: I'm not sure if this approach will be necessary. Brendan Cully
is adding support for pulling kernel image metadata into xend early during
domain creation. We can include target arch as part of that metadata and
then plumb that into the BlkifController to set the protocol automatically.
Unless there are other reasons to want to manually specify the protocol then
we are probably better to wait for the 'proper' solution.

 * blkback: You didn't sign off, and the changeset comment is stale (still
refers to v1/v2 protocols for example). Also in a couple of places (the
blkif_mmap functions in both blkback and blktap) you switch on blkif
protocol with cases 1 and 2, rather than using the enumeration names. That
looks a bit dodgy to me -- did you miss updating them?

 -- Keir

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

  Hi,

>  * blktools: I'm not sure if this approach will be necessary. Brendan Cully
> is adding support for pulling kernel image metadata into xend early during
> domain creation. We can include target arch as part of that metadata and
> then plumb that into the BlkifController to set the protocol automatically.
> Unless there are other reasons to want to manually specify the protocol then
> we are probably better to wait for the 'proper' solution.

I don't mind, as noted in the patch that is just the easiest way I've
found and most likely not the best one.  Just drop it.

>  * blkback: You didn't sign off, and the changeset comment is stale (still
> refers to v1/v2 protocols for example). Also in a couple of places (the
> blkif_mmap functions in both blkback and blktap) you switch on blkif
> protocol with cases 1 and 2, rather than using the enumeration names. That
> looks a bit dodgy to me -- did you miss updating them?

Yep, thanks for spotting these.  Fixed version attached.  Also changed
the blk_protocol to enum, so the compiler will warn on such bugs in the
future.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: blkback-bimodal.diff --]
[-- Type: text/x-patch, Size: 23981 bytes --]

multiprotocol blkback drivers.

This is a patch for the block backend drivers to support multiple ring
protocols.  This is needed for 32-on-64 support.  Right there are three
protocols: native, x86_32 and x86_64.  If needed it can be extended.

Interface changes (io/blkif.h)
 * Define the x86_32 and x86_64 structs additionally to the native
   version.
 * Add helper functions to convert them requests to native.

Backend changes:
 * Look at the "protocol" name of the frontend and switch ring
   handling accordingly.  If the protocol node isn't present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn't need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
---
 linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c   |   71 +++++++++----
 linux-2.6-xen-sparse/drivers/xen/blkback/common.h    |    6 -
 linux-2.6-xen-sparse/drivers/xen/blkback/interface.c |   32 +++++-
 linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c    |   19 +++
 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c     |   74 ++++++++++----
 linux-2.6-xen-sparse/drivers/xen/blktap/common.h     |    6 -
 linux-2.6-xen-sparse/drivers/xen/blktap/interface.c  |   32 +++++-
 linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c     |   19 +++
 linux-2.6-xen-sparse/include/xen/blkif.h             |   97 +++++++++++++++++++
 xen/include/public/io/blkif.h                        |   14 +-
 10 files changed, 306 insertions(+), 64 deletions(-)

Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/blkback.c
@@ -298,17 +298,20 @@ irqreturn_t blkif_be_int(int irq, void *
 
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->common.req_cons;
+	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
-	while ((rc != rp) && !RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+	while ((rc != rp)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))
+			break;
 
 		pending_req = alloc_req();
 		if (NULL == pending_req) {
@@ -317,8 +320,20 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */
+		switch (blkif->blk_protocol) {
+		case BLKIF_PROTOCOL_NATIVE:
+			memcpy(&req, RING_GET_REQUEST(&blk_rings->native, rc), sizeof(req));
+			break;
+		case BLKIF_PROTOCOL_X86_32:
+			blkif_get_x86_32_req(&req, RING_GET_REQUEST(&blk_rings->x86_32, rc));
+			break;
+		case BLKIF_PROTOCOL_X86_64:
+			blkif_get_x86_64_req(&req, RING_GET_REQUEST(&blk_rings->x86_64, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->common.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -498,34 +513,48 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
 			  unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
-	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case BLKIF_PROTOCOL_NATIVE:
+		memcpy(RING_GET_RESPONSE(&blk_rings->native, blk_rings->native.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_32:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_32, blk_rings->x86_32.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_64:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_64, blk_rings->x86_64.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->common.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify);
+	if (blk_rings->common.rsp_prod_pvt == blk_rings->common.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do);
 
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->common)) {
 		more_to_do = 1;
-
 	}
+
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 
 	if (more_to_do)
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/common.h
@@ -40,8 +40,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 #include <xen/xenbus.h>
@@ -67,7 +66,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	enum blkif_protocol blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* The VBD attached to this interface. */
 	struct vbd        vbd;
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/interface.c
@@ -95,7 +95,6 @@ static void unmap_frontend_page(blkif_t 
 
 int blkif_map(blkif_t *blkif, unsigned long shared_page, unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -111,8 +110,31 @@ int blkif_map(blkif_t *blkif, unsigned l
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case BLKIF_PROTOCOL_NATIVE:
+	{
+		blkif_sring_t *sring;
+		sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.native, sring, PAGE_SIZE);
+		break;
+	}
+	case BLKIF_PROTOCOL_X86_32:
+	{
+		blkif_x86_32_sring_t *sring_x86_32;
+		sring_x86_32 = (blkif_x86_32_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_32, sring_x86_32, PAGE_SIZE);
+		break;
+	}
+	case BLKIF_PROTOCOL_X86_64:
+	{
+		blkif_x86_64_sring_t *sring_x86_64;
+		sring_x86_64 = (blkif_x86_64_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_64, sring_x86_64, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, blkif_be_int, 0, "blkif-backend", blkif);
@@ -143,10 +165,10 @@ void blkif_disconnect(blkif_t *blkif)
 		blkif->irq = 0;
 	}
 
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.common.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.common.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
@@ -459,6 +459,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	char protocol[64] = "";
 	int err;
 
 	DPRINTK("%s", dev->otherend);
@@ -472,6 +473,24 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%63s", protocol, NULL);
+	if (err)
+		strcpy(protocol, "unspecified, assuming native");
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
+	else {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
+		return -1;
+	}
+	printk("blkback: ring-ref %ld, event-channel %d, protocol %d (%s)\n",
+	       ring_ref, evtchn, be->blkif->blk_protocol, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13553/xen/include/public/io/blkif.h
===================================================================
--- build-32-unstable-13553.orig/xen/include/public/io/blkif.h
+++ build-32-unstable-13553/xen/include/public/io/blkif.h
@@ -71,18 +71,20 @@
  */
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
+struct blkif_request_segment {
+    grant_ref_t gref;        /* reference to I/O buffer frame        */
+    /* @first_sect: first sector in frame to transfer (inclusive).   */
+    /* @last_sect: last sector in frame to transfer (inclusive).     */
+    uint8_t     first_sect, last_sect;
+};
+
 struct blkif_request {
     uint8_t        operation;    /* BLKIF_OP_???                         */
     uint8_t        nr_segments;  /* number of segments                   */
     blkif_vdev_t   handle;       /* only for read/write requests         */
     uint64_t       id;           /* private guest value, echoed in resp  */
     blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
-    struct blkif_request_segment {
-        grant_ref_t gref;        /* reference to I/O buffer frame        */
-        /* @first_sect: first sector in frame to transfer (inclusive).   */
-        /* @last_sect: last sector in frame to transfer (inclusive).     */
-        uint8_t     first_sect, last_sect;
-    } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 };
 typedef struct blkif_request blkif_request_t;
 
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
@@ -1091,15 +1091,15 @@ irqreturn_t tap_blkif_be_int(int irq, vo
 static int print_dbug = 1;
 static int do_block_io_op(blkif_t *blkif)
 {
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	blkif_request_t req;
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
 	int more_to_do = 0;
 	tap_blkif_t *info;
 
-	rc = blk_ring->req_cons;
-	rp = blk_ring->sring->req_prod;
+	rc = blk_rings->common.req_cons;
+	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
 	/*Check blkif has corresponding UE ring*/
@@ -1130,8 +1130,8 @@ static int do_block_io_op(blkif_t *blkif
 			more_to_do = 1;
 			break;
 		}
-		
-		if (RING_REQUEST_CONS_OVERFLOW(blk_ring, rc)) {
+
+		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc)) {
 			WPRINTK("RING_REQUEST_CONS_OVERFLOW!"
 			       " More to do\n");
 			more_to_do = 1;
@@ -1145,8 +1145,21 @@ static int do_block_io_op(blkif_t *blkif
 			break;
 		}
 
-		memcpy(&req, RING_GET_REQUEST(blk_ring, rc), sizeof(req));
-		blk_ring->req_cons = ++rc; /* before make_response() */	
+		switch (blkif->blk_protocol) {
+		case BLKIF_PROTOCOL_NATIVE:
+			memcpy(&req, RING_GET_REQUEST(&blk_rings->native, rc),
+			       sizeof(req));
+			break;
+		case BLKIF_PROTOCOL_X86_32:
+			blkif_get_x86_32_req(&req, RING_GET_REQUEST(&blk_rings->x86_32, rc));
+			break;
+		case BLKIF_PROTOCOL_X86_64:
+			blkif_get_x86_64_req(&req, RING_GET_REQUEST(&blk_rings->x86_64, rc));
+			break;
+		default:
+			BUG();
+		}
+		blk_rings->common.req_cons = ++rc; /* before make_response() */
 
 		switch (req.operation) {
 		case BLKIF_OP_READ:
@@ -1222,7 +1235,7 @@ static void dispatch_rw_block_io(blkif_t
 		WPRINTK("blktap: fe_ring is full, can't add "
 			"IO Request will be dropped. %d %d\n",
 			RING_SIZE(&info->ufe_ring),
-			RING_SIZE(&blkif->blk_ring));
+			RING_SIZE(&blkif->blk_rings.common));
 		goto fail_response;
 	}
 
@@ -1410,32 +1423,51 @@ static void dispatch_rw_block_io(blkif_t
 static void make_response(blkif_t *blkif, unsigned long id, 
                           unsigned short op, int st)
 {
-	blkif_response_t *resp;
+	blkif_response_t  resp;
 	unsigned long     flags;
-	blkif_back_ring_t *blk_ring = &blkif->blk_ring;
+	blkif_back_rings_t *blk_rings = &blkif->blk_rings;
 	int more_to_do = 0;
 	int notify;
 
+	resp.id        = id;
+	resp.operation = op;
+	resp.status    = st;
+
 	spin_lock_irqsave(&blkif->blk_ring_lock, flags);
-	/* Place on the response ring for the relevant domain. */ 
-	resp = RING_GET_RESPONSE(blk_ring, blk_ring->rsp_prod_pvt);
-	resp->id        = id;
-	resp->operation = op;
-	resp->status    = st;
-	blk_ring->rsp_prod_pvt++;
-	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(blk_ring, notify);
+	/* Place on the response ring for the relevant domain. */
+	switch (blkif->blk_protocol) {
+	case BLKIF_PROTOCOL_NATIVE:
+		memcpy(RING_GET_RESPONSE(&blk_rings->native,
+					 blk_rings->native.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_32:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_32,
+					 blk_rings->x86_32.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	case BLKIF_PROTOCOL_X86_64:
+		memcpy(RING_GET_RESPONSE(&blk_rings->x86_64,
+					 blk_rings->x86_64.rsp_prod_pvt),
+		       &resp, sizeof(resp));
+		break;
+	default:
+		BUG();
+	}
+	blk_rings->common.rsp_prod_pvt++;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify);
 
-	if (blk_ring->rsp_prod_pvt == blk_ring->req_cons) {
+	if (blk_rings->common.rsp_prod_pvt == blk_rings->common.req_cons) {
 		/*
 		 * Tail check for pending requests. Allows frontend to avoid
 		 * notifications if requests are already in flight (lower
 		 * overheads and promotes batching).
 		 */
-		RING_FINAL_CHECK_FOR_REQUESTS(blk_ring, more_to_do);
-	} else if (RING_HAS_UNCONSUMED_REQUESTS(blk_ring)) {
+		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do);
+	} else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->common)) {
 		more_to_do = 1;
+	}
 
-	}	
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
 	if (more_to_do)
 		blkif_notify_work(blkif);
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/common.h
@@ -39,8 +39,7 @@
 #include <asm/pgalloc.h>
 #include <xen/evtchn.h>
 #include <asm/hypervisor.h>
-#include <xen/interface/io/blkif.h>
-#include <xen/interface/io/ring.h>
+#include <xen/blkif.h>
 #include <xen/gnttab.h>
 #include <xen/driver_util.h>
 
@@ -58,7 +57,8 @@ typedef struct blkif_st {
 	/* Physical parameters of the comms window. */
 	unsigned int      irq;
 	/* Comms information. */
-	blkif_back_ring_t blk_ring;
+	enum blkif_protocol blk_protocol;
+	blkif_back_rings_t blk_rings;
 	struct vm_struct *blk_ring_area;
 	/* Back pointer to the backend_info. */
 	struct backend_info *be;
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/interface.c
@@ -96,7 +96,6 @@ static void unmap_frontend_page(blkif_t 
 int tap_blkif_map(blkif_t *blkif, unsigned long shared_page, 
 		  unsigned int evtchn)
 {
-	blkif_sring_t *sring;
 	int err;
 
 	/* Already connected through? */
@@ -112,8 +111,31 @@ int tap_blkif_map(blkif_t *blkif, unsign
 		return err;
 	}
 
-	sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
-	BACK_RING_INIT(&blkif->blk_ring, sring, PAGE_SIZE);
+	switch (blkif->blk_protocol) {
+	case BLKIF_PROTOCOL_NATIVE:
+	{
+		blkif_sring_t *sring;
+		sring = (blkif_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.native, sring, PAGE_SIZE);
+		break;
+	}
+	case BLKIF_PROTOCOL_X86_32:
+	{
+		blkif_x86_32_sring_t *sring_x86_32;
+		sring_x86_32 = (blkif_x86_32_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_32, sring_x86_32, PAGE_SIZE);
+		break;
+	}
+	case BLKIF_PROTOCOL_X86_64:
+	{
+		blkif_x86_64_sring_t *sring_x86_64;
+		sring_x86_64 = (blkif_x86_64_sring_t *)blkif->blk_ring_area->addr;
+		BACK_RING_INIT(&blkif->blk_rings.x86_64, sring_x86_64, PAGE_SIZE);
+		break;
+	}
+	default:
+		BUG();
+	}
 
 	err = bind_interdomain_evtchn_to_irqhandler(
 		blkif->domid, evtchn, tap_blkif_be_int,
@@ -134,10 +156,10 @@ void tap_blkif_unmap(blkif_t *blkif)
 		unbind_from_irqhandler(blkif->irq, blkif);
 		blkif->irq = 0;
 	}
-	if (blkif->blk_ring.sring) {
+	if (blkif->blk_rings.common.sring) {
 		unmap_frontend_page(blkif);
 		free_vm_area(blkif->blk_ring_area);
-		blkif->blk_ring.sring = NULL;
+		blkif->blk_rings.common.sring = NULL;
 	}
 }
 
Index: build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
===================================================================
--- build-32-unstable-13553.orig/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
+++ build-32-unstable-13553/linux-2.6-xen-sparse/drivers/xen/blktap/xenbus.c
@@ -340,6 +340,7 @@ static int connect_ring(struct backend_i
 	struct xenbus_device *dev = be->dev;
 	unsigned long ring_ref;
 	unsigned int evtchn;
+	char protocol[64];
 	int err;
 
 	DPRINTK("%s\n", dev->otherend);
@@ -353,6 +354,24 @@ static int connect_ring(struct backend_i
 		return err;
 	}
 
+	be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
+			    "%63s", protocol, NULL);
+	if (err)
+		strcpy(protocol, "unspecified, assuming native");
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
+	else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
+		be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
+	else {
+		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
+		return -1;
+	}
+	printk("blktap: ring-ref %ld, event-channel %d, protocol %d (%s)\n",
+	       ring_ref, evtchn, be->blkif->blk_protocol, protocol);
+
 	/* Map the shared frame, irq etc. */
 	err = tap_blkif_map(be->blkif, ring_ref, evtchn);
 	if (err) {
Index: build-32-unstable-13553/linux-2.6-xen-sparse/include/xen/blkif.h
===================================================================
--- /dev/null
+++ build-32-unstable-13553/linux-2.6-xen-sparse/include/xen/blkif.h
@@ -0,0 +1,97 @@
+#ifndef __XEN_BLKIF_H__
+#define __XEN_BLKIF_H__
+
+#include <xen/interface/io/ring.h>
+#include <xen/interface/io/blkif.h>
+#include <xen/interface/io/protocols.h>
+
+/* Not a real protocol.  Used to generate ring structs which contain
+ * the elements common to all protocols only.  This way we get a
+ * compiler-checkable way to use common struct elements, so we can
+ * avoid using switch(protocol) in a number of places.  */
+struct blkif_common_request {
+	char dummy;
+};
+struct blkif_common_response {
+	char dummy;
+};
+
+/* i386 protocol version */
+#pragma pack(push, 4)
+struct blkif_x86_32_request {
+	uint8_t        operation;    /* BLKIF_OP_???                         */
+	uint8_t        nr_segments;  /* number of segments                   */
+	blkif_vdev_t   handle;       /* only for read/write requests         */
+	uint64_t       id;           /* private guest value, echoed in resp  */
+	blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+	struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_x86_32_response {
+	uint64_t        id;              /* copied from request */
+	uint8_t         operation;       /* copied from request */
+	int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_x86_32_request blkif_x86_32_request_t;
+typedef struct blkif_x86_32_response blkif_x86_32_response_t;
+#pragma pack(pop)
+
+/* x86_64 protocol version */
+struct blkif_x86_64_request {
+	uint8_t        operation;    /* BLKIF_OP_???                         */
+	uint8_t        nr_segments;  /* number of segments                   */
+	blkif_vdev_t   handle;       /* only for read/write requests         */
+	uint64_t       __attribute__((__aligned__(8))) id;
+	blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+	struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+struct blkif_x86_64_response {
+	uint64_t       __attribute__((__aligned__(8))) id;
+	uint8_t         operation;       /* copied from request */
+	int16_t         status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_x86_64_request blkif_x86_64_request_t;
+typedef struct blkif_x86_64_response blkif_x86_64_response_t;
+
+DEFINE_RING_TYPES(blkif_common, struct blkif_common_request, struct blkif_common_response);
+DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request, struct blkif_x86_32_response);
+DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request, struct blkif_x86_64_response);
+
+union blkif_back_rings {
+	blkif_back_ring_t        native;
+	blkif_common_back_ring_t common;
+	blkif_x86_32_back_ring_t x86_32;
+	blkif_x86_64_back_ring_t x86_64;
+};
+typedef union blkif_back_rings blkif_back_rings_t;
+
+enum blkif_protocol {
+	BLKIF_PROTOCOL_NATIVE = 1,
+	BLKIF_PROTOCOL_X86_32 = 2,
+	BLKIF_PROTOCOL_X86_64 = 3,
+};
+
+static void inline blkif_get_x86_32_req(blkif_request_t *dst, blkif_x86_32_request_t *src)
+{
+	int i;
+	dst->operation = src->operation;
+	dst->nr_segments = src->nr_segments;
+	dst->handle = src->handle;
+	dst->id = src->id;
+	dst->sector_number = src->sector_number;
+	for (i = 0; i < src->nr_segments; i++)
+		dst->seg[i] = src->seg[i];
+}
+
+static void inline blkif_get_x86_64_req(blkif_request_t *dst, blkif_x86_64_request_t *src)
+{
+	int i;
+	dst->operation = src->operation;
+	dst->nr_segments = src->nr_segments;
+	dst->handle = src->handle;
+	dst->id = src->id;
+	dst->sector_number = src->sector_number;
+	for (i = 0; i < src->nr_segments; i++)
+		dst->seg[i] = src->seg[i];
+}
+
+#endif /* __XEN_BLKIF_H__ */

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

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

Re: [Xen-users] Does vt-x itself have perf. impact on Hypervisor w/o considering HVM?

[Liang Yang]

Without VT-x support, binary translation has to be used to make those 
non-virtualizable instructions throw exception. With VT-x support, no binary 
translation is needed. So you mean, binary translation could be implemented 
as efficient as they are done in hardware?

Thanks,

Liang

----- Original Message ----- 
From: "Petersson, Mats" <Mats.Petersson@amd.com>
To: "Liang Yang" <multisyncfe991@hotmail.com>; "Xen devel list" 
<xen-devel@lists.xensource.com>; <xen-users@lists.xensource.com>
Sent: Tuesday, January 23, 2007 3:05 AM
Subject: RE: [Xen-users] Does vt-x itself have perf. impact on Hypervisor 
w/o considering HVM?


> -----Original Message-----
> From: xen-users-bounces@lists.xensource.com
> [mailto:xen-users-bounces@lists.xensource.com] On Behalf Of Liang Yang
> Sent: 22 January 2007 18:33
> To: Xen devel list; xen-users@lists.xensource.com
> Subject: [Xen-users] Does vt-x itself have perf. impact on
> Hypervisor w/o considering HVM?
>
> Hello,
>
> Suppose I have two different kinds of CPUs which have exactly
> the same
> configuration except one supports VT-X while the other does
> not. If I want
> to test the I/O performance (or other perf. testing which is not
> particularly related to I/O) of the both domain0 and
> Para-Virtualized Guest
> Domain (HVM domain is not considered), shall I expect to get the same
> performance results on these two CPUs?

Assuming ALL other aspects are the same, when you're not using HVM,
there should be absolutely zero impact from it (aside from it using up a
few kilobytes of memory, to be precise, HVM (including both VMX and SVM)
takes up 129459 bytes when not used - more memory is allocated
dynamically when it's being used for obvious reasons. For modern
systems, that's so small that it doesn't matter).

--
Mats
>
> Thanks,
>
> Liang
>
>
> _______________________________________________
> Xen-users mailing list
> Xen-users@lists.xensource.com
> http://lists.xensource.com/xen-users
>
>
>

RE: [Xen-users] Does vt-x itself have perf. impact on Hypervisor w/o considering HVM?

[Petersson, Mats]

 

> -----Original Message-----
> From: Liang Yang [mailto:multisyncfe991@hotmail.com] 
> Sent: 23 January 2007 16:15
> To: Petersson, Mats; Xen devel list; xen-users@lists.xensource.com
> Subject: Re: [Xen-users] Does vt-x itself have perf. impact 
> on Hypervisor w/o considering HVM?
> 
> Without VT-x support, binary translation has to be used to make those 
> non-virtualizable instructions throw exception. With VT-x 
> support, no binary 
> translation is needed. So you mean, binary translation could 
> be implemented 
> as efficient as they are done in hardware?

Not at all - I'm saying that if you're not using VT-x, then VT-x has no
performance impact to your tests, which is what you originally asked. Or
at least, that's how I interpreted:
"both domain0 and Para-Virtualized Guest Domain (HVM domain is not
considered)" in your original post. 

Xen doesn't use binary translation, it uses para-virtualization, which
is another way to say "source-code of the operating systme being
modified to support virtualization". 

Binary translation or hardware support is needed if you're using an
operating system where it is unpractical to do para-virtualization (for
example, source-code is not publicly available (say Windows), or the
user-base isn't big enough to sustain a para-virtualization effort (say
Linux kernel 2.4.x)).

Whether binary translation or hardware is more efficient will depend on:
1. Implementation of the binary translation and hardware. 
2. What application and OS is being run on the system. 

It is by no means sure that Binary translation is either faster or
slower than hardware implementations of virtualization. There are too
many other factors that affect the situation to say for sure - one case
may be faster for binary translation (because the type of operations
suit binary translation), whilst another is slower.

For most common cases, Para-virtualization should be able to be overall
faster than both, because there are possibilities to bunch together
several operations that cause a hypervisor interaction, and those can be
"pacakged together" rahter than each event needing it's own separate
hypervisor interaction. Of course, REALLLY clever binary translation may
be able to detect a loop writing to the page-table and translate that
into a block-call of "translate these 100 page-writes", rather than
"for(...) do { translate one page write }" as the obvious solution would
be. In hardware, there is no choice but to intercept each and every
operation on it's own, so it's most likely the slowest operation, unless
there's some way to avoid the hypervisor being called overall (there are
some such cases too, and again, it depends on the exact circumstances of
the application + OS that is run as a guest). 

There's also more possibilities to add things to the hardware to support
future enhancements of the hardware, which just plain isn't going to
happen in the software solution. One such feature is nested paging,
which allows the guest-page-table operations to avoid being intercepted
(instead of an intercept and a whole bunch of code to perform the
page-table write, the processor does a second page-table access of the
"host-page-table" which translates the guest physical address into
machine physical address). 

--
Mats
> 
> Thanks,
> 
> Liang
> 
> ----- Original Message ----- 
> From: "Petersson, Mats" <Mats.Petersson@amd.com>
> To: "Liang Yang" <multisyncfe991@hotmail.com>; "Xen devel list" 
> <xen-devel@lists.xensource.com>; <xen-users@lists.xensource.com>
> Sent: Tuesday, January 23, 2007 3:05 AM
> Subject: RE: [Xen-users] Does vt-x itself have perf. impact 
> on Hypervisor 
> w/o considering HVM?
> 
> 
> > -----Original Message-----
> > From: xen-users-bounces@lists.xensource.com
> > [mailto:xen-users-bounces@lists.xensource.com] On Behalf Of 
> Liang Yang
> > Sent: 22 January 2007 18:33
> > To: Xen devel list; xen-users@lists.xensource.com
> > Subject: [Xen-users] Does vt-x itself have perf. impact on
> > Hypervisor w/o considering HVM?
> >
> > Hello,
> >
> > Suppose I have two different kinds of CPUs which have exactly
> > the same
> > configuration except one supports VT-X while the other does
> > not. If I want
> > to test the I/O performance (or other perf. testing which is not
> > particularly related to I/O) of the both domain0 and
> > Para-Virtualized Guest
> > Domain (HVM domain is not considered), shall I expect to 
> get the same
> > performance results on these two CPUs?
> 
> Assuming ALL other aspects are the same, when you're not using HVM,
> there should be absolutely zero impact from it (aside from it 
> using up a
> few kilobytes of memory, to be precise, HVM (including both 
> VMX and SVM)
> takes up 129459 bytes when not used - more memory is allocated
> dynamically when it's being used for obvious reasons. For modern
> systems, that's so small that it doesn't matter).
> 
> --
> Mats
> >
> > Thanks,
> >
> > Liang
> >
> >
> > _______________________________________________
> > Xen-users mailing list
> > Xen-users@lists.xensource.com
> > http://lists.xensource.com/xen-users
> >
> >
> >
> 
> 
> 
> 
> 
> 

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

Gerd Hoffmann wrote:
> Keir Fraser wrote:
>> On 22/1/07 14:01, "Gerd Hoffmann" <kraxel@suse.de> wrote:
>>
>>> Here we go.  Compile-tested on 32bit, more tests coming, full rebuild
>>> still in progress ...
>> Yeah, I like these. They can go in as soon as you're happy with them. One
>> exception is blkback -- I'm not keen on the v1/v2 thing as it is. I think we
>> should continue to include public/io/blkif.h and use the struct definition
>> there when protocol==XEN_IO_PROTO_ABI_NATIVE.
> 
> Fixed.  New versions attached, with comments added/updated and
> signed-off-by.  They are partly tested only though, 32-on-64 seems to be
> broken in current unstable, mixing 32bit and 64bit domains doesn't work.

What is the status of this?  It's not in the public tree yet.  Any
problems?  Or just stuck in the patch queue or regression testing?
Meanwhile I've tested the block backend patches with 3.0.4 in a mixed
environment -- works fine.

The pvfb backend has a stupid tyops (missing underscore) which breaks
the combination 32bit dom0 and 64bit domU.  Fix attached.  The patch
also adds a cast to fix a warning.

please apply,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: fbback-bimodal-fix.diff --]
[-- Type: text/x-patch, Size: 1322 bytes --]

Index: build-32-release304-13133/tools/xenfb/xenfb.c
===================================================================
--- build-32-release304-13133.orig/tools/xenfb/xenfb.c
+++ build-32-release304-13133/tools/xenfb/xenfb.c
@@ -367,12 +367,12 @@ static int xenfb_map_fb(struct xenfb_pri
 		 */
 		uint32_t *ptr32 = NULL;
 		uint32_t *ptr64 = NULL;
-#if defined(__i386_)
-		ptr32 = page->pd;
+#if defined(__i386__)
+		ptr32 = (void*)page->pd;
 		ptr64 = ((void*)page->pd) + 4;
 #elif defined(__x86_64__)
 		ptr32 = ((void*)page->pd) - 4;
-		ptr64 = page->pd;
+		ptr64 = (void*)page->pd;
 #endif
 		if (ptr32) {
 			if (0 == ptr32[1]) {
@@ -388,7 +388,7 @@ static int xenfb_map_fb(struct xenfb_pri
 		/* 64bit dom0, 32bit domU */
 		mode = 32;
 		pd   = ((void*)page->pd) - 4;
-#elif defined(__i386_)
+#elif defined(__i386__)
 	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_64)) {
 		/* 32bit dom0, 64bit domU */
 		mode = 64;
@@ -560,10 +560,10 @@ int xenfb_attach_dom(struct xenfb *xenfb
 	if (xenfb_wait_for_frontend_initialised(&xenfb->kbd) < 0)
 		goto error;
 
-	if (xenfb_bind(&xenfb->fb) < 0)
-		goto error;
 	if (xenfb_bind(&xenfb->kbd) < 0)
 		goto error;
+	if (xenfb_bind(&xenfb->fb) < 0)
+		goto error;
 
 	if (xenfb_xs_scanf1(xsh, xenfb->fb.otherend, "feature-update",
 			    "%d", &val) < 0)

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

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

Re: 32-on-64: pvfb issue

[Keir Fraser]

On 24/1/07 11:23, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> What is the status of this?  It's not in the public tree yet.  Any
> problems?  Or just stuck in the patch queue or regression testing?
> Meanwhile I've tested the block backend patches with 3.0.4 in a mixed
> environment -- works fine.

All the patches are in the staging tree so will be in the public tree after
regression tests have run.

 -- Keir

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Gerd Hoffmann <kraxel@suse.de> writes:

[...]
> The pvfb backend has a stupid tyops (missing underscore) which breaks
> the combination 32bit dom0 and 64bit domU.  Fix attached.  The patch
> also adds a cast to fix a warning.
>
> please apply,
>
>   Gerd
>
> -- 
> Gerd Hoffmann <kraxel@suse.de>
> Index: build-32-release304-13133/tools/xenfb/xenfb.c
> ===================================================================
> --- build-32-release304-13133.orig/tools/xenfb/xenfb.c
> +++ build-32-release304-13133/tools/xenfb/xenfb.c
[...]
> @@ -560,10 +560,10 @@ int xenfb_attach_dom(struct xenfb *xenfb
>  	if (xenfb_wait_for_frontend_initialised(&xenfb->kbd) < 0)
>  		goto error;
>  
> -	if (xenfb_bind(&xenfb->fb) < 0)
> -		goto error;
>  	if (xenfb_bind(&xenfb->kbd) < 0)
>  		goto error;
> +	if (xenfb_bind(&xenfb->fb) < 0)
> +		goto error;
>  
>  	if (xenfb_xs_scanf1(xsh, xenfb->fb.otherend, "feature-update",
>  			    "%d", &val) < 0)

Why is this patch hunk necessary?

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

  Hi,

>> @@ -560,10 +560,10 @@ int xenfb_attach_dom(struct xenfb *xenfb
>>  	if (xenfb_wait_for_frontend_initialised(&xenfb->kbd) < 0)
>>  		goto error;
>>  
>> -	if (xenfb_bind(&xenfb->fb) < 0)
>> -		goto error;
>>  	if (xenfb_bind(&xenfb->kbd) < 0)
>>  		goto error;
>> +	if (xenfb_bind(&xenfb->fb) < 0)
>> +		goto error;
>>  
>>  	if (xenfb_xs_scanf1(xsh, xenfb->fb.otherend, "feature-update",
>>  			    "%d", &val) < 0)
> 
> Why is this patch hunk necessary?

Oh, forgot to mention that, sorry.  Only vfb has a "protocol" node, vkbd
hasn't.  So binding fb last makes sure we have the protocol filled
correctly in the struct.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64: pvfb issue

[Markus Armbruster]

Gerd Hoffmann <kraxel@suse.de> writes:

>   Hi,
>
>>> @@ -560,10 +560,10 @@ int xenfb_attach_dom(struct xenfb *xenfb
>>>  	if (xenfb_wait_for_frontend_initialised(&xenfb->kbd) < 0)
>>>  		goto error;
>>>  
>>> -	if (xenfb_bind(&xenfb->fb) < 0)
>>> -		goto error;
>>>  	if (xenfb_bind(&xenfb->kbd) < 0)
>>>  		goto error;
>>> +	if (xenfb_bind(&xenfb->fb) < 0)
>>> +		goto error;
>>>  
>>>  	if (xenfb_xs_scanf1(xsh, xenfb->fb.otherend, "feature-update",
>>>  			    "%d", &val) < 0)
>> 
>> Why is this patch hunk necessary?
>
> Oh, forgot to mention that, sorry.  Only vfb has a "protocol" node, vkbd
> hasn't.  So binding fb last makes sure we have the protocol filled
> correctly in the struct.
>
> cheers,
>   Gerd

Unclean!

You put protocol[] into xenfb_private, which means it's shared between
vfb and vkbd.  That's defensible.  However, you really shouldn't read
it in xenfb_bind() then.  That reads it both from vfb (where it is
valid) and vkbd (where it is currently undefined), and the one read
last wins.

Reading it next to reading feature-update would be much cleaner.

Alternatively, put protocol[] into xenfb_device.

If you insist on keeping it the way it is, you really need a comment.
But cleaning it up should be less work than explaining it.

Re: 32-on-64: pvfb issue

[Gerd Hoffmann]

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

> Reading it next to reading feature-update would be much cleaner.

Done, updated patch attached.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: fbback-bimodal-fix.diff --]
[-- Type: text/x-patch, Size: 1766 bytes --]

---
 tools/xenfb/xenfb.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: build-32-unstable-13568/tools/xenfb/xenfb.c
===================================================================
--- build-32-unstable-13568.orig/tools/xenfb/xenfb.c
+++ build-32-unstable-13568/tools/xenfb/xenfb.c
@@ -362,12 +362,12 @@ static int xenfb_map_fb(struct xenfb_pri
 		 */
 		uint32_t *ptr32 = NULL;
 		uint32_t *ptr64 = NULL;
-#if defined(__i386_)
-		ptr32 = page->pd;
+#if defined(__i386__)
+		ptr32 = (void*)page->pd;
 		ptr64 = ((void*)page->pd) + 4;
 #elif defined(__x86_64__)
 		ptr32 = ((void*)page->pd) - 4;
-		ptr64 = page->pd;
+		ptr64 = (void*)page->pd;
 #endif
 		if (ptr32) {
 			if (0 == ptr32[1]) {
@@ -383,7 +383,7 @@ static int xenfb_map_fb(struct xenfb_pri
 		/* 64bit dom0, 32bit domU */
 		mode = 32;
 		pd   = ((void*)page->pd) - 4;
-#elif defined(__i386_)
+#elif defined(__i386__)
 	} else if (0 == strcmp(xenfb->protocol, XEN_IO_PROTO_ABI_X86_64)) {
 		/* 32bit dom0, 64bit domU */
 		mode = 64;
@@ -441,9 +441,6 @@ static int xenfb_bind(struct xenfb_devic
 	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "event-channel", "%u",
 			    &evtchn) < 0)
 		return -1;
-	if (xenfb_xs_scanf1(xenfb->xsh, dev->otherend, "protocol", "%63s",
-			    xenfb->protocol) < 0)
-		xenfb->protocol[0] = '\0';
 
 	dev->port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
 					       dev->otherend_id, evtchn);
@@ -567,6 +564,9 @@ int xenfb_attach_dom(struct xenfb *xenfb
 		errno = ENOTSUP;
 		goto error;
 	}
+	if (xenfb_xs_scanf1(xsh, xenfb->fb.otherend, "protocol", "%63s",
+			    xenfb->protocol) < 0)
+		xenfb->protocol[0] = '\0';
 	xenfb_xs_printf(xsh, xenfb->fb.nodename, "request-update", "1");
 
 	/* TODO check for permitted ranges */

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

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

32-on-64 broken in unstable.

[Gerd Hoffmann]

  Hi,

> [ bimodal driver patches ]
> They are partly tested only though, 32-on-64 seems to be
> broken in current unstable, mixing 32bit and 64bit domains doesn't work.

bisected that one, changeset 13522 is the culpit which breaks booting
32pae guests on 64bit xen (and dom0).

Ideas anyone?

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: 32-on-64 broken in unstable.

[Keir Fraser]

On 25/1/07 13:16, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> bisected that one, changeset 13522 is the culpit which breaks booting
> 32pae guests on 64bit xen (and dom0).
> 
> Ideas anyone?

The hook added to set_info_guest() to call vcpu_reset() is probably the most
likely culprit. However this code is churned some more by c/s 13594 which is
currently sitting in our staging tree (the Linux upgrade has caused some of
our regression tests to start failing). If necessary we'll do a manual push
to the public tree later today.

 -- Keir

Re: 32-on-64 broken in unstable.

[Gerd Hoffmann]

Keir Fraser wrote:
> On 25/1/07 13:16, "Gerd Hoffmann" <kraxel@suse.de> wrote:
> 
>> bisected that one, changeset 13522 is the culpit which breaks booting
>> 32pae guests on 64bit xen (and dom0).
>>
>> Ideas anyone?
> 
> The hook added to set_info_guest() to call vcpu_reset() is probably the most
> likely culprit.

It is.  ifdef'ing out these three lines makes it work again.

thanks,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>