> > 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@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.
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.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home