| Message ID | 1345217997-21965-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 > > > >
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 >> > >> >
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
> 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 > > >
> > 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 > > > > > > > >
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
> 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 > >
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 > >> > > >> >
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
----- 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 > > >
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 {
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(-)