Xen devel - Sep 2006 - [PATCH] Paravirt framebuffer frontend kernel support [1/5]

Initial patch from Anthony Liguori, ported to xenstore and current Xen
by Markus Armbruster.  Further improvements since the last submission
* Don''t set up on dom0 or if our domain isn''t setup for a
graphics
  console (katzj)
* Compute framebuffer size from screen dimension (armbru)
* Implement XENFB_TYPE_SET_EVENTS (armbru)
* Future-proof layout of shared page (armbru)
* Memory barriers (armbru)
* Coaelsce notifies: just one batch of events instead of one per event
(armbru)
* Error handling fixes (armbru)

Signed-off-by: Jeremy Katz <katzj@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>


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

Okay, this fixes a lot of the things I complained about last time, and
most of the new problems should be fairly easy to fix up.

There are still a couple of things from the last round which are
unfixed.  This is mostly my fault for going on holiday at an
inconvenient time; sorry about that.

The big problem with this is that you don''t support shadow translate
mode guests, because the frontend exposes mfns directly to the
backend.  The correct way of fixing this is to use grant tables, but,
as Anthony pointed out, that''s quite hard due to the amount of memory
you need to grant access to.

The easy way of doing this would be to just increase the size of the
grant tables.  Alternatively, you could add support for granting
access to ranges of memory, but that sounds like a whole lot of work.

You could also put in a quick hack of just translating the mfns in the
backend if the frontend is in shadow translate mode, but that''s not
really very nice.

It''d also be nice if the protocol supported putting the backend in an
unprivileged domain, which grant tables would give you for free, but
that''s a lower priority.


Aside from that, the lack of support for multiple framebuffers in one
guest is kind of annoying.  It''s not necessary to have this working in
a first cut of the patch, but it''d be good if the protocol included
some way of adding it in later, which the current one doesn''t.

Hard coding the resolution is also kind of icky, but is again
acceptable for a first cut.  It''d be good to have at least a plan for
how this is supposed to work before the thing gets merged, including
how to change the resolution of a live framebuffer.

Also, the spec you pointed me at last time appears to be out of date:
it still thinks you get the pgdir mfn from start info, which certainly
isn''t acceptable for new drivers.
> Initial patch from Anthony Liguori, ported to xenstore and current Xen
> by Markus Armbruster.  Further improvements since the last submission
> * Don''t set up on dom0 or if our domain isn''t setup for a
graphics
>   console (katzj)
> * Compute framebuffer size from screen dimension (armbru)
> * Implement XENFB_TYPE_SET_EVENTS (armbru)
> * Future-proof layout of shared page (armbru)
> * Memory barriers (armbru)
> * Coaelsce notifies: just one batch of events instead of one per event
> (armbru)
> * Error handling fixes (armbru)
> 
> Signed-off-by: Jeremy Katz <katzj@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c
> --- a/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep 02 12:11:54
2006 +0100
> +++ b/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep 02 15:11:17
2006 -0400
> @@ -1866,8 +1866,12 @@ void __init setup_arch(char **cmdline_p)
>  #endif
>  #endif
>  	} else {
> +#if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
> +		conswitchp = &dummy_con;
> +#else
>  		extern int console_use_vt;
>  		console_use_vt = 0;
> +#endif
>  	}
>  }
Not quite sure I understand what this is trying to achieve, or why it''s
related to the PV framebuffer stuff.
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/Kconfig
> --- a/linux-2.6-xen-sparse/drivers/xen/Kconfig	Sat Sep 02 12:11:54 2006
+0100
> +++ b/linux-2.6-xen-sparse/drivers/xen/Kconfig	Sat Sep 02 15:11:17 2006
-0400
> @@ -172,6 +172,29 @@ config XEN_NETDEV_FRONTEND
>  	  dedicated device-driver domain, or your master control domain
>  	  (domain 0), then you almost certainly want to say Y here.
>  
> +config XEN_FRAMEBUFFER
> +	tristate "Framebuffer-device frontend driver"
> +	depends on XEN && FB
> +	select FB_CFB_FILLRECT
> +	select FB_CFB_COPYAREA
> +	select FB_CFB_IMAGEBLIT
> +	default y
> +	help
> +	  The framebuffer-device frontend drivers allows the kernel to create a
> +	  virtual framebuffer.  This framebuffer can be viewed in another
> +	  domain.  Unless this domain has access to a real video card, you
> +	  probably want to say Y here.
> +
> +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.
> +
From last time:
> > > 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?
> > you shed some light on why you created two separate configs?
> They are two separate devices.  Seemed like a natural separation to me.
Perhaps XEN_KEYBOARD should depend on XEN_FRAMEBUFFER, given that it
doesn''t work without it.
>  config XEN_SCRUB_PAGES
>  	bool "Scrub memory before freeing it to Xen"
>  	default y
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02 12:11:54 2006
+0100
> +++ b/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02 15:11:17 2006
-0400
> @@ -15,3 +15,5 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= net
>  obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
>  obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= pciback/
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND)	+= pcifront/
> +obj-$(CONFIG_XEN_FRAMEBUFFER)		+= xenfb/
> +obj-$(CONFIG_XEN_KEYBOARD)		+= xenkbd/
> diff -r 5fa9b746d24f -r 510c6bceb87f linux-2.6-xen-sparse/mm/memory.c
> --- a/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 12:11:54 2006 +0100
> +++ b/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 15:11:17 2006 -0400
> @@ -881,6 +881,7 @@ unsigned long zap_page_range(struct vm_a
>  		tlb_finish_mmu(tlb, address, end);
>  	return end;
>  }
> +EXPORT_SYMBOL(zap_page_range);
It''s unfortunate that we have to modify an upstream file here, but I
can''t really see any alternative.
>  
>  /*
>   * Do a quick page-table lookup for a single page.
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile	Sat Sep 02 15:11:17
2006 -0400
> @@ -0,0 +1,1 @@
> +obj-$(CONFIG_XEN_FRAMEBUFFER)	:= xenfb.o
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c	Sat Sep 02 15:11:17
2006 -0400
> @@ -0,0 +1,571 @@
> +/*
> + * linux/drivers/video/xenfb.c -- Xen para-virtual frame buffer device
> + *
> + * Copyright (C) 2005-2006
> + *
> + *      Anthony Liguori <aliguori@us.ibm.com>
> + *
> + *  Based on linux/drivers/video/q40fb.c
> + *
> + *  This file is subject to the terms and conditions of the GNU General
Public
> + *  License. See the file COPYING in the main directory of this archive
for
> + *  more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/module.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <asm/hypervisor.h>
> +#include <xen/evtchn.h>
> +#include <xen/xenbus.h>
> +#include <linux/xenfb.h>
> +#include <linux/kthread.h>
> +
> +#define XENFB_WIDTH 800
> +#define XENFB_HEIGHT 600
> +#define XENFB_DEPTH 32
> +
> +static int xenfb_fps = 20;
> +static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT *
XENFB_DEPTH / 8;
> +
> +struct xenfb_mapping
> +{
> +	struct list_head	next;
Perhaps not the best name for a list head?
> +	struct vm_area_struct	*vma;
> +	atomic_t		map_refs;
> +	int			faults;
> +	struct xenfb_info	*info;
> +};
> +
> +struct xenfb_info
> +{
> +	struct task_struct		*kthread;
> +	wait_queue_head_t		wq;
> +
> +	unsigned char			*fb;
Why unsigned char?
> +	struct fb_fix_screeninfo	*fix;
> +	struct fb_var_screeninfo	*var;
Not quite sure what these are for.  Why not just get at them through
the fb_info pointer?
> +	struct fb_info			*fb_info;
> +	struct timer_list		refresh;
> +	int				dirty;
> +	int				y1, y2;
> +	int				x1, x2;
These could perhaps do with slightly more descriptive names?
> +
> +	struct semaphore		mm_lock;
That''s going to confuse people.

(Me, for a start)
> +	int				nr_pages;
> +	struct page			**pages;
> +	struct list_head		mappings;
> +
> +	unsigned			evtchn;
> +	int				irq;
> +	struct xenfb_page		*page;
> +	unsigned long 			*mfns;
> +	u32				flags;
Maybe a comment to say what sort of flags go in here?
> +};
> +
> +static void xenfb_do_update(struct xenfb_info *info,
> +			    int x, int y, int w, int h)
> +{
> +	union xenfb_out_event event;
> +	__u32 prod;
> +
> +	event.type = XENFB_TYPE_UPDATE;
> +	event.update.x = x;
> +	event.update.y = y;
> +	event.update.width = w;
> +	event.update.height = h;
> +
> +	prod = info->page->out_prod;
> +	if (prod - info->page->out_cons == XENFB_OUT_RING_LEN)
> +		return;		/* ring buffer full, event lost */
It seems to me like if we lose the event, we probably shouldn''t mark
the framebuffer as clean and throw away the update region.  Perhaps
the caller should instead reschedule the timer?

Also, is there any reason the test couldn''t use xenfb_queue_full?
> +	mb();			/* ensure ring space available */
> +	XENFB_OUT_RING_REF(info->page, prod) = event;
> +	wmb();			/* ensure ring contents visible */
> +	info->page->out_prod = prod + 1;
> +
> +	notify_remote_via_evtchn(info->evtchn);
> +}
> +
> +static int xenfb_queue_full(struct xenfb_info *info)
> +{
> +	__u32 cons, prod;
> +
> +	prod = info->page->out_prod;
> +	cons = info->page->out_cons;
> +	return prod - cons == XENFB_OUT_RING_LEN;
> +}
> +
> +static void xenfb_update_screen(struct xenfb_info *info)
> +{
> +	int y1, y2, x1, x2;
> +	struct list_head *item;
> +	struct xenfb_mapping *map;
> +
> +	if (!(info->flags & XENFB_FLAG_UPDATE))
> +		return;
> +	if (xenfb_queue_full(info))
> +		return;
> +
> +	y1 = info->y1;
> +	y2 = info->y2;
> +	x1 = info->x1;
> +	x2 = info->x2;
> +	info->y1 = info->y2 = info->x1 = info->x2 = 0;
> +	down(&info->mm_lock);
> +	list_for_each(item, &info->mappings) {
> +		map = list_entry(item, struct xenfb_mapping, next);
list_for_each_entry, perhaps?
> +		if (!map->faults)
> +			continue;
> +		zap_page_range(map->vma, map->vma->vm_start,
> +			       map->vma->vm_end - map->vma->vm_start, NULL);
> +		map->faults = 0;
> +	}
> +	up(&info->mm_lock);
> +
> +	xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
> +}
> +
> +static int xenfb_thread(void *data)
> +{
> +	struct xenfb_info *info = data;
> +
> +	for (;;) {
> +		if (kthread_should_stop())
> +			break;
> +		if (info->dirty) {
> +			info->dirty = 0;
> +			xenfb_update_screen(info);
> +		}
> +		wait_event_interruptible(info->wq,
> +			kthread_should_stop() || info->dirty);
> +	}
> +	return 0;
> +}
> +
> +static int xenfb_setcolreg(unsigned regno, unsigned red, unsigned green,
> +			   unsigned blue, unsigned transp,
> +			   struct fb_info *info)
> +{
> +	u32 v;
> +
> +	if (regno > info->cmap.len)
> +		return 1;
> +
> +	red   >>= (16 - info->var.red.length);
> +	green >>= (16 - info->var.green.length);
> +	blue  >>= (16 - info->var.blue.length);
> +
> +	v = (red << info->var.red.offset) |
> +	    (green << info->var.green.offset) |
> +	    (blue << info->var.blue.offset);
> +
> +	switch (info->var.bits_per_pixel) {
> +	case 16:
> +	case 24:
> +	case 32:
> +		((u32 *)info->pseudo_palette)[regno] = v;
> +		break;
> +	}
That looks exciting.  What is this code trying to achieve?

There are comments in xxxfb_setcolreg in skeletonfb to the effect that
pseudo_palette should only get played with if fix.visual is TRUECOLOR
or DIRECTCOLOR, and a specific warning against trying to do stuff with
bits_per_pixel here.  
> +	
> +	return 0;
> +}
> +
> +static void xenfb_timer(unsigned long data)
> +{
> +	struct xenfb_info *info = (struct xenfb_info *)data;
> +	info->dirty = 1;
> +	wake_up(&info->wq);
> +}
From last time:
> > 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.
It looks to me like we can lose updates to the screen in that case:

xenfb_timer runs, sets dirty to 1, kicks the thread
                                        xenfb_thread wakes up
                                        xenfb_thread reads the update region
another update happens, extends the update region, reschedules xenfb_timer
xenfb_timer runs again, sets dirty to 2, kicks the thread again
                                        xenfb_thread sets dirty to 0
                                        xenfb_thread sends an update using
                                                the old region

I suppose the race is sufficiently unlikely and sufficiently mild that
it can probably be ignored for all realistic purposes.
> +
> +static void xenfb_refresh(struct xenfb_info *info,
> +			  int x1, int y1, int w, int h)
Is there any reason why there can''t be multiple calls to this function
live at the same time?  In particular, if vm_nopage and xenfb_copyarea
happen at the same time it looks like we can lose part of the update
area.

Also, vm_nopage holds the mm_lock when it calls this, whereas the
other callers appear not to.  Was this deliberate?
> +{
> +	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;
> +	}
Presumably, these are just looking to see if there''s a pre-existing
region, and if not copying the new region across verbatim?  Could you
test dirty instead in that case?
> +
> +	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);
Would it be sensible to have a fallback so that an update is always
sent to the backend e.g. 100ms after any change to the framebuffer, so
that the user still sees something when the display is being updated
very frequently?
> +}
> +
> +static void xenfb_fillrect(struct fb_info *p, const struct fb_fillrect
*rect)
> +{
> +	struct xenfb_info *info = p->par;
> +
> +	cfb_fillrect(p, rect);
> +	xenfb_refresh(info, rect->dx, rect->dy, rect->width,
rect->height);
> +}
> +
> +static void xenfb_imageblit(struct fb_info *p, const struct fb_image
*image)
> +{
> +	struct xenfb_info *info = p->par;
> +
> +	cfb_imageblit(p, image);
> +	xenfb_refresh(info, image->dx, image->dy, image->width,
image->height);
> +}
> +
> +static void xenfb_copyarea(struct fb_info *p, const struct fb_copyarea
*area)
> +{
> +	struct xenfb_info *info = p->par;
> +
> +	cfb_copyarea(p, area);
> +	xenfb_refresh(info, area->dx, area->dy, area->width,
area->height);
> +}
> +
> +static void xenfb_vm_open(struct vm_area_struct *vma)
> +{
> +	struct xenfb_mapping *map = vma->vm_private_data;
> +	atomic_inc(&map->map_refs);
> +}
> +
> +static void xenfb_vm_close(struct vm_area_struct *vma)
> +{
> +	struct xenfb_mapping *map = vma->vm_private_data;
> +	struct xenfb_info *info = map->info;
> +
> +	down(&info->mm_lock);
> +	if (atomic_dec_and_test(&map->map_refs)) {
> +		list_del(&map->next);
> +		kfree(map);
> +	}
> +	up(&info->mm_lock);
> +}
> +
> +static struct page *xenfb_vm_nopage(struct vm_area_struct *vma,
> +				    unsigned long vaddr, int *type)
> +{
> +	struct xenfb_mapping *map = vma->vm_private_data;
> +	struct xenfb_info *info = map->info;
> +	int pgnr = (vaddr - vma->vm_start) >> PAGE_SHIFT;
> +	struct page *page;
> +	int y1, y2;
> +
> +	if (pgnr >= info->nr_pages)
> +		return NOPAGE_SIGBUS;
> +
> +	down(&info->mm_lock);
> +	page = info->pages[pgnr];
> +	get_page(page);
> +	map->faults++;
> +
> +	y1 = pgnr * PAGE_SIZE / info->fix->line_length;
> +	y2 = (pgnr * PAGE_SIZE + PAGE_SIZE - 1) / info->fix->line_length;
> +	if (y2 > info->var->yres)
> +		y2 = info->var->yres;
> +	xenfb_refresh(info, 0, y1, info->var->xres, y2 - y1);
> +	up(&info->mm_lock);
> +
> +	if (type)
> +		*type = VM_FAULT_MINOR;
> +
> +	return page;
> +}
> +
> +static struct vm_operations_struct xenfb_vm_ops = {
> +	.open	= xenfb_vm_open,
> +	.close	= xenfb_vm_close,
> +	.nopage	= xenfb_vm_nopage,
> +};
> +
> +static int xenfb_mmap(struct fb_info *fb_info, struct vm_area_struct *vma)
> +{
> +	struct xenfb_info *info = fb_info->par;
> +	struct xenfb_mapping *map;
> +	int ret;
> +	int map_pages;
> +
> +	down(&info->mm_lock);
> +
> +	ret = -EINVAL;
> +	if (!(vma->vm_flags & VM_WRITE))
> +		goto out;
> +	if (!(vma->vm_flags & VM_SHARED))
> +		goto out;
> +	if (vma->vm_pgoff != 0)
> +		goto out;
> +
> +	map_pages = (vma->vm_end - vma->vm_start + PAGE_SIZE-1) >>
PAGE_SHIFT;
> +	if (map_pages > info->nr_pages)
> +		goto out;
> +
> +	ret = -ENOMEM;
> +	map = kmalloc(sizeof(*map), GFP_KERNEL);
> +	if (map == NULL)
> +		goto out;
> +	memset(map, 0, sizeof(*map));
kzalloc, maybe?
> +
> +	map->vma = vma;
> +	map->faults = 0;
> +	map->info = info;
> +	atomic_set(&map->map_refs, 1);
> +	list_add(&map->next, &info->mappings);
> +	vma->vm_ops = &xenfb_vm_ops;
> +	vma->vm_flags |= (VM_DONTEXPAND | VM_RESERVED);
> +	vma->vm_private_data = map;
> +	ret = 0;
> +
> + out:
> +	up(&info->mm_lock);
> +	return ret;
> +}
> +
> +static struct fb_ops xenfb_fb_ops = {
> +	.owner		= THIS_MODULE,
> +	.fb_setcolreg	= xenfb_setcolreg,
> +	.fb_fillrect	= xenfb_fillrect,
> +	.fb_copyarea	= xenfb_copyarea,
> +	.fb_imageblit	= xenfb_imageblit,
> +	.fb_mmap	= xenfb_mmap,
> +};
> +
> +static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
> +				       struct pt_regs *regs)
> +{
> +	struct xenfb_info *info = dev_id;
> +	__u32 cons, prod;
> +
> +	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_IN_RING_REF(info->page, cons);
> +
> +		switch (event->type) {
> +		case XENFB_TYPE_SET_EVENTS:
> +			info->flags = event->set_events.flags;
> +			break;
> +		}
> +	}
> +	mb();			/* ensure we''re done with ring contents */
> +	info->page->in_cons = cons;
> +	notify_remote_via_evtchn(info->evtchn);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static unsigned long vmalloc_to_mfn(void *address)
> +{
> +	// FIXME was return pfn_to_mfn(vmalloc_to_pfn(address));
Not quite sure why this comment is here.
> +	return arbitrary_virt_to_machine(address) >> PAGE_SHIFT;
> +}
> +
> +static struct xenfb_info *xenfb_info;
> +
> +static int __init xenfb_probe(void)
I''d find it easier to be confident of the error handling here if the
function were split up a little.
> +{
> +	struct xenfb_info *info;
> +	int i, ret;
> +	struct fb_info *fb_info;
> +	struct evtchn_alloc_unbound alloc_unbound;
> +	struct evtchn_close close;
> +	struct xenbus_transaction xbt;
> +
> +        /* Nothing to do if running in dom0. */
> +        if (is_initial_xendomain())
> +		return -ENODEV;
> +#if 0
> +        /* if we''re not set up to use graphics mode, then
don''t initialize */
> +	if (xenbus_scanf(XBT_NIL, "console", "use_graphics",
"%d", &ret) < 0)
> +		return -ENODEV;
> +	if (ret == 0)
> +		return -ENODEV;
> +#endif
That''s interesting.  What is this here for?
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (info == NULL)
> +		return -ENOMEM;
> +	memset(info, 0, sizeof(*info));
> +
> +	INIT_LIST_HEAD(&info->mappings);
> +
> +	info->fb = vmalloc(xenfb_mem_len);
> +	if (info->fb == NULL)
> +		goto error;
> +	memset(info->fb, 0, xenfb_mem_len);
> +	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
> +	info->pages = kmalloc(sizeof(struct page*)*info->nr_pages,
GFP_KERNEL);
> +	if (info->pages == NULL)
> +		goto error_vfree;
> +	for (i = 0; i < info->nr_pages; i++)
> +		info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);
> +
> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
> +	// FIXME sizeof(struct xenfb_info)
Where did sizeof(u32) * 256 come from?  What does the comment mean?

fb_info isn''t released from the error path.
> +	if (fb_info == NULL)
> +		goto error_kfree;
> +
> +	info->mfns = vmalloc(sizeof(unsigned long) * info->nr_pages);
What happens if this fails?  Also, you never seem to vfree this if we
hit an error later on.
> +	/* set up shared page */
> +	info->page = (void *)__get_free_page(GFP_KERNEL);
> +	if (!info->page)
> +		goto error_kfree;
> +	/* set up event channel */
> +	alloc_unbound.dom = DOMID_SELF;
> +	alloc_unbound.remote_dom = 0;
> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
> +					  &alloc_unbound);
> +	if (ret)
> +		goto error_freep;
> +	info->evtchn = alloc_unbound.port;
> +
> +	for (i = 0; i < info->nr_pages; i++)
> +		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
> +	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
What happens if this domain is running in shadow translate mode?  It
looks to me like the backend will end up mapping completely the wrong
frame.
> +	info->page->pd[1] = 0;
> +	info->page->width = XENFB_WIDTH;
> +	info->page->height = XENFB_HEIGHT;
> +	info->page->depth = XENFB_DEPTH;
> +	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) {
> +		close.port = info->evtchn;
> +		HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
> +		goto error_freep;
> +	}
> +
> +	info->irq = ret;
> +	xenfb_info = info;
> +
> +	fb_info->pseudo_palette = fb_info->par;
> +	fb_info->par = info;
> +	fb_info->screen_base = info->fb;
> +
> +	memset(&fb_info->var, 0, sizeof(fb_info->var));
> +	memset(&fb_info->fix, 0, sizeof(fb_info->fix));
I think these are already guaranteed to be zero, aren''t they?
> +
> +	fb_info->fbops = &xenfb_fb_ops;
> +	fb_info->var.xres_virtual = fb_info->var.xres =
info->page->width;
> +	fb_info->var.yres_virtual = fb_info->var.yres =
info->page->height;
> +	fb_info->var.bits_per_pixel = info->page->depth;
> +
> +	fb_info->var.red = (struct fb_bitfield){16, 8, 0};
> +	fb_info->var.green = (struct fb_bitfield){8, 8, 0};
> +	fb_info->var.blue = (struct fb_bitfield){0, 8, 0};
> +
> +	fb_info->var.activate = FB_ACTIVATE_NOW;
> +	fb_info->var.height = -1;
> +	fb_info->var.width = -1;
> +	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
> +
> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
> +	fb_info->fix.line_length = info->page->line_length;
> +	fb_info->fix.smem_start = 0;
> +	fb_info->fix.smem_len = xenfb_mem_len;
> +	strcpy(fb_info->fix.id, "xen");
> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
> +	fb_info->fix.accel = FB_ACCEL_NONE;
> +
> +	fb_info->flags = FBINFO_FLAG_DEFAULT;
> +
> +	fb_alloc_cmap(&fb_info->cmap, 256, 0);
This can fail, and the error paths don''t bother to deallocate it.
> +
> +	info->fb_info = fb_info;
> +	info->fix = &fb_info->fix;
> +	info->var = &fb_info->var;
> +
> +	init_MUTEX(&info->mm_lock);
> +	init_waitqueue_head(&info->wq);
> +	init_timer(&info->refresh);
> +	info->refresh.function = xenfb_timer;
> +	info->refresh.data = (unsigned long)info;
> +
> +	info->kthread = kthread_run(xenfb_thread, info, "xenfb
thread");
> +	if (IS_ERR(info->kthread))
> +		goto error_unbind;
> +
> +	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));
> +	// FIXME grant tables?
Again, what happens in shadow translate mode?

Also, it''s kind of annoying that there''s no support for
multiple
framebuffers in a domU.  That''s very much a wishlist thing, though.
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_printf(xbt, "vfb", "event-channel",
"%u",
> +			    info->evtchn);
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_transaction_end(xbt, 0);
> +	if (ret) {
> +		if (ret == -EAGAIN)
> +			goto again;
> +		goto error_unreg;
> +	}
> +
> +	return 0;
> +
> + error_xenbus:
> +	xenbus_transaction_end(xbt, 1);
> + error_unreg:
> +	unregister_framebuffer(fb_info);
> + error_unbind:
> +	unbind_from_irqhandler(info->irq, info);
> +	// FIXME do we have to stop info->kthread?
If you''ve started it you certainly need to stop it.
> + error_freep:
> +	free_page((unsigned long)info->page);
> + error_kfree:
> +	kfree(info->pages);
> + error_vfree:
> +	vfree(info->fb);
> + error:
> +	kfree(info);
> +	xenfb_info = NULL;
> +
> +	return -ENODEV;
> +}
> +
> +static int __init xenfb_init(void)
> +{
> +	return xenfb_probe();
From last time:
> > 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?
I think if the frontend needs to know whether the backend is in kernel
or userspace then we''ve messed up somewhere.  Why do you expect this
to be difficult?

> +}
> +
> +static void __exit xenfb_cleanup(void)
> +{
> +	struct xenfb_info *info = xenfb_info;
> +
> +	unregister_framebuffer(info->fb_info);
> +	unbind_from_irqhandler(info->irq, info);
> +	free_page((unsigned long)info->page);
> +	kfree(info->pages);
> +	vfree(info->fb);
> +	kfree(info);
> +	xenfb_info = NULL;
I don''t think this releases all of the memory you allocate in
xenfb_probe.
> +}
> +
> +module_init(xenfb_init);
> +module_exit(xenfb_cleanup);
> +
> +MODULE_LICENSE("GPL");
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenkbd/Makefile
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/xenkbd/Makefile	Sat Sep 02 15:11:17
2006 -0400
> @@ -0,0 +1,1 @@
> +obj-$(CONFIG_XEN_KEYBOARD)	+= xenkbd.o
> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenkbd/xenkbd.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/xenkbd/xenkbd.c	Sat Sep 02 15:11:17
2006 -0400
> @@ -0,0 +1,178 @@
> +/*
> + * linux/drivers/input/keyboard/xenkbd.c -- Xen para-virtual input device
> + *
> + * Copyright (C) 2005
> + *
> + *      Anthony Liguori <aliguori@us.ibm.com>
> + *
> + *  Based on linux/drivers/input/mouse/sermouse.c
> + *
> + *  This file is subject to the terms and conditions of the GNU General
Public
> + *  License. See the file COPYING in the main directory of this archive
for
> + *  more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <asm/hypervisor.h>
> +#include <xen/evtchn.h>
> +#include <xen/xenbus.h>
> +#include <linux/xenkbd.h>
> +
> +struct xenkbd_device
> +{
> +	struct input_dev *dev;
> +	struct xenkbd_info *info;
> +	unsigned evtchn;
> +	int irq;
> +};
> +
> +static irqreturn_t input_handler(int rq, void *dev_id, struct pt_regs
*regs)
> +{
> +	struct xenkbd_device *dev = dev_id;
> +	struct xenkbd_info *info = dev ? dev->info : 0;
> +	static int button_map[3] = { BTN_RIGHT, BTN_MIDDLE, BTN_LEFT };
> +	__u32 cons, prod;
> +
> +	prod = info->in_prod;
> +	rmb();			/* ensure we see ring contents up to prod */
> +	for (cons = info->in_cons; cons != prod; cons++) {
> +		union xenkbd_in_event *event;
> +		event = &XENKBD_IN_RING_REF(info, cons);
> +	
> +		switch (event->type) {
> +		case XENKBD_TYPE_MOTION:
> +			input_report_rel(dev->dev, REL_X, event->motion.rel_x);
> +			input_report_rel(dev->dev, REL_Y, event->motion.rel_y);
> +			break;
> +		case XENKBD_TYPE_BUTTON:
> +			if (event->button.button < 3)
> +				input_report_key(dev->dev,
> +						 button_map[event->button.button],
> +						 event->button.pressed);
> +			break;
> +		case XENKBD_TYPE_KEY:
> +			input_report_key(dev->dev, event->key.keycode,
event->key.pressed);
> +			break;
> +		}
> +	}
> +	input_sync(dev->dev);
> +	mb();			/* ensure we got ring contents */
> +	info->in_cons = cons;
> +	notify_remote_via_evtchn(dev->evtchn);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct xenkbd_device *xenkbd_dev;
> +
> +int __init xenkbd_init(void)
> +{
> +	int ret = 0;
> +	int i;
> +	struct xenkbd_device *dev;
> +	struct input_dev *input_dev;
> +	struct evtchn_alloc_unbound alloc_unbound;
> +	struct xenbus_transaction xbt;
> +
> +        /* Nothing to do if running in dom0. */
> +        if (is_initial_xendomain())
> +	  return -ENODEV;
> +#if 0
> +        /* if we''re not set up to use graphics mode, then
don''t initialize */
> +	if (xenbus_scanf(XBT_NIL, "console", "use_graphics",
"%d", &ret) < 0)
> +	  return -ENODEV;
> +	if (ret == 0)
> +	  return -ENODEV;
> +#endif
Not sure about this.
> +
> +	dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +	if (!dev || !input_dev)
> +		return -ENOMEM;
You potentially leak memory if one of these fails and the other succeeds.

Also, the error paths don''t release input_dev.  In fact, nothing ever
releases input_dev, unless I''m missing something.
> +
> +	dev->dev = input_dev;
> +	dev->info = (void *)__get_free_page(GFP_KERNEL);
> +	if (!dev->info) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	alloc_unbound.dom = DOMID_SELF;
> +	alloc_unbound.remote_dom = 0;
> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
> +					  &alloc_unbound);
> +	if (ret)
> +		goto error_freep;
> +	dev->evtchn = alloc_unbound.port;
> +	ret = bind_evtchn_to_irqhandler(dev->evtchn, input_handler, 0,
> +					"xenkbd", dev);
> +	if (ret < 0)
> +		goto error_freep;
You might want to close the event channel here.
> +	dev->irq = ret;
> +
> +	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL);
> +	input_dev->keybit[LONG(BTN_MOUSE)] = BIT(BTN_LEFT) | BIT(BTN_RIGHT);
> +	input_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y);
> +
> +	/* FIXME not sure this is quite right */
> +	for (i = 0; i < 256; i++)
> +		set_bit(i, input_dev->keybit);
It looks reasonable, but perhaps memset would be a better choice.
> +
> +	input_dev->name = "Xen Virtual Keyboard/Mouse";
> +
> +	input_register_device(input_dev);
This can fail.
> +
> + again:
> +	ret = xenbus_transaction_start(&xbt);
> +	if (ret)
> +		goto error_unreg;
> +	ret = xenbus_printf(xbt, "vkbd", "page-ref",
"%lu",
> +			    virt_to_mfn(dev->info));
There''s no reason that I can see not to use grant tables here, and
that''d avoid all of the potential issues with shadow translate mode.
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_printf(xbt, "vkbd", "event-channel",
"%u",
> +			    dev->evtchn);
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_transaction_end(xbt, 0);
> +	if (ret) {
> +		if (ret == -EAGAIN)
> +			goto again;
> +		goto error_unreg;
> +	}
> +
> +	dev->info->in_cons = dev->info->in_prod = 0;
> +	dev->info->out_cons = dev->info->out_prod = 0;
> +	xenkbd_dev = dev;
> +
> +	return ret;
> +
> +	
> + error_xenbus:
> +	xenbus_transaction_end(xbt, 1);
> + error_unreg:
> +	input_unregister_device(input_dev);
> +	unbind_from_irqhandler(dev->irq, dev);
> + error_freep:
> +	free_page((unsigned long)dev->info);
> + error:
> +	kfree(dev);
> +	return ret;
> +}
> +
> +static void __exit xenkbd_cleanup(void)
> +{
> +	input_unregister_device(xenkbd_dev->dev);
> +	unbind_from_irqhandler(xenkbd_dev->irq, xenkbd_dev);
> +	free_page((unsigned long)xenkbd_dev->info);
> +	kfree(xenkbd_dev);
> +	xenkbd_dev = NULL;
> +}
> +
> +module_init(xenkbd_init);
> +module_exit(xenkbd_cleanup);
> +
> +MODULE_LICENSE("GPL");
> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02 15:11:17 2006
-0400
This is defining an io protocol, so belongs in xen/include/public/io
> @@ -0,0 +1,108 @@
> +/*
> + * linux/include/linux/xenfb.h -- Xen virtual frame buffer device
> + *
> + * Copyright (C) 2005
> + *
> + *      Anthony Liguori <aliguori@us.ibm.com>
> + *
> + *  This file is subject to the terms and conditions of the GNU General
Public
> + *  License. See the file COPYING in the main directory of this archive
for
> + *  more details.
> + */
> +
> +#ifndef _LINUX_XENFB_H
> +#define _LINUX_XENFB_H
> +
> +#include <asm/types.h>
> +
> +/* out events */
> +
> +#define XENFB_OUT_EVENT_SIZE 40
> +
> +#define XENFB_TYPE_MOTION 1
> +#define XENFB_TYPE_UPDATE 2
> +
The other tagged unions in Xen look more like this:

struct {
	u8 type;
	union {
		struct {
			u32 field1;
			u32 field2;
		} type1;
		struct {
			u16 field1;
		} type2;
	} u;
};

I''d find this a lot easier to read than the current approach of
putting the type into the individual event type structures.  Is there
any reason why this couldn''t be used here?
> +struct xenfb_motion	    /* currently unused */
> +{
> +	__u8 type;          /* XENFB_TYPE_MOTION */
> +	__u16 x;            /* The new x coordinate */
> +	__u16 y;            /* The new y coordinate */
> +};
I think if the comment said what was moving this''d be a lot clearer.
> +
> +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;
> +	struct xenfb_motion motion;
> +	struct xenfb_update update;
> +	char _[XENFB_OUT_EVENT_SIZE];
Perhaps ``pad'''' would be clearer than ``_''''?
> +};
> +
> +/* in events */
> +
> +#define XENFB_IN_EVENT_SIZE 40
> +
> +#define XENFB_TYPE_SET_EVENTS 1
> +
> +#define XENFB_FLAG_MOTION 1
> +#define XENFB_FLAG_UPDATE 2
> +#define XENFB_FLAG_COPY 4
> +#define XENFB_FLAG_FILL 8
> +
> +struct xenfb_set_events
> +{
> +	__u8 type;          /* XENFB_TYPE_SET_EVENTS */
> +	__u32 flags;        /* combination of XENFB_FLAG_* */
> +};
> +
> +union xenfb_in_event
> +{
> +	__u8 type;
> +	struct xenfb_set_events set_events;
> +	char _[XENFB_OUT_EVENT_SIZE];
> +};
> +
> +/* shared page */
> +
> +#define XENFB_IN_RING_SIZE 1024
> +#define XENFB_IN_RING_LEN (XENFB_IN_RING_SIZE / XENFB_IN_EVENT_SIZE)
> +#define XENFB_IN_RING_OFFS 1024
> +#define XENFB_IN_RING(page) \
> +    ((union xenfb_in_event *)((char *)(page) + XENFB_IN_RING_OFFS))
> +#define XENFB_IN_RING_REF(page, idx) \
> +    (XENFB_IN_RING((page))[(idx) % XENFB_IN_RING_LEN])
> +
> +#define XENFB_OUT_RING_SIZE 2048
> +#define XENFB_OUT_RING_LEN (XENFB_OUT_RING_SIZE / XENFB_OUT_EVENT_SIZE)
> +#define XENFB_OUT_RING_OFFS (XENFB_IN_RING_OFFS + XENFB_IN_RING_SIZE)
> +#define XENFB_OUT_RING(page) \
> +    ((union xenfb_out_event *)((char *)(page) + XENFB_OUT_RING_OFFS))
> +#define XENFB_OUT_RING_REF(page, idx) \
> +    (XENFB_OUT_RING((page))[(idx) % XENFB_OUT_RING_LEN])
> +
> +struct xenfb_page
> +{
> +	__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) */
> +
> +	unsigned long pd[2];	/* FIXME rename to pgdir? */
> +	/* FIXME pd[1] unused at this time, shrink? */
I think it''d be enough to just give some indication of where the magic
number 2 came from.

From last time:
> > 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.
We''re going to eventually need some way for the frontend to tell the
backend to change the display resolution and/or depth.  Could this be
used to select the initial resolution?  I''m not sure what the plan is
here, but I would guess something like this:

1) Framebuffer is operating normally, with the frontend modifying the
        buffer in memory and generating update events in the usual way.

2) Frontend decides it wants a resolution change.  Linux is made to stop
        updating the buffer.  Not quite sure how to do this, but it seems
	like something which most real framebuffers would need to do during
	a resolution change.  The frontend sends a start_change_res
        message to the backend with the desired new resolution.

3) The backend finishes off any pending updates, and picks up the
        start_change_res message.  It can then unmap the buffer and
        send back a change_res_goahead message.

4) The frontend picks up the change_res_goahead message.  It can now
        release the buffer area and allocate a new one of the
        correct size, and send a change_res_complete message to the
        correct size, and send a change_res_complete message to the
        backend.  Once that''s sent the frontend can go back to normal
        operation at the new resolution.

5) The backend picks up the change_res_complete message, remaps
        the buffer, and returns to normal operation.

change_res_complete is actually redundant, but will simplify the
backend enough that it''s probably worthwhile.

If you use a protocol like this, then you could have the frontend send
a start_change_res as the first thing after it starts up, and
communicate the information that way.  This also gives you some scope
for negotiating a lower res if the backend can''t cope with the
requested one for some reason.


The other way would be to do this through xenbus, but synchronising
with the data path would be a bit of a pain.

> +
> +	__u32 in_cons, in_prod;
> +	__u32 out_cons, out_prod;
> +};
> +
> +void xenfb_resume(void);
No longer exists.
> +
> +#endif
> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02 15:11:17 2006
-0400
Again, this belongs in xen/include/public/io.

> @@ -0,0 +1,92 @@
> +/*
> + * linux/include/linux/xenkbd.h -- Xen virtual keyboard/mouse
> + *
> + * Copyright (C) 2005
> + *
> + *      Anthony Liguori <aliguori@us.ibm.com>
> + *
> + *  This file is subject to the terms and conditions of the GNU General
Public
> + *  License. See the file COPYING in the main directory of this archive
for
> + *  more details.
> + */
> +
> +#ifndef _LINUX_XENKBD_H
> +#define _LINUX_XENKBD_H
> +
> +#include <asm/types.h>
> +
> +/* in events */
> +
> +#define XENKBD_IN_EVENT_SIZE 40
> +
> +#define XENKBD_TYPE_MOTION  1     /* mouse movement event */
> +#define XENKBD_TYPE_BUTTON  2     /* mouse button event */
> +#define XENKBD_TYPE_KEY     3     /* keyboard event */
> +
> +struct xenkbd_motion
> +{
> +	__u8 type;         /* XENKBD_TYPE_MOTION */
> +	__s16 rel_x;       /* relative X motion */
> +	__s16 rel_y;       /* relative Y motion */
> +};
Again, this is a slightly ugly way of doing tagged unions.
> +
> +struct xenkbd_button
> +{
> +	__u8 type;         /* XENKBD_TYPE_BUTTON */
> +	__u8 pressed;      /* 1 if pressed; 0 otherwise */
> +	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */
> +};
> +
> +struct xenkbd_key
> +{
> +	__u8 type;         /* XENKBD_TYPE_KEY */
> +	__u8 pressed;      /* 1 if pressed; 0 otherwise */
> +	__u16 keycode;     /* KEY_* from linux/input.h */
> +};
> +
> +union xenkbd_in_event
> +{
> +	__u8 type;
> +	struct xenkbd_motion motion;
> +	struct xenkbd_button button;
> +	struct xenkbd_key key;
> +	char _[XENKBD_IN_EVENT_SIZE];
> +};
> +
> +/* out events */
Maybe a comment that none are currently defined?
> +
> +#define XENKBD_OUT_EVENT_SIZE 40
> +
> +union xenkbd_out_event
> +{
> +	__u8 type;
> +	char _[XENKBD_OUT_EVENT_SIZE];
> +};
> +
> +/* shared page */
> +
> +#define XENKBD_IN_RING_SIZE 2048
> +#define XENKBD_IN_RING_LEN (XENKBD_IN_RING_SIZE / XENKBD_IN_EVENT_SIZE)
> +#define XENKBD_IN_RING_OFFS 1024
> +#define XENKBD_IN_RING(page) \
> +    ((union xenkbd_in_event *)((char *)(page) + XENKBD_IN_RING_OFFS))
> +#define XENKBD_IN_RING_REF(page, idx) \
> +    (XENKBD_IN_RING((page))[(idx) % XENKBD_IN_RING_LEN])
> +
> +#define XENKBD_OUT_RING_SIZE 1024
> +#define XENKBD_OUT_RING_LEN (XENKBD_OUT_RING_SIZE / XENKBD_OUT_EVENT_SIZE)
> +#define XENKBD_OUT_RING_OFFS (XENKBD_IN_RING_OFFS + XENKBD_IN_RING_SIZE)
> +#define XENKBD_OUT_RING(page) \
> +    ((union xenkbd_out_event *)((char *)(page) + XENKBD_OUT_RING_OFFS))
> +#define XENKBD_OUT_RING_REF(page, idx) \
> +    (XENKBD_OUT_RING((page))[(idx) % XENKBD_OUT_RING_LEN])
> +
> +struct xenkbd_info
> +{
> +	__u32 in_cons, in_prod;
> +	__u32 out_cons, out_prod;
> +};
> +
> +void xenkbd_resume(void);
No longer exists.
> +
> +#endif
Steven.


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

On 4/9/06 10:00 am, "Steven Smith" <sos22-xen@srcf.ucam.org>
wrote:
> You could also put in a quick hack of just translating the mfns in the
> backend if the frontend is in shadow translate mode, but that''s
not
> really very nice.
My aim is to have all translation done in Xen if a guest is auto-translated,
even if the mappings are created by a foreign domain (e.g., dom0). Maybe
this is the showstopper that should drive us to make this change.  As noted
already, this would also be a simplification and robustification for the
control libraries (which kludge their own P2M translation with
xc_get_pfn_list).

 -- Keir



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

On Mon, 2006-09-04 at 10:00 +0100, Steven Smith wrote:
> There are still a couple of things from the last round which are
> unfixed.  This is mostly my fault for going on holiday at an
> inconvenient time; sorry about that.
We won''t begrudge you a holiday -- hope it was a good one :-)  Leaving
some of the stuff for Markus to reply to
> The big problem with this is that you don''t support shadow
translate
> mode guests, because the frontend exposes mfns directly to the
> backend.  The correct way of fixing this is to use grant tables, but,
> as Anthony pointed out, that''s quite hard due to the amount of
memory
> you need to grant access to.
> 
> The easy way of doing this would be to just increase the size of the
> grant tables.  Alternatively, you could add support for granting
> access to ranges of memory, but that sounds like a whole lot of work.
> 
> You could also put in a quick hack of just translating the mfns in the
> backend if the frontend is in shadow translate mode, but that''s
not
> really very nice.
I wouldn''t complain if we go the route Keir suggested and have Xen do
it ;-)  But if not, I think that given the size of the memory in
question + grant tables, the quick hack is probably going to be the
"right" thing for this case.
> > diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c
> > --- a/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep 02
12:11:54 2006 +0100
> > +++ b/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep 02
15:11:17 2006 -0400
> > @@ -1866,8 +1866,12 @@ void __init setup_arch(char **cmdline_p)
> >  #endif
> >  #endif
> >  	} else {
> > +#if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
> > +		conswitchp = &dummy_con;
> > +#else
> >  		extern int console_use_vt;
> >  		console_use_vt = 0;
> > +#endif
> >  	}
> >  }
> Not quite sure I understand what this is trying to achieve, or why
it''s
> related to the PV framebuffer stuff.
It makes it so that console switching is achieved by the dummy console
and then when the fbcon driver is loaded, it takes over.  

[snip]
> Perhaps XEN_KEYBOARD should depend on XEN_FRAMEBUFFER, given that it
> doesn''t work without it.
Yeah, seems reasonable enough -- I''ve added to my tree.
> >  config XEN_SCRUB_PAGES
> >  	bool "Scrub memory before freeing it to Xen"
> >  	default y
> > diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/Makefile
> > --- a/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02 12:11:54
2006 +0100
> > +++ b/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02 15:11:17
2006 -0400
> > @@ -15,3 +15,5 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= net
> >  obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
> >  obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= pciback/
> >  obj-$(CONFIG_XEN_PCIDEV_FRONTEND)	+= pcifront/
> > +obj-$(CONFIG_XEN_FRAMEBUFFER)		+= xenfb/
> > +obj-$(CONFIG_XEN_KEYBOARD)		+= xenkbd/
> > diff -r 5fa9b746d24f -r 510c6bceb87f linux-2.6-xen-sparse/mm/memory.c
> > --- a/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 12:11:54 2006 +0100
> > +++ b/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 15:11:17 2006 -0400
> > @@ -881,6 +881,7 @@ unsigned long zap_page_range(struct vm_a
> >  		tlb_finish_mmu(tlb, address, end);
> >  	return end;
> >  }
> > +EXPORT_SYMBOL(zap_page_range);
> It''s unfortunate that we have to modify an upstream file here, but
I
> can''t really see any alternative.
Well, if we don''t allow the framebuffer to be modular then it
doesn''t
have to be exported.  But that also seems like the wrong answer :-)
> > +#if 0
> > +        /* if we''re not set up to use graphics mode, then
don''t initialize */
> > +	if (xenbus_scanf(XBT_NIL, "console",
"use_graphics", "%d", &ret) < 0)
> > +		return -ENODEV;
> > +	if (ret == 0)
> > +		return -ENODEV;
> > +#endif
> That''s interesting.  What is this here for?
It looks like it got accidentally #if 0''d while merging patches.
Whoops.  Basically, if the guest isn''t set up to use a graphical
console, then we shouldn''t initialize the devices.

Jeremy


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

> > There are still a couple of things from the last round which are
> > unfixed.  This is mostly my fault for going on holiday at an
> > inconvenient time; sorry about that.
> We won''t begrudge you a holiday -- hope it was a good one :-)
Yes, thank you.
> > The big problem with this is that you don''t support shadow
translate
> > mode guests, because the frontend exposes mfns directly to the
> > backend.  The correct way of fixing this is to use grant tables, but,
> > as Anthony pointed out, that''s quite hard due to the amount
of memory
> > you need to grant access to.
> > 
> > The easy way of doing this would be to just increase the size of the
> > grant tables.  Alternatively, you could add support for granting
> > access to ranges of memory, but that sounds like a whole lot of work.
> > 
> > You could also put in a quick hack of just translating the mfns in the
> > backend if the frontend is in shadow translate mode, but
that''s not
> > really very nice.
> I wouldn''t complain if we go the route Keir suggested and have Xen
do
> it ;-)  But if not, I think that given the size of the memory in
> question + grant tables, the quick hack is probably going to be the
> "right" thing for this case.
Okay.  Adding translations to the hypervisor isn''t going to happen
until after 3.0.3, so we should probably just leave this for a little
while.
> > > diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c
> > > --- a/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep
02 12:11:54 2006 +0100
> > > +++ b/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep
02 15:11:17 2006 -0400
> > > @@ -1866,8 +1866,12 @@ void __init setup_arch(char **cmdline_p)
> > >  #endif
> > >  #endif
> > >  	} else {
> > > +#if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
> > > +		conswitchp = &dummy_con;
> > > +#else
> > >  		extern int console_use_vt;
> > >  		console_use_vt = 0;
> > > +#endif
> > >  	}
> > >  }
> > Not quite sure I understand what this is trying to achieve, or why
it''s
> > related to the PV framebuffer stuff.
> It makes it so that console switching is achieved by the dummy console
> and then when the fbcon driver is loaded, it takes over.  
Okay.
> 
> [snip]
> > Perhaps XEN_KEYBOARD should depend on XEN_FRAMEBUFFER, given that it
> > doesn''t work without it.
> Yeah, seems reasonable enough -- I''ve added to my tree.
Thanks.
> > >  config XEN_SCRUB_PAGES
> > >  	bool "Scrub memory before freeing it to Xen"
> > >  	default y
> > > diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/Makefile
> > > --- a/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02
12:11:54 2006 +0100
> > > +++ b/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02
15:11:17 2006 -0400
> > > @@ -15,3 +15,5 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= net
> > >  obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
> > >  obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= pciback/
> > >  obj-$(CONFIG_XEN_PCIDEV_FRONTEND)	+= pcifront/
> > > +obj-$(CONFIG_XEN_FRAMEBUFFER)		+= xenfb/
> > > +obj-$(CONFIG_XEN_KEYBOARD)		+= xenkbd/
> > > diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/mm/memory.c
> > > --- a/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 12:11:54 2006
+0100
> > > +++ b/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 15:11:17 2006
-0400
> > > @@ -881,6 +881,7 @@ unsigned long zap_page_range(struct vm_a
> > >  		tlb_finish_mmu(tlb, address, end);
> > >  	return end;
> > >  }
> > > +EXPORT_SYMBOL(zap_page_range);
> > It''s unfortunate that we have to modify an upstream file
here, but I
> > can''t really see any alternative.
> Well, if we don''t allow the framebuffer to be modular then it
doesn''t
> have to be exported.  But that also seems like the wrong answer :-)
Yeah, adding the EXPORT_SYMBOL is the right thing to do here, it''s
just kind of annoying.  Oh well.
> 
> > > +#if 0
> > > +        /* if we''re not set up to use graphics mode,
then don''t initialize */
> > > +	if (xenbus_scanf(XBT_NIL, "console",
"use_graphics", "%d", &ret) < 0)
> > > +		return -ENODEV;
> > > +	if (ret == 0)
> > > +		return -ENODEV;
> > > +#endif
> > That''s interesting.  What is this here for?
> It looks like it got accidentally #if 0''d while merging patches.
> Whoops.  Basically, if the guest isn''t set up to use a graphical
> console, then we shouldn''t initialize the devices.
That seems like a good idea.  Please put this back in.

Steven.


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

On 6/9/06 10:11 am, "Steven Smith" <sos22-xen@srcf.ucam.org>
wrote:
>> I wouldn''t complain if we go the route Keir suggested and have
Xen do
>> it ;-)  But if not, I think that given the size of the memory in
>> question + grant tables, the quick hack is probably going to be the
>> "right" thing for this case.
> Okay.  Adding translations to the hypervisor isn''t going to happen
> until after 3.0.3, so we should probably just leave this for a little
> while.
Another option after 3.0.3 is to allow grant tables to support these large
mappings. I have some ideas how to do this fairly efficiently, given that
it''ll be okay to track mappings to all the pages as an aggregate.
It''s not
going to happen for now though -- it''ll just be a shame if we add
grant-table support later that it''ll change the setup protocol.
However, we
can probably maintain backward compatibility if we think about it a bit.

 -- Keir



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

Steven Smith <sos22-xen@srcf.ucam.org> writes:
> Okay, this fixes a lot of the things I complained about last time, and
> most of the new problems should be fairly easy to fix up.
>
> There are still a couple of things from the last round which are
> unfixed.  This is mostly my fault for going on holiday at an
> inconvenient time; sorry about that.
I''m about to commit the same crime :)  Expect me back on the 19th.
> The big problem with this is that you don''t support shadow
translate
> mode guests, because the frontend exposes mfns directly to the
> backend.  The correct way of fixing this is to use grant tables, but,
> as Anthony pointed out, that''s quite hard due to the amount of
memory
> you need to grant access to.
>
> The easy way of doing this would be to just increase the size of the
> grant tables.  Alternatively, you could add support for granting
> access to ranges of memory, but that sounds like a whole lot of work.
Yes, it would be great if Xen provided the means to do it right, but
as Keir wrote that''s post 3.0.3.
> You could also put in a quick hack of just translating the mfns in the
> backend if the frontend is in shadow translate mode, but that''s
not
> really very nice.
As far as I can see, this is our only option right now.  I''m ignorant
of `shadow translate mode''; can somebody please educate me on how to
detect and translate shadow translate mode frontend mfns in the
backend?
> It''d also be nice if the protocol supported putting the backend in
an
> unprivileged domain, which grant tables would give you for free, but
> that''s a lower priority.
I guess that has to wait until we can do the framebuffer mapping
nicely.
> Aside from that, the lack of support for multiple framebuffers in one
> guest is kind of annoying.  It''s not necessary to have this
working in
> a first cut of the patch, but it''d be good if the protocol
included
> some way of adding it in later, which the current one doesn''t.
Well, the only place that limits us to a single framebuffer is our use
of xenstore, isn''t it?.  All we need for additional framebuffers then
is new xenstore directories, say vfbN and vkbdN, where N is 0, 1, ...

If you feel it''s useful, I can rename the xenstore directories to vfb0
and vkbd0 now.
> Hard coding the resolution is also kind of icky, but is again
> acceptable for a first cut.  It''d be good to have at least a plan
for
> how this is supposed to work before the thing gets merged, including
> how to change the resolution of a live framebuffer.
It''s always good to have a plan for future work.  Note, however, that
the protocol is easily extensible, and therefore we don''t have to fear
coding ourselves into a box now that we have to break later, just
for lack of foresight.
> Also, the spec you pointed me at last time appears to be out of date:
> it still thinks you get the pgdir mfn from start info, which certainly
> isn''t acceptable for new drivers.
Yes, it''s out of date.  Let''s nail down things here, and if we
then
feel the spec is useful to have, we update it.
>> Initial patch from Anthony Liguori, ported to xenstore and current Xen
>> by Markus Armbruster.  Further improvements since the last submission
>> * Don''t set up on dom0 or if our domain isn''t setup
for a graphics
>>   console (katzj)
>> * Compute framebuffer size from screen dimension (armbru)
>> * Implement XENFB_TYPE_SET_EVENTS (armbru)
>> * Future-proof layout of shared page (armbru)
>> * Memory barriers (armbru)
>> * Coaelsce notifies: just one batch of events instead of one per event
>> (armbru)
>> * Error handling fixes (armbru)
>> 
>> Signed-off-by: Jeremy Katz <katzj@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c
>> --- a/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep 02
12:11:54 2006 +0100
>> +++ b/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c	Sat Sep 02
15:11:17 2006 -0400
>> @@ -1866,8 +1866,12 @@ void __init setup_arch(char **cmdline_p)
>>  #endif
>>  #endif
>>  	} else {
>> +#if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
>> +		conswitchp = &dummy_con;
>> +#else
>>  		extern int console_use_vt;
>>  		console_use_vt = 0;
>> +#endif
>>  	}
>>  }
> Not quite sure I understand what this is trying to achieve, or why
it''s
> related to the PV framebuffer stuff.
Jeremy answered this one.
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/Kconfig
>> --- a/linux-2.6-xen-sparse/drivers/xen/Kconfig	Sat Sep 02 12:11:54 2006
+0100
>> +++ b/linux-2.6-xen-sparse/drivers/xen/Kconfig	Sat Sep 02 15:11:17 2006
-0400
>> @@ -172,6 +172,29 @@ config XEN_NETDEV_FRONTEND
>>  	  dedicated device-driver domain, or your master control domain
>>  	  (domain 0), then you almost certainly want to say Y here.
>>  
>> +config XEN_FRAMEBUFFER
>> +	tristate "Framebuffer-device frontend driver"
>> +	depends on XEN && FB
>> +	select FB_CFB_FILLRECT
>> +	select FB_CFB_COPYAREA
>> +	select FB_CFB_IMAGEBLIT
>> +	default y
>> +	help
>> +	  The framebuffer-device frontend drivers allows the kernel to create
a
>> +	  virtual framebuffer.  This framebuffer can be viewed in another
>> +	  domain.  Unless this domain has access to a real video card, you
>> +	  probably want to say Y here.
>> +
>> +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.
>> +
>
> From last time:
>
>> > > 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?
>> > you shed some light on why you created two separate configs?
>> They are two separate devices.  Seemed like a natural separation to me.
> Perhaps XEN_KEYBOARD should depend on XEN_FRAMEBUFFER, given that it
> doesn''t work without it.
Good point.  Jeremy took care of it.
>>  config XEN_SCRUB_PAGES
>>  	bool "Scrub memory before freeing it to Xen"
>>  	default y
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/Makefile
>> --- a/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02 12:11:54
2006 +0100
>> +++ b/linux-2.6-xen-sparse/drivers/xen/Makefile	Sat Sep 02 15:11:17
2006 -0400
>> @@ -15,3 +15,5 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= net
>>  obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
>>  obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= pciback/
>>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND)	+= pcifront/
>> +obj-$(CONFIG_XEN_FRAMEBUFFER)		+= xenfb/
>> +obj-$(CONFIG_XEN_KEYBOARD)		+= xenkbd/
>> diff -r 5fa9b746d24f -r 510c6bceb87f linux-2.6-xen-sparse/mm/memory.c
>> --- a/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 12:11:54 2006 +0100
>> +++ b/linux-2.6-xen-sparse/mm/memory.c	Sat Sep 02 15:11:17 2006 -0400
>> @@ -881,6 +881,7 @@ unsigned long zap_page_range(struct vm_a
>>  		tlb_finish_mmu(tlb, address, end);
>>  	return end;
>>  }
>> +EXPORT_SYMBOL(zap_page_range);
> It''s unfortunate that we have to modify an upstream file here, but
I
> can''t really see any alternative.
Neither can I, nor the folks I asked.
>>  /*
>>   * Do a quick page-table lookup for a single page.
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile	Sat Sep 02
15:11:17 2006 -0400
>> @@ -0,0 +1,1 @@
>> +obj-$(CONFIG_XEN_FRAMEBUFFER)	:= xenfb.o
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c	Sat Sep 02
15:11:17 2006 -0400
>> @@ -0,0 +1,571 @@
>> +/*
>> + * linux/drivers/video/xenfb.c -- Xen para-virtual frame buffer device
>> + *
>> + * Copyright (C) 2005-2006
>> + *
>> + *      Anthony Liguori <aliguori@us.ibm.com>
>> + *
>> + *  Based on linux/drivers/video/q40fb.c
>> + *
>> + *  This file is subject to the terms and conditions of the GNU
General Public
>> + *  License. See the file COPYING in the main directory of this
archive for
>> + *  more details.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/fb.h>
>> +#include <linux/module.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/mm.h>
>> +#include <asm/hypervisor.h>
>> +#include <xen/evtchn.h>
>> +#include <xen/xenbus.h>
>> +#include <linux/xenfb.h>
>> +#include <linux/kthread.h>
>> +
>> +#define XENFB_WIDTH 800
>> +#define XENFB_HEIGHT 600
>> +#define XENFB_DEPTH 32
>> +
>> +static int xenfb_fps = 20;
>> +static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT *
XENFB_DEPTH / 8;
>> +
>> +struct xenfb_mapping
>> +{
>> +	struct list_head	next;
> Perhaps not the best name for a list head?
The mappings are kept in a list of struct xenfb_mapping, chained
through member next.  I''ve seen worse names.  Feel free to suggest a
better one.
>> +	struct vm_area_struct	*vma;
>> +	atomic_t		map_refs;
>> +	int			faults;
>> +	struct xenfb_info	*info;
>> +};
>> +
>> +struct xenfb_info
>> +{
>> +	struct task_struct		*kthread;
>> +	wait_queue_head_t		wq;
>> +
>> +	unsigned char			*fb;
> Why unsigned char?
I think this is a matter of style.  Plain char suggests characters,
while unsigned char suggests bytes.  fb points to an array of bytes.
>> +	struct fb_fix_screeninfo	*fix;
>> +	struct fb_var_screeninfo	*var;
> Not quite sure what these are for.  Why not just get at them through
> the fb_info pointer?
Fine with me.
>> +	struct fb_info			*fb_info;
>> +	struct timer_list		refresh;
>> +	int				dirty;
>
>> +	int				y1, y2;
>> +	int				x1, x2;
> These could perhaps do with slightly more descriptive names?
Would you settle for a descriptive comment instead?
>> +
>> +	struct semaphore		mm_lock;
> That''s going to confuse people.
>
> (Me, for a start)
What exactly confuses you?  The name?  Could rename to mm_sem.
>> +	int				nr_pages;
>> +	struct page			**pages;
>> +	struct list_head		mappings;
>> +
>> +	unsigned			evtchn;
>> +	int				irq;
>> +	struct xenfb_page		*page;
>> +	unsigned long 			*mfns;
>> +	u32				flags;
> Maybe a comment to say what sort of flags go in here?
Sure.
>> +};
>> +
>> +static void xenfb_do_update(struct xenfb_info *info,
>> +			    int x, int y, int w, int h)
>> +{
>> +	union xenfb_out_event event;
>> +	__u32 prod;
>> +
>> +	event.type = XENFB_TYPE_UPDATE;
>> +	event.update.x = x;
>> +	event.update.y = y;
>> +	event.update.width = w;
>> +	event.update.height = h;
>> +
>> +	prod = info->page->out_prod;
>> +	if (prod - info->page->out_cons == XENFB_OUT_RING_LEN)
>> +		return;		/* ring buffer full, event lost */
> It seems to me like if we lose the event, we probably shouldn''t
mark
> the framebuffer as clean and throw away the update region.  Perhaps
> the caller should instead reschedule the timer?
Actually, this can''t happen.  xenfb_update_screen() ensures
there''s
space in the ring before it proceeds to walk the mappings.  That space
can''t go away, because no other place puts stuff into the ring, and
only one instance of xenfb_do_update() can run (in the kernel thread).
> Also, is there any reason the test couldn''t use xenfb_queue_full?
I removed the redundant test instead.
>> +	mb();			/* ensure ring space available */
>> +	XENFB_OUT_RING_REF(info->page, prod) = event;
>> +	wmb();			/* ensure ring contents visible */
>> +	info->page->out_prod = prod + 1;
>> +
>> +	notify_remote_via_evtchn(info->evtchn);
>> +}
>> +
>> +static int xenfb_queue_full(struct xenfb_info *info)
>> +{
>> +	__u32 cons, prod;
>> +
>> +	prod = info->page->out_prod;
>> +	cons = info->page->out_cons;
>> +	return prod - cons == XENFB_OUT_RING_LEN;
>> +}
>> +
>> +static void xenfb_update_screen(struct xenfb_info *info)
>> +{
>> +	int y1, y2, x1, x2;
>> +	struct list_head *item;
>> +	struct xenfb_mapping *map;
>> +
>> +	if (!(info->flags & XENFB_FLAG_UPDATE))
>> +		return;
>> +	if (xenfb_queue_full(info))
>> +		return;
>> +
>> +	y1 = info->y1;
>> +	y2 = info->y2;
>> +	x1 = info->x1;
>> +	x2 = info->x2;
>> +	info->y1 = info->y2 = info->x1 = info->x2 = 0;
>> +	down(&info->mm_lock);
>> +	list_for_each(item, &info->mappings) {
>> +		map = list_entry(item, struct xenfb_mapping, next);
> list_for_each_entry, perhaps?
Going to try.
>> +		if (!map->faults)
>> +			continue;
>> +		zap_page_range(map->vma, map->vma->vm_start,
>> +			       map->vma->vm_end - map->vma->vm_start, NULL);
>> +		map->faults = 0;
>> +	}
>> +	up(&info->mm_lock);
>> +
>> +	xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
>> +}
>> +
>> +static int xenfb_thread(void *data)
>> +{
>> +	struct xenfb_info *info = data;
>> +
>> +	for (;;) {
>> +		if (kthread_should_stop())
>> +			break;
>> +		if (info->dirty) {
>> +			info->dirty = 0;
>> +			xenfb_update_screen(info);
>> +		}
>> +		wait_event_interruptible(info->wq,
>> +			kthread_should_stop() || info->dirty);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int xenfb_setcolreg(unsigned regno, unsigned red, unsigned
green,
>> +			   unsigned blue, unsigned transp,
>> +			   struct fb_info *info)
>> +{
>> +	u32 v;
>> +
>> +	if (regno > info->cmap.len)
>> +		return 1;
>> +
>> +	red   >>= (16 - info->var.red.length);
>> +	green >>= (16 - info->var.green.length);
>> +	blue  >>= (16 - info->var.blue.length);
>> +
>> +	v = (red << info->var.red.offset) |
>> +	    (green << info->var.green.offset) |
>> +	    (blue << info->var.blue.offset);
>> +
>> +	switch (info->var.bits_per_pixel) {
>> +	case 16:
>> +	case 24:
>> +	case 32:
>> +		((u32 *)info->pseudo_palette)[regno] = v;
>> +		break;
>> +	}
> That looks exciting.  What is this code trying to achieve?
Beats me.  Anthony?
> There are comments in xxxfb_setcolreg in skeletonfb to the effect that
> pseudo_palette should only get played with if fix.visual is TRUECOLOR
> or DIRECTCOLOR, and a specific warning against trying to do stuff with
> bits_per_pixel here.  
>
>> +	
>> +	return 0;
>> +}
>> +
>> +static void xenfb_timer(unsigned long data)
>> +{
>> +	struct xenfb_info *info = (struct xenfb_info *)data;
>> +	info->dirty = 1;
>> +	wake_up(&info->wq);
>> +}
>
> From last time:
>
>> > 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.
>
> It looks to me like we can lose updates to the screen in that case:
>
> xenfb_timer runs, sets dirty to 1, kicks the thread
>                                         xenfb_thread wakes up
>                                         xenfb_thread reads the update
region
> another update happens, extends the update region, reschedules xenfb_timer
> xenfb_timer runs again, sets dirty to 2, kicks the thread again
>                                         xenfb_thread sets dirty to 0
>                                         xenfb_thread sends an update using
>                                                 the old region
>
> I suppose the race is sufficiently unlikely and sufficiently mild that
> it can probably be ignored for all realistic purposes.
This can''t happen, because both use and update of the mappings is
protected by mm_lock:

  xenfb_timer runs, sets dirty to 1, kicks the thread
                                xenfb_thread wakes up 
                                xenfb_thread sets dirty to 0

                                xenfb_thread down mm_lock
                                xenfb_thread reads the update region
                                xenfb_thread up mm_lock

                                xenfb_thread sends an update using
                                        the old region

Possibly concurrent:

  another update happens:
  down mm_lock
  extends the update region, reschedules xenfb_timer
  up mm_lock

  xenfb_timer runs again, sets dirty to 1, kicks the thread again

Harmless race: dirty can conceivably become 1 when there''s no work to
do.  Then xenfb_thread wakes up, finds nothing to do and goes back to
sleep.

Looks safe to me.
>> +
>> +static void xenfb_refresh(struct xenfb_info *info,
>> +			  int x1, int y1, int w, int h)
> Is there any reason why there can''t be multiple calls to this
function
> live at the same time?  In particular, if vm_nopage and xenfb_copyarea
> happen at the same time it looks like we can lose part of the update
> area.
>
> Also, vm_nopage holds the mm_lock when it calls this, whereas the
> other callers appear not to.  Was this deliberate?
The question is what mm_lock protects.  If it protects info->mappings,
then xenfb_refresh() doesn''t care about it, as it doesn''t
access it.

If it should protect x1, x2, y1, y2 as well, then locking looks
flawed.

x1, x2, y1, y2 need protection if they can be updated concurrently.
Users are xenfb_update_screen() and xenfb_refresh().  The latter runs
from fb_ops methods and vm_operations_struct method nopage.  Looks
like they need protection, doesn''t it?  Anthony?
>> +{
>> +	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;
>> +	}
> Presumably, these are just looking to see if there''s a
pre-existing
> region, and if not copying the new region across verbatim?  Could you
> test dirty instead in that case?
What about initializing x1, y1 with an impossibly small value, and x2,
y2 with an impossibly large value?
>> +
>> +	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);
> Would it be sensible to have a fallback so that an update is always
> sent to the backend e.g. 100ms after any change to the framebuffer, so
> that the user still sees something when the display is being updated
> very frequently?
Could this be done in a later revision?  Happy to add the idea in a
comment now.
>> +}
>> +
>> +static void xenfb_fillrect(struct fb_info *p, const struct fb_fillrect
*rect)
>> +{
>> +	struct xenfb_info *info = p->par;
>> +
>> +	cfb_fillrect(p, rect);
>> +	xenfb_refresh(info, rect->dx, rect->dy, rect->width,
rect->height);
>> +}
>> +
>> +static void xenfb_imageblit(struct fb_info *p, const struct fb_image
*image)
>> +{
>> +	struct xenfb_info *info = p->par;
>> +
>> +	cfb_imageblit(p, image);
>> +	xenfb_refresh(info, image->dx, image->dy, image->width,
image->height);
>> +}
>> +
>> +static void xenfb_copyarea(struct fb_info *p, const struct fb_copyarea
*area)
>> +{
>> +	struct xenfb_info *info = p->par;
>> +
>> +	cfb_copyarea(p, area);
>> +	xenfb_refresh(info, area->dx, area->dy, area->width,
area->height);
>> +}
>> +
>> +static void xenfb_vm_open(struct vm_area_struct *vma)
>> +{
>> +	struct xenfb_mapping *map = vma->vm_private_data;
>> +	atomic_inc(&map->map_refs);
>> +}
>> +
>> +static void xenfb_vm_close(struct vm_area_struct *vma)
>> +{
>> +	struct xenfb_mapping *map = vma->vm_private_data;
>> +	struct xenfb_info *info = map->info;
>> +
>> +	down(&info->mm_lock);
>> +	if (atomic_dec_and_test(&map->map_refs)) {
>> +		list_del(&map->next);
>> +		kfree(map);
>> +	}
>> +	up(&info->mm_lock);
>> +}
>> +
>> +static struct page *xenfb_vm_nopage(struct vm_area_struct *vma,
>> +				    unsigned long vaddr, int *type)
>> +{
>> +	struct xenfb_mapping *map = vma->vm_private_data;
>> +	struct xenfb_info *info = map->info;
>> +	int pgnr = (vaddr - vma->vm_start) >> PAGE_SHIFT;
>> +	struct page *page;
>> +	int y1, y2;
>> +
>> +	if (pgnr >= info->nr_pages)
>> +		return NOPAGE_SIGBUS;
>> +
>> +	down(&info->mm_lock);
>> +	page = info->pages[pgnr];
>> +	get_page(page);
>> +	map->faults++;
>> +
>> +	y1 = pgnr * PAGE_SIZE / info->fix->line_length;
>> +	y2 = (pgnr * PAGE_SIZE + PAGE_SIZE - 1) /
info->fix->line_length;
>> +	if (y2 > info->var->yres)
>> +		y2 = info->var->yres;
>> +	xenfb_refresh(info, 0, y1, info->var->xres, y2 - y1);
>> +	up(&info->mm_lock);
>> +
>> +	if (type)
>> +		*type = VM_FAULT_MINOR;
>> +
>> +	return page;
>> +}
>> +
>> +static struct vm_operations_struct xenfb_vm_ops = {
>> +	.open	= xenfb_vm_open,
>> +	.close	= xenfb_vm_close,
>> +	.nopage	= xenfb_vm_nopage,
>> +};
>> +
>> +static int xenfb_mmap(struct fb_info *fb_info, struct vm_area_struct
*vma)
>> +{
>> +	struct xenfb_info *info = fb_info->par;
>> +	struct xenfb_mapping *map;
>> +	int ret;
>> +	int map_pages;
>> +
>> +	down(&info->mm_lock);
>> +
>> +	ret = -EINVAL;
>> +	if (!(vma->vm_flags & VM_WRITE))
>> +		goto out;
>> +	if (!(vma->vm_flags & VM_SHARED))
>> +		goto out;
>> +	if (vma->vm_pgoff != 0)
>> +		goto out;
>> +
>> +	map_pages = (vma->vm_end - vma->vm_start + PAGE_SIZE-1)
>> PAGE_SHIFT;
>> +	if (map_pages > info->nr_pages)
>> +		goto out;
>> +
>> +	ret = -ENOMEM;
>> +	map = kmalloc(sizeof(*map), GFP_KERNEL);
>> +	if (map == NULL)
>> +		goto out;
>> +	memset(map, 0, sizeof(*map));
> kzalloc, maybe?
Sure.
>> +
>> +	map->vma = vma;
>> +	map->faults = 0;
>> +	map->info = info;
>> +	atomic_set(&map->map_refs, 1);
>> +	list_add(&map->next, &info->mappings);
>> +	vma->vm_ops = &xenfb_vm_ops;
>> +	vma->vm_flags |= (VM_DONTEXPAND | VM_RESERVED);
>> +	vma->vm_private_data = map;
>> +	ret = 0;
>> +
>> + out:
>> +	up(&info->mm_lock);
>> +	return ret;
>> +}
>> +
>> +static struct fb_ops xenfb_fb_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.fb_setcolreg	= xenfb_setcolreg,
>> +	.fb_fillrect	= xenfb_fillrect,
>> +	.fb_copyarea	= xenfb_copyarea,
>> +	.fb_imageblit	= xenfb_imageblit,
>> +	.fb_mmap	= xenfb_mmap,
>> +};
>> +
>> +static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
>> +				       struct pt_regs *regs)
>> +{
>> +	struct xenfb_info *info = dev_id;
>> +	__u32 cons, prod;
>> +
>> +	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_IN_RING_REF(info->page, cons);
>> +
>> +		switch (event->type) {
>> +		case XENFB_TYPE_SET_EVENTS:
>> +			info->flags = event->set_events.flags;
>> +			break;
>> +		}
>> +	}
>> +	mb();			/* ensure we''re done with ring contents */
>> +	info->page->in_cons = cons;
>> +	notify_remote_via_evtchn(info->evtchn);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static unsigned long vmalloc_to_mfn(void *address)
>> +{
>> +	// FIXME was return pfn_to_mfn(vmalloc_to_pfn(address));
> Not quite sure why this comment is here.
Accident.  However, arbitrary_virt_to_machine() doesn''t appear to
exist on ia64, so I went back to the old code.

>> +	return arbitrary_virt_to_machine(address) >> PAGE_SHIFT;
>> +}
>> +
>> +static struct xenfb_info *xenfb_info;
>> +
>> +static int __init xenfb_probe(void)
> I''d find it easier to be confident of the error handling here if
the
> function were split up a little.
It''s just the way I found it.  My own idea of cleanup would be to make
xenfb_cleanup() work for the error case.
>> +{
>> +	struct xenfb_info *info;
>> +	int i, ret;
>> +	struct fb_info *fb_info;
>> +	struct evtchn_alloc_unbound alloc_unbound;
>> +	struct evtchn_close close;
>> +	struct xenbus_transaction xbt;
>> +
>> +        /* Nothing to do if running in dom0. */
>> +        if (is_initial_xendomain())
>> +		return -ENODEV;
>> +#if 0
>> +        /* if we''re not set up to use graphics mode, then
don''t initialize */
>> +	if (xenbus_scanf(XBT_NIL, "console",
"use_graphics", "%d", &ret) < 0)
>> +		return -ENODEV;
>> +	if (ret == 0)
>> +		return -ENODEV;
>> +#endif
> That''s interesting.  What is this here for?
Jeremy explained this one already.  The #if is an accident.
>> +
>> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
>> +	if (info == NULL)
>> +		return -ENOMEM;
>> +	memset(info, 0, sizeof(*info));
>> +
>> +	INIT_LIST_HEAD(&info->mappings);
>> +
>> +	info->fb = vmalloc(xenfb_mem_len);
>> +	if (info->fb == NULL)
>> +		goto error;
>> +	memset(info->fb, 0, xenfb_mem_len);
>> +	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >>
PAGE_SHIFT;
>> +	info->pages = kmalloc(sizeof(struct page*)*info->nr_pages,
GFP_KERNEL);
>> +	if (info->pages == NULL)
>> +		goto error_vfree;
>> +	for (i = 0; i < info->nr_pages; i++)
>> +		info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);
>> +
>> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
>> +	// FIXME sizeof(struct xenfb_info)
> Where did sizeof(u32) * 256 come from?  What does the comment mean?
I *suspect* that the correct argument is sizeof(struct xenfb_info),
but can''t yet exclude subtle, evil hackery going on here, so I put in
a comment for now.  Anthony, do you remember hackery here?
> fb_info isn''t released from the error path.
Fixing...
>> +	if (fb_info == NULL)
>> +		goto error_kfree;
>> +
>> +	info->mfns = vmalloc(sizeof(unsigned long) * info->nr_pages);
> What happens if this fails?  Also, you never seem to vfree this if we
> hit an error later on.
Fixing...
>> +	/* set up shared page */
>> +	info->page = (void *)__get_free_page(GFP_KERNEL);
>> +	if (!info->page)
>> +		goto error_kfree;
>> +	/* set up event channel */
>> +	alloc_unbound.dom = DOMID_SELF;
>> +	alloc_unbound.remote_dom = 0;
>> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
>> +					  &alloc_unbound);
>> +	if (ret)
>> +		goto error_freep;
>> +	info->evtchn = alloc_unbound.port;
>> +
>> +	for (i = 0; i < info->nr_pages; i++)
>> +		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
>> +	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
> What happens if this domain is running in shadow translate mode?  It
> looks to me like the backend will end up mapping completely the wrong
> frame.
Discussion above seems to suggest that our only option is to have the
backend detect and translate shadow translate mode frontends, so no
action is required here.  Correct?
>> +	info->page->pd[1] = 0;
>> +	info->page->width = XENFB_WIDTH;
>> +	info->page->height = XENFB_HEIGHT;
>> +	info->page->depth = XENFB_DEPTH;
>> +	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) {
>> +		close.port = info->evtchn;
>> +		HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
>> +		goto error_freep;
>> +	}
>> +
>> +	info->irq = ret;
>> +	xenfb_info = info;
>> +
>> +	fb_info->pseudo_palette = fb_info->par;
>> +	fb_info->par = info;
>> +	fb_info->screen_base = info->fb;
>> +
>> +	memset(&fb_info->var, 0, sizeof(fb_info->var));
>> +	memset(&fb_info->fix, 0, sizeof(fb_info->fix));
> I think these are already guaranteed to be zero, aren''t they?
Yes, framebuffer_alloc() clears *fb_info.  Happy to rely on that?
>> +
>> +	fb_info->fbops = &xenfb_fb_ops;
>> +	fb_info->var.xres_virtual = fb_info->var.xres =
info->page->width;
>> +	fb_info->var.yres_virtual = fb_info->var.yres =
info->page->height;
>> +	fb_info->var.bits_per_pixel = info->page->depth;
>> +
>> +	fb_info->var.red = (struct fb_bitfield){16, 8, 0};
>> +	fb_info->var.green = (struct fb_bitfield){8, 8, 0};
>> +	fb_info->var.blue = (struct fb_bitfield){0, 8, 0};
>> +
>> +	fb_info->var.activate = FB_ACTIVATE_NOW;
>> +	fb_info->var.height = -1;
>> +	fb_info->var.width = -1;
>> +	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
>> +
>> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
>> +	fb_info->fix.line_length = info->page->line_length;
>> +	fb_info->fix.smem_start = 0;
>> +	fb_info->fix.smem_len = xenfb_mem_len;
>> +	strcpy(fb_info->fix.id, "xen");
>> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
>> +	fb_info->fix.accel = FB_ACCEL_NONE;
>> +
>> +	fb_info->flags = FBINFO_FLAG_DEFAULT;
>> +
>> +	fb_alloc_cmap(&fb_info->cmap, 256, 0);
> This can fail, and the error paths don''t bother to deallocate it.
Fixing...
>> +
>> +	info->fb_info = fb_info;
>> +	info->fix = &fb_info->fix;
>> +	info->var = &fb_info->var;
>> +
>> +	init_MUTEX(&info->mm_lock);
>> +	init_waitqueue_head(&info->wq);
>> +	init_timer(&info->refresh);
>> +	info->refresh.function = xenfb_timer;
>> +	info->refresh.data = (unsigned long)info;
>> +
>> +	info->kthread = kthread_run(xenfb_thread, info, "xenfb
thread");
>> +	if (IS_ERR(info->kthread))
>> +		goto error_unbind;
>> +
>> +	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));
>> +	// FIXME grant tables?
> Again, what happens in shadow translate mode?
>
> Also, it''s kind of annoying that there''s no support for
multiple
> framebuffers in a domU.  That''s very much a wishlist thing,
though.
See above.
>> +	if (ret)
>> +		goto error_xenbus;
>> +	ret = xenbus_printf(xbt, "vfb", "event-channel",
"%u",
>> +			    info->evtchn);
>> +	if (ret)
>> +		goto error_xenbus;
>> +	ret = xenbus_transaction_end(xbt, 0);
>> +	if (ret) {
>> +		if (ret == -EAGAIN)
>> +			goto again;
>> +		goto error_unreg;
>> +	}
>> +
>> +	return 0;
>> +
>> + error_xenbus:
>> +	xenbus_transaction_end(xbt, 1);
>> + error_unreg:
>> +	unregister_framebuffer(fb_info);
>> + error_unbind:
>> +	unbind_from_irqhandler(info->irq, info);
>> +	// FIXME do we have to stop info->kthread?
> If you''ve started it you certainly need to stop it.
Fixing...
>> + error_freep:
>> +	free_page((unsigned long)info->page);
>> + error_kfree:
>> +	kfree(info->pages);
>> + error_vfree:
>> +	vfree(info->fb);
>> + error:
>> +	kfree(info);
>> +	xenfb_info = NULL;
>> +
>> +	return -ENODEV;
>> +}
>> +
>> +static int __init xenfb_init(void)
>> +{
>> +	return xenfb_probe();
>
> From last time:
>
>> > 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?
> I think if the frontend needs to know whether the backend is in kernel
> or userspace then we''ve messed up somewhere.  Why do you expect
this
> to be difficult?
I didn''t *expect* it to be difficult, I just couldn''t see how
to fit
Anthony''s code to xenbus_device without rewriting more than I like,
and I still can''t see it.  That''s why I asked for advice.

Anyway, I''m reluctant to throw out working code while there''s
so much
code that needs fixing.  I may be less reluctant once we''ve drained
the swamp some more.
>> +}
>> +
>> +static void __exit xenfb_cleanup(void)
>> +{
>> +	struct xenfb_info *info = xenfb_info;
>> +
>> +	unregister_framebuffer(info->fb_info);
>> +	unbind_from_irqhandler(info->irq, info);
>> +	free_page((unsigned long)info->page);
>> +	kfree(info->pages);
>> +	vfree(info->fb);
>> +	kfree(info);
>> +	xenfb_info = NULL;
> I don''t think this releases all of the memory you allocate in
> xenfb_probe.
Fixing...
>> +}
>> +
>> +module_init(xenfb_init);
>> +module_exit(xenfb_cleanup);
>> +
>> +MODULE_LICENSE("GPL");
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenkbd/Makefile
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/linux-2.6-xen-sparse/drivers/xen/xenkbd/Makefile	Sat Sep 02
15:11:17 2006 -0400
>> @@ -0,0 +1,1 @@
>> +obj-$(CONFIG_XEN_KEYBOARD)	+= xenkbd.o
>> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenkbd/xenkbd.c
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/linux-2.6-xen-sparse/drivers/xen/xenkbd/xenkbd.c	Sat Sep 02
15:11:17 2006 -0400
>> @@ -0,0 +1,178 @@
>> +/*
>> + * linux/drivers/input/keyboard/xenkbd.c -- Xen para-virtual input
device
>> + *
>> + * Copyright (C) 2005
>> + *
>> + *      Anthony Liguori <aliguori@us.ibm.com>
>> + *
>> + *  Based on linux/drivers/input/mouse/sermouse.c
>> + *
>> + *  This file is subject to the terms and conditions of the GNU
General Public
>> + *  License. See the file COPYING in the main directory of this
archive for
>> + *  more details.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/module.h>
>> +#include <linux/input.h>
>> +#include <asm/hypervisor.h>
>> +#include <xen/evtchn.h>
>> +#include <xen/xenbus.h>
>> +#include <linux/xenkbd.h>
>> +
>> +struct xenkbd_device
>> +{
>> +	struct input_dev *dev;
>> +	struct xenkbd_info *info;
>> +	unsigned evtchn;
>> +	int irq;
>> +};
>> +
>> +static irqreturn_t input_handler(int rq, void *dev_id, struct pt_regs
*regs)
>> +{
>> +	struct xenkbd_device *dev = dev_id;
>> +	struct xenkbd_info *info = dev ? dev->info : 0;
>> +	static int button_map[3] = { BTN_RIGHT, BTN_MIDDLE, BTN_LEFT };
>> +	__u32 cons, prod;
>> +
>> +	prod = info->in_prod;
>> +	rmb();			/* ensure we see ring contents up to prod */
>> +	for (cons = info->in_cons; cons != prod; cons++) {
>> +		union xenkbd_in_event *event;
>> +		event = &XENKBD_IN_RING_REF(info, cons);
>> +	
>> +		switch (event->type) {
>> +		case XENKBD_TYPE_MOTION:
>> +			input_report_rel(dev->dev, REL_X, event->motion.rel_x);
>> +			input_report_rel(dev->dev, REL_Y, event->motion.rel_y);
>> +			break;
>> +		case XENKBD_TYPE_BUTTON:
>> +			if (event->button.button < 3)
>> +				input_report_key(dev->dev,
>> +						 button_map[event->button.button],
>> +						 event->button.pressed);
>> +			break;
>> +		case XENKBD_TYPE_KEY:
>> +			input_report_key(dev->dev, event->key.keycode,
event->key.pressed);
>> +			break;
>> +		}
>> +	}
>> +	input_sync(dev->dev);
>> +	mb();			/* ensure we got ring contents */
>> +	info->in_cons = cons;
>> +	notify_remote_via_evtchn(dev->evtchn);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static struct xenkbd_device *xenkbd_dev;
>> +
>> +int __init xenkbd_init(void)
>> +{
>> +	int ret = 0;
>> +	int i;
>> +	struct xenkbd_device *dev;
>> +	struct input_dev *input_dev;
>> +	struct evtchn_alloc_unbound alloc_unbound;
>> +	struct xenbus_transaction xbt;
>> +
>> +        /* Nothing to do if running in dom0. */
>> +        if (is_initial_xendomain())
>> +	  return -ENODEV;
>> +#if 0
>> +        /* if we''re not set up to use graphics mode, then
don''t initialize */
>> +	if (xenbus_scanf(XBT_NIL, "console",
"use_graphics", "%d", &ret) < 0)
>> +	  return -ENODEV;
>> +	if (ret == 0)
>> +	  return -ENODEV;
>> +#endif
> Not sure about this.
Same as for xenfb.c.  The #if is an accident.
>> +
>> +	dev = kmalloc(sizeof(*dev), GFP_KERNEL);
>> +	input_dev = input_allocate_device();
>> +	if (!dev || !input_dev)
>> +		return -ENOMEM;
> You potentially leak memory if one of these fails and the other succeeds.
>
> Also, the error paths don''t release input_dev.  In fact, nothing
ever
> releases input_dev, unless I''m missing something.
Fixing...
>> +
>> +	dev->dev = input_dev;
>> +	dev->info = (void *)__get_free_page(GFP_KERNEL);
>> +	if (!dev->info) {
>> +		ret = -ENOMEM;
>> +		goto error;
>> +	}
>> +
>> +	alloc_unbound.dom = DOMID_SELF;
>> +	alloc_unbound.remote_dom = 0;
>> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
>> +					  &alloc_unbound);
>> +	if (ret)
>> +		goto error_freep;
>> +	dev->evtchn = alloc_unbound.port;
>> +	ret = bind_evtchn_to_irqhandler(dev->evtchn, input_handler, 0,
>> +					"xenkbd", dev);
>> +	if (ret < 0)
>> +		goto error_freep;
> You might want to close the event channel here.
Fixing...
>> +	dev->irq = ret;
>> +
>> +	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL);
>> +	input_dev->keybit[LONG(BTN_MOUSE)] = BIT(BTN_LEFT) |
BIT(BTN_RIGHT);
>> +	input_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y);
>> +
>> +	/* FIXME not sure this is quite right */
>> +	for (i = 0; i < 256; i++)
>> +		set_bit(i, input_dev->keybit);
> It looks reasonable, but perhaps memset would be a better choice.
Dunno.  Anthony?
>> +
>> +	input_dev->name = "Xen Virtual Keyboard/Mouse";
>> +
>> +	input_register_device(input_dev);
> This can fail.
Most uses elsewhere in the kernel don''t care.  Fixing anyway.
>> +
>> + again:
>> +	ret = xenbus_transaction_start(&xbt);
>> +	if (ret)
>> +		goto error_unreg;
>> +	ret = xenbus_printf(xbt, "vkbd", "page-ref",
"%lu",
>> +			    virt_to_mfn(dev->info));
> There''s no reason that I can see not to use grant tables here, and
> that''d avoid all of the potential issues with shadow translate
mode.
Since we have to fool around with shadow translate mode anyway for
sharing the framebuffer, I don''t quite see why we need to address this
right now.  This code works, and there''s no shortage of code that
needs fixing.

Same for vfb/page-ref.
>> +	if (ret)
>> +		goto error_xenbus;
>> +	ret = xenbus_printf(xbt, "vkbd", "event-channel",
"%u",
>> +			    dev->evtchn);
>> +	if (ret)
>> +		goto error_xenbus;
>> +	ret = xenbus_transaction_end(xbt, 0);
>> +	if (ret) {
>> +		if (ret == -EAGAIN)
>> +			goto again;
>> +		goto error_unreg;
>> +	}
>> +
>> +	dev->info->in_cons = dev->info->in_prod = 0;
>> +	dev->info->out_cons = dev->info->out_prod = 0;
>> +	xenkbd_dev = dev;
>> +
>> +	return ret;
>> +
>> +	
>> + error_xenbus:
>> +	xenbus_transaction_end(xbt, 1);
>> + error_unreg:
>> +	input_unregister_device(input_dev);
>> +	unbind_from_irqhandler(dev->irq, dev);
>> + error_freep:
>> +	free_page((unsigned long)dev->info);
>> + error:
>> +	kfree(dev);
>> +	return ret;
>> +}
>> +
>> +static void __exit xenkbd_cleanup(void)
>> +{
>> +	input_unregister_device(xenkbd_dev->dev);
>> +	unbind_from_irqhandler(xenkbd_dev->irq, xenkbd_dev);
>> +	free_page((unsigned long)xenkbd_dev->info);
>> +	kfree(xenkbd_dev);
>> +	xenkbd_dev = NULL;
>> +}
>> +
>> +module_init(xenkbd_init);
>> +module_exit(xenkbd_cleanup);
>> +
>> +MODULE_LICENSE("GPL");
>> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02 15:11:17
2006 -0400
>
> This is defining an io protocol, so belongs in xen/include/public/io
Okay.
>> @@ -0,0 +1,108 @@
>> +/*
>> + * linux/include/linux/xenfb.h -- Xen virtual frame buffer device
>> + *
>> + * Copyright (C) 2005
>> + *
>> + *      Anthony Liguori <aliguori@us.ibm.com>
>> + *
>> + *  This file is subject to the terms and conditions of the GNU
General Public
>> + *  License. See the file COPYING in the main directory of this
archive for
>> + *  more details.
>> + */
>> +
>> +#ifndef _LINUX_XENFB_H
>> +#define _LINUX_XENFB_H
>> +
>> +#include <asm/types.h>
>> +
>> +/* out events */
>> +
>> +#define XENFB_OUT_EVENT_SIZE 40
>> +
>> +#define XENFB_TYPE_MOTION 1
>> +#define XENFB_TYPE_UPDATE 2
>> +
>
> The other tagged unions in Xen look more like this:
>
> struct {
> 	u8 type;
> 	union {
> 		struct {
> 			u32 field1;
> 			u32 field2;
> 		} type1;
> 		struct {
> 			u16 field1;
> 		} type2;
> 	} u;
> };
>
> I''d find this a lot easier to read than the current approach of
> putting the type into the individual event type structures.  Is there
> any reason why this couldn''t be used here?
Matter of taste.  Will change it if you insist.
>> +struct xenfb_motion	    /* currently unused */
>> +{
>> +	__u8 type;          /* XENFB_TYPE_MOTION */
>> +	__u16 x;            /* The new x coordinate */
>> +	__u16 y;            /* The new y coordinate */
>> +};
> I think if the comment said what was moving this''d be a lot
clearer.
I''m not into documenting unused code I didn''t write and
don''t
understand, so I''m going to remove this.
>> +
>> +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;
>> +	struct xenfb_motion motion;
>> +	struct xenfb_update update;
>> +	char _[XENFB_OUT_EVENT_SIZE];
> Perhaps ``pad'''' would be clearer than
``_''''?
Works for me.
>> +};
>> +
>> +/* in events */
>> +
>> +#define XENFB_IN_EVENT_SIZE 40
>> +
>> +#define XENFB_TYPE_SET_EVENTS 1
>> +
>> +#define XENFB_FLAG_MOTION 1
>> +#define XENFB_FLAG_UPDATE 2
>> +#define XENFB_FLAG_COPY 4
>> +#define XENFB_FLAG_FILL 8
>> +
>> +struct xenfb_set_events
>> +{
>> +	__u8 type;          /* XENFB_TYPE_SET_EVENTS */
>> +	__u32 flags;        /* combination of XENFB_FLAG_* */
>> +};
>> +
>> +union xenfb_in_event
>> +{
>> +	__u8 type;
>> +	struct xenfb_set_events set_events;
>> +	char _[XENFB_OUT_EVENT_SIZE];
>> +};
>> +
>> +/* shared page */
>> +
>> +#define XENFB_IN_RING_SIZE 1024
>> +#define XENFB_IN_RING_LEN (XENFB_IN_RING_SIZE / XENFB_IN_EVENT_SIZE)
>> +#define XENFB_IN_RING_OFFS 1024
>> +#define XENFB_IN_RING(page) \
>> +    ((union xenfb_in_event *)((char *)(page) + XENFB_IN_RING_OFFS))
>> +#define XENFB_IN_RING_REF(page, idx) \
>> +    (XENFB_IN_RING((page))[(idx) % XENFB_IN_RING_LEN])
>> +
>> +#define XENFB_OUT_RING_SIZE 2048
>> +#define XENFB_OUT_RING_LEN (XENFB_OUT_RING_SIZE /
XENFB_OUT_EVENT_SIZE)
>> +#define XENFB_OUT_RING_OFFS (XENFB_IN_RING_OFFS + XENFB_IN_RING_SIZE)
>> +#define XENFB_OUT_RING(page) \
>> +    ((union xenfb_out_event *)((char *)(page) + XENFB_OUT_RING_OFFS))
>> +#define XENFB_OUT_RING_REF(page, idx) \
>> +    (XENFB_OUT_RING((page))[(idx) % XENFB_OUT_RING_LEN])
>> +
>> +struct xenfb_page
>> +{
>> +	__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) */
>> +
>> +	unsigned long pd[2];	/* FIXME rename to pgdir? */
>> +	/* FIXME pd[1] unused at this time, shrink? */
> I think it''d be enough to just give some indication of where the
magic
> number 2 came from.
Okay.
> From last time:
>
>> > 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.
> We''re going to eventually need some way for the frontend to tell
the
> backend to change the display resolution and/or depth.  Could this be
> used to select the initial resolution?  I''m not sure what the plan
is
> here, but I would guess something like this:
>
> 1) Framebuffer is operating normally, with the frontend modifying the
>         buffer in memory and generating update events in the usual way.
>
> 2) Frontend decides it wants a resolution change.  Linux is made to stop
>         updating the buffer.  Not quite sure how to do this, but it seems
> 	like something which most real framebuffers would need to do during
> 	a resolution change.  The frontend sends a start_change_res
>         message to the backend with the desired new resolution.
>
> 3) The backend finishes off any pending updates, and picks up the
>         start_change_res message.  It can then unmap the buffer and
>         send back a change_res_goahead message.
>
> 4) The frontend picks up the change_res_goahead message.  It can now
>         release the buffer area and allocate a new one of the
>         correct size, and send a change_res_complete message to the
>         correct size, and send a change_res_complete message to the
>         backend.  Once that''s sent the frontend can go back to
normal
>         operation at the new resolution.
>
> 5) The backend picks up the change_res_complete message, remaps
>         the buffer, and returns to normal operation.
>
> change_res_complete is actually redundant, but will simplify the
> backend enough that it''s probably worthwhile.
>
> If you use a protocol like this, then you could have the frontend send
> a start_change_res as the first thing after it starts up, and
> communicate the information that way.  This also gives you some scope
> for negotiating a lower res if the backend can''t cope with the
> requested one for some reason.
>
>
> The other way would be to do this through xenbus, but synchronising
> with the data path would be a bit of a pain.
Anthony, would you like to comment on this?
>> +
>> +	__u32 in_cons, in_prod;
>> +	__u32 out_cons, out_prod;
>> +};
>> +
>> +void xenfb_resume(void);
> No longer exists.
Removed.
>> +
>> +#endif
>> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02 15:11:17
2006 -0400
> Again, this belongs in xen/include/public/io.
Okay.
>> @@ -0,0 +1,92 @@
>> +/*
>> + * linux/include/linux/xenkbd.h -- Xen virtual keyboard/mouse
>> + *
>> + * Copyright (C) 2005
>> + *
>> + *      Anthony Liguori <aliguori@us.ibm.com>
>> + *
>> + *  This file is subject to the terms and conditions of the GNU
General Public
>> + *  License. See the file COPYING in the main directory of this
archive for
>> + *  more details.
>> + */
>> +
>> +#ifndef _LINUX_XENKBD_H
>> +#define _LINUX_XENKBD_H
>> +
>> +#include <asm/types.h>
>> +
>> +/* in events */
>> +
>> +#define XENKBD_IN_EVENT_SIZE 40
>> +
>> +#define XENKBD_TYPE_MOTION  1     /* mouse movement event */
>> +#define XENKBD_TYPE_BUTTON  2     /* mouse button event */
>> +#define XENKBD_TYPE_KEY     3     /* keyboard event */
>> +
>> +struct xenkbd_motion
>> +{
>> +	__u8 type;         /* XENKBD_TYPE_MOTION */
>> +	__s16 rel_x;       /* relative X motion */
>> +	__s16 rel_y;       /* relative Y motion */
>> +};
> Again, this is a slightly ugly way of doing tagged unions.
See above.
>> +
>> +struct xenkbd_button
>> +{
>> +	__u8 type;         /* XENKBD_TYPE_BUTTON */
>> +	__u8 pressed;      /* 1 if pressed; 0 otherwise */
>> +	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */
>> +};
>> +
>> +struct xenkbd_key
>> +{
>> +	__u8 type;         /* XENKBD_TYPE_KEY */
>> +	__u8 pressed;      /* 1 if pressed; 0 otherwise */
>> +	__u16 keycode;     /* KEY_* from linux/input.h */
>> +};
>> +
>> +union xenkbd_in_event
>> +{
>> +	__u8 type;
>> +	struct xenkbd_motion motion;
>> +	struct xenkbd_button button;
>> +	struct xenkbd_key key;
>> +	char _[XENKBD_IN_EVENT_SIZE];
>> +};
>> +
>> +/* out events */
> Maybe a comment that none are currently defined?
Sure.
>> +
>> +#define XENKBD_OUT_EVENT_SIZE 40
>> +
>> +union xenkbd_out_event
>> +{
>> +	__u8 type;
>> +	char _[XENKBD_OUT_EVENT_SIZE];
>> +};
>> +
>> +/* shared page */
>> +
>> +#define XENKBD_IN_RING_SIZE 2048
>> +#define XENKBD_IN_RING_LEN (XENKBD_IN_RING_SIZE /
XENKBD_IN_EVENT_SIZE)
>> +#define XENKBD_IN_RING_OFFS 1024
>> +#define XENKBD_IN_RING(page) \
>> +    ((union xenkbd_in_event *)((char *)(page) + XENKBD_IN_RING_OFFS))
>> +#define XENKBD_IN_RING_REF(page, idx) \
>> +    (XENKBD_IN_RING((page))[(idx) % XENKBD_IN_RING_LEN])
>> +
>> +#define XENKBD_OUT_RING_SIZE 1024
>> +#define XENKBD_OUT_RING_LEN (XENKBD_OUT_RING_SIZE /
XENKBD_OUT_EVENT_SIZE)
>> +#define XENKBD_OUT_RING_OFFS (XENKBD_IN_RING_OFFS +
XENKBD_IN_RING_SIZE)
>> +#define XENKBD_OUT_RING(page) \
>> +    ((union xenkbd_out_event *)((char *)(page) +
XENKBD_OUT_RING_OFFS))
>> +#define XENKBD_OUT_RING_REF(page, idx) \
>> +    (XENKBD_OUT_RING((page))[(idx) % XENKBD_OUT_RING_LEN])
>> +
>> +struct xenkbd_info
>> +{
>> +	__u32 in_cons, in_prod;
>> +	__u32 out_cons, out_prod;
>> +};
>> +
>> +void xenkbd_resume(void);
> No longer exists.
Removed.
>> +
>> +#endif
>
> Steven.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > Okay, this fixes a lot of the things I complained about last time, and
> > most of the new problems should be fairly easy to fix up.
> >
> > There are still a couple of things from the last round which are
> > unfixed.  This is mostly my fault for going on holiday at an
> > inconvenient time; sorry about that.
> I''m about to commit the same crime :)  Expect me back on the 19th.
Have fun.
> > The big problem with this is that you don''t support shadow
translate
> > mode guests, because the frontend exposes mfns directly to the
> > backend.  The correct way of fixing this is to use grant tables, but,
> > as Anthony pointed out, that''s quite hard due to the amount
of memory
> > you need to grant access to.
> >
> > The easy way of doing this would be to just increase the size of the
> > grant tables.  Alternatively, you could add support for granting
> > access to ranges of memory, but that sounds like a whole lot of work.
> Yes, it would be great if Xen provided the means to do it right, but
> as Keir wrote that''s post 3.0.3.
> 
> > You could also put in a quick hack of just translating the mfns in the
> > backend if the frontend is in shadow translate mode, but
that''s not
> > really very nice.
> As far as I can see, this is our only option right now.  I''m
ignorant
> of `shadow translate mode''; can somebody please educate me on how
to
> detect and translate shadow translate mode frontend mfns in the
> backend?
Shadow translate mode is a mode of the shadow page tables in which Xen
does machine<->physical address translations on behalf of the guest.
It''s mostly used for HVM guests to give them the impression of a
mostly-contiguous block of memory starting at machine address 0.

If you just bung the guest-supplied machine frame numbers through
xc_translate_gpfn_list you should be fine.  That does the translation
if you''re in shadow translate mode and does nothing if you''re
not.
> > It''d also be nice if the protocol supported putting the
backend in an
> > unprivileged domain, which grant tables would give you for free, but
> > that''s a lower priority.
> I guess that has to wait until we can do the framebuffer mapping
> nicely.
Yep.
> > Aside from that, the lack of support for multiple framebuffers in one
> > guest is kind of annoying.  It''s not necessary to have this
working in
> > a first cut of the patch, but it''d be good if the protocol
included
> > some way of adding it in later, which the current one
doesn''t.
> Well, the only place that limits us to a single framebuffer is our use
> of xenstore, isn''t it?.  All we need for additional framebuffers
then
> is new xenstore directories, say vfbN and vkbdN, where N is 0, 1, ...
> 
> If you feel it''s useful, I can rename the xenstore directories to
vfb0
> and vkbd0 now.
Yes, please.  It''d be good not to have to break the protocol in order
to add this in the future.  I''d prefer vfb/0 and vkbd/0, though, for
consistency with other types of devices.
> > Hard coding the resolution is also kind of icky, but is again
> > acceptable for a first cut.  It''d be good to have at least a
plan for
> > how this is supposed to work before the thing gets merged, including
> > how to change the resolution of a live framebuffer.
> It''s always good to have a plan for future work.  Note, however,
that
> the protocol is easily extensible, and therefore we don''t have to
fear
> coding ourselves into a box now that we have to break later, just
> for lack of foresight.
True.
> > Also, the spec you pointed me at last time appears to be out of date:
> > it still thinks you get the pgdir mfn from start info, which certainly
> > isn''t acceptable for new drivers.
> Yes, it''s out of date.  Let''s nail down things here, and
if we then
> feel the spec is useful to have, we update it.
Good plan.
> >>  /*
> >>   * Do a quick page-table lookup for a single page.
> >> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile
> >> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile	Sat Sep 02
15:11:17 2006 -0400
> >> @@ -0,0 +1,1 @@
> >> +obj-$(CONFIG_XEN_FRAMEBUFFER)	:= xenfb.o
> >> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c
> >> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c	Sat Sep 02
15:11:17 2006 -0400
> >> @@ -0,0 +1,571 @@
> >> +/*
> >> + * linux/drivers/video/xenfb.c -- Xen para-virtual frame buffer
device
> >> + *
> >> + * Copyright (C) 2005-2006
> >> + *
> >> + *      Anthony Liguori <aliguori@us.ibm.com>
> >> + *
> >> + *  Based on linux/drivers/video/q40fb.c
> >> + *
> >> + *  This file is subject to the terms and conditions of the GNU
General Public
> >> + *  License. See the file COPYING in the main directory of this
archive for
> >> + *  more details.
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/fb.h>
> >> +#include <linux/module.h>
> >> +#include <linux/vmalloc.h>
> >> +#include <linux/mm.h>
> >> +#include <asm/hypervisor.h>
> >> +#include <xen/evtchn.h>
> >> +#include <xen/xenbus.h>
> >> +#include <linux/xenfb.h>
> >> +#include <linux/kthread.h>
> >> +
> >> +#define XENFB_WIDTH 800
> >> +#define XENFB_HEIGHT 600
> >> +#define XENFB_DEPTH 32
> >> +
> >> +static int xenfb_fps = 20;
> >> +static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT *
XENFB_DEPTH / 8;
> >> +
> >> +struct xenfb_mapping
> >> +{
> >> +	struct list_head	next;
> > Perhaps not the best name for a list head?
> The mappings are kept in a list of struct xenfb_mapping, chained
> through member next.  I''ve seen worse names.  Feel free to suggest
a
> better one.
Well, list_heads are double linked, so next is kind of deceptive.
Perhaps just ``list''''?
> >> +	struct vm_area_struct	*vma;
> >> +	atomic_t		map_refs;
> >> +	int			faults;
> >> +	struct xenfb_info	*info;
> >> +};
> >> +
> >> +struct xenfb_info
> >> +{
> >> +	struct task_struct		*kthread;
> >> +	wait_queue_head_t		wq;
> >> +
> >> +	unsigned char			*fb;
> > Why unsigned char?
> I think this is a matter of style.  Plain char suggests characters,
> while unsigned char suggests bytes.  fb points to an array of bytes.
I''d have gone for void *, but, as you say, matter of style.
> >> +	struct fb_fix_screeninfo	*fix;
> >> +	struct fb_var_screeninfo	*var;
> > Not quite sure what these are for.  Why not just get at them through
> > the fb_info pointer?
> Fine with me.
Thanks.
> >> +	struct fb_info			*fb_info;
> >> +	struct timer_list		refresh;
> >> +	int				dirty;
> >
> >> +	int				y1, y2;
> >> +	int				x1, x2;
> > These could perhaps do with slightly more descriptive names?
> Would you settle for a descriptive comment instead?
Sure.
> >> +
> >> +	struct semaphore		mm_lock;
> > That''s going to confuse people.
> >
> > (Me, for a start)
> What exactly confuses you?  The name?  Could rename to mm_sem.
Sorry, got my wires crossed a bit here.  Solaris has a global mm_lock,
and Linux used to, and I thought Linux still did.  Ignore me, I''m
wibbling.
> >> +	int				nr_pages;
> >> +	struct page			**pages;
> >> +	struct list_head		mappings;
> >> +
> >> +	unsigned			evtchn;
> >> +	int				irq;
> >> +	struct xenfb_page		*page;
> >> +	unsigned long 			*mfns;
> >> +	u32				flags;
> > Maybe a comment to say what sort of flags go in here?
> Sure.
Thanks.
> >> +};
> >> +
> >> +static void xenfb_do_update(struct xenfb_info *info,
> >> +			    int x, int y, int w, int h)
> >> +{
> >> +	union xenfb_out_event event;
> >> +	__u32 prod;
> >> +
> >> +	event.type = XENFB_TYPE_UPDATE;
> >> +	event.update.x = x;
> >> +	event.update.y = y;
> >> +	event.update.width = w;
> >> +	event.update.height = h;
> >> +
> >> +	prod = info->page->out_prod;
> >> +	if (prod - info->page->out_cons == XENFB_OUT_RING_LEN)
> >> +		return;		/* ring buffer full, event lost */
> > It seems to me like if we lose the event, we probably
shouldn''t mark
> > the framebuffer as clean and throw away the update region.  Perhaps
> > the caller should instead reschedule the timer?
> Actually, this can''t happen.  xenfb_update_screen() ensures
there''s
> space in the ring before it proceeds to walk the mappings.  That space
> can''t go away, because no other place puts stuff into the ring,
and
> only one instance of xenfb_do_update() can run (in the kernel thread).
Okay.
> > Also, is there any reason the test couldn''t use
xenfb_queue_full?
> I removed the redundant test instead.
Thanks.
> >> +	mb();			/* ensure ring space available */
> >> +	XENFB_OUT_RING_REF(info->page, prod) = event;
> >> +	wmb();			/* ensure ring contents visible */
> >> +	info->page->out_prod = prod + 1;
> >> +
> >> +	notify_remote_via_evtchn(info->evtchn);
> >> +}
> >> +
> >> +static int xenfb_queue_full(struct xenfb_info *info)
> >> +{
> >> +	__u32 cons, prod;
> >> +
> >> +	prod = info->page->out_prod;
> >> +	cons = info->page->out_cons;
> >> +	return prod - cons == XENFB_OUT_RING_LEN;
> >> +}
> >> +
> >> +static void xenfb_update_screen(struct xenfb_info *info)
> >> +{
> >> +	int y1, y2, x1, x2;
> >> +	struct list_head *item;
> >> +	struct xenfb_mapping *map;
> >> +
> >> +	if (!(info->flags & XENFB_FLAG_UPDATE))
> >> +		return;
> >> +	if (xenfb_queue_full(info))
> >> +		return;
> >> +
> >> +	y1 = info->y1;
> >> +	y2 = info->y2;
> >> +	x1 = info->x1;
> >> +	x2 = info->x2;
> >> +	info->y1 = info->y2 = info->x1 = info->x2 = 0;
> >> +	down(&info->mm_lock);
> >> +	list_for_each(item, &info->mappings) {
> >> +		map = list_entry(item, struct xenfb_mapping, next);
> > list_for_each_entry, perhaps?
> Going to try.
Thanks.
> >> > 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.
> >
> > It looks to me like we can lose updates to the screen in that case:
> >
> > xenfb_timer runs, sets dirty to 1, kicks the thread
> >                                         xenfb_thread wakes up
> >                                         xenfb_thread reads the update
region
> > another update happens, extends the update region, reschedules
xenfb_timer
> > xenfb_timer runs again, sets dirty to 2, kicks the thread again
> >                                         xenfb_thread sets dirty to 0
> >                                         xenfb_thread sends an update
using
> >                                                 the old region
> >
> > I suppose the race is sufficiently unlikely and sufficiently mild that
> > it can probably be ignored for all realistic purposes.
> This can''t happen, because both use and update of the mappings is
> protected by mm_lock:
No it isn''t.  Look at xenfb_update_screen: we read and reset the
update area, and then acquire the lock immediately afterwards.
> 
>   xenfb_timer runs, sets dirty to 1, kicks the thread
>                                 xenfb_thread wakes up 
>                                 xenfb_thread sets dirty to 0
> 
>                                 xenfb_thread down mm_lock
>                                 xenfb_thread reads the update region
>                                 xenfb_thread up mm_lock
> 
>                                 xenfb_thread sends an update using
>                                         the old region
> 
> Possibly concurrent:
> 
>   another update happens:
>   down mm_lock
>   extends the update region, reschedules xenfb_timer
>   up mm_lock
> 
>   xenfb_timer runs again, sets dirty to 1, kicks the thread again
> 
> Harmless race: dirty can conceivably become 1 when there''s no work
to
> do.  Then xenfb_thread wakes up, finds nothing to do and goes back to
> sleep.
> 
> Looks safe to me.
>
> >> +
> >> +static void xenfb_refresh(struct xenfb_info *info,
> >> +			  int x1, int y1, int w, int h)
> > Is there any reason why there can''t be multiple calls to this
function
> > live at the same time?  In particular, if vm_nopage and xenfb_copyarea
> > happen at the same time it looks like we can lose part of the update
> > area.
> >
> > Also, vm_nopage holds the mm_lock when it calls this, whereas the
> > other callers appear not to.  Was this deliberate?
> The question is what mm_lock protects.  If it protects info->mappings,
> then xenfb_refresh() doesn''t care about it, as it doesn''t
access it.
> 
> If it should protect x1, x2, y1, y2 as well, then locking looks
> flawed.
> 
> x1, x2, y1, y2 need protection if they can be updated concurrently.
> Users are xenfb_update_screen() and xenfb_refresh().  The latter runs
> from fb_ops methods and vm_operations_struct method nopage.  Looks
> like they need protection, doesn''t it?  Anthony?
Once you''ve figured this out, could you put a comment somewhere
sensible, please?
> >> +{
> >> +	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;
> >> +	}
> > Presumably, these are just looking to see if there''s a
pre-existing
> > region, and if not copying the new region across verbatim?  Could you
> > test dirty instead in that case?
> What about initializing x1, y1 with an impossibly small value, and x2,
> y2 with an impossibly large value?
If you do it the other way around, that would be ideal.
> >> +
> >> +	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);
> > Would it be sensible to have a fallback so that an update is always
> > sent to the backend e.g. 100ms after any change to the framebuffer, so
> > that the user still sees something when the display is being updated
> > very frequently?
> Could this be done in a later revision?  Happy to add the idea in a
> comment now.
xenfb_fps is only 20.  20fps is hardly an excessively fast animation,
so I''d expect the display to lock up under some reasonably common and
plausible situations with the current design.  That isn''t really good
enough.

It''s not a terribly hard thing to fix:

if (jiffies - info->last_update_sent > HZ/10)
	xenfb_update_screen(info)
else
	mod_timer(&info->refresh, jiffies + HZ/xenfb_fps);

and then add an

info->last_update_sent = jiffies;

to xenfb_update_screen.  You might need to move the locking around a
little in update_screen, and maybe move the dirty test in there from
xenfb_thread, but that''s about it.
> >> +	map_pages = (vma->vm_end - vma->vm_start + PAGE_SIZE-1)
>> PAGE_SHIFT;
> >> +	if (map_pages > info->nr_pages)
> >> +		goto out;
> >> +
> >> +	ret = -ENOMEM;
> >> +	map = kmalloc(sizeof(*map), GFP_KERNEL);
> >> +	if (map == NULL)
> >> +		goto out;
> >> +	memset(map, 0, sizeof(*map));
> > kzalloc, maybe?
> Sure.
Thanks.
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static unsigned long vmalloc_to_mfn(void *address)
> >> +{
> >> +	// FIXME was return pfn_to_mfn(vmalloc_to_pfn(address));
> > Not quite sure why this comment is here.
> Accident.  However, arbitrary_virt_to_machine() doesn''t appear to
> exist on ia64, so I went back to the old code.
Fair enough.
> >> +	return arbitrary_virt_to_machine(address) >> PAGE_SHIFT;
> >> +}
> >> +
> >> +static struct xenfb_info *xenfb_info;
> >> +
> >> +static int __init xenfb_probe(void)
> > I''d find it easier to be confident of the error handling here
if the
> > function were split up a little.
> It''s just the way I found it.  My own idea of cleanup would be to
make
> xenfb_cleanup() work for the error case.
That would also be a good thing.  It would be even better if you could
do both. :)
> >> +
> >> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> >> +	if (info == NULL)
> >> +		return -ENOMEM;
> >> +	memset(info, 0, sizeof(*info));
> >> +
> >> +	INIT_LIST_HEAD(&info->mappings);
> >> +
> >> +	info->fb = vmalloc(xenfb_mem_len);
> >> +	if (info->fb == NULL)
> >> +		goto error;
> >> +	memset(info->fb, 0, xenfb_mem_len);
> >> +	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >>
PAGE_SHIFT;
> >> +	info->pages = kmalloc(sizeof(struct page*)*info->nr_pages,
GFP_KERNEL);
> >> +	if (info->pages == NULL)
> >> +		goto error_vfree;
> >> +	for (i = 0; i < info->nr_pages; i++)
> >> +		info->pages[i] = vmalloc_to_page(info->fb + i *
PAGE_SIZE);
> >> +
> >> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
> >> +	// FIXME sizeof(struct xenfb_info)
> > Where did sizeof(u32) * 256 come from?  What does the comment mean?
> I *suspect* that the correct argument is sizeof(struct xenfb_info),
> but can''t yet exclude subtle, evil hackery going on here, so I put
in
> a comment for now.  Anthony, do you remember hackery here?
I think it''s actually the size of the pseudo palette, but I
wouldn''t
swear to it.
> > fb_info isn''t released from the error path.
> Fixing...
Thanks.
> >> +	if (fb_info == NULL)
> >> +		goto error_kfree;
> >> +
> >> +	info->mfns = vmalloc(sizeof(unsigned long) *
info->nr_pages);
> > What happens if this fails?  Also, you never seem to vfree this if we
> > hit an error later on.
> Fixing...
Thanks.
> >> +	/* set up shared page */
> >> +	info->page = (void *)__get_free_page(GFP_KERNEL);
> >> +	if (!info->page)
> >> +		goto error_kfree;
> >> +	/* set up event channel */
> >> +	alloc_unbound.dom = DOMID_SELF;
> >> +	alloc_unbound.remote_dom = 0;
> >> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
> >> +					  &alloc_unbound);
> >> +	if (ret)
> >> +		goto error_freep;
> >> +	info->evtchn = alloc_unbound.port;
> >> +
> >> +	for (i = 0; i < info->nr_pages; i++)
> >> +		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
> >> +	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
> > What happens if this domain is running in shadow translate mode?  It
> > looks to me like the backend will end up mapping completely the wrong
> > frame.
> Discussion above seems to suggest that our only option is to have the
> backend detect and translate shadow translate mode frontends, so no
> action is required here.  Correct?
Agreed.
> >> +	info->page->pd[1] = 0;
> >> +	info->page->width = XENFB_WIDTH;
> >> +	info->page->height = XENFB_HEIGHT;
> >> +	info->page->depth = XENFB_DEPTH;
> >> +	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) {
> >> +		close.port = info->evtchn;
> >> +		HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
> >> +		goto error_freep;
> >> +	}
> >> +
> >> +	info->irq = ret;
> >> +	xenfb_info = info;
> >> +
> >> +	fb_info->pseudo_palette = fb_info->par;
> >> +	fb_info->par = info;
Eh?  What was the reasoning behind this?
> >> +	fb_info->screen_base = info->fb;
> >> +
> >> +	memset(&fb_info->var, 0, sizeof(fb_info->var));
> >> +	memset(&fb_info->fix, 0, sizeof(fb_info->fix));
> > I think these are already guaranteed to be zero, aren''t they?
> Yes, framebuffer_alloc() clears *fb_info.  Happy to rely on that?
I can''t see any reason not to.
> >> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
> >> +	fb_info->fix.line_length = info->page->line_length;
> >> +	fb_info->fix.smem_start = 0;
> >> +	fb_info->fix.smem_len = xenfb_mem_len;
> >> +	strcpy(fb_info->fix.id, "xen");
> >> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
> >> +	fb_info->fix.accel = FB_ACCEL_NONE;
> >> +
> >> +	fb_info->flags = FBINFO_FLAG_DEFAULT;
> >> +
> >> +	fb_alloc_cmap(&fb_info->cmap, 256, 0);
> > This can fail, and the error paths don''t bother to deallocate
it.
> Fixing...
Thanks.
> >> +
> >> + again:
> >> +	ret = xenbus_transaction_start(&xbt);
> >> +	if (ret)
> >> +		goto error_unreg;
> >> +	ret = xenbus_printf(xbt, "vfb", "page-ref",
"%lu",
> >> +			    virt_to_mfn(info->page));
> >> +	// FIXME grant tables?
> > Again, what happens in shadow translate mode?
> >
> > Also, it''s kind of annoying that there''s no support
for multiple
> > framebuffers in a domU.  That''s very much a wishlist thing,
though.
> See above.
Ack.
> >> +	unregister_framebuffer(fb_info);
> >> + error_unbind:
> >> +	unbind_from_irqhandler(info->irq, info);
> >> +	// FIXME do we have to stop info->kthread?
> > If you''ve started it you certainly need to stop it.
> Fixing...
Thanks.
> >> +	return -ENODEV;
> >> +}
> >> +
> >> +static int __init xenfb_init(void)
> >> +{
> >> +	return xenfb_probe();
> >
> > From last time:
> >
> >> > 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?
> > I think if the frontend needs to know whether the backend is in kernel
> > or userspace then we''ve messed up somewhere.  Why do you
expect this
> > to be difficult?
> I didn''t *expect* it to be difficult, I just couldn''t see
how to fit
> Anthony''s code to xenbus_device without rewriting more than I
like,
> and I still can''t see it.  That''s why I asked for advice.
The probing stuff seems to be fairly self-contained at the moment.
You''d probably have to rewrite xenfb_probe, but I can''t see
any need
to change much beyond that.

Unless I''m just missing something?
> Anyway, I''m reluctant to throw out working code while
there''s so much
> code that needs fixing.  I may be less reluctant once we''ve
drained
> the swamp some more.
So I think long term we''d probably like to use the standard xenbus
device setup protocol, going through XenbusStateInitialised,
XenbusStateConnected, XenbusStateClosing, etc.  That''d be a
non-backwards-compatible change if we do it later, so it''d be good to
get it in now.  xenbus_device gives you that for free, although I''ll
admit it is a bit of a pain to use.
> >> +}
> >> +
> >> +static void __exit xenfb_cleanup(void)
> >> +{
> >> +	struct xenfb_info *info = xenfb_info;
> >> +
> >> +	unregister_framebuffer(info->fb_info);
> >> +	unbind_from_irqhandler(info->irq, info);
> >> +	free_page((unsigned long)info->page);
> >> +	kfree(info->pages);
> >> +	vfree(info->fb);
> >> +	kfree(info);
> >> +	xenfb_info = NULL;
> > I don''t think this releases all of the memory you allocate in
> > xenfb_probe.
> Fixing...
Thanks.
> >> +	  return -ENODEV;
> >> +#if 0
> >> +        /* if we''re not set up to use graphics mode,
then don''t initialize */
> >> +	if (xenbus_scanf(XBT_NIL, "console",
"use_graphics", "%d", &ret) < 0)
> >> +	  return -ENODEV;
> >> +	if (ret == 0)
> >> +	  return -ENODEV;
> >> +#endif
> > Not sure about this.
> Same as for xenfb.c.  The #if is an accident.
Ack.
> >> +
> >> +	dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> >> +	input_dev = input_allocate_device();
> >> +	if (!dev || !input_dev)
> >> +		return -ENOMEM;
> > You potentially leak memory if one of these fails and the other
succeeds.
> >
> > Also, the error paths don''t release input_dev.  In fact,
nothing ever
> > releases input_dev, unless I''m missing something.
> Fixing...
Thanks.
> >> +
> >> +	dev->dev = input_dev;
> >> +	dev->info = (void *)__get_free_page(GFP_KERNEL);
> >> +	if (!dev->info) {
> >> +		ret = -ENOMEM;
> >> +		goto error;
> >> +	}
> >> +
> >> +	alloc_unbound.dom = DOMID_SELF;
> >> +	alloc_unbound.remote_dom = 0;
> >> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
> >> +					  &alloc_unbound);
> >> +	if (ret)
> >> +		goto error_freep;
> >> +	dev->evtchn = alloc_unbound.port;
> >> +	ret = bind_evtchn_to_irqhandler(dev->evtchn, input_handler,
0,
> >> +					"xenkbd", dev);
> >> +	if (ret < 0)
> >> +		goto error_freep;
> > You might want to close the event channel here.
> Fixing...
Thanks.
> >> +
> >> +	input_dev->name = "Xen Virtual Keyboard/Mouse";
> >> +
> >> +	input_register_device(input_dev);
> > This can fail.
> Most uses elsewhere in the kernel don''t care.  Fixing anyway.
Yeah, I know, but it''s good to get it right anyway.  Thanks.
> >> +
> >> + again:
> >> +	ret = xenbus_transaction_start(&xbt);
> >> +	if (ret)
> >> +		goto error_unreg;
> >> +	ret = xenbus_printf(xbt, "vkbd", "page-ref",
"%lu",
> >> +			    virt_to_mfn(dev->info));
> > There''s no reason that I can see not to use grant tables
here, and
> > that''d avoid all of the potential issues with shadow
translate mode.
> Since we have to fool around with shadow translate mode anyway for
> sharing the framebuffer, I don''t quite see why we need to address
this
> right now.  This code works, and there''s no shortage of code that
> needs fixing.
Alright, leave this for now and fix it when you fix the vfb equivalent
bits.
> Same for vfb/page-ref.
Well, that''s a reference to a bunch of untranslated frame numbers, so
it makes more sense for it to be untranslated itself.  The keyboard
page doesn''t require any further translation.
> >> +MODULE_LICENSE("GPL");
> >> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02
15:11:17 2006 -0400
> >
> > This is defining an io protocol, so belongs in xen/include/public/io
> Okay.
Thanks.
> >> +#include <asm/types.h>
> >> +
> >> +/* out events */
> >> +
> >> +#define XENFB_OUT_EVENT_SIZE 40
> >> +
> >> +#define XENFB_TYPE_MOTION 1
> >> +#define XENFB_TYPE_UPDATE 2
> >> +
> > The other tagged unions in Xen look more like this:
> >
> > struct {
> > 	u8 type;
> > 	union {
> > 		struct {
> > 			u32 field1;
> > 			u32 field2;
> > 		} type1;
> > 		struct {
> > 			u16 field1;
> > 		} type2;
> > 	} u;
> > };
> >
> > I''d find this a lot easier to read than the current approach
of
> > putting the type into the individual event type structures.  Is there
> > any reason why this couldn''t be used here?
> Matter of taste.  Will change it if you insist.
It''d be good to be consistent with the other protocols.
> >> +struct xenfb_motion	    /* currently unused */
> >> +{
> >> +	__u8 type;          /* XENFB_TYPE_MOTION */
> >> +	__u16 x;            /* The new x coordinate */
> >> +	__u16 y;            /* The new y coordinate */
> >> +};
> > I think if the comment said what was moving this''d be a lot
clearer.
> I''m not into documenting unused code I didn''t write and
don''t
> understand, so I''m going to remove this.
Even better.
> >> +
> >> +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;
> >> +	struct xenfb_motion motion;
> >> +	struct xenfb_update update;
> >> +	char _[XENFB_OUT_EVENT_SIZE];
> > Perhaps ``pad'''' would be clearer than
``_''''?
> Works for me.
Thanks.
> >> +struct xenfb_page
> >> +{
> >> +	__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) */
> >> +
> >> +	unsigned long pd[2];	/* FIXME rename to pgdir? */
> >> +	/* FIXME pd[1] unused at this time, shrink? */
> > I think it''d be enough to just give some indication of where
the magic
> > number 2 came from.
> Okay.
Thanks.
> >> +
> >> +	__u32 in_cons, in_prod;
> >> +	__u32 out_cons, out_prod;
> >> +};
> >> +
> >> +void xenfb_resume(void);
> > No longer exists.
> Removed.
Thanks.
> >> +
> >> +#endif
> >> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02
15:11:17 2006 -0400
> > Again, this belongs in xen/include/public/io.
> Okay.
Thanks.
> >> +union xenkbd_in_event
> >> +{
> >> +	__u8 type;
> >> +	struct xenkbd_motion motion;
> >> +	struct xenkbd_button button;
> >> +	struct xenkbd_key key;
> >> +	char _[XENKBD_IN_EVENT_SIZE];
> >> +};
> >> +
> >> +/* out events */
> > Maybe a comment that none are currently defined?
> Sure.
Thanks.
> >> +struct xenkbd_info
> >> +{
> >> +	__u32 in_cons, in_prod;
> >> +	__u32 out_cons, out_prod;
> >> +};
> >> +
> >> +void xenkbd_resume(void);
> > No longer exists.
> Removed.
Thanks.

Steven.


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

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:
> On 6/9/06 10:11 am, "Steven Smith"
<sos22-xen@srcf.ucam.org> wrote:
>
>>> I wouldn''t complain if we go the route Keir suggested and
have Xen do
>>> it ;-)  But if not, I think that given the size of the memory in
>>> question + grant tables, the quick hack is probably going to be the
>>> "right" thing for this case.
The quick hack being translating mfns in the backend.
>> Okay.  Adding translations to the hypervisor isn''t going to
happen
>> until after 3.0.3, so we should probably just leave this for a little
>> while.
>
> Another option after 3.0.3 is to allow grant tables to support these large
> mappings. I have some ideas how to do this fairly efficiently, given that
> it''ll be okay to track mappings to all the pages as an aggregate.
It''s not
> going to happen for now though -- it''ll just be a shame if we add
> grant-table support later that it''ll change the setup protocol.
However, we
> can probably maintain backward compatibility if we think about it a bit.
>
>  -- Keir
Is there anything we can do now to help with maintaining backward
compatibility later?

Evolving interfaces are a fact of life.  What about versioning?
Frontend puts its interface version in xenstore, bump it when we
change stuff (which should happen very rarely, of course), backend
queries the version and does the right thing.

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

On 21/9/06 7:41 pm, "Markus Armbruster" <armbru@redhat.com>
wrote:
> Is there anything we can do now to help with maintaining backward
> compatibility later?
> 
> Evolving interfaces are a fact of life.  What about versioning?
> Frontend puts its interface version in xenstore, bump it when we
> change stuff (which should happen very rarely, of course), backend
> queries the version and does the right thing.
We don''t need to think too hard about this right now. The way we do
this for
netfront/netback is to add feature nodes to xenstore. Absence of a feature
node (e.g., because backend is too old to support the feature) means the
feature is unavailable to the frontend and it will fall back to old methods.
In some cases feature negotiation is a handshake (backend writes a feature
node to indicate it supports a feature; frontend writes a request node to
indicate it wants to use a supported feature).

 -- Keir



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

> > Another option after 3.0.3 is to allow grant tables to support these
large
> > mappings. I have some ideas how to do this fairly efficiently, given
that
> > it''ll be okay to track mappings to all the pages as an
aggregate. It''s not
> > going to happen for now though -- it''ll just be a shame if we
add
> > grant-table support later that it''ll change the setup
protocol. However, we
> > can probably maintain backward compatibility if we think about it a
bit.
> Is there anything we can do now to help with maintaining backward
> compatibility later?
> 
> Evolving interfaces are a fact of life.  What about versioning?
> Frontend puts its interface version in xenstore, bump it when we
> change stuff (which should happen very rarely, of course), backend
> queries the version and does the right thing.
If we''re going to do this, it''d probably be worth exporting a
backend
version number as well.

Even better would be having a bunch of separate feature flags, like we
do now for the different netif protocols.  You might, for instance,
have backends which can use grant tables export feature-large-grants
to their area of xenbus, and then have frontends notice that and set
request-large-grants in their area if they support it.  If the
relevant nodes aren''t present, you conclude that the other end either
doesn''t support the feature or doesn''t want to use it for some
reason.

This has a couple of advantages over a pure version number scheme:

-- If either end doesn''t like the new protocol, it''s trivial
to make
   it fall back to the old one.  We might want to kill the old
   protocol eventually, but this at least makes the transition a bit
   easier.
-- You don''t need to write any code now.
-- I think it''s a bit cleaner than potentially artificially tying
   features together.

Thoughts?

Steven.


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

Steven Smith <sos22@cam.ac.uk> writes:
>> > Okay, this fixes a lot of the things I complained about last time,
and
>> > most of the new problems should be fairly easy to fix up.
>> >
>> > There are still a couple of things from the last round which are
>> > unfixed.  This is mostly my fault for going on holiday at an
>> > inconvenient time; sorry about that.
>> I''m about to commit the same crime :)  Expect me back on the
19th.
> Have fun.
>
>> > The big problem with this is that you don''t support
shadow translate
>> > mode guests, because the frontend exposes mfns directly to the
>> > backend.  The correct way of fixing this is to use grant tables,
but,
>> > as Anthony pointed out, that''s quite hard due to the
amount of memory
>> > you need to grant access to.
>> >
>> > The easy way of doing this would be to just increase the size of
the
>> > grant tables.  Alternatively, you could add support for granting
>> > access to ranges of memory, but that sounds like a whole lot of
work.
>> Yes, it would be great if Xen provided the means to do it right, but
>> as Keir wrote that''s post 3.0.3.
>> 
>> > You could also put in a quick hack of just translating the mfns in
the
>> > backend if the frontend is in shadow translate mode, but
that''s not
>> > really very nice.
>> As far as I can see, this is our only option right now.  I''m
ignorant
>> of `shadow translate mode''; can somebody please educate me on
how to
>> detect and translate shadow translate mode frontend mfns in the
>> backend?
> Shadow translate mode is a mode of the shadow page tables in which Xen
> does machine<->physical address translations on behalf of the guest.
> It''s mostly used for HVM guests to give them the impression of a
> mostly-contiguous block of memory starting at machine address 0.
How mostly is `mostly used for HVM guests''?  Is it actually used with
PV guests?  If not, coping with it is unnecessarily general --- I''d
rather just detect it (just in case) and bail out.
> If you just bung the guest-supplied machine frame numbers through
> xc_translate_gpfn_list you should be fine.  That does the translation
> if you''re in shadow translate mode and does nothing if
you''re not.
You mean xc_domain_translate_gpfn_list(), I suppose.
>> > It''d also be nice if the protocol supported putting the
backend in an
>> > unprivileged domain, which grant tables would give you for free,
but
>> > that''s a lower priority.
>> I guess that has to wait until we can do the framebuffer mapping
>> nicely.
> Yep.
>
>> > Aside from that, the lack of support for multiple framebuffers in
one
>> > guest is kind of annoying.  It''s not necessary to have
this working in
>> > a first cut of the patch, but it''d be good if the
protocol included
>> > some way of adding it in later, which the current one
doesn''t.
>> Well, the only place that limits us to a single framebuffer is our use
>> of xenstore, isn''t it?.  All we need for additional
framebuffers then
>> is new xenstore directories, say vfbN and vkbdN, where N is 0, 1, ...
>> 
>> If you feel it''s useful, I can rename the xenstore directories
to vfb0
>> and vkbd0 now.
> Yes, please.  It''d be good not to have to break the protocol in
order
> to add this in the future.  I''d prefer vfb/0 and vkbd/0, though,
for
> consistency with other types of devices.
Done.
>> > Hard coding the resolution is also kind of icky, but is again
>> > acceptable for a first cut.  It''d be good to have at
least a plan for
>> > how this is supposed to work before the thing gets merged,
including
>> > how to change the resolution of a live framebuffer.
>> It''s always good to have a plan for future work.  Note,
however, that
>> the protocol is easily extensible, and therefore we don''t have
to fear
>> coding ourselves into a box now that we have to break later, just
>> for lack of foresight.
> True.
>
>> > Also, the spec you pointed me at last time appears to be out of
date:
>> > it still thinks you get the pgdir mfn from start info, which
certainly
>> > isn''t acceptable for new drivers.
>> Yes, it''s out of date.  Let''s nail down things here,
and if we then
>> feel the spec is useful to have, we update it.
> Good plan.
>
>> >>  /*
>> >>   * Do a quick page-table lookup for a single page.
>> >> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile
>> >> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> >> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/Makefile	Sat Sep
02 15:11:17 2006 -0400
>> >> @@ -0,0 +1,1 @@
>> >> +obj-$(CONFIG_XEN_FRAMEBUFFER)	:= xenfb.o
>> >> diff -r 5fa9b746d24f -r 510c6bceb87f
linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c
>> >> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> >> +++ b/linux-2.6-xen-sparse/drivers/xen/xenfb/xenfb.c	Sat Sep
02 15:11:17 2006 -0400
>> >> @@ -0,0 +1,571 @@
>> >> +/*
>> >> + * linux/drivers/video/xenfb.c -- Xen para-virtual frame
buffer device
>> >> + *
>> >> + * Copyright (C) 2005-2006
>> >> + *
>> >> + *      Anthony Liguori <aliguori@us.ibm.com>
>> >> + *
>> >> + *  Based on linux/drivers/video/q40fb.c
>> >> + *
>> >> + *  This file is subject to the terms and conditions of the
GNU General Public
>> >> + *  License. See the file COPYING in the main directory of
this archive for
>> >> + *  more details.
>> >> + */
>> >> +
>> >> +#include <linux/kernel.h>
>> >> +#include <linux/errno.h>
>> >> +#include <linux/fb.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/vmalloc.h>
>> >> +#include <linux/mm.h>
>> >> +#include <asm/hypervisor.h>
>> >> +#include <xen/evtchn.h>
>> >> +#include <xen/xenbus.h>
>> >> +#include <linux/xenfb.h>
>> >> +#include <linux/kthread.h>
>> >> +
>> >> +#define XENFB_WIDTH 800
>> >> +#define XENFB_HEIGHT 600
>> >> +#define XENFB_DEPTH 32
>> >> +
>> >> +static int xenfb_fps = 20;
>> >> +static unsigned long xenfb_mem_len = XENFB_WIDTH *
XENFB_HEIGHT * XENFB_DEPTH / 8;
>> >> +
>> >> +struct xenfb_mapping
>> >> +{
>> >> +	struct list_head	next;
>> > Perhaps not the best name for a list head?
>> The mappings are kept in a list of struct xenfb_mapping, chained
>> through member next.  I''ve seen worse names.  Feel free to
suggest a
>> better one.
> Well, list_heads are double linked, so next is kind of deceptive.
> Perhaps just ``list''''?
Renamed to link.
>> >> +	struct vm_area_struct	*vma;
>> >> +	atomic_t		map_refs;
>> >> +	int			faults;
>> >> +	struct xenfb_info	*info;
>> >> +};
>> >> +
>> >> +struct xenfb_info
>> >> +{
>> >> +	struct task_struct		*kthread;
>> >> +	wait_queue_head_t		wq;
>> >> +
>> >> +	unsigned char			*fb;
>> > Why unsigned char?
>> I think this is a matter of style.  Plain char suggests characters,
>> while unsigned char suggests bytes.  fb points to an array of bytes.
> I''d have gone for void *, but, as you say, matter of style.
>
>> >> +	struct fb_fix_screeninfo	*fix;
>> >> +	struct fb_var_screeninfo	*var;
>> > Not quite sure what these are for.  Why not just get at them
through
>> > the fb_info pointer?
>> Fine with me.
> Thanks.
>
>> >> +	struct fb_info			*fb_info;
>> >> +	struct timer_list		refresh;
>> >> +	int				dirty;
>> >
>> >> +	int				y1, y2;
>> >> +	int				x1, x2;
>> > These could perhaps do with slightly more descriptive names?
>> Would you settle for a descriptive comment instead?
> Sure.
Done.
>> >> +
>> >> +	struct semaphore		mm_lock;
>> > That''s going to confuse people.
>> >
>> > (Me, for a start)
>> What exactly confuses you?  The name?  Could rename to mm_sem.
> Sorry, got my wires crossed a bit here.  Solaris has a global mm_lock,
> and Linux used to, and I thought Linux still did.  Ignore me, I''m
> wibbling.
>
>> >> +	int				nr_pages;
>> >> +	struct page			**pages;
>> >> +	struct list_head		mappings;
>> >> +
>> >> +	unsigned			evtchn;
>> >> +	int				irq;
>> >> +	struct xenfb_page		*page;
>> >> +	unsigned long 			*mfns;
>> >> +	u32				flags;
>> > Maybe a comment to say what sort of flags go in here?
>> Sure.
> Thanks.
>
>> >> +};
>> >> +
>> >> +static void xenfb_do_update(struct xenfb_info *info,
>> >> +			    int x, int y, int w, int h)
>> >> +{
>> >> +	union xenfb_out_event event;
>> >> +	__u32 prod;
>> >> +
>> >> +	event.type = XENFB_TYPE_UPDATE;
>> >> +	event.update.x = x;
>> >> +	event.update.y = y;
>> >> +	event.update.width = w;
>> >> +	event.update.height = h;
>> >> +
>> >> +	prod = info->page->out_prod;
>> >> +	if (prod - info->page->out_cons == XENFB_OUT_RING_LEN)
>> >> +		return;		/* ring buffer full, event lost */
>> > It seems to me like if we lose the event, we probably
shouldn''t mark
>> > the framebuffer as clean and throw away the update region. 
Perhaps
>> > the caller should instead reschedule the timer?
>> Actually, this can''t happen.  xenfb_update_screen() ensures
there''s
>> space in the ring before it proceeds to walk the mappings.  That space
>> can''t go away, because no other place puts stuff into the
ring, and
>> only one instance of xenfb_do_update() can run (in the kernel thread).
> Okay.
>
>> > Also, is there any reason the test couldn''t use
xenfb_queue_full?
>> I removed the redundant test instead.
> Thanks.
>
>> >> +	mb();			/* ensure ring space available */
>> >> +	XENFB_OUT_RING_REF(info->page, prod) = event;
>> >> +	wmb();			/* ensure ring contents visible */
>> >> +	info->page->out_prod = prod + 1;
>> >> +
>> >> +	notify_remote_via_evtchn(info->evtchn);
>> >> +}
>> >> +
>> >> +static int xenfb_queue_full(struct xenfb_info *info)
>> >> +{
>> >> +	__u32 cons, prod;
>> >> +
>> >> +	prod = info->page->out_prod;
>> >> +	cons = info->page->out_cons;
>> >> +	return prod - cons == XENFB_OUT_RING_LEN;
>> >> +}
>> >> +
>> >> +static void xenfb_update_screen(struct xenfb_info *info)
>> >> +{
>> >> +	int y1, y2, x1, x2;
>> >> +	struct list_head *item;
>> >> +	struct xenfb_mapping *map;
>> >> +
>> >> +	if (!(info->flags & XENFB_FLAG_UPDATE))
>> >> +		return;
>> >> +	if (xenfb_queue_full(info))
>> >> +		return;
>> >> +
>> >> +	y1 = info->y1;
>> >> +	y2 = info->y2;
>> >> +	x1 = info->x1;
>> >> +	x2 = info->x2;
>> >> +	info->y1 = info->y2 = info->x1 = info->x2 = 0;
>> >> +	down(&info->mm_lock);
>> >> +	list_for_each(item, &info->mappings) {
>> >> +		map = list_entry(item, struct xenfb_mapping, next);
>> > list_for_each_entry, perhaps?
>> Going to try.
> Thanks.
Done.
>> >> > 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.
>> >
>> > It looks to me like we can lose updates to the screen in that
case:
>> >
>> > xenfb_timer runs, sets dirty to 1, kicks the thread
>> >                                         xenfb_thread wakes up
>> >                                         xenfb_thread reads the
update region
>> > another update happens, extends the update region, reschedules
xenfb_timer
>> > xenfb_timer runs again, sets dirty to 2, kicks the thread again
>> >                                         xenfb_thread sets dirty to
0
>> >                                         xenfb_thread sends an
update using
>> >                                                 the old region
>> >
>> > I suppose the race is sufficiently unlikely and sufficiently mild
that
>> > it can probably be ignored for all realistic purposes.
>> This can''t happen, because both use and update of the mappings
is
>> protected by mm_lock:
> No it isn''t.  Look at xenfb_update_screen: we read and reset the
> update area, and then acquire the lock immediately afterwards.
See below.
>> 
>>   xenfb_timer runs, sets dirty to 1, kicks the thread
>>                                 xenfb_thread wakes up 
>>                                 xenfb_thread sets dirty to 0
>> 
>>                                 xenfb_thread down mm_lock
>>                                 xenfb_thread reads the update region
>>                                 xenfb_thread up mm_lock
>> 
>>                                 xenfb_thread sends an update using
>>                                         the old region
>> 
>> Possibly concurrent:
>> 
>>   another update happens:
>>   down mm_lock
>>   extends the update region, reschedules xenfb_timer
>>   up mm_lock
>> 
>>   xenfb_timer runs again, sets dirty to 1, kicks the thread again
>> 
>> Harmless race: dirty can conceivably become 1 when there''s no
work to
>> do.  Then xenfb_thread wakes up, finds nothing to do and goes back to
>> sleep.
>> 
>> Looks safe to me.
>>
>> >> +
>> >> +static void xenfb_refresh(struct xenfb_info *info,
>> >> +			  int x1, int y1, int w, int h)
>> > Is there any reason why there can''t be multiple calls to
this function
>> > live at the same time?  In particular, if vm_nopage and
xenfb_copyarea
>> > happen at the same time it looks like we can lose part of the
update
>> > area.
>> >
>> > Also, vm_nopage holds the mm_lock when it calls this, whereas the
>> > other callers appear not to.  Was this deliberate?
>> The question is what mm_lock protects.  If it protects
info->mappings,
>> then xenfb_refresh() doesn''t care about it, as it
doesn''t access it.
>> 
>> If it should protect x1, x2, y1, y2 as well, then locking looks
>> flawed.
>> 
>> x1, x2, y1, y2 need protection if they can be updated concurrently.
>> Users are xenfb_update_screen() and xenfb_refresh().  The latter runs
>> from fb_ops methods and vm_operations_struct method nopage.  Looks
>> like they need protection, doesn''t it?  Anthony?
> Once you''ve figured this out, could you put a comment somewhere
> sensible, please?
In Anthony''s code, mm_lock protects info->mapping.

However, the dirty rectangle x1, y1, x2, y2 needs protection as well,
because it''s written from multiple threads.  It could use a separate
lock, but that doesn''t seem to be worth the trouble, so I simply use
mm_lock.
>> >> +{
>> >> +	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;
>> >> +	}
>> > Presumably, these are just looking to see if there''s a
pre-existing
>> > region, and if not copying the new region across verbatim?  Could
you
>> > test dirty instead in that case?
>> What about initializing x1, y1 with an impossibly small value, and x2,
>> y2 with an impossibly large value?
> If you do it the other way around, that would be ideal.
Done.
>> >> +
>> >> +	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);
>> > Would it be sensible to have a fallback so that an update is
always
>> > sent to the backend e.g. 100ms after any change to the
framebuffer, so
>> > that the user still sees something when the display is being
updated
>> > very frequently?
>> Could this be done in a later revision?  Happy to add the idea in a
>> comment now.
>
> xenfb_fps is only 20.  20fps is hardly an excessively fast animation,
> so I''d expect the display to lock up under some reasonably common
and
> plausible situations with the current design.  That isn''t really
good
> enough.
>
> It''s not a terribly hard thing to fix:
>
> if (jiffies - info->last_update_sent > HZ/10)
> 	xenfb_update_screen(info)
> else
> 	mod_timer(&info->refresh, jiffies + HZ/xenfb_fps);
>
> and then add an
>
> info->last_update_sent = jiffies;
>
> to xenfb_update_screen.  You might need to move the locking around a
> little in update_screen, and maybe move the dirty test in there from
> xenfb_thread, but that''s about it.
I reread Anthony''s code, and I believe it''s is just fine.

When the first dirt arrives, the timer is not pending, so it gets
modified to fire in 1/xenfb_fps seconds.

If more dirt arrives before that time, the timer is pending, and
therefore does not get modified.

When the timer fires, xenfb_timer() runs and wakes the refresh thread.
This happens about 1/xenfb_fps seconds after the first dirt arrived.

When the next dirt arrives, the timer is again not pending, and my
argument loops.

If dirt arrives continuously, the timer fires approximately xenfb_fps
times per second.
>> >> +	map_pages = (vma->vm_end - vma->vm_start +
PAGE_SIZE-1) >> PAGE_SHIFT;
>> >> +	if (map_pages > info->nr_pages)
>> >> +		goto out;
>> >> +
>> >> +	ret = -ENOMEM;
>> >> +	map = kmalloc(sizeof(*map), GFP_KERNEL);
>> >> +	if (map == NULL)
>> >> +		goto out;
>> >> +	memset(map, 0, sizeof(*map));
>> > kzalloc, maybe?
>> Sure.
> Thanks.
>
>> >> +	return IRQ_HANDLED;
>> >> +}
>> >> +
>> >> +static unsigned long vmalloc_to_mfn(void *address)
>> >> +{
>> >> +	// FIXME was return pfn_to_mfn(vmalloc_to_pfn(address));
>> > Not quite sure why this comment is here.
>> Accident.  However, arbitrary_virt_to_machine() doesn''t appear
to
>> exist on ia64, so I went back to the old code.
> Fair enough.
>
>> >> +	return arbitrary_virt_to_machine(address) >>
PAGE_SHIFT;
>> >> +}
>> >> +
>> >> +static struct xenfb_info *xenfb_info;
>> >> +
>> >> +static int __init xenfb_probe(void)
>> > I''d find it easier to be confident of the error handling
here if the
>> > function were split up a little.
>> It''s just the way I found it.  My own idea of cleanup would be
to make
>> xenfb_cleanup() work for the error case.
> That would also be a good thing.  It would be even better if you could
> do both. :)
All cleanup is now in xenfb_cleanup().

On splitting xenfb_probe(): one step at a time.  If we aim for
perfection in the initial revision, we''ll never get it done.
>> >> +
>> >> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
>> >> +	if (info == NULL)
>> >> +		return -ENOMEM;
>> >> +	memset(info, 0, sizeof(*info));
>> >> +
>> >> +	INIT_LIST_HEAD(&info->mappings);
>> >> +
>> >> +	info->fb = vmalloc(xenfb_mem_len);
>> >> +	if (info->fb == NULL)
>> >> +		goto error;
>> >> +	memset(info->fb, 0, xenfb_mem_len);
>> >> +	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >>
PAGE_SHIFT;
>> >> +	info->pages = kmalloc(sizeof(struct
page*)*info->nr_pages, GFP_KERNEL);
>> >> +	if (info->pages == NULL)
>> >> +		goto error_vfree;
>> >> +	for (i = 0; i < info->nr_pages; i++)
>> >> +		info->pages[i] = vmalloc_to_page(info->fb + i *
PAGE_SIZE);
>> >> +
>> >> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
>> >> +	// FIXME sizeof(struct xenfb_info)
>> > Where did sizeof(u32) * 256 come from?  What does the comment
mean?
>> I *suspect* that the correct argument is sizeof(struct xenfb_info),
>> but can''t yet exclude subtle, evil hackery going on here, so I
put in
>> a comment for now.  Anthony, do you remember hackery here?
> I think it''s actually the size of the pseudo palette, but I
wouldn''t
> swear to it.
I think you''re right.  See `evil hackery'' below.
>> > fb_info isn''t released from the error path.
>> Fixing...
> Thanks.
>
>> >> +	if (fb_info == NULL)
>> >> +		goto error_kfree;
>> >> +
>> >> +	info->mfns = vmalloc(sizeof(unsigned long) *
info->nr_pages);
>> > What happens if this fails?  Also, you never seem to vfree this if
we
>> > hit an error later on.
>> Fixing...
> Thanks.
>
>> >> +	/* set up shared page */
>> >> +	info->page = (void *)__get_free_page(GFP_KERNEL);
>> >> +	if (!info->page)
>> >> +		goto error_kfree;
>> >> +	/* set up event channel */
>> >> +	alloc_unbound.dom = DOMID_SELF;
>> >> +	alloc_unbound.remote_dom = 0;
>> >> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
>> >> +					  &alloc_unbound);
>> >> +	if (ret)
>> >> +		goto error_freep;
>> >> +	info->evtchn = alloc_unbound.port;
>> >> +
>> >> +	for (i = 0; i < info->nr_pages; i++)
>> >> +		info->mfns[i] = vmalloc_to_mfn(info->fb + i *
PAGE_SIZE);
>> >> +	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
>> > What happens if this domain is running in shadow translate mode? 
It
>> > looks to me like the backend will end up mapping completely the
wrong
>> > frame.
>> Discussion above seems to suggest that our only option is to have the
>> backend detect and translate shadow translate mode frontends, so no
>> action is required here.  Correct?
> Agreed.
>
>> >> +	info->page->pd[1] = 0;
>> >> +	info->page->width = XENFB_WIDTH;
>> >> +	info->page->height = XENFB_HEIGHT;
>> >> +	info->page->depth = XENFB_DEPTH;
>> >> +	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) {
>> >> +		close.port = info->evtchn;
>> >> +		HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
>> >> +		goto error_freep;
>> >> +	}
>> >> +
>> >> +	info->irq = ret;
>> >> +	xenfb_info = info;
>> >> +
>> >> +	fb_info->pseudo_palette = fb_info->par;
>> >> +	fb_info->par = info;
> Eh?  What was the reasoning behind this?
I have no idea.  Ugly, evil hackery.  The only excuse I can see is
that it works.  Hope to slay it in a future revision.
>> >> +	fb_info->screen_base = info->fb;
>> >> +
>> >> +	memset(&fb_info->var, 0, sizeof(fb_info->var));
>> >> +	memset(&fb_info->fix, 0, sizeof(fb_info->fix));
>> > I think these are already guaranteed to be zero, aren''t
they?
>> Yes, framebuffer_alloc() clears *fb_info.  Happy to rely on that?
> I can''t see any reason not to.
Done.
>> >> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
>> >> +	fb_info->fix.line_length = info->page->line_length;
>> >> +	fb_info->fix.smem_start = 0;
>> >> +	fb_info->fix.smem_len = xenfb_mem_len;
>> >> +	strcpy(fb_info->fix.id, "xen");
>> >> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
>> >> +	fb_info->fix.accel = FB_ACCEL_NONE;
>> >> +
>> >> +	fb_info->flags = FBINFO_FLAG_DEFAULT;
>> >> +
>> >> +	fb_alloc_cmap(&fb_info->cmap, 256, 0);
>> > This can fail, and the error paths don''t bother to
deallocate it.
>> Fixing...
> Thanks.
>
>> >> +
>> >> + again:
>> >> +	ret = xenbus_transaction_start(&xbt);
>> >> +	if (ret)
>> >> +		goto error_unreg;
>> >> +	ret = xenbus_printf(xbt, "vfb",
"page-ref", "%lu",
>> >> +			    virt_to_mfn(info->page));
>> >> +	// FIXME grant tables?
>> > Again, what happens in shadow translate mode?
>> >
>> > Also, it''s kind of annoying that there''s no
support for multiple
>> > framebuffers in a domU.  That''s very much a wishlist
thing, though.
>> See above.
> Ack.
>
>> >> +	unregister_framebuffer(fb_info);
>> >> + error_unbind:
>> >> +	unbind_from_irqhandler(info->irq, info);
>> >> +	// FIXME do we have to stop info->kthread?
>> > If you''ve started it you certainly need to stop it.
>> Fixing...
> Thanks.
>
>> >> +	return -ENODEV;
>> >> +}
>> >> +
>> >> +static int __init xenfb_init(void)
>> >> +{
>> >> +	return xenfb_probe();
>> >
>> > From last time:
>> >
>> >> > 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?
>> > I think if the frontend needs to know whether the backend is in
kernel
>> > or userspace then we''ve messed up somewhere.  Why do you
expect this
>> > to be difficult?
>> I didn''t *expect* it to be difficult, I just couldn''t
see how to fit
>> Anthony''s code to xenbus_device without rewriting more than I
like,
>> and I still can''t see it.  That''s why I asked for
advice.
> The probing stuff seems to be fairly self-contained at the moment.
> You''d probably have to rewrite xenfb_probe, but I can''t
see any need
> to change much beyond that.
>
> Unless I''m just missing something?
>
>> Anyway, I''m reluctant to throw out working code while
there''s so much
>> code that needs fixing.  I may be less reluctant once we''ve
drained
>> the swamp some more.
> So I think long term we''d probably like to use the standard xenbus
> device setup protocol, going through XenbusStateInitialised,
> XenbusStateConnected, XenbusStateClosing, etc.  That''d be a
> non-backwards-compatible change if we do it later, so it''d be good
to
> get it in now.  xenbus_device gives you that for free, although
I''ll
> admit it is a bit of a pain to use.
We''ll have to break backwards compatibility anyway when we switch to
grant tables.  Why dig up the same code twice, and in the meantime
deprive users of the code that works?
>> >> +}
>> >> +
>> >> +static void __exit xenfb_cleanup(void)
>> >> +{
>> >> +	struct xenfb_info *info = xenfb_info;
>> >> +
>> >> +	unregister_framebuffer(info->fb_info);
>> >> +	unbind_from_irqhandler(info->irq, info);
>> >> +	free_page((unsigned long)info->page);
>> >> +	kfree(info->pages);
>> >> +	vfree(info->fb);
>> >> +	kfree(info);
>> >> +	xenfb_info = NULL;
>> > I don''t think this releases all of the memory you
allocate in
>> > xenfb_probe.
>> Fixing...
> Thanks.
>
>> >> +	  return -ENODEV;
>> >> +#if 0
>> >> +        /* if we''re not set up to use graphics mode,
then don''t initialize */
>> >> +	if (xenbus_scanf(XBT_NIL, "console",
"use_graphics", "%d", &ret) < 0)
>> >> +	  return -ENODEV;
>> >> +	if (ret == 0)
>> >> +	  return -ENODEV;
>> >> +#endif
>> > Not sure about this.
>> Same as for xenfb.c.  The #if is an accident.
> Ack.
>
>> >> +
>> >> +	dev = kmalloc(sizeof(*dev), GFP_KERNEL);
>> >> +	input_dev = input_allocate_device();
>> >> +	if (!dev || !input_dev)
>> >> +		return -ENOMEM;
>> > You potentially leak memory if one of these fails and the other
succeeds.
>> >
>> > Also, the error paths don''t release input_dev.  In fact,
nothing ever
>> > releases input_dev, unless I''m missing something.
>> Fixing...
> Thanks.
>
>> >> +
>> >> +	dev->dev = input_dev;
>> >> +	dev->info = (void *)__get_free_page(GFP_KERNEL);
>> >> +	if (!dev->info) {
>> >> +		ret = -ENOMEM;
>> >> +		goto error;
>> >> +	}
>> >> +
>> >> +	alloc_unbound.dom = DOMID_SELF;
>> >> +	alloc_unbound.remote_dom = 0;
>> >> +	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
>> >> +					  &alloc_unbound);
>> >> +	if (ret)
>> >> +		goto error_freep;
>> >> +	dev->evtchn = alloc_unbound.port;
>> >> +	ret = bind_evtchn_to_irqhandler(dev->evtchn,
input_handler, 0,
>> >> +					"xenkbd", dev);
>> >> +	if (ret < 0)
>> >> +		goto error_freep;
>> > You might want to close the event channel here.
>> Fixing...
> Thanks.
>
>> >> +
>> >> +	input_dev->name = "Xen Virtual Keyboard/Mouse";
>> >> +
>> >> +	input_register_device(input_dev);
>> > This can fail.
>> Most uses elsewhere in the kernel don''t care.  Fixing anyway.
> Yeah, I know, but it''s good to get it right anyway.  Thanks.
>
>> >> +
>> >> + again:
>> >> +	ret = xenbus_transaction_start(&xbt);
>> >> +	if (ret)
>> >> +		goto error_unreg;
>> >> +	ret = xenbus_printf(xbt, "vkbd",
"page-ref", "%lu",
>> >> +			    virt_to_mfn(dev->info));
>> > There''s no reason that I can see not to use grant tables
here, and
>> > that''d avoid all of the potential issues with shadow
translate mode.
>> Since we have to fool around with shadow translate mode anyway for
>> sharing the framebuffer, I don''t quite see why we need to
address this
>> right now.  This code works, and there''s no shortage of code
that
>> needs fixing.
> Alright, leave this for now and fix it when you fix the vfb equivalent
> bits.
>
>> Same for vfb/page-ref.
> Well, that''s a reference to a bunch of untranslated frame numbers,
so
> it makes more sense for it to be untranslated itself.  The keyboard
> page doesn''t require any further translation.
>
>> >> +MODULE_LICENSE("GPL");
>> >> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02
15:11:17 2006 -0400
>> >
>> > This is defining an io protocol, so belongs in
xen/include/public/io
>> Okay.
> Thanks.
>
>> >> +#include <asm/types.h>
>> >> +
>> >> +/* out events */
>> >> +
>> >> +#define XENFB_OUT_EVENT_SIZE 40
>> >> +
>> >> +#define XENFB_TYPE_MOTION 1
>> >> +#define XENFB_TYPE_UPDATE 2
>> >> +
>> > The other tagged unions in Xen look more like this:
>> >
>> > struct {
>> > 	u8 type;
>> > 	union {
>> > 		struct {
>> > 			u32 field1;
>> > 			u32 field2;
>> > 		} type1;
>> > 		struct {
>> > 			u16 field1;
>> > 		} type2;
>> > 	} u;
>> > };
>> >
>> > I''d find this a lot easier to read than the current
approach of
>> > putting the type into the individual event type structures.  Is
there
>> > any reason why this couldn''t be used here?
>> Matter of taste.  Will change it if you insist.
> It''d be good to be consistent with the other protocols.
How to pad such a struct to a fixed size?
>> >> +struct xenfb_motion	    /* currently unused */
>> >> +{
>> >> +	__u8 type;          /* XENFB_TYPE_MOTION */
>> >> +	__u16 x;            /* The new x coordinate */
>> >> +	__u16 y;            /* The new y coordinate */
>> >> +};
>> > I think if the comment said what was moving this''d be a
lot clearer.
>> I''m not into documenting unused code I didn''t write
and don''t
>> understand, so I''m going to remove this.
> Even better.
>
>> >> +
>> >> +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;
>> >> +	struct xenfb_motion motion;
>> >> +	struct xenfb_update update;
>> >> +	char _[XENFB_OUT_EVENT_SIZE];
>> > Perhaps ``pad'''' would be clearer than
``_''''?
>> Works for me.
> Thanks.
>
>> >> +struct xenfb_page
>> >> +{
>> >> +	__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) */
>> >> +
>> >> +	unsigned long pd[2];	/* FIXME rename to pgdir? */
>> >> +	/* FIXME pd[1] unused at this time, shrink? */
>> > I think it''d be enough to just give some indication of
where the magic
>> > number 2 came from.
>> Okay.
> Thanks.
>
>> >> +
>> >> +	__u32 in_cons, in_prod;
>> >> +	__u32 out_cons, out_prod;
>> >> +};
>> >> +
>> >> +void xenfb_resume(void);
>> > No longer exists.
>> Removed.
> Thanks.
>
>> >> +
>> >> +#endif
>> >> diff -r 5fa9b746d24f -r 510c6bceb87f
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	Sat Sep 02
15:11:17 2006 -0400
>> > Again, this belongs in xen/include/public/io.
>> Okay.
> Thanks.
>
>> >> +union xenkbd_in_event
>> >> +{
>> >> +	__u8 type;
>> >> +	struct xenkbd_motion motion;
>> >> +	struct xenkbd_button button;
>> >> +	struct xenkbd_key key;
>> >> +	char _[XENKBD_IN_EVENT_SIZE];
>> >> +};
>> >> +
>> >> +/* out events */
>> > Maybe a comment that none are currently defined?
>> Sure.
> Thanks.
>
>> >> +struct xenkbd_info
>> >> +{
>> >> +	__u32 in_cons, in_prod;
>> >> +	__u32 out_cons, out_prod;
>> >> +};
>> >> +
>> >> +void xenkbd_resume(void);
>> > No longer exists.
>> Removed.
> Thanks.
>
> Steven.
Next try.  The patch contains Amos Waterland''s patch for xenconsole to
use /dev/xvc0 instead of taking over ttys, with Jeremy''s fixes and
cleanups.  If you need those parts separate, let me know.


diff -r 47c098fdce14 arch/i386/kernel/setup-xen.c
--- a/arch/i386/kernel/setup-xen.c	Wed Sep 20 15:35:23 2006 +0200
+++ b/arch/i386/kernel/setup-xen.c	Sat Sep 30 10:34:01 2006 +0200
@@ -1788,8 +1788,9 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 	} else {
-		extern int console_use_vt;
-		console_use_vt = 0;
+#if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
+		conswitchp = &dummy_con;
+#endif
 	}
 #ifdef CONFIG_X86_TSC
 	tsc_init();
diff -r 47c098fdce14 arch/ia64/kernel/setup.c
--- a/arch/ia64/kernel/setup.c	Wed Sep 20 15:35:23 2006 +0200
+++ b/arch/ia64/kernel/setup.c	Sat Sep 30 10:34:01 2006 +0200
@@ -583,9 +583,9 @@ setup_arch (char **cmdline_p)
 		       xen_start_info->nr_pages, xen_start_info->flags);
 
 		if (!is_initial_xendomain()) {
-			extern int console_use_vt;
+#if !defined(CONFIG_VT) || !defined(CONFIG_DUMMY_CONSOLE)
 			conswitchp = NULL;
-			console_use_vt = 0;
+#endif
 		}
 	}
 #endif
diff -r 47c098fdce14 arch/x86_64/kernel/setup-xen.c
--- a/arch/x86_64/kernel/setup-xen.c	Wed Sep 20 15:35:23 2006 +0200
+++ b/arch/x86_64/kernel/setup-xen.c	Sat Sep 30 10:34:01 2006 +0200
@@ -931,9 +931,10 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 		} else {
-			extern int console_use_vt;
-			console_use_vt = 0;
-		}
+#if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
+                    conswitchp = &dummy_con;
+#endif
+                }
 	}
 #else	/* CONFIG_XEN */
 
diff -r 47c098fdce14 drivers/char/tty_io.c
--- a/drivers/char/tty_io.c	Wed Sep 20 15:35:23 2006 +0200
+++ b/drivers/char/tty_io.c	Sat Sep 30 10:34:01 2006 +0200
@@ -130,8 +130,6 @@ LIST_HEAD(tty_drivers);			/* linked list
    vt.c for deeply disgusting hack reasons */
 DEFINE_MUTEX(tty_mutex);
 
-int console_use_vt = 1;
-
 #ifdef CONFIG_UNIX98_PTYS
 extern struct tty_driver *ptm_driver;	/* Unix98 pty masters; for /dev/ptmx */
 extern int pty_limit;		/* Config limit on Unix98 ptys */
@@ -2485,7 +2483,7 @@ retry_open:
 		goto got_driver;
 	}
 #ifdef CONFIG_VT
-	if (console_use_vt && (device == MKDEV(TTY_MAJOR,0))) {
+	if (device == MKDEV(TTY_MAJOR,0)) {
 		extern struct tty_driver *console_driver;
 		driver = console_driver;
 		index = fg_console;
@@ -3911,8 +3909,6 @@ static int __init tty_init(void)
 #endif
 
 #ifdef CONFIG_VT
-	if (!console_use_vt)
-		goto out_vt;
 	cdev_init(&vc0_cdev, &console_fops);
 	if (cdev_add(&vc0_cdev, MKDEV(TTY_MAJOR, 0), 1) ||
 	    register_chrdev_region(MKDEV(TTY_MAJOR, 0), 1, "/dev/vc/0") <
0)
diff -r 47c098fdce14 drivers/xen/Kconfig
--- a/drivers/xen/Kconfig	Wed Sep 20 15:35:23 2006 +0200
+++ b/drivers/xen/Kconfig	Sat Sep 30 10:34:01 2006 +0200
@@ -172,6 +172,29 @@ config XEN_NETDEV_FRONTEND
 	  dedicated device-driver domain, or your master control domain
 	  (domain 0), then you almost certainly want to say Y here.
 
+config XEN_FRAMEBUFFER
+	tristate "Framebuffer-device frontend driver"
+	depends on XEN && FB
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	default y
+	help
+	  The framebuffer-device frontend drivers allows the kernel to create a
+	  virtual framebuffer.  This framebuffer can be viewed in another
+	  domain.  Unless this domain has access to a real video card, you
+	  probably want to say Y here.
+
+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.
+
 config XEN_SCRUB_PAGES
 	bool "Scrub memory before freeing it to Xen"
 	default y
diff -r 47c098fdce14 drivers/xen/Makefile
--- a/drivers/xen/Makefile	Wed Sep 20 15:35:23 2006 +0200
+++ b/drivers/xen/Makefile	Sat Sep 30 10:34:01 2006 +0200
@@ -15,3 +15,5 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= net
 obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
 obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= pciback/
 obj-$(CONFIG_XEN_PCIDEV_FRONTEND)	+= pcifront/
+obj-$(CONFIG_XEN_FRAMEBUFFER)		+= xenfb/
+obj-$(CONFIG_XEN_KEYBOARD)		+= xenkbd/
diff -r 47c098fdce14 drivers/xen/console/console.c
--- a/drivers/xen/console/console.c	Wed Sep 20 15:35:23 2006 +0200
+++ b/drivers/xen/console/console.c	Sat Sep 30 10:34:01 2006 +0200
@@ -58,19 +58,27 @@
 #include <asm/hypervisor.h>
 #include <xen/evtchn.h>
 #include <xen/xencons.h>
+#include <xen/xenbus.h>
 
 /*
  * Modes:
  *  ''xencons=off''  [XC_OFF]:     Console is disabled.
  *  ''xencons=tty''  [XC_TTY]:     Console attached to
''/dev/tty[0-9]+''.
  *  ''xencons=ttyS'' [XC_SERIAL]:  Console attached to
''/dev/ttyS[0-9]+''.
+ *  ''xencons=xvc''  [XC_XVC]:     Console attached to
''/dev/xvc[0-9]+''.
  *                 [XC_DEFAULT]: DOM0 -> XC_SERIAL ; all others ->
XC_TTY.
  * 
  * NB. In mode XC_TTY, we create dummy consoles for tty2-63. This suppresses
  * warnings from standard distro startup scripts.
  */
-static enum { XC_OFF, XC_DEFAULT, XC_TTY, XC_SERIAL } xc_mode = XC_DEFAULT;
+static enum { XC_OFF, XC_DEFAULT, XC_TTY, XC_SERIAL, XC_XVC } 
+    xc_mode = XC_DEFAULT;
 static int xc_num = -1;
+
+/* If we are in XC_XVC mode (a virtual console at /dev/xvcX), we need to
+ * comply with Lanana and use a minor under the low density serial major.
+ */
+#define XEN_XVC_MINOR 187
 
 #ifdef CONFIG_MAGIC_SYSRQ
 static unsigned long sysrq_requested;
@@ -86,6 +94,8 @@ static int __init xencons_setup(char *st
 		xc_mode = XC_SERIAL;
 	else if (!strncmp(str, "tty", 3))
 		xc_mode = XC_TTY;
+	else if (!strncmp(str, "xvc", 3))
+		xc_mode = XC_XVC;
 	else if (!strncmp(str, "off", 3))
 		xc_mode = XC_OFF;
 
@@ -96,6 +106,11 @@ static int __init xencons_setup(char *st
 			xc_num = n;
 		break;
 	case XC_TTY:
+		n = simple_strtol(str+3, &q, 10);
+		if (q > (str + 3))
+			xc_num = n;
+		break;
+	case XC_XVC:
 		n = simple_strtol(str+3, &q, 10);
 		if (q > (str + 3))
 			xc_num = n;
@@ -196,11 +211,20 @@ static int __init xen_console_init(void)
 		if (!xen_start_info->console.domU.evtchn)
 			goto out;
 		if (xc_mode == XC_DEFAULT)
-			xc_mode = XC_TTY;
+#ifdef CONFIG_XEN_FRAMEBUFFER
+			xc_mode = XC_XVC;
+#else
 		kcons_info.write = kcons_write;
+#endif
 	}
 
 	switch (xc_mode) {
+	case XC_XVC:
+		strcpy(kcons_info.name, "xvc");
+		if (xc_num == -1)
+			xc_num = 0;
+		break;
+
 	case XC_SERIAL:
 		strcpy(kcons_info.name, "ttyS");
 		if (xc_num == -1)
@@ -305,7 +329,7 @@ void dom0_init_screen_info(const struct 
 /******************** User-space console driver (/dev/console) ************/
 
 #define DRV(_d)         (_d)
-#define DUMMY_TTY(_tty) ((xc_mode != XC_SERIAL) &&		\
+#define DUMMY_TTY(_tty) ((xc_mode != XC_SERIAL) && (xc_mode != XC_XVC)
&& \
 			 ((_tty)->index != (xc_num - 1)))
 
 static struct termios *xencons_termios[MAX_NR_CONSOLES];
@@ -628,7 +652,8 @@ static int __init xencons_init(void)
 			return rc;
 	}
 
-	xencons_driver = alloc_tty_driver((xc_mode == XC_SERIAL) ?
+	xencons_driver = alloc_tty_driver(((xc_mode == XC_SERIAL) || 
+					   (xc_mode == XC_XVC)) ?
 					  1 : MAX_NR_CONSOLES);
 	if (xencons_driver == NULL)
 		return -ENOMEM;
@@ -648,6 +673,12 @@ static int __init xencons_init(void)
 		DRV(xencons_driver)->name        = "ttyS";
 		DRV(xencons_driver)->minor_start = 64 + xc_num;
 		DRV(xencons_driver)->name_base   = 0 + xc_num;
+	} else if (xc_mode == XC_XVC) {
+		DRV(xencons_driver)->name        = "xvc";
+		DRV(xencons_driver)->major       = 250; /* FIXME: until lanana a
+pproves for 204:187 */
+		DRV(xencons_driver)->minor_start = XEN_XVC_MINOR;
+		DRV(xencons_driver)->name_base   = xc_num;
 	} else {
 		DRV(xencons_driver)->name        = "tty";
 		DRV(xencons_driver)->minor_start = 1;
@@ -680,6 +711,20 @@ static int __init xencons_init(void)
 	printk("Xen virtual console successfully installed as %s%d\n",
 	       DRV(xencons_driver)->name, xc_num);
 
+        /* Don''t need to check about graphical fb for domain 0 */
+        if (is_initial_xendomain())
+		return 0;
+
+	rc = 0;
+	if (xenbus_scanf(XBT_NIL, "console", "use_graphics",
"%d", &rc) < 0)
+		printk(KERN_ERR "Unable to read console/use_graphics\n");
+	if (rc == 0) {
+               /* FIXME: this is ugly */
+	       unregister_console(&kcons_info);
+	       kcons_info.flags |= CON_CONSDEV;
+	       register_console(&kcons_info);
+	}
+
 	return 0;
 }
 
diff -r 47c098fdce14 mm/memory.c
--- a/mm/memory.c	Wed Sep 20 15:35:23 2006 +0200
+++ b/mm/memory.c	Sat Sep 30 10:34:01 2006 +0200
@@ -891,6 +891,7 @@ unsigned long zap_page_range(struct vm_a
 		tlb_finish_mmu(tlb, address, end);
 	return end;
 }
+EXPORT_SYMBOL(zap_page_range);
 
 /*
  * Do a quick page-table lookup for a single page.
diff -r 47c098fdce14 drivers/xen/xenfb/Makefile
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/xen/xenfb/Makefile	Sat Sep 30 10:34:01 2006 +0200
@@ -0,0 +1,1 @@
+obj-$(CONFIG_XEN_FRAMEBUFFER)	:= xenfb.o
diff -r 47c098fdce14 drivers/xen/xenfb/xenfb.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/xen/xenfb/xenfb.c	Sat Sep 30 10:34:01 2006 +0200
@@ -0,0 +1,580 @@
+/*
+ * linux/drivers/video/xenfb.c -- Xen para-virtual frame buffer device
+ *
+ * Copyright (C) 2005-2006
+ *
+ *      Anthony Liguori <aliguori@us.ibm.com>
+ *
+ *  Based on linux/drivers/video/q40fb.c
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License. See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+/*
+ * TODO:
+ *
+ * Switch to grant tables when they become capable of dealing with the
+ * frame buffer.
+ *
+ * Use xenbus_device API.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/module.h>
+#include <linux/vmalloc.h>
+#include <linux/mm.h>
+#include <asm/hypervisor.h>
+#include <xen/evtchn.h>
+#include <xen/interface/io/xenfb.h>
+#include <xen/xenbus.h>
+#include <linux/kthread.h>
+
+#define XENFB_WIDTH 800
+#define XENFB_HEIGHT 600
+#define XENFB_DEPTH 32
+
+struct xenfb_mapping
+{
+	struct list_head	link;
+	struct vm_area_struct	*vma;
+	atomic_t		map_refs;
+	int			faults;
+	struct xenfb_info	*info;
+};
+
+struct xenfb_info
+{
+	struct task_struct	*kthread;
+	wait_queue_head_t	wq;
+
+	unsigned char		*fb;
+	struct fb_info		*fb_info;
+	struct timer_list	refresh;
+	int			dirty;
+	int			x1, y1, x2, y2;	/* dirty rectangle,
+						   protected by mm_lock */
+	struct semaphore	mm_lock;
+	int			nr_pages;
+	struct page		**pages;
+	struct list_head	mappings; /* protected by mm_lock */
+
+	unsigned		evtchn;
+	int			irq;
+	struct xenfb_page	*page;
+	unsigned long 		*mfns;
+	u32			flags; /* combination of XENFB_FLAG_* */
+};
+
+static int xenfb_fps = 20;
+static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH /
8;
+
+static void __exit xenfb_cleanup(void);
+
+static void xenfb_do_update(struct xenfb_info *info,
+			    int x, int y, int w, int h)
+{
+	union xenfb_out_event event;
+	__u32 prod;
+
+	event.type = XENFB_TYPE_UPDATE;
+	event.update.x = x;
+	event.update.y = y;
+	event.update.width = w;
+	event.update.height = h;
+
+	prod = info->page->out_prod;
+	/* caller ensures !xenfb_queue_full() */
+	mb();			/* ensure ring space available */
+	XENFB_OUT_RING_REF(info->page, prod) = event;
+	wmb();			/* ensure ring contents visible */
+	info->page->out_prod = prod + 1;
+
+	notify_remote_via_evtchn(info->evtchn);
+}
+
+static int xenfb_queue_full(struct xenfb_info *info)
+{
+	__u32 cons, prod;
+
+	prod = info->page->out_prod;
+	cons = info->page->out_cons;
+	return prod - cons == XENFB_OUT_RING_LEN;
+}
+
+static void xenfb_update_screen(struct xenfb_info *info)
+{
+	int y1, y2, x1, x2;
+	struct xenfb_mapping *map;
+
+	if (!(info->flags & XENFB_FLAG_UPDATE))
+		return;
+	if (xenfb_queue_full(info))
+		return;
+
+	down(&info->mm_lock);
+
+	y1 = info->y1;
+	y2 = info->y2;
+	x1 = info->x1;
+	x2 = info->x2;
+	info->x1 = info->y1 = INT_MAX;
+	info->x2 = info->y2 = 0;
+
+	list_for_each_entry(map, &info->mappings, link) {
+		if (!map->faults)
+			continue;
+		zap_page_range(map->vma, map->vma->vm_start,
+			       map->vma->vm_end - map->vma->vm_start, NULL);
+		map->faults = 0;
+	}
+
+	up(&info->mm_lock);
+
+	xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
+}
+
+static int xenfb_thread(void *data)
+{
+	struct xenfb_info *info = data;
+
+	for (;;) {
+		if (kthread_should_stop())
+			break;
+		if (info->dirty) {
+			info->dirty = 0;
+			xenfb_update_screen(info);
+		}
+		wait_event_interruptible(info->wq,
+			kthread_should_stop() || info->dirty);
+	}
+	return 0;
+}
+
+static int xenfb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			   unsigned blue, unsigned transp,
+			   struct fb_info *info)
+{
+	u32 v;
+
+	if (regno > info->cmap.len)
+		return 1;
+
+	red   >>= (16 - info->var.red.length);
+	green >>= (16 - info->var.green.length);
+	blue  >>= (16 - info->var.blue.length);
+
+	v = (red << info->var.red.offset) |
+	    (green << info->var.green.offset) |
+	    (blue << info->var.blue.offset);
+
+	/* FIXME is this sane?  check against xxxfb_setcolreg()!  */
+	switch (info->var.bits_per_pixel) {
+	case 16:
+	case 24:
+	case 32:
+		((u32 *)info->pseudo_palette)[regno] = v;
+		break;
+	}
+	
+	return 0;
+}
+
+static void xenfb_timer(unsigned long data)
+{
+	struct xenfb_info *info = (struct xenfb_info *)data;
+	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->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);
+}
+
+static void xenfb_refresh(struct xenfb_info *info,
+			  int x1, int y1, int w, int h)
+{
+	down(&info->mm_lock);
+	__xenfb_refresh(info, x1, y1, w, h);
+	up(&info->mm_lock);
+}
+
+static void xenfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
+{
+	struct xenfb_info *info = p->par;
+
+	cfb_fillrect(p, rect);
+	xenfb_refresh(info, rect->dx, rect->dy, rect->width,
rect->height);
+}
+
+static void xenfb_imageblit(struct fb_info *p, const struct fb_image *image)
+{
+	struct xenfb_info *info = p->par;
+
+	cfb_imageblit(p, image);
+	xenfb_refresh(info, image->dx, image->dy, image->width,
image->height);
+}
+
+static void xenfb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
+{
+	struct xenfb_info *info = p->par;
+
+	cfb_copyarea(p, area);
+	xenfb_refresh(info, area->dx, area->dy, area->width,
area->height);
+}
+
+static void xenfb_vm_open(struct vm_area_struct *vma)
+{
+	struct xenfb_mapping *map = vma->vm_private_data;
+	atomic_inc(&map->map_refs);
+}
+
+static void xenfb_vm_close(struct vm_area_struct *vma)
+{
+	struct xenfb_mapping *map = vma->vm_private_data;
+	struct xenfb_info *info = map->info;
+
+	down(&info->mm_lock);
+	if (atomic_dec_and_test(&map->map_refs)) {
+		list_del(&map->link);
+		kfree(map);
+	}
+	up(&info->mm_lock);
+}
+
+static struct page *xenfb_vm_nopage(struct vm_area_struct *vma,
+				    unsigned long vaddr, int *type)
+{
+	struct xenfb_mapping *map = vma->vm_private_data;
+	struct xenfb_info *info = map->info;
+	int pgnr = (vaddr - vma->vm_start) >> PAGE_SHIFT;
+	struct page *page;
+	int y1, y2;
+
+	if (pgnr >= info->nr_pages)
+		return NOPAGE_SIGBUS;
+
+	down(&info->mm_lock);
+	page = info->pages[pgnr];
+	get_page(page);
+	map->faults++;
+
+	y1 = pgnr * PAGE_SIZE / info->fb_info->fix.line_length;
+	y2 = (pgnr * PAGE_SIZE + PAGE_SIZE - 1) /
info->fb_info->fix.line_length;
+	if (y2 > info->fb_info->var.yres)
+		y2 = info->fb_info->var.yres;
+	__xenfb_refresh(info, 0, y1, info->fb_info->var.xres, y2 - y1);
+	up(&info->mm_lock);
+
+	if (type)
+		*type = VM_FAULT_MINOR;
+
+	return page;
+}
+
+static struct vm_operations_struct xenfb_vm_ops = {
+	.open	= xenfb_vm_open,
+	.close	= xenfb_vm_close,
+	.nopage	= xenfb_vm_nopage,
+};
+
+static int xenfb_mmap(struct fb_info *fb_info, struct vm_area_struct *vma)
+{
+	struct xenfb_info *info = fb_info->par;
+	struct xenfb_mapping *map;
+	int ret;
+	int map_pages;
+
+	down(&info->mm_lock);
+
+	ret = -EINVAL;
+	if (!(vma->vm_flags & VM_WRITE))
+		goto out;
+	if (!(vma->vm_flags & VM_SHARED))
+		goto out;
+	if (vma->vm_pgoff != 0)
+		goto out;
+
+	map_pages = (vma->vm_end - vma->vm_start + PAGE_SIZE-1) >>
PAGE_SHIFT;
+	if (map_pages > info->nr_pages)
+		goto out;
+
+	ret = -ENOMEM;
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (map == NULL)
+		goto out;
+
+	map->vma = vma;
+	map->faults = 0;
+	map->info = info;
+	atomic_set(&map->map_refs, 1);
+	list_add(&map->link, &info->mappings);
+	vma->vm_ops = &xenfb_vm_ops;
+	vma->vm_flags |= (VM_DONTEXPAND | VM_RESERVED);
+	vma->vm_private_data = map;
+	ret = 0;
+
+ out:
+	up(&info->mm_lock);
+	return ret;
+}
+
+static struct fb_ops xenfb_fb_ops = {
+	.owner		= THIS_MODULE,
+	.fb_setcolreg	= xenfb_setcolreg,
+	.fb_fillrect	= xenfb_fillrect,
+	.fb_copyarea	= xenfb_copyarea,
+	.fb_imageblit	= xenfb_imageblit,
+	.fb_mmap	= xenfb_mmap,
+};
+
+static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
+				       struct pt_regs *regs)
+{
+	struct xenfb_info *info = dev_id;
+	__u32 cons, prod;
+
+	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_IN_RING_REF(info->page, cons);
+
+		switch (event->type) {
+		case XENFB_TYPE_SET_EVENTS:
+			info->flags = event->set_events.flags;
+			break;
+		}
+	}
+	mb();			/* ensure we''re done with ring contents */
+	info->page->in_cons = cons;
+	notify_remote_via_evtchn(info->evtchn);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned long vmalloc_to_mfn(void *address)
+{
+	return pfn_to_mfn(vmalloc_to_pfn(address));
+}
+
+static struct xenfb_info *xenfb_info;
+
+static int __init xenfb_init(void)
+{
+	struct xenfb_info *info;
+	int i, ret;
+	struct fb_info *fb_info;
+	struct evtchn_alloc_unbound alloc_unbound;
+	struct evtchn_close close;
+	struct xenbus_transaction xbt;
+
+        /* Nothing to do if running in dom0. */
+        if (is_initial_xendomain())
+		return -ENODEV;
+        /* if we''re not set up to use graphics mode, then
don''t initialize */
+	if (xenbus_scanf(XBT_NIL, "console", "use_graphics",
"%d", &ret) < 0)
+		return -ENODEV;
+	if (ret == 0)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (info == NULL)
+		return -ENOMEM;
+	xenfb_info = info;
+	info->irq = -1;
+	info->x1 = info->y1 = INT_MAX;
+	init_MUTEX(&info->mm_lock);
+	init_waitqueue_head(&info->wq);
+	init_timer(&info->refresh);
+	info->refresh.function = xenfb_timer;
+	info->refresh.data = (unsigned long)info;
+	INIT_LIST_HEAD(&info->mappings);
+
+	info->fb = vmalloc(xenfb_mem_len);
+	if (info->fb == NULL)
+		goto error;
+	memset(info->fb, 0, xenfb_mem_len);
+
+	info->nr_pages = (xenfb_mem_len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+
+	info->pages = kmalloc(sizeof(struct page *) * info->nr_pages,
+			      GFP_KERNEL);
+	if (info->pages == NULL)
+		goto error;
+	for (i = 0; i < info->nr_pages; i++)
+		info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);
+
+	info->mfns = vmalloc(sizeof(unsigned long) * info->nr_pages);
+	if (!info->mfns)
+		goto error;
+	for (i = 0; i < info->nr_pages; i++)
+		info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
+
+	/* set up shared page */
+	info->page = (void *)__get_free_page(GFP_KERNEL);
+	if (!info->page)
+		goto error;
+
+	info->page->pd[0] = vmalloc_to_mfn(info->mfns);
+	info->page->pd[1] = 0;
+	info->page->width = XENFB_WIDTH;
+	info->page->height = XENFB_HEIGHT;
+	info->page->depth = XENFB_DEPTH;
+	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;
+
+	/* set up event channel */
+	alloc_unbound.dom = DOMID_SELF;
+	alloc_unbound.remote_dom = 0;
+	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
+					  &alloc_unbound);
+	if (ret)
+		goto error;
+	info->evtchn = alloc_unbound.port;
+
+	ret = bind_evtchn_to_irqhandler(info->evtchn, xenfb_event_handler,
+					0, "xenfb", info);
+	if (ret < 0) {
+		close.port = info->evtchn;
+		HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
+		goto error;
+	}
+	info->irq = ret;
+
+	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
+				/* see fishy hackery below */
+	if (fb_info == NULL)
+		goto error;
+
+	/* FIXME fishy hackery */
+	fb_info->pseudo_palette = fb_info->par;
+	fb_info->par = info;
+	/* /FIXME */
+	fb_info->screen_base = info->fb;
+
+	fb_info->fbops = &xenfb_fb_ops;
+	fb_info->var.xres_virtual = fb_info->var.xres = info->page->width;
+	fb_info->var.yres_virtual = fb_info->var.yres =
info->page->height;
+	fb_info->var.bits_per_pixel = info->page->depth;
+
+	fb_info->var.red = (struct fb_bitfield){16, 8, 0};
+	fb_info->var.green = (struct fb_bitfield){8, 8, 0};
+	fb_info->var.blue = (struct fb_bitfield){0, 8, 0};
+
+	fb_info->var.activate = FB_ACTIVATE_NOW;
+	fb_info->var.height = -1;
+	fb_info->var.width = -1;
+	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
+
+	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
+	fb_info->fix.line_length = info->page->line_length;
+	fb_info->fix.smem_start = 0;
+	fb_info->fix.smem_len = xenfb_mem_len;
+	strcpy(fb_info->fix.id, "xen");
+	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
+	fb_info->fix.accel = FB_ACCEL_NONE;
+
+	fb_info->flags = FBINFO_FLAG_DEFAULT;
+
+	if (fb_alloc_cmap(&fb_info->cmap, 256, 0) < 0) {
+		framebuffer_release(fb_info);
+		goto error;
+	}
+
+	ret = register_framebuffer(fb_info);
+	if (ret) {
+		fb_dealloc_cmap(&info->fb_info->cmap);
+		framebuffer_release(fb_info);
+		goto error;
+	}
+	info->fb_info = fb_info;
+
+	info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
+	if (IS_ERR(info->kthread))
+		goto error;
+
+ again:
+	ret = xenbus_transaction_start(&xbt);
+	if (ret)
+		goto error;
+	ret = xenbus_printf(xbt, "vfb/0", "page-ref",
"%lu",
+			    virt_to_mfn(info->page));
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_printf(xbt, "vfb/0", "event-channel",
"%u",
+			    info->evtchn);
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_transaction_end(xbt, 0);
+	if (ret) {
+		if (ret == -EAGAIN)
+			goto again;
+		goto error;
+	}
+
+	printk(KERN_DEBUG "xenfb initialized\n");
+	return 0;
+
+ error_xenbus:
+	xenbus_transaction_end(xbt, 1);
+ error:
+	xenfb_cleanup();
+
+	return -ENODEV;
+}
+
+static void __exit xenfb_cleanup(void)
+{
+	struct xenfb_info *info = xenfb_info;
+
+	if (!info)
+		return;
+	printk(KERN_DEBUG "xenfb cleaning up\n");
+	del_timer(&info->refresh);
+	if (info->kthread)
+		kthread_stop(info->kthread);
+	if (info->irq >= 0)
+		unbind_from_irqhandler(info->irq, info);
+	if (info->fb_info) {
+		unregister_framebuffer(info->fb_info);
+		fb_dealloc_cmap(&info->fb_info->cmap);
+		framebuffer_release(info->fb_info);
+	}
+	free_page((unsigned long)info->page);
+	vfree(info->mfns);
+	kfree(info->pages);
+	vfree(info->fb);
+	kfree(info);
+	xenfb_info = NULL;
+	printk(KERN_DEBUG "xenfb cleaned up\n");
+}
+
+module_init(xenfb_init);
+module_exit(xenfb_cleanup);
+
+MODULE_LICENSE("GPL");
diff -r 47c098fdce14 drivers/xen/xenkbd/Makefile
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/xen/xenkbd/Makefile	Sat Sep 30 10:34:01 2006 +0200
@@ -0,0 +1,1 @@
+obj-$(CONFIG_XEN_KEYBOARD)	+= xenkbd.o
diff -r 47c098fdce14 drivers/xen/xenkbd/xenkbd.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/xen/xenkbd/xenkbd.c	Sat Sep 30 10:34:01 2006 +0200
@@ -0,0 +1,198 @@
+/*
+ * linux/drivers/input/keyboard/xenkbd.c -- Xen para-virtual input device
+ *
+ * Copyright (C) 2005
+ *
+ *      Anthony Liguori <aliguori@us.ibm.com>
+ *
+ *  Based on linux/drivers/input/mouse/sermouse.c
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License. See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+/*
+ * TODO:
+ *
+ * Switch to grant tables together with xenfb.c.
+ *
+ * Use xenbus_device API.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/input.h>
+#include <asm/hypervisor.h>
+#include <xen/evtchn.h>
+#include <xen/interface/io/xenkbd.h>
+#include <xen/xenbus.h>
+
+struct xenkbd_device
+{
+	struct input_dev *dev;
+	struct xenkbd_info *info;
+	unsigned evtchn;
+	int irq;
+};
+
+static void __exit xenkbd_cleanup(void);
+
+static irqreturn_t input_handler(int rq, void *dev_id, struct pt_regs *regs)
+{
+	struct xenkbd_device *dev = dev_id;
+	struct xenkbd_info *info = dev ? dev->info : 0;
+	static int button_map[3] = { BTN_RIGHT, BTN_MIDDLE, BTN_LEFT };
+	__u32 cons, prod;
+
+	prod = info->in_prod;
+	rmb();			/* ensure we see ring contents up to prod */
+	for (cons = info->in_cons; cons != prod; cons++) {
+		union xenkbd_in_event *event;
+		event = &XENKBD_IN_RING_REF(info, cons);
+	
+		switch (event->type) {
+		case XENKBD_TYPE_MOTION:
+			input_report_rel(dev->dev, REL_X, event->motion.rel_x);
+			input_report_rel(dev->dev, REL_Y, event->motion.rel_y);
+			break;
+		case XENKBD_TYPE_BUTTON:
+			if (event->button.button < 3)
+				input_report_key(dev->dev,
+						 button_map[event->button.button],
+						 event->button.pressed);
+			break;
+		case XENKBD_TYPE_KEY:
+			input_report_key(dev->dev, event->key.keycode, event->key.pressed);
+			break;
+		}
+	}
+	input_sync(dev->dev);
+	mb();			/* ensure we got ring contents */
+	info->in_cons = cons;
+	notify_remote_via_evtchn(dev->evtchn);
+
+	return IRQ_HANDLED;
+}
+
+static struct xenkbd_device *xenkbd_dev;
+
+int __init xenkbd_init(void)
+{
+	int ret = 0;
+	int i;
+	struct xenkbd_device *dev;
+	struct input_dev *input_dev;
+	struct evtchn_alloc_unbound alloc_unbound;
+	struct evtchn_close close;
+	struct xenbus_transaction xbt;
+
+        /* Nothing to do if running in dom0. */
+        if (is_initial_xendomain())
+		return -ENODEV;
+        /* if we''re not set up to use graphics mode, then
don''t initialize */
+	if (xenbus_scanf(XBT_NIL, "console", "use_graphics",
"%d", &ret) < 0)
+		return -ENODEV;
+	if (ret == 0)
+		return -ENODEV;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENODEV;
+	xenkbd_dev = dev;
+
+	dev->info = (void *)__get_free_page(GFP_KERNEL);
+	if (!dev->info) {
+		ret = -ENOMEM;
+		goto error;
+	}
+	dev->info->in_cons = dev->info->in_prod = 0;
+	dev->info->out_cons = dev->info->out_prod = 0;
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL);
+	input_dev->keybit[LONG(BTN_MOUSE)] = BIT(BTN_LEFT) | BIT(BTN_RIGHT);
+	input_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y);
+
+	/* FIXME not sure this is quite right */
+	for (i = 0; i < 256; i++)
+		set_bit(i, input_dev->keybit);
+
+	input_dev->name = "Xen Virtual Keyboard/Mouse";
+
+	ret = input_register_device(input_dev);
+	if (ret) {
+		input_free_device(input_dev);
+		goto error;
+	}
+	dev->dev = input_dev;
+
+	alloc_unbound.dom = DOMID_SELF;
+	alloc_unbound.remote_dom = 0;
+	ret = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound,
+					  &alloc_unbound);
+	if (ret)
+		goto error;
+	dev->evtchn = alloc_unbound.port;
+	ret = bind_evtchn_to_irqhandler(dev->evtchn, input_handler, 0,
+					"xenkbd", dev);
+	if (ret < 0) {
+		close.port = dev->evtchn;
+		HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
+		goto error;
+	}
+	dev->irq = ret;
+
+ again:
+	ret = xenbus_transaction_start(&xbt);
+	if (ret)
+		goto error;
+	ret = xenbus_printf(xbt, "vkbd/0", "page-ref",
"%lu",
+			    virt_to_mfn(dev->info));
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_printf(xbt, "vkbd/0", "event-channel",
"%u",
+			    dev->evtchn);
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_transaction_end(xbt, 0);
+	if (ret) {
+		if (ret == -EAGAIN)
+			goto again;
+		goto error;
+	}
+
+	printk(KERN_DEBUG "xenkbd initialized\n");
+	return ret;
+
+ error_xenbus:
+	xenbus_transaction_end(xbt, 1);
+ error:
+	xenkbd_cleanup();
+	return ret;
+}
+
+static void __exit xenkbd_cleanup(void)
+{
+	if (!xenkbd_dev)
+		return;
+	printk(KERN_DEBUG "xenkbd cleaning up\n");
+	if (xenkbd_dev->irq >= 0)
+		unbind_from_irqhandler(xenkbd_dev->irq, xenkbd_dev);
+	input_unregister_device(xenkbd_dev->dev);
+	free_page((unsigned long)xenkbd_dev->info);
+	kfree(xenkbd_dev);
+	xenkbd_dev = NULL;
+	printk(KERN_DEBUG "xenkbd cleaned up\n");
+}
+
+module_init(xenkbd_init);
+module_exit(xenkbd_cleanup);
+
+MODULE_LICENSE("GPL");
diff -r 47c098fdce14 include/xen/interface/io/xenfb.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/include/xen/interface/io/xenfb.h	Sat Sep 30 10:34:01 2006 +0200
@@ -0,0 +1,103 @@
+/*
+ * linux/include/linux/xenfb.h -- Xen virtual frame buffer device
+ *
+ * Copyright (C) 2005
+ *
+ *      Anthony Liguori <aliguori@us.ibm.com>
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License. See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+#ifndef _LINUX_XENFB_H
+#define _LINUX_XENFB_H
+
+#include <asm/types.h>
+
+/* out events */
+
+#define XENFB_OUT_EVENT_SIZE 40
+
+/* value 1 currently not used */
+#define XENFB_TYPE_UPDATE 2
+
+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;
+	struct xenfb_update update;
+	char pad[XENFB_OUT_EVENT_SIZE];
+};
+
+/* in events */
+
+#define XENFB_IN_EVENT_SIZE 40
+
+#define XENFB_TYPE_SET_EVENTS 1
+
+#define XENFB_FLAG_UPDATE 2
+
+struct xenfb_set_events
+{
+	__u8 type;          /* XENFB_TYPE_SET_EVENTS */
+	__u32 flags;        /* combination of XENFB_FLAG_* */
+};
+
+union xenfb_in_event
+{
+	__u8 type;
+	struct xenfb_set_events set_events;
+	char pad[XENFB_OUT_EVENT_SIZE];
+};
+
+/* shared page */
+
+#define XENFB_IN_RING_SIZE 1024
+#define XENFB_IN_RING_LEN (XENFB_IN_RING_SIZE / XENFB_IN_EVENT_SIZE)
+#define XENFB_IN_RING_OFFS 1024
+#define XENFB_IN_RING(page) \
+    ((union xenfb_in_event *)((char *)(page) + XENFB_IN_RING_OFFS))
+#define XENFB_IN_RING_REF(page, idx) \
+    (XENFB_IN_RING((page))[(idx) % XENFB_IN_RING_LEN])
+
+#define XENFB_OUT_RING_SIZE 2048
+#define XENFB_OUT_RING_LEN (XENFB_OUT_RING_SIZE / XENFB_OUT_EVENT_SIZE)
+#define XENFB_OUT_RING_OFFS (XENFB_IN_RING_OFFS + XENFB_IN_RING_SIZE)
+#define XENFB_OUT_RING(page) \
+    ((union xenfb_out_event *)((char *)(page) + XENFB_OUT_RING_OFFS))
+#define XENFB_OUT_RING_REF(page, idx) \
+    (XENFB_OUT_RING((page))[(idx) % XENFB_OUT_RING_LEN])
+
+struct xenfb_page
+{
+	__u32 in_cons, in_prod;
+	__u32 out_cons, out_prod;
+
+	__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) */
+
+	/*
+	 * Framebuffer page directory
+	 *
+	 * Each directory page holds PAGE_SIZE / sizeof(*pd)
+	 * framebuffer pages, and can thus map up to PAGE_SIZE *
+	 * PAGE_SIZE / sizeof(*pd) bytes.  With PAGE_SIZE == 4096 and
+	 * sizeof(unsigned long) == 4, that''s 4 Megs.  Two directory
+	 * pages should be enough for a while.
+	 */
+	unsigned long pd[2];
+};
+
+#endif
diff -r 47c098fdce14 include/xen/interface/io/xenkbd.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/include/xen/interface/io/xenkbd.h	Sat Sep 30 10:34:01 2006 +0200
@@ -0,0 +1,92 @@
+/*
+ * linux/include/linux/xenkbd.h -- Xen virtual keyboard/mouse
+ *
+ * Copyright (C) 2005
+ *
+ *      Anthony Liguori <aliguori@us.ibm.com>
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License. See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+#ifndef _LINUX_XENKBD_H
+#define _LINUX_XENKBD_H
+
+#include <asm/types.h>
+
+/* in events */
+
+#define XENKBD_IN_EVENT_SIZE 40
+
+#define XENKBD_TYPE_MOTION  1     /* mouse movement event */
+#define XENKBD_TYPE_BUTTON  2     /* mouse button event */
+#define XENKBD_TYPE_KEY     3     /* keyboard event */
+
+struct xenkbd_motion
+{
+	__u8 type;         /* XENKBD_TYPE_MOTION */
+	__s16 rel_x;       /* relative X motion */
+	__s16 rel_y;       /* relative Y motion */
+};
+
+struct xenkbd_button
+{
+	__u8 type;         /* XENKBD_TYPE_BUTTON */
+	__u8 pressed;      /* 1 if pressed; 0 otherwise */
+	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */
+};
+
+struct xenkbd_key
+{
+	__u8 type;         /* XENKBD_TYPE_KEY */
+	__u8 pressed;      /* 1 if pressed; 0 otherwise */
+	__u16 keycode;     /* KEY_* from linux/input.h */
+};
+
+union xenkbd_in_event
+{
+	__u8 type;
+	struct xenkbd_motion motion;
+	struct xenkbd_button button;
+	struct xenkbd_key key;
+	char pad[XENKBD_IN_EVENT_SIZE];
+};
+
+/* out events */
+
+#define XENKBD_OUT_EVENT_SIZE 40
+
+/* no out events currently defined */
+
+union xenkbd_out_event
+{
+	__u8 type;
+	char pad[XENKBD_OUT_EVENT_SIZE];
+};
+
+/* shared page */
+
+#define XENKBD_IN_RING_SIZE 2048
+#define XENKBD_IN_RING_LEN (XENKBD_IN_RING_SIZE / XENKBD_IN_EVENT_SIZE)
+#define XENKBD_IN_RING_OFFS 1024
+#define XENKBD_IN_RING(page) \
+    ((union xenkbd_in_event *)((char *)(page) + XENKBD_IN_RING_OFFS))
+#define XENKBD_IN_RING_REF(page, idx) \
+    (XENKBD_IN_RING((page))[(idx) % XENKBD_IN_RING_LEN])
+
+#define XENKBD_OUT_RING_SIZE 1024
+#define XENKBD_OUT_RING_LEN (XENKBD_OUT_RING_SIZE / XENKBD_OUT_EVENT_SIZE)
+#define XENKBD_OUT_RING_OFFS (XENKBD_IN_RING_OFFS + XENKBD_IN_RING_SIZE)
+#define XENKBD_OUT_RING(page) \
+    ((union xenkbd_out_event *)((char *)(page) + XENKBD_OUT_RING_OFFS))
+#define XENKBD_OUT_RING_REF(page, idx) \
+    (XENKBD_OUT_RING((page))[(idx) % XENKBD_OUT_RING_LEN])
+
+struct xenkbd_info
+{
+	__u32 in_cons, in_prod;
+	__u32 out_cons, out_prod;
+};
+
+#endif

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

Steven Smith <sos22@cam.ac.uk> writes:

[...]
>> As far as I can see, this is our only option right now.  I''m
ignorant
>> of `shadow translate mode''; can somebody please educate me on
how to
>> detect and translate shadow translate mode frontend mfns in the
>> backend?
> Shadow translate mode is a mode of the shadow page tables in which Xen
> does machine<->physical address translations on behalf of the guest.
> It''s mostly used for HVM guests to give them the impression of a
> mostly-contiguous block of memory starting at machine address 0.
>
> If you just bung the guest-supplied machine frame numbers through
> xc_translate_gpfn_list you should be fine.  That does the translation
> if you''re in shadow translate mode and does nothing if
you''re not.
Assuming you mean xc_domain_translate_gpfn_list().  I tried
translating fbdev_mfn right before use, as follows:

	ret = xc_domain_translate_gpfn_list(xenfb->xc, domid, 1,
					    &fbdev_mfn, &fbdev_mfn);
	if (ret < 0)
		goto error;
	xenfb->fb_info = xc_map_foreign_range(xenfb->xc, domid, XC_PAGE_SIZE,
					      PROT_READ | PROT_WRITE,
					      fbdev_mfn);
	if (xenfb->fb_info == NULL)
		goto error;

However, xc_domain_translate_gpfn_list() always fails with EINVAL for
me.  If I read its code correctly, this can happen because op.nr_gpfns
is too large, or !shadow_mode_translate().  I suspect its the latter.
Should I call it only when in shadow translate mode?  How to check for
that?  Or am I doing something wrong?

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

On 2/11/06 7:53 am, "Markus Armbruster" <armbru@redhat.com>
wrote:
> However, xc_domain_translate_gpfn_list() always fails with EINVAL for
> me.  If I read its code correctly, this can happen because op.nr_gpfns
> is too large, or !shadow_mode_translate().  I suspect its the latter.
> Should I call it only when in shadow translate mode?  How to check for
> that?  Or am I doing something wrong?
If the call returns -EINVAL you can carry on. The guest is not
shadow-translate and so you already had an MFN in hand in the first place.
We will soon get rid of these translate_gpfn calls anyway, as I''ll
change
Xen to accept GMFNs when mapping foreign pages.

 -- Keir



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

I''m now looking into a conversion to xenbus_driver API.  I started
with copying some voodoo from blkfront.c.  My
xenbus_register_frontend() succeeds.  Stupid question: what triggers
running of the probe function?  Because mine doesn''t.

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

On Thu, Nov 02, 2006 at 11:10:12AM +0100, Markus Armbruster wrote:
> I''m now looking into a conversion to xenbus_driver API.  I started
> with copying some voodoo from blkfront.c.  My
> xenbus_register_frontend() succeeds.  Stupid question: what triggers
> running of the probe function?  Because mine doesn''t.
Either the watch firing when the correct entries in the store are written (for
hotplugging) or it probes those paths at startup.

xenbus/xenbus_probe.c:

frontend_changed ->
dev_changed ->
xenbus_probe_node ->
device_register

or

xenbus_probe_init ->
xenbus_probe ->
xenbus_probe_devices ->
xenbus_probe_device_type ->
bus->probe ->
xenbus_probe_frontend ->
xenbus_probe_node ->
device_register

The device_register causes a the specific frontend device to be registered,
which triggers the probe.

HTH,

Ewan.

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

Ewan Mellor <ewan@xensource.com> writes:
> On Thu, Nov 02, 2006 at 11:10:12AM +0100, Markus Armbruster wrote:
>
>> I''m now looking into a conversion to xenbus_driver API.  I
started
>> with copying some voodoo from blkfront.c.  My
>> xenbus_register_frontend() succeeds.  Stupid question: what triggers
>> running of the probe function?  Because mine doesn''t.
>
> Either the watch firing when the correct entries in the store are written
(for
> hotplugging) or it probes those paths at startup.
>
> xenbus/xenbus_probe.c:
>
> frontend_changed ->
> dev_changed ->
> xenbus_probe_node ->
> device_register
>
> or
>
> xenbus_probe_init ->
> xenbus_probe ->
> xenbus_probe_devices ->
> xenbus_probe_device_type ->
> bus->probe ->
> xenbus_probe_frontend ->
> xenbus_probe_node ->
> device_register
>
> The device_register causes a the specific frontend device to be registered,
> which triggers the probe.
>
> HTH,
>
> Ewan.
Okay, thanks.  Next stupid question: I''m looking for a *user space*
xenbus_register_backend().  Where is it?

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

Ewan Mellor <ewan@xensource.com> writes:
> On Thu, Nov 02, 2006 at 11:10:12AM +0100, Markus Armbruster wrote:
>
>> I''m now looking into a conversion to xenbus_driver API.  I
started
>> with copying some voodoo from blkfront.c.  My
>> xenbus_register_frontend() succeeds.  Stupid question: what triggers
>> running of the probe function?  Because mine doesn''t.
>
> Either the watch firing when the correct entries in the store are written
(for
> hotplugging) or it probes those paths at startup.
>
> xenbus/xenbus_probe.c:
>
> frontend_changed ->
> dev_changed ->
> xenbus_probe_node ->
> device_register
>
> or
>
> xenbus_probe_init ->
> xenbus_probe ->
> xenbus_probe_devices ->
> xenbus_probe_device_type ->
> bus->probe ->
> xenbus_probe_frontend ->
> xenbus_probe_node ->
> device_register
>
> The device_register causes a the specific frontend device to be registered,
> which triggers the probe.
Err, I still don''t get it.  What exactly makes the guest run the
frontend''s probe?  I figure you''re trying to tell me that it
runs when
a certain xenstore entry is present at startup, or when it gets
created later.  Corrrect?  Which entry exactly?  I tried a few (more
voodoo), but no luck.

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

On Thu, Nov 02, 2006 at 04:02:48PM +0100, Markus Armbruster wrote:
> Ewan Mellor <ewan@xensource.com> writes:
> 
> > On Thu, Nov 02, 2006 at 11:10:12AM +0100, Markus Armbruster wrote:
> >
> >> I''m now looking into a conversion to xenbus_driver API. 
I started
> >> with copying some voodoo from blkfront.c.  My
> >> xenbus_register_frontend() succeeds.  Stupid question: what
triggers
> >> running of the probe function?  Because mine doesn''t.
> >
> > Either the watch firing when the correct entries in the store are
written (for
> > hotplugging) or it probes those paths at startup.
> >
> > xenbus/xenbus_probe.c:
> >
> > frontend_changed ->
> > dev_changed ->
> > xenbus_probe_node ->
> > device_register
> >
> > or
> >
> > xenbus_probe_init ->
> > xenbus_probe ->
> > xenbus_probe_devices ->
> > xenbus_probe_device_type ->
> > bus->probe ->
> > xenbus_probe_frontend ->
> > xenbus_probe_node ->
> > device_register
> >
> > The device_register causes a the specific frontend device to be
registered,
> > which triggers the probe.
> 
> Err, I still don''t get it.  What exactly makes the guest run the
> frontend''s probe?  I figure you''re trying to tell me that
it runs when
> a certain xenstore entry is present at startup, or when it gets
> created later.  Corrrect?  Which entry exactly?  I tried a few (more
> voodoo), but no luck.
/local/domain/<domid>/device/<devclass>/<devid>

<devclass> is the string that you''ve put in xenbus_driver.name
when
registering it with xenbus_register_frontend.  The xenbus driver registers a
watch on /local/domain/<domid>/device, and whenever that fires,
dev_changed
looks through the registered drivers, looking for the one with the right
<devclass>, and it all gets kicked off from there.

Ewan.

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

> Okay, thanks.  Next stupid question: I''m looking for a *user
space*
> xenbus_register_backend().  Where is it?
I don''t think there is such a thing at the moment, just because we
don''t currently have any userspace backends.

I think in your particularly case, the easiest thing is probably going
to be to get xend to set up a skeleton xenstore area for the frontend
in the usual way (see DevController.py and e.g. netif.py).  With a
normal in-kernel driver you''d be able to watch the relevant location
to notice when a new device is created, but you don''t have a
permanently-running daemon, which makes that a bit tricky.  I think
it''d be okay to just fake the relevant entry in the store (set
${backend_path}/hotplug-status to connected) from the backend.  It''s a
little skanky, but it should be adequate and reasonably
straightforward to implement.

Once you''ve got that, it should be easy to just do the usual state
machine directly in the backend:

-- Both front and back are put into Initialising by xend.
-- When the backend is ready, it goes to state InitWait and triggers
   the hotplug event.
-- When the frontend is ready, it sets up its area of the store, and goes to
   Initialised.
-- The backend notices this, connects to the ring, and goes to state
   Connected.
-- The frontend notices this, goes to Connected itself, and makes
   itself available to the rest of the operating system.

Teardown is usually started by the frontend, and goes like this:

-- Frontend goes to state Closing
-- Backend disconnects from the ring and goes to state Closing
-- Frontend disconnects from the ring and goes to state Closed
-- Backend goes to Closed and destroys itself, unless online=1 in the
   store, in which case it prepares for further connections.

Xend can also trigger backend teardown by simply deleting the frontend
area, e.g. if the frontend domain crashed.  The normal in-kernel
infrastructure calls otherend_changed with the state
XenbusStateUnknown in that case, which causes the backends to destroy
themselves.

Steven.


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

On Thu, Nov 02, 2006 at 04:19:55PM +0000, Steven Smith wrote:
> > Okay, thanks.  Next stupid question: I''m looking for a *user
space*
> > xenbus_register_backend().  Where is it?
> I don''t think there is such a thing at the moment, just because we
> don''t currently have any userspace backends.
> 
> I think in your particularly case, the easiest thing is probably going
> to be to get xend to set up a skeleton xenstore area for the frontend
> in the usual way (see DevController.py and e.g. netif.py).  With a
> normal in-kernel driver you''d be able to watch the relevant
location
> to notice when a new device is created, but you don''t have a
> permanently-running daemon, which makes that a bit tricky.  I think
> it''d be okay to just fake the relevant entry in the store (set
> ${backend_path}/hotplug-status to connected) from the backend. 
It''s a
> little skanky, but it should be adequate and reasonably
> straightforward to implement.
This is what we do for blktap -- tools/examples/blktap just calls success
immediately, which writes "connected" into the store as you suggest.

Ewan.

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

I tried to follow your advice, but Xend hates me.  I must be doing
something stupid, can anybody help?

I created a trivial xend/server/vfbif.py:

from xen.xend.server.DevController import DevController

class VfbifController(DevController):
    """Virtual frame buffer controller. Handles all vfb devices
for a domain.
    """

    def __init__(self, vm):
        DevController.__init__(self, vm)

    def getDeviceDetails(self, config):
        """@see DevController.getDeviceDetails"""
        devid = 0
        back = {}
        front = {}
        return (devid, back, front)

I added it to xend/XendDomainInfo.py, as follows:

--- XendDomainInfo.py.orig      2006-10-10 16:23:45.000000000 +0200
+++ XendDomainInfo.py   2006-11-03 14:33:28.000000000 +0100
@@ -1786,7 +1786,7 @@
     controllerClasses[device_class] = cls
 
 
-from xen.xend.server import blkif, netif, tpmif, pciif, iopif, irqif, usbif
+from xen.xend.server import blkif, netif, tpmif, pciif, iopif, irqif, usbif,
vfbif
 from xen.xend.server.BlktapController import BlktapController
 addControllerClass(''vbd'',  blkif.BlkifController)
 addControllerClass(''vif'',  netif.NetifController)
@@ -1796,3 +1796,4 @@
 addControllerClass(''irq'',  irqif.IRQController)
 addControllerClass(''usb'',  usbif.UsbifController)
 addControllerClass(''tap'',  BlktapController)
+addControllerClass(''vfb'',  vfbif.VfbifController)


I set hotplug-status in the backend, resulting in this:

    # xenstore-ls /local/domain/0/backend
    vfb = ""
     1 = ""
      0 = ""
       hotplug-status = "connected"

Then I attempt to create domain 1:

    # xm create -c parafat
[pygrub screen...]
    Using config file "/etc/xen/parafat".
    Going to boot Fedora Core (2.6.18.1)
      kernel: /vmlinuz-2.6.18.1
      initrd: /initrd-2.6.18.1.img
    Error: Device 0 not connected

This is actually from configuration when it tries to read
/local/domain/1/device/vfb/0/backend-id.  /local/domain/1/device
contains entries vbd and vif, but not vfb.  Shouldn''t xend have
written stuff there?

I''m confused.

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

> I created a trivial xend/server/vfbif.py:
Looks plausible.  You probably want to have a createDevice method
which calls DevController.createDevice and then spawns the backend,
but that can come later.
> I added it to xend/XendDomainInfo.py, as follows:
I think this bit is correct.

I think this should work if your SXP includes an entry for the
backend.  Adding something like ``(device (vfb ()))'''' to the
end of
the (vm) section should work.

``xm create -n'''' will convert an ordinary xm configuration
file to SXP
format, and ``xm create -C'''' can create a domain directly from
the
SXP.  Eventually, you''ll want to add something to the xm parser which
makes it possible to do this without directly editting the SXP, but
that can wait for a bit.

Steven.


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