[Qemu-devel] [PATCHv2 1/3] ui/spice-display.c: add missing initialization for valgrind

[Qemu-devel] [PATCHv2 1/3] ui/spice-display.c: add missing initialization for valgrind

[Alon Levy]

We can't initialize QXLDevSurfaceCreate field by field because it has a
pa hole, and so 4 bytes remain uninitialized when building on x86-64, so
just memset.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 ui/spice-display.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 5418eb3..3e8f0b3 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -244,6 +244,8 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
 {
     QXLDevSurfaceCreate surface;
 
+    memset(&surface, 0, sizeof(surface));
+
     dprint(1, "%s: %dx%d\n", __FUNCTION__,
            ds_get_width(ssd->ds), ds_get_height(ssd->ds));
 
-- 
1.7.10.1

[Qemu-devel] [PATCHv2 2/3] hw/qxl: s/qxl_guest_bug/qxl_set_guest_bug/

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |   49 +++++++++++++++++++++++++++----------------------
 hw/qxl.h |    3 ++-
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 2f1022a..3e41988 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -30,7 +30,7 @@
 /*
  * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
  * such can be changed by the guest, so to avoid a guest trigerrable
- * abort we just set qxl_guest_bug and set the return to NULL. Still
+ * abort we just qxl_set_guest_bug and set the return to NULL. Still
  * it may happen as a result of emulator bug as well.
  */
 #undef SPICE_RING_PROD_ITEM
@@ -40,7 +40,7 @@
         uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
         typeof(&(r)->items[prod]) m_item = &(r)->items[prod];           \
         if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
-            qxl_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
+            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
                           "! %p <= %p < %p", (uint8_t *)start,          \
                           (uint8_t *)m_item, (uint8_t *)end);           \
             ret = NULL;                                                 \
@@ -56,7 +56,7 @@
         uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
         typeof(&(r)->items[cons]) m_item = &(r)->items[cons];           \
         if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
-            qxl_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
+            qxl_set_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
                           "! %p <= %p < %p", (uint8_t *)start,          \
                           (uint8_t *)m_item, (uint8_t *)end);           \
             ret = NULL;                                                 \
@@ -138,7 +138,7 @@ static void qxl_reset_memslots(PCIQXLDevice *d);
 static void qxl_reset_surfaces(PCIQXLDevice *d);
 static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 
-void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
+void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
     qxl->guest_bug = 1;
@@ -412,7 +412,8 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
         uint32_t id = le32_to_cpu(cmd->surface_id);
 
         if (id >= NUM_SURFACES) {
-            qxl_guest_bug(qxl, "QXL_CMD_SURFACE id %d >= %d", id, NUM_SURFACES);
+            qxl_set_guest_bug(qxl, "QXL_CMD_SURFACE id %d >= %d", id,
+                              NUM_SURFACES);
             return 1;
         }
         qemu_mutex_lock(&qxl->track_lock);
@@ -1062,12 +1063,12 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
     trace_qxl_memslot_add_guest(d->id, slot_id, guest_start, guest_end);
 
     if (slot_id >= NUM_MEMSLOTS) {
-        qxl_guest_bug(d, "%s: slot_id >= NUM_MEMSLOTS %d >= %d", __func__,
+        qxl_set_guest_bug(d, "%s: slot_id >= NUM_MEMSLOTS %d >= %d", __func__,
                       slot_id, NUM_MEMSLOTS);
         return 1;
     }
     if (guest_start > guest_end) {
-        qxl_guest_bug(d, "%s: guest_start > guest_end 0x%" PRIx64
+        qxl_set_guest_bug(d, "%s: guest_start > guest_end 0x%" PRIx64
                          " > 0x%" PRIx64, __func__, guest_start, guest_end);
         return 1;
     }
@@ -1092,7 +1093,7 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
         break;
     }
     if (i == ARRAY_SIZE(regions)) {
-        qxl_guest_bug(d, "%s: finished loop without match", __func__);
+        qxl_set_guest_bug(d, "%s: finished loop without match", __func__);
         return 1;
     }
 
@@ -1106,7 +1107,7 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
         break;
     default:
         /* should not happen */
-        qxl_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
+        qxl_set_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
         return 1;
     }
 
@@ -1157,21 +1158,24 @@ void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
         return (void *)(intptr_t)offset;
     case MEMSLOT_GROUP_GUEST:
         if (slot >= NUM_MEMSLOTS) {
-            qxl_guest_bug(qxl, "slot too large %d >= %d", slot, NUM_MEMSLOTS);
+            qxl_set_guest_bug(qxl, "slot too large %d >= %d", slot,
+                              NUM_MEMSLOTS);
             return NULL;
         }
         if (!qxl->guest_slots[slot].active) {
-            qxl_guest_bug(qxl, "inactive slot %d\n", slot);
+            qxl_set_guest_bug(qxl, "inactive slot %d\n", slot);
             return NULL;
         }
         if (offset < qxl->guest_slots[slot].delta) {
-            qxl_guest_bug(qxl, "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
+            qxl_set_guest_bug(qxl,
+                          "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
                           slot, offset, qxl->guest_slots[slot].delta);
             return NULL;
         }
         offset -= qxl->guest_slots[slot].delta;
         if (offset > qxl->guest_slots[slot].size) {
-            qxl_guest_bug(qxl, "slot %d offset %"PRIu64" > size %"PRIu64"\n",
+            qxl_set_guest_bug(qxl,
+                          "slot %d offset %"PRIu64" > size %"PRIu64"\n",
                           slot, offset, qxl->guest_slots[slot].size);
             return NULL;
         }
@@ -1193,7 +1197,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
 
     if (qxl->mode == QXL_MODE_NATIVE) {
-        qxl_guest_bug(qxl, "%s: nop since already in QXL_MODE_NATIVE",
+        qxl_set_guest_bug(qxl, "%s: nop since already in QXL_MODE_NATIVE",
                       __func__);
     }
     qxl_exit_vga_mode(qxl);
@@ -1347,7 +1351,7 @@ async_common:
         async = QXL_ASYNC;
         qemu_mutex_lock(&d->async_lock);
         if (d->current_async != QXL_UNDEFINED_IO) {
-            qxl_guest_bug(d, "%d async started before last (%d) complete",
+            qxl_set_guest_bug(d, "%d async started before last (%d) complete",
                 io_port, d->current_async);
             qemu_mutex_unlock(&d->async_lock);
             return;
@@ -1409,11 +1413,12 @@ async_common:
         break;
     case QXL_IO_MEMSLOT_ADD:
         if (val >= NUM_MEMSLOTS) {
-            qxl_guest_bug(d, "QXL_IO_MEMSLOT_ADD: val out of range");
+            qxl_set_guest_bug(d, "QXL_IO_MEMSLOT_ADD: val out of range");
             break;
         }
         if (d->guest_slots[val].active) {
-            qxl_guest_bug(d, "QXL_IO_MEMSLOT_ADD: memory slot already active");
+            qxl_set_guest_bug(d,
+                        "QXL_IO_MEMSLOT_ADD: memory slot already active");
             break;
         }
         d->guest_slots[val].slot = d->ram->mem_slot;
@@ -1421,14 +1426,14 @@ async_common:
         break;
     case QXL_IO_MEMSLOT_DEL:
         if (val >= NUM_MEMSLOTS) {
-            qxl_guest_bug(d, "QXL_IO_MEMSLOT_DEL: val out of range");
+            qxl_set_guest_bug(d, "QXL_IO_MEMSLOT_DEL: val out of range");
             break;
         }
         qxl_del_memslot(d, val);
         break;
     case QXL_IO_CREATE_PRIMARY:
         if (val != 0) {
-            qxl_guest_bug(d, "QXL_IO_CREATE_PRIMARY (async=%d): val != 0",
+            qxl_set_guest_bug(d, "QXL_IO_CREATE_PRIMARY (async=%d): val != 0",
                           async);
             goto cancel_async;
         }
@@ -1437,7 +1442,7 @@ async_common:
         break;
     case QXL_IO_DESTROY_PRIMARY:
         if (val != 0) {
-            qxl_guest_bug(d, "QXL_IO_DESTROY_PRIMARY (async=%d): val != 0",
+            qxl_set_guest_bug(d, "QXL_IO_DESTROY_PRIMARY (async=%d): val != 0",
                           async);
             goto cancel_async;
         }
@@ -1449,7 +1454,7 @@ async_common:
         break;
     case QXL_IO_DESTROY_SURFACE_WAIT:
         if (val >= NUM_SURFACES) {
-            qxl_guest_bug(d, "QXL_IO_DESTROY_SURFACE (async=%d):"
+            qxl_set_guest_bug(d, "QXL_IO_DESTROY_SURFACE (async=%d):"
                              "%" PRIu64 " >= NUM_SURFACES", async, val);
             goto cancel_async;
         }
@@ -1473,7 +1478,7 @@ async_common:
         qxl_spice_destroy_surfaces(d, async);
         break;
     default:
-        qxl_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
+        qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
     }
     return;
 cancel_async:
diff --git a/hw/qxl.h b/hw/qxl.h
index 4e14643..1dd8abb 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -132,7 +132,8 @@ typedef struct PCIQXLDevice {
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...) GCC_FMT_ATTR(2, 3);
+void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
+    GCC_FMT_ATTR(2, 3);
 
 void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
-- 
1.7.10.1

[Qemu-devel] [PATCHv2 3/3] hmp/qxl: info spice: add qxl info

[Alon Levy]

For all devices print id, mode and guest_bug status.

Known problems: Prints devices from highest id to lowest.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hmp.c            |   11 +++++++++++
 hw/qxl.c         |   22 ++++++++++++++++++++++
 qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 ui/qemu-spice.h  |    5 +++++
 ui/spice-core.c  |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 130 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index bb0952e..dd681a9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -331,6 +331,7 @@ void hmp_info_spice(Monitor *mon)
 {
     SpiceChannelList *chan;
     SpiceInfo *info;
+    QXLInfoList *qxl;
 
     info = qmp_query_spice(NULL);
 
@@ -353,6 +354,16 @@ void hmp_info_spice(Monitor *mon)
     monitor_printf(mon, "  mouse-mode: %s\n",
                    SpiceQueryMouseMode_lookup[info->mouse_mode]);
 
+    for (qxl = info->qxl; qxl; qxl = qxl->next) {
+        if (qxl->value->guest_bug == -1 || qxl->value->mode == -1) {
+            continue;
+        }
+        monitor_printf(mon, "qxl-%d:\n", qxl->value->id);
+        monitor_printf(mon, "        mode: %s\n",
+                SpiceQueryQXLMode_lookup[qxl->value->mode]);
+        monitor_printf(mon, "   guest_bug: %"PRIu64"\n", qxl->value->guest_bug);
+    }
+
     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 3e41988..8ebab6f 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1705,6 +1705,28 @@ static DisplayChangeListener display_listener = {
     .dpy_refresh = display_refresh,
 };
 
+/* helpers for spice_info */
+int qxl_get_guest_bug(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->guest_bug;
+}
+
+int qxl_get_mode(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->mode;
+}
+
+int qxl_get_id(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->id;
+}
+
 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 2ca7195..54997ef 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -638,7 +638,7 @@
 ##
 # @SpiceQueryMouseMode
 #
-# An enumation of Spice mouse states.
+# An enumeration of Spice mouse states.
 #
 # @client: Mouse cursor position is determined by the client.
 #
@@ -655,6 +655,44 @@
   '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' ] }
+
+##
+# @QXLInfo
+#
+# Information about a QXL device.
+#
+# @id: qxl id, non negative integer, 0 for primary device.
+#
+# @guest_bug: Has a guest error been detected.
+#
+# @mode: Mode of device, based on guest activity.
+#
+# Since: 1.1
+##
+{ 'type': 'QXLInfo',
+  'data': {'id': 'int', 'guest_bug': 'int', 'mode': 'SpiceQueryQXLMode'} }
+
+##
 # @SpiceInfo
 #
 # Information about the SPICE session.
@@ -683,12 +721,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'],
+           'qxl': ['QXLInfo']} }
 
 ##
 # @query-spice
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 3299da8..edcf3a5 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -47,6 +47,11 @@ void do_info_spice(Monitor *mon, QObject **ret_data);
 
 CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
 
+/* implemented in hw/qxl.c */
+int qxl_get_guest_bug(DeviceState *dev);
+int qxl_get_mode(DeviceState *dev);
+int qxl_get_id(DeviceState *dev);
+
 #else  /* CONFIG_SPICE */
 #include "monitor.h"
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4fc48f8..25833e5 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -22,7 +22,6 @@
 #include "sysemu.h"
 
 #include "qemu-common.h"
-#include "qemu-spice.h"
 #include "qemu-thread.h"
 #include "qemu-timer.h"
 #include "qemu-queue.h"
@@ -37,6 +36,8 @@
 #include "migration.h"
 #include "monitor.h"
 #include "hw/hw.h"
+#include "hw/qdev.h"
+#include "qemu-spice.h"
 
 /* core bits */
 
@@ -419,6 +420,50 @@ static SpiceChannelList *qmp_query_spice_channels(void)
     return head;
 }
 
+static int qdev_walk_qxl(DeviceState *dev, void *opaque)
+{
+    QXLInfoList **cur = opaque;
+    QXLInfoList *qxl_info;
+    int first = 0;
+    const char *class_name = object_get_typename(OBJECT(dev));
+
+    if (strcmp(class_name, "qxl") != 0 &&
+        strcmp(class_name, "qxl-vga") != 0) {
+        return 0;
+    }
+    if ((*cur)->value == NULL) {
+        first = 1;
+        qxl_info = *cur;
+    } else {
+        qxl_info = g_malloc(sizeof(*qxl_info));
+    }
+    qxl_info->next = NULL;
+    qxl_info->value = g_malloc(sizeof(*qxl_info->value));
+    qxl_info->value->id = qxl_get_id(dev);
+    qxl_info->value->guest_bug = qxl_get_guest_bug(dev);
+    qxl_info->value->mode = qxl_get_mode(dev);
+    if (!first) {
+        (*cur)->next = qxl_info;
+        *cur = qxl_info;
+    }
+    return 0;
+}
+
+static int qbus_walk_all(BusState *bus, void *opaque)
+{
+    return 0;
+}
+
+static QXLInfoList *qmp_query_qxl(void)
+{
+    QXLInfoList *root = g_malloc0(sizeof(*root));
+    QXLInfoList *cur = root;
+    BusState *default_bus = sysbus_get_default();
+
+    qbus_walk_children(default_bus, qdev_walk_qxl, qbus_walk_all, &cur);
+    return root;
+}
+
 SpiceInfo *qmp_query_spice(Error **errp)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
@@ -461,6 +506,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
         info->has_tls_port = true;
         info->tls_port = tls_port;
     }
+    info->qxl = qmp_query_qxl();
 
 #if SPICE_SERVER_VERSION >= 0x000a03 /* 0.10.3 */
     info->mouse_mode = spice_server_is_server_mouse(spice_server) ?
-- 
1.7.10.1

Re: [Qemu-devel] [PATCHv2 1/3] ui/spice-display.c: add missing initialization for valgrind

[Gerd Hoffmann]

On 05/22/12 17:29, Alon Levy wrote:
> We can't initialize QXLDevSurfaceCreate field by field because it has a
> pa hole, and so 4 bytes remain uninitialized when building on x86-64, so
> just memset.

So you get valgrind warnings for the hole?  why?  nobody should ever
access the hole, so the missing initialization should not hurt in theory ...

> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  ui/spice-display.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 5418eb3..3e8f0b3 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -244,6 +244,8 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>  {
>      QXLDevSurfaceCreate surface;
>  
> +    memset(&surface, 0, sizeof(surface));
> +
>      dprint(1, "%s: %dx%d\n", __FUNCTION__,
>             ds_get_width(ssd->ds), ds_get_height(ssd->ds));
>  

Re: [Qemu-devel] [PATCHv2 1/3] ui/spice-display.c: add missing initialization for valgrind

[Alon Levy]

On Wed, May 23, 2012 at 08:59:22PM +0200, Gerd Hoffmann wrote:
> On 05/22/12 17:29, Alon Levy wrote:
> > We can't initialize QXLDevSurfaceCreate field by field because it has a
> > pa hole, and so 4 bytes remain uninitialized when building on x86-64, so
> > just memset.
> 
> So you get valgrind warnings for the hole?  why?  nobody should ever
> access the hole, so the missing initialization should not hurt in theory ...

Because we allocate this struct on the stack and then copy it over an fd
to spice, through the dispatcher pipe.

echo quit | valgrind --log-file=/tmp/valgrind.log
--leak-check=full ./x86_64-softmmu/qemu-system-x86_64 -monitor stdio
-spice port=1234 -vga qxl

==2677== Syscall param write(buf) points to uninitialised byte(s)
==2677==    at 0x659504D: ??? (syscall-template.S:82)
==2677==    by 0x91BCD54: write_safe (dispatcher.c:103)
==2677==    by 0x91BD168: dispatcher_send_message (dispatcher.c:182)
==2677==    by 0x91BEC24: red_dispatcher_create_primary_surface_sync (red_dispatcher.c:489)
==2677==    by 0x91BECAD: red_dispatcher_create_primary_surface (red_dispatcher.c:502)
==2677==    by 0x91BED03: qxl_worker_create_primary_surface (red_dispatcher.c:509)
==2677==    by 0x3403CC: qemu_spice_create_primary_surface (spice-display.c:106)
==2677==    by 0x340BAB: qemu_spice_create_host_primary (spice-display.c:260)
==2677==    by 0x40EC50: qxl_enter_vga_mode (qxl.c:931)
==2677==    by 0x40EFCA: qxl_soft_reset (qxl.c:982)
==2677==    by 0x40F088: qxl_hard_reset (qxl.c:1004)
==2677==    by 0x40F0DA: qxl_reset_handler (qxl.c:1011)
==2677==  Address 0x7feffef98 is on thread 1's stack
> 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  ui/spice-display.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index 5418eb3..3e8f0b3 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -244,6 +244,8 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
> >  {
> >      QXLDevSurfaceCreate surface;
> >  
> > +    memset(&surface, 0, sizeof(surface));
> > +
> >      dprint(1, "%s: %dx%d\n", __FUNCTION__,
> >             ds_get_width(ssd->ds), ds_get_height(ssd->ds));
> >  
> 

Re: [Qemu-devel] [PATCHv2 1/3] ui/spice-display.c: add missing initialization for valgrind

[Gerd Hoffmann]

On 05/24/12 08:43, Alon Levy wrote:
> On Wed, May 23, 2012 at 08:59:22PM +0200, Gerd Hoffmann wrote:
>> On 05/22/12 17:29, Alon Levy wrote:
>>> We can't initialize QXLDevSurfaceCreate field by field because it has a
>>> pa hole, and so 4 bytes remain uninitialized when building on x86-64, so
>>> just memset.
>>
>> So you get valgrind warnings for the hole?  why?  nobody should ever
>> access the hole, so the missing initialization should not hurt in theory ...
> 
> Because we allocate this struct on the stack and then copy it over an fd
> to spice, through the dispatcher pipe.

Ah, yea, copying will make valgrind complain of course ...

I'll go queue it up.

cheers,
  Gerd

Re: [Qemu-devel] [PATCHv2 1/3] ui/spice-display.c: add missing initialization for valgrind

[Gerd Hoffmann]

  Hi,

> I'll go queue it up.

Ahem, doesn't apply cleanly.  Guess this is just a patch ordering issue
though.  Can you send all your pending qxl bits as single patch series
please, so I don't have to play trial-and-error to figure the correct order?

thanks,
  Gerd

[Qemu-devel] [PATCH 1/4] ui/spice-display.c: add missing initialization for valgrind

[Alon Levy]

We can't initialize QXLDevSurfaceCreate field by field because it has a
pa hole, and so 4 bytes remain uninitialized when building on x86-64, so
just memset.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 ui/spice-display.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 5418eb3..3e8f0b3 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -244,6 +244,8 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
 {
     QXLDevSurfaceCreate surface;
 
+    memset(&surface, 0, sizeof(surface));
+
     dprint(1, "%s: %dx%d\n", __FUNCTION__,
            ds_get_width(ssd->ds), ds_get_height(ssd->ds));
 
-- 
1.7.10.1

[Qemu-devel] [PATCH 2/4] hw/qxl: s/qxl_guest_bug/qxl_set_guest_bug/

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |   49 +++++++++++++++++++++++++++----------------------
 hw/qxl.h |    3 ++-
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 3da3399..8777ba9 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -30,7 +30,7 @@
 /*
  * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
  * such can be changed by the guest, so to avoid a guest trigerrable
- * abort we just set qxl_guest_bug and set the return to NULL. Still
+ * abort we just qxl_set_guest_bug and set the return to NULL. Still
  * it may happen as a result of emulator bug as well.
  */
 #undef SPICE_RING_PROD_ITEM
@@ -40,7 +40,7 @@
         uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
         typeof(&(r)->items[prod]) m_item = &(r)->items[prod];           \
         if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
-            qxl_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
+            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
                           "! %p <= %p < %p", (uint8_t *)start,          \
                           (uint8_t *)m_item, (uint8_t *)end);           \
             ret = NULL;                                                 \
@@ -56,7 +56,7 @@
         uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
         typeof(&(r)->items[cons]) m_item = &(r)->items[cons];           \
         if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
-            qxl_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
+            qxl_set_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
                           "! %p <= %p < %p", (uint8_t *)start,          \
                           (uint8_t *)m_item, (uint8_t *)end);           \
             ret = NULL;                                                 \
@@ -138,7 +138,7 @@ static void qxl_reset_memslots(PCIQXLDevice *d);
 static void qxl_reset_surfaces(PCIQXLDevice *d);
 static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 
-void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
+void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
     if (qxl->guestdebug) {
@@ -411,7 +411,8 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
         uint32_t id = le32_to_cpu(cmd->surface_id);
 
         if (id >= NUM_SURFACES) {
-            qxl_guest_bug(qxl, "QXL_CMD_SURFACE id %d >= %d", id, NUM_SURFACES);
+            qxl_set_guest_bug(qxl, "QXL_CMD_SURFACE id %d >= %d", id,
+                              NUM_SURFACES);
             return 1;
         }
         qemu_mutex_lock(&qxl->track_lock);
@@ -1061,12 +1062,12 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
     trace_qxl_memslot_add_guest(d->id, slot_id, guest_start, guest_end);
 
     if (slot_id >= NUM_MEMSLOTS) {
-        qxl_guest_bug(d, "%s: slot_id >= NUM_MEMSLOTS %d >= %d", __func__,
+        qxl_set_guest_bug(d, "%s: slot_id >= NUM_MEMSLOTS %d >= %d", __func__,
                       slot_id, NUM_MEMSLOTS);
         return 1;
     }
     if (guest_start > guest_end) {
-        qxl_guest_bug(d, "%s: guest_start > guest_end 0x%" PRIx64
+        qxl_set_guest_bug(d, "%s: guest_start > guest_end 0x%" PRIx64
                          " > 0x%" PRIx64, __func__, guest_start, guest_end);
         return 1;
     }
@@ -1091,7 +1092,7 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
         break;
     }
     if (i == ARRAY_SIZE(regions)) {
-        qxl_guest_bug(d, "%s: finished loop without match", __func__);
+        qxl_set_guest_bug(d, "%s: finished loop without match", __func__);
         return 1;
     }
 
@@ -1105,7 +1106,7 @@ static int qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
         break;
     default:
         /* should not happen */
-        qxl_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
+        qxl_set_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
         return 1;
     }
 
@@ -1156,21 +1157,24 @@ void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
         return (void *)(intptr_t)offset;
     case MEMSLOT_GROUP_GUEST:
         if (slot >= NUM_MEMSLOTS) {
-            qxl_guest_bug(qxl, "slot too large %d >= %d", slot, NUM_MEMSLOTS);
+            qxl_set_guest_bug(qxl, "slot too large %d >= %d", slot,
+                              NUM_MEMSLOTS);
             return NULL;
         }
         if (!qxl->guest_slots[slot].active) {
-            qxl_guest_bug(qxl, "inactive slot %d\n", slot);
+            qxl_set_guest_bug(qxl, "inactive slot %d\n", slot);
             return NULL;
         }
         if (offset < qxl->guest_slots[slot].delta) {
-            qxl_guest_bug(qxl, "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
+            qxl_set_guest_bug(qxl,
+                          "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
                           slot, offset, qxl->guest_slots[slot].delta);
             return NULL;
         }
         offset -= qxl->guest_slots[slot].delta;
         if (offset > qxl->guest_slots[slot].size) {
-            qxl_guest_bug(qxl, "slot %d offset %"PRIu64" > size %"PRIu64"\n",
+            qxl_set_guest_bug(qxl,
+                          "slot %d offset %"PRIu64" > size %"PRIu64"\n",
                           slot, offset, qxl->guest_slots[slot].size);
             return NULL;
         }
@@ -1192,7 +1196,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
 
     if (qxl->mode == QXL_MODE_NATIVE) {
-        qxl_guest_bug(qxl, "%s: nop since already in QXL_MODE_NATIVE",
+        qxl_set_guest_bug(qxl, "%s: nop since already in QXL_MODE_NATIVE",
                       __func__);
     }
     qxl_exit_vga_mode(qxl);
@@ -1342,7 +1346,7 @@ async_common:
         async = QXL_ASYNC;
         qemu_mutex_lock(&d->async_lock);
         if (d->current_async != QXL_UNDEFINED_IO) {
-            qxl_guest_bug(d, "%d async started before last (%d) complete",
+            qxl_set_guest_bug(d, "%d async started before last (%d) complete",
                 io_port, d->current_async);
             qemu_mutex_unlock(&d->async_lock);
             return;
@@ -1403,11 +1407,12 @@ async_common:
         break;
     case QXL_IO_MEMSLOT_ADD:
         if (val >= NUM_MEMSLOTS) {
-            qxl_guest_bug(d, "QXL_IO_MEMSLOT_ADD: val out of range");
+            qxl_set_guest_bug(d, "QXL_IO_MEMSLOT_ADD: val out of range");
             break;
         }
         if (d->guest_slots[val].active) {
-            qxl_guest_bug(d, "QXL_IO_MEMSLOT_ADD: memory slot already active");
+            qxl_set_guest_bug(d,
+                        "QXL_IO_MEMSLOT_ADD: memory slot already active");
             break;
         }
         d->guest_slots[val].slot = d->ram->mem_slot;
@@ -1415,14 +1420,14 @@ async_common:
         break;
     case QXL_IO_MEMSLOT_DEL:
         if (val >= NUM_MEMSLOTS) {
-            qxl_guest_bug(d, "QXL_IO_MEMSLOT_DEL: val out of range");
+            qxl_set_guest_bug(d, "QXL_IO_MEMSLOT_DEL: val out of range");
             break;
         }
         qxl_del_memslot(d, val);
         break;
     case QXL_IO_CREATE_PRIMARY:
         if (val != 0) {
-            qxl_guest_bug(d, "QXL_IO_CREATE_PRIMARY (async=%d): val != 0",
+            qxl_set_guest_bug(d, "QXL_IO_CREATE_PRIMARY (async=%d): val != 0",
                           async);
             goto cancel_async;
         }
@@ -1431,7 +1436,7 @@ async_common:
         break;
     case QXL_IO_DESTROY_PRIMARY:
         if (val != 0) {
-            qxl_guest_bug(d, "QXL_IO_DESTROY_PRIMARY (async=%d): val != 0",
+            qxl_set_guest_bug(d, "QXL_IO_DESTROY_PRIMARY (async=%d): val != 0",
                           async);
             goto cancel_async;
         }
@@ -1443,7 +1448,7 @@ async_common:
         break;
     case QXL_IO_DESTROY_SURFACE_WAIT:
         if (val >= NUM_SURFACES) {
-            qxl_guest_bug(d, "QXL_IO_DESTROY_SURFACE (async=%d):"
+            qxl_set_guest_bug(d, "QXL_IO_DESTROY_SURFACE (async=%d):"
                              "%" PRIu64 " >= NUM_SURFACES", async, val);
             goto cancel_async;
         }
@@ -1467,7 +1472,7 @@ async_common:
         qxl_spice_destroy_surfaces(d, async);
         break;
     default:
-        qxl_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
+        qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
     }
     return;
 cancel_async:
diff --git a/hw/qxl.h b/hw/qxl.h
index 3102950..f2d4fdc 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -127,7 +127,8 @@ typedef struct PCIQXLDevice {
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...) GCC_FMT_ATTR(2, 3);
+void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
+    GCC_FMT_ATTR(2, 3);
 
 void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
                            struct QXLRect *area, struct QXLRect *dirty_rects,
-- 
1.7.10.1

[Qemu-devel] [PATCH 3/4] hmp/qxl: info spice: add qxl info

[Alon Levy]

For all devices print id, mode and guest_bug status.

Known problems: Prints devices from highest id to lowest.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hmp.c            |   11 +++++++++++
 hw/qxl.c         |   22 ++++++++++++++++++++++
 qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 ui/qemu-spice.h  |    5 +++++
 ui/spice-core.c  |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 130 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index bb0952e..5126921 100644
--- a/hmp.c
+++ b/hmp.c
@@ -331,6 +331,7 @@ void hmp_info_spice(Monitor *mon)
 {
     SpiceChannelList *chan;
     SpiceInfo *info;
+    QXLInfoList *qxl;
 
     info = qmp_query_spice(NULL);
 
@@ -353,6 +354,16 @@ void hmp_info_spice(Monitor *mon)
     monitor_printf(mon, "  mouse-mode: %s\n",
                    SpiceQueryMouseMode_lookup[info->mouse_mode]);
 
+    for (qxl = info->qxl; qxl; qxl = qxl->next) {
+        if (qxl->value->guest_bug == -1 || qxl->value->mode == -1) {
+            continue;
+        }
+        monitor_printf(mon, "qxl-%"PRId64":\n", qxl->value->id);
+        monitor_printf(mon, "        mode: %s\n",
+                SpiceQueryQXLMode_lookup[qxl->value->mode]);
+        monitor_printf(mon, "   guest_bug: %"PRIu64"\n", qxl->value->guest_bug);
+    }
+
     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 8777ba9..e28c482 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1699,6 +1699,28 @@ static DisplayChangeListener display_listener = {
     .dpy_refresh = display_refresh,
 };
 
+/* helpers for spice_info */
+int qxl_get_guest_bug(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->guest_bug;
+}
+
+int qxl_get_mode(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->mode;
+}
+
+int qxl_get_id(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->id;
+}
+
 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 2ca7195..54997ef 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -638,7 +638,7 @@
 ##
 # @SpiceQueryMouseMode
 #
-# An enumation of Spice mouse states.
+# An enumeration of Spice mouse states.
 #
 # @client: Mouse cursor position is determined by the client.
 #
@@ -655,6 +655,44 @@
   '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' ] }
+
+##
+# @QXLInfo
+#
+# Information about a QXL device.
+#
+# @id: qxl id, non negative integer, 0 for primary device.
+#
+# @guest_bug: Has a guest error been detected.
+#
+# @mode: Mode of device, based on guest activity.
+#
+# Since: 1.1
+##
+{ 'type': 'QXLInfo',
+  'data': {'id': 'int', 'guest_bug': 'int', 'mode': 'SpiceQueryQXLMode'} }
+
+##
 # @SpiceInfo
 #
 # Information about the SPICE session.
@@ -683,12 +721,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'],
+           'qxl': ['QXLInfo']} }
 
 ##
 # @query-spice
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 3299da8..edcf3a5 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -47,6 +47,11 @@ void do_info_spice(Monitor *mon, QObject **ret_data);
 
 CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
 
+/* implemented in hw/qxl.c */
+int qxl_get_guest_bug(DeviceState *dev);
+int qxl_get_mode(DeviceState *dev);
+int qxl_get_id(DeviceState *dev);
+
 #else  /* CONFIG_SPICE */
 #include "monitor.h"
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4fc48f8..25833e5 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -22,7 +22,6 @@
 #include "sysemu.h"
 
 #include "qemu-common.h"
-#include "qemu-spice.h"
 #include "qemu-thread.h"
 #include "qemu-timer.h"
 #include "qemu-queue.h"
@@ -37,6 +36,8 @@
 #include "migration.h"
 #include "monitor.h"
 #include "hw/hw.h"
+#include "hw/qdev.h"
+#include "qemu-spice.h"
 
 /* core bits */
 
@@ -419,6 +420,50 @@ static SpiceChannelList *qmp_query_spice_channels(void)
     return head;
 }
 
+static int qdev_walk_qxl(DeviceState *dev, void *opaque)
+{
+    QXLInfoList **cur = opaque;
+    QXLInfoList *qxl_info;
+    int first = 0;
+    const char *class_name = object_get_typename(OBJECT(dev));
+
+    if (strcmp(class_name, "qxl") != 0 &&
+        strcmp(class_name, "qxl-vga") != 0) {
+        return 0;
+    }
+    if ((*cur)->value == NULL) {
+        first = 1;
+        qxl_info = *cur;
+    } else {
+        qxl_info = g_malloc(sizeof(*qxl_info));
+    }
+    qxl_info->next = NULL;
+    qxl_info->value = g_malloc(sizeof(*qxl_info->value));
+    qxl_info->value->id = qxl_get_id(dev);
+    qxl_info->value->guest_bug = qxl_get_guest_bug(dev);
+    qxl_info->value->mode = qxl_get_mode(dev);
+    if (!first) {
+        (*cur)->next = qxl_info;
+        *cur = qxl_info;
+    }
+    return 0;
+}
+
+static int qbus_walk_all(BusState *bus, void *opaque)
+{
+    return 0;
+}
+
+static QXLInfoList *qmp_query_qxl(void)
+{
+    QXLInfoList *root = g_malloc0(sizeof(*root));
+    QXLInfoList *cur = root;
+    BusState *default_bus = sysbus_get_default();
+
+    qbus_walk_children(default_bus, qdev_walk_qxl, qbus_walk_all, &cur);
+    return root;
+}
+
 SpiceInfo *qmp_query_spice(Error **errp)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
@@ -461,6 +506,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
         info->has_tls_port = true;
         info->tls_port = tls_port;
     }
+    info->qxl = qmp_query_qxl();
 
 #if SPICE_SERVER_VERSION >= 0x000a03 /* 0.10.3 */
     info->mouse_mode = spice_server_is_server_mouse(spice_server) ?
-- 
1.7.10.1

Re: [Qemu-devel] [PATCH 3/4] hmp/qxl: info spice: add qxl info

[Gerd Hoffmann]

On 05/24/12 11:38, Alon Levy wrote:
> For all devices print id, mode and guest_bug status.

Queued up 1+2+4

For this one I wanna have an additional ack from qmp camp.
Also a nit:

>  # @SpiceInfo
>  #
>  # Information about the SPICE session.
> @@ -683,12 +721,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.
> +#

Code changed, but docs didn't ...

Otherwise looks fine to me.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/4] hmp/qxl: info spice: add qxl info

[Alon Levy]

On Thu, May 24, 2012 at 02:58:40PM +0200, Gerd Hoffmann wrote:
> On 05/24/12 11:38, Alon Levy wrote:
> > For all devices print id, mode and guest_bug status.
> 
> Queued up 1+2+4

Please unqueue "qxl: stop dirty loging when not in vga mode" and I'll
send it again with the separate patch "hw/qxl: ignore guest from
guestbug until reset" that accidentally got squashed into it.

> 
> For this one I wanna have an additional ack from qmp camp.
> Also a nit:
> 
> >  # @SpiceInfo
> >  #
> >  # Information about the SPICE session.
> > @@ -683,12 +721,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.
> > +#
> 
> Code changed, but docs didn't ...
> 
> Otherwise looks fine to me.
> 
> cheers,
>   Gerd
> 

[Qemu-devel] [PATCH 1/2] qxl: stop dirty loging when not in vga mode

[Alon Levy]

Tested with linux guest. Not sure how to check actual performance affect
of this. Checked with the previously send traceevent that the kvm ioctl
to start/stop dirty logging is being called.
(KVM_SET_USER_MEMORY_REGION).

Signed-off-by: Alon Levy <alevy@redhat.com>
---
These are the two patches to queue instead of the previously squashed patch.

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

diff --git a/hw/qxl.c b/hw/qxl.c
index 8777ba9..5a7be60 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -932,6 +932,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
     qemu_spice_create_host_primary(&d->ssd);
     d->mode = QXL_MODE_VGA;
     memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
+    vga_dirty_log_start(&d->vga);
 }
 
 static void qxl_exit_vga_mode(PCIQXLDevice *d)
@@ -940,6 +941,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
         return;
     }
     trace_qxl_exit_vga_mode(d->id);
+    vga_dirty_log_stop(&d->vga);
     qxl_destroy_primary(d, QXL_SYNC);
 }
 
-- 
1.7.10.1

[Qemu-devel] [PATCH 2/2] 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 |    3 +++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 5a7be60..b5e53ce 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:
@@ -1749,6 +1759,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 f2d4fdc..a4ab7cc 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -31,6 +31,9 @@ typedef struct PCIQXLDevice {
     uint32_t           debug;
     uint32_t           guestdebug;
     uint32_t           cmdlog;
+
+    uint32_t           guest_bug;
+
     enum qxl_mode      mode;
     uint32_t           cmdflags;
     int                generation;
-- 
1.7.10.1

[Qemu-devel] [PATCH 4/4] qxl: stop dirty loging when not in vga mode

[Alon Levy]

Tested with linux guest. Not sure how to check actual performance affect
of this. Checked with the previously send traceevent that the kvm ioctl
to start/stop dirty logging is being called.
(KVM_SET_USER_MEMORY_REGION).

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

diff --git a/hw/qxl.c b/hw/qxl.c
index e28c482..05ff176 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -932,6 +932,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
     qemu_spice_create_host_primary(&d->ssd);
     d->mode = QXL_MODE_VGA;
     memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
+    vga_dirty_log_start(&d->vga);
 }
 
 static void qxl_exit_vga_mode(PCIQXLDevice *d)
@@ -940,6 +941,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
         return;
     }
     trace_qxl_exit_vga_mode(d->id);
+    vga_dirty_log_stop(&d->vga);
     qxl_destroy_primary(d, QXL_SYNC);
 }
 
-- 
1.7.10.1

Re: [Qemu-devel] [PATCH 1/4] ui/spice-display.c: add missing initialization for valgrind

[Alon Levy]

On Thu, May 24, 2012 at 12:38:11PM +0300, Alon Levy wrote:
> We can't initialize QXLDevSurfaceCreate field by field because it has a
> pa hole, and so 4 bytes remain uninitialized when building on x86-64, so
> just memset.

This was supposed to be in reply to "<4FBDF65F.8070609@redhat.com>",
rebased on just pulled master, so should apply cleanly.

> 
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  ui/spice-display.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 5418eb3..3e8f0b3 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -244,6 +244,8 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>  {
>      QXLDevSurfaceCreate surface;
>  
> +    memset(&surface, 0, sizeof(surface));
> +
>      dprint(1, "%s: %dx%d\n", __FUNCTION__,
>             ds_get_width(ssd->ds), ds_get_height(ssd->ds));
>  
> -- 
> 1.7.10.1
> 
>