Quoting Markus Armbruster <armbru@xxxxxxxxxx>:
> > Here are the comments:
> > I don't really understand why you need to use vmalloc in so many
> > places, but that's could just be because I'm missing something.
>
> Huh? I can see just two places, both in xenfb_probe():
What would be the alternatives to vmalloc? Grant tables are unusable in their
current form because of the amount of memory needed to allocate (grant tables
are limited to 1024 4k references ATM I believe). The kmalloc() pool is
limited and my understanding is that it's not meant for very large allocations.
I'm not an expert here though so alternative suggestions are appreciated.
> * the frame buffer
>
> * the array of mfns for the frame buffer
>
> Anthony, care to explain why?
>
> >> +config XEN_KEYBOARD
> >> + tristate "Keyboard-device frontend driver"
> >> + depends on XEN
> >> + default y
> >> + help
> >> + The keyboard-device frontend driver allows the kernel to create a
> >> + virtual keyboard. This keyboard can then be driven by another
> >> + domain. If you've said Y to CONFIG_XEN_FRAMEBUFFER, you probably
> >> + want to say Y here.
> >> +
> > What happens if you configure XEN_KEYBOARD but not XEN_FRAMEBUFFER?
>
> The backend refuses to connect, i.e. you can't use it. Anthony, can
> you shed some light on why you created two separate configs?
They are two separate devices. Seemed like a natural separation to me.
> > Also, this appears to provide mouse support, and so is misnamed.
>
> It's a device producing input events, both keyboard and mouse. Care
> to suggest a better name?
There are keyboards that also have built in mice (typically a little pad in the
upper corner). Naming has never been my strong point though :-)
> >> +static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
> >> + struct pt_regs *regs)
> >> +{
> >> + struct xenfb_info *info = dev_id;
> >> + __u32 cons, prod;
> >> +
> >> + if (!info->page || !info->page->initialized)
> >> + return IRQ_NONE;
> >> +
> >> + prod = info->page->in_prod;
> >> + rmb(); /* ensure we see ring contents up to prod */
> >> + for (cons = info->page->in_cons; cons != prod; cons++) {
> >> + union xenfb_in_event *event;
> >> + event = &XENFB_RING_REF(info->page->in, cons);
> >> + notify_remote_via_evtchn(info->evtchn);
> >> + }
> > I don't think you need to do a notify on every pass of the loop. In
> > fact, I don't think you need any notify_remote, or in fact any loop at
> > all. What is this trying to achieve?
You could batch up the removal of all events and change the semantics of the
notification to "I've changed the queue in some way" verses "I've handled a
single event on the queue".
You do need *some* sort of notification so that if the other end is buffering
writes, it can know to retry a write.
Why even bother with the loop at all? Forward compatibility. When we add
backend events (and we will at some point), we can make them, semantically,
suggestions. This lets the front-end ignore requests that it doesn't
understand. At this case, it doesn't understand any so it just ignores them
all. However, if we didn't do the event channel notification, any future
clients would break as the queue would fill up.
> >> + info->page->in_cons = cons;
> >> +
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static unsigned long vmalloc_to_mfn(void *address)
> >> +{
> >> + return pfn_to_mfn(vmalloc_to_pfn(address));
> > Maybe use arbitrary_virt_to_machine?
If that existed when I wrote the code, I didn't know about it :-)
> >> +{
> > ...
> >> + info->page->width = 800;
> >> + info->page->height = 600;
> >> + info->page->depth = 32;
> > It might be nice to make these #defines somewhere.
>
> Matter of taste. Done.
I personally dislike single-use defines...
> >> +
> >> + ret = register_framebuffer(fb_info);
> >> + if (ret)
> >> + goto error_unbind;
> >> +
> >> + again:
> >> + ret = xenbus_transaction_start(&xbt);
> >> + if (ret)
> >> + goto error_unreg;
> >> + ret = xenbus_printf(xbt, "vfb", "page-ref", "%lu",
> >> + virt_to_mfn(info->page));
> > Grant tables?
>
> Elaborate, please.
Grant tables are a mechanism to do generic memory sharing between domains. It's
limited in its current form to small amounts of memory but it would be
appropriate for the queues at least. When I first wrote this, I wasn't using
XenBus so that it would be available early in boot. However, now there we're
using XenBus, grant tables would be a good option.
> >> +/* out events */
> >> +
> >> +#define XENFB_TYPE_MOTION 1
> > It's not obvious to me what XENFB_TYPE_MOTION would do, and it doesn't
> > appear to ever get generated.
>
> Appears to be a sketch of future work. I don't understand it either.
> Anthony?
It's perhaps poorly named. It's for supporting hardware cursor offloading. At
one point, I had hacked the fbdev driver to actually use hardware cursor
offloading and was generating these events. It could be removed until we
support cursor offloading.
> >> +union xenfb_out_event
> >> +{
> >> + __u8 type;
> > Does this really want to be in the union with the actual out_events?
>
> Convenient way to access the type. Sure, you could just access
> update.type, but that suggests we already know it's update, which is
> somewhat misleading when we don't.
It's a matter of personal taste.
> > Is it really a good idea for these to be on the shared page? What
> > happens if the {front,back}end is malicious and modifies them while
> > the framebuffer is operating normally?
Are we really concerned about malicious front-end and back-ends? A malicious
back-end disk driver could send data containing malware. Why would we concern
ourselves with that though?
> >> +
> >> + unsigned long pd[2];
> > This could do with a better name. Also, pd[1] appears to always be 0.
>
> I suspect it's short for `page directory'. Shall we call it pgdir?
>
> Yes, pd[1] is always 0 and appears not to be used. Anthony, why is pd
> an array?
To support very large framebuffers (1900x1200) you need > 4MB of data. The size
of the pd[] could be variable but an 8MB framebuffer seems large enough for
quite a long time...
> >> +
> >> + __u32 in_cons, in_prod;
> >> + __u32 out_cons, out_prod;
> >> +
> >> + union xenfb_in_event in[XENFB_IN_RING_SIZE];
> > As far as I can see, there's only ever one xenfb_in_event generated
> > throughout the life of the buffer, and it's ignored. Is this queue
> > necessary?
Forward compatibility. There will be more events in the future and preserving
guest ABI compatibility is important...
>
> Again, the protocol is designed for extendability. This is a
> placeholder for future xenkbd output events.
Think things like caps-lock enable. It will be needed eventually. The code is
there now to establish an ABI.
> >> +
> >> +/* shared page */
> >> +
> >> +#define XENKBD_IN_RING_SIZE (2048 / 40)
> >> +#define XENKBD_OUT_RING_SIZE (1024 / 40)
> > Why 2048 and 1024?
I assumed that input events would be more frequent (key events) than output
events (caps-lock on). We have other asymmetric queues in Xen (I believe the
console is asymmetric too).
> I fear the code doesn't implement the spec... rings follow info
> immediately, so they'll wander when info is extended. Want me to fix
> this?
Right, good catch!
Regards,
Anthony Liguori
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home