On Sat, Aug 18, 2012 at 07:07:46PM +0000, Blue Swirl wrote: > On Sat, Aug 18, 2012 at 4:16 PM, Alon Levy <al...@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 <al...@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 <al...@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 > >> > > >> >