Re: [Xen-devel] Getting rid of xen_init_display() (and its dubious call into main_loop_wait())

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> Sent: 28 June 2017 19:20
> To: Peter Maydell <peter.maydell@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; QEMU Developers <qemu-
> devel@xxxxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; Gerd
> Hoffmann <kraxel@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > >
> > > 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel