[Qemu-devel] [RFC 1/2] hw/qxl: ignore guest from guestbug until reset

[Qemu-devel] [RFC 1/2] hw/qxl: ignore guest from guestbug until reset

[Alon Levy]

reset only by a guest QXL_IO_RESET

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |    9 ++++++++-
 hw/qxl.h |    2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 6c11e70..a9a7778 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -141,6 +141,7 @@ static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
+    qxl->guest_bug = 1;
     if (qxl->guestdebug) {
         va_list ap;
         va_start(ap, msg);
@@ -571,7 +572,7 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     case QXL_MODE_NATIVE:
     case QXL_MODE_UNDEFINED:
         ring = &qxl->ram->cmd_ring;
-        if (SPICE_RING_IS_EMPTY(ring)) {
+        if (qxl->guest_bug || SPICE_RING_IS_EMPTY(ring)) {
             return false;
         }
         SPICE_RING_CONS_ITEM(qxl, ring, cmd);
@@ -1291,6 +1292,10 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     qxl_async_io async = QXL_SYNC;
     uint32_t orig_io_port = io_port;
 
+    if (d->guest_bug && !io_port == QXL_IO_RESET) {
+        return;
+    }
+
     switch (io_port) {
     case QXL_IO_RESET:
     case QXL_IO_SET_MODE:
@@ -1399,6 +1404,7 @@ async_common:
         }
         break;
     case QXL_IO_RESET:
+        d->guest_bug = 0;
         qxl_hard_reset(d, 0);
         break;
     case QXL_IO_MEMSLOT_ADD:
@@ -1742,6 +1748,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     qemu_mutex_init(&qxl->track_lock);
     qemu_mutex_init(&qxl->async_lock);
     qxl->current_async = QXL_UNDEFINED_IO;
+    qxl->guest_bug = 0;
 
     switch (qxl->revision) {
     case 1: /* spice 0.4 -- qxl-1 */
diff --git a/hw/qxl.h b/hw/qxl.h
index ec0cb1a..4e14643 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -35,6 +35,8 @@ typedef struct PCIQXLDevice {
     uint32_t           cmdlog;
     uint32_t           revision;
 
+    uint32_t           guest_bug;
+
     enum qxl_mode      mode;
     uint32_t           cmdflags;
     int                generation;
-- 
1.7.10.1

[Qemu-devel] [RFC 2/2] hmp/qxl: info spice: add qxl0_mode & qxl0_guest_bug

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hmp.c            |    7 +++++++
 hw/qxl.c         |   17 +++++++++++++++++
 qapi-schema.json |   32 ++++++++++++++++++++++++++++++--
 ui/qemu-spice.h  |    4 ++++
 ui/spice-core.c  |    3 +++
 5 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index 1f9fe0e..690da83 100644
--- a/hmp.c
+++ b/hmp.c
@@ -353,6 +353,13 @@ void hmp_info_spice(Monitor *mon)
     monitor_printf(mon, "  mouse-mode: %s\n",
                    SpiceQueryMouseMode_lookup[info->mouse_mode]);
 
+    if (info->qxl0_guest_bug != -1 && info->qxl0_mode != -1) {
+        monitor_printf(mon, " qxl0\n");
+        monitor_printf(mon, "   guest_bug: %"PRIu64"d\n", info->qxl0_guest_bug);
+        monitor_printf(mon, "        mode: %s\n",
+                SpiceQueryQXLMode_lookup[info->qxl0_mode]);
+    }
+
     if (!info->has_channels || info->channels == NULL) {
         monitor_printf(mon, "Channels: none\n");
     } else {
diff --git a/hw/qxl.c b/hw/qxl.c
index a9a7778..ddb1633 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1700,6 +1700,23 @@ static DisplayChangeListener display_listener = {
     .dpy_refresh = display_refresh,
 };
 
+/* helpers for spice_info. only show first device */
+int qxl0_guest_bug(void)
+{
+    if (!qxl0) {
+        return -1;
+    }
+    return qxl0->guest_bug;
+}
+
+int qxl0_mode(void)
+{
+    if (!qxl0) {
+        return -1;
+    }
+    return qxl0->mode;
+}
+
 static void qxl_init_ramsize(PCIQXLDevice *qxl, uint32_t ram_min_mb)
 {
     /* vga ram (bar 0) */
diff --git a/qapi-schema.json b/qapi-schema.json
index 4279259..edc958a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -636,7 +636,7 @@
 ##
 # @SpiceQueryMouseMode
 #
-# An enumation of Spice mouse states.
+# An enumeration of Spice mouse states.
 #
 # @client: Mouse cursor position is determined by the client.
 #
@@ -653,6 +653,29 @@
   'data': [ 'client', 'server', 'unknown' ] }
 
 ##
+# @SpiceQueryQXLMode
+#
+# An enumeration of QXL States.
+#
+# @undefined: guest driver in control but no primary device. Reached after a destroy primary IO
+#             from native mode.
+#
+# @vga: no device driver in control. default mode, returns to it after any vga port access.
+#
+# @compat: No information is available about mouse mode used by
+#           the spice server.
+#
+# @native: guest driver in control of device. Reached after a create primary IO.
+#
+# Note: hw/qxl.h has a qxl_mode enum, name chose to not confuse the two.
+#
+# Since: 1.1
+##
+{ 'enum': 'SpiceQueryQXLMode',
+  'data': [ 'undefined', 'vga', 'compat', 'native' ] }
+
+
+##
 # @SpiceInfo
 #
 # Information about the SPICE session.
@@ -681,12 +704,17 @@
 #
 # @channels: a list of @SpiceChannel for each active spice channel
 #
+# @qxl0_mode: Mode of the primary qxl device.
+#
+# @qxl0_guest_bug: Guest bug status of primary qxl device.
+#
 # Since: 0.14.0
 ##
 { 'type': 'SpiceInfo',
   'data': {'enabled': 'bool', '*host': 'str', '*port': 'int',
            '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
-           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']} }
+           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel'],
+           'qxl0_mode': 'SpiceQueryQXLMode', 'qxl0_guest_bug': 'int'} }
 
 ##
 # @query-spice
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 3299da8..e2f894b 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -47,6 +47,10 @@ void do_info_spice(Monitor *mon, QObject **ret_data);
 
 CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
 
+/* implemented in hw/qxl.c */
+int qxl0_guest_bug(void);
+int qxl0_mode(void);
+
 #else  /* CONFIG_SPICE */
 #include "monitor.h"
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4fc48f8..5616fe6 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -453,6 +453,9 @@ SpiceInfo *qmp_query_spice(Error **errp)
              SPICE_SERVER_VERSION & 0xff);
     info->compiled_version = g_strdup(version_string);
 
+    info->qxl0_mode = qxl0_mode();
+    info->qxl0_guest_bug = qxl0_guest_bug();
+
     if (port) {
         info->has_port = true;
         info->port = port;
-- 
1.7.10.1

Re: [Qemu-devel] [RFC 1/2] hw/qxl: ignore guest from guestbug until reset

[Gerd Hoffmann]

On 05/21/12 11:04, Alon Levy wrote:
> reset only by a guest QXL_IO_RESET

Looks sensible.  I guess you want clear guest_bug in the "guest touched
vga registers" code path too.

cheers,
  Gerd

Re: [Qemu-devel] [RFC 2/2] hmp/qxl: info spice: add qxl0_mode & qxl0_guest_bug

[Gerd Hoffmann]

  Hi,

> +    if (info->qxl0_guest_bug != -1 && info->qxl0_mode != -1) {
> +        monitor_printf(mon, " qxl0\n");
> +        monitor_printf(mon, "   guest_bug: %"PRIu64"d\n", info->qxl0_guest_bug);
> +        monitor_printf(mon, "        mode: %s\n",
> +                SpiceQueryQXLMode_lookup[info->qxl0_mode]);
> +    }

Anything else we might want export while being at it?  For example
whenever guest drivers use sync or async io commands?

What about secondary displays?

> +/* helpers for spice_info. only show first device */
> +int qxl0_guest_bug(void)
> +{
> +    if (!qxl0) {
> +        return -1;
> +    }
> +    return qxl0->guest_bug;
> +}
> +
> +int qxl0_mode(void)
> +{
> +    if (!qxl0) {
> +        return -1;
> +    }
> +    return qxl0->mode;
> +}

Hmm, that is a bit hackish.  qxl0 exists only for the reason that
displaychangelisteners don't support passing through a opaque pointer
(or other way to get the state).  I'd prefer to get rid of it, not add
more uses, although I can see that it is convenient ...


cheers,
  Gerd

Re: [Qemu-devel] [RFC 1/2] hw/qxl: ignore guest from guestbug until reset

[Alon Levy]

On Mon, May 21, 2012 at 11:12:35AM +0200, Gerd Hoffmann wrote:
> On 05/21/12 11:04, Alon Levy wrote:
> > reset only by a guest QXL_IO_RESET
> 
> Looks sensible.  I guess you want clear guest_bug in the "guest touched
> vga registers" code path too.

good point, will fix for v2.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [RFC 2/2] hmp/qxl: info spice: add qxl0_mode & qxl0_guest_bug

[Alon Levy]

On Mon, May 21, 2012 at 11:22:16AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > +    if (info->qxl0_guest_bug != -1 && info->qxl0_mode != -1) {
> > +        monitor_printf(mon, " qxl0\n");
> > +        monitor_printf(mon, "   guest_bug: %"PRIu64"d\n", info->qxl0_guest_bug);
> > +        monitor_printf(mon, "        mode: %s\n",
> > +                SpiceQueryQXLMode_lookup[info->qxl0_mode]);
> > +    }
> 
> Anything else we might want export while being at it?  For example
> whenever guest drivers use sync or async io commands?
> 
> What about secondary displays?

OK, so I can fix this and remove the qxl0 hack at the same stroke. I was
looking at using the system_bus, but that seems like another hack. The
alternative is to keep a linked list of qxl devices private to qxl.c and
use that, and the below functions become

int qxl_guest_bug(int index)
int qxl_mode(int index)
int qxl_device_count(void)

How does that sound?

> 
> > +/* helpers for spice_info. only show first device */
> > +int qxl0_guest_bug(void)
> > +{
> > +    if (!qxl0) {
> > +        return -1;
> > +    }
> > +    return qxl0->guest_bug;
> > +}
> > +
> > +int qxl0_mode(void)
> > +{
> > +    if (!qxl0) {
> > +        return -1;
> > +    }
> > +    return qxl0->mode;
> > +}
> 
> Hmm, that is a bit hackish.  qxl0 exists only for the reason that
> displaychangelisteners don't support passing through a opaque pointer
> (or other way to get the state).  I'd prefer to get rid of it, not add
> more uses, although I can see that it is convenient ...
> 
> 
> cheers,
>   Gerd

Re: [Qemu-devel] [RFC 2/2] hmp/qxl: info spice: add qxl0_mode & qxl0_guest_bug

[Gerd Hoffmann]

  Hi,

> OK, so I can fix this and remove the qxl0 hack at the same stroke. I was
> looking at using the system_bus, but that seems like another hack.

Didn't check what qom provides these days, searching for objects of a
certain type (for example a qxl device) should be possible IMO.  Dunno
whenever it actually is.

> The
> alternative is to keep a linked list of qxl devices private to qxl.c and
> use that, and the below functions become

Second best option if the QOM way doesn't fly.

> int qxl_guest_bug(int index)
> int qxl_mode(int index)
> int qxl_device_count(void)
> 
> How does that sound?

qxl_guest_bug(Object *dev) ?

cheers,
  Gerd

[Qemu-devel] [PATCH] hw/qxl: ignore guest from guestbug until reset

[Alon Levy]

soft_reset is called from any of:
 * QXL_IO_RESET
 * vga io
 * pci reset handler

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |   13 ++++++++++++-
 hw/qxl.h |    2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 05ff176..21c825c 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -141,6 +141,7 @@ static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
+    qxl->guest_bug = 1;
     if (qxl->guestdebug) {
         va_list ap;
         va_start(ap, msg);
@@ -151,6 +152,10 @@ void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
     }
 }
 
+static void qxl_clear_guest_bug(PCIQXLDevice *qxl)
+{
+    qxl->guest_bug = 0;
+}
 
 void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
@@ -572,7 +577,7 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     case QXL_MODE_NATIVE:
     case QXL_MODE_UNDEFINED:
         ring = &qxl->ram->cmd_ring;
-        if (SPICE_RING_IS_EMPTY(ring)) {
+        if (qxl->guest_bug || SPICE_RING_IS_EMPTY(ring)) {
             return false;
         }
         SPICE_RING_CONS_ITEM(qxl, ring, cmd);
@@ -980,6 +985,7 @@ static void qxl_soft_reset(PCIQXLDevice *d)
 {
     trace_qxl_soft_reset(d->id);
     qxl_check_state(d);
+    qxl_clear_guest_bug(d);
 
     if (d->id == 0) {
         qxl_enter_vga_mode(d);
@@ -1297,6 +1303,10 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     qxl_async_io async = QXL_SYNC;
     uint32_t orig_io_port = io_port;
 
+    if (d->guest_bug && !io_port == QXL_IO_RESET) {
+        return;
+    }
+
     switch (io_port) {
     case QXL_IO_RESET:
     case QXL_IO_SET_MODE:
@@ -1771,6 +1781,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     qemu_mutex_init(&qxl->track_lock);
     qemu_mutex_init(&qxl->async_lock);
     qxl->current_async = QXL_UNDEFINED_IO;
+    qxl->guest_bug = 0;
 
     switch (qxl->revision) {
     case 1: /* spice 0.4 -- qxl-1 */
diff --git a/hw/qxl.h b/hw/qxl.h
index 4e3b708..1dd8abb 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -35,6 +35,8 @@ typedef struct PCIQXLDevice {
     uint32_t           cmdlog;
     uint32_t           revision;
 
+    uint32_t           guest_bug;
+
     enum qxl_mode      mode;
     uint32_t           cmdflags;
     int                generation;
-- 
1.7.10.1