[Qemu-devel] [PATCH] hw/qxl: warn on sync io usage

[Qemu-devel] [PATCH] hw/qxl: warn on sync io usage

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
Try to warn people who keep getting bitten by this. In addition maybe
we should bug out if revision >= 3 and sync io is used, and warn if revision <
3 is used in the first place?

 hw/qxl.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/qxl.c b/hw/qxl.c
index 97ae22a..bd37a83 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1453,6 +1453,18 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
     qxl_rom_set_dirty(d);
 }
 
+static void sync_io_warning(PCIQXLDevice *qxl, uint32_t io_port)
+{
+    fprintf(stderr, "qxl-%d: WARNING: sync io used, see (RHBZ 747011)",
+            qxl->id);
+    fprintf(stderr, "qxl-%d: WARNING: virt-viewer/remote-viewer can hang\n",
+            qxl->id);
+    if (qxl->revision < 3) {
+        fprintf(stderr, "qxl-%d: WARNING: revision >= 3 should be used\n",
+                qxl->id);
+    }
+}
+
 static void ioport_write(void *opaque, target_phys_addr_t addr,
                          uint64_t val, unsigned size)
 {
@@ -1460,6 +1472,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     uint32_t io_port = addr;
     qxl_async_io async = QXL_SYNC;
     uint32_t orig_io_port = io_port;
+    static int warned_sync_io;
 
     if (d->guest_bug && io_port != QXL_IO_RESET) {
         return;
@@ -1497,6 +1510,20 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
         return;
     }
 
+    if (!warned_sync_io) {
+        switch (io_port) {
+        case QXL_IO_UPDATE_AREA:
+        case QXL_IO_MEMSLOT_ADD:
+        case QXL_IO_CREATE_PRIMARY:
+        case QXL_IO_DESTROY_PRIMARY:
+        case QXL_IO_DESTROY_SURFACE_WAIT:
+        case QXL_IO_DESTROY_ALL_SURFACES:
+            sync_io_warning(d, io_port);
+            warned_sync_io = 1;
+            break;
+        }
+    }
+
     /* we change the io_port to avoid ifdeffery in the main switch */
     orig_io_port = io_port;
     switch (io_port) {
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH] hw/qxl: warn on sync io usage

[Gerd Hoffmann]

  Hi,

> +static void sync_io_warning(PCIQXLDevice *qxl, uint32_t io_port)
> +{
> +    fprintf(stderr, "qxl-%d: WARNING: sync io used, see (RHBZ 747011)",
> +            qxl->id);
> +    fprintf(stderr, "qxl-%d: WARNING: virt-viewer/remote-viewer can hang\n",
> +            qxl->id);
> +    if (qxl->revision < 3) {
> +        fprintf(stderr, "qxl-%d: WARNING: revision >= 3 should be used\n",
> +                qxl->id);
> +    }
> +}

The message should also include hints how to fix that.

For the revision this probably means to update the machine type from
pc-0.12 (which sets rev=2 via compat properties) to something newer.
Telling the user what to do about it is tricky though as there seems to
be no simple GUI way to do that, at least not in virt-manager.  In the
other hand if the user manages to find the message in
/var/log/libvirt/qemu/${guest}.log he might be experienced enough to
just "virsh edit ${guest}".

For the sync I/O it's easy, just say something like "Update qxl drivers
in the guest."

BTW: You can print multi-line messages this way ...

	fprintf(stderr,
                "long line one\n"
		"long line two\n",
                args, here);

... which I find more readable in the source code.

Do we wanna have a "suggest to update guest drivers for device $foo" qmp
message for management?  Or has ovirt/rhev better ways (guest agent?) to
deal with that?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] hw/qxl: warn on sync io usage

[Alon Levy]

> Hi,
> 
> > +static void sync_io_warning(PCIQXLDevice *qxl, uint32_t io_port)
> > +{
> > +    fprintf(stderr, "qxl-%d: WARNING: sync io used, see (RHBZ
> > 747011)",
> > +            qxl->id);
> > +    fprintf(stderr, "qxl-%d: WARNING: virt-viewer/remote-viewer
> > can hang\n",
> > +            qxl->id);
> > +    if (qxl->revision < 3) {
> > +        fprintf(stderr, "qxl-%d: WARNING: revision >= 3 should be
> > used\n",
> > +                qxl->id);
> > +    }
> > +}
> 
> The message should also include hints how to fix that.
> 

Yes, that was the idea of the last line, but I like your idea of a QMP message below. Users divide into those who would launch qemu directly, and via administration interfaces, so I think a stderr message should be made for the former plus a QMP message for the later. The revision can be changed directly or via machine type like you noted. So I'll make a patch to warn on the former during initialization (revision = 2 due to [direct parameter|machine type] suggest [increase to 3|change machine type to >pc-0.12]). And I'll fix the later per your suggestion. I should also change spice to warn (we already warn when the keyboard is cleartext, but that warning is never displayed, so adding another such warning and fixing remote-viewer to display it).

> For the revision this probably means to update the machine type from
> pc-0.12 (which sets rev=2 via compat properties) to something newer.
> Telling the user what to do about it is tricky though as there seems
> to
> be no simple GUI way to do that, at least not in virt-manager.  In
> the
> other hand if the user manages to find the message in
> /var/log/libvirt/qemu/${guest}.log he might be experienced enough to
> just "virsh edit ${guest}".
> 
> For the sync I/O it's easy, just say something like "Update qxl
> drivers
> in the guest."
> 
> BTW: You can print multi-line messages this way ...
> 
> 	fprintf(stderr,
>                 "long line one\n"
> 		"long line two\n",
>                 args, here);

Thanks for that.

> 
> ... which I find more readable in the source code.
> 
> Do we wanna have a "suggest to update guest drivers for device $foo"
> qmp message for management?
I think so.

> Or has ovirt/rhev better ways (guest agent?) to deal with that?

I'll check.

> 
> cheers,
>   Gerd
> 
>