qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] spice: delay starting until display are initialized
@ 2021-01-28 11:13 marcandre.lureau
  2021-01-28 11:43 ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: marcandre.lureau @ 2021-01-28 11:13 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, kraxel, Marc-André Lureau

From: Marc-André Lureau <marcandre.lureau@redhat.com>

QEMU used to run qemu_spice.display_init() before vm_start(), and
QXL/display interfaces where started then. Now, vm_start() happens
before QXL/display interfaces are added and Spice server doesn't
automatically start them in this case (fixed in spice git)

Fixes Spice regression introduced after 5.2, with refactoring commits
b4e1a34211 ("vl: remove separate preconfig main_loop") and
facf7c60ee ("vl: initialize displays _after_ exiting preconfiguration"),
probably others.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/runstate.h | 1 +
 include/ui/qemu-spice.h   | 1 +
 softmmu/runstate.c        | 5 +++++
 ui/spice-core.c           | 9 ++++++++-
 ui/spice-display.c        | 2 ++
 5 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index e557f470d4..40b3083008 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -6,6 +6,7 @@
 
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
+RunState runstate_get(void);
 int runstate_is_running(void);
 bool runstate_needs_reset(void);
 bool runstate_store(char *str, size_t size);
diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 2beb792972..71ecd6cfd1 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -28,6 +28,7 @@
 
 void qemu_spice_input_init(void);
 void qemu_spice_display_init(void);
+void qemu_spice_display_init_done(void);
 bool qemu_spice_have_display_interface(QemuConsole *con);
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
 int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index beee050815..92ef7444d0 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -195,6 +195,11 @@ static void runstate_init(void)
     qemu_mutex_init(&vmstop_lock);
 }
 
+RunState runstate_get(void)
+{
+    return current_run_state;
+}
+
 /* This function will abort() on invalid state transitions */
 void runstate_set(RunState new_state)
 {
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 5746d0aae7..b621dd86b6 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -46,6 +46,7 @@ static const char *auth = "spice";
 static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
 static int spice_migration_completed;
+static int spice_display_init_done;
 static int spice_display_is_running;
 static int spice_have_target_host;
 
@@ -625,13 +626,19 @@ static int add_channel(void *opaque, const char *name, const char *value,
 static void vm_change_state_handler(void *opaque, int running,
                                     RunState state)
 {
-    if (running) {
+    if (running && spice_display_init_done) {
         qemu_spice_display_start();
     } else if (state != RUN_STATE_PAUSED) {
         qemu_spice_display_stop();
     }
 }
 
+void qemu_spice_display_init_done(void)
+{
+    spice_display_init_done = true;
+    vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
+}
+
 static void qemu_spice_init(void)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 0178d5766d..3d3e3bcb22 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
         }
         qemu_spice_display_init_one(con);
     }
+
+    qemu_spice_display_init_done();
 }
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 11:13 [PATCH] spice: delay starting until display are initialized marcandre.lureau
@ 2021-01-28 11:43 ` Gerd Hoffmann
  2021-01-28 11:57   ` Marc-André Lureau
  0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2021-01-28 11:43 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: pbonzini, qemu-devel

  Hi,

> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 0178d5766d..3d3e3bcb22 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
>          }
>          qemu_spice_display_init_one(con);
>      }

       if (runstate_is_running()) {
            qemu_spice_display_start();
       }

Isn't that enough?

take care,
  Gerd



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 11:43 ` Gerd Hoffmann
@ 2021-01-28 11:57   ` Marc-André Lureau
  2021-01-28 12:00     ` Marc-André Lureau
  0 siblings, 1 reply; 11+ messages in thread
From: Marc-André Lureau @ 2021-01-28 11:57 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Bonzini, Paolo, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

Hi

On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 0178d5766d..3d3e3bcb22 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
> >          }
> >          qemu_spice_display_init_one(con);
> >      }
>
>        if (runstate_is_running()) {
>             qemu_spice_display_start();
>        }
>
> Isn't that enough?
>

That should be fine too, I'll update the patch. thanks

[-- Attachment #2: Type: text/html, Size: 988 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 11:57   ` Marc-André Lureau
@ 2021-01-28 12:00     ` Marc-André Lureau
  2021-01-28 14:26       ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: Marc-André Lureau @ 2021-01-28 12:00 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Bonzini, Paolo, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

Hi

On Thu, Jan 28, 2021 at 3:57 PM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi
>
> On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>>   Hi,
>>
>> > diff --git a/ui/spice-display.c b/ui/spice-display.c
>> > index 0178d5766d..3d3e3bcb22 100644
>> > --- a/ui/spice-display.c
>> > +++ b/ui/spice-display.c
>> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
>> >          }
>> >          qemu_spice_display_init_one(con);
>> >      }
>>
>>        if (runstate_is_running()) {
>>             qemu_spice_display_start();
>>        }
>>
>> Isn't that enough?
>>
>
> That should be fine too, I'll update the patch. thanks
>

Actually no, we still need to prevent the initial
qemu_spice_display_start(), and do a single call when everything is ready
(since spice server doesn't handle adding interfaces dynamically when
running).

[-- Attachment #2: Type: text/html, Size: 1641 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 12:00     ` Marc-André Lureau
@ 2021-01-28 14:26       ` Gerd Hoffmann
  2021-01-28 14:35         ` Marc-André Lureau
  0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2021-01-28 14:26 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Bonzini, Paolo, qemu-devel

On Thu, Jan 28, 2021 at 04:00:20PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jan 28, 2021 at 3:57 PM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
> > Hi
> >
> > On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >>   Hi,
> >>
> >> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> >> > index 0178d5766d..3d3e3bcb22 100644
> >> > --- a/ui/spice-display.c
> >> > +++ b/ui/spice-display.c
> >> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
> >> >          }
> >> >          qemu_spice_display_init_one(con);
> >> >      }
> >>
> >>        if (runstate_is_running()) {
> >>             qemu_spice_display_start();
> >>        }
> >>
> >> Isn't that enough?
> >>
> >
> > That should be fine too, I'll update the patch. thanks
> >
> 
> Actually no, we still need to prevent the initial
> qemu_spice_display_start(), and do a single call when everything is ready
> (since spice server doesn't handle adding interfaces dynamically when
> running).

I still think that moving these three lines to the correct place is
enough.  Maybe even just qemu_spice_display_start() as it keeps track
of the state and you can safely call this twice.

take care,
  Gerd



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 14:26       ` Gerd Hoffmann
@ 2021-01-28 14:35         ` Marc-André Lureau
  2021-01-28 14:42           ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: Marc-André Lureau @ 2021-01-28 14:35 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Bonzini, Paolo, qemu-devel

Hi

On Thu, Jan 28, 2021 at 6:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Jan 28, 2021 at 04:00:20PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Jan 28, 2021 at 3:57 PM Marc-André Lureau <
> > marcandre.lureau@redhat.com> wrote:
> >
> > > Hi
> > >
> > > On Thu, Jan 28, 2021 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >>   Hi,
> > >>
> > >> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > >> > index 0178d5766d..3d3e3bcb22 100644
> > >> > --- a/ui/spice-display.c
> > >> > +++ b/ui/spice-display.c
> > >> > @@ -1188,4 +1188,6 @@ void qemu_spice_display_init(void)
> > >> >          }
> > >> >          qemu_spice_display_init_one(con);
> > >> >      }
> > >>
> > >>        if (runstate_is_running()) {
> > >>             qemu_spice_display_start();
> > >>        }
> > >>
> > >> Isn't that enough?
> > >>
> > >
> > > That should be fine too, I'll update the patch. thanks
> > >
> >
> > Actually no, we still need to prevent the initial
> > qemu_spice_display_start(), and do a single call when everything is ready
> > (since spice server doesn't handle adding interfaces dynamically when
> > running).
>
> I still think that moving these three lines to the correct place is
> enough.  Maybe even just qemu_spice_display_start() as it keeps track
> of the state and you can safely call this twice.
>

It's not enough, since the first time qemu_spice_display_start() is
called (on vm_start) the display interfaces aren't yet registered. And
spice server doesn't automatically start the newly added interfaces.



-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 14:35         ` Marc-André Lureau
@ 2021-01-28 14:42           ` Gerd Hoffmann
  2021-01-28 15:05             ` Marc-André Lureau
  0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2021-01-28 14:42 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Bonzini, Paolo, qemu-devel

> > I still think that moving these three lines to the correct place is
> > enough.  Maybe even just qemu_spice_display_start() as it keeps track
> > of the state and you can safely call this twice.
> 
> It's not enough, since the first time qemu_spice_display_start() is
> called (on vm_start) the display interfaces aren't yet registered. And
> spice server doesn't automatically start the newly added interfaces.

So move the vmstate handler registration call too?
I'd prefer to not add more state variables if we can avoid it ...

take care,
  Gerd



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 14:42           ` Gerd Hoffmann
@ 2021-01-28 15:05             ` Marc-André Lureau
  2021-01-28 16:34               ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: Marc-André Lureau @ 2021-01-28 15:05 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Bonzini, Paolo, qemu-devel

Hi

On Thu, Jan 28, 2021 at 6:42 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > I still think that moving these three lines to the correct place is
> > > enough.  Maybe even just qemu_spice_display_start() as it keeps track
> > > of the state and you can safely call this twice.
> >
> > It's not enough, since the first time qemu_spice_display_start() is
> > called (on vm_start) the display interfaces aren't yet registered. And
> > spice server doesn't automatically start the newly added interfaces.
>
> So move the vmstate handler registration call too?
> I'd prefer to not add more state variables if we can avoid it ...
>

Does that seem right to you?

diff --git a/ui/spice-core.c b/ui/spice-core.c
index b621dd86b6..f592331ce4 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -46,7 +46,6 @@ static const char *auth = "spice";
 static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
 static int spice_migration_completed;
-static int spice_display_init_done;
 static int spice_display_is_running;
 static int spice_have_target_host;

@@ -626,7 +625,7 @@ static int add_channel(void *opaque, const char
*name, const char *value,
 static void vm_change_state_handler(void *opaque, int running,
                                     RunState state)
 {
-    if (running && spice_display_init_done) {
+    if (running) {
         qemu_spice_display_start();
     } else if (state != RUN_STATE_PAUSED) {
         qemu_spice_display_stop();
@@ -635,7 +634,7 @@ static void vm_change_state_handler(void *opaque,
int running,

 void qemu_spice_display_init_done(void)
 {
-    spice_display_init_done = true;
+    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
     vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
 }

@@ -810,7 +809,6 @@ static void qemu_spice_init(void)

     qemu_spice_input_init();

-    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
     qemu_spice_display_stop();

     g_free(x509_key_file);


-- 
Marc-André Lureau


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 15:05             ` Marc-André Lureau
@ 2021-01-28 16:34               ` Gerd Hoffmann
  2021-01-28 19:28                 ` Marc-André Lureau
  0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2021-01-28 16:34 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Bonzini, Paolo, qemu-devel

  Hi,

> > So move the vmstate handler registration call too?
> > I'd prefer to not add more state variables if we can avoid it ...
> 
> Does that seem right to you?

> @@ -626,7 +625,7 @@ static int add_channel(void *opaque, const char
> *name, const char *value,
>  static void vm_change_state_handler(void *opaque, int running,
>                                      RunState state)
>  {
> -    if (running && spice_display_init_done) {
> +    if (running) {
>          qemu_spice_display_start();
>      } else if (state != RUN_STATE_PAUSED) {
>          qemu_spice_display_stop();
> @@ -635,7 +634,7 @@ static void vm_change_state_handler(void *opaque,
> int running,
> 
>  void qemu_spice_display_init_done(void)
>  {
> -    spice_display_init_done = true;
> +    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
>      vm_change_state_handler(NULL, runstate_is_running(), runstate_get());

I'd just call qemu_spice_display_start() directly here, the need for
runstate_get() goes away then.  Otherwise looks good to me.

take care,
  Gerd



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 16:34               ` Gerd Hoffmann
@ 2021-01-28 19:28                 ` Marc-André Lureau
  2021-01-29 14:22                   ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: Marc-André Lureau @ 2021-01-28 19:28 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Bonzini, Paolo, qemu-devel

On Thu, Jan 28, 2021 at 8:34 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > So move the vmstate handler registration call too?
> > > I'd prefer to not add more state variables if we can avoid it ...
> >
> > Does that seem right to you?
>
> > @@ -626,7 +625,7 @@ static int add_channel(void *opaque, const char
> > *name, const char *value,
> >  static void vm_change_state_handler(void *opaque, int running,
> >                                      RunState state)
> >  {
> > -    if (running && spice_display_init_done) {
> > +    if (running) {
> >          qemu_spice_display_start();
> >      } else if (state != RUN_STATE_PAUSED) {
> >          qemu_spice_display_stop();
> > @@ -635,7 +634,7 @@ static void vm_change_state_handler(void *opaque,
> > int running,
> >
> >  void qemu_spice_display_init_done(void)
> >  {
> > -    spice_display_init_done = true;
> > +    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
> >      vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
>
> I'd just call qemu_spice_display_start() directly here, the need for
> runstate_get() goes away then.  Otherwise looks good to me.

Hmm, that could work, but it will behave differently as we will start
spice even if the VM is not running then.


-- 
Marc-André Lureau


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] spice: delay starting until display are initialized
  2021-01-28 19:28                 ` Marc-André Lureau
@ 2021-01-29 14:22                   ` Gerd Hoffmann
  0 siblings, 0 replies; 11+ messages in thread
From: Gerd Hoffmann @ 2021-01-29 14:22 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: Bonzini, Paolo, qemu-devel

> > >  void qemu_spice_display_init_done(void)
> > >  {
> > > -    spice_display_init_done = true;
> > > +    qemu_add_vm_change_state_handler(vm_change_state_handler, NULL);
> > >      vm_change_state_handler(NULL, runstate_is_running(), runstate_get());
> >
> > I'd just call qemu_spice_display_start() directly here, the need for
> > runstate_get() goes away then.  Otherwise looks good to me.
> 
> Hmm, that could work, but it will behave differently as we will start
> spice even if the VM is not running then.

if (runstate_is_running())
    qemu_spice_display_start()

take care,
  Gerd



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-01-29 14:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 11:13 [PATCH] spice: delay starting until display are initialized marcandre.lureau
2021-01-28 11:43 ` Gerd Hoffmann
2021-01-28 11:57   ` Marc-André Lureau
2021-01-28 12:00     ` Marc-André Lureau
2021-01-28 14:26       ` Gerd Hoffmann
2021-01-28 14:35         ` Marc-André Lureau
2021-01-28 14:42           ` Gerd Hoffmann
2021-01-28 15:05             ` Marc-André Lureau
2021-01-28 16:34               ` Gerd Hoffmann
2021-01-28 19:28                 ` Marc-André Lureau
2021-01-29 14:22                   ` Gerd Hoffmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).