WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] 32-on-64: pvfb issue

To: Gerd Hoffmann <kraxel@xxxxxxx>
Subject: Re: [Xen-devel] 32-on-64: pvfb issue
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Mon, 22 Jan 2007 16:22:21 +0100
Cc: Xen devel list <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <keir@xxxxxxxxxxxxx>
Delivery-date: Mon, 22 Jan 2007 07:21:53 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <45B4C3C0.1030406@xxxxxxx> (Gerd Hoffmann's message of "Mon, 22 Jan 2007 15:01:36 +0100")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <C1D69CCA.7E66%keir@xxxxxxxxxxxxx> <45B46CBF.7010406@xxxxxxx> <45B4C3C0.1030406@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux)
Gerd Hoffmann <kraxel@xxxxxxx> 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@xxxxxxx>
> ---
>  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);
[...]

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