All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] qxl: remove assert and migration blocker
@ 2021-07-21  9:33 Gerd Hoffmann
  2021-07-21  9:33 ` [PATCH 1/2] qxl: remove assert in qxl_pre_save Gerd Hoffmann
  2021-07-21  9:33 ` [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert" Gerd Hoffmann
  0 siblings, 2 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2021-07-21  9:33 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Gerd Hoffmann

\r
\r
Gerd Hoffmann (2):\r
  qxl: remove assert in qxl_pre_save.\r
  Revert "qxl: add migration blocker to avoid pre-save assert"\r
\r
 hw/display/qxl.h |  1 -\r
 hw/display/qxl.c | 35 +++--------------------------------\r
 2 files changed, 3 insertions(+), 33 deletions(-)\r
\r
-- \r
2.31.1\r
\r



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

* [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-21  9:33 [PATCH 0/2] qxl: remove assert and migration blocker Gerd Hoffmann
@ 2021-07-21  9:33 ` Gerd Hoffmann
  2021-07-21 10:43   ` Marc-André Lureau
  2021-07-22 14:44   ` Dr. David Alan Gilbert
  2021-07-21  9:33 ` [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert" Gerd Hoffmann
  1 sibling, 2 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2021-07-21  9:33 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Gerd Hoffmann

Since commit 551dbd0846d2 ("migration: check pre_save return in
vmstate_save_state") the pre_save hook can fail.  So lets finally
use that to drop the guest-triggerable assert in qxl_pre_save().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 84f99088e0a0..3867b94fe236 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
     } else {
         d->last_release_offset = (uint8_t *)d->last_release - ram_start;
     }
-    assert(d->last_release_offset < d->vga.vram_size);
+    if (d->last_release_offset < d->vga.vram_size) {
+        return 1;
+    }
 
     return 0;
 }
-- 
2.31.1



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

* [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert"
  2021-07-21  9:33 [PATCH 0/2] qxl: remove assert and migration blocker Gerd Hoffmann
  2021-07-21  9:33 ` [PATCH 1/2] qxl: remove assert in qxl_pre_save Gerd Hoffmann
@ 2021-07-21  9:33 ` Gerd Hoffmann
  2021-07-21 10:44   ` Marc-André Lureau
  2021-07-21 14:15   ` Markus Armbruster
  1 sibling, 2 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2021-07-21  9:33 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Gerd Hoffmann

This reverts commit 86dbcdd9c7590d06db89ca256c5eaf0b4aba8858.

The pre-save assert is gone now, so the migration blocker
is not needed any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.h |  1 -
 hw/display/qxl.c | 31 -------------------------------
 2 files changed, 32 deletions(-)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index 379d3304abc1..30d21f4d0bdc 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -39,7 +39,6 @@ struct PCIQXLDevice {
     uint32_t           cmdlog;
 
     uint32_t           guest_bug;
-    Error              *migration_blocker;
 
     enum qxl_mode      mode;
     uint32_t           cmdflags;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 3867b94fe236..43482d4364ba 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -30,7 +30,6 @@
 #include "qemu/module.h"
 #include "hw/qdev-properties.h"
 #include "sysemu/runstate.h"
-#include "migration/blocker.h"
 #include "migration/vmstate.h"
 #include "trace.h"
 
@@ -666,30 +665,6 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "cmd", ext);
-        {
-            /*
-             * Windows 8 drivers place qxl commands in the vram
-             * (instead of the ram) bar.  We can't live migrate such a
-             * guest, so add a migration blocker in case we detect
-             * this, to avoid triggering the assert in pre_save().
-             *
-             * https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
-             */
-            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
-            if (msg != NULL && (
-                    msg < (void *)qxl->vga.vram_ptr ||
-                    msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
-                if (!qxl->migration_blocker) {
-                    Error *local_err = NULL;
-                    error_setg(&qxl->migration_blocker,
-                               "qxl: guest bug: command not in ram bar");
-                    migrate_add_blocker(qxl->migration_blocker, &local_err);
-                    if (local_err) {
-                        error_report_err(local_err);
-                    }
-                }
-            }
-        }
         trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
         return true;
     default:
@@ -1283,12 +1258,6 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
     qemu_spice_create_host_memslot(&d->ssd);
     qxl_soft_reset(d);
 
-    if (d->migration_blocker) {
-        migrate_del_blocker(d->migration_blocker);
-        error_free(d->migration_blocker);
-        d->migration_blocker = NULL;
-    }
-
     if (startstop) {
         qemu_spice_display_start();
     }
-- 
2.31.1



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-21  9:33 ` [PATCH 1/2] qxl: remove assert in qxl_pre_save Gerd Hoffmann
@ 2021-07-21 10:43   ` Marc-André Lureau
  2021-07-22 14:44   ` Dr. David Alan Gilbert
  1 sibling, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-07-21 10:43 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: QEMU, Markus Armbruster

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

On Wed, Jul 21, 2021 at 1:41 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Since commit 551dbd0846d2 ("migration: check pre_save return in
> vmstate_save_state") the pre_save hook can fail.  So lets finally
> use that to drop the guest-triggerable assert in qxl_pre_save().
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

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

---
>  hw/display/qxl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 84f99088e0a0..3867b94fe236 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>      } else {
>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>      }
> -    assert(d->last_release_offset < d->vga.vram_size);
> +    if (d->last_release_offset < d->vga.vram_size) {
> +        return 1;
> +    }
>
>      return 0;
>  }
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

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

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

* Re: [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert"
  2021-07-21  9:33 ` [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert" Gerd Hoffmann
@ 2021-07-21 10:44   ` Marc-André Lureau
  2021-07-21 14:15   ` Markus Armbruster
  1 sibling, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2021-07-21 10:44 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: QEMU, Markus Armbruster

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

On Wed, Jul 21, 2021 at 1:34 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> This reverts commit 86dbcdd9c7590d06db89ca256c5eaf0b4aba8858.
>
> The pre-save assert is gone now, so the migration blocker
> is not needed any more.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

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


> ---
>  hw/display/qxl.h |  1 -
>  hw/display/qxl.c | 31 -------------------------------
>  2 files changed, 32 deletions(-)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index 379d3304abc1..30d21f4d0bdc 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -39,7 +39,6 @@ struct PCIQXLDevice {
>      uint32_t           cmdlog;
>
>      uint32_t           guest_bug;
> -    Error              *migration_blocker;
>
>      enum qxl_mode      mode;
>      uint32_t           cmdflags;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 3867b94fe236..43482d4364ba 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -30,7 +30,6 @@
>  #include "qemu/module.h"
>  #include "hw/qdev-properties.h"
>  #include "sysemu/runstate.h"
> -#include "migration/blocker.h"
>  #include "migration/vmstate.h"
>  #include "trace.h"
>
> @@ -666,30 +665,6 @@ static int interface_get_command(QXLInstance *sin,
> struct QXLCommandExt *ext)
>          qxl->guest_primary.commands++;
>          qxl_track_command(qxl, ext);
>          qxl_log_command(qxl, "cmd", ext);
> -        {
> -            /*
> -             * Windows 8 drivers place qxl commands in the vram
> -             * (instead of the ram) bar.  We can't live migrate such a
> -             * guest, so add a migration blocker in case we detect
> -             * this, to avoid triggering the assert in pre_save().
> -             *
> -             *
> https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
> -             */
> -            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> -            if (msg != NULL && (
> -                    msg < (void *)qxl->vga.vram_ptr ||
> -                    msg > ((void *)qxl->vga.vram_ptr +
> qxl->vga.vram_size))) {
> -                if (!qxl->migration_blocker) {
> -                    Error *local_err = NULL;
> -                    error_setg(&qxl->migration_blocker,
> -                               "qxl: guest bug: command not in ram bar");
> -                    migrate_add_blocker(qxl->migration_blocker,
> &local_err);
> -                    if (local_err) {
> -                        error_report_err(local_err);
> -                    }
> -                }
> -            }
> -        }
>          trace_qxl_ring_command_get(qxl->id,
> qxl_mode_to_string(qxl->mode));
>          return true;
>      default:
> @@ -1283,12 +1258,6 @@ static void qxl_hard_reset(PCIQXLDevice *d, int
> loadvm)
>      qemu_spice_create_host_memslot(&d->ssd);
>      qxl_soft_reset(d);
>
> -    if (d->migration_blocker) {
> -        migrate_del_blocker(d->migration_blocker);
> -        error_free(d->migration_blocker);
> -        d->migration_blocker = NULL;
> -    }
> -
>      if (startstop) {
>          qemu_spice_display_start();
>      }
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

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

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

* Re: [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert"
  2021-07-21  9:33 ` [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert" Gerd Hoffmann
  2021-07-21 10:44   ` Marc-André Lureau
@ 2021-07-21 14:15   ` Markus Armbruster
  1 sibling, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2021-07-21 14:15 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel, armbru

Gerd Hoffmann <kraxel@redhat.com> writes:

> This reverts commit 86dbcdd9c7590d06db89ca256c5eaf0b4aba8858.
>
> The pre-save assert is gone now, so the migration blocker
> is not needed any more.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl.h |  1 -
>  hw/display/qxl.c | 31 -------------------------------
>  2 files changed, 32 deletions(-)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index 379d3304abc1..30d21f4d0bdc 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -39,7 +39,6 @@ struct PCIQXLDevice {
>      uint32_t           cmdlog;
>  
>      uint32_t           guest_bug;
> -    Error              *migration_blocker;
>  
>      enum qxl_mode      mode;
>      uint32_t           cmdflags;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 3867b94fe236..43482d4364ba 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -30,7 +30,6 @@
>  #include "qemu/module.h"
>  #include "hw/qdev-properties.h"
>  #include "sysemu/runstate.h"
> -#include "migration/blocker.h"
>  #include "migration/vmstate.h"
>  #include "trace.h"
>  
> @@ -666,30 +665,6 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
>          qxl->guest_primary.commands++;
>          qxl_track_command(qxl, ext);
>          qxl_log_command(qxl, "cmd", ext);
> -        {
> -            /*
> -             * Windows 8 drivers place qxl commands in the vram
> -             * (instead of the ram) bar.  We can't live migrate such a
> -             * guest, so add a migration blocker in case we detect
> -             * this, to avoid triggering the assert in pre_save().
> -             *
> -             * https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
> -             */
> -            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> -            if (msg != NULL && (
> -                    msg < (void *)qxl->vga.vram_ptr ||
> -                    msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
> -                if (!qxl->migration_blocker) {
> -                    Error *local_err = NULL;
> -                    error_setg(&qxl->migration_blocker,
> -                               "qxl: guest bug: command not in ram bar");
> -                    migrate_add_blocker(qxl->migration_blocker, &local_err);
> -                    if (local_err) {
> -                        error_report_err(local_err);
> -                    }
> -                }
> -            }
> -        }
>          trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
>          return true;
>      default:

Conflicts with my "[PATCH 09/16] migration: Unify failure check for
migrate_add_blocker()".  Easy to resolve: your patch goes first, I drop
the hunk from mine.

> @@ -1283,12 +1258,6 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
>      qemu_spice_create_host_memslot(&d->ssd);
>      qxl_soft_reset(d);
>  
> -    if (d->migration_blocker) {
> -        migrate_del_blocker(d->migration_blocker);
> -        error_free(d->migration_blocker);
> -        d->migration_blocker = NULL;
> -    }
> -
>      if (startstop) {
>          qemu_spice_display_start();
>      }



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-21  9:33 ` [PATCH 1/2] qxl: remove assert in qxl_pre_save Gerd Hoffmann
  2021-07-21 10:43   ` Marc-André Lureau
@ 2021-07-22 14:44   ` Dr. David Alan Gilbert
  2021-07-23  6:32     ` Gerd Hoffmann
  1 sibling, 1 reply; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-22 14:44 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel, armbru

* Gerd Hoffmann (kraxel@redhat.com) wrote:
> Since commit 551dbd0846d2 ("migration: check pre_save return in
> vmstate_save_state") the pre_save hook can fail.  So lets finally
> use that to drop the guest-triggerable assert in qxl_pre_save().
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 84f99088e0a0..3867b94fe236 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>      } else {
>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>      }
> -    assert(d->last_release_offset < d->vga.vram_size);
> +    if (d->last_release_offset < d->vga.vram_size) {
> +        return 1;

It would be great if there was an error_report or something there
so that we get some idea of what happened.

Dave

> +    }
>  
>      return 0;
>  }
> -- 
> 2.31.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-22 14:44   ` Dr. David Alan Gilbert
@ 2021-07-23  6:32     ` Gerd Hoffmann
  2021-07-23  6:47       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2021-07-23  6:32 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, armbru

On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
> * Gerd Hoffmann (kraxel@redhat.com) wrote:
> > Since commit 551dbd0846d2 ("migration: check pre_save return in
> > vmstate_save_state") the pre_save hook can fail.  So lets finally
> > use that to drop the guest-triggerable assert in qxl_pre_save().
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/display/qxl.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > index 84f99088e0a0..3867b94fe236 100644
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
> >      } else {
> >          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
> >      }
> > -    assert(d->last_release_offset < d->vga.vram_size);
> > +    if (d->last_release_offset < d->vga.vram_size) {
> > +        return 1;
> 
> It would be great if there was an error_report or something there
> so that we get some idea of what happened.

Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
the correct approach here, but I guess that wouldn't help much for
trouble-shooting as the message would only show up with "-d
guest_errors".  So better make that error_report("...") ?

take care,
  Gerd



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-23  6:32     ` Gerd Hoffmann
@ 2021-07-23  6:47       ` Philippe Mathieu-Daudé
  2021-07-23  6:55         ` Gerd Hoffmann
  0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-23  6:47 UTC (permalink / raw)
  To: Gerd Hoffmann, Dr. David Alan Gilbert; +Cc: qemu-devel, armbru

On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
> On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
>> * Gerd Hoffmann (kraxel@redhat.com) wrote:
>>> Since commit 551dbd0846d2 ("migration: check pre_save return in
>>> vmstate_save_state") the pre_save hook can fail.  So lets finally
>>> use that to drop the guest-triggerable assert in qxl_pre_save().
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  hw/display/qxl.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>>> index 84f99088e0a0..3867b94fe236 100644
>>> --- a/hw/display/qxl.c
>>> +++ b/hw/display/qxl.c
>>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>>>      } else {
>>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>>>      }
>>> -    assert(d->last_release_offset < d->vga.vram_size);
>>> +    if (d->last_release_offset < d->vga.vram_size) {
>>> +        return 1;
>>
>> It would be great if there was an error_report or something there
>> so that we get some idea of what happened.
> 
> Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
> the correct approach here, but I guess that wouldn't help much for
> trouble-shooting as the message would only show up with "-d
> guest_errors".  So better make that error_report("...") ?

It depends who is interested in such errors. Does management app care?



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-23  6:47       ` Philippe Mathieu-Daudé
@ 2021-07-23  6:55         ` Gerd Hoffmann
  2021-07-23  7:34           ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2021-07-23  6:55 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: armbru, Dr. David Alan Gilbert, qemu-devel

On Fri, Jul 23, 2021 at 08:47:09AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
> > On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
> >> * Gerd Hoffmann (kraxel@redhat.com) wrote:
> >>> Since commit 551dbd0846d2 ("migration: check pre_save return in
> >>> vmstate_save_state") the pre_save hook can fail.  So lets finally
> >>> use that to drop the guest-triggerable assert in qxl_pre_save().
> >>>
> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >>> ---
> >>>  hw/display/qxl.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> >>> index 84f99088e0a0..3867b94fe236 100644
> >>> --- a/hw/display/qxl.c
> >>> +++ b/hw/display/qxl.c
> >>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
> >>>      } else {
> >>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
> >>>      }
> >>> -    assert(d->last_release_offset < d->vga.vram_size);
> >>> +    if (d->last_release_offset < d->vga.vram_size) {
> >>> +        return 1;
> >>
> >> It would be great if there was an error_report or something there
> >> so that we get some idea of what happened.
> > 
> > Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
> > the correct approach here, but I guess that wouldn't help much for
> > trouble-shooting as the message would only show up with "-d
> > guest_errors".  So better make that error_report("...") ?
> 
> It depends who is interested in such errors.

Migration fails and you want know why.

Reason migration fails is the guest violates qxl specs (which doesn't
cause actual problems unless you want migrate the guest).

take care,
  Gerd



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-23  6:55         ` Gerd Hoffmann
@ 2021-07-23  7:34           ` Philippe Mathieu-Daudé
  2021-07-23  8:42             ` Markus Armbruster
  0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-23  7:34 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: armbru, Dr. David Alan Gilbert, qemu-devel

On 7/23/21 8:55 AM, Gerd Hoffmann wrote:
> On Fri, Jul 23, 2021 at 08:47:09AM +0200, Philippe Mathieu-Daudé wrote:
>> On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
>>> On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
>>>> * Gerd Hoffmann (kraxel@redhat.com) wrote:
>>>>> Since commit 551dbd0846d2 ("migration: check pre_save return in
>>>>> vmstate_save_state") the pre_save hook can fail.  So lets finally
>>>>> use that to drop the guest-triggerable assert in qxl_pre_save().
>>>>>
>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>> ---
>>>>>  hw/display/qxl.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>>>>> index 84f99088e0a0..3867b94fe236 100644
>>>>> --- a/hw/display/qxl.c
>>>>> +++ b/hw/display/qxl.c
>>>>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>>>>>      } else {
>>>>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>>>>>      }
>>>>> -    assert(d->last_release_offset < d->vga.vram_size);
>>>>> +    if (d->last_release_offset < d->vga.vram_size) {
>>>>> +        return 1;
>>>>
>>>> It would be great if there was an error_report or something there
>>>> so that we get some idea of what happened.
>>>
>>> Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
>>> the correct approach here, but I guess that wouldn't help much for
>>> trouble-shooting as the message would only show up with "-d
>>> guest_errors".  So better make that error_report("...") ?
>>
>> It depends who is interested in such errors.
> 
> Migration fails and you want know why.
> 
> Reason migration fails is the guest violates qxl specs (which doesn't
> cause actual problems unless you want migrate the guest).

Then I agree error_report() is better ("Report an error to the current
monitor if we have one, else stderr").



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-23  7:34           ` Philippe Mathieu-Daudé
@ 2021-07-23  8:42             ` Markus Armbruster
  2021-07-23  9:46               ` Gerd Hoffmann
  0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2021-07-23  8:42 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Gerd Hoffmann, Dr. David Alan Gilbert

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/23/21 8:55 AM, Gerd Hoffmann wrote:
>> On Fri, Jul 23, 2021 at 08:47:09AM +0200, Philippe Mathieu-Daudé wrote:
>>> On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
>>>> On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
>>>>> * Gerd Hoffmann (kraxel@redhat.com) wrote:
>>>>>> Since commit 551dbd0846d2 ("migration: check pre_save return in
>>>>>> vmstate_save_state") the pre_save hook can fail.  So lets finally
>>>>>> use that to drop the guest-triggerable assert in qxl_pre_save().
>>>>>>
>>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>>> ---
>>>>>>  hw/display/qxl.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>>>>>> index 84f99088e0a0..3867b94fe236 100644
>>>>>> --- a/hw/display/qxl.c
>>>>>> +++ b/hw/display/qxl.c
>>>>>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>>>>>>      } else {
>>>>>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>>>>>>      }
>>>>>> -    assert(d->last_release_offset < d->vga.vram_size);
>>>>>> +    if (d->last_release_offset < d->vga.vram_size) {
>>>>>> +        return 1;
>>>>>
>>>>> It would be great if there was an error_report or something there
>>>>> so that we get some idea of what happened.
>>>>
>>>> Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
>>>> the correct approach here, but I guess that wouldn't help much for
>>>> trouble-shooting as the message would only show up with "-d
>>>> guest_errors".  So better make that error_report("...") ?
>>>
>>> It depends who is interested in such errors.
>> 
>> Migration fails and you want know why.
>> 
>> Reason migration fails is the guest violates qxl specs (which doesn't
>> cause actual problems unless you want migrate the guest).
>
> Then I agree error_report() is better ("Report an error to the current
> monitor if we have one, else stderr").

No good for QMP, I'm afraid.

error_report() reports to stderr in QMP monitor context.  To get an
error to the QMP client, you need to use error_setg() and propagate all
the way to the QMP core.

Back to the patch.  If the thing we report is actually an *error*,
i.e. something that makes migration fail, then the QMP failure will
contain an error message.  That error must come from further up the call
stack.  If it's good enough, we don't need error_report() here.  Else,
we should error_setg() here.

If it's not actually an error, then error_report() is wrong.
warn_report() might be right.



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

* Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
  2021-07-23  8:42             ` Markus Armbruster
@ 2021-07-23  9:46               ` Gerd Hoffmann
  0 siblings, 0 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2021-07-23  9:46 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Philippe Mathieu-Daudé, Dr. David Alan Gilbert, qemu-devel

  Hi,

> >> Migration fails and you want know why.
> >> 
> >> Reason migration fails is the guest violates qxl specs (which doesn't
> >> cause actual problems unless you want migrate the guest).
> >
> > Then I agree error_report() is better ("Report an error to the current
> > monitor if we have one, else stderr").
> 
> No good for QMP, I'm afraid.
> 
> error_report() reports to stderr in QMP monitor context.  To get an
> error to the QMP client, you need to use error_setg() and propagate all
> the way to the QMP core.
> 
> Back to the patch.  If the thing we report is actually an *error*,
> i.e. something that makes migration fail, then the QMP failure will
> contain an error message.  That error must come from further up the call
> stack.  If it's good enough, we don't need error_report() here.  Else,
> we should error_setg() here.

Caller is here (migration/vmstate.c):

int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                         void *opaque, JSONWriter *vmdesc, int version_id)
{
    [ ... ]
    if (vmsd->pre_save) {
        ret = vmsd->pre_save(opaque);
        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
        if (ret) {
            error_report("pre-save failed: %s", vmsd->name);
            return ret;
        }
    }
    [ ... ]
}

No error_setg() anywhere close, it's error_report() all over the place.
I guess that makes adding error_report() the only short-term (aka 6.1)
option.

take care,
  Gerd



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

end of thread, other threads:[~2021-07-23  9:47 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21  9:33 [PATCH 0/2] qxl: remove assert and migration blocker Gerd Hoffmann
2021-07-21  9:33 ` [PATCH 1/2] qxl: remove assert in qxl_pre_save Gerd Hoffmann
2021-07-21 10:43   ` Marc-André Lureau
2021-07-22 14:44   ` Dr. David Alan Gilbert
2021-07-23  6:32     ` Gerd Hoffmann
2021-07-23  6:47       ` Philippe Mathieu-Daudé
2021-07-23  6:55         ` Gerd Hoffmann
2021-07-23  7:34           ` Philippe Mathieu-Daudé
2021-07-23  8:42             ` Markus Armbruster
2021-07-23  9:46               ` Gerd Hoffmann
2021-07-21  9:33 ` [PATCH 2/2] Revert "qxl: add migration blocker to avoid pre-save assert" Gerd Hoffmann
2021-07-21 10:44   ` Marc-André Lureau
2021-07-21 14:15   ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.