Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC

[Blue Swirl]

On Sat, Aug 18, 2012 at 4:16 PM, Alon Levy <address@hidden> 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 <address@hidden> 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 <address@hidden>
>> > ---
>> > 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
>> >
>> >