Re: [PATCH 2/2] PV framebuffer

[Markus Armbruster <armbru@redhat.com>]

Steven Smith <sos22-xen@srcf.ucam.org> writes:

>> PV framebuffer frontend.  Derived from http://hg.codemonkey.ws/vncfb
> This is the backend, isn't it?
>
>> diff -r 2e35cf028ff0 tools/python/xen/xend/XendDevices.py
>> --- a/tools/python/xen/xend/XendDevices.py	Thu Nov 09 15:43:24 2006 +0000
>> +++ b/tools/python/xen/xend/XendDevices.py	Thu Nov 09 17:58:26 2006 +0100
>> @@ -41,6 +41,8 @@ class XendDevices:
>>          'irq': irqif.IRQController,
>>          'usb': usbif.UsbifController,
>>          'tap': BlktapController,
>> +        'vfb': vfbif.VfbifController,
>> +        'vkbd': vfif.VkbdifController,
> Typo?  Should this be:
>
>   +        'vkbd': vfbif.VkbdifController,
>
> ?

Yes.  Thanks!

>> diff -r 2e35cf028ff0 tools/xenfb/vncfb.c
>> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/vncfb.c       Thu Nov 09 10:18:58 2006 +0100
>
>> +int main(int argc, char **argv)
>> +{
>> +	rfbScreenInfoPtr server;
>> +	char *fake_argv[7] = { "vncfb", "-rfbport", "5901", 
>> +                               "-desktop", "xen-vncfb", 
>> +                               "-listen", "0.0.0.0" };
> HVM VNC server defaults to listening on 127.0.0.1 now.  You might want
> to make this do the same thing.

Okay.

>> diff -r 2e35cf028ff0 tools/xenfb/xenfb.c
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/xenfb.c	Thu Nov 09 10:22:36 2006 +0100
>
>> +static int xenfb_hotplug(struct xs_handle *xsh, const char *backdir)
>> +{
>> +	if (xenfb_xs_printf(xsh, backdir, "hotplug-status", "connected"))
>> +		return -1;
>> +
>> +	if (!xs_watch(xsh, backdir, ""))
>> +		return -1;
>> +
>> +	switch (xenfb_wait_for_state(xsh, backdir,
>> +#if 1 /* TODO fudging state to permit restarting; to be removed */
>> +			(1 << XenbusStateInitialising)
>> +			| (1 << XenbusStateInitWait)
>> +			| (1 << XenbusStateConnected)
>> +#else
>> +			1 << XenbusStateInitialising,
>> +#endif
> I'm not sure what you're trying to do here.  It looks like you're
> waiting for the backend to go to state Initialising (or InitWait, or
> Connected), but this is the backend, so if it isn't there already
> you're going to wait forever.

Part of the crude hacks that let me kill and restart the backend
without disturbing the front end.

Here we wait for xend to create the initial xenstore nodes for us, by
waiting for the transition to XenbusStateInitialising.  If we
reconnect, we're already past that state.

Should go away when we implement reconnect cleanly.

>> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
>> +{
> ...
>> +	if (xenfb_hotplug(xsh, vfb_backdir) < 0)
>> +		goto error;
>> +	if (xenfb_hotplug(xsh, vkbd_backdir) < 0)
>> +		goto error;
>> +
>> +	if (xenfb_xs_printf(xsh, vkbd_backdir, "feature-abs-pointer", "1"))
>> +		goto error;
>> +	if (xenfb_xs_printf(xsh, vfb_backdir, "state", "%d",
>> +			    XenbusStateInitWait))
>> +		goto error;
>> +	if (xenfb_xs_printf(xsh, vkbd_backdir, "state", "%d",
>> +			    XenbusStateInitWait))
>> +		goto error;
>> +
> I'd probably reorder this a little to look more like this:
>
> (1) Set feature-abs-pointer
> (2) Set state to InitWait
> (3) Set hotplug status
>
> The only actual *required* constraint is (1) before (2), so that the
> frontend doesn't initialise before we've set the feature and
> potentially miss it.
>
> (2) before (3) is kind of nice, in that it makes sure that the backend
> is ready before xend unpauses the domain, and so the frontend'll be
> able to connect the first time it tries, but that's a lot less
> important here than for e.g. block devices.

Based on our previous discussions, I designed the startup protocol
this way:

    backend                          frontend
    ------------------------------------------------------------------
    hotplug_status = connected
    [makes xend populate xenstore, set fe and be state = Initialising]
    wait for be state = Initialising
      [i.e. wait for xend]
    write xs: feature-abs-pointer    write xs: feature-update
    be state = InitWait              fe state = Initialised
    ------------------------------ sync ------------------------------
    wait for fe state = Initialised  wait for be state = InitWait
    ------------------------------ sync ------------------------------
    read xs: feature-update          read xs: feature-abs-pointer
    write xs: request-update         write xs: request-abs-pointer
    be state = Connected             fe state = Connected
    ------------------------------ sync ------------------------------
    wait for fe state = Connected    wait for be state = Connected
    ------------------------------ sync ------------------------------
    read xs: request-abs-pointer     read xs: request-update

The symmetry made sense to me.

If this is not the right way to do it, please explain it to me once
again.  A diagram like the one I provided above might be more precise
and thus useful than prose, even if it's just as crude as mine.

>> + error:
>> +	serrno = errno;
>> + 	if (xenfb->fb)
>> + 		munmap(xenfb->fb, xenfb->fb_len);
>> + 	if (fbmfns)
>> +		munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
>> +        if (xenfb->kbd_info)
>> +		munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +        if (xenfb->fb_info)
>> +		munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +        if (xenfb->kbd_port);
>> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +        if (xenfb->fbdev_port)
>> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +        if (xsh) 
>> +		xs_daemon_close(xsh);
>> +        errno = serrno;
> It'd be good to set an error in xenbus when this fails.  Look at
> xenbus_dev_fatal and _dev_error in drivers/xen/xenbus/xenbus_client.c
> to see how to do this.

Thanks, I'll look into that.

>> +        return false;
>> +}
>> +
>
> Steven.