[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Getting rid of xen_init_display() (and its dubious call
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] Getting rid of xen_init_display() (and its dubious call into main_loop_wait()) |
Date: |
Wed, 28 Jun 2017 11:19:37 -0700 (PDT) |
User-agent: |
Alpine 2.10 (DEB 1266 2009-07-14) |
On Wed, 28 Jun 2017, Peter Maydell wrote:
> On 28 June 2017 at 01:06, Stefano Stabellini <address@hidden> 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 <address@hidden>
> >
> > 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.