[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC
From: |
Alon Levy |
Subject: |
Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC |
Date: |
Mon, 20 Aug 2012 11:56:36 +0300 |
User-agent: |
Mutt/1.5.21 (2011-07-01) |
On Sat, Aug 18, 2012 at 07:07:46PM +0000, Blue Swirl wrote:
> 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.
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
> >> >
> >> >
- [Qemu-devel] [PATCH v5 1.2 queue 0/4] QXL_IO_MONITORS_CONFIG_ASYNC + misc, Alon Levy, 2012/08/17
- [Qemu-devel] [PATCH v5 1.2 queue 1/4] qxl/update_area_io: guest_bug on invalid parameters, Alon Levy, 2012/08/17
- [Qemu-devel] [PATCH v5 1.2 queue 2/4] qxl: disallow unknown revisions, Alon Levy, 2012/08/17
- [Qemu-devel] [PATCH v5 1.2 queue 3/4] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/17
- [Qemu-devel] [PATCH v6 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/17
- [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/17
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Gerd Hoffmann, 2012/08/20
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/20
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/20
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Gerd Hoffmann, 2012/08/20
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/20
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Gerd Hoffmann, 2012/08/20
- Re: [Qemu-devel] [PATCH v7 1.2] qxl: add QXL_IO_MONITORS_CONFIG_ASYNC, Alon Levy, 2012/08/20
[Qemu-devel] [PATCH v5 1.2 queue 4/4] configure: print spice-protocol and spice-server versions, Alon Levy, 2012/08/17