Steven Smith <sos22-xen@xxxxxxxxxxxxx> 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@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.
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@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?
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@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.
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@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?
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@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.
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home