On 8/18/06, Jeremy Katz <katzj@xxxxxxxxxx> wrote:
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.
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.
+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.
config XEN_SCRUB_PAGES
bool "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;
+ 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?
+}
+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.
+ 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)
+ 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.
+}
+
+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 */
+ 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?
+ /* FIXME do I need a wmb() here? */
Only if you care about not losing the non-existent XENFB incoming
events.
+ 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?
+}
+
+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.
+static int __init xenfb_probe(void)
It might be nice to break this function up a little.
+{
...
+ info->page->width = 800;
+ info->page->height = 600;
+ info->page->depth = 32;
It might be nice to make these #defines somewhere.
+ 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.
+ goto error_kfree;
+
...
+
+ info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
kthread_run can fail.
+
+ 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?
+}
+void xenfb_resume(void)
This (a) doesn't work (obviously) and (b) isn't called from anywhere.
+{
+#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
+ 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.
+}
+
+module_exit(xenfb_cleanup);
+
+MODULE_LICENSE("GPL");
+static irqreturn_t input_handler(int rq, void *dev_id, struct pt_regs *regs)
+{
...
+ 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?
+ }
+ input_sync(dev->dev);
+ /* FIXME do I need a wmb() here? */
Depends how much you care about overflowing the ring.
+ info->in_cons = cons;
+
+ return IRQ_HANDLED;
+}
+
+static struct xenkbd_device *xenkbd_dev;
+static int xenkbd_irq;
Why are these needed?
+
+int __init xenkbd_init(void)
+{
...
+ ret = xenbus_transaction_end(xbt, 0);
+ if (ret) {
+ if (ret == -EAGAIN)
+ goto again;
+ /* FIXME really retry forever? */
It's what we do everywhere else.
+ goto error_unreg;
+ }
diff -r ec03b24a2d83 -r 6ca424e1867e linux-2.6-xen-sparse/include/linux/xenfb.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux-2.6-xen-sparse/include/linux/xenfb.h Fri Aug 18 16:17:58
2006 -0400
This seems to define the actual frontend<->backend protocol, so
probably belongs in xen/include/public/io.
+
+/* 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.
+#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 */
+ __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?
+ 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.
+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?
+
+ unsigned long pd[2];
This could do with a better name. Also, pd[1] appears to always be 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?
+ union xenfb_out_event out[XENFB_OUT_RING_SIZE];
+};
+
+void xenfb_resume(void);
+
+#endif
diff -r ec03b24a2d83 -r 6ca424e1867e linux-2.6-xen-sparse/include/linux/xenkbd.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux-2.6-xen-sparse/include/linux/xenkbd.h Fri Aug 18 16:17:58
2006 -0400
Again, belongs in public/io.
@@ -0,0 +1,82 @@
+/* out events */
+
+union xenkbd_out_event
+{
+ __u8 type;
+ char _[40];
+};
Eh?
+
+/* shared page */
+
+#define XENKBD_IN_RING_SIZE (2048 / 40)
+#define XENKBD_OUT_RING_SIZE (1024 / 40)
Why 2048 and 1024?
+
+#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?
+};
+
+void xenkbd_resume(void);
+
+#endif
diff -r 6ca424e1867e -r 103a9f38f17f tools/xenfb/vncfb.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/xenfb/vncfb.c Fri Aug 18 16:20:27 2006 -0400
@@ -0,0 +1,154 @@
+#if 0
+#define BUG() do { printf("%d\n", __LINE__); return 1; } while(0)
+#else
+extern void abort();
#include <stdlib.h> ?
+#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.
+struct xenfb *xenfb_new(void)
+{
...
+
+ xenfb->fd = open("/dev/xen/evtchn", O_RDWR);
xc_evtchn_open, maybe.
+ if (xenfb->fd == -1) {
...
+}
+
+int xenfb_get_fileno(struct xenfb *xenfb_pub)
+{
+ struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+ if (xenfb->domid == -1)
+ return -1;
Eh?
+
+ return xenfb->fd;
+}
+
+static void xenfb_unset_domid(struct xenfb_private *xenfb)
This does a fair bit more than unset the domid.
+{
+ 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?
+}
+static int xenfb_fb_event(struct xenfb_private *xenfb, union xenfb_in_event
*event)
+{
+ uint32_t prod;
+ 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.
+ info->in_prod = prod + 1;
+ notify.port = xenfb->fbdev_port;
+ return ioctl(xenfb->fd, IOCTL_EVTCHN_NOTIFY, ¬ify);
xc_evtchn_notify()?
+}
+
+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;
+
+ 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.
+ 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;
+ 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.
+{
+ 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);
+ 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.
+{
...
+ }
+ xs_daemon_close(xsh);
+ xsh = NULL;
+
+ printf("%d, %d, %d\n", xenfb->fd, xenfb->fbdev_evtchn, domid);
Umm?
+
+ 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.
+
+ 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.
+ 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?
+
+ 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?
+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?
+}
+
+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) {
+ 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.
+ 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 */
+ 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?
+ }
+ /* FIXME do I need a wmb() here? */
No, because the whole function is completely pointless.
+ 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;
+
+ 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?
+ }
+
+ if (write(xenfb->fd, &port, sizeof(port)) != sizeof(port))
+ return -1;
+
+ return 0;
+}
+
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
| 
Home