From mboxrd@z Thu Jan 1 00:00:00 1970 From: Markus Armbruster Subject: Re: 32-on-64: pvfb issue Date: Mon, 22 Jan 2007 16:22:21 +0100 Message-ID: <87bqkrqe6q.fsf@pike.pond.sub.org> References: <45B46CBF.7010406@suse.de> <45B4C3C0.1030406@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: In-Reply-To: <45B4C3C0.1030406@suse.de> (Gerd Hoffmann's message of "Mon, 22 Jan 2007 15:01:36 +0100") List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Gerd Hoffmann Cc: Xen devel list , Keir Fraser List-Id: xen-devel@lists.xenproject.org Gerd Hoffmann 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 > --- > 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 > #include > #include > +#include > #include > #include > > @@ -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 > #include > #include > +#include > #include > #include > #include > @@ -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 > #include > #include > +#include > #include > #include > #include > @@ -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); [...]