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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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