Patchwork [v7,1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC

login
register
mail settings
Submitter Alon Levy
Date Aug. 17, 2012, 3:39 p.m.
Message ID <1345217997-21965-1-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/178278/
State New
Headers show

Comments

Alon Levy - Aug. 17, 2012, 3:39 p.m.
Revision bumped to 4 for new IO support, enabled for spice-server >=
0.11.1. New io enabled iff spice-server >= 0.11.1 && spice-protocol >=
0.12.0.

On migration reissue spice_qxl_monitors_config_async.

RHBZ: 770842

Signed-off-by: Alon Levy <alevy@redhat.com>
---
Fixed another defined I missed. This time used grep to verify it's the last
one..

Alon

 configure          |  3 +++
 hw/qxl.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/qxl.h           |  7 +++++++
 trace-events       |  1 +
 ui/spice-display.h |  1 +
 5 files changed, 70 insertions(+), 2 deletions(-)
Blue Swirl - Aug. 18, 2012, 2:31 p.m.
On Fri, Aug 17, 2012 at 3:39 PM, Alon Levy <alevy@redhat.com> wrote:
> Revision bumped to 4 for new IO support, enabled for spice-server >=
> 0.11.1. New io enabled iff spice-server >= 0.11.1 && spice-protocol >=
> 0.12.0.
>
> On migration reissue spice_qxl_monitors_config_async.
>
> RHBZ: 770842
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> Fixed another defined I missed. This time used grep to verify it's the last
> one..
>
> Alon
>
>  configure          |  3 +++
>  hw/qxl.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/qxl.h           |  7 +++++++
>  trace-events       |  1 +
>  ui/spice-display.h |  1 +
>  5 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index cc774b5..dbf3af6 100755
> --- a/configure
> +++ b/configure
> @@ -2657,6 +2657,9 @@ EOF
>      spice="yes"
>      libs_softmmu="$libs_softmmu $spice_libs"
>      QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
> +    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
> +        QEMU_CFLAGS="$QEMU_CFLAGS -DQXL_HAS_IO_MONITORS_CONFIG_ASYNC=1"

Please put this to config-host.h instead of -D. Then change detection
for config-host.h and dependencies cause a rebuild.

> +    fi
>    else
>      if test "$spice" = "yes" ; then
>        feature_not_found "spice"
> diff --git a/hw/qxl.c b/hw/qxl.c
> index c978f5e..25d7759 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -27,6 +27,10 @@
>
>  #include "qxl.h"
>
> +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0

Just delete this and use defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).

> +#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
> @@ -249,6 +253,20 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
>      }
>  }
>
> +/* 0x00b01 == 0.11.1 */
> +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC

Like here.

> +static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl)
> +{
> +    trace_qxl_spice_monitors_config(qxl->id);
> +    qxl->guest_monitors_config = qxl->ram->monitors_config;
> +    spice_qxl_monitors_config_async(&qxl->ssd.qxl,
> +            qxl->ram->monitors_config,
> +            MEMSLOT_GROUP_GUEST,
> +            (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
> +                                      QXL_IO_MONITORS_CONFIG_ASYNC));
> +}
> +#endif
> +
>  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
>  {
>      trace_qxl_spice_reset_image_cache(qxl->id);
> @@ -538,6 +556,9 @@ static const char *io_port_to_string(uint32_t io_port)
>                                          = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
>          [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
>          [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
> +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> +        [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
> +#endif
>      };
>      return io_port_to_string[io_port];
>  }
> @@ -819,7 +840,10 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
>      case QXL_IO_DESTROY_PRIMARY_ASYNC:
>      case QXL_IO_UPDATE_AREA_ASYNC:
>      case QXL_IO_FLUSH_SURFACES_ASYNC:
> +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> +    case QXL_IO_MONITORS_CONFIG_ASYNC:
>          break;
> +#endif
>      case QXL_IO_CREATE_PRIMARY_ASYNC:
>          qxl_create_guest_primary_complete(qxl);
>          break;
> @@ -894,6 +918,8 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
>      case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA:
>          qxl_render_update_area_done(qxl, cookie);
>          break;
> +    case QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG:
> +        break;
>      default:
>          fprintf(stderr, "qxl: %s: unexpected cookie type %d\n",
>                  __func__, cookie->type);
> @@ -1333,7 +1359,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
>              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) {
> +            io_port < QXL_IO_RANGE_SIZE) {
>              qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
>          }
>          return;
> @@ -1361,6 +1387,10 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
>          io_port = QXL_IO_DESTROY_ALL_SURFACES;
>          goto async_common;
>      case QXL_IO_FLUSH_SURFACES_ASYNC:
> +/* 0x000b01 == 0.11.1 */
> +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> +#endif
>  async_common:
>          async = QXL_ASYNC;
>          qemu_mutex_lock(&d->async_lock);
> @@ -1502,6 +1532,12 @@ async_common:
>          d->mode = QXL_MODE_UNDEFINED;
>          qxl_spice_destroy_surfaces(d, async);
>          break;
> +/* 0x000b01 == 0.11.1 */
> +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> +        qxl_spice_monitors_config_async(d);
> +        break;
> +#endif
>      default:
>          qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
>      }
> @@ -1797,9 +1833,15 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>          io_size = 16;
>          break;
>      case 3: /* qxl-3 */
> +        pci_device_rev = QXL_REVISION_STABLE_V10;
> +        io_size = 32; /* PCI region size must be pow2 */
> +        break;
> +#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
> +    case 4: /* qxl-4 */
>          pci_device_rev = QXL_DEFAULT_REVISION;
>          io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
>          break;
> +#endif
>      default:
>          error_report("Invalid revision %d for qxl device (max %d)",
>                       qxl->revision, QXL_DEFAULT_REVISION);
> @@ -1998,7 +2040,20 @@ static int qxl_post_load(void *opaque, int version)
>          }
>          qxl_spice_loadvm_commands(d, cmds, out);
>          g_free(cmds);
> -
> +        if (d->guest_monitors_config) {
> +            /*
> +             * don't use QXL_COOKIE_TYPE_IO:
> +             *  - we are not running yet (post_load), we will assert
> +             *    in send_events
> +             *  - this is not a guest io, but a reply, so async_io isn't set.
> +             */
> +            spice_qxl_monitors_config_async(&d->ssd.qxl,
> +                    d->guest_monitors_config,
> +                    MEMSLOT_GROUP_GUEST,
> +                    (uintptr_t)qxl_cookie_new(
> +                        QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
> +                        0));
> +        }
>          break;
>      case QXL_MODE_COMPAT:
>          /* note: no need to call qxl_create_memslots, qxl_set_mode
> @@ -2065,6 +2120,7 @@ static VMStateDescription qxl_vmstate = {
>          VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
>                        vmstate_info_uint64, uint64_t),
>          VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
> +        VMSTATE_UINT64(guest_monitors_config, PCIQXLDevice),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 172baf6..0c5ebbd 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -71,6 +71,8 @@ typedef struct PCIQXLDevice {
>      } guest_surfaces;
>      QXLPHYSICAL        guest_cursor;
>
> +    QXLPHYSICAL        guest_monitors_config;
> +
>      QemuMutex          track_lock;
>
>      /* thread signaling */
> @@ -128,7 +130,12 @@ typedef struct PCIQXLDevice {
>          }                                                               \
>      } while (0)
>
> +/* 0x000b01 == 0.11.1 */
> +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
> +#else
>  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> +#endif

Alternatively, put the 0/1 logic here:
#ifndef CONFIG_QXL_ASYNC
#define QXL_ASYNC 0
#else
#define QXL_ASYNC 1
#endif

Even better, the value originally came from pkg-config check for
version. Isn't there a spice header which could provide the same
version information? Then the changes in configure would not be
needed.

>
>  /* qxl.c */
>  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
> diff --git a/trace-events b/trace-events
> index 04b0723..8fcbc50 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -956,6 +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_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"
> diff --git a/ui/spice-display.h b/ui/spice-display.h
> index 12e50b6..7fa095f 100644
> --- a/ui/spice-display.h
> +++ b/ui/spice-display.h
> @@ -51,6 +51,7 @@ typedef enum qxl_async_io {
>  enum {
>      QXL_COOKIE_TYPE_IO,
>      QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
> +    QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
>  };
>
>  typedef struct QXLCookie {
> --
> 1.7.11.2
>
>
Alon Levy - Aug. 18, 2012, 4:16 p.m.
On Sat, Aug 18, 2012 at 02:31:32PM +0000, Blue Swirl wrote:
> On Fri, Aug 17, 2012 at 3:39 PM, Alon Levy <alevy@redhat.com> wrote:
> > Revision bumped to 4 for new IO support, enabled for spice-server >=
> > 0.11.1. New io enabled iff spice-server >= 0.11.1 && spice-protocol >=
> > 0.12.0.
> >
> > On migration reissue spice_qxl_monitors_config_async.
> >
> > RHBZ: 770842
> >
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> > Fixed another defined I missed. This time used grep to verify it's the last
> > one..
> >
> > Alon
> >
> >  configure          |  3 +++
> >  hw/qxl.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  hw/qxl.h           |  7 +++++++
> >  trace-events       |  1 +
> >  ui/spice-display.h |  1 +
> >  5 files changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/configure b/configure
> > index cc774b5..dbf3af6 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2657,6 +2657,9 @@ EOF
> >      spice="yes"
> >      libs_softmmu="$libs_softmmu $spice_libs"
> >      QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
> > +    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
> > +        QEMU_CFLAGS="$QEMU_CFLAGS -DQXL_HAS_IO_MONITORS_CONFIG_ASYNC=1"
> 
> Please put this to config-host.h instead of -D. Then change detection
> for config-host.h and dependencies cause a rebuild.
ok.

> 
> > +    fi
> >    else
> >      if test "$spice" = "yes" ; then
> >        feature_not_found "spice"
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index c978f5e..25d7759 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -27,6 +27,10 @@
> >
> >  #include "qxl.h"
> >
> > +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
> 
> Just delete this and use defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).

So you are telling me to undo a change that Gerd asked for - could you
please at least debate about the merits of both approaches? the point of
having QXL_HAS_IO_MONITORS_CONFIG_ASYNC always defined was to allow
usage of #if without defined, which is shorter.

> 
> > +#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
> > @@ -249,6 +253,20 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
> >      }
> >  }
> >
> > +/* 0x00b01 == 0.11.1 */
> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> 
> Like here.
> 
> > +static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl)
> > +{
> > +    trace_qxl_spice_monitors_config(qxl->id);
> > +    qxl->guest_monitors_config = qxl->ram->monitors_config;
> > +    spice_qxl_monitors_config_async(&qxl->ssd.qxl,
> > +            qxl->ram->monitors_config,
> > +            MEMSLOT_GROUP_GUEST,
> > +            (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
> > +                                      QXL_IO_MONITORS_CONFIG_ASYNC));
> > +}
> > +#endif
> > +
> >  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
> >  {
> >      trace_qxl_spice_reset_image_cache(qxl->id);
> > @@ -538,6 +556,9 @@ static const char *io_port_to_string(uint32_t io_port)
> >                                          = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
> >          [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
> >          [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
> > +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > +        [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
> > +#endif
> >      };
> >      return io_port_to_string[io_port];
> >  }
> > @@ -819,7 +840,10 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
> >      case QXL_IO_DESTROY_PRIMARY_ASYNC:
> >      case QXL_IO_UPDATE_AREA_ASYNC:
> >      case QXL_IO_FLUSH_SURFACES_ASYNC:
> > +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> >          break;
> > +#endif
> >      case QXL_IO_CREATE_PRIMARY_ASYNC:
> >          qxl_create_guest_primary_complete(qxl);
> >          break;
> > @@ -894,6 +918,8 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
> >      case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA:
> >          qxl_render_update_area_done(qxl, cookie);
> >          break;
> > +    case QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG:
> > +        break;
> >      default:
> >          fprintf(stderr, "qxl: %s: unexpected cookie type %d\n",
> >                  __func__, cookie->type);
> > @@ -1333,7 +1359,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
> >              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) {
> > +            io_port < QXL_IO_RANGE_SIZE) {
> >              qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
> >          }
> >          return;
> > @@ -1361,6 +1387,10 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
> >          io_port = QXL_IO_DESTROY_ALL_SURFACES;
> >          goto async_common;
> >      case QXL_IO_FLUSH_SURFACES_ASYNC:
> > +/* 0x000b01 == 0.11.1 */
> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> > +#endif
> >  async_common:
> >          async = QXL_ASYNC;
> >          qemu_mutex_lock(&d->async_lock);
> > @@ -1502,6 +1532,12 @@ async_common:
> >          d->mode = QXL_MODE_UNDEFINED;
> >          qxl_spice_destroy_surfaces(d, async);
> >          break;
> > +/* 0x000b01 == 0.11.1 */
> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> > +        qxl_spice_monitors_config_async(d);
> > +        break;
> > +#endif
> >      default:
> >          qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
> >      }
> > @@ -1797,9 +1833,15 @@ static int qxl_init_common(PCIQXLDevice *qxl)
> >          io_size = 16;
> >          break;
> >      case 3: /* qxl-3 */
> > +        pci_device_rev = QXL_REVISION_STABLE_V10;
> > +        io_size = 32; /* PCI region size must be pow2 */
> > +        break;
> > +#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
> > +    case 4: /* qxl-4 */
> >          pci_device_rev = QXL_DEFAULT_REVISION;
> >          io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> >          break;
> > +#endif
> >      default:
> >          error_report("Invalid revision %d for qxl device (max %d)",
> >                       qxl->revision, QXL_DEFAULT_REVISION);
> > @@ -1998,7 +2040,20 @@ static int qxl_post_load(void *opaque, int version)
> >          }
> >          qxl_spice_loadvm_commands(d, cmds, out);
> >          g_free(cmds);
> > -
> > +        if (d->guest_monitors_config) {
> > +            /*
> > +             * don't use QXL_COOKIE_TYPE_IO:
> > +             *  - we are not running yet (post_load), we will assert
> > +             *    in send_events
> > +             *  - this is not a guest io, but a reply, so async_io isn't set.
> > +             */
> > +            spice_qxl_monitors_config_async(&d->ssd.qxl,
> > +                    d->guest_monitors_config,
> > +                    MEMSLOT_GROUP_GUEST,
> > +                    (uintptr_t)qxl_cookie_new(
> > +                        QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
> > +                        0));
> > +        }
> >          break;
> >      case QXL_MODE_COMPAT:
> >          /* note: no need to call qxl_create_memslots, qxl_set_mode
> > @@ -2065,6 +2120,7 @@ static VMStateDescription qxl_vmstate = {
> >          VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
> >                        vmstate_info_uint64, uint64_t),
> >          VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
> > +        VMSTATE_UINT64(guest_monitors_config, PCIQXLDevice),
> >          VMSTATE_END_OF_LIST()
> >      },
> >  };
> > diff --git a/hw/qxl.h b/hw/qxl.h
> > index 172baf6..0c5ebbd 100644
> > --- a/hw/qxl.h
> > +++ b/hw/qxl.h
> > @@ -71,6 +71,8 @@ typedef struct PCIQXLDevice {
> >      } guest_surfaces;
> >      QXLPHYSICAL        guest_cursor;
> >
> > +    QXLPHYSICAL        guest_monitors_config;
> > +
> >      QemuMutex          track_lock;
> >
> >      /* thread signaling */
> > @@ -128,7 +130,12 @@ typedef struct PCIQXLDevice {
> >          }                                                               \
> >      } while (0)
> >
> > +/* 0x000b01 == 0.11.1 */
> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
> > +#else
> >  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> > +#endif
> 
> Alternatively, put the 0/1 logic here:
> #ifndef CONFIG_QXL_ASYNC
> #define QXL_ASYNC 0
> #else
> #define QXL_ASYNC 1
> #endif
> 
> Even better, the value originally came from pkg-config check for
> version. Isn't there a spice header which could provide the same
> version information? Then the changes in configure would not be
> needed.

This would require changing spice-protocol - I'd rather just use the
existing version that is already provided, and do this next time around,
when spice-protocol is updated for some other reason.

> 
> >
> >  /* qxl.c */
> >  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
> > diff --git a/trace-events b/trace-events
> > index 04b0723..8fcbc50 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -956,6 +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_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"
> > diff --git a/ui/spice-display.h b/ui/spice-display.h
> > index 12e50b6..7fa095f 100644
> > --- a/ui/spice-display.h
> > +++ b/ui/spice-display.h
> > @@ -51,6 +51,7 @@ typedef enum qxl_async_io {
> >  enum {
> >      QXL_COOKIE_TYPE_IO,
> >      QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
> > +    QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
> >  };
> >
> >  typedef struct QXLCookie {
> > --
> > 1.7.11.2
> >
> >
Blue Swirl - Aug. 18, 2012, 7:07 p.m.
On Sat, Aug 18, 2012 at 4:16 PM, Alon Levy <alevy@redhat.com> wrote:
> On Sat, Aug 18, 2012 at 02:31:32PM +0000, Blue Swirl wrote:
>> On Fri, Aug 17, 2012 at 3:39 PM, Alon Levy <alevy@redhat.com> wrote:
>> > Revision bumped to 4 for new IO support, enabled for spice-server >=
>> > 0.11.1. New io enabled iff spice-server >= 0.11.1 && spice-protocol >=
>> > 0.12.0.
>> >
>> > On migration reissue spice_qxl_monitors_config_async.
>> >
>> > RHBZ: 770842
>> >
>> > Signed-off-by: Alon Levy <alevy@redhat.com>
>> > ---
>> > Fixed another defined I missed. This time used grep to verify it's the last
>> > one..
>> >
>> > Alon
>> >
>> >  configure          |  3 +++
>> >  hw/qxl.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> >  hw/qxl.h           |  7 +++++++
>> >  trace-events       |  1 +
>> >  ui/spice-display.h |  1 +
>> >  5 files changed, 70 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/configure b/configure
>> > index cc774b5..dbf3af6 100755
>> > --- a/configure
>> > +++ b/configure
>> > @@ -2657,6 +2657,9 @@ EOF
>> >      spice="yes"
>> >      libs_softmmu="$libs_softmmu $spice_libs"
>> >      QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
>> > +    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
>> > +        QEMU_CFLAGS="$QEMU_CFLAGS -DQXL_HAS_IO_MONITORS_CONFIG_ASYNC=1"
>>
>> Please put this to config-host.h instead of -D. Then change detection
>> for config-host.h and dependencies cause a rebuild.
> ok.
>
>>
>> > +    fi
>> >    else
>> >      if test "$spice" = "yes" ; then
>> >        feature_not_found "spice"
>> > diff --git a/hw/qxl.c b/hw/qxl.c
>> > index c978f5e..25d7759 100644
>> > --- a/hw/qxl.c
>> > +++ b/hw/qxl.c
>> > @@ -27,6 +27,10 @@
>> >
>> >  #include "qxl.h"
>> >
>> > +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>> > +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
>>
>> Just delete this and use defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
>
> So you are telling me to undo a change that Gerd asked for - could you
> please at least debate about the merits of both approaches? the point of
> having QXL_HAS_IO_MONITORS_CONFIG_ASYNC always defined was to allow
> usage of #if without defined, which is shorter.

Variables #defined by configure either do not exist or are defined,
I'd rather not deviate from this.

#if when something isn't defined is incorrect and this case could slip
in somehow since the same #define may already exist.

About shortness: it would take many #ifdefs shortened to #if (-3
chars, defined() -9) to balance the addition of these lines (about +60
chars).

>
>>
>> > +#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
>> > @@ -249,6 +253,20 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
>> >      }
>> >  }
>> >
>> > +/* 0x00b01 == 0.11.1 */
>> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>>
>> Like here.
>>
>> > +static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl)
>> > +{
>> > +    trace_qxl_spice_monitors_config(qxl->id);
>> > +    qxl->guest_monitors_config = qxl->ram->monitors_config;
>> > +    spice_qxl_monitors_config_async(&qxl->ssd.qxl,
>> > +            qxl->ram->monitors_config,
>> > +            MEMSLOT_GROUP_GUEST,
>> > +            (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
>> > +                                      QXL_IO_MONITORS_CONFIG_ASYNC));
>> > +}
>> > +#endif
>> > +
>> >  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
>> >  {
>> >      trace_qxl_spice_reset_image_cache(qxl->id);
>> > @@ -538,6 +556,9 @@ static const char *io_port_to_string(uint32_t io_port)
>> >                                          = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
>> >          [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
>> >          [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
>> > +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>> > +        [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
>> > +#endif
>> >      };
>> >      return io_port_to_string[io_port];
>> >  }
>> > @@ -819,7 +840,10 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
>> >      case QXL_IO_DESTROY_PRIMARY_ASYNC:
>> >      case QXL_IO_UPDATE_AREA_ASYNC:
>> >      case QXL_IO_FLUSH_SURFACES_ASYNC:
>> > +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
>> >          break;
>> > +#endif
>> >      case QXL_IO_CREATE_PRIMARY_ASYNC:
>> >          qxl_create_guest_primary_complete(qxl);
>> >          break;
>> > @@ -894,6 +918,8 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
>> >      case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA:
>> >          qxl_render_update_area_done(qxl, cookie);
>> >          break;
>> > +    case QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG:
>> > +        break;
>> >      default:
>> >          fprintf(stderr, "qxl: %s: unexpected cookie type %d\n",
>> >                  __func__, cookie->type);
>> > @@ -1333,7 +1359,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
>> >              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) {
>> > +            io_port < QXL_IO_RANGE_SIZE) {
>> >              qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
>> >          }
>> >          return;
>> > @@ -1361,6 +1387,10 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
>> >          io_port = QXL_IO_DESTROY_ALL_SURFACES;
>> >          goto async_common;
>> >      case QXL_IO_FLUSH_SURFACES_ASYNC:
>> > +/* 0x000b01 == 0.11.1 */
>> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
>> > +#endif
>> >  async_common:
>> >          async = QXL_ASYNC;
>> >          qemu_mutex_lock(&d->async_lock);
>> > @@ -1502,6 +1532,12 @@ async_common:
>> >          d->mode = QXL_MODE_UNDEFINED;
>> >          qxl_spice_destroy_surfaces(d, async);
>> >          break;
>> > +/* 0x000b01 == 0.11.1 */
>> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
>> > +        qxl_spice_monitors_config_async(d);
>> > +        break;
>> > +#endif
>> >      default:
>> >          qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
>> >      }
>> > @@ -1797,9 +1833,15 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>> >          io_size = 16;
>> >          break;
>> >      case 3: /* qxl-3 */
>> > +        pci_device_rev = QXL_REVISION_STABLE_V10;
>> > +        io_size = 32; /* PCI region size must be pow2 */
>> > +        break;
>> > +#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
>> > +    case 4: /* qxl-4 */
>> >          pci_device_rev = QXL_DEFAULT_REVISION;
>> >          io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
>> >          break;
>> > +#endif
>> >      default:
>> >          error_report("Invalid revision %d for qxl device (max %d)",
>> >                       qxl->revision, QXL_DEFAULT_REVISION);
>> > @@ -1998,7 +2040,20 @@ static int qxl_post_load(void *opaque, int version)
>> >          }
>> >          qxl_spice_loadvm_commands(d, cmds, out);
>> >          g_free(cmds);
>> > -
>> > +        if (d->guest_monitors_config) {
>> > +            /*
>> > +             * don't use QXL_COOKIE_TYPE_IO:
>> > +             *  - we are not running yet (post_load), we will assert
>> > +             *    in send_events
>> > +             *  - this is not a guest io, but a reply, so async_io isn't set.
>> > +             */
>> > +            spice_qxl_monitors_config_async(&d->ssd.qxl,
>> > +                    d->guest_monitors_config,
>> > +                    MEMSLOT_GROUP_GUEST,
>> > +                    (uintptr_t)qxl_cookie_new(
>> > +                        QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
>> > +                        0));
>> > +        }
>> >          break;
>> >      case QXL_MODE_COMPAT:
>> >          /* note: no need to call qxl_create_memslots, qxl_set_mode
>> > @@ -2065,6 +2120,7 @@ static VMStateDescription qxl_vmstate = {
>> >          VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
>> >                        vmstate_info_uint64, uint64_t),
>> >          VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
>> > +        VMSTATE_UINT64(guest_monitors_config, PCIQXLDevice),
>> >          VMSTATE_END_OF_LIST()
>> >      },
>> >  };
>> > diff --git a/hw/qxl.h b/hw/qxl.h
>> > index 172baf6..0c5ebbd 100644
>> > --- a/hw/qxl.h
>> > +++ b/hw/qxl.h
>> > @@ -71,6 +71,8 @@ typedef struct PCIQXLDevice {
>> >      } guest_surfaces;
>> >      QXLPHYSICAL        guest_cursor;
>> >
>> > +    QXLPHYSICAL        guest_monitors_config;
>> > +
>> >      QemuMutex          track_lock;
>> >
>> >      /* thread signaling */
>> > @@ -128,7 +130,12 @@ typedef struct PCIQXLDevice {
>> >          }                                                               \
>> >      } while (0)
>> >
>> > +/* 0x000b01 == 0.11.1 */
>> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>> > +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
>> > +#else
>> >  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
>> > +#endif
>>
>> Alternatively, put the 0/1 logic here:
>> #ifndef CONFIG_QXL_ASYNC
>> #define QXL_ASYNC 0
>> #else
>> #define QXL_ASYNC 1
>> #endif
>>
>> Even better, the value originally came from pkg-config check for
>> version. Isn't there a spice header which could provide the same
>> version information? Then the changes in configure would not be
>> needed.
>
> This would require changing spice-protocol - I'd rather just use the
> existing version that is already provided, and do this next time around,
> when spice-protocol is updated for some other reason.
>
>>
>> >
>> >  /* qxl.c */
>> >  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
>> > diff --git a/trace-events b/trace-events
>> > index 04b0723..8fcbc50 100644
>> > --- a/trace-events
>> > +++ b/trace-events
>> > @@ -956,6 +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_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"
>> > diff --git a/ui/spice-display.h b/ui/spice-display.h
>> > index 12e50b6..7fa095f 100644
>> > --- a/ui/spice-display.h
>> > +++ b/ui/spice-display.h
>> > @@ -51,6 +51,7 @@ typedef enum qxl_async_io {
>> >  enum {
>> >      QXL_COOKIE_TYPE_IO,
>> >      QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
>> > +    QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
>> >  };
>> >
>> >  typedef struct QXLCookie {
>> > --
>> > 1.7.11.2
>> >
>> >
Gerd Hoffmann - Aug. 20, 2012, 6:07 a.m.
Hi,

>>> +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>>> +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
>>
>> Just delete this and use defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
> 
> So you are telling me to undo a change that Gerd asked for - could you
> please at least debate about the merits of both approaches? the point of
> having QXL_HAS_IO_MONITORS_CONFIG_ASYNC always defined was to allow
> usage of #if without defined, which is shorter.

Hmm?  That wasn't that I meant, must have been a tyops.
I mean you should just do ...

#ifndef QXL_IO_MONITORS_CONFIG_ASYNC            <--- without *_HAS_*
#define QXL_IO_MONITORS_CONFIG_ASYNC $value
#endif

then you don't need QXL_HAS_IO_MONITORS_CONFIG_ASYNC (and all the
#ifdefs) at all ...

cheers,
  Gerd
Alon Levy - Aug. 20, 2012, 8 a.m.
> Hi,
> 
> >>> +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >>> +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
> >>
> >> Just delete this and use
> >> defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
> > 
> > So you are telling me to undo a change that Gerd asked for - could
> > you
> > please at least debate about the merits of both approaches? the
> > point of
> > having QXL_HAS_IO_MONITORS_CONFIG_ASYNC always defined was to allow
> > usage of #if without defined, which is shorter.
> 
> Hmm?  That wasn't that I meant, must have been a tyops.
> I mean you should just do ...
> 
> #ifndef QXL_IO_MONITORS_CONFIG_ASYNC            <--- without *_HAS_*
> #define QXL_IO_MONITORS_CONFIG_ASYNC $value
> #endif
> 
> then you don't need QXL_HAS_IO_MONITORS_CONFIG_ASYNC (and all the
> #ifdefs) at all ...

So you want me to give the io a value - at this point I'd rather just add spice-protocol as a submodule, then we don't need to do any of this. How about it?

> 
> cheers,
>   Gerd
> 
> 
>
Alon Levy - Aug. 20, 2012, 8:20 a.m.
> > Hi,
> > 
> > >>> +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> > >>> +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
> > >>
> > >> Just delete this and use
> > >> defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
> > > 
> > > So you are telling me to undo a change that Gerd asked for -
> > > could
> > > you
> > > please at least debate about the merits of both approaches? the
> > > point of
> > > having QXL_HAS_IO_MONITORS_CONFIG_ASYNC always defined was to
> > > allow
> > > usage of #if without defined, which is shorter.
> > 
> > Hmm?  That wasn't that I meant, must have been a tyops.
> > I mean you should just do ...
> > 
> > #ifndef QXL_IO_MONITORS_CONFIG_ASYNC            <--- without
> > *_HAS_*
> > #define QXL_IO_MONITORS_CONFIG_ASYNC $value
> > #endif
> > 
> > then you don't need QXL_HAS_IO_MONITORS_CONFIG_ASYNC (and all the
> > #ifdefs) at all ...
> 
> So you want me to give the io a value - at this point I'd rather just
> add spice-protocol as a submodule, then we don't need to do any of
> this. How about it?

I'm retracting the suggestion, I'll define the missing bits as requested and remove the ifdefs to satisfy Blue Swirl.

> 
> > 
> > cheers,
> >   Gerd
> > 
> > 
> > 
> 
>
Gerd Hoffmann - Aug. 20, 2012, 8:32 a.m.
On 08/20/12 10:00, Alon Levy wrote:
>> Hi,
>> 
>>>>> +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC +#define
>>>>> QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
>>>> 
>>>> Just delete this and use 
>>>> defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
>>> 
>>> So you are telling me to undo a change that Gerd asked for -
>>> could you please at least debate about the merits of both
>>> approaches? the point of having QXL_HAS_IO_MONITORS_CONFIG_ASYNC
>>> always defined was to allow usage of #if without defined, which
>>> is shorter.
>> 
>> Hmm?  That wasn't that I meant, must have been a tyops. I mean you
>> should just do ...
>> 
>> #ifndef QXL_IO_MONITORS_CONFIG_ASYNC            <--- without
>> *_HAS_* #define QXL_IO_MONITORS_CONFIG_ASYNC $value #endif
>> 
>> then you don't need QXL_HAS_IO_MONITORS_CONFIG_ASYNC (and all the 
>> #ifdefs) at all ...
> 
> So you want me to give the io a value

Well, it has one, right?  [ checking spice-protocol ]  It's 24.

> - at this point I'd rather just add spice-protocol as a submodule,
> then we don't need to do any of this. How about it?

No.  You can build qemu without submodules today as they are used for
ROMS only (which are also provided as precompiled binaries).

Maybe revisit upstream spice packaging?  spice internal usage of
spice-protocol is handled via submodules now.  Are there external users,
other than qemu?  Does it make sense to keep the spice-server /
spice-protocol split in the first place?  Or should spice-server just
provide the protocol headers too?

cheers,
  Gerd
Alon Levy - Aug. 20, 2012, 8:48 a.m.
> On 08/20/12 10:00, Alon Levy wrote:
> >> Hi,
> >> 
> >>>>> +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC +#define
> >>>>> QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
> >>>> 
> >>>> Just delete this and use
> >>>> defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
> >>> 
> >>> So you are telling me to undo a change that Gerd asked for -
> >>> could you please at least debate about the merits of both
> >>> approaches? the point of having QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >>> always defined was to allow usage of #if without defined, which
> >>> is shorter.
> >> 
> >> Hmm?  That wasn't that I meant, must have been a tyops. I mean you
> >> should just do ...
> >> 
> >> #ifndef QXL_IO_MONITORS_CONFIG_ASYNC            <--- without
> >> *_HAS_* #define QXL_IO_MONITORS_CONFIG_ASYNC $value #endif
> >> 
> >> then you don't need QXL_HAS_IO_MONITORS_CONFIG_ASYNC (and all the
> >> #ifdefs) at all ...
> > 
> > So you want me to give the io a value
> 
> Well, it has one, right?  [ checking spice-protocol ]  It's 24.

Yes, I'll define it as (the_previous_one + 1).

> 
> > - at this point I'd rather just add spice-protocol as a submodule,
> > then we don't need to do any of this. How about it?
> 
> No.  You can build qemu without submodules today as they are used for
> ROMS only (which are also provided as precompiled binaries).

Makes sense, I misunderstood the use of the submodules in qemu.

> 
> Maybe revisit upstream spice packaging?  spice internal usage of
> spice-protocol is handled via submodules now.  Are there external
> users,
> other than qemu?  Does it make sense to keep the spice-server /
> spice-protocol split in the first place?  Or should spice-server just
> provide the protocol headers too?

spice-server is a much larger project then spice-protocol. The agents and the drivers don't need any bits in spice-server. Sure, we can unify them - it would make it easier for qemu and Xspice (the only other external user). But submodule usage would be harder then. I actually like the current submodularization of spice-protocol - it's a small header only repository that is fast to fetch.

> 
> cheers,
>   Gerd
> 
>
Alon Levy - Aug. 20, 2012, 8:56 a.m.
On Sat, Aug 18, 2012 at 07:07:46PM +0000, Blue Swirl wrote:
> On Sat, Aug 18, 2012 at 4:16 PM, Alon Levy <alevy@redhat.com> wrote:
> > On Sat, Aug 18, 2012 at 02:31:32PM +0000, Blue Swirl wrote:
> >> On Fri, Aug 17, 2012 at 3:39 PM, Alon Levy <alevy@redhat.com> wrote:
> >> > Revision bumped to 4 for new IO support, enabled for spice-server >=
> >> > 0.11.1. New io enabled iff spice-server >= 0.11.1 && spice-protocol >=
> >> > 0.12.0.
> >> >
> >> > On migration reissue spice_qxl_monitors_config_async.
> >> >
> >> > RHBZ: 770842
> >> >
> >> > Signed-off-by: Alon Levy <alevy@redhat.com>
> >> > ---
> >> > Fixed another defined I missed. This time used grep to verify it's the last
> >> > one..
> >> >
> >> > Alon
> >> >
> >> >  configure          |  3 +++
> >> >  hw/qxl.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >> >  hw/qxl.h           |  7 +++++++
> >> >  trace-events       |  1 +
> >> >  ui/spice-display.h |  1 +
> >> >  5 files changed, 70 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/configure b/configure
> >> > index cc774b5..dbf3af6 100755
> >> > --- a/configure
> >> > +++ b/configure
> >> > @@ -2657,6 +2657,9 @@ EOF
> >> >      spice="yes"
> >> >      libs_softmmu="$libs_softmmu $spice_libs"
> >> >      QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
> >> > +    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
> >> > +        QEMU_CFLAGS="$QEMU_CFLAGS -DQXL_HAS_IO_MONITORS_CONFIG_ASYNC=1"
> >>
> >> Please put this to config-host.h instead of -D. Then change detection
> >> for config-host.h and dependencies cause a rebuild.
> > ok.
> >
> >>
> >> > +    fi
> >> >    else
> >> >      if test "$spice" = "yes" ; then
> >> >        feature_not_found "spice"
> >> > diff --git a/hw/qxl.c b/hw/qxl.c
> >> > index c978f5e..25d7759 100644
> >> > --- a/hw/qxl.c
> >> > +++ b/hw/qxl.c
> >> > @@ -27,6 +27,10 @@
> >> >
> >> >  #include "qxl.h"
> >> >
> >> > +#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >> > +#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
> >>
> >> Just delete this and use defined(QXL_HAS_IO_MONITORS_CONFIG_ASYNC).
> >
> > So you are telling me to undo a change that Gerd asked for - could you
> > please at least debate about the merits of both approaches? the point of
> > having QXL_HAS_IO_MONITORS_CONFIG_ASYNC always defined was to allow
> > usage of #if without defined, which is shorter.
> 
> Variables #defined by configure either do not exist or are defined,
> I'd rather not deviate from this.
> 
> #if when something isn't defined is incorrect and this case could slip
> in somehow since the same #define may already exist.

OK.

> 
> About shortness: it would take many #ifdefs shortened to #if (-3
> chars, defined() -9) to balance the addition of these lines (about +60
> chars).

Saw right through my idiotic argument. Which was based on my
misunderstanding of Gerd. So I'm doing what Gerd suggested - copying the
missing bits from spice-protocol wrapped with ifdef of _HAS_ which is
based on spice-protocol pkg-config check, which allows to remove most of
the ifdefs I've added.

> 
> >
> >>
> >> > +#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
> >> > @@ -249,6 +253,20 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
> >> >      }
> >> >  }
> >> >
> >> > +/* 0x00b01 == 0.11.1 */
> >> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >>
> >> Like here.
> >>
> >> > +static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl)
> >> > +{
> >> > +    trace_qxl_spice_monitors_config(qxl->id);
> >> > +    qxl->guest_monitors_config = qxl->ram->monitors_config;
> >> > +    spice_qxl_monitors_config_async(&qxl->ssd.qxl,
> >> > +            qxl->ram->monitors_config,
> >> > +            MEMSLOT_GROUP_GUEST,
> >> > +            (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
> >> > +                                      QXL_IO_MONITORS_CONFIG_ASYNC));
> >> > +}
> >> > +#endif
> >> > +
> >> >  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
> >> >  {
> >> >      trace_qxl_spice_reset_image_cache(qxl->id);
> >> > @@ -538,6 +556,9 @@ static const char *io_port_to_string(uint32_t io_port)
> >> >                                          = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
> >> >          [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
> >> >          [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
> >> > +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >> > +        [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
> >> > +#endif
> >> >      };
> >> >      return io_port_to_string[io_port];
> >> >  }
> >> > @@ -819,7 +840,10 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
> >> >      case QXL_IO_DESTROY_PRIMARY_ASYNC:
> >> >      case QXL_IO_UPDATE_AREA_ASYNC:
> >> >      case QXL_IO_FLUSH_SURFACES_ASYNC:
> >> > +#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> >> >          break;
> >> > +#endif
> >> >      case QXL_IO_CREATE_PRIMARY_ASYNC:
> >> >          qxl_create_guest_primary_complete(qxl);
> >> >          break;
> >> > @@ -894,6 +918,8 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
> >> >      case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA:
> >> >          qxl_render_update_area_done(qxl, cookie);
> >> >          break;
> >> > +    case QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG:
> >> > +        break;
> >> >      default:
> >> >          fprintf(stderr, "qxl: %s: unexpected cookie type %d\n",
> >> >                  __func__, cookie->type);
> >> > @@ -1333,7 +1359,7 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
> >> >              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) {
> >> > +            io_port < QXL_IO_RANGE_SIZE) {
> >> >              qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
> >> >          }
> >> >          return;
> >> > @@ -1361,6 +1387,10 @@ static void ioport_write(void *opaque, target_phys_addr_t addr,
> >> >          io_port = QXL_IO_DESTROY_ALL_SURFACES;
> >> >          goto async_common;
> >> >      case QXL_IO_FLUSH_SURFACES_ASYNC:
> >> > +/* 0x000b01 == 0.11.1 */
> >> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> >> > +#endif
> >> >  async_common:
> >> >          async = QXL_ASYNC;
> >> >          qemu_mutex_lock(&d->async_lock);
> >> > @@ -1502,6 +1532,12 @@ async_common:
> >> >          d->mode = QXL_MODE_UNDEFINED;
> >> >          qxl_spice_destroy_surfaces(d, async);
> >> >          break;
> >> > +/* 0x000b01 == 0.11.1 */
> >> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >> > +    case QXL_IO_MONITORS_CONFIG_ASYNC:
> >> > +        qxl_spice_monitors_config_async(d);
> >> > +        break;
> >> > +#endif
> >> >      default:
> >> >          qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
> >> >      }
> >> > @@ -1797,9 +1833,15 @@ static int qxl_init_common(PCIQXLDevice *qxl)
> >> >          io_size = 16;
> >> >          break;
> >> >      case 3: /* qxl-3 */
> >> > +        pci_device_rev = QXL_REVISION_STABLE_V10;
> >> > +        io_size = 32; /* PCI region size must be pow2 */
> >> > +        break;
> >> > +#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
> >> > +    case 4: /* qxl-4 */
> >> >          pci_device_rev = QXL_DEFAULT_REVISION;
> >> >          io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> >> >          break;
> >> > +#endif
> >> >      default:
> >> >          error_report("Invalid revision %d for qxl device (max %d)",
> >> >                       qxl->revision, QXL_DEFAULT_REVISION);
> >> > @@ -1998,7 +2040,20 @@ static int qxl_post_load(void *opaque, int version)
> >> >          }
> >> >          qxl_spice_loadvm_commands(d, cmds, out);
> >> >          g_free(cmds);
> >> > -
> >> > +        if (d->guest_monitors_config) {
> >> > +            /*
> >> > +             * don't use QXL_COOKIE_TYPE_IO:
> >> > +             *  - we are not running yet (post_load), we will assert
> >> > +             *    in send_events
> >> > +             *  - this is not a guest io, but a reply, so async_io isn't set.
> >> > +             */
> >> > +            spice_qxl_monitors_config_async(&d->ssd.qxl,
> >> > +                    d->guest_monitors_config,
> >> > +                    MEMSLOT_GROUP_GUEST,
> >> > +                    (uintptr_t)qxl_cookie_new(
> >> > +                        QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
> >> > +                        0));
> >> > +        }
> >> >          break;
> >> >      case QXL_MODE_COMPAT:
> >> >          /* note: no need to call qxl_create_memslots, qxl_set_mode
> >> > @@ -2065,6 +2120,7 @@ static VMStateDescription qxl_vmstate = {
> >> >          VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
> >> >                        vmstate_info_uint64, uint64_t),
> >> >          VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
> >> > +        VMSTATE_UINT64(guest_monitors_config, PCIQXLDevice),
> >> >          VMSTATE_END_OF_LIST()
> >> >      },
> >> >  };
> >> > diff --git a/hw/qxl.h b/hw/qxl.h
> >> > index 172baf6..0c5ebbd 100644
> >> > --- a/hw/qxl.h
> >> > +++ b/hw/qxl.h
> >> > @@ -71,6 +71,8 @@ typedef struct PCIQXLDevice {
> >> >      } guest_surfaces;
> >> >      QXLPHYSICAL        guest_cursor;
> >> >
> >> > +    QXLPHYSICAL        guest_monitors_config;
> >> > +
> >> >      QemuMutex          track_lock;
> >> >
> >> >      /* thread signaling */
> >> > @@ -128,7 +130,12 @@ typedef struct PCIQXLDevice {
> >> >          }                                                               \
> >> >      } while (0)
> >> >
> >> > +/* 0x000b01 == 0.11.1 */
> >> > +#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
> >> > +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
> >> > +#else
> >> >  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> >> > +#endif
> >>
> >> Alternatively, put the 0/1 logic here:
> >> #ifndef CONFIG_QXL_ASYNC
> >> #define QXL_ASYNC 0
> >> #else
> >> #define QXL_ASYNC 1
> >> #endif
> >>
> >> Even better, the value originally came from pkg-config check for
> >> version. Isn't there a spice header which could provide the same
> >> version information? Then the changes in configure would not be
> >> needed.
> >
> > This would require changing spice-protocol - I'd rather just use the
> > existing version that is already provided, and do this next time around,
> > when spice-protocol is updated for some other reason.
> >
> >>
> >> >
> >> >  /* qxl.c */
> >> >  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
> >> > diff --git a/trace-events b/trace-events
> >> > index 04b0723..8fcbc50 100644
> >> > --- a/trace-events
> >> > +++ b/trace-events
> >> > @@ -956,6 +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_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"
> >> > diff --git a/ui/spice-display.h b/ui/spice-display.h
> >> > index 12e50b6..7fa095f 100644
> >> > --- a/ui/spice-display.h
> >> > +++ b/ui/spice-display.h
> >> > @@ -51,6 +51,7 @@ typedef enum qxl_async_io {
> >> >  enum {
> >> >      QXL_COOKIE_TYPE_IO,
> >> >      QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
> >> > +    QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
> >> >  };
> >> >
> >> >  typedef struct QXLCookie {
> >> > --
> >> > 1.7.11.2
> >> >
> >> >
Gerd Hoffmann - Aug. 20, 2012, 9:02 a.m.
Hi,

>> Maybe revisit upstream spice packaging?  spice internal usage of 
>> spice-protocol is handled via submodules now.  Are there external 
>> users, other than qemu?  Does it make sense to keep the
>> spice-server / spice-protocol split in the first place?  Or should
>> spice-server just provide the protocol headers too?
> 
> spice-server is a much larger project then spice-protocol. The agents
> and the drivers don't need any bits in spice-server.

I didn't meant to kill the spice-protocol git repo, just the way it is
distributed.

Remove any makefiles & stuff from spice-protocol, so it is really just
the headers.  Any spice-internal users get it as submodule like they do
today.  spice-server gets updated to also install the spice-protocol
header files from the submodule.

Kills the whole protocol header version dance for the external users
(the submodule usage already does it for the internal ones).  When
you've installed spice-server-devel you automatically also have recent
enougth spice protocol headers installed.

> Sure, we can
> unify them - it would make it easier for qemu and Xspice (the only
> other external user).

So both external users need the spice server too.  So I think it makes
sense to have spice-server ship the protocol headers.

cheers,
  Gerd
Alon Levy - Aug. 20, 2012, 9:37 a.m.
----- Original Message -----
> Hi,
> 
> >> Maybe revisit upstream spice packaging?  spice internal usage of
> >> spice-protocol is handled via submodules now.  Are there external
> >> users, other than qemu?  Does it make sense to keep the
> >> spice-server / spice-protocol split in the first place?  Or should
> >> spice-server just provide the protocol headers too?
> > 
> > spice-server is a much larger project then spice-protocol. The
> > agents
> > and the drivers don't need any bits in spice-server.
> 
> I didn't meant to kill the spice-protocol git repo, just the way it
> is
> distributed.
> 
> Remove any makefiles & stuff from spice-protocol, so it is really
> just
> the headers.  Any spice-internal users get it as submodule like they
> do
> today.  spice-server gets updated to also install the spice-protocol
> header files from the submodule.
> 
> Kills the whole protocol header version dance for the external users
> (the submodule usage already does it for the internal ones).  When
> you've installed spice-server-devel you automatically also have
> recent
> enougth spice protocol headers installed.
> 

Yes, we could do it, I'm not sure it's worth the trouble. The only benefit is that it removes one more pkg-config check. But it means that anyone who wants to develop a new client has to install spice-server to get the client headers.

Right now the protocol has other problems - it doesn't contain the complete protocol specification, spice.proto, that is actually in spice-common. And it's wrongly named - it contains the agent protocol and the device "protocol" as well.

> > Sure, we can
> > unify them - it would make it easier for qemu and Xspice (the only
> > other external user).
> 
> So both external users need the spice server too.  So I think it
> makes
> sense to have spice-server ship the protocol headers.
> 
> cheers,
>   Gerd
> 
> 
>

Patch

diff --git a/configure b/configure
index cc774b5..dbf3af6 100755
--- a/configure
+++ b/configure
@@ -2657,6 +2657,9 @@  EOF
     spice="yes"
     libs_softmmu="$libs_softmmu $spice_libs"
     QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
+    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
+        QEMU_CFLAGS="$QEMU_CFLAGS -DQXL_HAS_IO_MONITORS_CONFIG_ASYNC=1"
+    fi
   else
     if test "$spice" = "yes" ; then
       feature_not_found "spice"
diff --git a/hw/qxl.c b/hw/qxl.c
index c978f5e..25d7759 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -27,6 +27,10 @@ 
 
 #include "qxl.h"
 
+#ifndef QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+#define QXL_HAS_IO_MONITORS_CONFIG_ASYNC 0
+#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
@@ -249,6 +253,20 @@  static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
     }
 }
 
+/* 0x00b01 == 0.11.1 */
+#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl)
+{
+    trace_qxl_spice_monitors_config(qxl->id);
+    qxl->guest_monitors_config = qxl->ram->monitors_config;
+    spice_qxl_monitors_config_async(&qxl->ssd.qxl,
+            qxl->ram->monitors_config,
+            MEMSLOT_GROUP_GUEST,
+            (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
+                                      QXL_IO_MONITORS_CONFIG_ASYNC));
+}
+#endif
+
 void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
 {
     trace_qxl_spice_reset_image_cache(qxl->id);
@@ -538,6 +556,9 @@  static const char *io_port_to_string(uint32_t io_port)
                                         = "QXL_IO_DESTROY_ALL_SURFACES_ASYNC",
         [QXL_IO_FLUSH_SURFACES_ASYNC]   = "QXL_IO_FLUSH_SURFACES_ASYNC",
         [QXL_IO_FLUSH_RELEASE]          = "QXL_IO_FLUSH_RELEASE",
+#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+        [QXL_IO_MONITORS_CONFIG_ASYNC]  = "QXL_IO_MONITORS_CONFIG_ASYNC",
+#endif
     };
     return io_port_to_string[io_port];
 }
@@ -819,7 +840,10 @@  static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
     case QXL_IO_DESTROY_PRIMARY_ASYNC:
     case QXL_IO_UPDATE_AREA_ASYNC:
     case QXL_IO_FLUSH_SURFACES_ASYNC:
+#if QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+    case QXL_IO_MONITORS_CONFIG_ASYNC:
         break;
+#endif
     case QXL_IO_CREATE_PRIMARY_ASYNC:
         qxl_create_guest_primary_complete(qxl);
         break;
@@ -894,6 +918,8 @@  static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA:
         qxl_render_update_area_done(qxl, cookie);
         break;
+    case QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG:
+        break;
     default:
         fprintf(stderr, "qxl: %s: unexpected cookie type %d\n",
                 __func__, cookie->type);
@@ -1333,7 +1359,7 @@  static void ioport_write(void *opaque, target_phys_addr_t addr,
             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) {
+            io_port < QXL_IO_RANGE_SIZE) {
             qxl_send_events(d, QXL_INTERRUPT_IO_CMD);
         }
         return;
@@ -1361,6 +1387,10 @@  static void ioport_write(void *opaque, target_phys_addr_t addr,
         io_port = QXL_IO_DESTROY_ALL_SURFACES;
         goto async_common;
     case QXL_IO_FLUSH_SURFACES_ASYNC:
+/* 0x000b01 == 0.11.1 */
+#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+    case QXL_IO_MONITORS_CONFIG_ASYNC:
+#endif
 async_common:
         async = QXL_ASYNC;
         qemu_mutex_lock(&d->async_lock);
@@ -1502,6 +1532,12 @@  async_common:
         d->mode = QXL_MODE_UNDEFINED;
         qxl_spice_destroy_surfaces(d, async);
         break;
+/* 0x000b01 == 0.11.1 */
+#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+    case QXL_IO_MONITORS_CONFIG_ASYNC:
+        qxl_spice_monitors_config_async(d);
+        break;
+#endif
     default:
         qxl_set_guest_bug(d, "%s: unexpected ioport=0x%x\n", __func__, io_port);
     }
@@ -1797,9 +1833,15 @@  static int qxl_init_common(PCIQXLDevice *qxl)
         io_size = 16;
         break;
     case 3: /* qxl-3 */
+        pci_device_rev = QXL_REVISION_STABLE_V10;
+        io_size = 32; /* PCI region size must be pow2 */
+        break;
+#if SPICE_SERVER_VERSION >= 0x000b01 /* 0.11.1 */
+    case 4: /* qxl-4 */
         pci_device_rev = QXL_DEFAULT_REVISION;
         io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
         break;
+#endif
     default:
         error_report("Invalid revision %d for qxl device (max %d)",
                      qxl->revision, QXL_DEFAULT_REVISION);
@@ -1998,7 +2040,20 @@  static int qxl_post_load(void *opaque, int version)
         }
         qxl_spice_loadvm_commands(d, cmds, out);
         g_free(cmds);
-
+        if (d->guest_monitors_config) {
+            /*
+             * don't use QXL_COOKIE_TYPE_IO:
+             *  - we are not running yet (post_load), we will assert
+             *    in send_events
+             *  - this is not a guest io, but a reply, so async_io isn't set.
+             */
+            spice_qxl_monitors_config_async(&d->ssd.qxl,
+                    d->guest_monitors_config,
+                    MEMSLOT_GROUP_GUEST,
+                    (uintptr_t)qxl_cookie_new(
+                        QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
+                        0));
+        }
         break;
     case QXL_MODE_COMPAT:
         /* note: no need to call qxl_create_memslots, qxl_set_mode
@@ -2065,6 +2120,7 @@  static VMStateDescription qxl_vmstate = {
         VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
                       vmstate_info_uint64, uint64_t),
         VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
+        VMSTATE_UINT64(guest_monitors_config, PCIQXLDevice),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/hw/qxl.h b/hw/qxl.h
index 172baf6..0c5ebbd 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -71,6 +71,8 @@  typedef struct PCIQXLDevice {
     } guest_surfaces;
     QXLPHYSICAL        guest_cursor;
 
+    QXLPHYSICAL        guest_monitors_config;
+
     QemuMutex          track_lock;
 
     /* thread signaling */
@@ -128,7 +130,12 @@  typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
+/* 0x000b01 == 0.11.1 */
+#if SPICE_SERVER_VERSION >= 0x000b01 && QXL_HAS_IO_MONITORS_CONFIG_ASYNC
+#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
+#else
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
+#endif
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
diff --git a/trace-events b/trace-events
index 04b0723..8fcbc50 100644
--- a/trace-events
+++ b/trace-events
@@ -956,6 +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_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"
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 12e50b6..7fa095f 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -51,6 +51,7 @@  typedef enum qxl_async_io {
 enum {
     QXL_COOKIE_TYPE_IO,
     QXL_COOKIE_TYPE_RENDER_UPDATE_AREA,
+    QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
 };
 
 typedef struct QXLCookie {