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

[Peter Maydell <peter.maydell@xxxxxxxxxx>]

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?

thanks
-- PMM

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