WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xenfb: make restartable
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Fri, 15 Aug 2008 08:39:06 -0400
Cc: Xen Development Mailing List <xen-devel@xxxxxxxxxxxxxxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>
Delivery-date: Fri, 15 Aug 2008 15:16:58 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20080814180251.GD4590@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> (Samuel Thibault's message of "Thu\, 14 Aug 2008 19\:02\:51 +0100")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <48A40B17.2090701@xxxxxxxxxx> <20080814111520.GH4590@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <48A4232B.6040500@xxxxxxxxxx> <20080814122931.GL4590@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <48A43EAD.7040701@xxxxxxxxxx> <20080814142849.GS4590@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <48A44967.4060401@xxxxxxxxxx> <20080814151359.GT4590@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20080814180251.GD4590@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux)
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