[Qemu-devel] [PATCH v2] S3&S4 support

[Qemu-devel] [PATCH v2] S3&S4 support

[Alon Levy]

This is a rewrite of v1, doesn't use any new api, reuses stop+start+destroy_surfaces
instead of the previously introduced update_mem spice server function. stop does
a flush of all commands, updating all the surfaces, and with destroy surfaces the
result is the same - everything has been pushed either to the release ring or to
the last_release list.

I've sent a bunch of patches that have previously appeared on the list, annotated below.

The before last patch actually does the implementation of the two new io's to be added
to spice-protocol V10, QXL_IO_FLUSH_SURFACES and QXL_IO_FLUSH_RELEASE.

The last patch requires some more attention - see it's commit message.

Alon Levy (12):
  qxl: set mm_time in vga update
   - previously sent as an RfC
  qxl: interface_get_command: fix reported mode
   - fixed with Gerd's comments (print INVALID if not know, used where it seemed appropriate)
  qxl: add mode to debugprint on destroy primary
  qxl: allow QXL_IO_LOG also in vga
  qxl: abort on panic instead of exit
  qxl-logger: add timestamp to command log
  qxl: update and add debug prints
  qxl: add dev id to guest prints
  qxl: add io_port_to_string
  qxl: update revision to QXL_REVISION_STABLE_V10
  qxl: add QXL_IO_FLUSH_{SURFACES,RELEASE} for guest S3&S4 support
  qxl: add primary_created state, change mode lifetimes

 hw/qxl-logger.c    |    4 +-
 hw/qxl.c           |  180 ++++++++++++++++++++++++++++++++++++++++++++-------
 hw/qxl.h           |    3 +-
 ui/spice-display.c |    5 ++
 4 files changed, 165 insertions(+), 27 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: set mm_time in vga update

[Alon Levy]

This fixes a problem where on windows 7 startup phase, before the qxl driver
is loaded, the drawables are sufficiently large and video like to trigger a
stream, but the lack of a filled mm time field triggers a warning in spice-gtk.
---
 ui/spice-display.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index bb1e4a7..93ebc19 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -182,6 +182,7 @@ static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     QXLCommand *cmd;
     uint8_t *src, *dst;
     int by, bw, bh;
+    struct timespec time_space;
 
     if (qemu_spice_rect_is_empty(&ssd->dirty)) {
         return NULL;
@@ -208,6 +209,10 @@ static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
     drawable->surfaces_dest[0] = -1;
     drawable->surfaces_dest[1] = -1;
     drawable->surfaces_dest[2] = -1;
+    clock_gettime(CLOCK_MONOTONIC, &time_space);
+    /* time in milliseconds from epoch. */
+    drawable->mm_time = time_space.tv_sec * 1000
+                      + time_space.tv_nsec / 1000 / 1000;
 
     drawable->u.copy.rop_descriptor  = SPICE_ROPD_OP_PUT;
     drawable->u.copy.src_bitmap      = (intptr_t)image;
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: interface_get_command: fix reported mode

[Alon Levy]

report correct mode when in undefined mode.
introduces qxl_mode_to_string, and uses it in another place that looks
helpful (qxl_post_load)
---
 hw/qxl.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index e45f33a..e2b07dd 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -335,6 +335,21 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->n_surfaces = NUM_SURFACES;
 }
 
+static const char *qxl_mode_to_string(int mode)
+{
+    switch (mode) {
+    case QXL_MODE_COMPAT:
+        return "compat";
+    case QXL_MODE_NATIVE:
+        return "native";
+    case QXL_MODE_UNDEFINED:
+        return "undefined";
+    case QXL_MODE_VGA:
+        return "vga";
+    }
+    return "INVALID";
+}
+
 /* called from spice server thread context only */
 static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 {
@@ -357,18 +372,19 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         }
         qemu_mutex_unlock(&qxl->ssd.lock);
         if (ret) {
+            dprint(qxl, 2, "%s %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode));
             qxl_log_command(qxl, "vga", ext);
         }
         return ret;
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
     case QXL_MODE_UNDEFINED:
-        dprint(qxl, 2, "%s: %s\n", __FUNCTION__,
-               qxl->cmdflags ? "compat" : "native");
+        dprint(qxl, 4, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode));
         ring = &qxl->ram->cmd_ring;
         if (SPICE_RING_IS_EMPTY(ring)) {
             return false;
         }
+        dprint(qxl, 2, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode));
         SPICE_RING_CONS_ITEM(ring, cmd);
         ext->cmd      = *cmd;
         ext->group_id = MEMSLOT_GROUP_GUEST;
@@ -1500,7 +1516,8 @@ static int qxl_post_load(void *opaque, int version)
 
     d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset);
 
-    dprint(d, 1, "%s: restore mode\n", __FUNCTION__);
+    dprint(d, 1, "%s: restore mode (%s)\n", __FUNCTION__,
+        qxl_mode_to_string(d->mode));
     newmode = d->mode;
     d->mode = QXL_MODE_UNDEFINED;
     switch (newmode) {
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: add mode to debugprint on destroy primary

[Alon Levy]

---
 hw/qxl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index e2b07dd..5266707 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1088,7 +1088,7 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     }
     case QXL_IO_DESTROY_PRIMARY:
         PANIC_ON(val != 0);
-        dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (%d)\n", d->mode);
+        dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (%s)\n", qxl_mode_to_string(d->mode));
         if (d->mode != QXL_MODE_UNDEFINED) {
             d->mode = QXL_MODE_UNDEFINED;
             qemu_spice_destroy_primary_surface(&d->ssd, 0);
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: allow QXL_IO_LOG also in vga

[Alon Levy]

The driver may change us to vga mode and still issue a QXL_IO_LOG,
which we can easily support.
---
 hw/qxl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 5266707..96f9c55 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1001,9 +1001,9 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_MEMSLOT_ADD_ASYNC:
     case QXL_IO_MEMSLOT_DEL:
     case QXL_IO_CREATE_PRIMARY:
-    case QXL_IO_LOG:
     case QXL_IO_CREATE_PRIMARY_ASYNC:
     case QXL_IO_UPDATE_IRQ:
+    case QXL_IO_LOG:
         break;
     default:
         if (d->mode == QXL_MODE_NATIVE || d->mode == QXL_MODE_COMPAT)
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: abort on panic instead of exit

[Alon Levy]

---
 hw/qxl.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.h b/hw/qxl.h
index 6dd38e6..584f02b 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -87,7 +87,7 @@ typedef struct PCIQXLDevice {
 
 #define PANIC_ON(x) if ((x)) {                         \
     printf("%s: PANIC %s failed\n", __FUNCTION__, #x); \
-    exit(-1);                                          \
+    abort();                                          \
 }
 
 #define dprint(_qxl, _level, _fmt, ...)                                 \
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl-logger: add timestamp to command log

[Alon Levy]

---
 hw/qxl-logger.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/qxl-logger.c b/hw/qxl-logger.c
index 76f43e6..74cadba 100644
--- a/hw/qxl-logger.c
+++ b/hw/qxl-logger.c
@@ -19,6 +19,7 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu-timer.h"
 #include "qxl.h"
 
 static const char *qxl_type[] = {
@@ -223,7 +224,8 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
     if (!qxl->cmdlog) {
         return;
     }
-    fprintf(stderr, "qxl-%d/%s:", qxl->id, ring);
+    fprintf(stderr, "%ld qxl-%d/%s:", qemu_get_clock_ns(vm_clock),
+            qxl->id, ring);
     fprintf(stderr, " cmd @ 0x%" PRIx64 " %s%s", ext->cmd.data,
             qxl_name(qxl_type, ext->cmd.type),
             compat ? "(compat)" : "");
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: update and add debug prints

[Alon Levy]

---
 hw/qxl.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 96f9c55..865e985 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -361,7 +361,7 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 
     switch (qxl->mode) {
     case QXL_MODE_VGA:
-        dprint(qxl, 2, "%s: vga\n", __FUNCTION__);
+        dprint(qxl, 4, "%s: vga\n", __FUNCTION__);
         ret = false;
         qemu_mutex_lock(&qxl->ssd.lock);
         if (qxl->ssd.update != NULL) {
@@ -700,7 +700,8 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
     qemu_spice_create_host_memslot(&d->ssd);
     qxl_soft_reset(d);
 
-    dprint(d, 1, "%s: done\n", __FUNCTION__);
+    dprint(d, 1, "%s: done (num_free_res %d, %p)\n", __FUNCTION__,
+        d->num_free_res, d->last_release);
 }
 
 static void qxl_reset_handler(DeviceState *dev)
@@ -1040,6 +1041,7 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         pthread_yield();
         if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
+            dprint(d, 1, "QXL_IO_NOTIFY_OOM (return after pthread_yield)\n");
             break;
         }
         d->oom_running = 1;
@@ -1057,7 +1059,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         break;
     case QXL_IO_RESET:
-        dprint(d, 1, "QXL_IO_RESET\n");
+        dprint(d, 1, "QXL_IO_RESET %d (%p)\n", d->num_free_res,
+               d->last_release);
         qxl_hard_reset(d, 0);
         break;
     case QXL_IO_MEMSLOT_ADD:
@@ -1073,6 +1076,7 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         break;
     }
     case QXL_IO_MEMSLOT_DEL:
+        dprint(d, 1, "QXL_IO_MEMSLOT_DEL %d\n", val);
         qxl_del_memslot(d, val);
         break;
     case QXL_IO_CREATE_PRIMARY:
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: add dev id to guest prints

[Alon Levy]

---
 hw/qxl.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 865e985..3d5c823 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1055,7 +1055,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         break;
     case QXL_IO_LOG:
         if (d->guestdebug) {
-            fprintf(stderr, "qxl/guest: %s", d->ram->log_buf);
+            fprintf(stderr, "qxl/guest-%d: %ld: %s", d->id,
+                    qemu_get_clock_ns(vm_clock), d->ram->log_buf);
         }
         break;
     case QXL_IO_RESET:
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: add io_port_to_string

[Alon Levy]

---
 hw/qxl.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 3d5c823..b4bc376 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -350,6 +350,67 @@ static const char *qxl_mode_to_string(int mode)
     return "INVALID";
 }
 
+static const char *io_port_to_string(uint32_t io_port)
+{
+    if (io_port >= QXL_IO_RANGE_SIZE) {
+        return "out of range";
+    }
+    switch(io_port) {
+    case QXL_IO_NOTIFY_CMD:
+        return "QXL_IO_NOTIFY_CMD";
+    case QXL_IO_NOTIFY_CURSOR:
+        return "QXL_IO_NOTIFY_CURSOR";
+    case QXL_IO_UPDATE_AREA:
+        return "QXL_IO_UPDATE_AREA";
+    case QXL_IO_UPDATE_IRQ:
+        return "QXL_IO_UPDATE_IRQ";
+    case QXL_IO_NOTIFY_OOM:
+        return "QXL_IO_NOTIFY_OOM";
+    case QXL_IO_RESET:
+        return "QXL_IO_RESET";
+    case QXL_IO_SET_MODE:
+        return "QXL_IO_SET_MODE";
+    case QXL_IO_LOG:
+        return "QXL_IO_LOG";
+    case QXL_IO_MEMSLOT_ADD:
+        return "QXL_IO_MEMSLOT_ADD";
+    case QXL_IO_MEMSLOT_DEL:
+        return "QXL_IO_MEMSLOT_DEL";
+    case QXL_IO_DETACH_PRIMARY:
+        return "QXL_IO_DETACH_PRIMARY";
+    case QXL_IO_ATTACH_PRIMARY:
+        return "QXL_IO_ATTACH_PRIMARY";
+    case QXL_IO_CREATE_PRIMARY:
+        return "QXL_IO_CREATE_PRIMARY";
+    case QXL_IO_DESTROY_PRIMARY:
+        return "QXL_IO_DESTROY_PRIMARY";
+    case QXL_IO_DESTROY_SURFACE_WAIT:
+        return "QXL_IO_DESTROY_SURFACE_WAIT";
+    case QXL_IO_DESTROY_ALL_SURFACES:
+        return "QXL_IO_DESTROY_ALL_SURFACES";
+    case QXL_IO_UPDATE_AREA_ASYNC:
+        return "QXL_IO_UPDATE_AREA_ASYNC";
+    case QXL_IO_NOTIFY_OOM_ASYNC:
+        return "QXL_IO_NOTIFY_OOM_ASYNC";
+    case QXL_IO_MEMSLOT_ADD_ASYNC:
+        return "QXL_IO_MEMSLOT_ADD_ASYNC";
+    case QXL_IO_CREATE_PRIMARY_ASYNC:
+        return "QXL_IO_CREATE_PRIMARY_ASYNC";
+    case QXL_IO_DESTROY_PRIMARY_ASYNC:
+        return "QXL_IO_DESTROY_PRIMARY_ASYNC";
+    case QXL_IO_DESTROY_SURFACE_ASYNC:
+        return "QXL_IO_DESTROY_SURFACE_ASYNC";
+    case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
+        return "QXL_IO_DESTROY_ALL_SURFACES_ASYNC";
+    case QXL_IO_FLUSH_SURFACES:
+        return "QXL_IO_FLUSH_SURFACES";
+    case QXL_IO_FLUSH_RELEASE:
+        return "QXL_IO_FLUSH_RELEASE";
+    }
+    // not reached?
+    return "error in io_port_to_string";
+}
+
 /* called from spice server thread context only */
 static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
 {
@@ -1009,7 +1070,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     default:
         if (d->mode == QXL_MODE_NATIVE || d->mode == QXL_MODE_COMPAT)
             break;
-        dprint(d, 1, "%s: unexpected port 0x%x in vga mode\n", __FUNCTION__, io_port);
+        dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n",
+            __FUNCTION__, io_port, io_port_to_string(io_port));
         /* be nice to buggy guest drivers */
         if (io_port >= QXL_IO_UPDATE_AREA_ASYNC &&
             io_port <= QXL_IO_DESTROY_ALL_SURFACES_ASYNC) {
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: update revision to QXL_REVISION_STABLE_V10

[Alon Levy]

also errors if provided revision is wrong. 0 is reserved for experimental
revision, the other valid values are as before:
 1 - V04
 2 - V06
And the new
 3 - V10
---
 hw/qxl.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index b4bc376..969a984 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1424,18 +1424,24 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     qxl->num_surfaces = NUM_SURFACES;
 
     switch (qxl->revision) {
-    case 1: /* spice 0.4 -- qxl-1 */
+    case QXL_REVISION_STABLE_V04: /* spice 0.4 -- qxl-1 */
         pci_device_id  = QXL_DEVICE_ID_STABLE;
         pci_device_rev = QXL_REVISION_STABLE_V04;
         break;
-    case 2: /* spice 0.6 -- qxl-2 */
+    case QXL_REVISION_STABLE_V06: /* spice 0.6 -- qxl-2 */
         pci_device_id  = QXL_DEVICE_ID_STABLE;
         pci_device_rev = QXL_REVISION_STABLE_V06;
         break;
-    default: /* experimental */
+    case QXL_REVISION_STABLE_V10: /* spice 1.0 -- qxl-3 */
+        pci_device_id  = QXL_DEVICE_ID_STABLE;
+        pci_device_rev = QXL_REVISION_STABLE_V10;
+        break;
+    case 0: /* experimental */
         pci_device_id  = QXL_DEVICE_ID_DEVEL;
         pci_device_rev = 1;
         break;
+    default:
+        error_report("bad revision paramter");
     }
 
     pci_config_set_vendor_id(config, REDHAT_PCI_VENDOR_ID);
@@ -1723,7 +1729,7 @@ static PCIDeviceInfo qxl_info_secondary = {
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT32("ram_size", PCIQXLDevice, vga.vram_size, 64 * 1024 * 1024),
         DEFINE_PROP_UINT32("vram_size", PCIQXLDevice, vram_size, 64 * 1024 * 1024),
-        DEFINE_PROP_UINT32("revision", PCIQXLDevice, revision, 2),
+        DEFINE_PROP_UINT32("revision", PCIQXLDevice, revision, QXL_REVISION_STABLE_V10),
         DEFINE_PROP_UINT32("debug", PCIQXLDevice, debug, 0),
         DEFINE_PROP_UINT32("guestdebug", PCIQXLDevice, guestdebug, 0),
         DEFINE_PROP_UINT32("cmdlog", PCIQXLDevice, cmdlog, 0),
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: add QXL_IO_FLUSH_{SURFACES, RELEASE} for guest S3&S4 support

[Alon Levy]

Add two new IOs.
 QXL_IO_FLUSH_SURFACES - equivalent to update area for all surfaces, used
  to reduce vmexits from NumSurfaces to 1 on guest S3, S4 and resolution change (windows
  driver implementation is such that this is done on each of those occasions).
 QXL_IO_FLUSH_RELEASE - used to ensure anything on last_release is put on the release ring
  for the client to free.

Cc: Yonit Halperin <yhalperi@redhat.com>
---
 hw/qxl.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 969a984..fab0208 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1167,6 +1167,30 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_DESTROY_ALL_SURFACES:
         qemu_spice_destroy_surfaces(&d->ssd);
         break;
+    case QXL_IO_FLUSH_SURFACES:
+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES (%d) entry (%s, s#=%d, res#=%d)\n",
+            val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res);
+        qemu_spice_stop(&d->ssd);
+        qemu_spice_start(&d->ssd);
+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES exit (%s, s#=%d, res#=%d,%p)\n",
+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res, d->last_release);
+        break;
+    case QXL_IO_FLUSH_RELEASE: {
+        QXLReleaseRing *ring = &d->ram->release_ring;
+        if (ring->prod - ring->cons + 1 == ring->num_items) {
+            // TODO - "return" a value to the guest and let it loop?
+            fprintf(stderr,
+                "ERROR: no flush, full release ring [p%d,%dc]\n",
+                ring->prod, ring->cons);
+        }
+        qxl_push_free_res(d, 1 /* flush */);
+        dprint(d, 1, "QXL_IO_FLUSH_RELEASE exit (%s, s#=%d, res#=%d,%p)\n",
+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res, d->last_release);
+        break;
+    }
     case QXL_IO_MEMSLOT_ADD_ASYNC:
         PANIC_ON(val >= NUM_MEMSLOTS);
         PANIC_ON(d->guest_slots[val].active);
-- 
1.7.5.4

[Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes

[Alon Levy]

Currently we define the following relation between modes and primary surface
existance:

 QXL_MODE_COMPAT
 QXL_MODE_NATIVE  primary exists
 QXL_MODE_UNDEFINED
 QXL_MODE_VGA     primary doesn't exist

This, coupled with disallowing various IO's when not in QXL_MODE_NATIVE or COMPAT
means that the S3 implementation gets more complex - This is wrong:
 DESTROY_SURFACES
 SURFACE_RELEASE
 .. wakeup
 PRIMARY_CREATE <-- error, we are already in NATIVE_MODE, not allowed!

This patch adds an explicit primary_created status, changed by:
CREATE_PRIMARY, DESTROY_PRIMARY, DESTROY_SURFACES

This patch also changes the lifetime of QXL_MODE_NATIVE.
 CREATE_PRIMARY - enter QXL_MODE_NATIVE
 vga io - exit QXL_MODE_NATIVE

anything else doesn't effect it, specifically DESTROY_PRIMARY.
---
 hw/qxl.c |   38 +++++++++++++++++++++++++++-----------
 hw/qxl.h |    1 +
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index fab0208..51a5270 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -666,6 +666,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
         return;
     }
     dprint(d, 1, "%s\n", __FUNCTION__);
+    d->primary_created = 1;
     qemu_spice_create_host_primary(&d->ssd);
     d->mode = QXL_MODE_VGA;
     memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
@@ -677,10 +678,9 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
         return;
     }
     dprint(d, 1, "%s\n", __FUNCTION__);
-    if (d->mode != QXL_MODE_UNDEFINED) {
-        d->mode = QXL_MODE_UNDEFINED;
-        qemu_spice_destroy_primary_surface(&d->ssd, 0);
-    }
+    d->mode = QXL_MODE_UNDEFINED;
+    d->primary_created = 0;
+    qemu_spice_destroy_primary_surface(&d->ssd, 0);
 }
 
 static void qxl_set_irq(PCIQXLDevice *d)
@@ -780,7 +780,9 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         dprint(qxl, 1, "%s\n", __FUNCTION__);
         if (qxl->mode != QXL_MODE_UNDEFINED) {
             qxl->mode = QXL_MODE_UNDEFINED;
-            qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
+            if (qxl->primary_created) {
+                qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
+            }
         }
         qxl_soft_reset(qxl);
     }
@@ -912,8 +914,10 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
 {
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
 
-    assert(qxl->mode != QXL_MODE_NATIVE);
-    qxl_exit_vga_mode(qxl);
+    if (qxl->mode != QXL_MODE_NATIVE) {
+        qxl_exit_vga_mode(qxl);
+    }
+    assert(!qxl->primary_created);
 
     dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__,
            le32_to_cpu(sc->width), le32_to_cpu(sc->height));
@@ -933,6 +937,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
         surface->flags |= QXL_SURF_FLAG_KEEP_DATA;
     }
 
+    qxl->primary_created = 1;
     qxl->mode = QXL_MODE_NATIVE;
     qxl->cmdflags = 0;
 
@@ -1156,15 +1161,19 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_DESTROY_PRIMARY:
         PANIC_ON(val != 0);
         dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (%s)\n", qxl_mode_to_string(d->mode));
-        if (d->mode != QXL_MODE_UNDEFINED) {
-            d->mode = QXL_MODE_UNDEFINED;
+        if (d->primary_created) {
+            d->primary_created = 0;
             qemu_spice_destroy_primary_surface(&d->ssd, 0);
         }
         break;
     case QXL_IO_DESTROY_SURFACE_WAIT:
+        if (val == 0) {
+            d->primary_created = 0;
+        }
         qemu_spice_destroy_surface_wait(&d->ssd, val);
         break;
     case QXL_IO_DESTROY_ALL_SURFACES:
+        d->primary_created = 0;
         qemu_spice_destroy_surfaces(&d->ssd);
         break;
     case QXL_IO_FLUSH_SURFACES:
@@ -1209,8 +1218,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_DESTROY_PRIMARY_ASYNC:
         PANIC_ON(val != 0);
         dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC\n");
-        if (d->mode != QXL_MODE_UNDEFINED) {
-            d->mode = QXL_MODE_UNDEFINED;
+        if (d->primary_created) {
+            d->primary_created = 0;
             async = qemu_mallocz(sizeof(*async));
             goto async_common;
         } else {
@@ -1218,13 +1227,20 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
             break;
         }
     case QXL_IO_UPDATE_AREA_ASYNC:
+        dprint(d, 1, "QXL_IO_UPDATE_AREA_ASYNC %d\n", val);
         async = qemu_mallocz(sizeof(*async));
         async->update_area = d->ram->update_area;
         async->update_surface = d->ram->update_surface;
         goto async_common;
     case QXL_IO_NOTIFY_OOM_ASYNC:
+        goto async_common;
     case QXL_IO_DESTROY_SURFACE_ASYNC:
+        if (val == 0) {
+            d->primary_created = 0;
+        }
+        goto async_common;
     case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
+        d->primary_created = 0;
         async = qemu_mallocz(sizeof(*async));
     async_common:
         async->port = io_port;
diff --git a/hw/qxl.h b/hw/qxl.h
index 584f02b..893f8d8 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -23,6 +23,7 @@ typedef struct PCIQXLDevice {
     uint32_t           guestdebug;
     uint32_t           cmdlog;
     enum qxl_mode      mode;
+    uint32_t           primary_created;
     uint32_t           cmdflags;
     int                generation;
     uint32_t           revision;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes

[yhalperi]

On 06/24/2011 04:02 PM, Alon Levy wrote:
> Currently we define the following relation between modes and primary surface
> existance:
>
>   QXL_MODE_COMPAT
>   QXL_MODE_NATIVE  primary exists
>   QXL_MODE_UNDEFINED
>   QXL_MODE_VGA     primary doesn't exist
>
> This, coupled with disallowing various IO's when not in QXL_MODE_NATIVE or COMPAT
> means that the S3 implementation gets more complex - This is wrong:
>   DESTROY_SURFACES
>   SURFACE_RELEASE
>   .. wakeup
>   PRIMARY_CREATE<-- error, we are already in NATIVE_MODE, not allowed!
>

Shouldn't the mode change to QXL_MODE_UNDEFINED on DESTROY_SURFACES? (I 
can see it currently doesn't, but it seems like a mistake)
> This patch adds an explicit primary_created status, changed by:
> CREATE_PRIMARY, DESTROY_PRIMARY, DESTROY_SURFACES
>
> This patch also changes the lifetime of QXL_MODE_NATIVE.
>   CREATE_PRIMARY - enter QXL_MODE_NATIVE
>   vga io - exit QXL_MODE_NATIVE
>
> anything else doesn't effect it, specifically DESTROY_PRIMARY.
> ---
>   hw/qxl.c |   38 +++++++++++++++++++++++++++-----------
>   hw/qxl.h |    1 +
>   2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/hw/qxl.c b/hw/qxl.c
> index fab0208..51a5270 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -666,6 +666,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
>           return;
>       }
>       dprint(d, 1, "%s\n", __FUNCTION__);
> +    d->primary_created = 1;
>       qemu_spice_create_host_primary(&d->ssd);
>       d->mode = QXL_MODE_VGA;
>       memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
> @@ -677,10 +678,9 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
>           return;
>       }
>       dprint(d, 1, "%s\n", __FUNCTION__);
> -    if (d->mode != QXL_MODE_UNDEFINED) {
> -        d->mode = QXL_MODE_UNDEFINED;
> -        qemu_spice_destroy_primary_surface(&d->ssd, 0);
> -    }
> +    d->mode = QXL_MODE_UNDEFINED;
> +    d->primary_created = 0;
> +    qemu_spice_destroy_primary_surface(&d->ssd, 0);
>   }
>
>   static void qxl_set_irq(PCIQXLDevice *d)
> @@ -780,7 +780,9 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>           dprint(qxl, 1, "%s\n", __FUNCTION__);
>           if (qxl->mode != QXL_MODE_UNDEFINED) {
>               qxl->mode = QXL_MODE_UNDEFINED;
> -            qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
> +            if (qxl->primary_created) {
> +                qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
> +            }
>           }
>           qxl_soft_reset(qxl);
>       }
> @@ -912,8 +914,10 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>   {
>       QXLSurfaceCreate *sc =&qxl->guest_primary.surface;
>
> -    assert(qxl->mode != QXL_MODE_NATIVE);
> -    qxl_exit_vga_mode(qxl);
> +    if (qxl->mode != QXL_MODE_NATIVE) {
> +        qxl_exit_vga_mode(qxl);
> +    }
> +    assert(!qxl->primary_created);
>
>       dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__,
>              le32_to_cpu(sc->width), le32_to_cpu(sc->height));
> @@ -933,6 +937,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>           surface->flags |= QXL_SURF_FLAG_KEEP_DATA;
>       }
>
> +    qxl->primary_created = 1;
>       qxl->mode = QXL_MODE_NATIVE;
>       qxl->cmdflags = 0;
>
> @@ -1156,15 +1161,19 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
>       case QXL_IO_DESTROY_PRIMARY:
>           PANIC_ON(val != 0);
>           dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (%s)\n", qxl_mode_to_string(d->mode));
> -        if (d->mode != QXL_MODE_UNDEFINED) {
> -            d->mode = QXL_MODE_UNDEFINED;
> +        if (d->primary_created) {
> +            d->primary_created = 0;
>               qemu_spice_destroy_primary_surface(&d->ssd, 0);
>           }
>           break;
>       case QXL_IO_DESTROY_SURFACE_WAIT:
> +        if (val == 0) {
> +            d->primary_created = 0;
> +        }
>           qemu_spice_destroy_surface_wait(&d->ssd, val);
>           break;
>       case QXL_IO_DESTROY_ALL_SURFACES:
> +        d->primary_created = 0;
>           qemu_spice_destroy_surfaces(&d->ssd);
>           break;
>       case QXL_IO_FLUSH_SURFACES:
> @@ -1209,8 +1218,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
>       case QXL_IO_DESTROY_PRIMARY_ASYNC:
>           PANIC_ON(val != 0);
>           dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC\n");
> -        if (d->mode != QXL_MODE_UNDEFINED) {
> -            d->mode = QXL_MODE_UNDEFINED;
> +        if (d->primary_created) {
> +            d->primary_created = 0;
>               async = qemu_mallocz(sizeof(*async));
>               goto async_common;
>           } else {
> @@ -1218,13 +1227,20 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
>               break;
>           }
>       case QXL_IO_UPDATE_AREA_ASYNC:
> +        dprint(d, 1, "QXL_IO_UPDATE_AREA_ASYNC %d\n", val);
>           async = qemu_mallocz(sizeof(*async));
>           async->update_area = d->ram->update_area;
>           async->update_surface = d->ram->update_surface;
>           goto async_common;
>       case QXL_IO_NOTIFY_OOM_ASYNC:
> +        goto async_common;
>       case QXL_IO_DESTROY_SURFACE_ASYNC:
> +        if (val == 0) {
> +            d->primary_created = 0;
> +        }
> +        goto async_common;
>       case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
> +        d->primary_created = 0;
>           async = qemu_mallocz(sizeof(*async));
>       async_common:
>           async->port = io_port;
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 584f02b..893f8d8 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -23,6 +23,7 @@ typedef struct PCIQXLDevice {
>       uint32_t           guestdebug;
>       uint32_t           cmdlog;
>       enum qxl_mode      mode;
> +    uint32_t           primary_created;
>       uint32_t           cmdflags;
>       int                generation;
>       uint32_t           revision;

Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes

[Alon Levy]

On Mon, Jun 27, 2011 at 10:42:46AM +0300, yhalperi wrote:
> On 06/24/2011 04:02 PM, Alon Levy wrote:
> >Currently we define the following relation between modes and primary surface
> >existance:
> >
> >  QXL_MODE_COMPAT
> >  QXL_MODE_NATIVE  primary exists
> >  QXL_MODE_UNDEFINED
> >  QXL_MODE_VGA     primary doesn't exist
> >
> >This, coupled with disallowing various IO's when not in QXL_MODE_NATIVE or COMPAT
> >means that the S3 implementation gets more complex - This is wrong:
> >  DESTROY_SURFACES
> >  SURFACE_RELEASE
> >  .. wakeup
> >  PRIMARY_CREATE<-- error, we are already in NATIVE_MODE, not allowed!
> >
> 
> Shouldn't the mode change to QXL_MODE_UNDEFINED on DESTROY_SURFACES?
> (I can see it currently doesn't, but it seems like a mistake)

No, that is the point - this patch makes the lifetime of QXL_MODE_NATIVE be
the lifetime of the driver in the guest, kinda. we never unload the driver
in the guest, the only time it stops being operational is when we enter vga
mode and that's explicitly done when we get any vga port write/read.

In other words, with this patch the only thing that changes qxl->mode is:
 PRIMARY_CREATE - qxl->mode=QXL_MODE_NATIVE
 vga port write - qxl->mode=QXL_MODE_VGA

so yes, UNDEFINED becomes useless. Should be dropped then, no?

> >This patch adds an explicit primary_created status, changed by:
> >CREATE_PRIMARY, DESTROY_PRIMARY, DESTROY_SURFACES
> >
> >This patch also changes the lifetime of QXL_MODE_NATIVE.
> >  CREATE_PRIMARY - enter QXL_MODE_NATIVE
> >  vga io - exit QXL_MODE_NATIVE
> >
> >anything else doesn't effect it, specifically DESTROY_PRIMARY.
> >---
> >  hw/qxl.c |   38 +++++++++++++++++++++++++++-----------
> >  hw/qxl.h |    1 +
> >  2 files changed, 28 insertions(+), 11 deletions(-)
> >
> >diff --git a/hw/qxl.c b/hw/qxl.c
> >index fab0208..51a5270 100644
> >--- a/hw/qxl.c
> >+++ b/hw/qxl.c
> >@@ -666,6 +666,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
> >          return;
> >      }
> >      dprint(d, 1, "%s\n", __FUNCTION__);
> >+    d->primary_created = 1;
> >      qemu_spice_create_host_primary(&d->ssd);
> >      d->mode = QXL_MODE_VGA;
> >      memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
> >@@ -677,10 +678,9 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
> >          return;
> >      }
> >      dprint(d, 1, "%s\n", __FUNCTION__);
> >-    if (d->mode != QXL_MODE_UNDEFINED) {
> >-        d->mode = QXL_MODE_UNDEFINED;
> >-        qemu_spice_destroy_primary_surface(&d->ssd, 0);
> >-    }
> >+    d->mode = QXL_MODE_UNDEFINED;
> >+    d->primary_created = 0;
> >+    qemu_spice_destroy_primary_surface(&d->ssd, 0);
> >  }
> >
> >  static void qxl_set_irq(PCIQXLDevice *d)
> >@@ -780,7 +780,9 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          dprint(qxl, 1, "%s\n", __FUNCTION__);
> >          if (qxl->mode != QXL_MODE_UNDEFINED) {
> >              qxl->mode = QXL_MODE_UNDEFINED;
> >-            qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
> >+            if (qxl->primary_created) {
> >+                qemu_spice_destroy_primary_surface(&qxl->ssd, 0);
> >+            }
> >          }
> >          qxl_soft_reset(qxl);
> >      }
> >@@ -912,8 +914,10 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
> >  {
> >      QXLSurfaceCreate *sc =&qxl->guest_primary.surface;
> >
> >-    assert(qxl->mode != QXL_MODE_NATIVE);
> >-    qxl_exit_vga_mode(qxl);
> >+    if (qxl->mode != QXL_MODE_NATIVE) {
> >+        qxl_exit_vga_mode(qxl);
> >+    }
> >+    assert(!qxl->primary_created);
> >
> >      dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__,
> >             le32_to_cpu(sc->width), le32_to_cpu(sc->height));
> >@@ -933,6 +937,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
> >          surface->flags |= QXL_SURF_FLAG_KEEP_DATA;
> >      }
> >
> >+    qxl->primary_created = 1;
> >      qxl->mode = QXL_MODE_NATIVE;
> >      qxl->cmdflags = 0;
> >
> >@@ -1156,15 +1161,19 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >      case QXL_IO_DESTROY_PRIMARY:
> >          PANIC_ON(val != 0);
> >          dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (%s)\n", qxl_mode_to_string(d->mode));
> >-        if (d->mode != QXL_MODE_UNDEFINED) {
> >-            d->mode = QXL_MODE_UNDEFINED;
> >+        if (d->primary_created) {
> >+            d->primary_created = 0;
> >              qemu_spice_destroy_primary_surface(&d->ssd, 0);
> >          }
> >          break;
> >      case QXL_IO_DESTROY_SURFACE_WAIT:
> >+        if (val == 0) {
> >+            d->primary_created = 0;
> >+        }
> >          qemu_spice_destroy_surface_wait(&d->ssd, val);
> >          break;
> >      case QXL_IO_DESTROY_ALL_SURFACES:
> >+        d->primary_created = 0;
> >          qemu_spice_destroy_surfaces(&d->ssd);
> >          break;
> >      case QXL_IO_FLUSH_SURFACES:
> >@@ -1209,8 +1218,8 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >      case QXL_IO_DESTROY_PRIMARY_ASYNC:
> >          PANIC_ON(val != 0);
> >          dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC\n");
> >-        if (d->mode != QXL_MODE_UNDEFINED) {
> >-            d->mode = QXL_MODE_UNDEFINED;
> >+        if (d->primary_created) {
> >+            d->primary_created = 0;
> >              async = qemu_mallocz(sizeof(*async));
> >              goto async_common;
> >          } else {
> >@@ -1218,13 +1227,20 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >              break;
> >          }
> >      case QXL_IO_UPDATE_AREA_ASYNC:
> >+        dprint(d, 1, "QXL_IO_UPDATE_AREA_ASYNC %d\n", val);
> >          async = qemu_mallocz(sizeof(*async));
> >          async->update_area = d->ram->update_area;
> >          async->update_surface = d->ram->update_surface;
> >          goto async_common;
> >      case QXL_IO_NOTIFY_OOM_ASYNC:
> >+        goto async_common;
> >      case QXL_IO_DESTROY_SURFACE_ASYNC:
> >+        if (val == 0) {
> >+            d->primary_created = 0;
> >+        }
> >+        goto async_common;
> >      case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
> >+        d->primary_created = 0;
> >          async = qemu_mallocz(sizeof(*async));
> >      async_common:
> >          async->port = io_port;
> >diff --git a/hw/qxl.h b/hw/qxl.h
> >index 584f02b..893f8d8 100644
> >--- a/hw/qxl.h
> >+++ b/hw/qxl.h
> >@@ -23,6 +23,7 @@ typedef struct PCIQXLDevice {
> >      uint32_t           guestdebug;
> >      uint32_t           cmdlog;
> >      enum qxl_mode      mode;
> >+    uint32_t           primary_created;
> >      uint32_t           cmdflags;
> >      int                generation;
> >      uint32_t           revision;
> 

Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes

[Gerd Hoffmann]

   Hi,

>> Shouldn't the mode change to QXL_MODE_UNDEFINED on DESTROY_SURFACES?
>> (I can see it currently doesn't, but it seems like a mistake)
>
> No, that is the point - this patch makes the lifetime of QXL_MODE_NATIVE be
> the lifetime of the driver in the guest, kinda. we never unload the driver
> in the guest, the only time it stops being operational is when we enter vga
> mode and that's explicitly done when we get any vga port write/read.

I agree with yonit here, moving to UNDEFINED makes sense.  Keeping track 
of guest driver state in qxl is asking for trouble.  It will blow up 
when it comes to S4 support and we'll exit and restart the qemu process.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v2] qxl: allow QXL_IO_LOG also in vga

[Gerd Hoffmann]

   Hi,

> The driver may change us to vga mode and still issue a QXL_IO_LOG,
> which we can easily support.

> @@ -1001,9 +1001,9 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> -    case QXL_IO_LOG:
>       case QXL_IO_CREATE_PRIMARY_ASYNC:
>       case QXL_IO_UPDATE_IRQ:
> +    case QXL_IO_LOG:
>           break;

That is a NOP, and it also doesn't apply.  It isn't the only patch which 
doesn't apply cleanly, looks like your series isn't based on 
upstream/master ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v2] qxl: add primary_created state, change mode lifetimes

[Alon Levy]

On Wed, Jun 29, 2011 at 10:41:43AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >>Shouldn't the mode change to QXL_MODE_UNDEFINED on DESTROY_SURFACES?
> >>(I can see it currently doesn't, but it seems like a mistake)
> >
> >No, that is the point - this patch makes the lifetime of QXL_MODE_NATIVE be
> >the lifetime of the driver in the guest, kinda. we never unload the driver
> >in the guest, the only time it stops being operational is when we enter vga
> >mode and that's explicitly done when we get any vga port write/read.
> 
> I agree with yonit here, moving to UNDEFINED makes sense.  Keeping
> track of guest driver state in qxl is asking for trouble.  It will
> blow up when it comes to S4 support and we'll exit and restart the
> qemu process.

Actually S4 support == S3 support. There is absolutely no difference. Since qemu
still does a reset. But I already checked dropping this patch and using UNDEFINED
instead and it works fine. I just had to additionally allow io when in UNDEFINED. I
think that makes sense, since UNDEFINED is just the same as "driver working but primary
destroyed".

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH v2] qxl: allow QXL_IO_LOG also in vga

[Alon Levy]

On Wed, Jun 29, 2011 at 11:22:05AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >The driver may change us to vga mode and still issue a QXL_IO_LOG,
> >which we can easily support.
> 
> >@@ -1001,9 +1001,9 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >-    case QXL_IO_LOG:
> >      case QXL_IO_CREATE_PRIMARY_ASYNC:
> >      case QXL_IO_UPDATE_IRQ:
> >+    case QXL_IO_LOG:
> >          break;
> 
> That is a NOP, and it also doesn't apply.  It isn't the only patch
> which doesn't apply cleanly, looks like your series isn't based on
> upstream/master ...

Yeah, my bad. I did something wierd, and missed this. I'll send a v3. Do you have other comments?

Unrelated, but have you looked at the async patches that I sent, and yonit's comments?
 - (me) catch EINTR and EAGAIN in async thread read
 - (yonit) reset surfaces count and surfaces when doing destroy_all

And I think there were some others I forget.

> 
> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH v2] qxl: allow QXL_IO_LOG also in vga

[Gerd Hoffmann]

   Hi,

> Yeah, my bad. I did something wierd, and missed this. I'll send a v3. Do you have other comments?

I stopped looking after figuring stuff doesn't apply to master.

> Unrelated, but have you looked at the async patches that I sent, and yonit's comments?
>   - (me) catch EINTR and EAGAIN in async thread read
>   - (yonit) reset surfaces count and surfaces when doing destroy_all

http://cgit.freedesktop.org/spice/qemu/log/?h=bz700134 is what I 
currently have.  Some of your patches are in there already.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v2] qxl: allow QXL_IO_LOG also in vga

[Alon Levy]

On Wed, Jun 29, 2011 at 12:02:30PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >Yeah, my bad. I did something wierd, and missed this. I'll send a v3. Do you have other comments?
> 
> I stopped looking after figuring stuff doesn't apply to master.
> 
> >Unrelated, but have you looked at the async patches that I sent, and yonit's comments?
> >  - (me) catch EINTR and EAGAIN in async thread read
> >  - (yonit) reset surfaces count and surfaces when doing destroy_all
> 
> http://cgit.freedesktop.org/spice/qemu/log/?h=bz700134 is what I
> currently have.  Some of your patches are in there already.

I tried rebasing on that, testing showed there was a deadlock, you can't reuse wlock for
track_command since it is called from interface_get_command, called from red_worker, which
may be during update_area, which already took the wlock. Adding a separate track_lock will
probably work.

> 
> cheers,
>   Gerd
> 

[Qemu-devel] [PATCH v2] qxl: add QXL_IO_FLUSH_{SURFACES, RELEASE} for guest S3&S4 support

[Alon Levy]

Add two new IOs.
 QXL_IO_FLUSH_SURFACES - equivalent to update area for all surfaces, used
  to reduce vmexits from NumSurfaces to 1 on guest S3, S4 and resolution change (windows
  driver implementation is such that this is done on each of those occasions).
 QXL_IO_FLUSH_RELEASE - used to ensure anything on last_release is put on the release ring
  for the client to free.

Cc: Yonit Halperin <yhalperi@redhat.com>
---
 hw/qxl.c |   25 +++++++++++++++++++++++++
 hw/qxl.h |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index e57ccf2..4007847 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -177,6 +177,11 @@ static void qxl_spice_destroy_surface_wait_async(PCIQXLDevice *qxl, uint32_t id,
     }
 }
 
+void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl)
+{
+    qxl->ssd.worker->flush_surfaces_async(qxl->ssd.worker, 0);
+}
+
 void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt *ext,
                                uint32_t count)
 {
@@ -1187,6 +1192,7 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
     case QXL_IO_DESTROY_PRIMARY_ASYNC:
     case QXL_IO_DESTROY_SURFACE_ASYNC:
     case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
+    case QXL_IO_FLUSH_SURFACES_ASYNC:
         if (!qemu_spice_supports_async(&d->ssd)) {
             fprintf(stderr, "qxl: error: async not supported by libspice but guest driver used it\n");
             return;
@@ -1306,6 +1312,25 @@ static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         qxl_spice_destroy_surface_wait_async(d, val, async);
         break;
+    case QXL_IO_FLUSH_RELEASE: {
+        QXLReleaseRing *ring = &d->ram->release_ring;
+        if (ring->prod - ring->cons + 1 == ring->num_items) {
+            fprintf(stderr,
+                "ERROR: no flush, full release ring [p%d,%dc]\n",
+                ring->prod, ring->cons);
+        }
+        qxl_push_free_res(d, 1 /* flush */);
+        dprint(d, 1, "QXL_IO_FLUSH_RELEASE exit (%s, s#=%d, res#=%d,%p)\n",
+            qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+            d->num_free_res, d->last_release);
+        break;
+    }
+    case QXL_IO_FLUSH_SURFACES_ASYNC:
+        dprint(d, 1, "QXL_IO_FLUSH_SURFACES_ASYNC (%d) (%s, s#=%d, res#=%d)\n",
+               val, qxl_mode_to_string(d->mode), d->guest_surfaces.count,
+               d->num_free_res);
+        qxl_spice_flush_surfaces_async(d);
+        break;
     case QXL_IO_DESTROY_ALL_SURFACES_ASYNC:
     case QXL_IO_DESTROY_ALL_SURFACES:
         d->mode = QXL_MODE_UNDEFINED;
diff --git a/hw/qxl.h b/hw/qxl.h
index 16639ce..23d6215 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -129,6 +129,7 @@ void qxl_spice_oom_async(PCIQXLDevice *qxl, int async);
 void qxl_spice_reset_memslots(PCIQXLDevice *qxl);
 void qxl_spice_reset_image_cache(PCIQXLDevice *qxl);
 void qxl_spice_reset_cursor(PCIQXLDevice *qxl);
+void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl);
 
 /* qxl-logger.c */
 void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id);
-- 
1.7.5.4