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@xxxxxxxxxx>
> Cc: Markus Armbruster <armbru@xxxxxxxxxx>
> Cc: Anthony Liguori <aliguori@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + *
> + * 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.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home