Xen devel - Aug 2006 - [PATCH] 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 8/18/06, Jeremy Katz <katzj@redhat.com> 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@lists.xensource.com
http://lists.xensource.com/xen-devel

"Christian Limpach" <christian.limpach@gmail.com> writes:
> On 8/18/06, Jeremy Katz <katzj@redhat.com> 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
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.
> 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?
>> +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?
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?
> 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?
>>  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?
Can''t see why not, but I''m looking at this code for the first
time,
too.  Want me to try?
>> +}
>
>> +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.
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.
>> +	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.
That''s how I read this code.
>> +}
>> +
>
>> +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?
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.
>> +	/* 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.
>> +	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.
>> +}
>> +
>> +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.
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.
>> +static int __init xenfb_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.
>> +{
> ...
>> +	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.
>> +	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.
>> +		goto error_kfree;
>> +
> ...
>> +
>> +	info->kthread = kthread_run(xenfb_thread, info, "xenfb
thread");
> kthread_run can fail.
Error checking supplied.

Hmm, do we have to stop the kthread?
>> +
>> +	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?
Elaborate, please.
>> +}
>
>> +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?
>> +{
>> +#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.
I considered that, but it wasn''t obvious to me how to make it fit
cleanly to a user-space backend.  Care to advise?
>> +}
>> +
>> +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?
Specified by the protocol.
>> +	}
>> +	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.
>> +	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.
>> +
>> +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.
Works for me then.
>> +		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.
Appears to be a sketch of future work.  I don''t understand it either.
Anthony?
>> +#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?
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.
>> +	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.
Yes.  See discussion of XENFB_TYPE_SET_EVENTS above.
>> +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.
>> +
>> +	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?
>> +
>> +	__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?
See discussion of XENFB_TYPE_SET_EVENTS above.
>> +	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?
Again, the protocol is designed for extendability.  This is a
placeholder for future xenkbd output events.
>> +
>> +/* 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.
>> +
>> +#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?
Placeholder for xenkbd output events.
>> +};
>> +
>> +void xenkbd_resume(void);
>> +
>> +#endif
[Review of backend...]

I''ll reply to this separately.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

"Christian Limpach" <christian.limpach@gmail.com> writes:

[Review of frontend...]

I replied to that separately.
>> 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> ?
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?
>> +#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.
>> +struct xenfb *xenfb_new(void)
>> +{
> ...
>> +
>> +	xenfb->fd = open("/dev/xen/evtchn", O_RDWR);
> xc_evtchn_open, maybe.
Certainly.
>> +	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?
I don''t understand why this is necessary or useful either. 
I''ll
remove it and see what happens.
>> +
>> +	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().
>> +{
>> +	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?
Yes.
>> +}
>
>> +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.
You didn''t ask for memory barriers in similar code in the frontend.
Did you just miss it there, or do I miss something?
>> +	info->in_prod = prod + 1;
>> +	notify.port = xenfb->fbdev_port;
>> +	return ioctl(xenfb->fd, IOCTL_EVTCHN_NOTIFY, &notify);
> xc_evtchn_notify()?
Yes.
>> +}
>> +
>> +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.
Yes, just as in xenfb_fb_event() above.
>> +	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.
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.
>> +{
>> +	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.
Renamed to xenfb_attach_dom().
>> +{
> ...
>> +	}
>> +	xs_daemon_close(xsh);
>> +	xsh = NULL;
>> +
>> +	printf("%d, %d, %d\n", xenfb->fd,
xenfb->fbdev_evtchn, domid);
> Umm?
Debugging leftover, removed.
>> +
>> +	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.
I think I got them all :)
>> +
>> +	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.
I think this is a leftover of the pre-xenstore way to connect things.
I''ll look into getting rid of the initialized flag.
>> +	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.
>> +
>> +		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.
>> +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.
>> +}
>> +
>> +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.
I''m not sure I understand.  Can you give an example of how things can
go wrong?
>> +	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?
Skeleton code.  There are no output events defined, yet.
>> +	}
>> +	/* 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().
>> +	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?
Not with the current frontend.
>> +	}
>> +
>> +	if (write(xenfb->fd, &port, sizeof(port)) != sizeof(port))
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Quoting Markus Armbruster <armbru@redhat.com>:
> > 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.
> * the frame buffer
>
> * the array of mfns for the frame buffer
>
> Anthony, care to explain why?
>
> >> +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?
>
> 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.
> > 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 :-)
> >> +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?
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.
> >> +	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 :-)
> >> +{
> > ...
> >> +	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...
> >> +
> >> +	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?
>
> 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.
> >> +/* 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.
>
> 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.
> >> +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.
> > 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?
> >> +
> >> +	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...
> >> +
> >> +	__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?
Forward compatibility.  There will be more events in the future and preserving
guest ABI compatibility is important...
>
> 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.
> >> +
> >> +/* 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).
> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Quoting Markus Armbruster <armbru@redhat.com>:
> "Christian Limpach" <christian.limpach@gmail.com> writes:
>
> [Review of frontend...]
>
> I replied to that separately.
>
> >> 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> ?
>
> 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.
> >> +
> >> +	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.
> > 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.
> 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.
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.
> > 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.
> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel