diff mbox

[v2] xenfb: remove xen_init_display "temporary" hack

Message ID alpine.DEB.2.10.1706281125330.2919@sstabellini-ThinkPad-X260 (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Stabellini June 28, 2017, 6:37 p.m. UTC 
Initialize xenfb properly, as all other backends, from its own
"initialise" function.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

---
Changes in v2:
- remove xen_init_display from xen_backend.h
- handle cases where vkbd is missing

Comments

Paul Durrant June 29, 2017, 8:24 a.m. UTC | #1 
> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 28 June 2017 19:37
> To: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> Cc: sstabellini@kernel.org; peter.maydell@linaro.org; Anthony Perard
> <anthony.perard@citrix.com>; kraxel@redhat.com; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: [PATCH v2] xenfb: remove xen_init_display "temporary" hack
> 
> Initialize xenfb properly, as all other backends, from its own
> "initialise" function.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
> Changes in v2:
> - remove xen_init_display from xen_backend.h
> - handle cases where vkbd is missing
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index e76c0d8..3b0168b 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -71,7 +71,6 @@ struct XenFB {
>      int               fbpages;
>      int               feature_update;
>      int               bug_trigger;
> -    int               have_console;
>      int               do_resize;
> 
>      struct {
> @@ -80,6 +79,7 @@ struct XenFB {
>      int               up_count;
>      int               up_fullscreen;
>  };
> +static const GraphicHwOps xenfb_ops;
> 
>  /* -------------------------------------------------------------------- */
> 
> @@ -855,6 +855,8 @@ static int fb_init(struct XenDevice *xendev)
>  static int fb_initialise(struct XenDevice *xendev)
>  {
>      struct XenFB *fb = container_of(xendev, struct XenFB, c.xendev);
> +    struct XenDevice *xin;
> +    struct XenInput *in;

I think the scope of 'in' can be limited to the 'else' clause where it is used below.

>      struct xenfb_page *fb_page;
>      int videoram;
>      int rc;
> @@ -877,16 +879,16 @@ static int fb_initialise(struct XenDevice *xendev)
>      if (rc != 0)
>  	return rc;
> 
> -#if 0  /* handled in xen_init_display() for now */
> -    if (!fb->have_console) {
> -        fb->c.ds = graphic_console_init(xenfb_update,
> -                                        xenfb_invalidate,
> -                                        NULL,
> -                                        NULL,
> -                                        fb);
> -        fb->have_console = 1;
> +    fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb);
> +
> +    xin = xen_pv_find_xendev("vkbd", xen_domid, 0);
> +    if (xin == NULL) {
> +        xen_pv_printf(xendev, 1, "xenfb is up, but vkbd is not present\n");
> +    } else {
> +        in = container_of(xin, struct XenInput, c.xendev);
> +        in->c.con = fb->c.con;

I notice that in->c.con is tested in input_initialise() and it will fail if NULL, so there is an inherent race between the frontends and also processing of xenstore watches. That seems a little fragile at best, but the dependency of input_initialise() on in->c.con is also somewhat bogus as the vkbd backend is created for xenfv machine types whereas xenfb is only there for xenpv machine types. I think this needs to be fixed before xenfb can be relied on to initialize correctly in all circumstances.

  Paul

> +        xen_be_check_state(xin);
>      }
> -#endif
> 
>      if (xenstore_read_fe_int(xendev, "feature-update", &fb-
> >feature_update) == -1)
>  	fb->feature_update = 0;
> @@ -972,42 +974,3 @@ static const GraphicHwOps xenfb_ops = {
>      .gfx_update  = xenfb_update,
>      .update_interval = xenfb_update_interval,
>  };
> -
> -/*
> - * FIXME/TODO: Kill this.
> - * Temporary needed while DisplayState reorganization is in flight.
> - */
> -void xen_init_display(int domid)
> -{
> -    struct XenDevice *xfb, *xin;
> -    struct XenFB *fb;
> -    struct XenInput *in;
> -    int i = 0;
> -
> -wait_more:
> -    i++;
> -    main_loop_wait(true);
> -    xfb = xen_pv_find_xendev("vfb", domid, 0);
> -    xin = xen_pv_find_xendev("vkbd", domid, 0);
> -    if (!xfb || !xin) {
> -        if (i < 256) {
> -            usleep(10000);
> -            goto wait_more;
> -        }
> -        xen_pv_printf(NULL, 1, "displaystate setup failed\n");
> -        return;
> -    }
> -
> -    /* vfb */
> -    fb = container_of(xfb, struct XenFB, c.xendev);
> -    fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb);
> -    fb->have_console = 1;
> -
> -    /* vkbd */
> -    in = container_of(xin, struct XenInput, c.xendev);
> -    in->c.con = fb->c.con;
> -
> -    /* retry ->init() */
> -    xen_be_check_state(xin);
> -    xen_be_check_state(xfb);
> -}
> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> index 79aef4e..31d2f25 100644
> --- a/hw/xenpv/xen_machine_pv.c
> +++ b/hw/xenpv/xen_machine_pv.c
> @@ -94,9 +94,6 @@ static void xen_init_pv(MachineState *machine)
> 
>      /* config cleanup hook */
>      atexit(xen_config_cleanup);
> -
> -    /* setup framebuffer */
> -    xen_init_display(xen_domid);
>  }
> 
>  static void xenpv_machine_init(MachineClass *mc)
> diff --git a/include/hw/xen/xen_backend.h
> b/include/hw/xen/xen_backend.h
> index 852c2ea..8a6fbcb 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -55,8 +55,6 @@ extern struct XenDevOps xen_netdev_ops;       /*
> xen_nic.c         */
>  extern struct XenDevOps xen_usb_ops;          /* xen-usb.c         */
>  #endif
> 
> -void xen_init_display(int domid);
> -
>  /* configuration (aka xenbus setup) */
>  void xen_config_cleanup(void);
>  int xen_config_dev_blk(DriveInfo *disk);
Stefano Stabellini July 6, 2017, 11:37 p.m. UTC | #2 
On Thu, 29 Jun 2017, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > Sent: 28 June 2017 19:37
> > To: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> > Cc: sstabellini@kernel.org; peter.maydell@linaro.org; Anthony Perard
> > <anthony.perard@citrix.com>; kraxel@redhat.com; Paul Durrant
> > <Paul.Durrant@citrix.com>
> > Subject: [PATCH v2] xenfb: remove xen_init_display "temporary" hack
> > 
> > Initialize xenfb properly, as all other backends, from its own
> > "initialise" function.
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > ---
> > Changes in v2:
> > - remove xen_init_display from xen_backend.h
> > - handle cases where vkbd is missing
> > 
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > index e76c0d8..3b0168b 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -71,7 +71,6 @@ struct XenFB {
> >      int               fbpages;
> >      int               feature_update;
> >      int               bug_trigger;
> > -    int               have_console;
> >      int               do_resize;
> > 
> >      struct {
> > @@ -80,6 +79,7 @@ struct XenFB {
> >      int               up_count;
> >      int               up_fullscreen;
> >  };
> > +static const GraphicHwOps xenfb_ops;
> > 
> >  /* -------------------------------------------------------------------- */
> > 
> > @@ -855,6 +855,8 @@ static int fb_init(struct XenDevice *xendev)
> >  static int fb_initialise(struct XenDevice *xendev)
> >  {
> >      struct XenFB *fb = container_of(xendev, struct XenFB, c.xendev);
> > +    struct XenDevice *xin;
> > +    struct XenInput *in;
> 
> I think the scope of 'in' can be limited to the 'else' clause where it is used below.
> 
> >      struct xenfb_page *fb_page;
> >      int videoram;
> >      int rc;
> > @@ -877,16 +879,16 @@ static int fb_initialise(struct XenDevice *xendev)
> >      if (rc != 0)
> >  	return rc;
> > 
> > -#if 0  /* handled in xen_init_display() for now */
> > -    if (!fb->have_console) {
> > -        fb->c.ds = graphic_console_init(xenfb_update,
> > -                                        xenfb_invalidate,
> > -                                        NULL,
> > -                                        NULL,
> > -                                        fb);
> > -        fb->have_console = 1;
> > +    fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb);
> > +
> > +    xin = xen_pv_find_xendev("vkbd", xen_domid, 0);
> > +    if (xin == NULL) {
> > +        xen_pv_printf(xendev, 1, "xenfb is up, but vkbd is not present\n");
> > +    } else {
> > +        in = container_of(xin, struct XenInput, c.xendev);
> > +        in->c.con = fb->c.con;
> 
> I notice that in->c.con is tested in input_initialise() and it will fail if NULL, so there is an inherent race between the frontends and also processing of xenstore watches. That seems a little fragile at best, but the dependency of input_initialise() on in->c.con is also somewhat bogus as the vkbd backend is created for xenfv machine types whereas xenfb is only there for xenpv machine types. I think this needs to be fixed before xenfb can be relied on to initialize correctly in all circumstances.

Yes, you are right that it is fragile. Also, there is no need for this
dependency: I'll remove it, and use qemu_console_lookup_by_index
instead.

Owen's patch will take care of making the qemu_console_lookup_by_index
call conditional on xenfb->raw_pointer_wanted != 1. 


> > +        xen_be_check_state(xin);
> >      }
> > -#endif
> > 
> >      if (xenstore_read_fe_int(xendev, "feature-update", &fb-
> > >feature_update) == -1)
> >  	fb->feature_update = 0;
> > @@ -972,42 +974,3 @@ static const GraphicHwOps xenfb_ops = {
> >      .gfx_update  = xenfb_update,
> >      .update_interval = xenfb_update_interval,
> >  };
> > -
> > -/*
> > - * FIXME/TODO: Kill this.
> > - * Temporary needed while DisplayState reorganization is in flight.
> > - */
> > -void xen_init_display(int domid)
> > -{
> > -    struct XenDevice *xfb, *xin;
> > -    struct XenFB *fb;
> > -    struct XenInput *in;
> > -    int i = 0;
> > -
> > -wait_more:
> > -    i++;
> > -    main_loop_wait(true);
> > -    xfb = xen_pv_find_xendev("vfb", domid, 0);
> > -    xin = xen_pv_find_xendev("vkbd", domid, 0);
> > -    if (!xfb || !xin) {
> > -        if (i < 256) {
> > -            usleep(10000);
> > -            goto wait_more;
> > -        }
> > -        xen_pv_printf(NULL, 1, "displaystate setup failed\n");
> > -        return;
> > -    }
> > -
> > -    /* vfb */
> > -    fb = container_of(xfb, struct XenFB, c.xendev);
> > -    fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb);
> > -    fb->have_console = 1;
> > -
> > -    /* vkbd */
> > -    in = container_of(xin, struct XenInput, c.xendev);
> > -    in->c.con = fb->c.con;
> > -
> > -    /* retry ->init() */
> > -    xen_be_check_state(xin);
> > -    xen_be_check_state(xfb);
> > -}
> > diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> > index 79aef4e..31d2f25 100644
> > --- a/hw/xenpv/xen_machine_pv.c
> > +++ b/hw/xenpv/xen_machine_pv.c
> > @@ -94,9 +94,6 @@ static void xen_init_pv(MachineState *machine)
> > 
> >      /* config cleanup hook */
> >      atexit(xen_config_cleanup);
> > -
> > -    /* setup framebuffer */
> > -    xen_init_display(xen_domid);
> >  }
> > 
> >  static void xenpv_machine_init(MachineClass *mc)
> > diff --git a/include/hw/xen/xen_backend.h
> > b/include/hw/xen/xen_backend.h
> > index 852c2ea..8a6fbcb 100644
> > --- a/include/hw/xen/xen_backend.h
> > +++ b/include/hw/xen/xen_backend.h
> > @@ -55,8 +55,6 @@ extern struct XenDevOps xen_netdev_ops;       /*
> > xen_nic.c         */
> >  extern struct XenDevOps xen_usb_ops;          /* xen-usb.c         */
> >  #endif
> > 
> > -void xen_init_display(int domid);
> > -
> >  /* configuration (aka xenbus setup) */
> >  void xen_config_cleanup(void);
> >  int xen_config_dev_blk(DriveInfo *disk);
>
diff mbox

Patch

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index e76c0d8..3b0168b 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -71,7 +71,6 @@  struct XenFB {
     int               fbpages;
     int               feature_update;
     int               bug_trigger;
-    int               have_console;
     int               do_resize;
 
     struct {
@@ -80,6 +79,7 @@  struct XenFB {
     int               up_count;
     int               up_fullscreen;
 };
+static const GraphicHwOps xenfb_ops;
 
 /* -------------------------------------------------------------------- */
 
@@ -855,6 +855,8 @@  static int fb_init(struct XenDevice *xendev)
 static int fb_initialise(struct XenDevice *xendev)
 {
     struct XenFB *fb = container_of(xendev, struct XenFB, c.xendev);
+    struct XenDevice *xin;
+    struct XenInput *in;
     struct xenfb_page *fb_page;
     int videoram;
     int rc;
@@ -877,16 +879,16 @@  static int fb_initialise(struct XenDevice *xendev)
     if (rc != 0)
 	return rc;
 
-#if 0  /* handled in xen_init_display() for now */
-    if (!fb->have_console) {
-        fb->c.ds = graphic_console_init(xenfb_update,
-                                        xenfb_invalidate,
-                                        NULL,
-                                        NULL,
-                                        fb);
-        fb->have_console = 1;
+    fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb);
+
+    xin = xen_pv_find_xendev("vkbd", xen_domid, 0);
+    if (xin == NULL) {
+        xen_pv_printf(xendev, 1, "xenfb is up, but vkbd is not present\n");
+    } else {
+        in = container_of(xin, struct XenInput, c.xendev);
+        in->c.con = fb->c.con;
+        xen_be_check_state(xin);
     }
-#endif
 
     if (xenstore_read_fe_int(xendev, "feature-update", &fb->feature_update) == -1)
 	fb->feature_update = 0;
@@ -972,42 +974,3 @@  static const GraphicHwOps xenfb_ops = {
     .gfx_update  = xenfb_update,
     .update_interval = xenfb_update_interval,
 };
-
-/*
- * FIXME/TODO: Kill this.
- * Temporary needed while DisplayState reorganization is in flight.
- */
-void xen_init_display(int domid)
-{
-    struct XenDevice *xfb, *xin;
-    struct XenFB *fb;
-    struct XenInput *in;
-    int i = 0;
-
-wait_more:
-    i++;
-    main_loop_wait(true);
-    xfb = xen_pv_find_xendev("vfb", domid, 0);
-    xin = xen_pv_find_xendev("vkbd", domid, 0);
-    if (!xfb || !xin) {
-        if (i < 256) {
-            usleep(10000);
-            goto wait_more;
-        }
-        xen_pv_printf(NULL, 1, "displaystate setup failed\n");
-        return;
-    }
-
-    /* vfb */
-    fb = container_of(xfb, struct XenFB, c.xendev);
-    fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb);
-    fb->have_console = 1;
-
-    /* vkbd */
-    in = container_of(xin, struct XenInput, c.xendev);
-    in->c.con = fb->c.con;
-
-    /* retry ->init() */
-    xen_be_check_state(xin);
-    xen_be_check_state(xfb);
-}
diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 79aef4e..31d2f25 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -94,9 +94,6 @@  static void xen_init_pv(MachineState *machine)
 
     /* config cleanup hook */
     atexit(xen_config_cleanup);
-
-    /* setup framebuffer */
-    xen_init_display(xen_domid);
 }
 
 static void xenpv_machine_init(MachineClass *mc)
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 852c2ea..8a6fbcb 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -55,8 +55,6 @@  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
 extern struct XenDevOps xen_usb_ops;          /* xen-usb.c         */
 #endif
 
-void xen_init_display(int domid);
-
 /* configuration (aka xenbus setup) */
 void xen_config_cleanup(void);
 int xen_config_dev_blk(DriveInfo *disk);