[Qemu-devel] [PATCH 1/3] hw/qxl: tracing fixes

[Qemu-devel] [PATCH 1/3] hw/qxl: tracing fixes

[Alon Levy]

Add two new trace events:
qxl_send_events(int qid, uint32_t events) "%d %d"
qxl_set_guest_bug(int qid) "%d"

Change qxl_io_unexpected_vga_mode parameters to be equivalent to those
of qxl_io_write for easier grouping under a single systemtap probe.

Change d to qxl in one place.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c     | 8 +++++---
 trace-events | 6 ++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 80ba401..94eb3c8 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, ...)
 {
+    trace_qxl_set_guest_bug(qxl->id);
     qxl_send_events(qxl, QXL_INTERRUPT_ERROR);
     qxl->guest_bug = 1;
     if (qxl->guestdebug) {
@@ -1403,7 +1404,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
             break;
         }
         trace_qxl_io_unexpected_vga_mode(d->id,
-            io_port, io_port_to_string(io_port));
+            addr, val, io_port_to_string(io_port));
         /* be nice to buggy guest drivers */
         if (io_port >= QXL_IO_UPDATE_AREA_ASYNC &&
             io_port < QXL_IO_RANGE_SIZE) {
@@ -1595,9 +1596,9 @@ cancel_async:
 static uint64_t ioport_read(void *opaque, target_phys_addr_t addr,
                             unsigned size)
 {
-    PCIQXLDevice *d = opaque;
+    PCIQXLDevice *qxl = opaque;
 
-    trace_qxl_io_read_unexpected(d->id);
+    trace_qxl_io_read_unexpected(qxl->id);
     return 0xff;
 }
 
@@ -1627,6 +1628,7 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
     uint32_t old_pending;
     uint32_t le_events = cpu_to_le32(events);
 
+    trace_qxl_send_events(d->id, events);
     assert(qemu_spice_display_is_running(&d->ssd));
     old_pending = __sync_fetch_and_or(&d->ram->int_pending, le_events);
     if ((old_pending & le_events) == le_events) {
diff --git a/trace-events b/trace-events
index 8fcbc50..42dfb93 100644
--- a/trace-events
+++ b/trace-events
@@ -925,7 +925,7 @@ qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
 qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty) "%d #dirty=%d"
 qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
 qxl_io_read_unexpected(int qid) "%d"
-qxl_io_unexpected_vga_mode(int qid, uint32_t io_port, const char *desc) "%d 0x%x (%s)"
+qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
 qxl_io_write(int qid, const char *mode, uint64_t addr, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " val=%"PRIu64" size=%u async=%d"
 qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
 qxl_post_load(int qid, const char *mode) "%d %s"
@@ -956,7 +956,7 @@ qxl_spice_destroy_surfaces(int qid, int async) "%d async=%d"
 qxl_spice_destroy_surface_wait_complete(int qid, uint32_t id) "%d sid=%d"
 qxl_spice_destroy_surface_wait(int qid, uint32_t id, int async) "%d sid=%d async=%d"
 qxl_spice_flush_surfaces_async(int qid, uint32_t surface_count, uint32_t num_free_res) "%d s#=%d, res#=%d"
-qxl_spice_monitors_config(int id) "%d"
+qxl_spice_monitors_config(int qid) "%d"
 qxl_spice_loadvm_commands(int qid, void *ext, uint32_t count) "%d ext=%p count=%d"
 qxl_spice_oom(int qid) "%d"
 qxl_spice_reset_cursor(int qid) "%d"
@@ -965,6 +965,8 @@ qxl_spice_reset_memslots(int qid) "%d"
 qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t right, uint32_t top, uint32_t bottom) "%d sid=%d [%d,%d,%d,%d]"
 qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
 qxl_surfaces_dirty(int qid, int surface, int offset, int size) "%d surface=%d offset=%d size=%d"
+qxl_send_events(int qid, uint32_t events) "%d %d"
+qxl_set_guest_bug(int qid) "%d"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.7.12

[Qemu-devel] [PATCH 2/3] hw/qxl: add support for QXL_IO_CAPABILITIES_SET

[Alon Levy]

Guest set capabilities are written to the spice server via
spice_qxl_guest_capabilities_set, provided the compiled against
spice server is new enough to provide this symbol.

QXL device revision will be raised in a later patch to advertise this
support to the guest.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 configure    |  7 +++++++
 hw/qxl.c     | 24 ++++++++++++++++++++++++
 trace-events |  1 +
 3 files changed, 32 insertions(+)

diff --git a/configure b/configure
index 30be784..d0e3eb1 100755
--- a/configure
+++ b/configure
@@ -2709,6 +2709,9 @@ EOF
     if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
         spice_qxl_io_monitors_config_async="yes"
     fi
+    if $pkg_config --atleast-version=0.12.2 spice-protocol >/dev/null 2>&1; then
+        spice_qxl_io_client_monitors_config="yes"
+    fi
   else
     if test "$spice" = "yes" ; then
       feature_not_found "spice"
@@ -3456,6 +3459,10 @@ if test "$spice_qxl_io_monitors_config_async" = "yes" ; then
   echo "CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC=y" >> $config_host_mak
 fi
 
+if test "$spice_qxl_io_client_monitors_config" = "yes" ; then
+  echo "CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG=y" >> $config_host_mak
+fi
+
 if test "$smartcard" = "yes" ; then
   echo "CONFIG_SMARTCARD=y" >> $config_host_mak
 fi
diff --git a/hw/qxl.c b/hw/qxl.c
index 94eb3c8..583a2d9 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -32,6 +32,10 @@
 #define QXL_IO_MONITORS_CONFIG_ASYNC (QXL_IO_FLUSH_RELEASE + 1)
 #endif
 
+#ifndef CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG
+#define QXL_IO_CAPABILITIES_SET (QXL_IO_FLUSH_RELEASE + 2)
+#endif
+
 /*
  * 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
@@ -289,6 +293,21 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
 #endif
 }
 
+static void qxl_spice_guest_capabilities_set(PCIQXLDevice *qxl)
+{
+#if SPICE_SERVER_VERSION >= 0x000b05 && \
+    defined(CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG)
+    uint8_t guest_capabilities[sizeof(qxl->ram->guest_capabilities)];
+
+    trace_qxl_io_capabilities_set(qxl->id, sizeof(qxl->ram->guest_capabilities),
+                                  qxl->ram->guest_capabilities);
+    memcpy(guest_capabilities, qxl->ram->guest_capabilities,
+           sizeof(guest_capabilities));
+    spice_qxl_guest_capabilities_set(&qxl->ssd.qxl, sizeof(guest_capabilities),
+                                     guest_capabilities);
+#endif
+}
+
 void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
 {
     trace_qxl_spice_reset_image_cache(qxl->id);
@@ -576,6 +595,7 @@ static const char *io_port_to_string(uint32_t io_port)
         [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
         [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
         [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
+        [QXL_IO_CAPABILITIES_SET]       = "QXL_IO_CAPABILITIES_SET",
     };
 
     if (io_port >= ARRAY_SIZE(io_port_to_string)) {
@@ -1398,6 +1418,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
     case QXL_IO_LOG:
     case QXL_IO_MEMSLOT_ADD_ASYNC:
     case QXL_IO_CREATE_PRIMARY_ASYNC:
+    case QXL_IO_CAPABILITIES_SET:
         break;
     default:
         if (d->mode != QXL_MODE_VGA) {
@@ -1580,6 +1601,9 @@ async_common:
     case QXL_IO_MONITORS_CONFIG_ASYNC:
         qxl_spice_monitors_config_async(d, 0);
         break;
+    case QXL_IO_CAPABILITIES_SET:
+        qxl_spice_guest_capabilities_set(d);
+        break;
     default:
         qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
     }
diff --git a/trace-events b/trace-events
index 42dfb93..7a35f62 100644
--- a/trace-events
+++ b/trace-events
@@ -927,6 +927,7 @@ qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
 qxl_io_read_unexpected(int qid) "%d"
 qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
 qxl_io_write(int qid, const char *mode, uint64_t addr, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " val=%"PRIu64" size=%u async=%d"
+qxl_io_capabilities_set(int qid, int caps_size, uint8_t *caps) "%d %d %p"
 qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
 qxl_post_load(int qid, const char *mode) "%d %s"
 qxl_pre_load(int qid) "%d"
-- 
1.7.12

[Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

Until now we used only the agent to change the monitor count and each
monitor resolution. This patch introduces the qemu part of using the
device as the mediator instead of the agent via virtio-serial.

Spice (>=0.11.5) calls the new QXLInterface::client_monitors_config,
generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which the
client indicates handling of (after reading from
QXLRom::client_monitors_config) by QXL_IO_CLIENT_MONITORS_CONFIG_DONE.

The maximal number of monitors is limited on the QXLRom to 63.

QXL checks the existance of a QXL_GUEST_CAP_CLIENT_MONITORS_CONFIG_ISR
before issuing the interrupt. (this is in addition to spice checking for
the same capability).

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qxl.h     |  2 ++
 trace-events |  5 ++++
 3 files changed, 82 insertions(+)

diff --git a/hw/qxl.c b/hw/qxl.c
index 583a2d9..18c63ff 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -34,6 +34,7 @@
 
 #ifndef CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG
 #define QXL_IO_CAPABILITIES_SET (QXL_IO_FLUSH_RELEASE + 2)
+#define QXL_IO_CLIENT_MONITORS_CONFIG_DONE (QXL_IO_FLUSH_RELEASE + 3)
 #endif
 
 /*
@@ -596,6 +597,8 @@ static const char *io_port_to_string(uint32_t io_port)
         [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
         [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
         [QXL_IO_CAPABILITIES_SET]       = "QXL_IO_CAPABILITIES_SET",
+        [QXL_IO_CLIENT_MONITORS_CONFIG_DONE] =
+                                          "QXL_IO_CLIENT_MONITORS_CONFIG_DONE",
     };
 
     if (io_port >= ARRAY_SIZE(io_port_to_string)) {
@@ -986,6 +989,64 @@ static void interface_set_client_capabilities(QXLInstance *sin,
 
 #endif
 
+#if defined(CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG) \
+    && SPICE_SERVER_VERSION >= 0x000b05
+static int qxl_test_guest_cap(PCIQXLDevice *qxl, unsigned cap)
+{
+    int ind = cap / 8;
+    uint8_t mask = 1 << (cap & 7);
+
+    if (ind >= sizeof(qxl->ram->guest_capabilities)) {
+        return 0;
+    }
+    return qxl->ram->guest_capabilities[ind] & mask;
+}
+
+/* called from main context only */
+static void interface_client_monitors_config(QXLInstance *sin,
+                                        VDAgentMonitorsConfig *monitors_config)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
+    int i;
+
+    if (!qxl_test_guest_cap(qxl, QXL_GUEST_CAP_CLIENT_MONITORS_CONFIG_ISR)) {
+        trace_qxl_client_monitors_config_unsupported_by_guest(qxl->id,
+                                                              monitors_config);
+        return;
+    }
+    if (qxl->client_monitors_config_isr_in_progress) {
+        trace_qxl_client_monitors_config_ignored_guest_busy(qxl->id,
+                                                            monitors_config);
+        return;
+    }
+    rom->client_monitors_config.count = monitors_config->num_of_monitors;
+    /* monitors_config->flags ignored */
+    if (rom->client_monitors_config.count >=
+            ARRAY_SIZE(rom->client_monitors_config.heads)) {
+        trace_qxl_client_monitors_config_capped(qxl->id,
+                                monitors_config->num_of_monitors,
+                                ARRAY_SIZE(rom->client_monitors_config.heads));
+        rom->client_monitors_config.count =
+            ARRAY_SIZE(rom->client_monitors_config.heads);
+    }
+    for (i = 0 ; i < rom->client_monitors_config.count ; ++i) {
+        VDAgentMonConfig *monitor = &monitors_config->monitors[i];
+        QXLURect *rect = &rom->client_monitors_config.heads[i];
+        /* monitor->depth ignored */
+        rect->left = monitor->x;
+        rect->top = monitor->y;
+        rect->right = monitor->x + monitor->width;
+        rect->bottom = monitor->y + monitor->height;
+    }
+    trace_qxl_interrupt_client_monitors_config(qxl->id,
+                        rom->client_monitors_config.count,
+                        rom->client_monitors_config.heads);
+    qxl->client_monitors_config_isr_in_progress = 1;
+    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+}
+#endif
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -1010,6 +1071,10 @@ static const QXLInterface qxl_interface = {
 #if SPICE_SERVER_VERSION >= 0x000b04
     .set_client_capabilities = interface_set_client_capabilities,
 #endif
+#if SPICE_SERVER_VERSION >= 0x000b05 && \
+    defined(CONFIG_QXL_IO_CLIENT_MONITORS_CONFIG)
+    .client_monitors_config = interface_client_monitors_config,
+#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1604,6 +1669,16 @@ async_common:
     case QXL_IO_CAPABILITIES_SET:
         qxl_spice_guest_capabilities_set(d);
         break;
+    case QXL_IO_CLIENT_MONITORS_CONFIG_DONE:
+        trace_qxl_io_client_monitors_config_done(d->id);
+        if (!d->client_monitors_config_isr_in_progress) {
+            qxl_set_guest_bug(d,
+                "%s: unexpected QXL_IO_CLIENT_MONITORS_CONFIG_DONE\n",
+                __func__);
+        } else {
+            d->client_monitors_config_isr_in_progress = 0;
+        }
+        break;
     default:
         qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
     }
diff --git a/hw/qxl.h b/hw/qxl.h
index 5553824..a1ede3c 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -72,6 +72,8 @@ typedef struct PCIQXLDevice {
 
     QXLPHYSICAL        guest_monitors_config;
 
+    int                client_monitors_config_isr_in_progress;
+
     QemuMutex          track_lock;
 
     /* thread signaling */
diff --git a/trace-events b/trace-events
index 7a35f62..4ea7cdd 100644
--- a/trace-events
+++ b/trace-events
@@ -928,6 +928,7 @@ qxl_io_read_unexpected(int qid) "%d"
 qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
 qxl_io_write(int qid, const char *mode, uint64_t addr, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " val=%"PRIu64" size=%u async=%d"
 qxl_io_capabilities_set(int qid, int caps_size, uint8_t *caps) "%d %d %p"
+qxl_io_client_monitors_config_done(int qid) "%d"
 qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
 qxl_post_load(int qid, const char *mode) "%d %s"
 qxl_pre_load(int qid) "%d"
@@ -968,6 +969,10 @@ qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dir
 qxl_surfaces_dirty(int qid, int surface, int offset, int size) "%d surface=%d offset=%d size=%d"
 qxl_send_events(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
+qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
+qxl_client_monitors_config_unsupported_by_guest(int qid, void *client_monitors_config) "%d %p"
+qxl_client_monitors_config_ignored_guest_busy(int qid, void *client_monitors_config) "%d %p"
+qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.7.12

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Gerd Hoffmann]

On 09/11/12 08:56, Alon Levy wrote:
> Until now we used only the agent to change the monitor count and each
> monitor resolution. This patch introduces the qemu part of using the
> device as the mediator instead of the agent via virtio-serial.
> 
> Spice (>=0.11.5) calls the new QXLInterface::client_monitors_config,
> generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which the
> client indicates handling of (after reading from
> QXLRom::client_monitors_config) by QXL_IO_CLIENT_MONITORS_CONFIG_DONE.

I don't think an explicit handshake via
QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea.

How about this update protocol:

qemu:
  (1) set QXLRom::client_monitors_config_updating
  (2) fill QXLRom::client_monitors_config
  (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
  (4) clear QXLRom::client_monitors_config_updating

guest:
  (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
  (2) wait until QXLRom::client_monitors_config_updating is clear
  (3) parse QXLRom::client_monitors_config
  (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
      (a) when set, goto (1).
      (b) when clear we are done.

While thinking about it:  I think we also don't need the guest
capabilities.  With the handshake gone we can update
QXLRom::client_monitors_config unconditionally.

We might want to notify spice-server when the guest flips the
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can
route the event accordingly.  Or we just route it unconditionally both
ways and let the guest sort it (i.e. make vdagent ignore monitors config
when the qxl kms driver is active).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> On 09/11/12 08:56, Alon Levy wrote:
> > Until now we used only the agent to change the monitor count and
> > each
> > monitor resolution. This patch introduces the qemu part of using
> > the
> > device as the mediator instead of the agent via virtio-serial.
> > 
> > Spice (>=0.11.5) calls the new
> > QXLInterface::client_monitors_config,
> > generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which
> > the
> > client indicates handling of (after reading from
> > QXLRom::client_monitors_config) by
> > QXL_IO_CLIENT_MONITORS_CONFIG_DONE.
> 
> I don't think an explicit handshake via
> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea.

Why? I don't see the below as being better - it just moves the checking to the guest, and racily.

> 
> How about this update protocol:
> 
> qemu:
>   (1) set QXLRom::client_monitors_config_updating
>   (2) fill QXLRom::client_monitors_config
>   (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>   (4) clear QXLRom::client_monitors_config_updating
> 
> guest:
>   (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>   (2) wait until QXLRom::client_monitors_config_updating is clear
>   (3) parse QXLRom::client_monitors_config
>   (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>       (a) when set, goto (1).
>       (b) when clear we are done.
> 

Hmm, you are making the guest more complicated instead of the host side, don't know if that's better.

Point (2) is a busy wait, no? Also, guest will have to handle half old / half new configurations:
qemu(1)
qemu(2)
qemu(3)
qemu(4)
qemu(1)
qemu(2) start
guest(1)
guest(2)
guest(3) reads half old half new
qemu(2) complete
qemu(3)
qemu(4)
guest(1)
guest(2)
guest(3) reads full new

Thinking about it some more, I can complete the handling of outstanding client_monitors_config requests by having an additional struct (we don't need to keep the whole bunch, just the current and pending). That would make the logic similar to what you outlined (but reverse - qemu does the waiting and re-interrupt-ing).

> While thinking about it:  I think we also don't need the guest
> capabilities.  With the handshake gone we can update
> QXLRom::client_monitors_config unconditionally.

The capability is there for spice, not for qemu, I used it in qemu since it was already there.

Spice needs it to know if to pass the message (VDAgentMonitorsConfig) to the guest agent or not.

But if you just meant we don't need to check the capability in qemu code, I understand.

> 
> We might want to notify spice-server when the guest flips the
> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can
> route the event accordingly.

That would mean even more complex logic, to delay spice-server from sending the monitors command to the guest while more data could be incoming (which can be multiple chunks comprising one message, so we must wait for it to complete before pushing the VDAgentMonitorsConfig message).

>  Or we just route it unconditionally
> both
> ways and let the guest sort it (i.e. make vdagent ignore monitors
> config
> when the qxl kms driver is active).

Routing it only one way is simpler in spice code. In other words, I had a buggy version doing that and decided that I should just do it one way or the other to not bang my head on it. But it's also simpler to handle - what order are the events going to happen in the guest? Also, not only spice-vdagent plus our driver, but with, for instance, gnome-settings-daemon listening to the uevent from the kernel it will do modesetting by itself, racing with spice-vdagent.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Gerd Hoffmann]

  Hi,

>> I don't think an explicit handshake via
>> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea.
> 
> Why? I don't see the below as being better - it just moves the checking to the guest, and racily.

It is more robust.  We don't have to keep state in qxl for the
handshake, one less opportunity for a buggy guest driver to screw up
things.  It is also closer to how real hardware handles this.

Yes, there is a race, but we detect and handle that case, so it is no
problem.

>>
>> How about this update protocol:
>>
>> qemu:
>>   (1) set QXLRom::client_monitors_config_updating
>>   (2) fill QXLRom::client_monitors_config
>>   (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>>   (4) clear QXLRom::client_monitors_config_updating
>>
>> guest:
>>   (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>>   (2) wait until QXLRom::client_monitors_config_updating is clear
>>   (3) parse QXLRom::client_monitors_config
>>   (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>>       (a) when set, goto (1).
>>       (b) when clear we are done.
>>
> 
> Hmm, you are making the guest more complicated instead of the host
> side, don't know if that's better.
> 
> Point (2) is a busy wait, no?

The guest doesn't have to spin, it can yield().  But yes, better don't
do that in a IRQ handler.

> Also, guest will have to handle half old / half new configurations:

It should not.

> qemu(1)
> qemu(2) start
> guest(1)
> guest(2)
> guest(3) reads half old half new

No, guest will wait at (2).

But I just see it can happen the other way around though: guest starts
reading and qemu starts updating in parallel.  We need one additional
step (3a):  The guest needs to check
QXLRom::client_monitors_config_updating is clear after parsing the data.

When qemu updated the data while the guest was reading it one of the two
conditions will be true: either (3a) if qemu is still busy updating or
(4) if qemu finished updating.

>> We might want to notify spice-server when the guest flips the
>> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can
>> route the event accordingly.
> 
> That would mean even more complex logic, to delay spice-server from
> sending the monitors command to the guest while more data could be
> incoming (which can be multiple chunks comprising one message, so we
> must wait for it to complete before pushing the VDAgentMonitorsConfig
> message).

I don't understand.  Why is the client capability bit fundamentally
different from the irq mask?  I'd expect a guest driver which supports
it would set the irq mask bit and the capability bit at the same time, no?

>> Or we just route it unconditionally both ways and let the guest
>> sort it (i.e. make vdagent ignore monitors config when the qxl kms
>> driver is active).

> Routing it only one way is simpler in spice code. In other words, I
> had a buggy version doing that and decided that I should just do it
> one way or the other to not bang my head on it. But it's also simpler
> to handle - what order are the events going to happen in the guest?
> Also, not only spice-vdagent plus our driver, but with, for instance,
> gnome-settings-daemon listening to the uevent from the kernel it will
> do modesetting by itself, racing with spice-vdagent.

I was thinking about spice-vdagent detecting the qxl kms driver is
active, then just ignore all VDAgentMonitorsConfig messages
unconditionally if this happens to be the case, so there will be no
race.  But if you think it is too complicated, no problem.  Was just an
idea.  Notifying spice-server which way to route is fine with me too.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> Hi,
> 
> >> I don't think an explicit handshake via
> >> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea.
> > 
> > Why? I don't see the below as being better - it just moves the
> > checking to the guest, and racily.
> 
> It is more robust.

I suggested a way for it to be as robust - I take robust to mean "no lost messages except for intermediate ones", which both solutions suffer from anyway.

> We don't have to keep state in qxl for the
> handshake
You mean it's preferable to have state on the device (QXLRom::client_monitors_config_updating) rather then private qxl device state?

> one less opportunity for a buggy guest driver to screw up
> things.
The logic you outline can be screwed up as well, the logic for writing this io is basically:

        qxl_display_copy_rom_client_monitors_config(qdev);
        qxl_crtc_set_from_monitors_config(qdev);
        outb(0, qdev->io_base + QXL_IO_CLIENT_MONITORS_CONFIG_DONE);

> It is also closer to how real hardware handles this.

I don't know how it is in real hardware.

> 
> Yes, there is a race, but we detect and handle that case, so it is no
> problem.
> 
> >>
> >> How about this update protocol:
> >>
> >> qemu:
> >>   (1) set QXLRom::client_monitors_config_updating
> >>   (2) fill QXLRom::client_monitors_config
> >>   (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> >>   (4) clear QXLRom::client_monitors_config_updating
> >>
> >> guest:
> >>   (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
> >>   (2) wait until QXLRom::client_monitors_config_updating is clear
> >>   (3) parse QXLRom::client_monitors_config
> >>   (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
> >>       (a) when set, goto (1).
> >>       (b) when clear we are done.
> >>
> > 
> > Hmm, you are making the guest more complicated instead of the host
> > side, don't know if that's better.
> > 
> > Point (2) is a busy wait, no?
> 
> The guest doesn't have to spin, it can yield().  But yes, better
> don't
> do that in a IRQ handler.
> 
> > Also, guest will have to handle half old / half new configurations:
> 
> It should not.
> 
> > qemu(1)
> > qemu(2) start
> > guest(1)
> > guest(2)
> > guest(3) reads half old half new
> 
> No, guest will wait at (2).
> 
> But I just see it can happen the other way around though: guest
> starts
> reading and qemu starts updating in parallel.  We need one additional
> step (3a):  The guest needs to check
> QXLRom::client_monitors_config_updating is clear after parsing the
> data.
> 
> When qemu updated the data while the guest was reading it one of the
> two
> conditions will be true: either (3a) if qemu is still busy updating
> or
> (4) if qemu finished updating.

Making things more complicated in the host, qemu, means making the kernel driver in the guest simpler, so even though you have a good solution for the race you discovered I don't see why the alternative is worse. (answered point to point above).

> 
> >> We might want to notify spice-server when the guest flips the
> >> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we
> >> can
> >> route the event accordingly.
> > 
> > That would mean even more complex logic, to delay spice-server from
> > sending the monitors command to the guest while more data could be
> > incoming (which can be multiple chunks comprising one message, so
> > we
> > must wait for it to complete before pushing the
> > VDAgentMonitorsConfig
> > message).
> 
> I don't understand.  Why is the client capability bit fundamentally
> different from the irq mask?  I'd expect a guest driver which
> supports
> it would set the irq mask bit and the capability bit at the same
> time, no?

You are right, so we can do away with the guest capabilities field, but remain with the guest capabilities api in spice, because spice depends on it for the routing decision.

> 
> >> Or we just route it unconditionally both ways and let the guest
> >> sort it (i.e. make vdagent ignore monitors config when the qxl kms
> >> driver is active).
> 
> > Routing it only one way is simpler in spice code. In other words, I
> > had a buggy version doing that and decided that I should just do it
> > one way or the other to not bang my head on it. But it's also
> > simpler
> > to handle - what order are the events going to happen in the guest?
> > Also, not only spice-vdagent plus our driver, but with, for
> > instance,
> > gnome-settings-daemon listening to the uevent from the kernel it
> > will
> > do modesetting by itself, racing with spice-vdagent.
> 
> I was thinking about spice-vdagent detecting the qxl kms driver is
> active, then just ignore all VDAgentMonitorsConfig messages
> unconditionally if this happens to be the case, so there will be no
> race. But if you think it is too complicated, no problem. 

This sounds like a good idea, didn't think of it. But that leaves me fixing the vdagent code now :) Also, I guess I still think doing just one message looks simpler.

> Was just an idea.  Notifying spice-server which way to route is fine with me too.

OK, if it's all the same to you I'll stick with spice-server routing the message.

Overall, if you find this tedious I will switch to your suggestion since it isn't such a big deal.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Gerd Hoffmann]

On 09/11/12 13:29, Alon Levy wrote:
>> Hi,
>> 
>>>> I don't think an explicit handshake via 
>>>> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea.
>>> 
>>> Why? I don't see the below as being better - it just moves the 
>>> checking to the guest, and racily.
>> 
>> It is more robust.
> 
> I suggested a way for it to be as robust - I take robust to mean "no
> lost messages except for intermediate ones", which both solutions
> suffer from anyway.
> 
>> We don't have to keep state in qxl for the handshake
> You mean it's preferable to have state on the device
> (QXLRom::client_monitors_config_updating) rather then private qxl
> device state?

The difference is that QXLRom::client_monitors_config_updating is
one-way, it informs the guest what qemu is doing.

qxl->client_monitors_config_isr_in_progress depends on the guest doing
the handshake correctly.

>> It is also closer to how real hardware handles this.
> 
> I don't know how it is in real hardware.

Modern hardware would stuff this into an event queue, simliar to a
virtqueue.  Raise a ring-full IRQ in case it can't.

Two monitor config updates in a row would simply result in two event
queue entries.

Setting up a new spice ring just for that is probably overkill though.
But (maybe you've seen it) for a virtio-based qxl design I would clearly
pass that kind of data as virtqueue payload in a event queue, so a new
update wouldn't overwrite the previous one.

>> When qemu updated the data while the guest was reading it one of
>> the two conditions will be true: either (3a) if qemu is still busy
>> updating or (4) if qemu finished updating.
> 
> Making things more complicated in the host, qemu, means making the
> kernel driver in the guest simpler, so even though you have a good
> solution for the race you discovered I don't see why the alternative
> is worse. (answered point to point above).

Real hardware tends to be the other way around.

>> I was thinking about spice-vdagent detecting the qxl kms driver is 
>> active, then just ignore all VDAgentMonitorsConfig messages 
>> unconditionally if this happens to be the case, so there will be
>> no race. But if you think it is too complicated, no problem.
> 
> This sounds like a good idea, didn't think of it. But that leaves me
> fixing the vdagent code now :) Also, I guess I still think doing just
> one message looks simpler.
> 
>> Was just an idea.  Notifying spice-server which way to route is
>> fine with me too.
> 
> OK, if it's all the same to you I'll stick with spice-server routing
> the message.
> 
> Overall, if you find this tedious I will switch to your suggestion
> since it isn't such a big deal.

I think sending both ways unconditionally could make things easier.

For starters you'll have valid data in QXLRom unconditionally, even in
case the spice client has sent it before the qxl kms driver was loaded
and signaled support (this will only work in case the update is
handshake-free).

You have a better view on the big picture though, how this all interacts
vdagent starting and vdagent capability negotiation.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Hans de Goede]

Hi,

Sorry for top posting, but trying to summarize this thread here.

I must say I like Gerd's approach, as it unifies code paths mostly,
instead of having yet another interface where we do 2 way capabilities
negotiation, with all the extra test matrix entries that would entice
for full testing, we keep things simple.

So we would have:
1) monitor config in rom space
2) QXL_INTERRUPT_CLIENT_MONITORS_CONFIG to tell the guest it is updated
3) Some way to avoid a new monitor config arriving and the guest being
    busy reading the previous race.
4) The server will always update the monitor config in rom space
5) If the guest has not requested QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
    and there is an agent the server will send the monitor info to the
    agent

Note an alternative to the handshake suggested is simply adding a crc
to the monitor config block. If that fails we hit the the (rare) race and
the guest re-reads it.

Regards,

Hans

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> Hi,
> 
> Sorry for top posting, but trying to summarize this thread here.
> 
> I must say I like Gerd's approach, as it unifies code paths mostly,
> instead of having yet another interface where we do 2 way
> capabilities
> negotiation, with all the extra test matrix entries that would entice
> for full testing, we keep things simple.

So you are suggesting to send the message to both parties, and ignore it in the guest agent if it sees a qxl device. That's the only way this works, since otherwise you need a handshake between spice and qxl:

server
(1) receive VDAgentMonitorsConfig config
(2) call qxl->client_monitors_config
(3) wait for qxl->client_monitors_config_not_acked <-- after timeout? when do we decide interrupt wasn't cleared due to guest not supporting it, or due to not enough time having passed?
(4a) if timeout, send VDAgentMonitorsConfig to agent
(4b) else done

> 
> So we would have:
> 1) monitor config in rom space
> 2) QXL_INTERRUPT_CLIENT_MONITORS_CONFIG to tell the guest it is
> updated
> 3) Some way to avoid a new monitor config arriving and the guest
> being
>     busy reading the previous race.
> 4) The server will always update the monitor config in rom space
> 5) If the guest has not requested
> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>     and there is an agent the server will send the monitor info to
>     the
>     agent
> 
> Note an alternative to the handshake suggested is simply adding a crc
> to the monitor config block. If that fails we hit the the (rare) race
> and
> the guest re-reads it.
> 
> Regards,
> 
> Hans
> 
> 

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> > Hi,
> > 
> > Sorry for top posting, but trying to summarize this thread here.
> > 
> > I must say I like Gerd's approach, as it unifies code paths mostly,
> > instead of having yet another interface where we do 2 way
> > capabilities
> > negotiation, with all the extra test matrix entries that would
> > entice
> > for full testing, we keep things simple.
> 
> So you are suggesting to send the message to both parties, and ignore
> it in the guest agent if it sees a qxl device.
s/device/drm device file/ - this is linux specific right now, for windows guests we would check for something else presumably (not interesting right now).

> That's the only way
> this works, since otherwise you need a handshake between spice and
> qxl:
> 
> server
> (1) receive VDAgentMonitorsConfig config
> (2) call qxl->client_monitors_config
> (3) wait for qxl->client_monitors_config_not_acked <-- after timeout?
> when do we decide interrupt wasn't cleared due to guest not
> supporting it, or due to not enough time having passed?
> (4a) if timeout, send VDAgentMonitorsConfig to agent
> (4b) else done
> 
> > 
> > So we would have:
> > 1) monitor config in rom space
> > 2) QXL_INTERRUPT_CLIENT_MONITORS_CONFIG to tell the guest it is
> > updated
> > 3) Some way to avoid a new monitor config arriving and the guest
> > being
> >     busy reading the previous race.
> > 4) The server will always update the monitor config in rom space
> > 5) If the guest has not requested
> > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> >     and there is an agent the server will send the monitor info to
> >     the
> >     agent
> > 
> > Note an alternative to the handshake suggested is simply adding a
> > crc
> > to the monitor config block. If that fails we hit the the (rare)
> > race
> > and
> > the guest re-reads it.
> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Hans de Goede]

Hi,

On 09/11/2012 02:03 PM, Alon Levy wrote:
>> Hi,
>>
>> Sorry for top posting, but trying to summarize this thread here.
>>
>> I must say I like Gerd's approach, as it unifies code paths mostly,
>> instead of having yet another interface where we do 2 way
>> capabilities
>> negotiation, with all the extra test matrix entries that would entice
>> for full testing, we keep things simple.
>
> So you are suggesting to send the message to both parties, and ignore it in the guest agent if it sees a qxl device.

No, slightly more sophisticated, what I'm suggesting is that the server
will not forward the VDAgentMonitorsConfig to the agent, if the qxl-dev's
int_mask contains QXL_INTERRUPT_CLIENT_MONITORS_CONFIG.

This should work because either we have a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
aware driver, ie the new kms driver and it sets QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
in int_mask, or we don't and then the right thing to do is send the message to
the agent to deal with it.

This will need spice-server <-> qemu/hw/qxl.c coordination, but I assume we
will need some changes there anyway to set QXL_INTERRUPT_CLIENT_MONITORS_CONFIG,
the qemu/hw/qxl.c function patching the MonitorsConfig into the romspace,
and setting the irq in int_pending, could return whether or not the flag was
set in int_mask for example.

Regards,

Hans

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Gerd Hoffmann]

  Hi,

> This will need spice-server <-> qemu/hw/qxl.c coordination, but I assume we
> will need some changes there anyway to set
> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG,
> the qemu/hw/qxl.c function patching the MonitorsConfig into the romspace,
> and setting the irq in int_pending, could return whether or not the flag
> was
> set in int_mask for example.

Yes, interface_client_monitors_config() can return whenever the guest
has masked the interrupt or not, and based on that spice-server can send
the message to the agent or not.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> Hi,
> 
> > This will need spice-server <-> qemu/hw/qxl.c coordination, but I
> > assume we
> > will need some changes there anyway to set
> > QXL_INTERRUPT_CLIENT_MONITORS_CONFIG,
> > the qemu/hw/qxl.c function patching the MonitorsConfig into the
> > romspace,
> > and setting the irq in int_pending, could return whether or not the
> > flag
> > was
> > set in int_mask for example.
> 
> Yes, interface_client_monitors_config() can return whenever the guest
> has masked the interrupt or not, and based on that spice-server can
> send
> the message to the agent or not.

ok, I'm missing something here. (and trying to catch up via Vol 3A is taking too long).
I thought the order is:
(1) qemu raises interrupt
(2) qemu calls kvm ioctl
(3) guest interrupt handler
(4) guest clears interrupt by writing ~0 to qxl ram_header->int_mask.
(5) qemu detects this next time it raises interrupt.

so where does qemu/hw/qxl.c get a chance to see this masking *immediately* after it raises the interrupt, i.e. before (2) above, since otherwise there is a timeout here, you need to add a callback, it gets complicated, and then the unconditional two way sending looks much better. (I'm already on the same page with you on not needing guest capabilities at this point, even though for the future it did look like a good thing to have).

> 
> cheers,
>   Gerd
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Gerd Hoffmann]

> ok, I'm missing something here. (and trying to catch up via Vol 3A is taking too long).
> I thought the order is:
> (1) qemu raises interrupt
> (2) qemu calls kvm ioctl
> (3) guest interrupt handler
> (4) guest clears interrupt by writing ~0 to qxl ram_header->int_mask.
> (5) qemu detects this next time it raises interrupt.

> so where does qemu/hw/qxl.c get a chance to see this masking
> *immediately* after it raises the interrupt, i.e. before (2) above,
> since otherwise there is a timeout here, you need to add a callback,
> it gets complicated, and then the unconditional two way sending looks
> much better. (I'm already on the same page with you on not needing
> guest capabilities at this point, even though for the future it did
> look like a good thing to have).

There are two registers:

  (1) the interrupt enable register (aka ram->int_mask)
  (2) the interrupt status register (aka ram->int_pending)

qemu sets the irq bit in the status register each time the irq condition
is meet.  qemu actually raises an irq in case the guest has the irq bit
set in the enable register.  guest acks the irq by clearing the irq bit
in the status register (then issue QXL_IO_UPDATE_IRQ to notify qemu that
it touched interrupt registers, which we need because our registers in
memory not mmio space).

So qxl can simply look at the enable register bit to figure whenever the
guest is interested in specific interrupts or not.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> > ok, I'm missing something here. (and trying to catch up via Vol 3A
> > is taking too long).
> > I thought the order is:
> > (1) qemu raises interrupt
> > (2) qemu calls kvm ioctl
> > (3) guest interrupt handler
> > (4) guest clears interrupt by writing ~0 to qxl
> > ram_header->int_mask.
> > (5) qemu detects this next time it raises interrupt.
> 
> > so where does qemu/hw/qxl.c get a chance to see this masking
> > *immediately* after it raises the interrupt, i.e. before (2) above,
> > since otherwise there is a timeout here, you need to add a
> > callback,
> > it gets complicated, and then the unconditional two way sending
> > looks
> > much better. (I'm already on the same page with you on not needing
> > guest capabilities at this point, even though for the future it did
> > look like a good thing to have).
> 
> There are two registers:
> 
>   (1) the interrupt enable register (aka ram->int_mask)
>   (2) the interrupt status register (aka ram->int_pending)
> 
> qemu sets the irq bit in the status register each time the irq
> condition
> is meet.  qemu actually raises an irq in case the guest has the irq
> bit
> set in the enable register.  guest acks the irq by clearing the irq
> bit
> in the status register (then issue QXL_IO_UPDATE_IRQ to notify qemu
> that
> it touched interrupt registers, which we need because our registers
> in
> memory not mmio space).
> 
> So qxl can simply look at the enable register bit to figure whenever
> the
> guest is interested in specific interrupts or not.

Hans and myself discussed offline the current windows driver implementation. In short, it sets ram->int_mask to ~0, thereby claiming to support all 32 interrupts (including those we haven't thought of yet..).

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Hans de Goede]

Hi,

On 09/11/2012 03:05 PM, Alon Levy wrote:
>>> ok, I'm missing something here. (and trying to catch up via Vol 3A
>>> is taking too long).
>>> I thought the order is:
>>> (1) qemu raises interrupt
>>> (2) qemu calls kvm ioctl
>>> (3) guest interrupt handler
>>> (4) guest clears interrupt by writing ~0 to qxl
>>> ram_header->int_mask.
>>> (5) qemu detects this next time it raises interrupt.
>>
>>> so where does qemu/hw/qxl.c get a chance to see this masking
>>> *immediately* after it raises the interrupt, i.e. before (2) above,
>>> since otherwise there is a timeout here, you need to add a
>>> callback,
>>> it gets complicated, and then the unconditional two way sending
>>> looks
>>> much better. (I'm already on the same page with you on not needing
>>> guest capabilities at this point, even though for the future it did
>>> look like a good thing to have).
>>
>> There are two registers:
>>
>>    (1) the interrupt enable register (aka ram->int_mask)
>>    (2) the interrupt status register (aka ram->int_pending)
>>
>> qemu sets the irq bit in the status register each time the irq
>> condition
>> is meet.  qemu actually raises an irq in case the guest has the irq
>> bit
>> set in the enable register.  guest acks the irq by clearing the irq
>> bit
>> in the status register (then issue QXL_IO_UPDATE_IRQ to notify qemu
>> that
>> it touched interrupt registers, which we need because our registers
>> in
>> memory not mmio space).
>>
>> So qxl can simply look at the enable register bit to figure whenever
>> the
>> guest is interested in specific interrupts or not.
>
> Hans and myself discussed offline the current windows driver implementation. In short, it sets ram->int_mask to ~0, thereby claiming to support all 32 interrupts (including those we haven't thought of yet..).

Right, thinking more about this, this means that the don't send it to
the agent when QXL_INTERRUPT_CLIENT_MONITORS_CONFIG is set in mask trick
won't work, for windows with an older driver.

I suggest rather then doing the whole capabilities dance, we simply detect
the (older) windows driver (mask == ~0), and then treat that as
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG not being set in mask, a bit of hack
but still much simpler then adding a full capabilities interface.

If windows ever wants to actually support CLIENT_MONITORS_CONFIG through
the driver rather then trough the agent, the driver will need updating
anyways and we can then drop the ~0 replacing it with the proper mask.

Regards,

Hans

Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

> Hi,
> 
> On 09/11/2012 03:05 PM, Alon Levy wrote:
> >>> ok, I'm missing something here. (and trying to catch up via Vol
> >>> 3A
> >>> is taking too long).
> >>> I thought the order is:
> >>> (1) qemu raises interrupt
> >>> (2) qemu calls kvm ioctl
> >>> (3) guest interrupt handler
> >>> (4) guest clears interrupt by writing ~0 to qxl
> >>> ram_header->int_mask.
> >>> (5) qemu detects this next time it raises interrupt.
> >>
> >>> so where does qemu/hw/qxl.c get a chance to see this masking
> >>> *immediately* after it raises the interrupt, i.e. before (2)
> >>> above,
> >>> since otherwise there is a timeout here, you need to add a
> >>> callback,
> >>> it gets complicated, and then the unconditional two way sending
> >>> looks
> >>> much better. (I'm already on the same page with you on not
> >>> needing
> >>> guest capabilities at this point, even though for the future it
> >>> did
> >>> look like a good thing to have).
> >>
> >> There are two registers:
> >>
> >>    (1) the interrupt enable register (aka ram->int_mask)
> >>    (2) the interrupt status register (aka ram->int_pending)
> >>
> >> qemu sets the irq bit in the status register each time the irq
> >> condition
> >> is meet.  qemu actually raises an irq in case the guest has the
> >> irq
> >> bit
> >> set in the enable register.  guest acks the irq by clearing the
> >> irq
> >> bit
> >> in the status register (then issue QXL_IO_UPDATE_IRQ to notify
> >> qemu
> >> that
> >> it touched interrupt registers, which we need because our
> >> registers
> >> in
> >> memory not mmio space).
> >>
> >> So qxl can simply look at the enable register bit to figure
> >> whenever
> >> the
> >> guest is interested in specific interrupts or not.
> >
> > Hans and myself discussed offline the current windows driver
> > implementation. In short, it sets ram->int_mask to ~0, thereby
> > claiming to support all 32 interrupts (including those we haven't
> > thought of yet..).
> 
> Right, thinking more about this, this means that the don't send it to
> the agent when QXL_INTERRUPT_CLIENT_MONITORS_CONFIG is set in mask
> trick
> won't work, for windows with an older driver.
> 
> I suggest rather then doing the whole capabilities dance, we simply
> detect
> the (older) windows driver (mask == ~0), and then treat that as
> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG not being set in mask, a bit of
> hack
> but still much simpler then adding a full capabilities interface.
> 
> If windows ever wants to actually support CLIENT_MONITORS_CONFIG
> through
> the driver rather then trough the agent, the driver will need
> updating
> anyways and we can then drop the ~0 replacing it with the proper
> mask.

That sounds good, so I'll make sure the kms driver doesn't have this bug.

Updated patches for spice-protocol, spice & qemu coming up.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> 

[Qemu-devel] [PATCH 0/3] client monitors config support

[Alon Levy]

v3:
 - no addition of guest capabilities, use interrupt mask instead, ignore
   0 or ~0 that are set by current windows driver.
 - use crc to solve possible write while read.
 - limit heads to 64, statically allocated on rom by host.
 - some misc trace fixes.

QEMU:

Alon Levy (3):
  hw/qxl: tracing fixes
  qxl: add trace-event for QXL_IO_LOG
  hw/qxl: support client monitor configuration via device

 configure    |  7 +++++
 hw/qxl.c     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 trace-events | 11 ++++++--
 3 files changed, 101 insertions(+), 5 deletions(-)

spice-protocol:

Alon Levy (2):
  qxl_dev.h: add client monitors configuration notification to guest
  Release 0.12.2

 configure.ac    |  2 +-
 spice/qxl_dev.h | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

spice-common:

Alon Levy (1):
  Update spice-protocol module

 spice-protocol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

spice:

Alon Levy (7):
  server/red_dispatcher: change a printerr to debug
  update spice-common module
  server: add QXLInterface::client_monitors_config
  server/red_dispatcher: client_monitors_config support
  server: Filter VD_AGENT_MONITORS_CONFIG
  server/tests: agent mock, client_monitors_config
  spice-server 0.11.5

 configure.ac                       |  6 ++--
 server/agent-msg-filter.c          |  8 +++++
 server/agent-msg-filter.h          |  1 +
 server/red_dispatcher.c            | 51 +++++++++++++++++++++++++++++-
 server/red_dispatcher.h            |  4 +++
 server/reds.c                      | 65 ++++++++++++++++++++++++++++++++++++--
 server/spice.h                     | 14 +++++---
 server/tests/basic_event_loop.c    |  2 +-
 server/tests/test_display_base.c   | 46 +++++++++++++++++++++++++++
 server/tests/test_display_base.h   |  1 +
 server/tests/test_display_no_ssl.c |  1 +
 spice-common                       |  2 +-
 12 files changed, 189 insertions(+), 12 deletions(-)



-- 
1.7.12

[Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy]

Until now we used only the agent to change the monitor count and each
monitor resolution. This patch introduces the qemu part of using the
device as the mediator instead of the agent via virtio-serial.

Spice (>=0.11.5) calls the new QXLInterface::client_monitors_config,
which returns wether the interrupt is enabled, and if so and given a non
NULL monitors config will
generate an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG with crc
checksum for the guest to verify a second call hasn't interfered.

The maximal number of monitors is limited on the QXLRom to 64.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 configure    |  7 ++++++
 hw/qxl.c     | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 trace-events |  6 ++++-
 3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 30be784..35b5f1b 100755
--- a/configure
+++ b/configure
@@ -2709,6 +2709,9 @@ EOF
     if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
         spice_qxl_io_monitors_config_async="yes"
     fi
+    if $pkg_config --atleast-version=0.12.2 spice-protocol > /dev/null 2>&1; then
+        spice_qxl_client_monitors_config="yes"
+    fi
   else
     if test "$spice" = "yes" ; then
       feature_not_found "spice"
@@ -3456,6 +3459,10 @@ if test "$spice_qxl_io_monitors_config_async" = "yes" ; then
   echo "CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC=y" >> $config_host_mak
 fi
 
+if test "$spice_qxl_client_monitors_config" = "yes" ; then
+  echo "CONFIG_QXL_CLIENT_MONITORS_CONFIG=y" >> $config_host_mak
+fi
+
 if test "$smartcard" = "yes" ; then
   echo "CONFIG_SMARTCARD=y" >> $config_host_mak
 fi
diff --git a/hw/qxl.c b/hw/qxl.c
index 12dfc79..3a0c059 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -18,6 +18,8 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <zlib.h>
+
 #include "qemu-common.h"
 #include "qemu-timer.h"
 #include "qemu-queue.h"
@@ -966,6 +968,79 @@ static void interface_set_client_capabilities(QXLInstance *sin,
 
 #endif
 
+#if defined(CONFIG_QXL_CLIENT_MONITORS_CONFIG) \
+    && SPICE_SERVER_VERSION >= 0x000b05
+
+static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
+{
+    /*
+     * zlib xors the seed with 0xffffffff, and xors the result
+     * again with 0xffffffff; Both are not done with linux's crc32,
+     * which we want to be compatible with, so undo that.
+     */
+    return crc32(0xffffffff, p, len) ^ 0xffffffff;
+}
+
+/* called from main context only */
+static int interface_client_monitors_config(QXLInstance *sin,
+                                        VDAgentMonitorsConfig *monitors_config)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
+    int i;
+
+    /*
+     * Older windows drivers set int_mask to 0 when their ISR is called,
+     * then later set it to ~0. So it doesn't relate to the actual interrupts
+     * handled. However, they are old, so clearly they don't support this
+     * interrupt
+     */
+    if (qxl->ram->int_mask == 0 || qxl->ram->int_mask == ~0 ||
+        !(qxl->ram->int_mask & QXL_INTERRUPT_CLIENT_MONITORS_CONFIG)) {
+        trace_qxl_client_monitors_config_unsupported_by_guest(qxl->id,
+                                                            qxl->ram->int_mask,
+                                                            monitors_config);
+        return 0;
+    }
+    if (!monitors_config) {
+        return 1;
+    }
+    memset(&rom->client_monitors_config, 0,
+           sizeof(rom->client_monitors_config));
+    rom->client_monitors_config.count = monitors_config->num_of_monitors;
+    /* monitors_config->flags ignored */
+    if (rom->client_monitors_config.count >=
+            ARRAY_SIZE(rom->client_monitors_config.heads)) {
+        trace_qxl_client_monitors_config_capped(qxl->id,
+                                monitors_config->num_of_monitors,
+                                ARRAY_SIZE(rom->client_monitors_config.heads));
+        rom->client_monitors_config.count =
+            ARRAY_SIZE(rom->client_monitors_config.heads);
+    }
+    for (i = 0 ; i < rom->client_monitors_config.count ; ++i) {
+        VDAgentMonConfig *monitor = &monitors_config->monitors[i];
+        QXLURect *rect = &rom->client_monitors_config.heads[i];
+        /* monitor->depth ignored */
+        rect->left = monitor->x;
+        rect->top = monitor->y;
+        rect->right = monitor->x + monitor->width;
+        rect->bottom = monitor->y + monitor->height;
+    }
+    rom->client_monitors_config_crc = qxl_crc32(
+            (const uint8_t *)&rom->client_monitors_config,
+            sizeof(rom->client_monitors_config));
+    trace_qxl_client_monitors_config_crc(qxl->id,
+            sizeof(rom->client_monitors_config),
+            rom->client_monitors_config_crc);
+
+    trace_qxl_interrupt_client_monitors_config(qxl->id,
+                        rom->client_monitors_config.count,
+                        rom->client_monitors_config.heads);
+    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+    return 1;
+}
+#endif
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -990,6 +1065,10 @@ static const QXLInterface qxl_interface = {
 #if SPICE_SERVER_VERSION >= 0x000b04
     .set_client_capabilities = interface_set_client_capabilities,
 #endif
+#if SPICE_SERVER_VERSION >= 0x000b05 && \
+    defined(CONFIG_QXL_CLIENT_MONITORS_CONFIG)
+    .client_monitors_config = interface_client_monitors_config,
+#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
diff --git a/trace-events b/trace-events
index 564b773..93691f8 100644
--- a/trace-events
+++ b/trace-events
@@ -924,7 +924,7 @@ qxl_interface_update_area_complete_rest(int qid, uint32_t num_updated_rects) "%d
 qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
 qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty) "%d #dirty=%d"
 qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
-qxl_io_log(int qid, const uint8_t *str) "%d %s"
+qxl_io_log(int qid, const uint8_t *log_buf) "%d %s"
 qxl_io_read_unexpected(int qid) "%d"
 qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
 qxl_io_write(int qid, const char *mode, uint64_t addr, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " val=%"PRIu64" size=%u async=%d"
@@ -968,6 +968,10 @@ qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dir
 qxl_surfaces_dirty(int qid, int surface, int offset, int size) "%d surface=%d offset=%d size=%d"
 qxl_send_events(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
+qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
+qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"
+qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
+qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.7.12