Re: [Xen-devel] [PATCH] Paravirt framebuffer backend tools [2/5]

[Markus Armbruster <armbru@xxxxxxxxxx>]

Steven Smith <sos22-xen@xxxxxxxxxxxxx> writes:

>> > -- The setup protocol doesn't look much like the normal xenbus state
>> >    machine.  There may be a good reason for this, but I haven't heard
>> >    one yet.  I know the standard way is badly documented and
>> >    non-trivial to understand from the existing implementations; sorry
>> >    about that.
>> Anthony wrote it that way.  I don't know why, but it's simple, and it
>> works.
>> 
>> I'm most reluctant to throw out working code while there's so much
>> code that needs fixing.  That situation is improving, though.
> Okay.
>
>> >> + for (;;) {
>> >> +         FD_ZERO(&readfds);
>> >> +         FD_SET(fd, &readfds);
>> >> +         tv = (struct timeval){0, 10000};
>> >> +
>> >> +         if (select(fd + 1, &readfds, NULL, NULL, &tv) < 0) {
>> >> +                 if (errno == EINTR)
>> >> +                         continue;
>> >> +                 fprintf(stderr, "Connection to domain broke (%s)\n",
>> >> +                         strerror(errno));
>> > It's not really the connection to the domain that's broken as the
>> > event channel device.  Not terribly important.
>> Happy to reword it; suggestions?
> I dunno; maybe something like ``select() failed on event channel
> device (%s)\n"?  I'd rather have a message which most users would find
> meaningless than one which is actually misleading.  Given that this
> should almost never happen I don't think it's worth worrying about
> that much.

Works for me.

>> >                  What happens if someone has a four, five, six button
>> > mouse?
>> The extra buttons get mapped to negative values here (ugh), truncated
>> to __u8 in xenfb_send_button() (double-ugh) and thrown away in the
>> frontend.  The 256th button would get misinterpreted as the first one,
>> though :)
> I'd prefer to discard invalid events here in the backend if possible,
> just on the general principle that sending garbage across the ring
> buffer is a bad idea even if you know it's going to get discarded.

Agreed.

>> The xenkbd protocol provides for three mouse buttons.  From xenkbd.h:
>> 
>>      __u8 button;       /* the button (0, 1, 2 is right, middle, left) */
>> 
>> This is extensible as long as we handle unknown buttons gracefully.  I
>> do hate the weird encoding of buttons, though.  Too hebrew for my
>> taste.
> It's certainly not how I would have done it, but I don't think it
> makes much difference.
>
>> SDL provides for five buttons (SDL_BUTTON_LEFT, SDL_BUTTON_MIDDLE,
>> SDL_BUTTON_RIGHT, SDL_BUTTON_WHEELUP, SDL_BUTTON_WHEELDOWN).
>> 
>> LibVNCServer provides for 8 buttons.
>> 
>> The kernel input layer provides for 8 mouse buttons (BTN_LEFT,
>> BTN_RIGHT, BTN_MIDDLE, BTN_SIDE, BTN_EXTRA, BTN_FORWARD, BTN_BACK,
>> BTN_TASK).
>> 
>> How to best map buttons from VNC and SDL to input beyond the first
>> three?  If we can find a workable answer for that, providing for more
>> buttons should be trivial.
> *shrug* We might as well just send input layer codes across the ring
> buffer and do the translation in the backend.  That makes the Linux
> frontend easier and doesn't make other operating systems any harder.
> If we later find that some other operating system supports buttons
> which the input layer doesn't then we can figure out what to do with
> them then; provided we make the frontend discard buttons it doesn't
> understand it should be trivial to do this in a backwards compatible
> way.

The input layer treats mouse buttons just like keys.  If we use its
button encoding, we can just as well use XENKBD_TYPE_KEY and drop
XENKBD_TYPE_BUTTON.  Any objections?

[...]
>> >> + n_fbdirs = n_fbmfns * sizeof(unsigned long);
>> > ...
>> >> + n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>> >> +
>> >> + fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ, 
>> >> xenfb->fb_info->pd, n_fbdirs);
>> >> + if (fbmfns == NULL)
>> >> +         goto error;
>> >> +
>> >> + xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ | 
>> >> PROT_WRITE, fbmfns, n_fbmfns);
>> >> + if (xenfb->fb == NULL)
>> >> +         goto error;
>> > Irritatingly, map_foreign_batch doesn't actually return success or
>> > failure through its return value, but by setting the high bits on the
>> > failed entry in the array you pass in.  If the array is mapped
>> > readonly, or is shared with a remote domain, there's no way to detect
>> > failure.
>> Sounds like a design flaw to me.
> That's one way of putting it, certainly. :)
>
>> xc_map_foreign_batch() returns NULL on obvious failures, but the
>> ioctl() might cause the behavior you described.
>> 
>> I looked for other users of xc_map_foreign_batch() and I can't see
>> them checking success other than by examining the return value.
>> 
>> fbmfns[] is mapped PROT_READ from the frontend's domain.
> I think the correct fix here is probably just to fix up
> xc_map_foreign_batch() to have a more sane calling convention.  As you
> say, most of its existing users seem to make the same assumptions as
> have happened here.  I can't actually see any good way of dealing with
> this in the standard libxc API.
>
>> > You might also want to consider using xc_map_foreign_ranges, since
>> > that has a useful return value, but it would mean copying the pfn
>> > arrays and translating them to a slightly different format.
>> Isn't that function private?
> Oops, yes, sorry, forgot about that.
>
>> What do you want me to do here?
> I'd leave it as a known bug for now.  We'll add a new interface to
> libxc once 3.0.3's dealt with.

Excellent.

>> >> +
>> >> + event.type = XENFB_TYPE_SET_EVENTS;
>> >> + event.set_events.flags = XENFB_FLAG_UPDATE;
>> >> + if (xenfb_fb_event(xenfb, &event))
>> >> +         goto error;
>> > Would it make sense to negotiate this via xenbus at connection setup
>> > time?  It's not like it's likely to change during the life of the
>> > buffer.
>> Can you point to an example of such a negotiation between a frontend
>> and a backend via xenbus?
> The netif feature flags are probably the most obvious example.  For
> instance, to turn on copy rx mode, you first have the backend put
> feature-rx-copy=1 in its xenstore area, and then when the frontend
> connects it notices this and puts request-rx-copy=1 in its area.  The
> backend reads this out as part of connection setup, and rx copy mode
> is turned on.
>
> The equivalent in this case would be for the backend to set
> request-update=1 in its area when it starts, and then for the frontend
> to set provides-update=1 if appropriate.

I'll look into this when/if I find the time.

> Of course, this sort of thing only works well for flags which don't
> change while the buffer is live.  I'd certainly expect
> XENFB_FLAG_UPDATE to fit into that category, but perhaps you have some
> future plans which wouldn't work well with this?

I can't guarantee we won't need a mechanism to switch things during
operation at some time, but neither can I guarantee that
XENFB_TYPE_SET_EVENTS as it is now will do for that then.

Instead of attempting to cover all possible future cases of option
negotiation / mode switching, let's just provide for what we need now
in a simple and reasonably general way.

[...]

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