| Message ID | alpine.DEB.2.10.1706271438090.24648@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28 June 2017 at 01:06, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Tue, 27 Jun 2017, Peter Maydell wrote: >> So, there is exactly one caller of main_loop_wait() in the tree that >> passes it 'true' as an argument. That caller is in xen_init_display() >> in hw/dispaly/xenfb.c. The function was added in 2009 with the comment >> "FIXME/TODO: Kill this. Temporary needed while DisplayState >> reorganization is in flight." >> >> I'd like to think that we've now completed whatever reorg that was, >> 8 years on, so can we now get rid of this function? It definitely >> seems very dubious to have a display init function with a busy loop >> and a call into main_loop_wait()... > > LOL, you gotta love "temporary fixes". I am happy to see it wasn't me > that added it ;-) > > I think the following should do the trick. Thanks! > --- > > 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> > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > index e76c0d8..873e51f 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,12 @@ 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; > - } > -#endif > + fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb); > + > + xin = xen_pv_find_xendev("vkbd", xen_domid, 0); > + in = container_of(xin, struct XenInput, c.xendev); > + in->c.con = fb->c.con; Won't this crash if xen_pv_find_xendev() returned NULL? Or are we guaranteed that that can't happen here? thanks -- PMM
On Wed, 28 Jun 2017, Peter Maydell wrote: > On 28 June 2017 at 01:06, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Tue, 27 Jun 2017, Peter Maydell wrote: > >> So, there is exactly one caller of main_loop_wait() in the tree that > >> passes it 'true' as an argument. That caller is in xen_init_display() > >> in hw/dispaly/xenfb.c. The function was added in 2009 with the comment > >> "FIXME/TODO: Kill this. Temporary needed while DisplayState > >> reorganization is in flight." > >> > >> I'd like to think that we've now completed whatever reorg that was, > >> 8 years on, so can we now get rid of this function? It definitely > >> seems very dubious to have a display init function with a busy loop > >> and a call into main_loop_wait()... > > > > LOL, you gotta love "temporary fixes". I am happy to see it wasn't me > > that added it ;-) > > > > I think the following should do the trick. > > Thanks! > > > --- > > > > 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> > > > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > > index e76c0d8..873e51f 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,12 @@ 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; > > - } > > -#endif > > + fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb); > > + > > + xin = xen_pv_find_xendev("vkbd", xen_domid, 0); > > + in = container_of(xin, struct XenInput, c.xendev); > > + in->c.con = fb->c.con; > > Won't this crash if xen_pv_find_xendev() returned NULL? > Or are we guaranteed that that can't happen here? As long as there is a vkdb device, it will be already added to the xendevs list at this point. However, if there isn't a device at all, then it would crash. In that case, I think we should print a warning and continue without it. I'll send an updated patch.
> -----Original Message----- > From: Stefano Stabellini [mailto:sstabellini@kernel.org] > Sent: 28 June 2017 19:20 > To: Peter Maydell <peter.maydell@linaro.org> > Cc: Stefano Stabellini <sstabellini@kernel.org>; QEMU Developers <qemu- > devel@nongnu.org>; Anthony Perard <anthony.perard@citrix.com>; Gerd > Hoffmann <kraxel@redhat.com>; Paul Durrant <Paul.Durrant@citrix.com>; > xen-devel@lists.xenproject.org > Subject: Re: Getting rid of xen_init_display() (and its dubious call into > main_loop_wait()) > > On Wed, 28 Jun 2017, Peter Maydell wrote: > > On 28 June 2017 at 01:06, Stefano Stabellini <sstabellini@kernel.org> > wrote: > > > On Tue, 27 Jun 2017, Peter Maydell wrote: > > >> So, there is exactly one caller of main_loop_wait() in the tree that > > >> passes it 'true' as an argument. That caller is in xen_init_display() > > >> in hw/dispaly/xenfb.c. The function was added in 2009 with the > comment > > >> "FIXME/TODO: Kill this. Temporary needed while DisplayState > > >> reorganization is in flight." > > >> > > >> I'd like to think that we've now completed whatever reorg that was, > > >> 8 years on, so can we now get rid of this function? It definitely > > >> seems very dubious to have a display init function with a busy loop > > >> and a call into main_loop_wait()... > > > > > > LOL, you gotta love "temporary fixes". I am happy to see it wasn't me > > > that added it ;-) > > > > > > I think the following should do the trick. > > > > Thanks! > > > > > --- > > > > > > 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> > > > > > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > > > index e76c0d8..873e51f 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,12 @@ 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; > > > - } > > > -#endif > > > + fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb); > > > + > > > + xin = xen_pv_find_xendev("vkbd", xen_domid, 0); > > > + in = container_of(xin, struct XenInput, c.xendev); > > > + in->c.con = fb->c.con; > > > > Won't this crash if xen_pv_find_xendev() returned NULL? > > Or are we guaranteed that that can't happen here? > > As long as there is a vkdb device, it will be already added to the > xendevs list at this point. However, if there isn't a device at all, > then it would crash. In that case, I think we should print a warning and > continue without it. > > I'll send an updated patch. There is still the fact the vkbd can't initialise until vfb has done so. This interdependency is subtle and IMO bogus. It needs to be cleared up. Paul
diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index e76c0d8..873e51f 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,12 @@ 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; - } -#endif + fb->c.con = graphic_console_init(NULL, 0, &xenfb_ops, fb); + + xin = xen_pv_find_xendev("vkbd", xen_domid, 0); + in = container_of(xin, struct XenInput, c.xendev); + in->c.con = fb->c.con; + xen_be_check_state(xin); if (xenstore_read_fe_int(xendev, "feature-update", &fb->feature_update) == -1) fb->feature_update = 0; @@ -972,42 +970,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)