Re: [Xen-devel] [PATCH] Paravirt framebuffer series [0/6]

["Christian Limpach" <christian.limpach@xxxxxxxxx>]

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, &notify);
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, &notify);
> +}
> +
> +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