Paravirt framebuffer series [0/6]
PATCH 1/1 Paravirt framebuffer series [0/6]
The following series of patches is a roll-up of everything needed for the paravirt framebuffer to work for me.
http://lists.xensource.com/archives/html/xen-devel/2006-07/msg00156.html
is the last mail about the basic idea and there's also
http://wiki.xensource.com/xenwiki/VirtualFramebuffer
This set of patches includes all of the basic infrastructure from the previous patchset. Also, for infrastructure, it includes Amos Waterland's work to get xenconsole using /dev/xvc0 instead of taking over the primary tty devices.
The new patches add support for launching sdlfb or vncfb from xend if you've configured your guest to do so. We also
set /<dompath>/console/use_graphics to let the guest know that it should default to the graphical console. Otherwise, it falls back to using the xenconsole device.
Jeremy
Quoted 11 lines · Jeremy Katz
The following series of patches is a roll-up of everything needed for the paravirt framebuffer to work for me.
http://lists.xensource.com/archives/html/xen-devel/2006-07/msg00156.html
is the last mail about the basic idea and there's also
http://wiki.xensource.com/xenwiki/VirtualFramebufferThis set of patches includes all of the basic infrastructure from the previous patchset. Also, for infrastructure, it includes Amos Waterland's work to get xenconsole using
/dev/xvc0instead of taking over the primary tty devices.
Quoted 5 lines
The new patches add support for launching sdlfb or vncfb from xend if you've configured your guest to do so. We also
set /<dompath>/console/use_graphics to let the guest know that it should default to the graphical console. Otherwise, it falls back to using the xenconsole device.
We've now had a chance to look through these patches. Steven who did the review is unavailable for a couple of weeks, so I'm forwarding his comments. I think it's absolutely necessary to address all issues with the protocol used between the frontend and the backend - the layout of any structures used on the communication rings and all information which gets written to xenstore.
christian
Here are the comments:
I don't really understand why you need to use vmalloc in so many places, but that's could just be because I'm missing something.
Review
Quoted 10 lines
(quoted diff) +10
-0+config XEN_KEYBOARD + tristate "Keyboard-device frontend driver" + depends on XEN + default y + help + The keyboard-device frontend driver allows the kernel to create a + virtual keyboard. This keyboard can then be driven by another + domain. If you've said Y to CONFIG_XEN_FRAMEBUFFER, you probably + want to say Y here. +
What happens if you configure XEN_KEYBOARD but not XEN_FRAMEBUFFER?
Also, this appears to provide mouse support, and so is misnamed.
Quoted 10 lines
config
XEN_SCRUB_PAGES(quoted diff) +6
-0bool "Scrub memory before freeing it to Xen" default y +static int xenfb_thread(void *data) +{ + struct xenfb_info *info = data; + DECLARE_WAITQUEUE(wait, current); + + add_wait_queue(&info->wq, &wait);
Review
Quoted 10 lines
(quoted diff) +10
-0+ for (;;) { + if (kthread_should_stop()) + break; + if (info->dirty) + xenfb_update_screen(info); + set_current_state(TASK_INTERRUPTIBLE); + schedule(); + } + remove_wait_queue(&info->wq, &wait); + return 0;
Is there any reason this couldn't use wait_event_interruptible?
Review
Quoted 6 lines
(quoted diff) +5
-0+} +static void xenfb_timer(unsigned long data) +{ + struct xenfb_info *info = (struct xenfb_info *)data; + info->dirty++;
Could this end up running at the same time as update_screen and race to update dirty? I suppose it doesn't actually matter too much if it does.
Quoted 22 lines
(quoted diff) +22
-0+ wake_up(&info->wq); +} + +static void xenfb_refresh(struct xenfb_info *info, + int x1, int y1, int w, int h) +{ + int y2, x2; + + y2 = y1 + h; + x2 = x1 + w; + if (info->y2 == 0) { + info->y1 = y1; + info->y2 = y2; + } + if (info->x2 == 0) { + info->x1 = x1; + info->x2 = x2; + } + + if (info->y1 > y1) + info->y1 = y1; + if (info->y2 < y2)
Review
Quoted 10 lines
(quoted diff) +10
-0+ info->y2 = y2; + if (info->x1 > x1) + info->x1 = x1; + if (info->x2 < x2) + info->x2 = x2; + + if (timer_pending(&info->refresh)) + return; + + mod_timer(&info->refresh, jiffies + HZ/xenfb_fps);
So the actual refresh is sent iff the screen goes idle for a certain amount of time? I guess that makes sense.
Quoted 9 lines
(quoted diff) +8
-0+} + +static irqreturn_t xenfb_event_handler(int rq, void *dev_id, + struct pt_regs *regs) +{ + struct xenfb_info *info = dev_id; + __u32 cons, prod; +
Review
Quoted 10 lines
(quoted diff) +10
-0+ if (!info->page || !info->page->initialized) + return IRQ_NONE; + + prod = info->page->in_prod; + rmb(); /* ensure we see ring contents up to prod */ + for (cons = info->page->in_cons; cons != prod; cons++) { + union xenfb_in_event *event; + event = &XENFB_RING_REF(info->page->in, cons); + notify_remote_via_evtchn(info->evtchn); + }
I don't think you need to do a notify on every pass of the loop. In fact, I don't think you need any notify_remote, or in fact any loop at all. What is this trying to achieve?
Review
Quoted 1 line
(quoted diff) +1
-0+ /* FIXME do I need a wmb() here? */
Only if you care about not losing the non-existent XENFB incoming events.
Review
Quoted 8 lines
(quoted diff) +8
-0+ info->page->in_cons = cons; + + return IRQ_HANDLED; +} + +static unsigned long vmalloc_to_mfn(void *address) +{ + return pfn_to_mfn(vmalloc_to_pfn(address));
Maybe use arbitrary_virt_to_machine?
Review
Quoted 4 lines
(quoted diff) +4
-0+} + +static struct xenfb_info *xenfb_info; +static int xenfb_irq;
Is there any good reason why these need to be here? xenfb_irq could be put into struct xenfb_info.
Quoted 1 line
+static int
__initxenfb_probe(void)
It might be nice to break this function up a little.
Quoted 1 line
+{
...
Review
Quoted 3 lines
(quoted diff) +3
-0+ info->page->width = 800; + info->page->height = 600; + info->page->depth = 32;
It might be nice to make these #defines somewhere.
Review
Quoted 9 lines
(quoted diff) +9
-0+ info->page->line_length = (info->page->depth / 8) * info->page->width; + info->page->mem_length = xenfb_mem_len; + info->page->in_cons = info->page->in_prod = 0; + info->page->out_cons = info->page->out_prod = 0; + + ret = bind_evtchn_to_irqhandler(info->evtchn, xenfb_event_handler, + 0, "xenfb", info); + if (ret < 0) + // FIXME need to close evtchn?
As far as I can see, yes.
Quoted 2 lines
(quoted diff) +2
-0+ goto error_kfree; +
...
Review
Quoted 2 lines
(quoted diff) +2
-0+ + info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
kthread_run can fail.
Review
Quoted 11 lines
(quoted diff) +11
-0+ + ret = register_framebuffer(fb_info); + if (ret) + goto error_unbind; + + again: + ret = xenbus_transaction_start(&xbt); + if (ret) + goto error_unreg; + ret = xenbus_printf(xbt, "vfb", "page-ref", "%lu", + virt_to_mfn(info->page));
Grant tables?
Review
Quoted 3 lines
(quoted diff) +2
-0+} +void xenfb_resume(void)
This (a) doesn't work (obviously) and (b) isn't called from anywhere.
Quoted 16 lines
(quoted diff) +16
-0+{ +#if 0 /* FIXME */ + int i, ret; + + xenfb_info->page = mfn_to_virt(xen_start_info->fbdev_mfn); + for (i = 0; i < xenfb_info->nr_pages; i++) + xenfb_info->mfns[i] = vmalloc_to_mfn(xenfb_info->fb + i * PAGE_SIZE); + xenfb_info->page->pd[0] = vmalloc_to_mfn(xenfb_info->mfns); + + if (xenfb_irq) + unbind_from_irqhandler(xenfb_irq, NULL); + + printk("xenfb: resume(%d)\n", xen_start_info->fbdev_evtchn); + ret = bind_evtchn_to_irqhandler(xen_start_info->fbdev_evtchn, + xenfb_event_handler, 0, "xenfb", xenfb_info); + if (ret <= 0)
Review
Quoted 10 lines
(quoted diff) +10
-0+ return; + xenfb_irq = ret; +#else + printk(KERN_DEBUG "xenfb_resume not implemented\n"); +#endif +} + +static int __init xenfb_init(void) +{ + return xenfb_probe();
You might want to consider using xenbus_device and friends here.
Quoted 8 lines
(quoted diff) +7
-0+} + +module_exit(xenfb_cleanup); + +MODULE_LICENSE("GPL"); +static irqreturn_t input_handler(int rq, void *dev_id, struct pt_regs *regs) +{
...
Review
Quoted 8 lines
(quoted diff) +8
-0+ event->button.pressed); + break; + case XENKBD_TYPE_KEY: + input_report_key(dev->dev, event->key.keycode, event->key.pressed); + break; + } + + notify_remote_via_evtchn(dev->evtchn);
Do you need to do a notify here?
Review
Quoted 3 lines
(quoted diff) +3
-0+ } + input_sync(dev->dev); + /* FIXME do I need a wmb() here? */
Depends how much you care about overflowing the ring.
Review
Quoted 7 lines
(quoted diff) +7
-0+ info->in_cons = cons; + + return IRQ_HANDLED; +} + +static struct xenkbd_device *xenkbd_dev; +static int xenkbd_irq;
Why are these needed?
Quoted 3 lines
(quoted diff) +3
-0+ +int __init xenkbd_init(void) +{
...
Review
Quoted 5 lines
(quoted diff) +5
-0+ ret = xenbus_transaction_end(xbt, 0); + if (ret) { + if (ret == -EAGAIN) + goto again; + /* FIXME really retry forever? */
It's what we do everywhere else.
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 6 lines
(quoted diff) +2
-0+ goto error_unreg; + }diff -r
ec03b24a2d83-r6ca424e1867elinux-2.6-xen-sparse/include/linux/xenfb.hlinux-2.6-xen-sparse/include/linux/xenfb.h new file +0
-0
This seems to define the actual frontend<->backend protocol, so probably belongs in xen/include/public/io.
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 4 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +4
-0+ +/* out events */ + +#define XENFB_TYPE_MOTION 1
It's not obvious to me what XENFB_TYPE_MOTION would do, and it doesn't appear to ever get generated.
Quoted 11 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +11
-0+#define XENFB_TYPE_UPDATE 2 + +struct xenfb_motion +{ +__u8 type;/* XENFB_TYPE_MOTION */ +__u16 x;/* The new x coordinate */ +__u16 y;/* The new y coordinate */ +}; + +struct xenfb_update +{
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 10 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +10
-0+__u8 type;/* XENFB_TYPE_UPDATE */ +__u16 x;/* source x */ +__u16 y;/* source y */ +__u16 width;/* rect width */ +__u16 height;/* rect height */ +}; + +union xenfb_out_event +{ +__u8 type;
Does this really want to be in the union with the actual out_events?
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +8
-0+struct xenfb_motion motion; +struct xenfb_update update; +char _[40 ]; +}; + +/* in events */ + +#define XENFB_TYPE_SET_EVENTS 1
This is generated but there's no code anywhere to actually process it.
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +8
-0+struct xenfb_page +{ +__u8 initialized; +__u16 width;/* the width of the framebuffer (in pixels) */ +__u16 height;/* the height of the framebuffer (in pixels) */ +__u32 line_length;/* the length of a row of pixels (in bytes) */ +__u32 mem_length;/* the length of the framebuffer (in bytes) */ +__u8 depth;/* the depth of a pixel (in bits) */
Is it really a good idea for these to be on the shared page? What happens if the {front,back}end is malicious and modifies them while the framebuffer is operating normally?
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 2 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +2
-0+ +unsigned long pd[2 ];
This could do with a better name. Also, pd[1] appears to always be 0.
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +5
-0+ +__u32 in_cons, in_prod; +__u32 out_cons, out_prod; + +union xenfb_in_event in[XENFB_IN_RING_SIZE ];
As far as I can see, there's only ever one xenfb_in_event generated throughout the life of the buffer, and it's ignored. Is this queue necessary?
linux-2.6-xen-sparse/include/linux/xenkbd.h
Quoted 9 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +6
-0+ union xenfb_out_event out[XENFB_OUT_RING_SIZE]; +}; + +void xenfb_resume (void ); + +#endif diff -r
ec03b24a2d83-r6ca424e1867elinux-2.6-xen-sparse/include/linux/xenkbd.hlinux-2.6-xen-sparse/include/linux/xenkbd.h new file +0
-0
Again, belongs in public/io.
linux-2.6-xen-sparse/include/linux/xenkbd.h
Quoted 9 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +7
-0@@ -0,0 +1,82 @@ +/* out events */ + +union xenkbd_out_event +{ +__u8 type; +char _[40 ]; +};
Eh?
linux-2.6-xen-sparse/include/linux/xenkbd.h
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +5
-0+ +/* shared page */ + +#define XENKBD_IN_RING_SIZE (2048 / 40) +#define XENKBD_OUT_RING_SIZE (1024 / 40)
Why 2048 and 1024?
linux-2.6-xen-sparse/include/linux/xenkbd.h
Quoted 12 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +12
-0+ +#define XENKBD_RING_SIZE (ring) (sizeof((ring)) / sizeof(*(ring))) +#define XENKBD_RING_REF (ring, idx) (ring)[(idx) % XENKBD_RING_SIZE((ring))] + +struct xenkbd_info +{ +__u8 initialized; +__u32 in_cons, in_prod; +__u32 out_cons, out_prod; + +union xenkbd_in_event in[XENKBD_IN_RING_SIZE ]; +union xenkbd_out_event out[XENKBD_OUT_RING_SIZE ];
What's the out ring for?
Quoted 10 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +5
-0+}; + +void xenkbd_resume (void ); + +#endif diff -r
6ca424e1867e-r103a9f38f17ftools/xenfb/vncfb.c
tools/xenfb/vncfb.c
Quoted 5 lines
tools/xenfb/vncfb.c new file +4
-0@@ -0,0 +1,154 @@ +#if 0 +#define BUG () do { printf("%d\n", __LINE__); return 1; } while(0) +#else +extern void abort ();
#include <stdlib.h> ?
tools/xenfb/vncfb.c
Quoted 6 lines
tools/xenfb/vncfb.c new file +5
-0+#define BUG () abort(); +#endif + + +rfbRunEventLoop (server,-1 , TRUE);
I'm a little worried that you appear to have a multithreaded program with no locks here, but nevermind.
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+struct xenfb *xenfb_new (void ) +{
...
tools/xenfb/vncfb.c
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+ + xenfb->fd =open ("/dev/xen/evtchn" ,O_RDWR );
xc_evtchn_open, maybe.
Quoted 1 line
tools/xenfb/vncfb.c new file +1
-0+if (xenfb->fd ==-1 ) {
...
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+} + +int xenfb_get_fileno (struct xenfb *xenfb_pub) +{ +struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub; +if (xenfb->domid ==-1 ) +return -1 ;
Eh?
tools/xenfb/vncfb.c
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+ +return xenfb->fd; +} + +static void xenfb_unset_domid (struct xenfb_private *xenfb)
This does a fair bit more than unset the domid.
tools/xenfb/vncfb.c
Quoted 12 lines
tools/xenfb/vncfb.c new file +12
-0+{ +struct ioctl_evtchn_unbind unbind; + + xenfb->domid =-1 ; + +munmap (xenfb->fb, xenfb->fb_info->mem_length); +munmap (xenfb->fb_info,XC_PAGE_SIZE ); +munmap (xenfb->kbd_info,XC_PAGE_SIZE ); + unbind.port = xenfb->fbdev_port; +ioctl (xenfb->fd,IOCTL_EVTCHN_UNBIND , &unbind); + unbind.port = xenfb->kbd_port; +ioctl (xenfb->fd,IOCTL_EVTCHN_UNBIND , &unbind);
xc_evtchn_unbind?
Quoted 5 lines
tools/xenfb/vncfb.c new file +4
-0+} +static int xenfb_fb_event (struct xenfb_private *xenfb,union xenfb_in_event *event) +{ +uint32_t prod;
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+struct xenfb_page *info = xenfb->fb_info; +struct ioctl_evtchn_notify notify; + + prod = info->in_prod; +if (prod - info->in_cons ==XENFB_RING_SIZE (info->in)) { + errno =EAGAIN ; +return -1 ; + } + +XENFB_RING_REF (info->in, prod) = *event;
Need a memory barrier here.
tools/xenfb/vncfb.c
Quoted 3 lines
tools/xenfb/vncfb.c new file +3
-0+ info->in_prod = prod +1 ; + notify.port = xenfb->fbdev_port; +return ioctl (xenfb->fd,IOCTL_EVTCHN_NOTIFY , ¬ify);
xc_evtchn_notify()?
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+} + +static int xenfb_kbd_event (struct xenfb_private *xenfb,union xenkbd_in_event *event) +{ +uint32_t prod;
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+struct xenkbd_info *info = xenfb->kbd_info; +struct ioctl_evtchn_notify notify; + + prod = info->in_prod; +if (prod - info->in_cons ==XENKBD_RING_SIZE (info->in)) { + errno =EAGAIN ; +return -1 ; + } + +XENKBD_RING_REF (info->in, prod) = *event;
Need a memory barrier.
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+ info->in_prod = prod +1 ; + notify.port = xenfb->kbd_port; +return ioctl (xenfb->fd,IOCTL_EVTCHN_NOTIFY , ¬ify); +} + +static char *xenfb_path_in_dom (struct xs_handle *h, +unsigned domid,const char *path, +char *buffer,size_t size) +{ +char *domp =xs_get_domain_path (h, domid);
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+int n =snprintf (buffer, size,"%s/%s" , domp, path); +free (domp); +if (n >= size) +return NULL ; +return buffer; +} + +static int xenfb_xs_scanf1 (struct xs_handle *xsh,unsigned domid, +const char *path,const char *fmt, +void *dest)
Not entirely convinced this is a good idea. You could do this properly; vscanf isn't hard to use.
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+{ +char buffer[1024 ]; +char *p; +int ret; + + p =xenfb_path_in_dom (xsh, domid, path, buffer,sizeof (buffer)); + p =xs_read (xsh,XBT_NULL , p,NULL );
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+if (!p) +return -ENOENT ; + ret =sscanf (p, fmt, dest); +free (p); +if (ret !=1 ) +return -EDOM ; +return 0 ; +} + +bool xenfb_set_domid (struct xenfb *xenfb_pub,int domid)
Perhaps misnamed? This does a bit more than setting the domid.
Quoted 1 line
+{
...
tools/xenfb/vncfb.c
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+ } +xs_daemon_close (xsh); + xsh =NULL ; + +printf ("%d, %d, %d\n" , xenfb->fd, xenfb->fbdev_evtchn, domid);
Umm?
tools/xenfb/vncfb.c
Quoted 4 lines
tools/xenfb/vncfb.c new file +4
-0+ + bind.remote_port = xenfb->fbdev_evtchn; + bind.remote_domain = domid; + xenfb->fbdev_port =ioctl (xenfb->fd,IOCTL_EVTCHN_BIND_INTERDOMAIN , &bind);
I'm goign to stop pointing out the xc_evtchn wrappers now.
tools/xenfb/vncfb.c
Quoted 11 lines
tools/xenfb/vncfb.c new file +11
-0+ + xenfb->kbd_info =xc_map_foreign_range (xenfb->xc, domid,XC_PAGE_SIZE , +PROT_READ |PROT_WRITE , + xenfb->kbd_mfn); +if (xenfb->kbd_info ==NULL ) + goto error_fbinfo; + +while (!xenfb->fb_info->initialized) { +struct timeval tv = {0 ,10000 }; +select (0 ,NULL ,NULL ,NULL , &tv); + }
If you ever need to go into this loop there's a problem somewhere. You shouldn't be able to connect things up until the shared area is initialised.
tools/xenfb/vncfb.c
Quoted 6 lines
tools/xenfb/vncfb.c new file +6
-0+ xenfb->fb =xc_map_foreign_batch (xenfb->xc, domid,PROT_READ |PROT_WRITE , xenfb->fbmfns, xenfb->n_fbmfns); +if (xenfb->fb ==NULL ) + goto error_fbmfns; + + { +union xenfb_in_event event;
I know we do this in lots of other places, but it's really ugly. Can this move to the start of the function, please?
tools/xenfb/vncfb.c
Quoted 9 lines
tools/xenfb/vncfb.c new file +9
-0+ + event.type =XENFB_TYPE_SET_EVENTS ; + event.set_events.flags =XENFB_FLAG_UPDATE ; + +if (xenfb_fb_event (xenfb, &event)) + goto error_fb; + } + +munmap (xenfb->fbmfns, xenfb->n_fbdirs *XC_PAGE_SIZE );
Why was fbmfns in xenfb rather than a local?
tools/xenfb/vncfb.c
Quoted 4 lines
tools/xenfb/vncfb.c new file +4
-0+static void xenfb_update (struct xenfb_private *xenfb,int x,int y,int width,int height) +{ +if (xenfb->pub.update) + xenfb->pub.update (&xenfb->pub, x, y, width, height);
Can pub.update ever be null?
Quoted 9 lines
tools/xenfb/vncfb.c new file +9
-0+} + +static void xenfb_on_fb_event (struct xenfb_private *xenfb) +{ +uint32_t prod, cons; +struct xenfb_page *info = xenfb->fb_info; + + prod = info->out_prod; +rmb ();/* ensure we see ring contents up to prod */
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+for (cons = info->out_cons; cons != prod; cons++) { +union xenfb_out_event *event = &XENFB_RING_REF (info->out, cons); + +switch (event->type) { +case XENFB_TYPE_UPDATE : +xenfb_update (xenfb, event->update.x, event->update.y, event->update.width, event->update.height); +break ; + } + } +/* FIXME do I need a wmb() here? */
Maybe. Leaving it out means that the frontend can't reliably test for ring overflow.
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+ info->out_cons = cons; +} + +static void xenfb_on_kbd_event (struct xenfb_private *xenfb) +{ +uint32_t prod, cons; +struct xenkbd_info *info = xenfb->kbd_info;
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+ + prod = info->out_prod; +rmb ();/* ensure we see ring contents up to prod */ +for (cons = info->out_cons; cons != prod; cons++) { +union xenkbd_out_event *event = &XENKBD_RING_REF (info->out, cons); + +switch (event->type) { +default : +break ; + }
Huh?
tools/xenfb/vncfb.c
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+ } +/* FIXME do I need a wmb() here? */
No, because the whole function is completely pointless.
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+ info->out_cons = cons; +} + +int xenfb_on_incoming (struct xenfb *xenfb_pub) +{
tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub; +evtchn_port_t port; + +if (read (xenfb->fd, &port,sizeof (port)) !=sizeof (port)) +return -1 ; + +if (port == xenfb->fbdev_port) { +xenfb_on_fb_event (xenfb); + }else if (port == xenfb->kbd_port) { +xenfb_on_kbd_event (xenfb);
Does this ever happen?
Quoted 8 lines
tools/xenfb/vncfb.c new file +8
-0+ } + +if (write (xenfb->fd, &port,sizeof (port)) !=sizeof (port)) +return -1 ; + +return 0 ; +} +
Quoted 18 lines · Christian Limpach
Quoted 11 lines · Jeremy Katz
The following series of patches is a roll-up of everything needed for the paravirt framebuffer to work for me.
http://lists.xensource.com/archives/html/xen-devel/2006-07/msg00156.html
is the last mail about the basic idea and there's also
http://wiki.xensource.com/xenwiki/VirtualFramebufferThis set of patches includes all of the basic infrastructure from the previous patchset. Also, for infrastructure, it includes Amos Waterland's work to get xenconsole using
/dev/xvc0instead of taking over the primary tty devices.Quoted 5 lines
The new patches add support for launching sdlfb or vncfb from xend if you've configured your guest to do so. We also
set /<dompath>/console/use_graphics to let the guest know that it should default to the graphical console. Otherwise, it falls back to using the xenconsole device.
Quoted 8 lines
We've now had a chance to look through these patches. Steven who did the review is unavailable for a couple of weeks, so I'm forwarding his comments. I think it's absolutely necessary to address all issues with the protocol used between the frontend and the backend - the layout of any structures used on the communication rings and all information which gets written to xenstore.
christian
Thanks a lot for the detailed review!
As I inherited much of the code, I can't really answer some of your questions about why certain things are done the way they're done. I'll do what I can...
It would be useful to know which issues must be fixed before you can merge the frame buffer, and which could be done afterwards.
Quoted 3 lines
Here are the comments:
I don't really understand why you need to use vmalloc in so many places, but that's could just be because I'm missing something.
Huh? I can see just two places, both in xenfb_probe():
* the frame buffer
* the array of mfns for the frame buffer
Anthony, care to explain why?
Quoted 11 lines
Review
Quoted 10 lines
(quoted diff) +10
-0+config XEN_KEYBOARD + tristate "Keyboard-device frontend driver" + depends on XEN + default y + help + The keyboard-device frontend driver allows the kernel to create a + virtual keyboard. This keyboard can then be driven by another + domain. If you've said Y to CONFIG_XEN_FRAMEBUFFER, you probably + want to say Y here. +What happens if you configure
XEN_KEYBOARDbut notXEN_FRAMEBUFFER?
The backend refuses to connect, i.e. you can't use it. Anthony, can you shed some light on why you created two separate configs?
Quoted 1 line
Also, this appears to provide mouse support, and so is misnamed.
It's a device producing input events, both keyboard and mouse. Care to suggest a better name?
Quoted 13 lines
Quoted 13 lines
config
XEN_SCRUB_PAGES(quoted diff) +9
-0bool "Scrub memory before freeing it to Xen" default y +static int xenfb_thread(void *data) +{ + struct xenfb_info *info = data; + DECLARE_WAITQUEUE(wait, current); + + add_wait_queue(&info->wq, &wait); + for (;;) { + if (kthread_should_stop()) + break;
Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ if (info->dirty) + xenfb_update_screen(info); + set_current_state(TASK_INTERRUPTIBLE); + schedule(); + } + remove_wait_queue(&info->wq, &wait); + return 0;Is there any reason this couldn't use
wait_event_interruptible?
Can't see why not, but I'm looking at this code for the first time, too. Want me to try?
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+} +static void xenfb_timer(unsigned long data) +{ + struct xenfb_info *info = (struct xenfb_info *)data; + info->dirty++;Could this end up running at the same time as
update_screenand race to update dirty? I suppose it doesn't actually matter too much if it does.
xenfb_timer() runs from the timer interrupt, sets dirty, then kicks the wait queue.
A separate kernel thread running xenfb_thread() sleeps on the wait queue. When it wakes up, and dirty is set, it clears dirty and updates the screen. Then goes back to sleep.
Obviously, dirty is always set when the thread wakes up. If xenfb_timer() runs before the thread goes to sleep again, the wakeup is lost, and the value of dirty doesn't matter. I believe that's okay.
I clarified info->dirty++ to info->dirty = 1.
Quoted 26 lines
Quoted 26 lines
(quoted diff) +26
-0+ wake_up(&info->wq); +} + +static void xenfb_refresh(struct xenfb_info *info, + int x1, int y1, int w, int h) +{ + int y2, x2; + + y2 = y1 + h; + x2 = x1 + w; + if (info->y2 == 0) { + info->y1 = y1; + info->y2 = y2; + } + if (info->x2 == 0) { + info->x1 = x1; + info->x2 = x2; + } + + if (info->y1 > y1) + info->y1 = y1; + if (info->y2 < y2) + info->y2 = y2; + if (info->x1 > x1) + info->x1 = x1; + if (info->x2 < x2)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ info->x2 = x2; + + if (timer_pending(&info->refresh)) + return; + + mod_timer(&info->refresh, jiffies + HZ/xenfb_fps);So the actual refresh is sent iff the screen goes idle for a certain amount of time? I guess that makes sense.
That's how I read this code.
Quoted 14 lines
Quoted 14 lines
(quoted diff) +13
-0+} + +static irqreturn_t xenfb_event_handler(int rq, void *dev_id, + struct pt_regs *regs) +{ + struct xenfb_info *info = dev_id; + __u32 cons, prod; + + if (!info->page || !info->page->initialized) + return IRQ_NONE; + + prod = info->page->in_prod; + rmb(); /* ensure we see ring contents up to prod */
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ for (cons = info->page->in_cons; cons != prod; cons++) { + union xenfb_in_event *event; + event = &XENFB_RING_REF(info->page->in, cons); + notify_remote_via_evtchn(info->evtchn); + }I don't think you need to do a notify on every pass of the loop. In fact, I don't think you need any
notify_remote, or in fact any loop at all. What is this trying to achieve?
Let me digress a bit.
The code implements the (informal) specification at
http://wiki.xensource.com/xenwiki/VirtualFramebuffer
That specification is designed for possible growth: the protocol between frontend and backend can be extended by new events without breaking backward compatibility.
Some parts of the protocol are just room for growth, e.g. when there are no events of certain kinds defined (yet). The code implementing those parts is partly just to deal gracefully with future extensions, partly pure skeleton. If you look only at the code, this may seem pointless, especially the skeletons.
Back to the code at hand.
I figure we need the loop because event channels don't count events.
The notification is specified by the protocol. Why? I don't know. I didn't design it I just faithfully ported the code to current Xen.
Of course, the protocol *should* be discussed now. Anthony, I think we could use your help there.
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ /* FIXME do I need a wmb() here? */Only if you care about not losing the non-existent XENFB incoming events.
There is an in event: XENFB_TYPE_SET_EVENTS. It's ignored, though. If I read the spec correctly, no output events should be generated unless requested, and this is the means to request them. Currently, the backend always requests update events, the front end ignores such requests, and always generates update events. Do you want this fixed for the initial merge?
I put the wmb() in.
Quoted 9 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ info->page->in_cons = cons; + + return IRQ_HANDLED; +} + +static unsigned long vmalloc_to_mfn(void *address) +{ + return pfn_to_mfn(vmalloc_to_pfn(address));Maybe use
arbitrary_virt_to_machine?
Beats me :)
ADDRESS points into memory obtained from vmalloc(). Tell me how to translate that to mfn, and I'll fix the code.
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+} + +static struct xenfb_info *xenfb_info; +static int xenfb_irq;Is there any good reason why these need to be here?
xenfb_irqcould be put into structxenfb_info.
Agreed on xenfb_irq.
As far as I can see, xenfb_info is needed only for disabled code in xenfb_resume(). If it bothers you, I'll take it out.
Quoted 2 lines
Quoted 1 line
+static int
__initxenfb_probe(void)It might be nice to break this function up a little.
It's a bit long, but the flow's straightforward. If it bothers you, I can try to factor some parts into functions.
Quoted 6 lines
Quoted 1 line
+{
...
Review
Quoted 3 lines
(quoted diff) +3
-0+ info->page->width = 800; + info->page->height = 600; + info->page->depth = 32;It might be nice to make these #defines somewhere.
Matter of taste. Done.
Hmm, xenfb_mem_len should be something like width * height * depth / CHAR_BIT, suitably rounded up.
Quoted 10 lines
Review
Quoted 9 lines
(quoted diff) +9
-0+ info->page->line_length = (info->page->depth / 8) * info->page->width; + info->page->mem_length = xenfb_mem_len; + info->page->in_cons = info->page->in_prod = 0; + info->page->out_cons = info->page->out_prod = 0; + + ret = bind_evtchn_to_irqhandler(info->evtchn, xenfb_event_handler, + 0, "xenfb", info); + if (ret < 0) + // FIXME need to close evtchn?As far as I can see, yes.
Done.
Quoted 6 lines
Quoted 2 lines
(quoted diff) +2
-0+ goto error_kfree; +...
Review
Quoted 2 lines
(quoted diff) +2
-0+ + info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
kthread_runcan fail.
Error checking supplied.
Hmm, do we have to stop the kthread?
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+ + ret = register_framebuffer(fb_info); + if (ret) + goto error_unbind;
Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ + again: + ret = xenbus_transaction_start(&xbt); + if (ret) + goto error_unreg; + ret = xenbus_printf(xbt, "vfb", "page-ref", "%lu", + virt_to_mfn(info->page));Grant tables?
Elaborate, please.
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+} +void xenfb_resume(void)This (a) doesn't work (obviously) and (b) isn't called from anywhere.
Yes. I suspect it's work in progress on suspend/resume. Shall I take it out for now?
Quoted 19 lines
Quoted 19 lines
(quoted diff) +19
-0+{ +#if 0 /* FIXME */ + int i, ret; + + xenfb_info->page = mfn_to_virt(xen_start_info->fbdev_mfn); + for (i = 0; i < xenfb_info->nr_pages; i++) + xenfb_info->mfns[i] = vmalloc_to_mfn(xenfb_info->fb + i * PAGE_SIZE); + xenfb_info->page->pd[0] = vmalloc_to_mfn(xenfb_info->mfns); + + if (xenfb_irq) + unbind_from_irqhandler(xenfb_irq, NULL); + + printk("xenfb: resume(%d)\n", xen_start_info->fbdev_evtchn); + ret = bind_evtchn_to_irqhandler(xen_start_info->fbdev_evtchn, + xenfb_event_handler, 0, "xenfb", xenfb_info); + if (ret <= 0) + return; + xenfb_irq = ret; +#else
Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ printk(KERN_DEBUG "xenfb_resume not implemented\n"); +#endif +} + +static int __init xenfb_init(void) +{ + return xenfb_probe();You might want to consider using
xenbus_deviceand friends here.
I considered that, but it wasn't obvious to me how to make it fit cleanly to a user-space backend. Care to advise?
Quoted 9 lines
Quoted 8 lines
(quoted diff) +7
-0+} + +module_exit(xenfb_cleanup); + +MODULE_LICENSE("GPL"); +static irqreturn_t input_handler(int rq, void *dev_id, struct pt_regs *regs) +{...
Quoted 9 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ event->button.pressed); + break; + case XENKBD_TYPE_KEY: + input_report_key(dev->dev, event->key.keycode, event->key.pressed); + break; + } + + notify_remote_via_evtchn(dev->evtchn);Do you need to do a notify here?
Specified by the protocol.
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ } + input_sync(dev->dev); + /* FIXME do I need a wmb() here? */Depends how much you care about overflowing the ring.
I put the wmb() in.
Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ info->in_cons = cons; + + return IRQ_HANDLED; +} + +static struct xenkbd_device *xenkbd_dev; +static int xenkbd_irq;Why are these needed?
Same deal as for xenfb_info and xenfb_irq above, I guess.
Quoted 10 lines
Quoted 3 lines
(quoted diff) +3
-0+ +int __init xenkbd_init(void) +{...
Review
Quoted 5 lines
(quoted diff) +5
-0+ ret = xenbus_transaction_end(xbt, 0); + if (ret) { + if (ret == -EAGAIN) + goto again; + /* FIXME really retry forever? */It's what we do everywhere else.
Works for me then.
Quoted 9 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 6 lines
(quoted diff) +2
-0+ goto error_unreg; + }diff -r
ec03b24a2d83-r6ca424e1867elinux-2.6-xen-sparse/include/linux/xenfb.hlinux-2.6-xen-sparse/include/linux/xenfb.h new file +0
-0This seems to define the actual frontend<->backend protocol, so probably belongs in xen/include/public/io.
Quoted 6 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 4 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +4
-0+ +/* out events */ + +#define XENFB_TYPE_MOTION 1It's not obvious to me what
XENFB_TYPE_MOTIONwould do, and it doesn't appear to ever get generated.
Appears to be a sketch of future work. I don't understand it either. Anthony?
Quoted 14 lines
Quoted 14 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +14
-0+#define XENFB_TYPE_UPDATE 2 + +struct xenfb_motion +{ +__u8 type;/* XENFB_TYPE_MOTION */ +__u16 x;/* The new x coordinate */ +__u16 y;/* The new y coordinate */ +}; + +struct xenfb_update +{ +__u8 type;/* XENFB_TYPE_UPDATE */ +__u16 x;/* source x */ +__u16 y;/* source y */
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 7 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +7
-0+__u16 width;/* rect width */ +__u16 height;/* rect height */ +}; + +union xenfb_out_event +{ +__u8 type;Does this really want to be in the union with the actual
out_events?
Convenient way to access the type. Sure, you could just access update.type, but that suggests we already know it's update, which is somewhat misleading when we don't.
Quoted 9 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +8
-0+struct xenfb_motion motion; +struct xenfb_update update; +char _[40 ]; +}; + +/* in events */ + +#define XENFB_TYPE_SET_EVENTS 1This is generated but there's no code anywhere to actually process it.
Yes. See discussion of XENFB_TYPE_SET_EVENTS above.
Quoted 11 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +8
-0+struct xenfb_page +{ +__u8 initialized; +__u16 width;/* the width of the framebuffer (in pixels) */ +__u16 height;/* the height of the framebuffer (in pixels) */ +__u32 line_length;/* the length of a row of pixels (in bytes) */ +__u32 mem_length;/* the length of the framebuffer (in bytes) */ +__u8 depth;/* the depth of a pixel (in bits) */ Is it really a good idea for these to be on the shared page? What happens if the {front,back}end is malicious and modifies them while the framebuffer is operating normally?
Obviously, both frontend and backend should be robust against bad shared page contents. The shared page is writable by both, because both need to step the rings.
The members above are for information flowing from frontend to backend. For safety, they should be write-only in the frontend, and the backend should copy the info to a safer place, checking its consistency.
I can improve safety as sketched. Feel free to suggest a better way to transmit the same information.
Quoted 3 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 2 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +2
-0+ +unsigned long pd[2 ];This could do with a better name. Also, pd[1] appears to always be 0.
I suspect it's short for `page directory'. Shall we call it pgdir?
Yes, pd[1] is always 0 and appears not to be used. Anthony, why is pd an array?
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenfb.h
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +5
-0+ +__u32 in_cons, in_prod; +__u32 out_cons, out_prod; + +union xenfb_in_event in[XENFB_IN_RING_SIZE ];As far as I can see, there's only ever one
xenfb_in_eventgenerated throughout the life of the buffer, and it's ignored. Is this queue necessary?
See discussion of XENFB_TYPE_SET_EVENTS above.
Quoted 13 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h
Quoted 9 lines
linux-2.6-xen-sparse/include/linux/xenfb.h new file +6
-0+ union xenfb_out_event out[XENFB_OUT_RING_SIZE]; +}; + +void xenfb_resume (void ); + +#endif diff -r
ec03b24a2d83-r6ca424e1867elinux-2.6-xen-sparse/include/linux/xenkbd.hlinux-2.6-xen-sparse/include/linux/xenkbd.h new file +0
-0Again, belongs in public/io.
Quoted 1 line
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +0
-0@@ -0,0 +1,82 @@
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h line 7
Quoted 7 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +7
-0+/* out events */ + +union xenkbd_out_event +{ +__u8 type; +char _[40 ]; +};Eh?
Again, the protocol is designed for extendability. This is a placeholder for future xenkbd output events.
Quoted 6 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h line 12
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +5
-0+ +/* shared page */ + +#define XENKBD_IN_RING_SIZE (2048 / 40) +#define XENKBD_OUT_RING_SIZE (1024 / 40)Why 2048 and 1024?
The shared page is carved up into three parts: info at offset 0, size up to 1024, input ring at offset 1024, size 2*1024, output ring at offset 3*1024, size 1024.
I fear the code doesn't implement the spec... rings follow info immediately, so they'll wander when info is extended. Want me to fix this?
Same issue with xenfb.h, with ring sizes swapped.
Quoted 5 lines
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +5
-0+ +#define XENKBD_RING_SIZE (ring) (sizeof((ring)) / sizeof(*(ring))) +#define XENKBD_RING_REF (ring, idx) (ring)[(idx) % XENKBD_RING_SIZE((ring))] + +struct xenkbd_info
Quoted 8 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h line 24
Quoted 7 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +7
-0+{ +__u8 initialized; +__u32 in_cons, in_prod; +__u32 out_cons, out_prod; + +union xenkbd_in_event in[XENKBD_IN_RING_SIZE ]; +union xenkbd_out_event out[XENKBD_OUT_RING_SIZE ];What's the out ring for?
Placeholder for xenkbd output events.
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h line 29
Quoted 5 lines
linux-2.6-xen-sparse/include/linux/xenkbd.h new file +5
-0+}; + +void xenkbd_resume (void ); + +#endif
[Review of backend...]
I'll reply to this separately.
Quoting Markus Armbruster <[email protected]>:
Quoted 5 lines
Quoted 3 lines
Here are the comments:
I don't really understand why you need to use vmalloc in so many places, but that's could just be because I'm missing something.Huh? I can see just two places, both in
xenfb_probe():
What would be the alternatives to vmalloc? Grant tables are unusable in their current form because of the amount of memory needed to allocate (grant tables are limited to 1024 4k references ATM I believe). The kmalloc() pool is limited and my understanding is that it's not meant for very large allocations.
I'm not an expert here though so alternative suggestions are appreciated.
Quoted 18 lines
* the frame buffer
* the array of mfns for the frame buffer
Anthony, care to explain why?
Quoted 11 lines
Review
Quoted 10 lines
(quoted diff) +10
-0+config XEN_KEYBOARD + tristate "Keyboard-device frontend driver" + depends on XEN + default y + help + The keyboard-device frontend driver allows the kernel to create a + virtual keyboard. This keyboard can then be driven by another + domain. If you've said Y to CONFIG_XEN_FRAMEBUFFER, you probably + want to say Y here. +What happens if you configure
XEN_KEYBOARDbut notXEN_FRAMEBUFFER?
Quoted 2 lines
The backend refuses to connect, i.e. you can't use it. Anthony, can you shed some light on why you created two separate configs?
They are two separate devices. Seemed like a natural separation to me.
Quoted 4 lines
Quoted 1 line
Also, this appears to provide mouse support, and so is misnamed.
It's a device producing input events, both keyboard and mouse. Care to suggest a better name?
There are keyboards that also have built in mice (typically a little pad in the upper corner). Naming has never been my strong point though :-)
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+static irqreturn_t xenfb_event_handler(int rq, void *dev_id, + struct pt_regs *regs) +{ + struct xenfb_info *info = dev_id; + __u32 cons, prod; + + if (!info->page || !info->page->initialized) + return IRQ_NONE; + + prod = info->page->in_prod; + rmb(); /* ensure we see ring contents up to prod */
Quoted 8 lines
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ for (cons = info->page->in_cons; cons != prod; cons++) { + union xenfb_in_event *event; + event = &XENFB_RING_REF(info->page->in, cons); + notify_remote_via_evtchn(info->evtchn); + }I don't think you need to do a notify on every pass of the loop. In fact, I don't think you need any
notify_remote, or in fact any loop at all. What is this trying to achieve?
You could batch up the removal of all events and change the semantics of the notification to "I've changed the queue in some way" verses "I've handled a single event on the queue".
You do need *some* sort of notification so that if the other end is buffering writes, it can know to retry a write.
Why even bother with the loop at all? Forward compatibility. When we add backend events (and we will at some point), we can make them, semantically, suggestions. This lets the front-end ignore requests that it doesn't understand. At this case, it doesn't understand any so it just ignores them all. However, if we didn't do the event channel notification, any future clients would break as the queue would fill up.
Quoted 9 lines
Quoted 9 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ info->page->in_cons = cons; + + return IRQ_HANDLED; +} + +static unsigned long vmalloc_to_mfn(void *address) +{ + return pfn_to_mfn(vmalloc_to_pfn(address));Maybe use
arbitrary_virt_to_machine?
If that existed when I wrote the code, I didn't know about it :-)
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
+{
...
Review
Quoted 3 lines
(quoted diff) +3
-0+ info->page->width = 800; + info->page->height = 600; + info->page->depth = 32;It might be nice to make these #defines somewhere.
Matter of taste. Done.
I personally dislike single-use defines...
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+ + ret = register_framebuffer(fb_info); + if (ret) + goto error_unbind;Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ + again: + ret = xenbus_transaction_start(&xbt); + if (ret) + goto error_unreg; + ret = xenbus_printf(xbt, "vfb", "page-ref", "%lu", + virt_to_mfn(info->page));Grant tables?
Quoted 1 line
Elaborate, please.
Grant tables are a mechanism to do generic memory sharing between domains. It's limited in its current form to small amounts of memory but it would be appropriate for the queues at least. When I first wrote this, I wasn't using XenBus so that it would be available early in boot. However, now there we're using XenBus, grant tables would be a good option.
Quoted 8 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+/* out events */ + +#define XENFB_TYPE_MOTION 1It's not obvious to me what
XENFB_TYPE_MOTIONwould do, and it doesn't appear to ever get generated.Appears to be a sketch of future work. I don't understand it either. Anthony?
It's perhaps poorly named. It's for supporting hardware cursor offloading. At one point, I had hacked the fbdev driver to actually use hardware cursor offloading and was generating these events. It could be removed until we support cursor offloading.
Quoted 8 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+union xenfb_out_event +{ + __u8 type;Does this really want to be in the union with the actual
out_events?Convenient way to access the type. Sure, you could just access update.type, but that suggests we already know it's update, which is somewhat misleading when we don't.
It's a matter of personal taste.
Quoted 3 lines
Quoted 3 lines
Is it really a good idea for these to be on the shared page? What happens if the {front,back}end is malicious and modifies them while the framebuffer is operating normally?
Are we really concerned about malicious front-end and back-ends? A malicious back-end disk driver could send data containing malware. Why would we concern ourselves with that though?
Quoted 8 lines
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + unsigned long pd[2];This could do with a better name. Also, pd[1] appears to always be 0.
I suspect it's short for `page directory'. Shall we call it pgdir?
Yes, pd[1] is always 0 and appears not to be used. Anthony, why is pd an array?
To support very large framebuffers (1900x1200) you need > 4MB of data. The size of the pd[] could be variable but an 8MB framebuffer seems large enough for quite a long time...
Quoted 8 lines
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + __u32 in_cons, in_prod; + __u32 out_cons, out_prod; + + union xenfb_in_event in[XENFB_IN_RING_SIZE];As far as I can see, there's only ever one
xenfb_in_eventgenerated throughout the life of the buffer, and it's ignored. Is this queue necessary?
Forward compatibility. There will be more events in the future and preserving guest ABI compatibility is important...
Quoted 3 lines
Again, the protocol is designed for extendability. This is a placeholder for future xenkbd output events.
Think things like caps-lock enable. It will be needed eventually. The code is there now to establish an ABI.
Quoted 6 lines
Quoted 6 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ +/* shared page */ + +#define XENKBD_IN_RING_SIZE (2048 / 40) +#define XENKBD_OUT_RING_SIZE (1024 / 40)Why 2048 and 1024?
I assumed that input events would be more frequent (key events) than output events (caps-lock on). We have other asymmetric queues in Xen (I believe the console is asymmetric too).
Quoted 3 lines
I fear the code doesn't implement the spec... rings follow info immediately, so they'll wander when info is extended. Want me to fix this?
Right, good catch!
Regards,
Anthony Liguori
"Christian Limpach" <[email protected]> writes:
[Review of frontend...]
I replied to that separately.
Quoted 10 lines
tools/xenfb/vncfb.c
Quoted 9 lines
diff -r
6ca424e1867e-r103a9f38f17ftools/xenfb/vncfb.ctools/xenfb/vncfb.c new file +4
-0@@ -0,0 +1,154 @@ +#if 0 +#define BUG () do { printf("%d\n", __LINE__); return 1; } while(0) +#else +extern void abort ();#include <
stdlib.h> ?
My debug code crept into the patch, oops.
Anthony's code had the first BUG() definition. I hate it. I want code to crash hard when it detects it messed up. Should be cleaned up. What behavior do you prefer?
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 6 lines
tools/xenfb/vncfb.c new file +5
-0+#define BUG () abort(); +#endif + + +rfbRunEventLoop (server,-1 , TRUE);I'm a little worried that you appear to have a multithreaded program with no locks here, but nevermind.
As far as I can see, our client code doesn't share data with LibVNCServer, it only calls its services. Therefore, I don't think locking is necessary, unless the LibVNCServer API requires it. And that would be odd, wouldn't it? I can't tell for sure, as I'm not familiar with it.
Quoted 6 lines
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+struct xenfb *xenfb_new (void ) +{...
tools/xenfb/vncfb.c
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+ + xenfb->fd =open ("/dev/xen/evtchn" ,O_RDWR );
xc_evtchn_open, maybe.
Certainly.
Quoted 10 lines
Quoted 1 line
tools/xenfb/vncfb.c new file +1
-0+if (xenfb->fd ==-1 ) {...
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+} + +int xenfb_get_fileno (struct xenfb *xenfb_pub) +{ +struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub; +if (xenfb->domid ==-1 ) +return -1 ;Eh?
I don't understand why this is necessary or useful either. I'll remove it and see what happens.
Quoted 6 lines
tools/xenfb/vncfb.c
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+ +return xenfb->fd; +} + +static void xenfb_unset_domid (struct xenfb_private *xenfb)This does a fair bit more than unset the domid.
Sometimes things outgrow initial names... renamed to xenfb_detach_dom().
Quoted 5 lines
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+{ +struct ioctl_evtchn_unbind unbind; + + xenfb->domid =-1 ; +
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+munmap (xenfb->fb, xenfb->fb_info->mem_length); +munmap (xenfb->fb_info,XC_PAGE_SIZE ); +munmap (xenfb->kbd_info,XC_PAGE_SIZE ); + unbind.port = xenfb->fbdev_port; +ioctl (xenfb->fd,IOCTL_EVTCHN_UNBIND , &unbind); + unbind.port = xenfb->kbd_port; +ioctl (xenfb->fd,IOCTL_EVTCHN_UNBIND , &unbind);
xc_evtchn_unbind?
Yes.
Quoted 16 lines
Quoted 5 lines
tools/xenfb/vncfb.c new file +4
-0+} +static int xenfb_fb_event (struct xenfb_private *xenfb,union xenfb_in_event *event) +{ +uint32_t prod;tools/xenfb/vncfb.c
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+struct xenfb_page *info = xenfb->fb_info; +struct ioctl_evtchn_notify notify; + + prod = info->in_prod; +if (prod - info->in_cons ==XENFB_RING_SIZE (info->in)) { + errno =EAGAIN ; +return -1 ; + } + +XENFB_RING_REF (info->in, prod) = *event;Need a memory barrier here.
You didn't ask for memory barriers in similar code in the frontend. Did you just miss it there, or do I miss something?
Quoted 4 lines
tools/xenfb/vncfb.c
Quoted 3 lines
tools/xenfb/vncfb.c new file +3
-0+ info->in_prod = prod +1 ; + notify.port = xenfb->fbdev_port; +return ioctl (xenfb->fd,IOCTL_EVTCHN_NOTIFY , ¬ify);
xc_evtchn_notify()?
Yes.
Quoted 8 lines
Quoted 8 lines
tools/xenfb/vncfb.c new file +8
-0+} + +static int xenfb_kbd_event (struct xenfb_private *xenfb,union xenkbd_in_event *event) +{ +uint32_t prod; +struct xenkbd_info *info = xenfb->kbd_info; +struct ioctl_evtchn_notify notify; +
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+ prod = info->in_prod; +if (prod - info->in_cons ==XENKBD_RING_SIZE (info->in)) { + errno =EAGAIN ; +return -1 ; + } + +XENKBD_RING_REF (info->in, prod) = *event;Need a memory barrier.
Yes, just as in xenfb_fb_event() above.
Quoted 14 lines
Quoted 14 lines
tools/xenfb/vncfb.c new file +14
-0+ info->in_prod = prod +1 ; + notify.port = xenfb->kbd_port; +return ioctl (xenfb->fd,IOCTL_EVTCHN_NOTIFY , ¬ify); +} + +static char *xenfb_path_in_dom (struct xs_handle *h, +unsigned domid,const char *path, +char *buffer,size_t size) +{ +char *domp =xs_get_domain_path (h, domid); +int n =snprintf (buffer, size,"%s/%s" , domp, path); +free (domp); +if (n >= size) +return NULL ;
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 6 lines
tools/xenfb/vncfb.c new file +6
-0+return buffer; +} + +static int xenfb_xs_scanf1 (struct xs_handle *xsh,unsigned domid, +const char *path,const char *fmt, +void *dest)Not entirely convinced this is a good idea. You could do this properly; vscanf isn't hard to use.
Laziness on my part. Always try the most stupid thing that could possibly solve the problem first ;)
I feel the proper solution belongs into a library anyway.
Quoted 10 lines
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+{ +char buffer[1024 ]; +char *p; +int ret; + + p =xenfb_path_in_dom (xsh, domid, path, buffer,sizeof (buffer)); + p =xs_read (xsh,XBT_NULL , p,NULL ); +if (!p) +return -ENOENT ; + ret =sscanf (p, fmt, dest);
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+free (p); +if (ret !=1 ) +return -EDOM ; +return 0 ; +} + +bool xenfb_set_domid (struct xenfb *xenfb_pub,int domid)Perhaps misnamed? This does a bit more than setting the domid.
Renamed to xenfb_attach_dom().
Quoted 8 lines
Quoted 1 line
+{
...
tools/xenfb/vncfb.c
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+ } +xs_daemon_close (xsh); + xsh =NULL ; + +printf ("%d, %d, %d\n" , xenfb->fd, xenfb->fbdev_evtchn, domid);Umm?
Debugging leftover, removed.
Quoted 5 lines
tools/xenfb/vncfb.c
Quoted 4 lines
tools/xenfb/vncfb.c new file +4
-0+ + bind.remote_port = xenfb->fbdev_evtchn; + bind.remote_domain = domid; + xenfb->fbdev_port =ioctl (xenfb->fd,IOCTL_EVTCHN_BIND_INTERDOMAIN , &bind);I'm goign to stop pointing out the
xc_evtchnwrappers now.
I think I got them all :)
Quoted 6 lines
Quoted 6 lines
tools/xenfb/vncfb.c new file +6
-0+ + xenfb->kbd_info =xc_map_foreign_range (xenfb->xc, domid,XC_PAGE_SIZE , +PROT_READ |PROT_WRITE , + xenfb->kbd_mfn); +if (xenfb->kbd_info ==NULL ) + goto error_fbinfo;
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 5 lines
tools/xenfb/vncfb.c new file +5
-0+ +while (!xenfb->fb_info->initialized) { +struct timeval tv = {0 ,10000 }; +select (0 ,NULL ,NULL ,NULL , &tv); + }If you ever need to go into this loop there's a problem somewhere. You shouldn't be able to connect things up until the shared area is initialised.
I think this is a leftover of the pre-xenstore way to connect things. I'll look into getting rid of the initialized flag.
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 6 lines
tools/xenfb/vncfb.c new file +6
-0+ xenfb->fb =xc_map_foreign_batch (xenfb->xc, domid,PROT_READ |PROT_WRITE , xenfb->fbmfns, xenfb->n_fbmfns); +if (xenfb->fb ==NULL ) + goto error_fbmfns; + + { +union xenfb_in_event event;I know we do this in lots of other places, but it's really ugly. Can this move to the start of the function, please?
Sure.
Quoted 10 lines
tools/xenfb/vncfb.c
Quoted 9 lines
tools/xenfb/vncfb.c new file +9
-0+ + event.type =XENFB_TYPE_SET_EVENTS ; + event.set_events.flags =XENFB_FLAG_UPDATE ; + +if (xenfb_fb_event (xenfb, &event)) + goto error_fb; + } + +munmap (xenfb->fbmfns, xenfb->n_fbdirs *XC_PAGE_SIZE );Why was fbmfns in xenfb rather than a local?
I guess Anthony felt it logically belongs there. It could be made local, along with a few other members. I can do that.
Quoted 5 lines
tools/xenfb/vncfb.c
Quoted 4 lines
tools/xenfb/vncfb.c new file +4
-0+static void xenfb_update (struct xenfb_private *xenfb,int x,int y,int width,int height) +{ +if (xenfb->pub.update) + xenfb->pub.update (&xenfb->pub, x, y, width, height);Can pub.update ever be null?
Can't happen because both users (vncfb.c and sdlfb.c) fill it in before they call xenfb_update(). But xenfb.c doesn't want to know about that, so it checks.
Quoted 13 lines
Quoted 13 lines
tools/xenfb/vncfb.c new file +13
-0+} + +static void xenfb_on_fb_event (struct xenfb_private *xenfb) +{ +uint32_t prod, cons; +struct xenfb_page *info = xenfb->fb_info; + + prod = info->out_prod; +rmb ();/* ensure we see ring contents up to prod */ +for (cons = info->out_cons; cons != prod; cons++) { +union xenfb_out_event *event = &XENFB_RING_REF (info->out, cons); + +switch (event->type) {
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 6 lines
tools/xenfb/vncfb.c new file +6
-0+case XENFB_TYPE_UPDATE : +xenfb_update (xenfb, event->update.x, event->update.y, event->update.width, event->update.height); +break ; + } + } +/* FIXME do I need a wmb() here? */ Maybe. Leaving it out means that the frontend can't reliably test for ring overflow.
I'm not sure I understand. Can you give an example of how things can go wrong?
Quoted 10 lines
Quoted 10 lines
tools/xenfb/vncfb.c new file +10
-0+ info->out_cons = cons; +} + +static void xenfb_on_kbd_event (struct xenfb_private *xenfb) +{ +uint32_t prod, cons; +struct xenkbd_info *info = xenfb->kbd_info; + + prod = info->out_prod; +rmb ();/* ensure we see ring contents up to prod */
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+for (cons = info->out_cons; cons != prod; cons++) { +union xenkbd_out_event *event = &XENKBD_RING_REF (info->out, cons); + +switch (event->type) { +default : +break ; + }Huh?
Skeleton code. There are no output events defined, yet.
Quoted 3 lines
tools/xenfb/vncfb.c
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+ } +/* FIXME do I need a wmb() here? */ No, because the whole function is completely pointless.
It's a skeleton. I can remove it if it bothers you. If we keep it, it should be as correct as we can make it.
Otherwise, same deal as for xenfb_fb_event().
Quoted 8 lines
Quoted 8 lines
tools/xenfb/vncfb.c new file +8
-0+ info->out_cons = cons; +} + +int xenfb_on_incoming (struct xenfb *xenfb_pub) +{ +struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub; +evtchn_port_t port; +
Quoted 8 lines
tools/xenfb/vncfb.c
Quoted 7 lines
tools/xenfb/vncfb.c new file +7
-0+if (read (xenfb->fd, &port,sizeof (port)) !=sizeof (port)) +return -1 ; + +if (port == xenfb->fbdev_port) { +xenfb_on_fb_event (xenfb); + }else if (port == xenfb->kbd_port) { +xenfb_on_kbd_event (xenfb);Does this ever happen?
Not with the current frontend.
Quoted 8 lines
Quoted 8 lines
tools/xenfb/vncfb.c new file +8
-0+ } + +if (write (xenfb->fd, &port,sizeof (port)) !=sizeof (port)) +return -1 ; + +return 0 ; +} +
Quoting Markus Armbruster <[email protected]>:
Quoted 17 lines
"Christian Limpach" <[email protected]> writes:
[Review of frontend...]
I replied to that separately.
Quoted 10 lines
tools/xenfb/vncfb.c
Quoted 9 lines
diff -r
6ca424e1867e-r103a9f38f17ftools/xenfb/vncfb.ctools/xenfb/vncfb.c new file +4
-0@@ -0,0 +1,154 @@ +#if 0 +#define BUG () do { printf("%d\n", __LINE__); return 1; } while(0) +#else +extern void abort ();#include <
stdlib.h> ?
Quoted 3 lines
My debug code crept into the patch, oops.
Anthony's code had the first
BUG()definition. I hate it.
That's just a macro I use for finding where a problem is when things crash in weird ways. It can be removed as there shouldn't be any BUG()s in the submitted patch.
Quoted 10 lines
Quoted 4 lines
tools/xenfb/vncfb.c
Quoted 2 lines
tools/xenfb/vncfb.c new file +2
-0+ +rfbRunEventLoop (server,-1 , TRUE);I'm a little worried that you appear to have a multithreaded program with no locks here, but nevermind.
As far as I can see, our client code doesn't share data with LibVNCServer, it only calls its services. Therefore, I don't think locking is necessary, unless the LibVNCServer API requires it. And that would be odd, wouldn't it? I can't tell for sure, as I'm not familiar with it.
LibVNCServer sucks. This is one of the reasons why. It pulled a bunch of code from Xvnc. Xvnc is completely synchronous so the solution was to stick it in a thread. Of course, there's no locks anywhere.
This is one of the many reasons I wrote a new VNC implementation for QEMU. Borrying that code for the backend would be a good thing.
Quoted 3 lines
Quoted 3 lines
If you ever need to go into this loop there's a problem somewhere. You shouldn't be able to connect things up until the shared area is initialised.
With XenBus, the loop is no longer needed. It was needed before.
Quoted 3 lines
Can't happen because both users (
vncfb.candsdlfb.c) fill it in before they callxenfb_update(). Butxenfb.cdoesn't want to know about that, so it checks.
The idea was for xenfb_* to be a library for writing various front-ends. I still think that's a good idea. Eventually, we'll have gtkfb, qtfb, etc. Being a bit more defensive can never hurt.
Quoted 1 line
Quoted 1 line
No, because the whole function is completely pointless.
No, it's not. If you have queues, and you buffer them properly, you absolutely have to make sure both sides flush the queues regularly.
Quoted 2 lines
It's a skeleton. I can remove it if it bothers you. If we keep it, it should be as correct as we can make it.
If you remove it, you risk causing breakages with future guests.
Regards,
Anthony Liguori