Re: [Xen-devel] [PATCH] xenfb: make restartable

[Markus Armbruster <armbru@xxxxxxxxxx>]

Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx> writes:

> Samuel Thibault, le Thu 14 Aug 2008 16:14:00 +0100, a Ãcrit :
>> > Another pvgrub issue:  Is booting guests with the framebuffer enabled
>> > supposed to work?
>> 
>> Yes, but you have to not enable the framebuffer in the grub
>> configuration, because the framebuffer is currently not able to restart.
>
> Oh actually it was a lot easier than I thought, see patch below (which
> is actually partly bug fixes).
>
>
>
> xenfb: make restartable
>
> - Fix the pixel unmapping, which should happen during the Closing state.
> - Fix qemu segfault when a guest shuts its fb down.
> - Once connected, if frontend state goes from Closed to Initialized,
> restart the connection loop.
>
> Signed-off-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>
>
> diff -r 38783464a671 extras/mini-os/fbfront.c
> --- a/extras/mini-os/fbfront.c        Thu Aug 14 16:16:44 2008 +0100
> +++ b/extras/mini-os/fbfront.c        Thu Aug 14 18:59:27 2008 +0100
[Not familiar with this code...]
> diff -r 38783464a671 tools/ioemu/hw/xenfb.c
> --- a/tools/ioemu/hw/xenfb.c  Thu Aug 14 16:16:44 2008 +0100
> +++ b/tools/ioemu/hw/xenfb.c  Thu Aug 14 18:59:27 2008 +0100
> @@ -445,6 +445,13 @@ static void xenfb_unbind(struct xenfb_de
>               xc_evtchn_unbind(dev->xenfb->evt_xch, dev->port);
>               dev->port = -1;
>       }
> +
> +     if (!strcmp(dev->devicetype, "vfb")) {
> +         if (dev->xenfb->pixels) {
> +                 munmap(dev->xenfb->pixels, dev->xenfb->fb_len);
> +                 dev->xenfb->pixels = NULL;
> +         }
> +     }
>  }
>  
>  

The check for vfb is ugly.  The only alternative I can see is fb ==
dev->xenfb->fb.  Matter of taste.

Also somewhat ugly is the fact that xenfb_unbind() is no longer the
exact reverse of xenfb_bind(), but reverses xenfb_map_fb() as well.
Could be avoided by creating xenfb_unmap_fb() and calling it after
xenfb_unbind() from xenfb_detach_dom() unconditionally, and from
xenfb_on_state_change() when dev is fb.

> @@ -452,10 +459,6 @@ static void xenfb_detach_dom(struct xenf
>  {
>       xenfb_unbind(&xenfb->fb);
>       xenfb_unbind(&xenfb->kbd);
> -     if (xenfb->pixels) {
> -             munmap(xenfb->pixels, xenfb->fb_len);
> -             xenfb->pixels = NULL;
> -     }
>  }
>  
>  /* Remove the backend area in xenbus since the framebuffer really is
> @@ -653,6 +656,16 @@ static int xenfb_on_state_change(struct 
>                  not much point in us continuing. */
>               return -1;
>       case XenbusStateInitialising:
> +             if (dev->state != XenbusStateClosed)
> +                 /* Do not let a domain make us skip the closing state */
> +                 return 0;

Why does this work?

> +             xenfb_switch_state(dev, XenbusStateInitWait);
> +             xs_unwatch(dev->xenfb->xsh, dev->otherend, "");
> +             if (!strcmp(dev->devicetype, "vkbd")) {
> +                 fprintf(stderr, "FB: Waiting for KBD backend creation\n");
> +                 xenfb_wait_for_frontend(&dev->xenfb->kbd, 
> xenfb_frontend_initialized_kbd);
> +             }
> +             break;

Man, I hate this state machine...  not your fault.

Gerd's new one is much clearer (I asked for that).

>       case XenbusStateInitWait:
>       case XenbusStateInitialised:
>       case XenbusStateConnected:
> @@ -1274,6 +1287,9 @@ static void xenfb_update(void *opaque)
>      struct xenfb *xenfb = opaque;
>      int period;
>  
> +    if (!xenfb->fb.page)
> +        return;
> +
>      if (xenfb_queue_full(xenfb))
>          return;
>  
>

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