[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.