Patchwork spice: add qxl device

login
register
mail settings
Submitter Gerd Hoffmann
Date Nov. 2, 2010, 1:34 p.m.
Message ID <1288704898-30234-1-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/69882/
State New
Headers show

Comments

Gerd Hoffmann - Nov. 2, 2010, 1:34 p.m.
qxl is a paravirtual graphics card.  The qxl device is the bridge
between the guest and the spice server (aka libspice-server).  The
spice server will send the rendering commands to the spice client, which
will actually render them.

The spice server is also able to render locally, which is done in case
the guest wants read something from video memory.  Local rendering is
also used to support display over vnc and sdl.

qxl is activated using "-vga qxl".  qxl supports multihead, additional
cards can be added via '-device qxl".

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.target |    1 +
 hw/hw.h         |   14 +
 hw/pc.c         |    8 +
 hw/qxl-logger.c |  236 +++++++++
 hw/qxl-render.c |  213 ++++++++
 hw/qxl.c        | 1552 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qxl.h        |  110 ++++
 hw/vga_int.h    |    2 +-
 qemu-options.hx |    6 +-
 sysemu.h        |    3 +-
 ui/spice-core.c |   15 +
 vl.c            |    4 +-
 12 files changed, 2160 insertions(+), 4 deletions(-)
 create mode 100644 hw/qxl-logger.c
 create mode 100644 hw/qxl-render.c
 create mode 100644 hw/qxl.c
 create mode 100644 hw/qxl.h
Anthony Liguori - Nov. 16, 2010, 2:24 p.m.
On 11/02/2010 08:34 AM, Gerd Hoffmann wrote:
> qxl is a paravirtual graphics card.  The qxl device is the bridge
> between the guest and the spice server (aka libspice-server).  The
> spice server will send the rendering commands to the spice client, which
> will actually render them.
>
> The spice server is also able to render locally, which is done in case
> the guest wants read something from video memory.  Local rendering is
> also used to support display over vnc and sdl.
>
> qxl is activated using "-vga qxl".  qxl supports multihead, additional
> cards can be added via '-device qxl".
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   Makefile.target |    1 +
>   hw/hw.h         |   14 +
>   hw/pc.c         |    8 +
>   hw/qxl-logger.c |  236 +++++++++
>   hw/qxl-render.c |  213 ++++++++
>   hw/qxl.c        | 1552 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/qxl.h        |  110 ++++
>   hw/vga_int.h    |    2 +-
>   qemu-options.hx |    6 +-
>   sysemu.h        |    3 +-
>   ui/spice-core.c |   15 +
>   vl.c            |    4 +-
>   12 files changed, 2160 insertions(+), 4 deletions(-)
>   create mode 100644 hw/qxl-logger.c
>   create mode 100644 hw/qxl-render.c
>   create mode 100644 hw/qxl.c
>   create mode 100644 hw/qxl.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 91e6e74..6c4d64c 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -203,6 +203,7 @@ obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>   obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>   obj-i386-y += debugcon.o multiboot.o
>   obj-i386-y += pc_piix.o
> +obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>
>   # shared objects
>   obj-ppc-y = ppc.o
> diff --git a/hw/hw.h b/hw/hw.h
> index 9d2cfc2..234c713 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -528,6 +528,17 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>       .start        = (_start),                                        \
>   }
>
> +#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
> +    .name         = (stringify(_field)),                             \
> +    .version_id   = (_version),                                      \
> +    .field_exists = (_test),                                         \
> +    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> +    .info         =&vmstate_info_buffer,                            \
> +    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
> +    .offset       = offsetof(_state, _field),                        \
> +    .start        = (_start),                                        \
> +}
> +
>   #define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) { \
>       .name       = (stringify(_field)),                               \
>       .version_id = (_version),                                        \
> @@ -745,6 +756,9 @@ extern const VMStateDescription vmstate_i2c_slave;
>   #define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
>       VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
>
> +#define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
> +    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
> +
>   #define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
>       VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 69b13bf..0c31db1 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -40,6 +40,7 @@
>   #include "sysbus.h"
>   #include "sysemu.h"
>   #include "blockdev.h"
> +#include "ui/qemu-spice.h"
>
>   /* output Bochs bios info messages */
>   //#define DEBUG_BIOS
> @@ -991,6 +992,13 @@ void pc_vga_init(PCIBus *pci_bus)
>               pci_vmsvga_init(pci_bus);
>           else
>               fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__);
> +#ifdef CONFIG_SPICE
> +    } else if (qxl_enabled) {
> +        if (pci_bus)
> +            pci_create_simple(pci_bus, -1, "qxl");
> +        else
> +            fprintf(stderr, "%s: qxl: no PCI bus\n", __FUNCTION__);
> +#endif
>       } else if (std_vga_enabled) {
>           if (pci_bus) {
>               pci_vga_init(pci_bus, 0, 0);
> diff --git a/hw/qxl-logger.c b/hw/qxl-logger.c
> new file mode 100644
> index 0000000..cad5165
> --- /dev/null
> +++ b/hw/qxl-logger.c
> @@ -0,0 +1,236 @@
> +/*
> + * qxl command logging -- for debug purposes
> + */
> +
> +#include<stdio.h>
> +#include<stdbool.h>
> +#include<stdint.h>
> +#include<string.h>
>    

These headers shouldn't be needed.  This file also needs a 
copyright.diff --git a/hw/qxl-render.c b/hw/qxl-render.c

Does QXL have a specification?

> new file mode 100644
> index 0000000..e3941c8
> --- /dev/null
> +++ b/hw/qxl-render.c
> @@ -0,0 +1,213 @@
> +/*
> + * qxl local rendering (aka display on sdl/vnc)
> + */
> +#include<stdio.h>
> +#include<stdbool.h>
> +#include<stdint.h>
> +#include<string.h>
>    

Same as above.

> +#include "qxl.h"
> +
> +static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
> +{
> +    uint8_t *src = qxl->guest_primary.data;
> +    uint8_t *dst = qxl->guest_primary.flipped;
> +    int len, i;
> +
> +    src += (qxl->guest_primary.surface.height - rect->top - 1) *
> +        qxl->guest_primary.stride;
> +    dst += rect->top  * qxl->guest_primary.stride;
> +    src += rect->left * qxl->guest_primary.bytes_pp;
> +    dst += rect->left * qxl->guest_primary.bytes_pp;
> +    len  = (rect->right - rect->left) * qxl->guest_primary.bytes_pp;
> +
> +    for (i = rect->top; i<  rect->bottom; i++) {
> +        memcpy(dst, src, len);
> +        dst += qxl->guest_primary.stride;
> +        src -= qxl->guest_primary.stride;
> +    }
> +}
> +
> +void qxl_render_resize(PCIQXLDevice *qxl)
> +{
> +    QXLSurfaceCreate *sc =&qxl->guest_primary.surface;
> +
> +    qxl->guest_primary.stride = sc->stride;
> +    qxl->guest_primary.resized++;
> +    switch (sc->format) {
> +    case SPICE_SURFACE_FMT_16_555:
> +        qxl->guest_primary.bytes_pp = 2;
> +        qxl->guest_primary.bits_pp = 15;
> +        break;
> +    case SPICE_SURFACE_FMT_16_565:
> +        qxl->guest_primary.bytes_pp = 2;
> +        qxl->guest_primary.bits_pp = 16;
> +        break;
> +    case SPICE_SURFACE_FMT_32_xRGB:
> +    case SPICE_SURFACE_FMT_32_ARGB:
> +        qxl->guest_primary.bytes_pp = 4;
> +        qxl->guest_primary.bits_pp = 32;
> +        break;
> +    default:
> +        fprintf(stderr, "%s: unhandled format: %x\n", __FUNCTION__,
> +                qxl->guest_primary.surface.format);
> +        qxl->guest_primary.bytes_pp = 4;
> +        qxl->guest_primary.bits_pp = 32;
> +        break;
> +    }
> +}
> +
> +void qxl_render_update(PCIQXLDevice *qxl)
> +{
> +    VGACommonState *vga =&qxl->vga;
> +    QXLRect dirty[32], update;
> +    void *ptr;
> +    int i;
> +
> +    if (qxl->guest_primary.resized) {
> +        qxl->guest_primary.resized = 0;
> +
> +        if (qxl->guest_primary.flipped) {
> +            qemu_free(qxl->guest_primary.flipped);
> +            qxl->guest_primary.flipped = NULL;
> +        }
> +        qemu_free_displaysurface(vga->ds);
> +
> +        qxl->guest_primary.data = qemu_get_ram_ptr(qxl->vga.vram_offset);
> +        if (qxl->guest_primary.stride<  0) {
> +            /* spice surface is upside down ->  need extra buffer to flip */
> +            qxl->guest_primary.stride = -qxl->guest_primary.stride;
> +            qxl->guest_primary.flipped = qemu_malloc(qxl->guest_primary.surface.width *
> +                                                     qxl->guest_primary.stride);
> +            ptr = qxl->guest_primary.flipped;
> +        } else {
> +            ptr = qxl->guest_primary.data;
> +        }
> +        dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n",
> +               __FUNCTION__,
> +               qxl->guest_primary.surface.width,
> +               qxl->guest_primary.surface.height,
> +               qxl->guest_primary.stride,
> +               qxl->guest_primary.bytes_pp,
> +               qxl->guest_primary.bits_pp,
> +               qxl->guest_primary.flipped ? "yes" : "no");
> +        vga->ds->surface =
> +            qemu_create_displaysurface_from(qxl->guest_primary.surface.width,
> +                                            qxl->guest_primary.surface.height,
> +                                            qxl->guest_primary.bits_pp,
> +                                            qxl->guest_primary.stride,
> +                                            ptr);
> +        dpy_resize(vga->ds);
> +    }
> +
> +    if (!qxl->guest_primary.commands) {
> +        return;
> +    }
> +    qxl->guest_primary.commands = 0;
> +
> +    update.left   = 0;
> +    update.right  = qxl->guest_primary.surface.width;
> +    update.top    = 0;
> +    update.bottom = qxl->guest_primary.surface.height;
> +
> +    memset(dirty, 0, sizeof(dirty));
> +    qxl->ssd.worker->update_area(qxl->ssd.worker, 0,&update,
> +                                 dirty, ARRAY_SIZE(dirty), 1);
> +
> +    for (i = 0; i<  ARRAY_SIZE(dirty); i++) {
> +        if (qemu_spice_rect_is_empty(dirty+i)) {
> +            break;
> +        }
> +        if (qxl->guest_primary.flipped) {
> +            qxl_flip(qxl, dirty+i);
> +        }
> +        dpy_update(vga->ds,
> +                   dirty[i].left, dirty[i].top,
> +                   dirty[i].right - dirty[i].left,
> +                   dirty[i].bottom - dirty[i].top);
> +    }
> +}
> +
> +static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
> +{
> +    QEMUCursor *c;
> +    uint8_t *image, *mask;
> +    int size;
> +
> +    c = cursor_alloc(cursor->header.width, cursor->header.height);
> +    c->hot_x = cursor->header.hot_spot_x;
> +    c->hot_y = cursor->header.hot_spot_y;
> +    switch (cursor->header.type) {
> +    case SPICE_CURSOR_TYPE_ALPHA:
> +        size = cursor->header.width * cursor->header.height * sizeof(uint32_t);
> +        memcpy(c->data, cursor->chunk.data, size);
> +        if (qxl->debug>  2) {
> +            cursor_print_ascii_art(c, "qxl/alpha");
> +        }
> +        break;
> +    case SPICE_CURSOR_TYPE_MONO:
> +        mask  = cursor->chunk.data;
> +        image = mask + cursor_get_mono_bpl(c) * c->width;
> +        cursor_set_mono(c, 0xffffff, 0x000000, image, 1, mask);
> +        if (qxl->debug>  2) {
> +            cursor_print_ascii_art(c, "qxl/mono");
> +        }
> +        break;
> +    default:
> +        fprintf(stderr, "%s: not implemented: type %d\n",
> +                __FUNCTION__, cursor->header.type);
> +        goto fail;
> +    }
> +    return c;
> +
> +fail:
> +    cursor_put(c);
> +    return NULL;
> +}
> +
> +
> +/* called from spice server thread context only */
> +void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
> +{
> +    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +    QXLCursor *cursor;
> +    QEMUCursor *c;
> +    int x = -1, y = -1;
> +
> +    if (!qxl->ssd.ds->mouse_set || !qxl->ssd.ds->cursor_define) {
> +        return;
> +    }
> +
> +    if (qxl->debug>  1&&  cmd->type != QXL_CURSOR_MOVE) {
> +        fprintf(stderr, "%s", __FUNCTION__);
> +        qxl_log_cmd_cursor(qxl, cmd, ext->group_id);
> +        fprintf(stderr, "\n");
> +    }
> +    switch (cmd->type) {
> +    case QXL_CURSOR_SET:
> +        x = cmd->u.set.position.x;
> +        y = cmd->u.set.position.y;
> +        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
> +        if (cursor->chunk.data_size != cursor->data_size) {
> +            fprintf(stderr, "%s: multiple chunks\n", __FUNCTION__);
> +            return;
> +        }
> +        c = qxl_cursor(qxl, cursor);
> +        if (c == NULL) {
> +            c = cursor_builtin_left_ptr();
> +        }
> +        qemu_mutex_lock_iothread();
> +        qxl->ssd.ds->cursor_define(c);
> +        qxl->ssd.ds->mouse_set(x, y, 1);
> +        qemu_mutex_unlock_iothread();
> +        cursor_put(c);
> +        break;
> +    case QXL_CURSOR_MOVE:
> +        x = cmd->u.position.x;
> +        y = cmd->u.position.y;
> +        qemu_mutex_lock_iothread();
> +        qxl->ssd.ds->mouse_set(x, y, 1);
> +        qemu_mutex_unlock_iothread();
> +        break;
> +    }
> +}
> diff --git a/hw/qxl.c b/hw/qxl.c
> new file mode 100644
> index 0000000..e9bf804
> --- /dev/null
> +++ b/hw/qxl.c
> @@ -0,0 +1,1552 @@
> +#include<stdio.h>
> +#include<stdlib.h>
> +#include<stdbool.h>
> +#include<stdint.h>
> +#include<string.h>
> +#include<pthread.h>
> +
> +#include "qemu-common.h"
> +#include "qemu-timer.h"
> +#include "qemu-queue.h"
> +#include "monitor.h"
> +#include "sysemu.h"
> +
> +#include "qxl.h"
> +
> +#undef SPICE_RING_PROD_ITEM
> +#define SPICE_RING_PROD_ITEM(r, ret) {                                  \
> +        typeof(r) start = r;                                            \
> +        typeof(r) end = r + 1;                                          \
> +        uint32_t prod = (r)->prod&  SPICE_RING_INDEX_MASK(r);           \
> +        typeof(&(r)->items[prod]) m_item =&(r)->items[prod];           \
> +        if (!((uint8_t*)m_item>= (uint8_t*)(start)&&  (uint8_t*)(m_item + 1)<= (uint8_t*)(end))) { \
> +            abort();                                                    \
> +        }                                                               \
> +        ret =&m_item->el;                                              \
> +    }
> +
> +#undef SPICE_RING_CONS_ITEM
> +#define SPICE_RING_CONS_ITEM(r, ret) {                                  \
> +        typeof(r) start = r;                                            \
> +        typeof(r) end = r + 1;                                          \
> +        uint32_t cons = (r)->cons&  SPICE_RING_INDEX_MASK(r);           \
> +        typeof(&(r)->items[cons]) m_item =&(r)->items[cons];           \
> +        if (!((uint8_t*)m_item>= (uint8_t*)(start)&&  (uint8_t*)(m_item + 1)<= (uint8_t*)(end))) { \
> +            abort();                                                    \
> +        }                                                               \
> +        ret =&m_item->el;                                              \
> +    }
> +
> +#undef ALIGN
> +#define ALIGN(a, b) (((a) + ((b) - 1))&  ~((b) - 1))
> +
> +#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9"
> +
> +#define QXL_MODE(_x, _y, _b, _o)                  \
> +    {   .x_res = _x,                              \
> +        .y_res = _y,                              \
> +        .bits  = _b,                              \
> +        .stride = (_x) * (_b) / 8,                \
> +        .x_mili = PIXEL_SIZE * (_x),              \
> +        .y_mili = PIXEL_SIZE * (_y),              \
> +        .orientation = _o,                        \
> +    }
> +
> +#define QXL_MODE_16_32(x_res, y_res, orientation) \
> +    QXL_MODE(x_res, y_res, 16, orientation),      \
> +    QXL_MODE(x_res, y_res, 32, orientation)
> +
> +#define QXL_MODE_EX(x_res, y_res)                 \
> +    QXL_MODE_16_32(x_res, y_res, 0),              \
> +    QXL_MODE_16_32(y_res, x_res, 1),              \
> +    QXL_MODE_16_32(x_res, y_res, 2),              \
> +    QXL_MODE_16_32(y_res, x_res, 3)
> +
> +static QXLMode qxl_modes[] = {
> +    QXL_MODE_EX(640, 480),
> +    QXL_MODE_EX(800, 480),
> +    QXL_MODE_EX(800, 600),
> +    QXL_MODE_EX(832, 624),
> +    QXL_MODE_EX(960, 640),
> +    QXL_MODE_EX(1024, 600),
> +    QXL_MODE_EX(1024, 768),
> +    QXL_MODE_EX(1152, 864),
> +    QXL_MODE_EX(1152, 870),
> +    QXL_MODE_EX(1280, 720),
> +    QXL_MODE_EX(1280, 760),
> +    QXL_MODE_EX(1280, 768),
> +    QXL_MODE_EX(1280, 800),
> +    QXL_MODE_EX(1280, 960),
> +    QXL_MODE_EX(1280, 1024),
> +    QXL_MODE_EX(1360, 768),
> +    QXL_MODE_EX(1366, 768),
> +    QXL_MODE_EX(1400, 1050),
> +    QXL_MODE_EX(1440, 900),
> +    QXL_MODE_EX(1600, 900),
> +    QXL_MODE_EX(1600, 1200),
> +    QXL_MODE_EX(1680, 1050),
> +    QXL_MODE_EX(1920, 1080),
> +#if VGA_RAM_SIZE>= (16 * 1024 * 1024)
> +    /* these modes need more than 8 MB video memory */
> +    QXL_MODE_EX(1920, 1200),
> +    QXL_MODE_EX(1920, 1440),
> +    QXL_MODE_EX(2048, 1536),
> +    QXL_MODE_EX(2560, 1440),
> +    QXL_MODE_EX(2560, 1600),
> +#endif
> +#if VGA_RAM_SIZE>= (32 * 1024 * 1024)
> +    /* these modes need more than 16 MB video memory */
> +    QXL_MODE_EX(2560, 2048),
> +    QXL_MODE_EX(2800, 2100),
> +    QXL_MODE_EX(3200, 2400),
> +#endif
> +};
> +
> +static int device_id = 0;
> +static PCIQXLDevice *qxl0;
> +
> +static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
> +static void qxl_destroy_primary(PCIQXLDevice *d);
> +static void qxl_reset_memslots(PCIQXLDevice *d);
> +static void qxl_reset_surfaces(PCIQXLDevice *d);
> +static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
> +
> +static inline uint32_t msb_mask(uint32_t val)
> +{
> +    uint32_t mask;
> +
> +    do {
> +        mask = ~(val - 1)&  val;
> +        val&= ~mask;
> +    } while (mask<  val);
> +
> +    return mask;
> +}
> +
> +static ram_addr_t qxl_rom_size(void)
> +{
> +    uint32_t rom_size = sizeof(QXLRom) + sizeof(QXLModes) + sizeof(qxl_modes);
> +    rom_size = MAX(rom_size, TARGET_PAGE_SIZE);
> +    rom_size = msb_mask(rom_size * 2 - 1);
> +    return rom_size;
> +}
> +
> +static void init_qxl_rom(PCIQXLDevice *d)
> +{
>    

You don't really mean ROM, right.  This is a set of configuration 
registers exposed to the guest via PCI, right?

> +    QXLRom *rom = qemu_get_ram_ptr(d->rom_offset);
>    

This function should not be used in a device model.

> +    QXLModes *modes = (QXLModes *)(rom + 1);
> +    uint32_t ram_header_size;
> +    uint32_t surface0_area_size;
> +    uint32_t num_pages;
> +    uint32_t fb, maxfb = 0;
> +    int i;
> +
> +    memset(rom, 0, d->rom_size);
> +
> +    rom->magic         = cpu_to_le32(QXL_ROM_MAGIC);
> +    rom->id            = cpu_to_le32(d->id);
> +    rom->log_level     = cpu_to_le32(d->guestdebug);
> +    rom->modes_offset  = cpu_to_le32(sizeof(QXLRom));
> +
> +    rom->slot_gen_bits = MEMSLOT_GENERATION_BITS;
> +    rom->slot_id_bits  = MEMSLOT_SLOT_BITS;
> +    rom->slots_start   = 1;
> +    rom->slots_end     = NUM_MEMSLOTS - 1;
> +    rom->n_surfaces    = cpu_to_le32(NUM_SURFACES);
> +
> +    modes->n_modes     = cpu_to_le32(ARRAY_SIZE(qxl_modes));
> +    for (i = 0; i<  modes->n_modes; i++) {
> +        fb = qxl_modes[i].y_res * qxl_modes[i].stride;
> +        if (maxfb<  fb) {
> +            maxfb = fb;
> +        }
> +        modes->modes[i].id          = cpu_to_le32(i);
> +        modes->modes[i].x_res       = cpu_to_le32(qxl_modes[i].x_res);
> +        modes->modes[i].y_res       = cpu_to_le32(qxl_modes[i].y_res);
> +        modes->modes[i].bits        = cpu_to_le32(qxl_modes[i].bits);
> +        modes->modes[i].stride      = cpu_to_le32(qxl_modes[i].stride);
> +        modes->modes[i].x_mili      = cpu_to_le32(qxl_modes[i].x_mili);
> +        modes->modes[i].y_mili      = cpu_to_le32(qxl_modes[i].y_mili);
> +        modes->modes[i].orientation = cpu_to_le32(qxl_modes[i].orientation);
> +    }
> +    if (maxfb<  VGA_RAM_SIZE&&  d->id == 0)
> +        maxfb = VGA_RAM_SIZE;
> +
> +    ram_header_size    = ALIGN(sizeof(QXLRam), 4096);
> +    surface0_area_size = ALIGN(maxfb, 4096);
> +    num_pages          = d->vga.vram_size;
> +    num_pages         -= ram_header_size;
> +    num_pages         -= surface0_area_size;
> +    num_pages          = num_pages / TARGET_PAGE_SIZE;
> +
> +    rom->draw_area_offset   = cpu_to_le32(0);
> +    rom->surface0_area_size = cpu_to_le32(surface0_area_size);
> +    rom->pages_offset       = cpu_to_le32(surface0_area_size);
> +    rom->num_pages          = cpu_to_le32(num_pages);
> +    rom->ram_header_offset  = cpu_to_le32(d->vga.vram_size - ram_header_size);
> +
> +    d->shadow_rom = *rom;
> +    d->rom        = rom;
> +    d->modes      = modes;
> +}
> +
> +static void init_qxl_ram(PCIQXLDevice *d)
> +{
> +    uint8_t *buf;
> +    uint64_t *item;
> +
> +    buf = d->vga.vram_ptr;
> +    d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
> +    d->ram->magic       = cpu_to_le32(QXL_RAM_MAGIC);
> +    d->ram->int_pending = cpu_to_le32(0);
> +    d->ram->int_mask    = cpu_to_le32(0);
> +    SPICE_RING_INIT(&d->ram->cmd_ring);
> +    SPICE_RING_INIT(&d->ram->cursor_ring);
> +    SPICE_RING_INIT(&d->ram->release_ring);
> +    SPICE_RING_PROD_ITEM(&d->ram->release_ring, item);
> +    *item = 0;
> +    qxl_ring_set_dirty(d);
> +}
> +
> +/* can be called from spice server thread context */
> +static void qxl_set_dirty(ram_addr_t addr, ram_addr_t end)
> +{
> +    while (addr<  end) {
> +        cpu_physical_memory_set_dirty(addr);
> +        addr += TARGET_PAGE_SIZE;
> +    }
> +}
> +
> +static void qxl_rom_set_dirty(PCIQXLDevice *qxl)
> +{
> +    ram_addr_t addr = qxl->rom_offset;
> +    qxl_set_dirty(addr, addr + qxl->rom_size);
> +}
> +
> +/* called from spice server thread context only */
> +static void qxl_ram_set_dirty(PCIQXLDevice *qxl, void *ptr)
> +{
> +    ram_addr_t addr = qxl->vga.vram_offset;
> +    void *base = qxl->vga.vram_ptr;
> +    intptr_t offset;
> +
> +    offset = ptr - base;
> +    offset&= ~(TARGET_PAGE_SIZE-1);
> +    assert(offset<  qxl->vga.vram_size);
> +    qxl_set_dirty(addr + offset, addr + offset + TARGET_PAGE_SIZE);
> +}
> +
> +/* can be called from spice server thread context */
> +static void qxl_ring_set_dirty(PCIQXLDevice *qxl)
> +{
> +    ram_addr_t addr = qxl->vga.vram_offset + qxl->shadow_rom.ram_header_offset;
> +    ram_addr_t end  = qxl->vga.vram_offset + qxl->vga.vram_size;
> +    qxl_set_dirty(addr, end);
> +}
> +
> +/*
> + * keep track of some command state, for savevm/loadvm.
> + * called from spice server thread context only
> + */
> +static void qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
> +{
> +    switch (le32_to_cpu(ext->cmd.type)) {
> +    case QXL_CMD_SURFACE:
> +    {
> +        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +        uint32_t id = le32_to_cpu(cmd->surface_id);
> +        PANIC_ON(id>= NUM_SURFACES);
> +        if (cmd->type == QXL_SURFACE_CMD_CREATE) {
> +            qxl->guest_surfaces.cmds[id] = ext->cmd.data;
> +            qxl->guest_surfaces.count++;
> +            if (qxl->guest_surfaces.max<  qxl->guest_surfaces.count)
> +                qxl->guest_surfaces.max = qxl->guest_surfaces.count;
> +        }
> +        if (cmd->type == QXL_SURFACE_CMD_DESTROY) {
> +            qxl->guest_surfaces.cmds[id] = 0;
> +            qxl->guest_surfaces.count--;
> +        }
> +        break;
> +    }
> +    case QXL_CMD_CURSOR:
> +    {
> +        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +        if (cmd->type == QXL_CURSOR_SET) {
> +            qxl->guest_cursor = ext->cmd.data;
> +        }
> +        break;
> +    }
> +    }
> +}
> +
> +/* spice display interface callbacks */
> +
> +static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +
> +    dprint(qxl, 1, "%s:\n", __FUNCTION__);
> +    qxl->ssd.worker = qxl_worker;
> +}
> +
> +static void interface_set_compression_level(QXLInstance *sin, int level)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +
> +    dprint(qxl, 1, "%s: %d\n", __FUNCTION__, level);
> +    qxl->shadow_rom.compression_level = cpu_to_le32(level);
> +    qxl->rom->compression_level = cpu_to_le32(level);
> +    qxl_rom_set_dirty(qxl);
> +}
> +
> +static void interface_set_mm_time(QXLInstance *sin, uint32_t mm_time)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +
> +    qxl->shadow_rom.mm_clock = cpu_to_le32(mm_time);
> +    qxl->rom->mm_clock = cpu_to_le32(mm_time);
> +    qxl_rom_set_dirty(qxl);
> +}
> +
> +static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +
> +    dprint(qxl, 1, "%s:\n", __FUNCTION__);
> +    info->memslot_gen_bits = MEMSLOT_GENERATION_BITS;
> +    info->memslot_id_bits = MEMSLOT_SLOT_BITS;
> +    info->num_memslots = NUM_MEMSLOTS;
> +    info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
> +    info->internal_groupslot_id = 0;
> +    info->qxl_ram_size = le32_to_cpu(qxl->shadow_rom.num_pages)<<  TARGET_PAGE_BITS;
> +    info->n_surfaces = NUM_SURFACES;
> +}
> +
> +/* called from spice server thread context only */
> +static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    SimpleSpiceUpdate *update;
> +    QXLCommandRing *ring;
> +    QXLCommand *cmd;
> +    int notify;
> +
> +    switch (qxl->mode) {
> +    case QXL_MODE_VGA:
> +        dprint(qxl, 2, "%s: vga\n", __FUNCTION__);
> +        update = qemu_spice_create_update(&qxl->ssd);
> +        if (update == NULL) {
> +            return false;
> +        }
> +        *ext = update->ext;
> +        qxl_log_command(qxl, "vga", ext);
> +        return true;
> +    case QXL_MODE_COMPAT:
> +    case QXL_MODE_NATIVE:
> +    case QXL_MODE_UNDEFINED:
> +        dprint(qxl, 2, "%s: %s\n", __FUNCTION__,
> +               qxl->cmdflags ? "compat" : "native");
> +        ring =&qxl->ram->cmd_ring;
> +        if (SPICE_RING_IS_EMPTY(ring)) {
> +            return false;
> +        }
> +        SPICE_RING_CONS_ITEM(ring, cmd);
> +        ext->cmd      = *cmd;
> +        ext->group_id = MEMSLOT_GROUP_GUEST;
> +        ext->flags    = qxl->cmdflags;
> +        SPICE_RING_POP(ring, notify);
> +        qxl_ring_set_dirty(qxl);
> +        if (notify) {
> +            qxl_send_events(qxl, QXL_INTERRUPT_DISPLAY);
> +        }
> +        qxl->guest_primary.commands++;
> +        qxl_track_command(qxl, ext);
> +        qxl_log_command(qxl, "cmd", ext);
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
> +/* called from spice server thread context only */
> +static int interface_req_cmd_notification(QXLInstance *sin)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    int wait = 1;
> +
> +    switch (qxl->mode) {
> +    case QXL_MODE_COMPAT:
> +    case QXL_MODE_NATIVE:
> +    case QXL_MODE_UNDEFINED:
> +        SPICE_RING_CONS_WAIT(&qxl->ram->cmd_ring, wait);
> +        qxl_ring_set_dirty(qxl);
> +        break;
> +    default:
> +        /* nothing */
> +        break;
> +    }
> +    return wait;
> +}
> +
> +/* called from spice server thread context only */
> +static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
> +{
> +    QXLReleaseRing *ring =&d->ram->release_ring;
> +    uint64_t *item;
> +    int notify;
> +
> +#define QXL_FREE_BUNCH_SIZE 32
> +
> +    if (ring->prod - ring->cons + 1 == ring->num_items) {
> +        /* ring full -- can't push */
> +        return;
> +    }
> +    if (!flush&&  d->oom_running) {
> +        /* collect everything from oom handler before pushing */
> +        return;
> +    }
> +    if (!flush&&  d->num_free_res<  QXL_FREE_BUNCH_SIZE) {
> +        /* collect a bit more before pushing */
> +        return;
> +    }
> +
> +    SPICE_RING_PUSH(ring, notify);
> +    dprint(d, 2, "free: push %d items, notify %s, ring %d/%d [%d,%d]\n",
> +           d->num_free_res, notify ? "yes" : "no",
> +           ring->prod - ring->cons, ring->num_items,
> +           ring->prod, ring->cons);
> +    if (notify) {
> +        qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
> +    }
> +    SPICE_RING_PROD_ITEM(ring, item);
> +    *item = 0;
> +    d->num_free_res = 0;
> +    d->last_release = NULL;
> +    qxl_ring_set_dirty(d);
> +}
> +
> +/* called from spice server thread context only */
> +static void interface_release_resource(QXLInstance *sin,
> +                                       struct QXLReleaseInfoExt ext)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    QXLReleaseRing *ring;
> +    uint64_t *item, id;
> +
> +    if (ext.group_id == MEMSLOT_GROUP_HOST) {
> +        /* host group ->  vga mode update request */
> +        qemu_spice_destroy_update(&qxl->ssd, (void*)ext.info->id);
> +        return;
> +    }
> +
> +    /*
> +     * ext->info points into guest-visible memory
> +     * pci bar 0, $command.release_info
> +     */
> +    ring =&qxl->ram->release_ring;
> +    SPICE_RING_PROD_ITEM(ring, item);
> +    if (*item == 0) {
> +        /* stick head into the ring */
> +        id = ext.info->id;
> +        ext.info->next = 0;
> +        qxl_ram_set_dirty(qxl,&ext.info->next);
> +        *item = id;
> +        qxl_ring_set_dirty(qxl);
> +    } else {
> +        /* append item to the list */
> +        qxl->last_release->next = ext.info->id;
> +        qxl_ram_set_dirty(qxl,&qxl->last_release->next);
> +        ext.info->next = 0;
> +        qxl_ram_set_dirty(qxl,&ext.info->next);
> +    }
> +    qxl->last_release = ext.info;
> +    qxl->num_free_res++;
> +    dprint(qxl, 3, "%4d\r", qxl->num_free_res);
> +    qxl_push_free_res(qxl, 0);
> +}
> +
> +/* called from spice server thread context only */
> +static int interface_get_cursor_command(QXLInstance *sin, struct QXLCommandExt *ext)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    QXLCursorRing *ring;
> +    QXLCommand *cmd;
> +    int notify;
> +
> +    switch (qxl->mode) {
> +    case QXL_MODE_COMPAT:
> +    case QXL_MODE_NATIVE:
> +    case QXL_MODE_UNDEFINED:
> +        ring =&qxl->ram->cursor_ring;
> +        if (SPICE_RING_IS_EMPTY(ring)) {
> +            return false;
> +        }
> +        SPICE_RING_CONS_ITEM(ring, cmd);
> +        ext->cmd      = *cmd;
> +        ext->group_id = MEMSLOT_GROUP_GUEST;
> +        ext->flags    = qxl->cmdflags;
> +        SPICE_RING_POP(ring, notify);
> +        qxl_ring_set_dirty(qxl);
> +        if (notify) {
> +            qxl_send_events(qxl, QXL_INTERRUPT_CURSOR);
> +        }
> +        qxl->guest_primary.commands++;
> +        qxl_track_command(qxl, ext);
> +        qxl_log_command(qxl, "csr", ext);
> +        if (qxl->id == 0) {
> +            qxl_render_cursor(qxl, ext);
> +        }
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
> +/* called from spice server thread context only */
> +static int interface_req_cursor_notification(QXLInstance *sin)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    int wait = 1;
> +
> +    switch (qxl->mode) {
> +    case QXL_MODE_COMPAT:
> +    case QXL_MODE_NATIVE:
> +    case QXL_MODE_UNDEFINED:
> +        SPICE_RING_CONS_WAIT(&qxl->ram->cursor_ring, wait);
> +        qxl_ring_set_dirty(qxl);
> +        break;
> +    default:
> +        /* nothing */
> +        break;
> +    }
> +    return wait;
> +}
> +
> +/* called from spice server thread context */
> +static void interface_notify_update(QXLInstance *sin, uint32_t update_id)
> +{
> +    fprintf(stderr, "%s: abort()\n", __FUNCTION__);
> +    abort();
> +}
> +
> +/* called from spice server thread context only */
> +static int interface_flush_resources(QXLInstance *sin)
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +    int ret;
> +
> +    dprint(qxl, 1, "free: guest flush (have %d)\n", qxl->num_free_res);
> +    ret = qxl->num_free_res;
> +    if (ret) {
> +        qxl_push_free_res(qxl, 1);
> +    }
> +    return ret;
> +}
> +
> +static const QXLInterface qxl_interface = {
> +    .base.type               = SPICE_INTERFACE_QXL,
> +    .base.description        = "qxl gpu",
> +    .base.major_version      = SPICE_INTERFACE_QXL_MAJOR,
> +    .base.minor_version      = SPICE_INTERFACE_QXL_MINOR,
> +
> +    .attache_worker          = interface_attach_worker,
> +    .set_compression_level   = interface_set_compression_level,
> +    .set_mm_time             = interface_set_mm_time,
> +    .get_init_info           = interface_get_init_info,
> +
> +    /* the callbacks below are called from spice server thread context */
> +    .get_command             = interface_get_command,
> +    .req_cmd_notification    = interface_req_cmd_notification,
> +    .release_resource        = interface_release_resource,
> +    .get_cursor_command      = interface_get_cursor_command,
> +    .req_cursor_notification = interface_req_cursor_notification,
> +    .notify_update           = interface_notify_update,
> +    .flush_resources         = interface_flush_resources,
> +};
> +
> +static void qxl_enter_vga_mode(PCIQXLDevice *d)
> +{
> +    if (d->mode == QXL_MODE_VGA) {
> +        return;
> +    }
> +    dprint(d, 1, "%s\n", __FUNCTION__);
> +    qemu_spice_create_host_primary(&d->ssd);
> +    d->mode = QXL_MODE_VGA;
> +    memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
> +}
> +
> +static void qxl_exit_vga_mode(PCIQXLDevice *d)
> +{
> +    if (d->mode != QXL_MODE_VGA) {
> +        return;
> +    }
> +    dprint(d, 1, "%s\n", __FUNCTION__);
> +    qxl_destroy_primary(d);
> +}
> +
> +static void qxl_set_irq(PCIQXLDevice *d)
> +{
> +    uint32_t pending = le32_to_cpu(d->ram->int_pending);
> +    uint32_t mask    = le32_to_cpu(d->ram->int_mask);
> +    int level = !!(pending&  mask);
> +    qemu_set_irq(d->pci.irq[0], level);
> +    qxl_ring_set_dirty(d);
> +}
> +
> +static void qxl_write_config(PCIDevice *d, uint32_t address,
> +                             uint32_t val, int len)
> +{
> +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, d);
> +    VGACommonState *vga =&qxl->vga;
> +
> +    if (qxl->id == 0) {
> +        vga_dirty_log_stop(vga);
> +    }
> +    pci_default_write_config(d, address, val, len);
> +    if (qxl->id == 0) {
> +        if (vga->map_addr&&  qxl->pci.io_regions[0].addr == -1)
> +            vga->map_addr = 0;
> +        vga_dirty_log_start(vga);
> +    }
> +}
> +
> +static void qxl_check_state(PCIQXLDevice *d)
> +{
> +    QXLRam *ram = d->ram;
> +
> +    assert(SPICE_RING_IS_EMPTY(&ram->cmd_ring));
> +    assert(SPICE_RING_IS_EMPTY(&ram->cursor_ring));
> +}
> +
> +static void qxl_reset_state(PCIQXLDevice *d)
> +{
> +    QXLRam *ram = d->ram;
> +    QXLRom *rom = d->rom;
> +
> +    assert(SPICE_RING_IS_EMPTY(&ram->cmd_ring));
> +    assert(SPICE_RING_IS_EMPTY(&ram->cursor_ring));
> +    d->shadow_rom.update_id = cpu_to_le32(0);
> +    *rom = d->shadow_rom;
> +    qxl_rom_set_dirty(d);
> +    init_qxl_ram(d);
> +    d->num_free_res = 0;
> +    d->last_release = NULL;
> +    memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
> +}
> +
> +static void qxl_soft_reset(PCIQXLDevice *d)
> +{
> +    dprint(d, 1, "%s:\n", __FUNCTION__);
> +    qxl_check_state(d);
> +
> +    if (d->id == 0) {
> +        qxl_enter_vga_mode(d);
> +    } else {
> +        d->mode = QXL_MODE_UNDEFINED;
> +    }
> +}
> +
> +static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
> +{
> +    dprint(d, 1, "%s: start%s\n", __FUNCTION__,
> +           loadvm ? " (loadvm)" : "");
> +
> +    qemu_mutex_unlock_iothread();
> +    d->ssd.worker->reset_cursor(d->ssd.worker);
> +    d->ssd.worker->reset_image_cache(d->ssd.worker);
> +    qemu_mutex_lock_iothread();
> +    qxl_reset_surfaces(d);
> +    qxl_reset_memslots(d);
> +
> +    /* pre loadvm reset must not touch QXLRam.  This lives in
> +     * device memory, is migrated together with RAM and thus
> +     * already loaded at this point */
> +    if (!loadvm) {
> +        qxl_reset_state(d);
> +    }
> +    qemu_spice_create_host_memslot(&d->ssd);
> +    qxl_soft_reset(d);
> +
> +    dprint(d, 1, "%s: done\n", __FUNCTION__);
> +}
> +
> +static void qxl_reset_handler(DeviceState *dev)
> +{
> +    PCIQXLDevice *d = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> +    qxl_hard_reset(d, 0);
> +}
> +
> +static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    VGACommonState *vga = opaque;
> +    PCIQXLDevice *qxl = container_of(vga, PCIQXLDevice, vga);
> +
> +    if (qxl->mode != QXL_MODE_VGA) {
> +        dprint(qxl, 1, "%s\n", __FUNCTION__);
> +        qxl_destroy_primary(qxl);
> +        qxl_soft_reset(qxl);
> +    }
> +    vga_ioport_write(opaque, addr, val);
> +}
> +
> +static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta)
> +{
> +    static const int regions[] = {
> +        QXL_RAM_RANGE_INDEX,
> +        QXL_VRAM_RANGE_INDEX,
> +    };
> +    uint64_t guest_start;
> +    uint64_t guest_end;
> +    int pci_region;
> +    pcibus_t pci_start;
> +    pcibus_t pci_end;
> +    intptr_t virt_start;
> +    QXLDevMemSlot memslot;
> +    int i;
> +
> +    guest_start = le64_to_cpu(d->guest_slots[slot_id].slot.mem_start);
> +    guest_end   = le64_to_cpu(d->guest_slots[slot_id].slot.mem_end);
> +
> +    dprint(d, 1, "%s: slot %d: guest phys 0x%" PRIx64 " - 0x%" PRIx64 "\n",
> +           __FUNCTION__, slot_id,
> +           guest_start, guest_end);
> +
> +    PANIC_ON(slot_id>= NUM_MEMSLOTS);
> +    PANIC_ON(guest_start>  guest_end);
> +
> +    for (i = 0; i<  ARRAY_SIZE(regions); i++) {
> +        pci_region = regions[i];
> +        pci_start = d->pci.io_regions[pci_region].addr;
> +        pci_end = pci_start + d->pci.io_regions[pci_region].size;
> +        /* mapped? */
> +        if (pci_start == -1) {
> +            continue;
> +        }
> +        /* start address in range ? */
> +        if (guest_start<  pci_start || guest_start>  pci_end) {
> +            continue;
> +        }
> +        /* end address in range ? */
> +        if (guest_end>  pci_end) {
> +            continue;
> +        }
> +        /* passed */
> +        break;
> +    }
> +    PANIC_ON(i == ARRAY_SIZE(regions)); /* finished loop without match */
> +
> +    switch (pci_region) {
> +    case QXL_RAM_RANGE_INDEX:
> +        virt_start = (intptr_t)qemu_get_ram_ptr(d->vga.vram_offset);
> +        break;
> +    case QXL_VRAM_RANGE_INDEX:
> +        virt_start = (intptr_t)qemu_get_ram_ptr(d->vram_offset);
> +        break;
> +    default:
> +        /* should not happen */
> +        abort();
> +    }
> +
> +    memslot.slot_id = slot_id;
> +    memslot.slot_group_id = MEMSLOT_GROUP_GUEST; /* guest group */
> +    memslot.virt_start = virt_start + (guest_start - pci_start);
> +    memslot.virt_end   = virt_start + (guest_end   - pci_start);
> +    memslot.addr_delta = memslot.virt_start - delta;
> +    memslot.generation = d->rom->slot_generation = 0;
> +    qxl_rom_set_dirty(d);
> +
> +    dprint(d, 1, "%s: slot %d: host virt 0x%" PRIx64 " - 0x%" PRIx64 "\n",
> +           __FUNCTION__, memslot.slot_id,
> +           memslot.virt_start, memslot.virt_end);
> +
> +    d->ssd.worker->add_memslot(d->ssd.worker,&memslot);
> +    d->guest_slots[slot_id].ptr = (void*)memslot.virt_start;
> +    d->guest_slots[slot_id].size = memslot.virt_end - memslot.virt_start;
> +    d->guest_slots[slot_id].delta = delta;
> +    d->guest_slots[slot_id].active = 1;
> +}
> +
> +static void qxl_del_memslot(PCIQXLDevice *d, uint32_t slot_id)
> +{
> +    dprint(d, 1, "%s: slot %d\n", __FUNCTION__, slot_id);
> +    d->ssd.worker->del_memslot(d->ssd.worker, MEMSLOT_GROUP_HOST, slot_id);
> +    d->guest_slots[slot_id].active = 0;
> +}
> +
> +static void qxl_reset_memslots(PCIQXLDevice *d)
> +{
> +    dprint(d, 1, "%s:\n", __FUNCTION__);
> +    d->ssd.worker->reset_memslots(d->ssd.worker);
> +    memset(&d->guest_slots, 0, sizeof(d->guest_slots));
> +}
> +
> +static void qxl_reset_surfaces(PCIQXLDevice *d)
> +{
> +    dprint(d, 1, "%s:\n", __FUNCTION__);
> +    d->mode = QXL_MODE_UNDEFINED;
> +    qemu_mutex_unlock_iothread();
> +    d->ssd.worker->destroy_surfaces(d->ssd.worker);
> +    qemu_mutex_lock_iothread();
> +    memset(&d->guest_surfaces.cmds, 0, sizeof(d->guest_surfaces.cmds));
> +}
> +
> +/* called from spice server thread context only */
> +void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
> +{
> +    uint64_t phys   = le64_to_cpu(pqxl);
> +    uint32_t slot   = (phys>>  (64 -  8))&  0xff;
> +    uint64_t offset = phys&  0xffffffffffff;
> +
> +    switch (group_id) {
> +    case MEMSLOT_GROUP_HOST:
> +        return (void*)offset;
> +    case MEMSLOT_GROUP_GUEST:
> +        PANIC_ON(slot>  NUM_MEMSLOTS);
> +        PANIC_ON(!qxl->guest_slots[slot].active);
> +        PANIC_ON(offset<  qxl->guest_slots[slot].delta);
> +        offset -= qxl->guest_slots[slot].delta;
> +        PANIC_ON(offset>  qxl->guest_slots[slot].size)
> +        return qxl->guest_slots[slot].ptr + offset;
> +    default:
> +        PANIC_ON(1);
> +    }
> +}
> +
> +static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm)
> +{
> +    QXLDevSurfaceCreate surface;
> +    QXLSurfaceCreate *sc =&qxl->guest_primary.surface;
> +
> +    assert(qxl->mode != QXL_MODE_NATIVE);
> +    qxl_exit_vga_mode(qxl);
> +
> +    dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__,
> +           le32_to_cpu(sc->width), le32_to_cpu(sc->height));
> +
> +    surface.format     = le32_to_cpu(sc->format);
> +    surface.height     = le32_to_cpu(sc->height);
> +    surface.mem        = le64_to_cpu(sc->mem);
> +    surface.position   = le32_to_cpu(sc->position);
> +    surface.stride     = le32_to_cpu(sc->stride);
> +    surface.width      = le32_to_cpu(sc->width);
> +    surface.type       = le32_to_cpu(sc->type);
> +    surface.flags      = le32_to_cpu(sc->flags);
> +
> +    surface.mouse_mode = true;
> +    surface.group_id   = MEMSLOT_GROUP_GUEST;
> +    if (loadvm) {
> +        surface.flags |= QXL_SURF_FLAG_KEEP_DATA;
> +    }
> +
> +    qxl->mode = QXL_MODE_NATIVE;
> +    qxl->cmdflags = 0;
> +    qxl->ssd.worker->create_primary_surface(qxl->ssd.worker, 0,&surface);
> +
> +    /* for local rendering */
> +    qxl_render_resize(qxl);
> +}
> +
> +static void qxl_destroy_primary(PCIQXLDevice *d)
> +{
> +    if (d->mode == QXL_MODE_UNDEFINED) {
> +        return;
> +    }
> +
> +    dprint(d, 1, "%s\n", __FUNCTION__);
> +
> +    d->mode = QXL_MODE_UNDEFINED;
> +    d->ssd.worker->destroy_primary_surface(d->ssd.worker, 0);
> +}
> +
> +static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
> +{
> +    pcibus_t start = d->pci.io_regions[QXL_RAM_RANGE_INDEX].addr;
> +    pcibus_t end   = d->pci.io_regions[QXL_RAM_RANGE_INDEX].size + start;
> +    QXLMode *mode = d->modes->modes + modenr;
> +    uint64_t devmem = d->pci.io_regions[QXL_RAM_RANGE_INDEX].addr;
> +    QXLMemSlot slot = {
> +        .mem_start = start,
> +        .mem_end = end
> +    };
> +    QXLSurfaceCreate surface = {
> +        .width      = mode->x_res,
> +        .height     = mode->y_res,
> +        .stride     = -mode->x_res * 4,
> +        .format     = SPICE_SURFACE_FMT_32_xRGB,
> +        .flags      = loadvm ? QXL_SURF_FLAG_KEEP_DATA : 0,
> +        .mouse_mode = true,
> +        .mem        = devmem + d->shadow_rom.draw_area_offset,
> +    };
> +
> +    dprint(d, 1, "%s: mode %d  [ %d x %d @ %d bpp devmem 0x%lx ]\n", __FUNCTION__,
> +           modenr, mode->x_res, mode->y_res, mode->bits, devmem);
> +    if (!loadvm) {
> +        qxl_hard_reset(d, 0);
> +    }
> +
> +    d->guest_slots[0].slot = slot;
> +    qxl_add_memslot(d, 0, devmem);
> +
> +    d->guest_primary.surface = surface;
> +    qxl_create_guest_primary(d, 0);
> +
> +    d->mode = QXL_MODE_COMPAT;
> +    d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
> +#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
> +    if (mode->bits == 16) {
> +        d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
> +    }
> +#endif
> +    d->shadow_rom.mode = cpu_to_le32(modenr);
> +    d->rom->mode = cpu_to_le32(modenr);
> +    qxl_rom_set_dirty(d);
> +}
> +
> +static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    PCIQXLDevice *d = opaque;
> +    uint32_t io_port = addr - d->io_base;
> +
> +    switch (io_port) {
> +    case QXL_IO_RESET:
> +    case QXL_IO_SET_MODE:
> +    case QXL_IO_MEMSLOT_ADD:
> +    case QXL_IO_MEMSLOT_DEL:
> +    case QXL_IO_CREATE_PRIMARY:
> +        break;
> +    default:
> +        if (d->mode == QXL_MODE_NATIVE || d->mode == QXL_MODE_COMPAT)
> +            break;
> +        dprint(d, 1, "%s: unexpected port 0x%x in vga mode\n", __FUNCTION__, io_port);
> +        return;
> +    }
> +
> +    switch (io_port) {
> +    case QXL_IO_UPDATE_AREA:
> +    {
> +        QXLRect update = d->ram->update_area;
> +        qemu_mutex_unlock_iothread();
> +        d->ssd.worker->update_area(d->ssd.worker, d->ram->update_surface,
> +&update, NULL, 0, 0);
> +        qemu_mutex_lock_iothread();
> +        break;
> +    }
> +    case QXL_IO_NOTIFY_CMD:
> +        d->ssd.worker->wakeup(d->ssd.worker);
> +        break;
> +    case QXL_IO_NOTIFY_CURSOR:
> +        d->ssd.worker->wakeup(d->ssd.worker);
> +        break;
> +    case QXL_IO_UPDATE_IRQ:
> +        qxl_set_irq(d);
> +        break;
> +    case QXL_IO_NOTIFY_OOM:
> +        if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
> +            break;
> +        }
> +        pthread_yield();
> +        if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
> +            break;
> +        }
> +        d->oom_running = 1;
> +        d->ssd.worker->oom(d->ssd.worker);
> +        d->oom_running = 0;
> +        break;
> +    case QXL_IO_SET_MODE:
> +        dprint(d, 1, "QXL_SET_MODE %d\n", val);
> +        qxl_set_mode(d, val, 0);
> +        break;
> +    case QXL_IO_LOG:
> +        if (d->guestdebug) {
> +            fprintf(stderr, "qxl/guest: %s", d->ram->log_buf);
> +        }
> +        break;
> +    case QXL_IO_RESET:
> +        dprint(d, 1, "QXL_IO_RESET\n");
> +        qxl_hard_reset(d, 0);
> +        break;
> +    case QXL_IO_MEMSLOT_ADD:
> +        PANIC_ON(val>= NUM_MEMSLOTS);
> +        PANIC_ON(d->guest_slots[val].active);
> +        d->guest_slots[val].slot = d->ram->mem_slot;
> +        qxl_add_memslot(d, val, 0);
> +        break;
> +    case QXL_IO_MEMSLOT_DEL:
> +        qxl_del_memslot(d, val);
> +        break;
> +    case QXL_IO_CREATE_PRIMARY:
> +        PANIC_ON(val != 0);
> +        dprint(d, 1, "QXL_IO_CREATE_PRIMARY\n");
> +        d->guest_primary.surface = d->ram->create_surface;
> +        qxl_create_guest_primary(d, 0);
> +        break;
> +    case QXL_IO_DESTROY_PRIMARY:
> +        PANIC_ON(val != 0);
> +        dprint(d, 1, "QXL_IO_DESTROY_PRIMARY\n");
> +        qxl_destroy_primary(d);
> +        break;
> +    case QXL_IO_DESTROY_SURFACE_WAIT:
> +        d->ssd.worker->destroy_surface_wait(d->ssd.worker, val);
> +        break;
> +    case QXL_IO_DESTROY_ALL_SURFACES:
> +        d->ssd.worker->destroy_surfaces(d->ssd.worker);
> +        break;
> +    default:
> +        fprintf(stderr, "%s: ioport=0x%x, abort()\n", __FUNCTION__, io_port);
> +        abort();
> +    }
> +}
> +
> +static uint32_t ioport_read(void *opaque, uint32_t addr)
> +{
> +    PCIQXLDevice *d = opaque;
> +
> +    dprint(d, 1, "%s: unexpected\n", __FUNCTION__);
> +    return 0xff;
> +}
> +
> +static void qxl_map(PCIDevice *pci, int region_num,
> +                    pcibus_t addr, pcibus_t size, int type)
> +{
> +    static const char *names[] = {
> +        [ QXL_IO_RANGE_INDEX ]   = "ioports",
> +        [ QXL_RAM_RANGE_INDEX ]  = "devram",
> +        [ QXL_ROM_RANGE_INDEX ]  = "rom",
> +        [ QXL_VRAM_RANGE_INDEX ] = "vram",
> +    };
> +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, pci);
> +
> +    dprint(qxl, 1, "%s: bar %d [%s] addr 0x%lx size 0x%lx\n", __FUNCTION__,
> +            region_num, names[region_num], addr, size);
> +
> +    switch (region_num) {
> +    case QXL_IO_RANGE_INDEX:
> +        register_ioport_write(addr, size, 1, ioport_write, pci);
> +        register_ioport_read(addr, size, 1, ioport_read, pci);
> +        qxl->io_base = addr;
> +        break;
> +    case QXL_RAM_RANGE_INDEX:
> +        cpu_register_physical_memory(addr, size, qxl->vga.vram_offset | IO_MEM_RAM);
> +        qxl->vga.map_addr = addr;
> +        qxl->vga.map_end = addr + size;
> +        if (qxl->id == 0) {
> +            vga_dirty_log_start(&qxl->vga);
> +        }
> +        break;
> +    case QXL_ROM_RANGE_INDEX:
> +        cpu_register_physical_memory(addr, size, qxl->rom_offset | IO_MEM_ROM);
> +        break;
> +    case QXL_VRAM_RANGE_INDEX:
> +        cpu_register_physical_memory(addr, size, qxl->vram_offset | IO_MEM_RAM);
> +        break;
> +    }
> +}
> +
> +static void pipe_read(void *opaque)
> +{
> +    PCIQXLDevice *d = opaque;
> +    char dummy;
> +    int len;
> +
> +    do {
> +        len = read(d->pipe[0],&dummy, sizeof(dummy));
> +    } while (len == sizeof(dummy));
> +    qxl_set_irq(d);
> +}
> +
> +/* called from spice server thread context only */
> +static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
> +{
> +    uint32_t old_pending;
> +    uint32_t le_events = cpu_to_le32(events);
> +
> +    assert(d->ssd.running);
> +    old_pending = __sync_fetch_and_or(&d->ram->int_pending, le_events);
> +    if ((old_pending&  le_events) == le_events) {
> +        return;
> +    }
> +    if (pthread_self() == d->main) {
> +        qxl_set_irq(d);
> +    } else {
> +        if (write(d->pipe[1], d, 1) != 1) {
> +            dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
> +        }
> +    }
> +}
> +
> +static void init_pipe_signaling(PCIQXLDevice *d)
> +{
> +   if (pipe(d->pipe)<  0) {
> +       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
> +       return;
> +   }
> +#ifdef CONFIG_IOTHREAD
> +   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
> +#else
> +   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
> +#endif
> +   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
> +   fcntl(d->pipe[0], F_SETOWN, getpid());
> +
> +   d->main = pthread_self();
> +   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
> +}
> +
> +/* graphics console */
> +
> +static void qxl_hw_update(void *opaque)
> +{
> +    PCIQXLDevice *qxl = opaque;
> +    VGACommonState *vga =&qxl->vga;
> +
> +    switch (qxl->mode) {
> +    case QXL_MODE_VGA:
> +        vga->update(vga);
> +        break;
> +    case QXL_MODE_COMPAT:
> +    case QXL_MODE_NATIVE:
> +        qxl_render_update(qxl);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void qxl_hw_invalidate(void *opaque)
> +{
> +    PCIQXLDevice *qxl = opaque;
> +    VGACommonState *vga =&qxl->vga;
> +
> +    vga->invalidate(vga);
> +}
> +
> +static void qxl_hw_screen_dump(void *opaque, const char *filename)
> +{
> +    PCIQXLDevice *qxl = opaque;
> +    VGACommonState *vga =&qxl->vga;
> +
> +    switch (qxl->mode) {
> +    case QXL_MODE_COMPAT:
> +    case QXL_MODE_NATIVE:
> +        qxl_render_update(qxl);
> +        ppm_save(filename, qxl->ssd.ds->surface);
> +        break;
> +    case QXL_MODE_VGA:
> +        vga->screen_dump(vga, filename);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void qxl_hw_text_update(void *opaque, console_ch_t *chardata)
> +{
> +    PCIQXLDevice *qxl = opaque;
> +    VGACommonState *vga =&qxl->vga;
> +
> +    if (qxl->mode == QXL_MODE_VGA) {
> +        vga->text_update(vga, chardata);
> +        return;
> +    }
> +}
> +
> +static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
> +{
> +    PCIQXLDevice *qxl = opaque;
> +    qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
> +
> +    if (!running&&  qxl->mode == QXL_MODE_NATIVE) {
> +        /* dirty all vram (which holds surfaces) to make sure it is saved */
> +        /* FIXME #1: should go out during "live" stage */
> +        /* FIXME #2: we only need to save the areas which are actually used */
> +        ram_addr_t addr = qxl->vram_offset;
> +        qxl_set_dirty(addr, addr + qxl->vram_size);
> +    }
> +}
> +
> +/* display change listener */
> +
> +static void display_update(struct DisplayState *ds, int x, int y, int w, int h)
> +{
> +    if (qxl0->mode == QXL_MODE_VGA) {
> +        qemu_spice_display_update(&qxl0->ssd, x, y, w, h);
> +    }
> +}
> +
> +static void display_resize(struct DisplayState *ds)
> +{
> +    if (qxl0->mode == QXL_MODE_VGA) {
> +        qemu_spice_display_resize(&qxl0->ssd);
> +    }
> +}
> +
> +static void display_refresh(struct DisplayState *ds)
> +{
> +    if (qxl0->mode == QXL_MODE_VGA) {
> +        qemu_spice_display_refresh(&qxl0->ssd);
> +    }
> +}
> +
> +static DisplayChangeListener display_listener = {
> +    .dpy_update  = display_update,
> +    .dpy_resize  = display_resize,
> +    .dpy_refresh = display_refresh,
> +};
> +
> +static int qxl_init(PCIDevice *dev)
> +{
> +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, dev);
> +    VGACommonState *vga =&qxl->vga;
> +    uint8_t* config = qxl->pci.config;
> +    ram_addr_t ram_size = msb_mask(qxl->vga.vram_size * 2 - 1);
> +    uint32_t pci_device_id;
> +    uint32_t pci_device_rev;
> +    uint32_t io_size;
> +
> +    if (device_id == 0&&  dev->qdev.hotplugged) {
> +        device_id++;
> +    }
> +
> +    qxl->id = device_id;
> +    qxl->mode = QXL_MODE_UNDEFINED;
> +    qxl->generation = 1;
> +    qxl->num_memslots = NUM_MEMSLOTS;
> +    qxl->num_surfaces = NUM_SURFACES;
> +
> +    switch (qxl->revision) {
> +    case 1: /* spice 0.4 -- qxl-1 */
> +        pci_device_id  = QXL_DEVICE_ID_STABLE;
> +        pci_device_rev = QXL_REVISION_STABLE_V04;
> +        break;
> +    case 2: /* spice 0.6 -- qxl-2 */
> +        pci_device_id  = QXL_DEVICE_ID_STABLE;
> +        pci_device_rev = QXL_REVISION_STABLE_V06;
> +        break;
> +    default: /* experimental */
> +        pci_device_id  = QXL_DEVICE_ID_DEVEL;
> +        pci_device_rev = 1;
> +        break;
> +    }
> +
> +    if (!qxl->id) {
> +        if (ram_size<  32 * 1024 * 1024)
> +            ram_size = 32 * 1024 * 1024;
> +        vga_common_init(vga, ram_size);
> +        vga_init(vga);
> +        register_ioport_write(0x3c0, 16, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3b4,  2, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3d4,  2, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3ba,  1, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3da,  1, 1, qxl_vga_ioport_write, vga);
> +
> +        vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> +                                       qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> +        qxl->ssd.ds = vga->ds;
> +        qxl->ssd.bufsize = (16 * 1024 * 1024);
> +        qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
> +
> +        qxl0 = qxl;
> +        register_displaychangelistener(vga->ds,&display_listener);
> +
> +        if (qxl->pci.romfile == NULL) {
> +            if (pci_device_id == 0x01ff) {
> +                qxl->pci.romfile = qemu_strdup("vgabios-qxldev.bin");
> +            } else {
> +                qxl->pci.romfile = qemu_strdup("vgabios-qxl.bin");
> +            }
> +        }
> +        pci_config_set_class(config, PCI_CLASS_DISPLAY_VGA);
> +    } else {
> +        if (ram_size<  16 * 1024 * 1024)
> +            ram_size = 16 * 1024 * 1024;
> +        qxl->vga.vram_size = ram_size;
> +        qxl->vga.vram_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vgavram",
> +                                              qxl->vga.vram_size);
> +        qxl->vga.vram_ptr = qemu_get_ram_ptr(qxl->vga.vram_offset);
> +
> +        pci_config_set_class(config, PCI_CLASS_DISPLAY_OTHER);
> +    }
> +
> +    pci_config_set_vendor_id(config, REDHAT_PCI_VENDOR_ID);
> +    pci_config_set_device_id(config, pci_device_id);
> +    pci_set_byte(&config[PCI_REVISION_ID], pci_device_rev);
> +    pci_set_byte(&config[PCI_INTERRUPT_PIN], 1);
> +
> +    qxl->rom_size = qxl_rom_size();
> +    qxl->rom_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vrom", qxl->rom_size);
> +    init_qxl_rom(qxl);
> +    init_qxl_ram(qxl);
> +
> +    if (qxl->vram_size<  16 * 1024 * 1024) {
> +        qxl->vram_size = 16 * 1024 * 1024;
> +    }
> +    if (qxl->revision == 1) {
> +        qxl->vram_size = 4096;
> +    }
> +    qxl->vram_size = msb_mask(qxl->vram_size * 2 - 1);
> +    qxl->vram_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vram", qxl->vram_size);
> +
> +    io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> +    if (qxl->revision == 1) {
> +        io_size = 8;
> +    }
> +
> +    pci_register_bar(&qxl->pci, QXL_IO_RANGE_INDEX,
> +                     io_size, PCI_BASE_ADDRESS_SPACE_IO, qxl_map);
> +
> +    pci_register_bar(&qxl->pci, QXL_ROM_RANGE_INDEX,
> +                     qxl->rom_size, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     qxl_map);
> +
> +    pci_register_bar(&qxl->pci, QXL_RAM_RANGE_INDEX,
> +                     qxl->vga.vram_size, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     qxl_map);
> +
> +    pci_register_bar(&qxl->pci, QXL_VRAM_RANGE_INDEX, qxl->vram_size,
> +                     PCI_BASE_ADDRESS_SPACE_MEMORY, qxl_map);
> +
> +    qxl->ssd.qxl.base.sif =&qxl_interface.base;
> +    qxl->ssd.qxl.id = qxl->id;
> +    qemu_spice_add_interface(&qxl->ssd.qxl.base);
> +    qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
> +
> +    init_pipe_signaling(qxl);
> +    qxl_reset_state(qxl);
> +
> +    device_id++;
> +    return 0;
> +}
> +
> +static void qxl_pre_save(void *opaque)
> +{
> +    PCIQXLDevice* d = opaque;
> +    uint8_t *ram_start = d->vga.vram_ptr;
> +
> +    dprint(d, 1, "%s:\n", __FUNCTION__);
> +    if (d->last_release == NULL) {
> +        d->last_release_offset = 0;
> +    } else {
> +        d->last_release_offset = (uint8_t *)d->last_release - ram_start;
> +    }
> +    assert(d->last_release_offset<  d->vga.vram_size);
> +}
> +
> +static int qxl_pre_load(void *opaque)
> +{
> +    PCIQXLDevice* d = opaque;
> +
> +    dprint(d, 1, "%s: start\n", __FUNCTION__);
> +    qxl_hard_reset(d, 1);
> +    qxl_exit_vga_mode(d);
> +    dprint(d, 1, "%s: done\n", __FUNCTION__);
> +    return 0;
> +}
> +
> +static int qxl_post_load(void *opaque, int version)
> +{
> +    PCIQXLDevice* d = opaque;
> +    uint8_t *ram_start = d->vga.vram_ptr;
> +    QXLCommandExt *cmds;
> +    int in, out, i, newmode;
> +
> +    dprint(d, 1, "%s: start\n", __FUNCTION__);
> +
> +    assert(d->last_release_offset<  d->vga.vram_size);
> +    if (d->last_release_offset == 0) {
> +        d->last_release = NULL;
> +    } else {
> +        d->last_release = (QXLReleaseInfo *)(ram_start + d->last_release_offset);
> +    }
> +
> +    d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset);
> +
> +    dprint(d, 1, "%s: restore mode\n", __FUNCTION__);
> +    newmode = d->mode;
> +    d->mode = QXL_MODE_UNDEFINED;
> +    switch (newmode) {
> +    case QXL_MODE_UNDEFINED:
> +        break;
> +    case QXL_MODE_VGA:
> +        qxl_enter_vga_mode(d);
> +        break;
> +    case QXL_MODE_NATIVE:
> +        for (i = 0; i<  NUM_MEMSLOTS; i++) {
> +            if (!d->guest_slots[i].active) {
> +                continue;
> +            }
> +            qxl_add_memslot(d, i, 0);
> +        }
> +        qxl_create_guest_primary(d, 1);
> +
> +        /* replay surface-create and cursor-set commands */
> +        cmds = qemu_mallocz(sizeof(QXLCommandExt) * (NUM_SURFACES + 1));
> +        for (in = 0, out = 0; in<  NUM_SURFACES; in++) {
> +            if (d->guest_surfaces.cmds[in] == 0) {
> +                continue;
> +            }
> +            cmds[out].cmd.data = d->guest_surfaces.cmds[in];
> +            cmds[out].cmd.type = QXL_CMD_SURFACE;
> +            cmds[out].group_id = MEMSLOT_GROUP_GUEST;
> +            out++;
> +        }
> +        cmds[out].cmd.data = d->guest_cursor;
> +        cmds[out].cmd.type = QXL_CMD_CURSOR;
> +        cmds[out].group_id = MEMSLOT_GROUP_GUEST;
> +        out++;
> +        d->ssd.worker->loadvm_commands(d->ssd.worker, cmds, out);
> +        qemu_free(cmds);
> +
> +        break;
> +    case QXL_MODE_COMPAT:
> +        qxl_set_mode(d, d->shadow_rom.mode, 1);
> +        break;
> +    }
> +    dprint(d, 1, "%s: done\n", __FUNCTION__);
> +
> +    /* spice 0.4 compatibility -- accept but ignore */
> +    qemu_free(d->worker_data);
> +    d->worker_data = NULL;
> +    d->worker_data_size = 0;
> +
> +    return 0;
> +}
> +
> +#define QXL_SAVE_VERSION 20
> +
> +static bool qxl_test_worker_data(void *opaque, int version_id)
> +{
> +    PCIQXLDevice* d = opaque;
> +
> +    if (d->revision != 1) {
> +        return false;
> +    }
> +    if (!d->worker_data_size) {
> +        return false;
> +    }
> +    if (!d->worker_data) {
> +        d->worker_data = qemu_malloc(d->worker_data_size);
> +    }
> +    return true;
> +}
> +
> +static bool qxl_test_spice04(void *opaque, int version_id)
> +{
> +    PCIQXLDevice* d = opaque;
> +    return d->revision == 1;
> +}
> +
> +static bool qxl_test_spice06(void *opaque)
> +{
> +    PCIQXLDevice* d = opaque;
> +    return d->revision>  1;
> +}
> +
> +static VMStateDescription qxl_memslot = {
> +    .name               = "qxl-memslot",
> +    .version_id         = QXL_SAVE_VERSION,
> +    .minimum_version_id = QXL_SAVE_VERSION,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(slot.mem_start, struct guest_slots),
> +        VMSTATE_UINT64(slot.mem_end,   struct guest_slots),
> +        VMSTATE_UINT32(active,         struct guest_slots),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static VMStateDescription qxl_surface = {
> +    .name               = "qxl-surface",
> +    .version_id         = QXL_SAVE_VERSION,
> +    .minimum_version_id = QXL_SAVE_VERSION,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(width,      QXLSurfaceCreate),
> +        VMSTATE_UINT32(height,     QXLSurfaceCreate),
> +        VMSTATE_INT32(stride,      QXLSurfaceCreate),
> +        VMSTATE_UINT32(format,     QXLSurfaceCreate),
> +        VMSTATE_UINT32(position,   QXLSurfaceCreate),
> +        VMSTATE_UINT32(mouse_mode, QXLSurfaceCreate),
> +        VMSTATE_UINT32(flags,      QXLSurfaceCreate),
> +        VMSTATE_UINT32(type,       QXLSurfaceCreate),
> +        VMSTATE_UINT64(mem,        QXLSurfaceCreate),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static VMStateDescription qxl_vmstate_spice06 = {
> +    .name               = "qxl/spice06",
> +    .version_id         = QXL_SAVE_VERSION,
> +    .minimum_version_id = QXL_SAVE_VERSION,
> +    .fields = (VMStateField []) {
> +        VMSTATE_INT32_EQUAL(num_memslots, PCIQXLDevice),
> +        VMSTATE_STRUCT_ARRAY(guest_slots, PCIQXLDevice, NUM_MEMSLOTS, 0,
> +                             qxl_memslot, struct guest_slots),
> +        VMSTATE_STRUCT(guest_primary.surface, PCIQXLDevice, 0,
> +                       qxl_surface, QXLSurfaceCreate),
> +        VMSTATE_INT32_EQUAL(num_surfaces, PCIQXLDevice),
> +        VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
> +                      vmstate_info_uint64, uint64_t),
> +        VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static VMStateDescription qxl_vmstate = {
> +    .name               = "qxl",
> +    .version_id         = QXL_SAVE_VERSION,
> +    .minimum_version_id = QXL_SAVE_VERSION,
> +    .pre_save           = qxl_pre_save,
> +    .pre_load           = qxl_pre_load,
> +    .post_load          = qxl_post_load,
> +    .fields = (VMStateField []) {
> +        VMSTATE_PCI_DEVICE(pci, PCIQXLDevice),
> +        VMSTATE_STRUCT(vga, PCIQXLDevice, 0, vmstate_vga_common, VGACommonState),
> +        VMSTATE_UINT32(shadow_rom.mode, PCIQXLDevice),
> +        VMSTATE_UINT32(num_free_res, PCIQXLDevice),
> +        VMSTATE_UINT32(last_release_offset, PCIQXLDevice),
> +        VMSTATE_UINT32(mode, PCIQXLDevice),
> +        VMSTATE_UINT32(ssd.unique, PCIQXLDevice),
> +
> +        /* spice 0.4 sends/expects them */
> +        VMSTATE_VBUFFER_UINT32(vga.vram_ptr, PCIQXLDevice, 0, qxl_test_spice04, 0,
> +                               vga.vram_size),
> +        VMSTATE_UINT32_TEST(worker_data_size, PCIQXLDevice, qxl_test_spice04),
> +        VMSTATE_VBUFFER_UINT32(worker_data, PCIQXLDevice, 0, qxl_test_worker_data, 0,
> +                               worker_data_size),
> +
> +        VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection[]) {
> +        {
> +            /* additional spice 0.6 state */
> +            .vmsd   =&qxl_vmstate_spice06,
> +            .needed = qxl_test_spice06,
> +        },{
> +            /* end of list */
> +        },
> +    },
> +};
> +
> +static PCIDeviceInfo qxl_info = {
> +    .qdev.name    = "qxl",
> +    .qdev.desc    = "Spice QXL GPU",
> +    .qdev.size    = sizeof(PCIQXLDevice),
> +    .qdev.reset   = qxl_reset_handler,
> +    .qdev.vmsd    =&qxl_vmstate,
> +    .init         = qxl_init,
> +    .config_write = qxl_write_config,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_UINT32("ram_size", PCIQXLDevice, vga.vram_size, 64 * 1024 * 1024),
> +        DEFINE_PROP_UINT32("vram_size", PCIQXLDevice, vram_size, 64 * 1024 * 1024),
> +        DEFINE_PROP_UINT32("revision", PCIQXLDevice, revision, 2),
> +        DEFINE_PROP_UINT32("debug", PCIQXLDevice, debug, 0),
> +        DEFINE_PROP_UINT32("guestdebug", PCIQXLDevice, guestdebug, 0),
> +        DEFINE_PROP_UINT32("cmdlog", PCIQXLDevice, cmdlog, 0),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void qxl_register(void)
> +{
> +    pci_qdev_register(&qxl_info);
> +}
> +
> +device_init(qxl_register);
>    

I don't see how compatibility works at all with having the QXL device in 
a separate library.  How do we control what features are 
enabled/disabled in the QXL device?

Is there a specification of the QXL device?  I would like to understand 
why we need to emulate the QXL device outside of QEMU.

N.B. I'm *not* advocating implementing Spice in QEMU but AFAICT, QXL 
commands can either be rendered locally or offloaded via Spice.  Is it 
within the realm of possibility to have a local rendering mode entirely 
within QEMU and then have an explicit command overload interface that we 
can tie into libspice?  With QEMU decoding the QXL commands, it means 
that QEMU provides the guest visible interface which is useful from an 
architectural security PoV.

Regards,

Anthony Liguori
Michael S. Tsirkin - Nov. 16, 2010, 5:43 p.m.
On Tue, Nov 02, 2010 at 02:34:58PM +0100, Gerd Hoffmann wrote:
> +        if (ram_size < 32 * 1024 * 1024)
> +            ram_size = 32 * 1024 * 1024;
> +        vga_common_init(vga, ram_size);
> +        vga_init(vga);
> +        register_ioport_write(0x3c0, 16, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3b4,  2, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3d4,  2, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3ba,  1, 1, qxl_vga_ioport_write, vga);
> +        register_ioport_write(0x3da,  1, 1, qxl_vga_ioport_write, vga);
> +
> +        vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> +                                       qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> +        qxl->ssd.ds = vga->ds;
> +        qxl->ssd.bufsize = (16 * 1024 * 1024);
> +        qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
> +
> +        qxl0 = qxl;

What happens when this device is then removed?

> +        register_displaychangelistener(vga->ds, &display_listener);
> +
> +        if (qxl->pci.romfile == NULL) {
> +            if (pci_device_id == 0x01ff) {
> +                qxl->pci.romfile = qemu_strdup("vgabios-qxldev.bin");
> +            } else {
> +                qxl->pci.romfile = qemu_strdup("vgabios-qxl.bin");
> +            }
> +        }
> +        pci_config_set_class(config, PCI_CLASS_DISPLAY_VGA);
> +    } else {
> +        if (ram_size < 16 * 1024 * 1024)
> +            ram_size = 16 * 1024 * 1024;
> +        qxl->vga.vram_size = ram_size;
> +        qxl->vga.vram_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vgavram",
> +                                              qxl->vga.vram_size);
> +        qxl->vga.vram_ptr = qemu_get_ram_ptr(qxl->vga.vram_offset);
> +
> +        pci_config_set_class(config, PCI_CLASS_DISPLAY_OTHER);

So 1st device has device id different from the rest?
Why?

> +    }
> +
> +    pci_config_set_vendor_id(config, REDHAT_PCI_VENDOR_ID);
> +    pci_config_set_device_id(config, pci_device_id);
> +    pci_set_byte(&config[PCI_REVISION_ID], pci_device_rev);
> +    pci_set_byte(&config[PCI_INTERRUPT_PIN], 1);
> +
> +    qxl->rom_size = qxl_rom_size();
> +    qxl->rom_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vrom", qxl->rom_size);
> +    init_qxl_rom(qxl);
> +    init_qxl_ram(qxl);
> +
> +    if (qxl->vram_size < 16 * 1024 * 1024) {
> +        qxl->vram_size = 16 * 1024 * 1024;
> +    }
> +    if (qxl->revision == 1) {
> +        qxl->vram_size = 4096;
> +    }
> +    qxl->vram_size = msb_mask(qxl->vram_size * 2 - 1);
> +    qxl->vram_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vram", qxl->vram_size);
> +
> +    io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> +    if (qxl->revision == 1) {
> +        io_size = 8;
> +    }
> +
> +    pci_register_bar(&qxl->pci, QXL_IO_RANGE_INDEX,
> +                     io_size, PCI_BASE_ADDRESS_SPACE_IO, qxl_map);
> +
> +    pci_register_bar(&qxl->pci, QXL_ROM_RANGE_INDEX,
> +                     qxl->rom_size, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     qxl_map);
> +
> +    pci_register_bar(&qxl->pci, QXL_RAM_RANGE_INDEX,
> +                     qxl->vga.vram_size, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     qxl_map);
> +
> +    pci_register_bar(&qxl->pci, QXL_VRAM_RANGE_INDEX, qxl->vram_size,
> +                     PCI_BASE_ADDRESS_SPACE_MEMORY, qxl_map);
> +
> +    qxl->ssd.qxl.base.sif = &qxl_interface.base;
> +    qxl->ssd.qxl.id = qxl->id;
> +    qemu_spice_add_interface(&qxl->ssd.qxl.base);
> +    qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
> +
> +    init_pipe_signaling(qxl);
> +    qxl_reset_state(qxl);
> +
> +    device_id++;

what happens when this wraps around?
Since it's an int probably undefined behaviour ...
Gerd Hoffmann - Nov. 17, 2010, 1:14 p.m.
Hi,

> These headers shouldn't be needed. This file also needs a copyright.diff
> --git a/hw/qxl-render.c b/hw/qxl-render.c
>
> Does QXL have a specification?

The spice protocol has a specification (slightly outdated though).

http://www.spice-space.org/docs/spice_protocol.pdf

That covers the rendering commands.  That are certainly not all aspects 
of the qxl device though.

>> +static void init_qxl_rom(PCIQXLDevice *d)
>> +{
>
> You don't really mean ROM, right. This is a set of configuration
> registers exposed to the guest via PCI, right?

It isn't the option rom bar.  It is a memory bar with (read-only) 
configuration information for the guest.

>> + QXLRom *rom = qemu_get_ram_ptr(d->rom_offset);
>
> This function should not be used in a device model.

--verbose please.

I think this one I can replace with cpu_physical_memory_write().

There are other cases though where I hand pointers to device memory 
(commands, image data) to the spice server. cpu_physical_memory_rw() 
wouldn't work there.

>> +device_init(qxl_register);
>
> I don't see how compatibility works at all with having the QXL device in
> a separate library. How do we control what features are enabled/disabled
> in the QXL device?

There are no features one can enable or disable.

What we have are spice server config options which (for example) affect 
how the spice server sends image data to the client, i.e. whenever lossy 
jpeg compression (aka wan support) is enabled or not.  That isn't 
visible to the guest and the qxl device though.

There are two revisions of the qxl device:

  - spice 0.4.x is pci revision 1.
  - spice 0.6.x is pci revision 2 (this submission).

The qxl device is backward-compatible, i.e. spice 0.4.x guest drivers 
can continue to work with the current qxl device.  Nevertheless there is 
a special compat mode (activated via qxl.rev=1 property) which makes the 
current qxl device look exactly like a spice 0.4.x one.  This is needed 
for two reasons:

  - migration compatibility.
  - some spice 0.4.x guest drivers are quite picky and refuse to load
    if they find a pci revision != 1.

It isn't clear yet how we'll handle any guest-visible changes to the 
spice display protocol such as new rendering commands.  One option is to 
raise the pci revision again.  Another option is to add feature flags to 
the rom bar (the configuration area, not option rom bar). 
Enabling/disabling these features can be handled using qdev properties.

> Is there a specification of the QXL device?

See above.

> I would like to understand
> why we need to emulate the QXL device outside of QEMU.

Usually the rendering doesn't happen at all on the server side ...

> N.B. I'm *not* advocating implementing Spice in QEMU but AFAICT, QXL
> commands can either be rendered locally or offloaded via Spice. Is it
> within the realm of possibility to have a local rendering mode entirely
> within QEMU and then have an explicit command overload interface that we
> can tie into libspice?

Usually the rendering commands are send using the spice protocol to the 
spice client and are rendered there.  Server-side local rendering 
happens in some special cases (read back video memory for screen shots, 
flush state for migration).  Local rendering is also used to handle vnc/sdl.

Have qemu parse and process the rendering commands for sdl/vnc is 
certainly possible.  You would effectively have to copy the spice 
rendering engine into qemu though.  It would be alot of work and alot of 
code which is duplicated for IMHO no good reason.

> With QEMU decoding the QXL commands, it means
> that QEMU provides the guest visible interface which is useful from an
> architectural security PoV.

--verbose.

cheers,
   Gerd
Gerd Hoffmann - Nov. 17, 2010, 1:28 p.m.
On 11/16/10 18:43, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 02:34:58PM +0100, Gerd Hoffmann wrote:
>> +        if (ram_size<  32 * 1024 * 1024)
>> +            ram_size = 32 * 1024 * 1024;
>> +        vga_common_init(vga, ram_size);
>> +        vga_init(vga);
>> +        register_ioport_write(0x3c0, 16, 1, qxl_vga_ioport_write, vga);
>> +        register_ioport_write(0x3b4,  2, 1, qxl_vga_ioport_write, vga);
>> +        register_ioport_write(0x3d4,  2, 1, qxl_vga_ioport_write, vga);
>> +        register_ioport_write(0x3ba,  1, 1, qxl_vga_ioport_write, vga);
>> +        register_ioport_write(0x3da,  1, 1, qxl_vga_ioport_write, vga);
>> +
>> +        vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
>> +                                       qxl_hw_screen_dump, qxl_hw_text_update, qxl);
>> +        qxl->ssd.ds = vga->ds;
>> +        qxl->ssd.bufsize = (16 * 1024 * 1024);
>> +        qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
>> +
>> +        qxl0 = qxl;
>
> What happens when this device is then removed?

Better don't try ...

The primary vga can't be hot-unplugged in qemu.  Not only because the 
qxl0 pointer would point into nowhere in this case, but also because you 
can't unregister the graphic console.  Also having non-pci ressources 
(legacy vga I/O ports) is a problem.

>> +        pci_config_set_class(config, PCI_CLASS_DISPLAY_VGA);
>> +    } else {
>> +        pci_config_set_class(config, PCI_CLASS_DISPLAY_OTHER);
>
> So 1st device has device id different from the rest?

Yes.

> Why?

Because the first one actually *is* different. It is the only one which 
is vga compatible.  It serves as primary display.  You'll see the boot 
messages there.

>> +    device_id++;
>
> what happens when this wraps around?
> Since it's an int probably undefined behaviour ...

I doubt you'll see it wrap in any real world scenario.  Even with one
hotplug + unplug cycle per second you'll need a bunch of years to see it 
wrap.  Beside that at least in windows the device can't be unplugged in 
the first place, windows will veto the unplug request.

cheers,
   Gerd
Michael S. Tsirkin - Nov. 17, 2010, 1:58 p.m.
On Wed, Nov 17, 2010 at 02:28:21PM +0100, Gerd Hoffmann wrote:
> On 11/16/10 18:43, Michael S. Tsirkin wrote:
> >On Tue, Nov 02, 2010 at 02:34:58PM +0100, Gerd Hoffmann wrote:
> >>+        if (ram_size<  32 * 1024 * 1024)
> >>+            ram_size = 32 * 1024 * 1024;
> >>+        vga_common_init(vga, ram_size);
> >>+        vga_init(vga);
> >>+        register_ioport_write(0x3c0, 16, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3b4,  2, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3d4,  2, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3ba,  1, 1, qxl_vga_ioport_write, vga);
> >>+        register_ioport_write(0x3da,  1, 1, qxl_vga_ioport_write, vga);
> >>+
> >>+        vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> >>+                                       qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> >>+        qxl->ssd.ds = vga->ds;
> >>+        qxl->ssd.bufsize = (16 * 1024 * 1024);
> >>+        qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
> >>+
> >>+        qxl0 = qxl;
> >
> >What happens when this device is then removed?
> 
> Better don't try ...

Better prevent it then?

> The primary vga can't be hot-unplugged in qemu.  Not only because
> the qxl0 pointer would point into nowhere in this case, but also
> because you can't unregister the graphic console.

But do we really have to make this part of qxl design, with global
vars and stuff? Even if we have a limitation in qemu, qxl should be able
to keep all its data in device state I think ...

>  Also having non-pci ressources (legacy vga I/O ports) is a problem.

I'm not sure why is this a problem. It shouldn't be.

> >>+        pci_config_set_class(config, PCI_CLASS_DISPLAY_VGA);
> >>+    } else {
> >>+        pci_config_set_class(config, PCI_CLASS_DISPLAY_OTHER);
> >
> >So 1st device has device id different from the rest?
> 
> Yes.
> 
> >Why?
> 
> Because the first one actually *is* different. It is the only one
> which is vga compatible.  It serves as primary display.  You'll see
> the boot messages there.

I thought it's up to the guest where to send boot messages.
Modern BIOSes have options to control this behaviour.
I think it's a mistake to make things such as device class
depend on the order devices are created in.
It would be better to make it a separate property, or give it a
different name.

> >>+    device_id++;
> >
> >what happens when this wraps around?
> >Since it's an int probably undefined behaviour ...
> 
> I doubt you'll see it wrap in any real world scenario.  Even with one
> hotplug + unplug cycle per second you'll need a bunch of years to
> see it wrap.  Beside that at least in windows the device can't be
> unplugged in the first place, windows will veto the unplug request.
> 
> cheers,
>   Gerd

Yes but we are splitting the unplug part from the guest eject, so user
will in the future be able to perform surprise removal just like with
real hardware. And with this in place, it need not take even a
millisecond to unplug a device.
Gerd Hoffmann - Nov. 17, 2010, 3:20 p.m.
Hi,

>>>> +        qxl0 = qxl;
>>>
>>> What happens when this device is then removed?
>>
>> Better don't try ...
>
> Better prevent it then?

How can I do that?

>> The primary vga can't be hot-unplugged in qemu.  Not only because
>> the qxl0 pointer would point into nowhere in this case, but also
>> because you can't unregister the graphic console.
>
> But do we really have to make this part of qxl design, with global
> vars and stuff? Even if we have a limitation in qemu, qxl should be able
> to keep all its data in device state I think ...

It isn't part of the qxl design.  It is just that 
register_displaychangelistener() doesn't provide a way to pass through a 
device state pointer, so these callbacks use qxl0 instead.

>>   Also having non-pci ressources (legacy vga I/O ports) is a problem.
>
> I'm not sure why is this a problem. It shouldn't be.

Because it has ressources not visible in the pci config space?

But you know the pci specs better than me.  If you think it would work 
you are probably right ;)

>> Because the first one actually *is* different. It is the only one
>> which is vga compatible.  It serves as primary display.  You'll see
>> the boot messages there.
>
> I thought it's up to the guest where to send boot messages.
> Modern BIOSes have options to control this behaviour.

It isn't.  For one qemu has no multihead support.  You'll see 
non-primary displays only when connecting via spice because of that.

I also doubt seabios supports this.  How does this work btw?  Only one 
vga adapter can drive the legacy vga ports, right?  Is there some way to 
enable/disable this per vga device?  If so: does qemu emulate this 
correctly?

> I think it's a mistake to make things such as device class
> depend on the order devices are created in.
> It would be better to make it a separate property, or give it a
> different name.

Having qxl-vga and qxl devices would certainly be an option.  Works 
probably better than some qdev property usability-wise.

>> I doubt you'll see it wrap in any real world scenario.  Even with one
>> hotplug + unplug cycle per second you'll need a bunch of years to
>> see it wrap.  Beside that at least in windows the device can't be
>> unplugged in the first place, windows will veto the unplug request.
>
> Yes but we are splitting the unplug part from the guest eject, so user
> will in the future be able to perform surprise removal just like with
> real hardware. And with this in place, it need not take even a
> millisecond to unplug a device.

If you do that your guest will likely be quite upset and you'll have 
much bigger problems than the counter wrapping after millions of 
hotplugs.  I also don't see the point in plugging a display like mad.

Can we care about the *real* problems please?

thanks,
   Gerd
Michael S. Tsirkin - Nov. 17, 2010, 4:42 p.m.
On Wed, Nov 17, 2010 at 04:20:45PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>>>+        qxl0 = qxl;
> >>>
> >>>What happens when this device is then removed?
> >>
> >>Better don't try ...
> >
> >Better prevent it then?
> 
> How can I do that?
> 
> >>The primary vga can't be hot-unplugged in qemu.  Not only because
> >>the qxl0 pointer would point into nowhere in this case, but also
> >>because you can't unregister the graphic console.
> >
> >But do we really have to make this part of qxl design, with global
> >vars and stuff? Even if we have a limitation in qemu, qxl should be able
> >to keep all its data in device state I think ...
> 
> It isn't part of the qxl design.  It is just that
> register_displaychangelistener() doesn't provide a way to pass
> through a device state pointer, so these callbacks use qxl0 instead.

I see. Fair enough, we'd have to fix that API first.

> >>  Also having non-pci ressources (legacy vga I/O ports) is a problem.
> >
> >I'm not sure why is this a problem. It shouldn't be.
> 
> Because it has ressources not visible in the pci config space?
> But you know the pci specs better than me.  If you think it would
> work you are probably right ;)

Devices report using these legacy resources by declaring special values
in the programming interface register. See appendix D in the spec
if you are curious. VGA is 03.

> >>Because the first one actually *is* different. It is the only one
> >>which is vga compatible.  It serves as primary display.  You'll see
> >>the boot messages there.
> >
> >I thought it's up to the guest where to send boot messages.
> >Modern BIOSes have options to control this behaviour.
> 
> It isn't.  For one qemu has no multihead support.  You'll see
> non-primary displays only when connecting via spice because of that.
> 
> I also doubt seabios supports this.

Not now. But that's guest stuff.

>  How does this work btw?  Only
> one vga adapter can drive the legacy vga ports, right?  Is there
> some way to enable/disable this per vga device?

Yes, just disable IO memory.

>  If so: does qemu
> emulate this correctly?

It mostly does.

> >I think it's a mistake to make things such as device class
> >depend on the order devices are created in.
> >It would be better to make it a separate property, or give it a
> >different name.
> 
> Having qxl-vga and qxl devices would certainly be an option.  Works
> probably better than some qdev property usability-wise.

Sounds good.

> >>I doubt you'll see it wrap in any real world scenario.  Even with one
> >>hotplug + unplug cycle per second you'll need a bunch of years to
> >>see it wrap.  Beside that at least in windows the device can't be
> >>unplugged in the first place, windows will veto the unplug request.
> >
> >Yes but we are splitting the unplug part from the guest eject, so user
> >will in the future be able to perform surprise removal just like with
> >real hardware. And with this in place, it need not take even a
> >millisecond to unplug a device.
> 
> If you do that your guest will likely be quite upset and you'll have
> much bigger problems than the counter wrapping after millions of
> hotplugs.

In the guest. Maybe. Although WHQL includes surprise removal tests
for some cards, so it might work better than you think.

But the counter wrapping will at least in theory crash qemu,
this is an even bigger problem than a guest crash.

>  I also don't see the point in plugging a display like
> mad.

Just to see if you can exploit some memory corruption maybe?

> Can we care about the *real* problems please?
> 
> thanks,
>   Gerd
Gerd Hoffmann - Nov. 17, 2010, 5:02 p.m.
Hi,

>>>> Better don't try ...
>>>
>>> Better prevent it then?
>>
>> How can I do that?

Question still stands:  Is there some way to disable hotplug for certain 
pci devices?

>>   How does this work btw?  Only
>> one vga adapter can drive the legacy vga ports, right?  Is there
>> some way to enable/disable this per vga device?
>
> Yes, just disable IO memory.

This is supposed to disable legacy vga ports (0x03c0+) too?

>>   If so: does qemu
>> emulate this correctly?
>
> It mostly does.

I doubt it actually enables/disables the legacy vga ports.

> But the counter wrapping will at least in theory crash qemu,
> this is an even bigger problem than a guest crash.

I can put it a limit at one million hotplugs or so ...

>>   I also don't see the point in plugging a display like
>> mad.
>
> Just to see if you can exploit some memory corruption maybe?

Can the guest do that without the hosts help?
Especially plugging *in* something?

cheers,
   Gerd
Michael S. Tsirkin - Nov. 17, 2010, 6 p.m.
On Wed, Nov 17, 2010 at 06:02:39PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >>>>Better don't try ...
> >>>
> >>>Better prevent it then?
> >>
> >>How can I do that?
> 
> Question still stands:  Is there some way to disable hotplug for
> certain pci devices?

Not sure. It's really a work-around bug, maybe it's better to fix
it properly.

> >>  How does this work btw?  Only
> >>one vga adapter can drive the legacy vga ports, right?  Is there
> >>some way to enable/disable this per vga device?
> >
> >Yes, just disable IO memory.
> 
> This is supposed to disable legacy vga ports (0x03c0+) too?

Sure.

> >>  If so: does qemu
> >>emulate this correctly?
> >
> >It mostly does.
> 
> I doubt it actually enables/disables the legacy vga ports.

I'll check when I have the time. We can fix it if it doesn't,

> >But the counter wrapping will at least in theory crash qemu,
> >this is an even bigger problem than a guest crash.
> 
> I can put it a limit at one million hotplugs or so ...

How about a simple allocator? With at most 256 devices
on a pci bus * 8 functions, it need not be hard: just
a long long bitmask with ffsll used to find a free bit.

Also - what is the ID used for? What happens if it's not
unique?

> >>  I also don't see the point in plugging a display like
> >>mad.
> >
> >Just to see if you can exploit some memory corruption maybe?
> 
> Can the guest do that without the hosts help?
> Especially plugging *in* something?
> 
> cheers,
>   Gerd

No, but a monitor user connected to qemu over a qmp socket can do this.
Gleb Natapov - Nov. 18, 2010, 8:09 a.m.
On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > >>  If so: does qemu
> > >>emulate this correctly?
> > >
> > >It mostly does.
> > 
> > I doubt it actually enables/disables the legacy vga ports.
> 
> I'll check when I have the time. We can fix it if it doesn't,
> 
So many guests (all of them?) just assume that vga ports and
framebuffer is there. So what "fixing" this will buy us?

--
			Gleb.
Gerd Hoffmann - Nov. 18, 2010, 8:22 a.m.
On 11/18/10 09:09, Gleb Natapov wrote:
> On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
>>>>>   If so: does qemu
>>>>> emulate this correctly?
>>>>
>>>> It mostly does.
>>>
>>> I doubt it actually enables/disables the legacy vga ports.
>>
>> I'll check when I have the time. We can fix it if it doesn't,
>>
> So many guests (all of them?) just assume that vga ports and
> framebuffer is there. So what "fixing" this will buy us?

Nothing?

Certainly not for the qemu standard vga.  The only way for guests is to 
drive that one is via legacy vga ports and vesa vga bios.  Hotplug is 
never ever going to work here.  Also the vga memory window @ 0xa0000 
with the piix chipset interactions is good for some extra fun ...

cirrus might be fixable to be hotpluggable.  It also can be programmed 
via mmio bar instead of legacy vga ports, so having two cirrus cards in 
one system might be possible to get work.  I doubt it is worth the 
trouble though.

I still think that vga cards should simply be flagged to be not 
hot-pluggable.

cheers,
   Gerd
Michael S. Tsirkin - Nov. 18, 2010, 9:03 a.m.
On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > >>  If so: does qemu
> > > >>emulate this correctly?
> > > >
> > > >It mostly does.
> > > 
> > > I doubt it actually enables/disables the legacy vga ports.
> > 
> > I'll check when I have the time. We can fix it if it doesn't,
> > 
> So many guests (all of them?) just assume that vga ports and
> framebuffer is there.

Why do you think they disable io memory then?

> So what "fixing" this will buy us?

Besides spec compliancy, you mean?  Ability to support multiple VGA
cards. That's how it works I think: BIOS enables IO on the primary
VGA device only.

> --
> 			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 9:08 a.m.
On Thu, Nov 18, 2010 at 09:22:02AM +0100, Gerd Hoffmann wrote:
> On 11/18/10 09:09, Gleb Natapov wrote:
> >On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> >>>>>  If so: does qemu
> >>>>>emulate this correctly?
> >>>>
> >>>>It mostly does.
> >>>
> >>>I doubt it actually enables/disables the legacy vga ports.
> >>
> >>I'll check when I have the time. We can fix it if it doesn't,
> >>
> >So many guests (all of them?) just assume that vga ports and
> >framebuffer is there. So what "fixing" this will buy us?
> 
> Nothing?
> 
> Certainly not for the qemu standard vga.  The only way for guests is
> to drive that one is via legacy vga ports and vesa vga bios.
> Hotplug is never ever going to work here.
> Also the vga memory window @ 0xa0000 with the piix chipset
> interactions is good for some extra fun ...
> 
> cirrus might be fixable to be hotpluggable.  It also can be
> programmed via mmio bar instead of legacy vga ports, so having two
> cirrus cards in one system might be possible to get work.  I doubt
> it is worth the trouble though.

We will need to fix this for proper pci memory management, anyway.

> I still think that vga cards should simply be flagged to be not
> hot-pluggable.
> 
> cheers,
>   Gerd

Send a patch.
Gleb Natapov - Nov. 18, 2010, 9:11 a.m.
On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > >>  If so: does qemu
> > > > >>emulate this correctly?
> > > > >
> > > > >It mostly does.
> > > > 
> > > > I doubt it actually enables/disables the legacy vga ports.
> > > 
> > > I'll check when I have the time. We can fix it if it doesn't,
> > > 
> > So many guests (all of them?) just assume that vga ports and
> > framebuffer is there.
> 
> Why do you think they disable io memory then?
> 
Who and how and when disables io memory? Some guests are designed to run
even on old ISA machines that have no way to disable anything. The
device is just there.

This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
program them into PCI IO bars to be nice.

> > So what "fixing" this will buy us?
> 
> Besides spec compliancy, you mean?  Ability to support multiple VGA
> cards. That's how it works I think: BIOS enables IO on the primary
> VGA device only.
> 
What spec defines hot-plug for primary VGA adapter? Our BIOS should support
-M isa machine too. There is no way to disable VGA or even check if it
is present there.

--
			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 9:30 a.m.
On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > >>  If so: does qemu
> > > > > >>emulate this correctly?
> > > > > >
> > > > > >It mostly does.
> > > > > 
> > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > 
> > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > 
> > > So many guests (all of them?) just assume that vga ports and
> > > framebuffer is there.
> > 
> > Why do you think they disable io memory then?
> > 
> Who and how and when disables io memory?

I think guest will do this if you disable the device through the device
manager. This might need a reboot to become effective.

> Some guests are designed to run
> even on old ISA machines that have no way to disable anything. The
> device is just there.
> 
> This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> program them into PCI IO bars to be nice.

HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
the card, they are hardwired there. However, the card must not claim any
io transactions if IO memory is disabled in command register.

When qemu is started, it works correctly: the io memory is disabled and card does
not claim any io. Then BIOS comes along and enables io. At this point
map callback is invoked and maps io memory, card starts claiming io.

What is broken is that if BIOS/guest then disables IO memory,
(I think - even if guest is rebooted!) we will keep claiming IO transactions.
That our emulation does this seems to be a clear spec violation, we are
just lucky that BIOS/guest does not do this at the moment.



> > > So what "fixing" this will buy us?
> > 
> > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > cards. That's how it works I think: BIOS enables IO on the primary
> > VGA device only.
> > 
> What spec defines hot-plug for primary VGA adapter?

No idea about hotplug. I am talking about multiple VGA cards,
enabling/disabling them dynamically should be possible.

> Our BIOS should support
> -M isa machine too.

> There is no way to disable VGA or even check if it
> is present there.

So I guess with isa you can't have multiple VGA cards work.  With PCI
I think you can.

> --
> 			Gleb.
Gleb Natapov - Nov. 18, 2010, 9:57 a.m.
On Thu, Nov 18, 2010 at 11:30:27AM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > >>  If so: does qemu
> > > > > > >>emulate this correctly?
> > > > > > >
> > > > > > >It mostly does.
> > > > > > 
> > > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > > 
> > > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > > 
> > > > So many guests (all of them?) just assume that vga ports and
> > > > framebuffer is there.
> > > 
> > > Why do you think they disable io memory then?
> > > 
> > Who and how and when disables io memory?
> 
> I think guest will do this if you disable the device through the device
> manager. This might need a reboot to become effective.
> 
Try to do it with primary VGA adapter and tell us what happens :)

> > Some guests are designed to run
> > even on old ISA machines that have no way to disable anything. The
> > device is just there.
> > 
> > This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> > program them into PCI IO bars to be nice.
> 
> HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
> the card, they are hardwired there. However, the card must not claim any
> io transactions if IO memory is disabled in command register.
> 
Is this correct also for legacy ports? This wouldn't be backwards
compatible to ISA machines, so old software my not run properly back in
the days when transaction from ISA to PCI happened. So my guess is that
old ISA ports works in backwards compatible way.

> When qemu is started, it works correctly: the io memory is disabled and card does
> not claim any io. Then BIOS comes along and enables io. At this point
> map callback is invoked and maps io memory, card starts claiming io.
Looking at the code I see that cirrus claims all IO ports and
framebuffer memory during init function unconditionally.

> 
> What is broken is that if BIOS/guest then disables IO memory,
> (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> That our emulation does this seems to be a clear spec violation, we are
> just lucky that BIOS/guest does not do this at the moment.
> 
> 
> 
> > > > So what "fixing" this will buy us?
> > > 
> > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > cards. That's how it works I think: BIOS enables IO on the primary
> > > VGA device only.
> > > 
> > What spec defines hot-plug for primary VGA adapter?
> 
> No idea about hotplug. I am talking about multiple VGA cards,
> enabling/disabling them dynamically should be possible.
Of course. With properly designed VGA card you should be able to have
more then one, but one of them will provide legacy functionality and is
not removable.

> 
> > Our BIOS should support
> > -M isa machine too.
> 
> > There is no way to disable VGA or even check if it
> > is present there.
> 
> So I guess with isa you can't have multiple VGA cards work.  With PCI
> I think you can.
> 
With isa you just need custom designed HW and software and you can have
as many VGA cards as you want :)

--
			Gleb.
Gerd Hoffmann - Nov. 18, 2010, 10:46 a.m.
Hi,

>> I still think that vga cards should simply be flagged to be not
>> hot-pluggable.
>
> Send a patch.

[x] done.

cheers,
   Gerd
Michael S. Tsirkin - Nov. 18, 2010, 11:33 a.m.
On Thu, Nov 18, 2010 at 11:57:51AM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 11:30:27AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> > > On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > > >>  If so: does qemu
> > > > > > > >>emulate this correctly?
> > > > > > > >
> > > > > > > >It mostly does.
> > > > > > > 
> > > > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > > > 
> > > > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > > > 
> > > > > So many guests (all of them?) just assume that vga ports and
> > > > > framebuffer is there.
> > > > 
> > > > Why do you think they disable io memory then?
> > > > 
> > > Who and how and when disables io memory?
> > 
> > I think guest will do this if you disable the device through the device
> > manager. This might need a reboot to become effective.
> > 
> Try to do it with primary VGA adapter and tell us what happens :)
> 
> > > Some guests are designed to run
> > > even on old ISA machines that have no way to disable anything. The
> > > device is just there.
> > > 
> > > This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> > > program them into PCI IO bars to be nice.
> > 
> > HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
> > the card, they are hardwired there. However, the card must not claim any
> > io transactions if IO memory is disabled in command register.
> > 
> Is this correct also for legacy ports?

Yes. The spec is quite explicit on this point:

A function that supports a PC legacy function (IDE, VGA, etc.) is
allowed to claim those addresses associated with the specific function
when the I/O Space (see Figure 6-2) enable bit is set.  These addresses
are not requested using a Base Address register but are assigned by
initialization software. If a device identifies itself as a legacy
function (class code), the initialization software grants the device
permission to claim the I/O legacy addresses by setting the device’s I/O
Space enable bit.


> This wouldn't be backwards
> compatible to ISA machines, so old software my not run properly back in
> the days when transaction from ISA to PCI happened.

initialization software could be the BIOS.
So maybe BIOS update was needed in the transition.

> So my guess is that
> old ISA ports works in backwards compatible way.

The spec seems to contradict this.

> > When qemu is started, it works correctly: the io memory is disabled and card does
> > not claim any io. Then BIOS comes along and enables io. At this point
> > map callback is invoked and maps io memory, card starts claiming io.
> Looking at the code I see that cirrus claims all IO ports and
> framebuffer memory during init function unconditionally.

So that may be OK for ISA, but not for PCI.

> > 
> > What is broken is that if BIOS/guest then disables IO memory,
> > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > That our emulation does this seems to be a clear spec violation, we are
> > just lucky that BIOS/guest does not do this at the moment.
> > 
> > 
> > 
> > > > > So what "fixing" this will buy us?
> > > > 
> > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > VGA device only.
> > > > 
> > > What spec defines hot-plug for primary VGA adapter?
> > 
> > No idea about hotplug. I am talking about multiple VGA cards,
> > enabling/disabling them dynamically should be possible.
> Of course. With properly designed VGA card you should be able to have
> more then one,

And, for that to have a chance to work when all cards are identical, you
don't claim IO when IO is disabled.

> but one of them will provide legacy functionality
> and is not removable.

The guest might not support hotplug. But there's no way
it can prevent surprise removal. qemu should not crash
when this happens.

> > 
> > > Our BIOS should support
> > > -M isa machine too.
> > 
> > > There is no way to disable VGA or even check if it
> > > is present there.
> > 
> > So I guess with isa you can't have multiple VGA cards work.  With PCI
> > I think you can.
> > 
> With isa you just need custom designed HW and software and you can have
> as many VGA cards as you want :)
> 
> --
> 			Gleb.
Gleb Natapov - Nov. 18, 2010, 11:55 a.m.
On Thu, Nov 18, 2010 at 01:33:08PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 11:57:51AM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 11:30:27AM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> > > > On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > > > > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > > > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > >>  If so: does qemu
> > > > > > > > >>emulate this correctly?
> > > > > > > > >
> > > > > > > > >It mostly does.
> > > > > > > > 
> > > > > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > > > > 
> > > > > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > > > > 
> > > > > > So many guests (all of them?) just assume that vga ports and
> > > > > > framebuffer is there.
> > > > > 
> > > > > Why do you think they disable io memory then?
> > > > > 
> > > > Who and how and when disables io memory?
> > > 
> > > I think guest will do this if you disable the device through the device
> > > manager. This might need a reboot to become effective.
> > > 
> > Try to do it with primary VGA adapter and tell us what happens :)
> > 
> > > > Some guests are designed to run
> > > > even on old ISA machines that have no way to disable anything. The
> > > > device is just there.
> > > > 
> > > > This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> > > > program them into PCI IO bars to be nice.
> > > 
> > > HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
> > > the card, they are hardwired there. However, the card must not claim any
> > > io transactions if IO memory is disabled in command register.
> > > 
> > Is this correct also for legacy ports?
> 
> Yes. The spec is quite explicit on this point:
> 
> A function that supports a PC legacy function (IDE, VGA, etc.) is
> allowed to claim those addresses associated with the specific function
> when the I/O Space (see Figure 6-2) enable bit is set.  These addresses
> are not requested using a Base Address register but are assigned by
> initialization software. If a device identifies itself as a legacy
> function (class code), the initialization software grants the device
> permission to claim the I/O legacy addresses by setting the device’s I/O
> Space enable bit.
> 
> 
What do they mean by "initialization software". How addresses is
assigned by initialization software without use of Base Address register? 
Looks like "initialization software" is something internal to HW. And
what spec says about legacy mmio?

> > This wouldn't be backwards
> > compatible to ISA machines, so old software my not run properly back in
> > the days when transaction from ISA to PCI happened.
> 
> initialization software could be the BIOS.
> So maybe BIOS update was needed in the transition.
> 
That is possible.

> > So my guess is that
> > old ISA ports works in backwards compatible way.
> 
> The spec seems to contradict this.
> 
> > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > not claim any io. Then BIOS comes along and enables io. At this point
> > > map callback is invoked and maps io memory, card starts claiming io.
> > Looking at the code I see that cirrus claims all IO ports and
> > framebuffer memory during init function unconditionally.
> 
> So that may be OK for ISA, but not for PCI.
> 
The code does it for both.

> > > 
> > > What is broken is that if BIOS/guest then disables IO memory,
> > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > That our emulation does this seems to be a clear spec violation, we are
> > > just lucky that BIOS/guest does not do this at the moment.
> > > 
> > > 
> > > 
> > > > > > So what "fixing" this will buy us?
> > > > > 
> > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > VGA device only.
> > > > > 
> > > > What spec defines hot-plug for primary VGA adapter?
> > > 
> > > No idea about hotplug. I am talking about multiple VGA cards,
> > > enabling/disabling them dynamically should be possible.
> > Of course. With properly designed VGA card you should be able to have
> > more then one,
> 
> And, for that to have a chance to work when all cards are identical, you
> don't claim IO when IO is disabled.
> 
But then only one card will be able to use IO since enabling IO on more
the one cards will cause conflict.

> > but one of them will provide legacy functionality
> > and is not removable.
> 
> The guest might not support hotplug. But there's no way
> it can prevent surprise removal. qemu should not crash
> when this happens.
Qemu can prevent any removal, surprise or not. Qemu can just
disallow device removal.

--
			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 12:03 p.m.
On Thu, Nov 18, 2010 at 01:55:29PM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 01:33:08PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 11:57:51AM +0200, Gleb Natapov wrote:
> > > On Thu, Nov 18, 2010 at 11:30:27AM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> > > > > On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > > > > > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > > > > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > > >>  If so: does qemu
> > > > > > > > > >>emulate this correctly?
> > > > > > > > > >
> > > > > > > > > >It mostly does.
> > > > > > > > > 
> > > > > > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > > > > > 
> > > > > > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > > > > > 
> > > > > > > So many guests (all of them?) just assume that vga ports and
> > > > > > > framebuffer is there.
> > > > > > 
> > > > > > Why do you think they disable io memory then?
> > > > > > 
> > > > > Who and how and when disables io memory?
> > > > 
> > > > I think guest will do this if you disable the device through the device
> > > > manager. This might need a reboot to become effective.
> > > > 
> > > Try to do it with primary VGA adapter and tell us what happens :)
> > > 
> > > > > Some guests are designed to run
> > > > > even on old ISA machines that have no way to disable anything. The
> > > > > device is just there.
> > > > > 
> > > > > This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> > > > > program them into PCI IO bars to be nice.
> > > > 
> > > > HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
> > > > the card, they are hardwired there. However, the card must not claim any
> > > > io transactions if IO memory is disabled in command register.
> > > > 
> > > Is this correct also for legacy ports?
> > 
> > Yes. The spec is quite explicit on this point:
> > 
> > A function that supports a PC legacy function (IDE, VGA, etc.) is
> > allowed to claim those addresses associated with the specific function
> > when the I/O Space (see Figure 6-2) enable bit is set.  These addresses
> > are not requested using a Base Address register but are assigned by
> > initialization software. If a device identifies itself as a legacy
> > function (class code), the initialization software grants the device
> > permission to claim the I/O legacy addresses by setting the device’s I/O
> > Space enable bit.
> > 
> > 
> What do they mean by "initialization software".

BIOS or OS.

> How addresses is
> assigned by initialization software without use of Base Address register? 

As it says:
" If a device identifies itself as a legacy
" function (class code), the initialization software grants the device
" permission to claim the I/O legacy addresses by setting the device’s
" I/O
" Space enable bit.

So you look at the class code and know which addresses will be claimed.
The relevant table is in the appendix D, take a look there.

> Looks like "initialization software" is something internal to HW.

Not really. Seabios does this simply by enabling io unconditionally.
It could easily detect multiple VGA cards and only enable one.

> And what spec says about legacy mmio?

What do you want to know?

> > > This wouldn't be backwards
> > > compatible to ISA machines, so old software my not run properly back in
> > > the days when transaction from ISA to PCI happened.
> > 
> > initialization software could be the BIOS.
> > So maybe BIOS update was needed in the transition.
> > 
> That is possible.
> 
> > > So my guess is that
> > > old ISA ports works in backwards compatible way.
> > 
> > The spec seems to contradict this.
> > 
> > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > map callback is invoked and maps io memory, card starts claiming io.
> > > Looking at the code I see that cirrus claims all IO ports and
> > > framebuffer memory during init function unconditionally.
> > 
> > So that may be OK for ISA, but not for PCI.
> > 
> The code does it for both.

Yep. So it's a bug.

> > > > 
> > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > That our emulation does this seems to be a clear spec violation, we are
> > > > just lucky that BIOS/guest does not do this at the moment.
> > > > 
> > > > 
> > > > 
> > > > > > > So what "fixing" this will buy us?
> > > > > > 
> > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > VGA device only.
> > > > > > 
> > > > > What spec defines hot-plug for primary VGA adapter?
> > > > 
> > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > enabling/disabling them dynamically should be possible.
> > > Of course. With properly designed VGA card you should be able to have
> > > more then one,
> > 
> > And, for that to have a chance to work when all cards are identical, you
> > don't claim IO when IO is disabled.
> > 
> But then only one card will be able to use IO since enabling IO on more
> the one cards will cause conflict.

Sure. That's life for legacy io though.

> > > but one of them will provide legacy functionality
> > > and is not removable.
> > 
> > The guest might not support hotplug. But there's no way
> > it can prevent surprise removal. qemu should not crash
> > when this happens.
> Qemu can prevent any removal, surprise or not. Qemu can just
> disallow device removal.

Yes, but that won't emulate real hardware faithfully.
On real hardware with a hotplug supporting slot
(and without an EM lock :) ) you can yank the card out
and the guest can do nothing about it.

> --
> 			Gleb.
Gleb Natapov - Nov. 18, 2010, 12:27 p.m.
On Thu, Nov 18, 2010 at 02:03:11PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 01:55:29PM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 01:33:08PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 18, 2010 at 11:57:51AM +0200, Gleb Natapov wrote:
> > > > On Thu, Nov 18, 2010 at 11:30:27AM +0200, Michael S. Tsirkin wrote:
> > > > > On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> > > > > > On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > > > > > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > > > >>  If so: does qemu
> > > > > > > > > > >>emulate this correctly?
> > > > > > > > > > >
> > > > > > > > > > >It mostly does.
> > > > > > > > > > 
> > > > > > > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > > > > > > 
> > > > > > > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > > > > > > 
> > > > > > > > So many guests (all of them?) just assume that vga ports and
> > > > > > > > framebuffer is there.
> > > > > > > 
> > > > > > > Why do you think they disable io memory then?
> > > > > > > 
> > > > > > Who and how and when disables io memory?
> > > > > 
> > > > > I think guest will do this if you disable the device through the device
> > > > > manager. This might need a reboot to become effective.
> > > > > 
> > > > Try to do it with primary VGA adapter and tell us what happens :)
> > > > 
> > > > > > Some guests are designed to run
> > > > > > even on old ISA machines that have no way to disable anything. The
> > > > > > device is just there.
> > > > > > 
> > > > > > This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> > > > > > program them into PCI IO bars to be nice.
> > > > > 
> > > > > HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
> > > > > the card, they are hardwired there. However, the card must not claim any
> > > > > io transactions if IO memory is disabled in command register.
> > > > > 
> > > > Is this correct also for legacy ports?
> > > 
> > > Yes. The spec is quite explicit on this point:
> > > 
> > > A function that supports a PC legacy function (IDE, VGA, etc.) is
> > > allowed to claim those addresses associated with the specific function
> > > when the I/O Space (see Figure 6-2) enable bit is set.  These addresses
> > > are not requested using a Base Address register but are assigned by
> > > initialization software. If a device identifies itself as a legacy
> > > function (class code), the initialization software grants the device
> > > permission to claim the I/O legacy addresses by setting the device’s I/O
> > > Space enable bit.
> > > 
> > > 
> > What do they mean by "initialization software".
> 
> BIOS or OS.
> 
> > How addresses is
> > assigned by initialization software without use of Base Address register? 
> 
> As it says:
> " If a device identifies itself as a legacy
> " function (class code), the initialization software grants the device
> " permission to claim the I/O legacy addresses by setting the device’s
> " I/O
> " Space enable bit.
> 
> So you look at the class code and know which addresses will be claimed.
> The relevant table is in the appendix D, take a look there.
> 
Strange wording "assigned by initialization software". Not "enabled",
but "assigned".

> > Looks like "initialization software" is something internal to HW.
> 
> Not really. Seabios does this simply by enabling io unconditionally.
> It could easily detect multiple VGA cards and only enable one.
> 
> > And what spec says about legacy mmio?
> 
> What do you want to know?
How it claims access to framebuffer. Legacy VGA has not only IO space
but MMIO space too.

> 
> > > > This wouldn't be backwards
> > > > compatible to ISA machines, so old software my not run properly back in
> > > > the days when transaction from ISA to PCI happened.
> > > 
> > > initialization software could be the BIOS.
> > > So maybe BIOS update was needed in the transition.
> > > 
> > That is possible.
> > 
> > > > So my guess is that
> > > > old ISA ports works in backwards compatible way.
> > > 
> > > The spec seems to contradict this.
> > > 
> > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > Looking at the code I see that cirrus claims all IO ports and
> > > > framebuffer memory during init function unconditionally.
> > > 
> > > So that may be OK for ISA, but not for PCI.
> > > 
> > The code does it for both.
> 
> Yep. So it's a bug.
> 
> > > > > 
> > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > 
> > > > > 
> > > > > 
> > > > > > > > So what "fixing" this will buy us?
> > > > > > > 
> > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > VGA device only.
> > > > > > > 
> > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > 
> > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > enabling/disabling them dynamically should be possible.
> > > > Of course. With properly designed VGA card you should be able to have
> > > > more then one,
> > > 
> > > And, for that to have a chance to work when all cards are identical, you
> > > don't claim IO when IO is disabled.
> > > 
> > But then only one card will be able to use IO since enabling IO on more
> > the one cards will cause conflict.
> 
> Sure. That's life for legacy io though.
> 
But that is the point. You can't have two regular VGA card
simultaneously. The card should be designed to work in legacy
mode and non-legacy mode. Then one of them will be used by legacy
software like BIOS and another will be driven from an OS by a driver
written specifically for the card.

> > > > but one of them will provide legacy functionality
> > > > and is not removable.
> > > 
> > > The guest might not support hotplug. But there's no way
> > > it can prevent surprise removal. qemu should not crash
> > > when this happens.
> > Qemu can prevent any removal, surprise or not. Qemu can just
> > disallow device removal.
> 
> Yes, but that won't emulate real hardware faithfully.
To the letter. There is no HW with hot-unplaggable primary
vga card. You are welcome to surprise remove vga card from your
machine and see what will happen.

> On real hardware with a hotplug supporting slot
> (and without an EM lock :) ) you can yank the card out
> and the guest can do nothing about it.
> 
And you will not find primary vga there.

--
			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 2:04 p.m.
On Thu, Nov 18, 2010 at 02:27:26PM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 02:03:11PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 01:55:29PM +0200, Gleb Natapov wrote:
> > > On Thu, Nov 18, 2010 at 01:33:08PM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 18, 2010 at 11:57:51AM +0200, Gleb Natapov wrote:
> > > > > On Thu, Nov 18, 2010 at 11:30:27AM +0200, Michael S. Tsirkin wrote:
> > > > > > On Thu, Nov 18, 2010 at 11:11:49AM +0200, Gleb Natapov wrote:
> > > > > > > On Thu, Nov 18, 2010 at 11:03:21AM +0200, Michael S. Tsirkin wrote:
> > > > > > > > On Thu, Nov 18, 2010 at 10:09:35AM +0200, Gleb Natapov wrote:
> > > > > > > > > On Wed, Nov 17, 2010 at 08:00:08PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > > > > >>  If so: does qemu
> > > > > > > > > > > >>emulate this correctly?
> > > > > > > > > > > >
> > > > > > > > > > > >It mostly does.
> > > > > > > > > > > 
> > > > > > > > > > > I doubt it actually enables/disables the legacy vga ports.
> > > > > > > > > > 
> > > > > > > > > > I'll check when I have the time. We can fix it if it doesn't,
> > > > > > > > > > 
> > > > > > > > > So many guests (all of them?) just assume that vga ports and
> > > > > > > > > framebuffer is there.
> > > > > > > > 
> > > > > > > > Why do you think they disable io memory then?
> > > > > > > > 
> > > > > > > Who and how and when disables io memory?
> > > > > > 
> > > > > > I think guest will do this if you disable the device through the device
> > > > > > manager. This might need a reboot to become effective.
> > > > > > 
> > > > > Try to do it with primary VGA adapter and tell us what happens :)
> > > > > 
> > > > > > > Some guests are designed to run
> > > > > > > even on old ISA machines that have no way to disable anything. The
> > > > > > > device is just there.
> > > > > > > 
> > > > > > > This is the same with IDE ports. BIOS "knows" legacy ISA ports and just
> > > > > > > program them into PCI IO bars to be nice.
> > > > > > 
> > > > > > HAven't checked IDE, for VGA AFAIK BIOS does not program legacy ports in
> > > > > > the card, they are hardwired there. However, the card must not claim any
> > > > > > io transactions if IO memory is disabled in command register.
> > > > > > 
> > > > > Is this correct also for legacy ports?
> > > > 
> > > > Yes. The spec is quite explicit on this point:
> > > > 
> > > > A function that supports a PC legacy function (IDE, VGA, etc.) is
> > > > allowed to claim those addresses associated with the specific function
> > > > when the I/O Space (see Figure 6-2) enable bit is set.  These addresses
> > > > are not requested using a Base Address register but are assigned by
> > > > initialization software. If a device identifies itself as a legacy
> > > > function (class code), the initialization software grants the device
> > > > permission to claim the I/O legacy addresses by setting the device’s I/O
> > > > Space enable bit.
> > > > 
> > > > 
> > > What do they mean by "initialization software".
> > 
> > BIOS or OS.
> > 
> > > How addresses is
> > > assigned by initialization software without use of Base Address register? 
> > 
> > As it says:
> > " If a device identifies itself as a legacy
> > " function (class code), the initialization software grants the device
> > " permission to claim the I/O legacy addresses by setting the device’s
> > " I/O
> > " Space enable bit.
> > 
> > So you look at the class code and know which addresses will be claimed.
> > The relevant table is in the appendix D, take a look there.
> > 
> Strange wording "assigned by initialization software". Not "enabled",
> but "assigned".
> 
> > > Looks like "initialization software" is something internal to HW.
> > 
> > Not really. Seabios does this simply by enabling io unconditionally.
> > It could easily detect multiple VGA cards and only enable one.
> > 
> > > And what spec says about legacy mmio?
> > 
> > What do you want to know?
> How it claims access to framebuffer. Legacy VGA has not only IO space
> but MMIO space too.

There's a separate bit to enable memory is that is what you
are asking about.

The spec specifies the address ranges:

	Base class 03. Sub-class 00. Interface 000 0000b:

	VGA-compatible controller. Memory addresses 0A 0000h through 0B FFFFh.
	I/O addresses 3B0h to 3BBh and 3C0h to 3DFh and all aliases of these
	addresses.


> > 
> > > > > This wouldn't be backwards
> > > > > compatible to ISA machines, so old software my not run properly back in
> > > > > the days when transaction from ISA to PCI happened.
> > > > 
> > > > initialization software could be the BIOS.
> > > > So maybe BIOS update was needed in the transition.
> > > > 
> > > That is possible.
> > > 
> > > > > So my guess is that
> > > > > old ISA ports works in backwards compatible way.
> > > > 
> > > > The spec seems to contradict this.
> > > > 
> > > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > > Looking at the code I see that cirrus claims all IO ports and
> > > > > framebuffer memory during init function unconditionally.
> > > > 
> > > > So that may be OK for ISA, but not for PCI.
> > > > 
> > > The code does it for both.
> > 
> > Yep. So it's a bug.
> > 
> > > > > > 
> > > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > > > So what "fixing" this will buy us?
> > > > > > > > 
> > > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > > VGA device only.
> > > > > > > > 
> > > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > > 
> > > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > > enabling/disabling them dynamically should be possible.
> > > > > Of course. With properly designed VGA card you should be able to have
> > > > > more then one,
> > > > 
> > > > And, for that to have a chance to work when all cards are identical, you
> > > > don't claim IO when IO is disabled.
> > > > 
> > > But then only one card will be able to use IO since enabling IO on more
> > > the one cards will cause conflict.
> > 
> > Sure. That's life for legacy io though.
> > 
> But that is the point. You can't have two regular VGA card
> simultaneously.

You can't *enable* them simultaneously. The fact that we cant create
them in qemu is a bug.

> The card should be designed to work in legacy
> mode and non-legacy mode. Then one of them will be used by legacy
> software like BIOS and another will be driven from an OS by a driver
> written specifically for the card.

There's nothing in the spec that says so. IMO it
should be possible to have two cards and have BIOS (or maybe even the
OS) select which one to use.

> > > > > but one of them will provide legacy functionality
> > > > > and is not removable.
> > > > 
> > > > The guest might not support hotplug. But there's no way
> > > > it can prevent surprise removal. qemu should not crash
> > > > when this happens.
> > > Qemu can prevent any removal, surprise or not. Qemu can just
> > > disallow device removal.
> > 
> > Yes, but that won't emulate real hardware faithfully.
> To the letter. There is no HW with hot-unplaggable primary
> vga card. You are welcome to surprise remove vga card from your
> machine and see what will happen.

This is different from removing any other card with hotplug module
unloaded in OS how?  OS might crash but so what? You can always reboot
it, hardware won't be damaged, so qemu shouldn't crash too.

> > On real hardware with a hotplug supporting slot
> > (and without an EM lock :) ) you can yank the card out
> > and the guest can do nothing about it.
> > 
> And you will not find primary vga there.

Where? PCI spec explicitly allows VGA cards behind expansion slots,
stick it there, and you can remove it too.

> --
> 			Gleb.
Gleb Natapov - Nov. 18, 2010, 2:57 p.m.
On Thu, Nov 18, 2010 at 04:04:14PM +0200, Michael S. Tsirkin wrote:
> > > What do you want to know?
> > How it claims access to framebuffer. Legacy VGA has not only IO space
> > but MMIO space too.
> 
> There's a separate bit to enable memory is that is what you
> are asking about.
> 
> The spec specifies the address ranges:
> 
> 	Base class 03. Sub-class 00. Interface 000 0000b:
> 
> 	VGA-compatible controller. Memory addresses 0A 0000h through 0B FFFFh.
> 	I/O addresses 3B0h to 3BBh and 3C0h to 3DFh and all aliases of these
> 	addresses.
> 
> 
So MMIO space is also not configurable? In short you can't insert two
vga card in two pci slots and use both simultaneously.

> > > > > > This wouldn't be backwards
> > > > > > compatible to ISA machines, so old software my not run properly back in
> > > > > > the days when transaction from ISA to PCI happened.
> > > > > 
> > > > > initialization software could be the BIOS.
> > > > > So maybe BIOS update was needed in the transition.
> > > > > 
> > > > That is possible.
> > > > 
> > > > > > So my guess is that
> > > > > > old ISA ports works in backwards compatible way.
> > > > > 
> > > > > The spec seems to contradict this.
> > > > > 
> > > > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > > > Looking at the code I see that cirrus claims all IO ports and
> > > > > > framebuffer memory during init function unconditionally.
> > > > > 
> > > > > So that may be OK for ISA, but not for PCI.
> > > > > 
> > > > The code does it for both.
> > > 
> > > Yep. So it's a bug.
> > > 
> > > > > > > 
> > > > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > > > So what "fixing" this will buy us?
> > > > > > > > > 
> > > > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > > > VGA device only.
> > > > > > > > > 
> > > > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > > > 
> > > > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > > > enabling/disabling them dynamically should be possible.
> > > > > > Of course. With properly designed VGA card you should be able to have
> > > > > > more then one,
> > > > > 
> > > > > And, for that to have a chance to work when all cards are identical, you
> > > > > don't claim IO when IO is disabled.
> > > > > 
> > > > But then only one card will be able to use IO since enabling IO on more
> > > > the one cards will cause conflict.
> > > 
> > > Sure. That's life for legacy io though.
> > > 
> > But that is the point. You can't have two regular VGA card
> > simultaneously.
> 
> You can't *enable* them simultaneously. The fact that we cant create
> them in qemu is a bug.
You can insert two of them on real HW too.

> 
> > The card should be designed to work in legacy
> > mode and non-legacy mode. Then one of them will be used by legacy
> > software like BIOS and another will be driven from an OS by a driver
> > written specifically for the card.
> 
> There's nothing in the spec that says so. IMO it
> should be possible to have two cards and have BIOS (or maybe even the
> OS) select which one to use.
Two use for what? If you have two cards you most probably want to use both
of them otherwise why have you put two of them in the same computer. But
only one of then can work in legacy mode and be usable without specialized
driver. This shouldn't be spelled out by the spec.

> 
> > > > > > but one of them will provide legacy functionality
> > > > > > and is not removable.
> > > > > 
> > > > > The guest might not support hotplug. But there's no way
> > > > > it can prevent surprise removal. qemu should not crash
> > > > > when this happens.
> > > > Qemu can prevent any removal, surprise or not. Qemu can just
> > > > disallow device removal.
> > > 
> > > Yes, but that won't emulate real hardware faithfully.
> > To the letter. There is no HW with hot-unplaggable primary
> > vga card. You are welcome to surprise remove vga card from your
> > machine and see what will happen.
> 
> This is different from removing any other card with hotplug module
> unloaded in OS how?  OS might crash but so what? You can always reboot
> it, hardware won't be damaged, so qemu shouldn't crash too.
If guest can't handle unplug there is no meaning for qemu to provide it.
You can have development mobo were chipset can be unplugged. Should we
allow to hot-unplug chipset?

> 
> > > On real hardware with a hotplug supporting slot
> > > (and without an EM lock :) ) you can yank the card out
> > > and the guest can do nothing about it.
> > > 
> > And you will not find primary vga there.
> 
> Where? PCI spec explicitly allows VGA cards behind expansion slots,
> stick it there, and you can remove it too.
> 
You can't remove PCI card just from any PCI slot IIRC. Slot and add-on
card should be specifically designed to be hot-unpluggable. (Something
to do with when power and data lines are disconnected during un-plug
IIRC). Not all things you can yank from mobo while OS is running are
"unpluggable" :)

--
			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 3:25 p.m.
On Thu, Nov 18, 2010 at 04:57:55PM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 04:04:14PM +0200, Michael S. Tsirkin wrote:
> > > > What do you want to know?
> > > How it claims access to framebuffer. Legacy VGA has not only IO space
> > > but MMIO space too.
> > 
> > There's a separate bit to enable memory is that is what you
> > are asking about.
> > 
> > The spec specifies the address ranges:
> > 
> > 	Base class 03. Sub-class 00. Interface 000 0000b:
> > 
> > 	VGA-compatible controller. Memory addresses 0A 0000h through 0B FFFFh.
> > 	I/O addresses 3B0h to 3BBh and 3C0h to 3DFh and all aliases of these
> > 	addresses.
> > 
> > 
> So MMIO space is also not configurable? In short you can't insert two
> vga card in two pci slots and use both simultaneously.

I think so, yes. But you can switch between them by disabling
one and enabling another one.

> > > > > > > This wouldn't be backwards
> > > > > > > compatible to ISA machines, so old software my not run properly back in
> > > > > > > the days when transaction from ISA to PCI happened.
> > > > > > 
> > > > > > initialization software could be the BIOS.
> > > > > > So maybe BIOS update was needed in the transition.
> > > > > > 
> > > > > That is possible.
> > > > > 
> > > > > > > So my guess is that
> > > > > > > old ISA ports works in backwards compatible way.
> > > > > > 
> > > > > > The spec seems to contradict this.
> > > > > > 
> > > > > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > > > > Looking at the code I see that cirrus claims all IO ports and
> > > > > > > framebuffer memory during init function unconditionally.
> > > > > > 
> > > > > > So that may be OK for ISA, but not for PCI.
> > > > > > 
> > > > > The code does it for both.
> > > > 
> > > > Yep. So it's a bug.
> > > > 
> > > > > > > > 
> > > > > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > > > So what "fixing" this will buy us?
> > > > > > > > > > 
> > > > > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > > > > VGA device only.
> > > > > > > > > > 
> > > > > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > > > > 
> > > > > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > > > > enabling/disabling them dynamically should be possible.
> > > > > > > Of course. With properly designed VGA card you should be able to have
> > > > > > > more then one,
> > > > > > 
> > > > > > And, for that to have a chance to work when all cards are identical, you
> > > > > > don't claim IO when IO is disabled.
> > > > > > 
> > > > > But then only one card will be able to use IO since enabling IO on more
> > > > > the one cards will cause conflict.
> > > > 
> > > > Sure. That's life for legacy io though.
> > > > 
> > > But that is the point. You can't have two regular VGA card
> > > simultaneously.
> > 
> > You can't *enable* them simultaneously. The fact that we cant create
> > them in qemu is a bug.
> You can insert two of them on real HW too.

Yes. You can insert any number of VGA cards on a PCI bus,
BIOS can configure one and disable the rest.

> > 
> > > The card should be designed to work in legacy
> > > mode and non-legacy mode. Then one of them will be used by legacy
> > > software like BIOS and another will be driven from an OS by a driver
> > > written specifically for the card.
> > 
> > There's nothing in the spec that says so. IMO it
> > should be possible to have two cards and have BIOS (or maybe even the
> > OS) select which one to use.
> Two use for what? If you have two cards you most probably want to use both
> of them otherwise why have you put two of them in the same computer.

There could be a variety of reasons, such as dual boot
where one card would work better with linux, another
with windows.

> But
> only one of then can work in legacy mode and be usable without specialized
> driver. This shouldn't be spelled out by the spec.

Yes. But guest software can control which.

> > 
> > > > > > > but one of them will provide legacy functionality
> > > > > > > and is not removable.
> > > > > > 
> > > > > > The guest might not support hotplug. But there's no way
> > > > > > it can prevent surprise removal. qemu should not crash
> > > > > > when this happens.
> > > > > Qemu can prevent any removal, surprise or not. Qemu can just
> > > > > disallow device removal.
> > > > 
> > > > Yes, but that won't emulate real hardware faithfully.
> > > To the letter. There is no HW with hot-unplaggable primary
> > > vga card. You are welcome to surprise remove vga card from your
> > > machine and see what will happen.
> > 
> > This is different from removing any other card with hotplug module
> > unloaded in OS how?  OS might crash but so what? You can always reboot
> > it, hardware won't be damaged, so qemu shouldn't crash too.
> If guest can't handle unplug there is no meaning for qemu to provide it.

Sure there's meaning. Giving guest access to backend
has security implications. We must have a way to revoke that
access even if guest misbehaves.

I expect surprise removal to be of most use for
assigned devices. But even for emulated devices, we have a small
number of slots available, so it would still be useful to free up the PCI slot,
even if guest needs to be rebooted then.

> You can have development mobo were chipset can be unplugged. Should we
> allow to hot-unplug chipset?

If it's useful, we could :)

> > 
> > > > On real hardware with a hotplug supporting slot
> > > > (and without an EM lock :) ) you can yank the card out
> > > > and the guest can do nothing about it.
> > > > 
> > > And you will not find primary vga there.
> > 
> > Where? PCI spec explicitly allows VGA cards behind expansion slots,
> > stick it there, and you can remove it too.
> > 
> You can't remove PCI card just from any PCI slot IIRC. Slot and add-on
> card should be specifically designed to be hot-unpluggable.
> (Something
> to do with when power and data lines are disconnected during un-plug
> IIRC). Not all things you can yank from mobo while OS is running are
> "unpluggable" :)

That's true for PCI/PCI-X. I didn't check PCI Express, maybe there all cards
are hotpluggable. Do you know?


> --
> 			Gleb.
Gleb Natapov - Nov. 18, 2010, 3:42 p.m.
On Thu, Nov 18, 2010 at 05:25:48PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 04:57:55PM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 04:04:14PM +0200, Michael S. Tsirkin wrote:
> > > > > What do you want to know?
> > > > How it claims access to framebuffer. Legacy VGA has not only IO space
> > > > but MMIO space too.
> > > 
> > > There's a separate bit to enable memory is that is what you
> > > are asking about.
> > > 
> > > The spec specifies the address ranges:
> > > 
> > > 	Base class 03. Sub-class 00. Interface 000 0000b:
> > > 
> > > 	VGA-compatible controller. Memory addresses 0A 0000h through 0B FFFFh.
> > > 	I/O addresses 3B0h to 3BBh and 3C0h to 3DFh and all aliases of these
> > > 	addresses.
> > > 
> > > 
> > So MMIO space is also not configurable? In short you can't insert two
> > vga card in two pci slots and use both simultaneously.
> 
> I think so, yes. But you can switch between them by disabling
> one and enabling another one.
> 
Sure. At init time.

> > > > > > > > This wouldn't be backwards
> > > > > > > > compatible to ISA machines, so old software my not run properly back in
> > > > > > > > the days when transaction from ISA to PCI happened.
> > > > > > > 
> > > > > > > initialization software could be the BIOS.
> > > > > > > So maybe BIOS update was needed in the transition.
> > > > > > > 
> > > > > > That is possible.
> > > > > > 
> > > > > > > > So my guess is that
> > > > > > > > old ISA ports works in backwards compatible way.
> > > > > > > 
> > > > > > > The spec seems to contradict this.
> > > > > > > 
> > > > > > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > > > > > Looking at the code I see that cirrus claims all IO ports and
> > > > > > > > framebuffer memory during init function unconditionally.
> > > > > > > 
> > > > > > > So that may be OK for ISA, but not for PCI.
> > > > > > > 
> > > > > > The code does it for both.
> > > > > 
> > > > > Yep. So it's a bug.
> > > > > 
> > > > > > > > > 
> > > > > > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > > > So what "fixing" this will buy us?
> > > > > > > > > > > 
> > > > > > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > > > > > VGA device only.
> > > > > > > > > > > 
> > > > > > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > > > > > 
> > > > > > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > > > > > enabling/disabling them dynamically should be possible.
> > > > > > > > Of course. With properly designed VGA card you should be able to have
> > > > > > > > more then one,
> > > > > > > 
> > > > > > > And, for that to have a chance to work when all cards are identical, you
> > > > > > > don't claim IO when IO is disabled.
> > > > > > > 
> > > > > > But then only one card will be able to use IO since enabling IO on more
> > > > > > the one cards will cause conflict.
> > > > > 
> > > > > Sure. That's life for legacy io though.
> > > > > 
> > > > But that is the point. You can't have two regular VGA card
> > > > simultaneously.
> > > 
> > > You can't *enable* them simultaneously. The fact that we cant create
> > > them in qemu is a bug.
> > You can insert two of them on real HW too.
> 
> Yes. You can insert any number of VGA cards on a PCI bus,
> BIOS can configure one and disable the rest.
> 
Bios can configure one as legacy. And all other can be happily used by
guest OS with proper drivers.

> > > > > > > > but one of them will provide legacy functionality
> > > > > > > > and is not removable.
> > > > > > > 
> > > > > > > The guest might not support hotplug. But there's no way
> > > > > > > it can prevent surprise removal. qemu should not crash
> > > > > > > when this happens.
> > > > > > Qemu can prevent any removal, surprise or not. Qemu can just
> > > > > > disallow device removal.
> > > > > 
> > > > > Yes, but that won't emulate real hardware faithfully.
> > > > To the letter. There is no HW with hot-unplaggable primary
> > > > vga card. You are welcome to surprise remove vga card from your
> > > > machine and see what will happen.
> > > 
> > > This is different from removing any other card with hotplug module
> > > unloaded in OS how?  OS might crash but so what? You can always reboot
> > > it, hardware won't be damaged, so qemu shouldn't crash too.
> > If guest can't handle unplug there is no meaning for qemu to provide it.
> 
> Sure there's meaning. Giving guest access to backend
> has security implications. We must have a way to revoke that
> access even if guest misbehaves.
> 
I am not sure what do you mean by "giving guest access to backend"?
Guest shouldn't be able to surprise removal VGA card, or any card at all
for that matter.

> I expect surprise removal to be of most use for
> assigned devices. But even for emulated devices, we have a small
> number of slots available, so it would still be useful to free up the PCI slot,
> even if guest needs to be rebooted then.
> 
We are talking about should we require primary VGA to be
hot-unplaggable. The last thing you want to remove to free PCI slots is
primary VGA card especially if no guest OS can handle it ;)

> > You can have development mobo were chipset can be unplugged. Should we
> > allow to hot-unplug chipset?
> 
> If it's useful, we could :)
On real HW you used to have memory controller there :)

> 
> > > 
> > > > > On real hardware with a hotplug supporting slot
> > > > > (and without an EM lock :) ) you can yank the card out
> > > > > and the guest can do nothing about it.
> > > > > 
> > > > And you will not find primary vga there.
> > > 
> > > Where? PCI spec explicitly allows VGA cards behind expansion slots,
> > > stick it there, and you can remove it too.
> > > 
> > You can't remove PCI card just from any PCI slot IIRC. Slot and add-on
> > card should be specifically designed to be hot-unpluggable.
> > (Something
> > to do with when power and data lines are disconnected during un-plug
> > IIRC). Not all things you can yank from mobo while OS is running are
> > "unpluggable" :)
> 
> That's true for PCI/PCI-X. I didn't check PCI Express, maybe there all cards
> are hotpluggable. Do you know?
> 
> 
Nope, no idea. On PCI Express you have much less lines to care about
though.

--
			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 4:04 p.m.
On Thu, Nov 18, 2010 at 05:42:41PM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 05:25:48PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 04:57:55PM +0200, Gleb Natapov wrote:
> > > On Thu, Nov 18, 2010 at 04:04:14PM +0200, Michael S. Tsirkin wrote:
> > > > > > What do you want to know?
> > > > > How it claims access to framebuffer. Legacy VGA has not only IO space
> > > > > but MMIO space too.
> > > > 
> > > > There's a separate bit to enable memory is that is what you
> > > > are asking about.
> > > > 
> > > > The spec specifies the address ranges:
> > > > 
> > > > 	Base class 03. Sub-class 00. Interface 000 0000b:
> > > > 
> > > > 	VGA-compatible controller. Memory addresses 0A 0000h through 0B FFFFh.
> > > > 	I/O addresses 3B0h to 3BBh and 3C0h to 3DFh and all aliases of these
> > > > 	addresses.
> > > > 
> > > > 
> > > So MMIO space is also not configurable? In short you can't insert two
> > > vga card in two pci slots and use both simultaneously.
> > 
> > I think so, yes. But you can switch between them by disabling
> > one and enabling another one.
> > 
> Sure. At init time.
> 
> > > > > > > > > This wouldn't be backwards
> > > > > > > > > compatible to ISA machines, so old software my not run properly back in
> > > > > > > > > the days when transaction from ISA to PCI happened.
> > > > > > > > 
> > > > > > > > initialization software could be the BIOS.
> > > > > > > > So maybe BIOS update was needed in the transition.
> > > > > > > > 
> > > > > > > That is possible.
> > > > > > > 
> > > > > > > > > So my guess is that
> > > > > > > > > old ISA ports works in backwards compatible way.
> > > > > > > > 
> > > > > > > > The spec seems to contradict this.
> > > > > > > > 
> > > > > > > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > > > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > > > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > > > > > > Looking at the code I see that cirrus claims all IO ports and
> > > > > > > > > framebuffer memory during init function unconditionally.
> > > > > > > > 
> > > > > > > > So that may be OK for ISA, but not for PCI.
> > > > > > > > 
> > > > > > > The code does it for both.
> > > > > > 
> > > > > > Yep. So it's a bug.
> > > > > > 
> > > > > > > > > > 
> > > > > > > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > > > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > > > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > > > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > > > So what "fixing" this will buy us?
> > > > > > > > > > > > 
> > > > > > > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > > > > > > VGA device only.
> > > > > > > > > > > > 
> > > > > > > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > > > > > > 
> > > > > > > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > > > > > > enabling/disabling them dynamically should be possible.
> > > > > > > > > Of course. With properly designed VGA card you should be able to have
> > > > > > > > > more then one,
> > > > > > > > 
> > > > > > > > And, for that to have a chance to work when all cards are identical, you
> > > > > > > > don't claim IO when IO is disabled.
> > > > > > > > 
> > > > > > > But then only one card will be able to use IO since enabling IO on more
> > > > > > > the one cards will cause conflict.
> > > > > > 
> > > > > > Sure. That's life for legacy io though.
> > > > > > 
> > > > > But that is the point. You can't have two regular VGA card
> > > > > simultaneously.
> > > > 
> > > > You can't *enable* them simultaneously. The fact that we cant create
> > > > them in qemu is a bug.
> > > You can insert two of them on real HW too.
> > 
> > Yes. You can insert any number of VGA cards on a PCI bus,
> > BIOS can configure one and disable the rest.
> > 
> Bios can configure one as legacy. And all other can be happily used by
> guest OS with proper drivers.

Maybe. There's no standard way to disable 'legacy mode' though.
So a card might or might not keep claiming the legacy ranges (assuming
io/memory is enabled).  Maybe if you want to use multiple cards all of
them need drivers.

> > > > > > > > > but one of them will provide legacy functionality
> > > > > > > > > and is not removable.
> > > > > > > > 
> > > > > > > > The guest might not support hotplug. But there's no way
> > > > > > > > it can prevent surprise removal. qemu should not crash
> > > > > > > > when this happens.
> > > > > > > Qemu can prevent any removal, surprise or not. Qemu can just
> > > > > > > disallow device removal.
> > > > > > 
> > > > > > Yes, but that won't emulate real hardware faithfully.
> > > > > To the letter. There is no HW with hot-unplaggable primary
> > > > > vga card. You are welcome to surprise remove vga card from your
> > > > > machine and see what will happen.
> > > > 
> > > > This is different from removing any other card with hotplug module
> > > > unloaded in OS how?  OS might crash but so what? You can always reboot
> > > > it, hardware won't be damaged, so qemu shouldn't crash too.
> > > If guest can't handle unplug there is no meaning for qemu to provide it.
> > 
> > Sure there's meaning. Giving guest access to backend
> > has security implications. We must have a way to revoke that
> > access even if guest misbehaves.
> > 
> I am not sure what do you mean by "giving guest access to backend"?
> Guest shouldn't be able to surprise removal VGA card, or any card at all
> for that matter.

WHQL includes surprise removal tests. So any card that passed
that will work with surprise removal.

> > I expect surprise removal to be of most use for
> > assigned devices. But even for emulated devices, we have a small
> > number of slots available, so it would still be useful to free up the PCI slot,
> > even if guest needs to be rebooted then.
> > 
> We are talking about should we require primary VGA to be
> hot-unplaggable. The last thing you want to remove to free PCI slots is
> primary VGA card especially if no guest OS can handle it ;)

What I am saying is we need surprise removal generally.
If won't be too bad if we make these commands fail for VGA.

> > > You can have development mobo were chipset can be unplugged. Should we
> > > allow to hot-unplug chipset?
> > 
> > If it's useful, we could :)
> On real HW you used to have memory controller there :)
> 
> > 
> > > > 
> > > > > > On real hardware with a hotplug supporting slot
> > > > > > (and without an EM lock :) ) you can yank the card out
> > > > > > and the guest can do nothing about it.
> > > > > > 
> > > > > And you will not find primary vga there.
> > > > 
> > > > Where? PCI spec explicitly allows VGA cards behind expansion slots,
> > > > stick it there, and you can remove it too.
> > > > 
> > > You can't remove PCI card just from any PCI slot IIRC. Slot and add-on
> > > card should be specifically designed to be hot-unpluggable.
> > > (Something
> > > to do with when power and data lines are disconnected during un-plug
> > > IIRC). Not all things you can yank from mobo while OS is running are
> > > "unpluggable" :)
> > 
> > That's true for PCI/PCI-X. I didn't check PCI Express, maybe there all cards
> > are hotpluggable. Do you know?
> > 
> > 
> Nope, no idea. On PCI Express you have much less lines to care about
> though.
> 
> --
> 			Gleb.
Gleb Natapov - Nov. 18, 2010, 4:10 p.m.
On Thu, Nov 18, 2010 at 06:04:56PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 05:42:41PM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 05:25:48PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 18, 2010 at 04:57:55PM +0200, Gleb Natapov wrote:
> > > > On Thu, Nov 18, 2010 at 04:04:14PM +0200, Michael S. Tsirkin wrote:
> > > > > > > What do you want to know?
> > > > > > How it claims access to framebuffer. Legacy VGA has not only IO space
> > > > > > but MMIO space too.
> > > > > 
> > > > > There's a separate bit to enable memory is that is what you
> > > > > are asking about.
> > > > > 
> > > > > The spec specifies the address ranges:
> > > > > 
> > > > > 	Base class 03. Sub-class 00. Interface 000 0000b:
> > > > > 
> > > > > 	VGA-compatible controller. Memory addresses 0A 0000h through 0B FFFFh.
> > > > > 	I/O addresses 3B0h to 3BBh and 3C0h to 3DFh and all aliases of these
> > > > > 	addresses.
> > > > > 
> > > > > 
> > > > So MMIO space is also not configurable? In short you can't insert two
> > > > vga card in two pci slots and use both simultaneously.
> > > 
> > > I think so, yes. But you can switch between them by disabling
> > > one and enabling another one.
> > > 
> > Sure. At init time.
> > 
> > > > > > > > > > This wouldn't be backwards
> > > > > > > > > > compatible to ISA machines, so old software my not run properly back in
> > > > > > > > > > the days when transaction from ISA to PCI happened.
> > > > > > > > > 
> > > > > > > > > initialization software could be the BIOS.
> > > > > > > > > So maybe BIOS update was needed in the transition.
> > > > > > > > > 
> > > > > > > > That is possible.
> > > > > > > > 
> > > > > > > > > > So my guess is that
> > > > > > > > > > old ISA ports works in backwards compatible way.
> > > > > > > > > 
> > > > > > > > > The spec seems to contradict this.
> > > > > > > > > 
> > > > > > > > > > > When qemu is started, it works correctly: the io memory is disabled and card does
> > > > > > > > > > > not claim any io. Then BIOS comes along and enables io. At this point
> > > > > > > > > > > map callback is invoked and maps io memory, card starts claiming io.
> > > > > > > > > > Looking at the code I see that cirrus claims all IO ports and
> > > > > > > > > > framebuffer memory during init function unconditionally.
> > > > > > > > > 
> > > > > > > > > So that may be OK for ISA, but not for PCI.
> > > > > > > > > 
> > > > > > > > The code does it for both.
> > > > > > > 
> > > > > > > Yep. So it's a bug.
> > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > What is broken is that if BIOS/guest then disables IO memory,
> > > > > > > > > > > (I think - even if guest is rebooted!) we will keep claiming IO transactions.
> > > > > > > > > > > That our emulation does this seems to be a clear spec violation, we are
> > > > > > > > > > > just lucky that BIOS/guest does not do this at the moment.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > > > So what "fixing" this will buy us?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Besides spec compliancy, you mean?  Ability to support multiple VGA
> > > > > > > > > > > > > cards. That's how it works I think: BIOS enables IO on the primary
> > > > > > > > > > > > > VGA device only.
> > > > > > > > > > > > > 
> > > > > > > > > > > > What spec defines hot-plug for primary VGA adapter?
> > > > > > > > > > > 
> > > > > > > > > > > No idea about hotplug. I am talking about multiple VGA cards,
> > > > > > > > > > > enabling/disabling them dynamically should be possible.
> > > > > > > > > > Of course. With properly designed VGA card you should be able to have
> > > > > > > > > > more then one,
> > > > > > > > > 
> > > > > > > > > And, for that to have a chance to work when all cards are identical, you
> > > > > > > > > don't claim IO when IO is disabled.
> > > > > > > > > 
> > > > > > > > But then only one card will be able to use IO since enabling IO on more
> > > > > > > > the one cards will cause conflict.
> > > > > > > 
> > > > > > > Sure. That's life for legacy io though.
> > > > > > > 
> > > > > > But that is the point. You can't have two regular VGA card
> > > > > > simultaneously.
> > > > > 
> > > > > You can't *enable* them simultaneously. The fact that we cant create
> > > > > them in qemu is a bug.
> > > > You can insert two of them on real HW too.
> > > 
> > > Yes. You can insert any number of VGA cards on a PCI bus,
> > > BIOS can configure one and disable the rest.
> > > 
> > Bios can configure one as legacy. And all other can be happily used by
> > guest OS with proper drivers.
> 
> Maybe. There's no standard way to disable 'legacy mode' though.
> So a card might or might not keep claiming the legacy ranges (assuming
> io/memory is enabled).  Maybe if you want to use multiple cards all of
> them need drivers.
> 
May be.

> > > > > > > > > > but one of them will provide legacy functionality
> > > > > > > > > > and is not removable.
> > > > > > > > > 
> > > > > > > > > The guest might not support hotplug. But there's no way
> > > > > > > > > it can prevent surprise removal. qemu should not crash
> > > > > > > > > when this happens.
> > > > > > > > Qemu can prevent any removal, surprise or not. Qemu can just
> > > > > > > > disallow device removal.
> > > > > > > 
> > > > > > > Yes, but that won't emulate real hardware faithfully.
> > > > > > To the letter. There is no HW with hot-unplaggable primary
> > > > > > vga card. You are welcome to surprise remove vga card from your
> > > > > > machine and see what will happen.
> > > > > 
> > > > > This is different from removing any other card with hotplug module
> > > > > unloaded in OS how?  OS might crash but so what? You can always reboot
> > > > > it, hardware won't be damaged, so qemu shouldn't crash too.
> > > > If guest can't handle unplug there is no meaning for qemu to provide it.
> > > 
> > > Sure there's meaning. Giving guest access to backend
> > > has security implications. We must have a way to revoke that
> > > access even if guest misbehaves.
> > > 
> > I am not sure what do you mean by "giving guest access to backend"?
> > Guest shouldn't be able to surprise removal VGA card, or any card at all
> > for that matter.
> 
> WHQL includes surprise removal tests. So any card that passed
> that will work with surprise removal.
> 
Yeah. But it is not real "surprise removal". It will not crash qemu.

> > > I expect surprise removal to be of most use for
> > > assigned devices. But even for emulated devices, we have a small
> > > number of slots available, so it would still be useful to free up the PCI slot,
> > > even if guest needs to be rebooted then.
> > > 
> > We are talking about should we require primary VGA to be
> > hot-unplaggable. The last thing you want to remove to free PCI slots is
> > primary VGA card especially if no guest OS can handle it ;)
> 
> What I am saying is we need surprise removal generally.
For all HW that we allow to remove from monitor sure.

> If won't be too bad if we make these commands fail for VGA.
> 
The patch is on the list already.

--
			Gleb.
Michael S. Tsirkin - Nov. 18, 2010, 4:14 p.m.
On Thu, Nov 18, 2010 at 06:10:12PM +0200, Gleb Natapov wrote:
> > WHQL includes surprise removal tests. So any card that passed
> > that will work with surprise removal.
> > 
> Yeah. But it is not real "surprise removal". It will not crash qemu.

That was the problem I saw. That the way qxl is written, surprise
removal could crash qemu.

> > > > I expect surprise removal to be of most use for
> > > > assigned devices. But even for emulated devices, we have a small
> > > > number of slots available, so it would still be useful to free up the PCI slot,
> > > > even if guest needs to be rebooted then.
> > > > 
> > > We are talking about should we require primary VGA to be
> > > hot-unplaggable. The last thing you want to remove to free PCI slots is
> > > primary VGA card especially if no guest OS can handle it ;)
> > 
> > What I am saying is we need surprise removal generally.
> For all HW that we allow to remove from monitor sure.
> 
> > If won't be too bad if we make these commands fail for VGA.
> > 
> The patch is on the list already.

That's good. If my review is needed, Cc me so it won't get lost
in the noise.

Patch

diff --git a/Makefile.target b/Makefile.target
index 91e6e74..6c4d64c 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -203,6 +203,7 @@  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
 obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += debugcon.o multiboot.o
 obj-i386-y += pc_piix.o
+obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
 
 # shared objects
 obj-ppc-y = ppc.o
diff --git a/hw/hw.h b/hw/hw.h
index 9d2cfc2..234c713 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -528,6 +528,17 @@  extern const VMStateInfo vmstate_info_unused_buffer;
     .start        = (_start),                                        \
 }
 
+#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
+    .name         = (stringify(_field)),                             \
+    .version_id   = (_version),                                      \
+    .field_exists = (_test),                                         \
+    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
+    .info         = &vmstate_info_buffer,                            \
+    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
+    .offset       = offsetof(_state, _field),                        \
+    .start        = (_start),                                        \
+}
+
 #define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, _size) { \
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
@@ -745,6 +756,9 @@  extern const VMStateDescription vmstate_i2c_slave;
 #define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
     VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
 
+#define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
+    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
+
 #define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
     VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
 
diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..0c31db1 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -40,6 +40,7 @@ 
 #include "sysbus.h"
 #include "sysemu.h"
 #include "blockdev.h"
+#include "ui/qemu-spice.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -991,6 +992,13 @@  void pc_vga_init(PCIBus *pci_bus)
             pci_vmsvga_init(pci_bus);
         else
             fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__);
+#ifdef CONFIG_SPICE
+    } else if (qxl_enabled) {
+        if (pci_bus)
+            pci_create_simple(pci_bus, -1, "qxl");
+        else
+            fprintf(stderr, "%s: qxl: no PCI bus\n", __FUNCTION__);
+#endif
     } else if (std_vga_enabled) {
         if (pci_bus) {
             pci_vga_init(pci_bus, 0, 0);
diff --git a/hw/qxl-logger.c b/hw/qxl-logger.c
new file mode 100644
index 0000000..cad5165
--- /dev/null
+++ b/hw/qxl-logger.c
@@ -0,0 +1,236 @@ 
+/*
+ * qxl command logging -- for debug purposes
+ */
+
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+
+#include "qxl.h"
+
+static const char *qxl_type[] = {
+    [ QXL_CMD_NOP ]     = "nop",
+    [ QXL_CMD_DRAW ]    = "draw",
+    [ QXL_CMD_UPDATE ]  = "update",
+    [ QXL_CMD_CURSOR ]  = "cursor",
+    [ QXL_CMD_MESSAGE ] = "message",
+    [ QXL_CMD_SURFACE ] = "surface",
+};
+
+static const char *qxl_draw_type[] = {
+    [ QXL_DRAW_NOP         ] = "nop",
+    [ QXL_DRAW_FILL        ] = "fill",
+    [ QXL_DRAW_OPAQUE      ] = "opaque",
+    [ QXL_DRAW_COPY        ] = "copy",
+    [ QXL_COPY_BITS        ] = "copy-bits",
+    [ QXL_DRAW_BLEND       ] = "blend",
+    [ QXL_DRAW_BLACKNESS   ] = "blackness",
+    [ QXL_DRAW_WHITENESS   ] = "whitemess",
+    [ QXL_DRAW_INVERS      ] = "invers",
+    [ QXL_DRAW_ROP3        ] = "rop3",
+    [ QXL_DRAW_STROKE      ] = "stroke",
+    [ QXL_DRAW_TEXT        ] = "text",
+    [ QXL_DRAW_TRANSPARENT ] = "transparent",
+    [ QXL_DRAW_ALPHA_BLEND ] = "alpha-blend",
+};
+
+static const char *qxl_draw_effect[] = {
+    [ QXL_EFFECT_BLEND            ] = "blend",
+    [ QXL_EFFECT_OPAQUE           ] = "opaque",
+    [ QXL_EFFECT_REVERT_ON_DUP    ] = "revert-on-dup",
+    [ QXL_EFFECT_BLACKNESS_ON_DUP ] = "blackness-on-dup",
+    [ QXL_EFFECT_WHITENESS_ON_DUP ] = "whiteness-on-dup",
+    [ QXL_EFFECT_NOP_ON_DUP       ] = "nop-on-dup",
+    [ QXL_EFFECT_NOP              ] = "nop",
+    [ QXL_EFFECT_OPAQUE_BRUSH     ] = "opaque-brush",
+};
+
+static const char *qxl_surface_cmd[] = {
+   [ QXL_SURFACE_CMD_CREATE  ] = "create",
+   [ QXL_SURFACE_CMD_DESTROY ] = "destroy",
+};
+
+static const char *spice_surface_fmt[] = {
+   [ SPICE_SURFACE_FMT_INVALID  ] = "invalid",
+   [ SPICE_SURFACE_FMT_1_A      ] = "alpha/1",
+   [ SPICE_SURFACE_FMT_8_A      ] = "alpha/8",
+   [ SPICE_SURFACE_FMT_16_555   ] = "555/16",
+   [ SPICE_SURFACE_FMT_16_565   ] = "565/16",
+   [ SPICE_SURFACE_FMT_32_xRGB  ] = "xRGB/32",
+   [ SPICE_SURFACE_FMT_32_ARGB  ] = "ARGB/32",
+};
+
+static const char *qxl_cursor_cmd[] = {
+   [ QXL_CURSOR_SET   ] = "set",
+   [ QXL_CURSOR_MOVE  ] = "move",
+   [ QXL_CURSOR_HIDE  ] = "hide",
+   [ QXL_CURSOR_TRAIL ] = "trail",
+};
+
+static const char *spice_cursor_type[] = {
+   [ SPICE_CURSOR_TYPE_ALPHA   ] = "alpha",
+   [ SPICE_CURSOR_TYPE_MONO    ] = "mono",
+   [ SPICE_CURSOR_TYPE_COLOR4  ] = "color4",
+   [ SPICE_CURSOR_TYPE_COLOR8  ] = "color8",
+   [ SPICE_CURSOR_TYPE_COLOR16 ] = "color16",
+   [ SPICE_CURSOR_TYPE_COLOR24 ] = "color24",
+   [ SPICE_CURSOR_TYPE_COLOR32 ] = "color32",
+};
+
+static const char *qxl_v2n(const char *n[], size_t l, int v)
+{
+    if (v >= l || !n[v]) {
+        return "???";
+    }
+    return n[v];
+}
+#define qxl_name(_list, _value) qxl_v2n(_list, ARRAY_SIZE(_list), _value)
+
+static void qxl_log_image(PCIQXLDevice *qxl, QXLPHYSICAL addr, int group_id)
+{
+    QXLImage *image;
+    QXLImageDescriptor *desc;
+
+    image = qxl_phys2virt(qxl, addr, group_id);
+    desc = &image->descriptor;
+    fprintf(stderr, " (id %" PRIx64 " type %d flags %d width %d height %d",
+            desc->id, desc->type, desc->flags, desc->width, desc->height);
+    switch (desc->type) {
+    case SPICE_IMAGE_TYPE_BITMAP:
+        fprintf(stderr, ", fmt %d flags %d x %d y %d stride %d"
+                " palette %" PRIx64 " data %" PRIx64,
+                image->bitmap.format, image->bitmap.flags,
+                image->bitmap.x, image->bitmap.y,
+                image->bitmap.stride,
+                image->bitmap.palette, image->bitmap.data);
+        break;
+    }
+    fprintf(stderr, ")");
+}
+
+static void qxl_log_rect(QXLRect *rect)
+{
+    fprintf(stderr, " %dx%d+%d+%d",
+            rect->right - rect->left,
+            rect->bottom - rect->top,
+            rect->left, rect->top);
+}
+
+static void qxl_log_cmd_draw_copy(PCIQXLDevice *qxl, QXLCopy *copy, int group_id)
+{
+    fprintf(stderr, " src %" PRIx64,
+            copy->src_bitmap);
+    qxl_log_image(qxl, copy->src_bitmap, group_id);
+    fprintf(stderr, " area");
+    qxl_log_rect(&copy->src_area);
+    fprintf(stderr, " rop %d", copy->rop_descriptor);
+}
+
+static void qxl_log_cmd_draw(PCIQXLDevice *qxl, QXLDrawable *draw, int group_id)
+{
+    fprintf(stderr, ": surface_id %d type %s effect %s",
+            draw->surface_id,
+            qxl_name(qxl_draw_type, draw->type),
+            qxl_name(qxl_draw_effect, draw->effect));
+    switch (draw->type) {
+    case QXL_DRAW_COPY:
+        qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id);
+        break;
+    }
+}
+
+static void qxl_log_cmd_draw_compat(PCIQXLDevice *qxl, QXLCompatDrawable *draw,
+                                    int group_id)
+{
+    fprintf(stderr, ": type %s effect %s",
+            qxl_name(qxl_draw_type, draw->type),
+            qxl_name(qxl_draw_effect, draw->effect));
+    if (draw->bitmap_offset) {
+        fprintf(stderr, ": bitmap %d",
+                draw->bitmap_offset);
+        qxl_log_rect(&draw->bitmap_area);
+    }
+    switch (draw->type) {
+    case QXL_DRAW_COPY:
+        qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id);
+        break;
+    }
+}
+
+static void qxl_log_cmd_surface(PCIQXLDevice *qxl, QXLSurfaceCmd *cmd)
+{
+    fprintf(stderr, ": %s id %d",
+            qxl_name(qxl_surface_cmd, cmd->type),
+            cmd->surface_id);
+    if (cmd->type == QXL_SURFACE_CMD_CREATE) {
+        fprintf(stderr, " size %dx%d stride %d format %s (count %d, max %d)",
+                cmd->u.surface_create.width,
+                cmd->u.surface_create.height,
+                cmd->u.surface_create.stride,
+                qxl_name(spice_surface_fmt, cmd->u.surface_create.format),
+                qxl->guest_surfaces.count, qxl->guest_surfaces.max);
+    }
+    if (cmd->type == QXL_SURFACE_CMD_DESTROY) {
+        fprintf(stderr, " (count %d)", qxl->guest_surfaces.count);
+    }
+}
+
+void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id)
+{
+    QXLCursor *cursor;
+
+    fprintf(stderr, ": %s",
+            qxl_name(qxl_cursor_cmd, cmd->type));
+    switch (cmd->type) {
+    case QXL_CURSOR_SET:
+        fprintf(stderr, " +%d+%d visible %s, shape @ 0x%" PRIx64,
+                cmd->u.set.position.x,
+                cmd->u.set.position.y,
+                cmd->u.set.visible ? "yes" : "no",
+                cmd->u.set.shape);
+        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id);
+        fprintf(stderr, " type %s size %dx%d hot-spot +%d+%d"
+                " unique 0x%" PRIx64 " data-size %d",
+                qxl_name(spice_cursor_type, cursor->header.type),
+                cursor->header.width, cursor->header.height,
+                cursor->header.hot_spot_x, cursor->header.hot_spot_y,
+                cursor->header.unique, cursor->data_size);
+        break;
+    case QXL_CURSOR_MOVE:
+        fprintf(stderr, " +%d+%d", cmd->u.position.x, cmd->u.position.y);
+        break;
+    }
+}
+
+void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
+{
+    bool compat = ext->flags & QXL_COMMAND_FLAG_COMPAT;
+    void *data;
+
+    if (!qxl->cmdlog) {
+        return;
+    }
+    fprintf(stderr, "qxl-%d/%s:", qxl->id, ring);
+    fprintf(stderr, " cmd @ 0x%" PRIx64 " %s%s", ext->cmd.data,
+            qxl_name(qxl_type, ext->cmd.type),
+            compat ? "(compat)" : "");
+
+    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+    switch (ext->cmd.type) {
+    case QXL_CMD_DRAW:
+        if (!compat) {
+            qxl_log_cmd_draw(qxl, data, ext->group_id);
+        } else {
+            qxl_log_cmd_draw_compat(qxl, data, ext->group_id);
+        }
+        break;
+    case QXL_CMD_SURFACE:
+        qxl_log_cmd_surface(qxl, data);
+        break;
+    case QXL_CMD_CURSOR:
+        qxl_log_cmd_cursor(qxl, data, ext->group_id);
+        break;
+    }
+    fprintf(stderr, "\n");
+}
diff --git a/hw/qxl-render.c b/hw/qxl-render.c
new file mode 100644
index 0000000..e3941c8
--- /dev/null
+++ b/hw/qxl-render.c
@@ -0,0 +1,213 @@ 
+/*
+ * qxl local rendering (aka display on sdl/vnc)
+ */
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+
+#include "qxl.h"
+
+static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect)
+{
+    uint8_t *src = qxl->guest_primary.data;
+    uint8_t *dst = qxl->guest_primary.flipped;
+    int len, i;
+
+    src += (qxl->guest_primary.surface.height - rect->top - 1) *
+        qxl->guest_primary.stride;
+    dst += rect->top  * qxl->guest_primary.stride;
+    src += rect->left * qxl->guest_primary.bytes_pp;
+    dst += rect->left * qxl->guest_primary.bytes_pp;
+    len  = (rect->right - rect->left) * qxl->guest_primary.bytes_pp;
+
+    for (i = rect->top; i < rect->bottom; i++) {
+        memcpy(dst, src, len);
+        dst += qxl->guest_primary.stride;
+        src -= qxl->guest_primary.stride;
+    }
+}
+
+void qxl_render_resize(PCIQXLDevice *qxl)
+{
+    QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
+
+    qxl->guest_primary.stride = sc->stride;
+    qxl->guest_primary.resized++;
+    switch (sc->format) {
+    case SPICE_SURFACE_FMT_16_555:
+        qxl->guest_primary.bytes_pp = 2;
+        qxl->guest_primary.bits_pp = 15;
+        break;
+    case SPICE_SURFACE_FMT_16_565:
+        qxl->guest_primary.bytes_pp = 2;
+        qxl->guest_primary.bits_pp = 16;
+        break;
+    case SPICE_SURFACE_FMT_32_xRGB:
+    case SPICE_SURFACE_FMT_32_ARGB:
+        qxl->guest_primary.bytes_pp = 4;
+        qxl->guest_primary.bits_pp = 32;
+        break;
+    default:
+        fprintf(stderr, "%s: unhandled format: %x\n", __FUNCTION__,
+                qxl->guest_primary.surface.format);
+        qxl->guest_primary.bytes_pp = 4;
+        qxl->guest_primary.bits_pp = 32;
+        break;
+    }
+}
+
+void qxl_render_update(PCIQXLDevice *qxl)
+{
+    VGACommonState *vga = &qxl->vga;
+    QXLRect dirty[32], update;
+    void *ptr;
+    int i;
+
+    if (qxl->guest_primary.resized) {
+        qxl->guest_primary.resized = 0;
+
+        if (qxl->guest_primary.flipped) {
+            qemu_free(qxl->guest_primary.flipped);
+            qxl->guest_primary.flipped = NULL;
+        }
+        qemu_free_displaysurface(vga->ds);
+
+        qxl->guest_primary.data = qemu_get_ram_ptr(qxl->vga.vram_offset);
+        if (qxl->guest_primary.stride < 0) {
+            /* spice surface is upside down -> need extra buffer to flip */
+            qxl->guest_primary.stride = -qxl->guest_primary.stride;
+            qxl->guest_primary.flipped = qemu_malloc(qxl->guest_primary.surface.width *
+                                                     qxl->guest_primary.stride);
+            ptr = qxl->guest_primary.flipped;
+        } else {
+            ptr = qxl->guest_primary.data;
+        }
+        dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d, flip %s\n",
+               __FUNCTION__,
+               qxl->guest_primary.surface.width,
+               qxl->guest_primary.surface.height,
+               qxl->guest_primary.stride,
+               qxl->guest_primary.bytes_pp,
+               qxl->guest_primary.bits_pp,
+               qxl->guest_primary.flipped ? "yes" : "no");
+        vga->ds->surface =
+            qemu_create_displaysurface_from(qxl->guest_primary.surface.width,
+                                            qxl->guest_primary.surface.height,
+                                            qxl->guest_primary.bits_pp,
+                                            qxl->guest_primary.stride,
+                                            ptr);
+        dpy_resize(vga->ds);
+    }
+
+    if (!qxl->guest_primary.commands) {
+        return;
+    }
+    qxl->guest_primary.commands = 0;
+
+    update.left   = 0;
+    update.right  = qxl->guest_primary.surface.width;
+    update.top    = 0;
+    update.bottom = qxl->guest_primary.surface.height;
+
+    memset(dirty, 0, sizeof(dirty));
+    qxl->ssd.worker->update_area(qxl->ssd.worker, 0, &update,
+                                 dirty, ARRAY_SIZE(dirty), 1);
+
+    for (i = 0; i < ARRAY_SIZE(dirty); i++) {
+        if (qemu_spice_rect_is_empty(dirty+i)) {
+            break;
+        }
+        if (qxl->guest_primary.flipped) {
+            qxl_flip(qxl, dirty+i);
+        }
+        dpy_update(vga->ds,
+                   dirty[i].left, dirty[i].top,
+                   dirty[i].right - dirty[i].left,
+                   dirty[i].bottom - dirty[i].top);
+    }
+}
+
+static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
+{
+    QEMUCursor *c;
+    uint8_t *image, *mask;
+    int size;
+
+    c = cursor_alloc(cursor->header.width, cursor->header.height);
+    c->hot_x = cursor->header.hot_spot_x;
+    c->hot_y = cursor->header.hot_spot_y;
+    switch (cursor->header.type) {
+    case SPICE_CURSOR_TYPE_ALPHA:
+        size = cursor->header.width * cursor->header.height * sizeof(uint32_t);
+        memcpy(c->data, cursor->chunk.data, size);
+        if (qxl->debug > 2) {
+            cursor_print_ascii_art(c, "qxl/alpha");
+        }
+        break;
+    case SPICE_CURSOR_TYPE_MONO:
+        mask  = cursor->chunk.data;
+        image = mask + cursor_get_mono_bpl(c) * c->width;
+        cursor_set_mono(c, 0xffffff, 0x000000, image, 1, mask);
+        if (qxl->debug > 2) {
+            cursor_print_ascii_art(c, "qxl/mono");
+        }
+        break;
+    default:
+        fprintf(stderr, "%s: not implemented: type %d\n",
+                __FUNCTION__, cursor->header.type);
+        goto fail;
+    }
+    return c;
+
+fail:
+    cursor_put(c);
+    return NULL;
+}
+
+
+/* called from spice server thread context only */
+void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
+{
+    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+    QXLCursor *cursor;
+    QEMUCursor *c;
+    int x = -1, y = -1;
+
+    if (!qxl->ssd.ds->mouse_set || !qxl->ssd.ds->cursor_define) {
+        return;
+    }
+
+    if (qxl->debug > 1 && cmd->type != QXL_CURSOR_MOVE) {
+        fprintf(stderr, "%s", __FUNCTION__);
+        qxl_log_cmd_cursor(qxl, cmd, ext->group_id);
+        fprintf(stderr, "\n");
+    }
+    switch (cmd->type) {
+    case QXL_CURSOR_SET:
+        x = cmd->u.set.position.x;
+        y = cmd->u.set.position.y;
+        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
+        if (cursor->chunk.data_size != cursor->data_size) {
+            fprintf(stderr, "%s: multiple chunks\n", __FUNCTION__);
+            return;
+        }
+        c = qxl_cursor(qxl, cursor);
+        if (c == NULL) {
+            c = cursor_builtin_left_ptr();
+        }
+        qemu_mutex_lock_iothread();
+        qxl->ssd.ds->cursor_define(c);
+        qxl->ssd.ds->mouse_set(x, y, 1);
+        qemu_mutex_unlock_iothread();
+        cursor_put(c);
+        break;
+    case QXL_CURSOR_MOVE:
+        x = cmd->u.position.x;
+        y = cmd->u.position.y;
+        qemu_mutex_lock_iothread();
+        qxl->ssd.ds->mouse_set(x, y, 1);
+        qemu_mutex_unlock_iothread();
+        break;
+    }
+}
diff --git a/hw/qxl.c b/hw/qxl.c
new file mode 100644
index 0000000..e9bf804
--- /dev/null
+++ b/hw/qxl.c
@@ -0,0 +1,1552 @@ 
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+#include <pthread.h>
+
+#include "qemu-common.h"
+#include "qemu-timer.h"
+#include "qemu-queue.h"
+#include "monitor.h"
+#include "sysemu.h"
+
+#include "qxl.h"
+
+#undef SPICE_RING_PROD_ITEM
+#define SPICE_RING_PROD_ITEM(r, ret) {                                  \
+        typeof(r) start = r;                                            \
+        typeof(r) end = r + 1;                                          \
+        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
+        typeof(&(r)->items[prod]) m_item = &(r)->items[prod];           \
+        if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
+            abort();                                                    \
+        }                                                               \
+        ret = &m_item->el;                                              \
+    }
+
+#undef SPICE_RING_CONS_ITEM
+#define SPICE_RING_CONS_ITEM(r, ret) {                                  \
+        typeof(r) start = r;                                            \
+        typeof(r) end = r + 1;                                          \
+        uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
+        typeof(&(r)->items[cons]) m_item = &(r)->items[cons];           \
+        if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
+            abort();                                                    \
+        }                                                               \
+        ret = &m_item->el;                                              \
+    }
+
+#undef ALIGN
+#define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
+
+#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9" 
+
+#define QXL_MODE(_x, _y, _b, _o)                  \
+    {   .x_res = _x,                              \
+        .y_res = _y,                              \
+        .bits  = _b,                              \
+        .stride = (_x) * (_b) / 8,                \
+        .x_mili = PIXEL_SIZE * (_x),              \
+        .y_mili = PIXEL_SIZE * (_y),              \
+        .orientation = _o,                        \
+    }
+
+#define QXL_MODE_16_32(x_res, y_res, orientation) \
+    QXL_MODE(x_res, y_res, 16, orientation),      \
+    QXL_MODE(x_res, y_res, 32, orientation)
+
+#define QXL_MODE_EX(x_res, y_res)                 \
+    QXL_MODE_16_32(x_res, y_res, 0),              \
+    QXL_MODE_16_32(y_res, x_res, 1),              \
+    QXL_MODE_16_32(x_res, y_res, 2),              \
+    QXL_MODE_16_32(y_res, x_res, 3)
+
+static QXLMode qxl_modes[] = {
+    QXL_MODE_EX(640, 480),
+    QXL_MODE_EX(800, 480),
+    QXL_MODE_EX(800, 600),
+    QXL_MODE_EX(832, 624),
+    QXL_MODE_EX(960, 640),
+    QXL_MODE_EX(1024, 600),
+    QXL_MODE_EX(1024, 768),
+    QXL_MODE_EX(1152, 864),
+    QXL_MODE_EX(1152, 870),
+    QXL_MODE_EX(1280, 720),
+    QXL_MODE_EX(1280, 760),
+    QXL_MODE_EX(1280, 768),
+    QXL_MODE_EX(1280, 800),
+    QXL_MODE_EX(1280, 960),
+    QXL_MODE_EX(1280, 1024),
+    QXL_MODE_EX(1360, 768),
+    QXL_MODE_EX(1366, 768),
+    QXL_MODE_EX(1400, 1050),
+    QXL_MODE_EX(1440, 900),
+    QXL_MODE_EX(1600, 900),
+    QXL_MODE_EX(1600, 1200),
+    QXL_MODE_EX(1680, 1050),
+    QXL_MODE_EX(1920, 1080),
+#if VGA_RAM_SIZE >= (16 * 1024 * 1024)
+    /* these modes need more than 8 MB video memory */
+    QXL_MODE_EX(1920, 1200),
+    QXL_MODE_EX(1920, 1440),
+    QXL_MODE_EX(2048, 1536),
+    QXL_MODE_EX(2560, 1440),
+    QXL_MODE_EX(2560, 1600),
+#endif
+#if VGA_RAM_SIZE >= (32 * 1024 * 1024)
+    /* these modes need more than 16 MB video memory */
+    QXL_MODE_EX(2560, 2048),
+    QXL_MODE_EX(2800, 2100),
+    QXL_MODE_EX(3200, 2400),
+#endif
+};
+
+static int device_id = 0;
+static PCIQXLDevice *qxl0;
+
+static void qxl_send_events(PCIQXLDevice *d, uint32_t events);
+static void qxl_destroy_primary(PCIQXLDevice *d);
+static void qxl_reset_memslots(PCIQXLDevice *d);
+static void qxl_reset_surfaces(PCIQXLDevice *d);
+static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
+
+static inline uint32_t msb_mask(uint32_t val)
+{
+    uint32_t mask;
+
+    do {
+        mask = ~(val - 1) & val;
+        val &= ~mask;
+    } while (mask < val);
+
+    return mask;
+}
+
+static ram_addr_t qxl_rom_size(void)
+{
+    uint32_t rom_size = sizeof(QXLRom) + sizeof(QXLModes) + sizeof(qxl_modes);
+    rom_size = MAX(rom_size, TARGET_PAGE_SIZE);
+    rom_size = msb_mask(rom_size * 2 - 1);
+    return rom_size;
+}
+
+static void init_qxl_rom(PCIQXLDevice *d)
+{
+    QXLRom *rom = qemu_get_ram_ptr(d->rom_offset);
+    QXLModes *modes = (QXLModes *)(rom + 1);
+    uint32_t ram_header_size;
+    uint32_t surface0_area_size;
+    uint32_t num_pages;
+    uint32_t fb, maxfb = 0;
+    int i;
+
+    memset(rom, 0, d->rom_size);
+
+    rom->magic         = cpu_to_le32(QXL_ROM_MAGIC);
+    rom->id            = cpu_to_le32(d->id);
+    rom->log_level     = cpu_to_le32(d->guestdebug);
+    rom->modes_offset  = cpu_to_le32(sizeof(QXLRom));
+
+    rom->slot_gen_bits = MEMSLOT_GENERATION_BITS;
+    rom->slot_id_bits  = MEMSLOT_SLOT_BITS;
+    rom->slots_start   = 1;
+    rom->slots_end     = NUM_MEMSLOTS - 1;
+    rom->n_surfaces    = cpu_to_le32(NUM_SURFACES);
+
+    modes->n_modes     = cpu_to_le32(ARRAY_SIZE(qxl_modes));
+    for (i = 0; i < modes->n_modes; i++) {
+        fb = qxl_modes[i].y_res * qxl_modes[i].stride;
+        if (maxfb < fb) {
+            maxfb = fb;
+        }
+        modes->modes[i].id          = cpu_to_le32(i);
+        modes->modes[i].x_res       = cpu_to_le32(qxl_modes[i].x_res);
+        modes->modes[i].y_res       = cpu_to_le32(qxl_modes[i].y_res);
+        modes->modes[i].bits        = cpu_to_le32(qxl_modes[i].bits);
+        modes->modes[i].stride      = cpu_to_le32(qxl_modes[i].stride);
+        modes->modes[i].x_mili      = cpu_to_le32(qxl_modes[i].x_mili);
+        modes->modes[i].y_mili      = cpu_to_le32(qxl_modes[i].y_mili);
+        modes->modes[i].orientation = cpu_to_le32(qxl_modes[i].orientation);
+    }
+    if (maxfb < VGA_RAM_SIZE && d->id == 0)
+        maxfb = VGA_RAM_SIZE;
+
+    ram_header_size    = ALIGN(sizeof(QXLRam), 4096);
+    surface0_area_size = ALIGN(maxfb, 4096);
+    num_pages          = d->vga.vram_size;
+    num_pages         -= ram_header_size;
+    num_pages         -= surface0_area_size;
+    num_pages          = num_pages / TARGET_PAGE_SIZE;
+
+    rom->draw_area_offset   = cpu_to_le32(0);
+    rom->surface0_area_size = cpu_to_le32(surface0_area_size);
+    rom->pages_offset       = cpu_to_le32(surface0_area_size);
+    rom->num_pages          = cpu_to_le32(num_pages);
+    rom->ram_header_offset  = cpu_to_le32(d->vga.vram_size - ram_header_size);
+
+    d->shadow_rom = *rom;
+    d->rom        = rom;
+    d->modes      = modes;
+}
+
+static void init_qxl_ram(PCIQXLDevice *d)
+{
+    uint8_t *buf;
+    uint64_t *item;
+
+    buf = d->vga.vram_ptr;
+    d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
+    d->ram->magic       = cpu_to_le32(QXL_RAM_MAGIC);
+    d->ram->int_pending = cpu_to_le32(0);
+    d->ram->int_mask    = cpu_to_le32(0);
+    SPICE_RING_INIT(&d->ram->cmd_ring);
+    SPICE_RING_INIT(&d->ram->cursor_ring);
+    SPICE_RING_INIT(&d->ram->release_ring);
+    SPICE_RING_PROD_ITEM(&d->ram->release_ring, item);
+    *item = 0;
+    qxl_ring_set_dirty(d);
+}
+
+/* can be called from spice server thread context */
+static void qxl_set_dirty(ram_addr_t addr, ram_addr_t end)
+{
+    while (addr < end) {
+        cpu_physical_memory_set_dirty(addr);
+        addr += TARGET_PAGE_SIZE;
+    }
+}
+
+static void qxl_rom_set_dirty(PCIQXLDevice *qxl)
+{
+    ram_addr_t addr = qxl->rom_offset;
+    qxl_set_dirty(addr, addr + qxl->rom_size);
+}
+
+/* called from spice server thread context only */
+static void qxl_ram_set_dirty(PCIQXLDevice *qxl, void *ptr)
+{
+    ram_addr_t addr = qxl->vga.vram_offset;
+    void *base = qxl->vga.vram_ptr;
+    intptr_t offset;
+
+    offset = ptr - base;
+    offset &= ~(TARGET_PAGE_SIZE-1);
+    assert(offset < qxl->vga.vram_size);
+    qxl_set_dirty(addr + offset, addr + offset + TARGET_PAGE_SIZE);
+}
+
+/* can be called from spice server thread context */
+static void qxl_ring_set_dirty(PCIQXLDevice *qxl)
+{
+    ram_addr_t addr = qxl->vga.vram_offset + qxl->shadow_rom.ram_header_offset;
+    ram_addr_t end  = qxl->vga.vram_offset + qxl->vga.vram_size;
+    qxl_set_dirty(addr, end);
+}
+
+/*
+ * keep track of some command state, for savevm/loadvm.
+ * called from spice server thread context only
+ */
+static void qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
+{
+    switch (le32_to_cpu(ext->cmd.type)) {
+    case QXL_CMD_SURFACE:
+    {
+        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+        uint32_t id = le32_to_cpu(cmd->surface_id);
+        PANIC_ON(id >= NUM_SURFACES);
+        if (cmd->type == QXL_SURFACE_CMD_CREATE) {
+            qxl->guest_surfaces.cmds[id] = ext->cmd.data;
+            qxl->guest_surfaces.count++;
+            if (qxl->guest_surfaces.max < qxl->guest_surfaces.count)
+                qxl->guest_surfaces.max = qxl->guest_surfaces.count;
+        }
+        if (cmd->type == QXL_SURFACE_CMD_DESTROY) {
+            qxl->guest_surfaces.cmds[id] = 0;
+            qxl->guest_surfaces.count--;
+        }
+        break;
+    }
+    case QXL_CMD_CURSOR:
+    {
+        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+        if (cmd->type == QXL_CURSOR_SET) {
+            qxl->guest_cursor = ext->cmd.data;
+        }
+        break;
+    }
+    }
+}
+
+/* spice display interface callbacks */
+
+static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    dprint(qxl, 1, "%s:\n", __FUNCTION__);
+    qxl->ssd.worker = qxl_worker;
+}
+
+static void interface_set_compression_level(QXLInstance *sin, int level)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    dprint(qxl, 1, "%s: %d\n", __FUNCTION__, level);
+    qxl->shadow_rom.compression_level = cpu_to_le32(level);
+    qxl->rom->compression_level = cpu_to_le32(level);
+    qxl_rom_set_dirty(qxl);
+}
+
+static void interface_set_mm_time(QXLInstance *sin, uint32_t mm_time)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    qxl->shadow_rom.mm_clock = cpu_to_le32(mm_time);
+    qxl->rom->mm_clock = cpu_to_le32(mm_time);
+    qxl_rom_set_dirty(qxl);
+}
+
+static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    dprint(qxl, 1, "%s:\n", __FUNCTION__);
+    info->memslot_gen_bits = MEMSLOT_GENERATION_BITS;
+    info->memslot_id_bits = MEMSLOT_SLOT_BITS;
+    info->num_memslots = NUM_MEMSLOTS;
+    info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
+    info->internal_groupslot_id = 0;
+    info->qxl_ram_size = le32_to_cpu(qxl->shadow_rom.num_pages) << TARGET_PAGE_BITS;
+    info->n_surfaces = NUM_SURFACES;
+}
+
+/* called from spice server thread context only */
+static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    SimpleSpiceUpdate *update;
+    QXLCommandRing *ring;
+    QXLCommand *cmd;
+    int notify;
+
+    switch (qxl->mode) {
+    case QXL_MODE_VGA:
+        dprint(qxl, 2, "%s: vga\n", __FUNCTION__);
+        update = qemu_spice_create_update(&qxl->ssd);
+        if (update == NULL) {
+            return false;
+        }
+        *ext = update->ext;
+        qxl_log_command(qxl, "vga", ext);
+        return true;
+    case QXL_MODE_COMPAT:
+    case QXL_MODE_NATIVE:
+    case QXL_MODE_UNDEFINED:
+        dprint(qxl, 2, "%s: %s\n", __FUNCTION__,
+               qxl->cmdflags ? "compat" : "native");
+        ring = &qxl->ram->cmd_ring;
+        if (SPICE_RING_IS_EMPTY(ring)) {
+            return false;
+        }
+        SPICE_RING_CONS_ITEM(ring, cmd);
+        ext->cmd      = *cmd;
+        ext->group_id = MEMSLOT_GROUP_GUEST;
+        ext->flags    = qxl->cmdflags;
+        SPICE_RING_POP(ring, notify);
+        qxl_ring_set_dirty(qxl);
+        if (notify) {
+            qxl_send_events(qxl, QXL_INTERRUPT_DISPLAY);
+        }
+        qxl->guest_primary.commands++;
+        qxl_track_command(qxl, ext);
+        qxl_log_command(qxl, "cmd", ext);
+        return true;
+    default:
+        return false;
+    }
+}
+
+/* called from spice server thread context only */
+static int interface_req_cmd_notification(QXLInstance *sin)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    int wait = 1;
+
+    switch (qxl->mode) {
+    case QXL_MODE_COMPAT:
+    case QXL_MODE_NATIVE:
+    case QXL_MODE_UNDEFINED:
+        SPICE_RING_CONS_WAIT(&qxl->ram->cmd_ring, wait);
+        qxl_ring_set_dirty(qxl);
+        break;
+    default:
+        /* nothing */
+        break;
+    }
+    return wait;
+}
+
+/* called from spice server thread context only */
+static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
+{
+    QXLReleaseRing *ring = &d->ram->release_ring;
+    uint64_t *item;
+    int notify;
+
+#define QXL_FREE_BUNCH_SIZE 32
+
+    if (ring->prod - ring->cons + 1 == ring->num_items) {
+        /* ring full -- can't push */
+        return;
+    }
+    if (!flush && d->oom_running) {
+        /* collect everything from oom handler before pushing */
+        return;
+    }
+    if (!flush && d->num_free_res < QXL_FREE_BUNCH_SIZE) {
+        /* collect a bit more before pushing */
+        return;
+    }
+
+    SPICE_RING_PUSH(ring, notify);
+    dprint(d, 2, "free: push %d items, notify %s, ring %d/%d [%d,%d]\n",
+           d->num_free_res, notify ? "yes" : "no",
+           ring->prod - ring->cons, ring->num_items,
+           ring->prod, ring->cons);
+    if (notify) {
+        qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
+    }
+    SPICE_RING_PROD_ITEM(ring, item);
+    *item = 0;
+    d->num_free_res = 0;
+    d->last_release = NULL;
+    qxl_ring_set_dirty(d);
+}
+
+/* called from spice server thread context only */
+static void interface_release_resource(QXLInstance *sin,
+                                       struct QXLReleaseInfoExt ext)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    QXLReleaseRing *ring;
+    uint64_t *item, id;
+
+    if (ext.group_id == MEMSLOT_GROUP_HOST) {
+        /* host group -> vga mode update request */
+        qemu_spice_destroy_update(&qxl->ssd, (void*)ext.info->id);
+        return;
+    }
+
+    /*
+     * ext->info points into guest-visible memory
+     * pci bar 0, $command.release_info
+     */
+    ring = &qxl->ram->release_ring;
+    SPICE_RING_PROD_ITEM(ring, item);
+    if (*item == 0) {
+        /* stick head into the ring */
+        id = ext.info->id;
+        ext.info->next = 0;
+        qxl_ram_set_dirty(qxl, &ext.info->next);
+        *item = id;
+        qxl_ring_set_dirty(qxl);
+    } else {
+        /* append item to the list */
+        qxl->last_release->next = ext.info->id;
+        qxl_ram_set_dirty(qxl, &qxl->last_release->next);
+        ext.info->next = 0;
+        qxl_ram_set_dirty(qxl, &ext.info->next);
+    }
+    qxl->last_release = ext.info;
+    qxl->num_free_res++;
+    dprint(qxl, 3, "%4d\r", qxl->num_free_res);
+    qxl_push_free_res(qxl, 0);
+}
+
+/* called from spice server thread context only */
+static int interface_get_cursor_command(QXLInstance *sin, struct QXLCommandExt *ext)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    QXLCursorRing *ring;
+    QXLCommand *cmd;
+    int notify;
+
+    switch (qxl->mode) {
+    case QXL_MODE_COMPAT:
+    case QXL_MODE_NATIVE:
+    case QXL_MODE_UNDEFINED:
+        ring = &qxl->ram->cursor_ring;
+        if (SPICE_RING_IS_EMPTY(ring)) {
+            return false;
+        }
+        SPICE_RING_CONS_ITEM(ring, cmd);
+        ext->cmd      = *cmd;
+        ext->group_id = MEMSLOT_GROUP_GUEST;
+        ext->flags    = qxl->cmdflags;
+        SPICE_RING_POP(ring, notify);
+        qxl_ring_set_dirty(qxl);
+        if (notify) {
+            qxl_send_events(qxl, QXL_INTERRUPT_CURSOR);
+        }
+        qxl->guest_primary.commands++;
+        qxl_track_command(qxl, ext);
+        qxl_log_command(qxl, "csr", ext);
+        if (qxl->id == 0) {
+            qxl_render_cursor(qxl, ext);
+        }
+        return true;
+    default:
+        return false;
+    }
+}
+
+/* called from spice server thread context only */
+static int interface_req_cursor_notification(QXLInstance *sin)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    int wait = 1;
+
+    switch (qxl->mode) {
+    case QXL_MODE_COMPAT:
+    case QXL_MODE_NATIVE:
+    case QXL_MODE_UNDEFINED:
+        SPICE_RING_CONS_WAIT(&qxl->ram->cursor_ring, wait);
+        qxl_ring_set_dirty(qxl);
+        break;
+    default:
+        /* nothing */
+        break;
+    }
+    return wait;
+}
+
+/* called from spice server thread context */
+static void interface_notify_update(QXLInstance *sin, uint32_t update_id)
+{
+    fprintf(stderr, "%s: abort()\n", __FUNCTION__);
+    abort();
+}
+
+/* called from spice server thread context only */
+static int interface_flush_resources(QXLInstance *sin)
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+    int ret;
+
+    dprint(qxl, 1, "free: guest flush (have %d)\n", qxl->num_free_res);
+    ret = qxl->num_free_res;
+    if (ret) {
+        qxl_push_free_res(qxl, 1);
+    }
+    return ret;
+}
+
+static const QXLInterface qxl_interface = {
+    .base.type               = SPICE_INTERFACE_QXL,
+    .base.description        = "qxl gpu",
+    .base.major_version      = SPICE_INTERFACE_QXL_MAJOR,
+    .base.minor_version      = SPICE_INTERFACE_QXL_MINOR,
+
+    .attache_worker          = interface_attach_worker,
+    .set_compression_level   = interface_set_compression_level,
+    .set_mm_time             = interface_set_mm_time,
+    .get_init_info           = interface_get_init_info,
+
+    /* the callbacks below are called from spice server thread context */
+    .get_command             = interface_get_command,
+    .req_cmd_notification    = interface_req_cmd_notification,
+    .release_resource        = interface_release_resource,
+    .get_cursor_command      = interface_get_cursor_command,
+    .req_cursor_notification = interface_req_cursor_notification,
+    .notify_update           = interface_notify_update,
+    .flush_resources         = interface_flush_resources,
+};
+
+static void qxl_enter_vga_mode(PCIQXLDevice *d)
+{
+    if (d->mode == QXL_MODE_VGA) {
+        return;
+    }
+    dprint(d, 1, "%s\n", __FUNCTION__);
+    qemu_spice_create_host_primary(&d->ssd);
+    d->mode = QXL_MODE_VGA;
+    memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
+}
+
+static void qxl_exit_vga_mode(PCIQXLDevice *d)
+{
+    if (d->mode != QXL_MODE_VGA) {
+        return;
+    }
+    dprint(d, 1, "%s\n", __FUNCTION__);
+    qxl_destroy_primary(d);
+}
+
+static void qxl_set_irq(PCIQXLDevice *d)
+{
+    uint32_t pending = le32_to_cpu(d->ram->int_pending);
+    uint32_t mask    = le32_to_cpu(d->ram->int_mask);
+    int level = !!(pending & mask);
+    qemu_set_irq(d->pci.irq[0], level);
+    qxl_ring_set_dirty(d);
+}
+
+static void qxl_write_config(PCIDevice *d, uint32_t address,
+                             uint32_t val, int len)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, d);
+    VGACommonState *vga = &qxl->vga;
+
+    if (qxl->id == 0) {
+        vga_dirty_log_stop(vga);
+    }
+    pci_default_write_config(d, address, val, len);
+    if (qxl->id == 0) {
+        if (vga->map_addr && qxl->pci.io_regions[0].addr == -1)
+            vga->map_addr = 0;
+        vga_dirty_log_start(vga);
+    }
+}
+
+static void qxl_check_state(PCIQXLDevice *d)
+{
+    QXLRam *ram = d->ram;
+
+    assert(SPICE_RING_IS_EMPTY(&ram->cmd_ring));
+    assert(SPICE_RING_IS_EMPTY(&ram->cursor_ring));
+}
+
+static void qxl_reset_state(PCIQXLDevice *d)
+{
+    QXLRam *ram = d->ram;
+    QXLRom *rom = d->rom;
+
+    assert(SPICE_RING_IS_EMPTY(&ram->cmd_ring));
+    assert(SPICE_RING_IS_EMPTY(&ram->cursor_ring));
+    d->shadow_rom.update_id = cpu_to_le32(0);
+    *rom = d->shadow_rom;
+    qxl_rom_set_dirty(d);
+    init_qxl_ram(d);
+    d->num_free_res = 0;
+    d->last_release = NULL;
+    memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
+}
+
+static void qxl_soft_reset(PCIQXLDevice *d)
+{
+    dprint(d, 1, "%s:\n", __FUNCTION__);
+    qxl_check_state(d);
+
+    if (d->id == 0) {
+        qxl_enter_vga_mode(d);
+    } else {
+        d->mode = QXL_MODE_UNDEFINED;
+    }
+}
+
+static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
+{
+    dprint(d, 1, "%s: start%s\n", __FUNCTION__,
+           loadvm ? " (loadvm)" : "");
+
+    qemu_mutex_unlock_iothread();
+    d->ssd.worker->reset_cursor(d->ssd.worker);
+    d->ssd.worker->reset_image_cache(d->ssd.worker);
+    qemu_mutex_lock_iothread();
+    qxl_reset_surfaces(d);
+    qxl_reset_memslots(d);
+
+    /* pre loadvm reset must not touch QXLRam.  This lives in
+     * device memory, is migrated together with RAM and thus
+     * already loaded at this point */
+    if (!loadvm) {
+        qxl_reset_state(d);
+    }
+    qemu_spice_create_host_memslot(&d->ssd);
+    qxl_soft_reset(d);
+
+    dprint(d, 1, "%s: done\n", __FUNCTION__);
+}
+
+static void qxl_reset_handler(DeviceState *dev)
+{
+    PCIQXLDevice *d = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+    qxl_hard_reset(d, 0);
+}
+
+static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    VGACommonState *vga = opaque;
+    PCIQXLDevice *qxl = container_of(vga, PCIQXLDevice, vga);
+
+    if (qxl->mode != QXL_MODE_VGA) {
+        dprint(qxl, 1, "%s\n", __FUNCTION__);
+        qxl_destroy_primary(qxl);
+        qxl_soft_reset(qxl);
+    }
+    vga_ioport_write(opaque, addr, val);
+}
+
+static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta)
+{
+    static const int regions[] = {
+        QXL_RAM_RANGE_INDEX,
+        QXL_VRAM_RANGE_INDEX,
+    };
+    uint64_t guest_start;
+    uint64_t guest_end;
+    int pci_region;
+    pcibus_t pci_start;
+    pcibus_t pci_end;
+    intptr_t virt_start;
+    QXLDevMemSlot memslot;
+    int i;
+
+    guest_start = le64_to_cpu(d->guest_slots[slot_id].slot.mem_start);
+    guest_end   = le64_to_cpu(d->guest_slots[slot_id].slot.mem_end);
+
+    dprint(d, 1, "%s: slot %d: guest phys 0x%" PRIx64 " - 0x%" PRIx64 "\n",
+           __FUNCTION__, slot_id,
+           guest_start, guest_end);
+
+    PANIC_ON(slot_id >= NUM_MEMSLOTS);
+    PANIC_ON(guest_start > guest_end);
+
+    for (i = 0; i < ARRAY_SIZE(regions); i++) {
+        pci_region = regions[i];
+        pci_start = d->pci.io_regions[pci_region].addr;
+        pci_end = pci_start + d->pci.io_regions[pci_region].size;
+        /* mapped? */
+        if (pci_start == -1) {
+            continue;
+        }
+        /* start address in range ? */
+        if (guest_start < pci_start || guest_start > pci_end) {
+            continue;
+        }
+        /* end address in range ? */
+        if (guest_end > pci_end) {
+            continue;
+        }
+        /* passed */
+        break;
+    }
+    PANIC_ON(i == ARRAY_SIZE(regions)); /* finished loop without match */
+
+    switch (pci_region) {
+    case QXL_RAM_RANGE_INDEX:
+        virt_start = (intptr_t)qemu_get_ram_ptr(d->vga.vram_offset);
+        break;
+    case QXL_VRAM_RANGE_INDEX:
+        virt_start = (intptr_t)qemu_get_ram_ptr(d->vram_offset);
+        break;
+    default:
+        /* should not happen */
+        abort();
+    }
+
+    memslot.slot_id = slot_id;
+    memslot.slot_group_id = MEMSLOT_GROUP_GUEST; /* guest group */
+    memslot.virt_start = virt_start + (guest_start - pci_start);
+    memslot.virt_end   = virt_start + (guest_end   - pci_start);
+    memslot.addr_delta = memslot.virt_start - delta;
+    memslot.generation = d->rom->slot_generation = 0;
+    qxl_rom_set_dirty(d);
+
+    dprint(d, 1, "%s: slot %d: host virt 0x%" PRIx64 " - 0x%" PRIx64 "\n",
+           __FUNCTION__, memslot.slot_id,
+           memslot.virt_start, memslot.virt_end);
+
+    d->ssd.worker->add_memslot(d->ssd.worker, &memslot);
+    d->guest_slots[slot_id].ptr = (void*)memslot.virt_start;
+    d->guest_slots[slot_id].size = memslot.virt_end - memslot.virt_start;
+    d->guest_slots[slot_id].delta = delta;
+    d->guest_slots[slot_id].active = 1;
+}
+
+static void qxl_del_memslot(PCIQXLDevice *d, uint32_t slot_id)
+{
+    dprint(d, 1, "%s: slot %d\n", __FUNCTION__, slot_id);
+    d->ssd.worker->del_memslot(d->ssd.worker, MEMSLOT_GROUP_HOST, slot_id);
+    d->guest_slots[slot_id].active = 0;
+}
+
+static void qxl_reset_memslots(PCIQXLDevice *d)
+{
+    dprint(d, 1, "%s:\n", __FUNCTION__);
+    d->ssd.worker->reset_memslots(d->ssd.worker);
+    memset(&d->guest_slots, 0, sizeof(d->guest_slots));
+}
+
+static void qxl_reset_surfaces(PCIQXLDevice *d)
+{
+    dprint(d, 1, "%s:\n", __FUNCTION__);
+    d->mode = QXL_MODE_UNDEFINED;
+    qemu_mutex_unlock_iothread();
+    d->ssd.worker->destroy_surfaces(d->ssd.worker);
+    qemu_mutex_lock_iothread();
+    memset(&d->guest_surfaces.cmds, 0, sizeof(d->guest_surfaces.cmds));
+}
+
+/* called from spice server thread context only */
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
+{
+    uint64_t phys   = le64_to_cpu(pqxl);
+    uint32_t slot   = (phys >> (64 -  8)) & 0xff;
+    uint64_t offset = phys & 0xffffffffffff;
+
+    switch (group_id) {
+    case MEMSLOT_GROUP_HOST:
+        return (void*)offset;
+    case MEMSLOT_GROUP_GUEST:
+        PANIC_ON(slot > NUM_MEMSLOTS);
+        PANIC_ON(!qxl->guest_slots[slot].active);
+        PANIC_ON(offset < qxl->guest_slots[slot].delta);
+        offset -= qxl->guest_slots[slot].delta;
+        PANIC_ON(offset > qxl->guest_slots[slot].size)
+        return qxl->guest_slots[slot].ptr + offset;
+    default:
+        PANIC_ON(1);
+    }
+}
+
+static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm)
+{
+    QXLDevSurfaceCreate surface;
+    QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
+
+    assert(qxl->mode != QXL_MODE_NATIVE);
+    qxl_exit_vga_mode(qxl);
+
+    dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__,
+           le32_to_cpu(sc->width), le32_to_cpu(sc->height));
+
+    surface.format     = le32_to_cpu(sc->format);
+    surface.height     = le32_to_cpu(sc->height);
+    surface.mem        = le64_to_cpu(sc->mem);
+    surface.position   = le32_to_cpu(sc->position);
+    surface.stride     = le32_to_cpu(sc->stride);
+    surface.width      = le32_to_cpu(sc->width);
+    surface.type       = le32_to_cpu(sc->type);
+    surface.flags      = le32_to_cpu(sc->flags);
+
+    surface.mouse_mode = true;
+    surface.group_id   = MEMSLOT_GROUP_GUEST;
+    if (loadvm) {
+        surface.flags |= QXL_SURF_FLAG_KEEP_DATA;
+    }
+
+    qxl->mode = QXL_MODE_NATIVE;
+    qxl->cmdflags = 0;
+    qxl->ssd.worker->create_primary_surface(qxl->ssd.worker, 0, &surface);
+
+    /* for local rendering */
+    qxl_render_resize(qxl);
+}
+
+static void qxl_destroy_primary(PCIQXLDevice *d)
+{
+    if (d->mode == QXL_MODE_UNDEFINED) {
+        return;
+    }
+
+    dprint(d, 1, "%s\n", __FUNCTION__);
+
+    d->mode = QXL_MODE_UNDEFINED;
+    d->ssd.worker->destroy_primary_surface(d->ssd.worker, 0);
+}
+
+static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
+{
+    pcibus_t start = d->pci.io_regions[QXL_RAM_RANGE_INDEX].addr;
+    pcibus_t end   = d->pci.io_regions[QXL_RAM_RANGE_INDEX].size + start;
+    QXLMode *mode = d->modes->modes + modenr;
+    uint64_t devmem = d->pci.io_regions[QXL_RAM_RANGE_INDEX].addr;
+    QXLMemSlot slot = {
+        .mem_start = start,
+        .mem_end = end
+    };
+    QXLSurfaceCreate surface = {
+        .width      = mode->x_res,
+        .height     = mode->y_res,
+        .stride     = -mode->x_res * 4,
+        .format     = SPICE_SURFACE_FMT_32_xRGB,
+        .flags      = loadvm ? QXL_SURF_FLAG_KEEP_DATA : 0,
+        .mouse_mode = true,
+        .mem        = devmem + d->shadow_rom.draw_area_offset,
+    };
+
+    dprint(d, 1, "%s: mode %d  [ %d x %d @ %d bpp devmem 0x%lx ]\n", __FUNCTION__,
+           modenr, mode->x_res, mode->y_res, mode->bits, devmem);
+    if (!loadvm) {
+        qxl_hard_reset(d, 0);
+    }
+
+    d->guest_slots[0].slot = slot;
+    qxl_add_memslot(d, 0, devmem);
+
+    d->guest_primary.surface = surface;
+    qxl_create_guest_primary(d, 0);
+
+    d->mode = QXL_MODE_COMPAT;
+    d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
+#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
+    if (mode->bits == 16) {
+        d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
+    }
+#endif
+    d->shadow_rom.mode = cpu_to_le32(modenr);
+    d->rom->mode = cpu_to_le32(modenr);
+    qxl_rom_set_dirty(d);
+}
+
+static void ioport_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIQXLDevice *d = opaque;
+    uint32_t io_port = addr - d->io_base;
+
+    switch (io_port) {
+    case QXL_IO_RESET:
+    case QXL_IO_SET_MODE:
+    case QXL_IO_MEMSLOT_ADD:
+    case QXL_IO_MEMSLOT_DEL:
+    case QXL_IO_CREATE_PRIMARY:
+        break;
+    default:
+        if (d->mode == QXL_MODE_NATIVE || d->mode == QXL_MODE_COMPAT)
+            break;
+        dprint(d, 1, "%s: unexpected port 0x%x in vga mode\n", __FUNCTION__, io_port);
+        return;
+    }
+
+    switch (io_port) {
+    case QXL_IO_UPDATE_AREA:
+    {
+        QXLRect update = d->ram->update_area;
+        qemu_mutex_unlock_iothread();
+        d->ssd.worker->update_area(d->ssd.worker, d->ram->update_surface,
+                                   &update, NULL, 0, 0);
+        qemu_mutex_lock_iothread();
+        break;
+    }
+    case QXL_IO_NOTIFY_CMD:
+        d->ssd.worker->wakeup(d->ssd.worker);
+        break;
+    case QXL_IO_NOTIFY_CURSOR:
+        d->ssd.worker->wakeup(d->ssd.worker);
+        break;
+    case QXL_IO_UPDATE_IRQ:
+        qxl_set_irq(d);
+        break;
+    case QXL_IO_NOTIFY_OOM:
+        if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
+            break;
+        }
+        pthread_yield();
+        if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
+            break;
+        }
+        d->oom_running = 1;
+        d->ssd.worker->oom(d->ssd.worker);
+        d->oom_running = 0;
+        break;
+    case QXL_IO_SET_MODE:
+        dprint(d, 1, "QXL_SET_MODE %d\n", val);
+        qxl_set_mode(d, val, 0);
+        break;
+    case QXL_IO_LOG:
+        if (d->guestdebug) {
+            fprintf(stderr, "qxl/guest: %s", d->ram->log_buf);
+        }
+        break;
+    case QXL_IO_RESET:
+        dprint(d, 1, "QXL_IO_RESET\n");
+        qxl_hard_reset(d, 0);
+        break;
+    case QXL_IO_MEMSLOT_ADD:
+        PANIC_ON(val >= NUM_MEMSLOTS);
+        PANIC_ON(d->guest_slots[val].active);
+        d->guest_slots[val].slot = d->ram->mem_slot;
+        qxl_add_memslot(d, val, 0);
+        break;
+    case QXL_IO_MEMSLOT_DEL:
+        qxl_del_memslot(d, val);
+        break;
+    case QXL_IO_CREATE_PRIMARY:
+        PANIC_ON(val != 0);
+        dprint(d, 1, "QXL_IO_CREATE_PRIMARY\n");
+        d->guest_primary.surface = d->ram->create_surface;
+        qxl_create_guest_primary(d, 0);
+        break;
+    case QXL_IO_DESTROY_PRIMARY:
+        PANIC_ON(val != 0);
+        dprint(d, 1, "QXL_IO_DESTROY_PRIMARY\n");
+        qxl_destroy_primary(d);
+        break;
+    case QXL_IO_DESTROY_SURFACE_WAIT:
+        d->ssd.worker->destroy_surface_wait(d->ssd.worker, val);
+        break;
+    case QXL_IO_DESTROY_ALL_SURFACES:
+        d->ssd.worker->destroy_surfaces(d->ssd.worker);
+        break;
+    default:
+        fprintf(stderr, "%s: ioport=0x%x, abort()\n", __FUNCTION__, io_port);
+        abort();
+    }
+}
+
+static uint32_t ioport_read(void *opaque, uint32_t addr)
+{
+    PCIQXLDevice *d = opaque;
+
+    dprint(d, 1, "%s: unexpected\n", __FUNCTION__);
+    return 0xff;
+}
+
+static void qxl_map(PCIDevice *pci, int region_num,
+                    pcibus_t addr, pcibus_t size, int type)
+{
+    static const char *names[] = {
+        [ QXL_IO_RANGE_INDEX ]   = "ioports",
+        [ QXL_RAM_RANGE_INDEX ]  = "devram",
+        [ QXL_ROM_RANGE_INDEX ]  = "rom",
+        [ QXL_VRAM_RANGE_INDEX ] = "vram",
+    };
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, pci);
+
+    dprint(qxl, 1, "%s: bar %d [%s] addr 0x%lx size 0x%lx\n", __FUNCTION__,
+            region_num, names[region_num], addr, size);
+
+    switch (region_num) {
+    case QXL_IO_RANGE_INDEX:
+        register_ioport_write(addr, size, 1, ioport_write, pci);
+        register_ioport_read(addr, size, 1, ioport_read, pci);
+        qxl->io_base = addr;
+        break;
+    case QXL_RAM_RANGE_INDEX:
+        cpu_register_physical_memory(addr, size, qxl->vga.vram_offset | IO_MEM_RAM);
+        qxl->vga.map_addr = addr;
+        qxl->vga.map_end = addr + size;
+        if (qxl->id == 0) {
+            vga_dirty_log_start(&qxl->vga);
+        }
+        break;
+    case QXL_ROM_RANGE_INDEX:
+        cpu_register_physical_memory(addr, size, qxl->rom_offset | IO_MEM_ROM);
+        break;
+    case QXL_VRAM_RANGE_INDEX:
+        cpu_register_physical_memory(addr, size, qxl->vram_offset | IO_MEM_RAM);
+        break;
+    }
+}
+
+static void pipe_read(void *opaque)
+{
+    PCIQXLDevice *d = opaque;
+    char dummy;
+    int len;
+
+    do {
+        len = read(d->pipe[0], &dummy, sizeof(dummy));
+    } while (len == sizeof(dummy));
+    qxl_set_irq(d);
+}
+
+/* called from spice server thread context only */
+static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
+{
+    uint32_t old_pending;
+    uint32_t le_events = cpu_to_le32(events);
+
+    assert(d->ssd.running);
+    old_pending = __sync_fetch_and_or(&d->ram->int_pending, le_events);
+    if ((old_pending & le_events) == le_events) {
+        return;
+    }
+    if (pthread_self() == d->main) {
+        qxl_set_irq(d);
+    } else {
+        if (write(d->pipe[1], d, 1) != 1) {
+            dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
+        }
+    }
+}
+
+static void init_pipe_signaling(PCIQXLDevice *d)
+{
+   if (pipe(d->pipe) < 0) {
+       dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__);
+       return;
+   }
+#ifdef CONFIG_IOTHREAD
+   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
+#else
+   fcntl(d->pipe[0], F_SETFL, O_NONBLOCK /* | O_ASYNC */);
+#endif
+   fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
+   fcntl(d->pipe[0], F_SETOWN, getpid());
+
+   d->main = pthread_self();
+   qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
+}
+
+/* graphics console */
+
+static void qxl_hw_update(void *opaque)
+{
+    PCIQXLDevice *qxl = opaque;
+    VGACommonState *vga = &qxl->vga;
+
+    switch (qxl->mode) {
+    case QXL_MODE_VGA:
+        vga->update(vga);
+        break;
+    case QXL_MODE_COMPAT:
+    case QXL_MODE_NATIVE:
+        qxl_render_update(qxl);
+        break;
+    default:
+        break;
+    }
+}
+
+static void qxl_hw_invalidate(void *opaque)
+{
+    PCIQXLDevice *qxl = opaque;
+    VGACommonState *vga = &qxl->vga;
+
+    vga->invalidate(vga);
+}
+
+static void qxl_hw_screen_dump(void *opaque, const char *filename)
+{
+    PCIQXLDevice *qxl = opaque;
+    VGACommonState *vga = &qxl->vga;
+
+    switch (qxl->mode) {
+    case QXL_MODE_COMPAT:
+    case QXL_MODE_NATIVE:
+        qxl_render_update(qxl);
+        ppm_save(filename, qxl->ssd.ds->surface);
+        break;
+    case QXL_MODE_VGA:
+        vga->screen_dump(vga, filename);
+        break;
+    default:
+        break;
+    }
+}
+
+static void qxl_hw_text_update(void *opaque, console_ch_t *chardata)
+{
+    PCIQXLDevice *qxl = opaque;
+    VGACommonState *vga = &qxl->vga;
+
+    if (qxl->mode == QXL_MODE_VGA) {
+        vga->text_update(vga, chardata);
+        return;
+    }
+}
+
+static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
+{
+    PCIQXLDevice *qxl = opaque;
+    qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
+
+    if (!running && qxl->mode == QXL_MODE_NATIVE) {
+        /* dirty all vram (which holds surfaces) to make sure it is saved */
+        /* FIXME #1: should go out during "live" stage */
+        /* FIXME #2: we only need to save the areas which are actually used */
+        ram_addr_t addr = qxl->vram_offset;
+        qxl_set_dirty(addr, addr + qxl->vram_size);
+    }
+}
+
+/* display change listener */
+
+static void display_update(struct DisplayState *ds, int x, int y, int w, int h)
+{
+    if (qxl0->mode == QXL_MODE_VGA) {
+        qemu_spice_display_update(&qxl0->ssd, x, y, w, h);
+    }
+}
+
+static void display_resize(struct DisplayState *ds)
+{
+    if (qxl0->mode == QXL_MODE_VGA) {
+        qemu_spice_display_resize(&qxl0->ssd);
+    }
+}
+
+static void display_refresh(struct DisplayState *ds)
+{
+    if (qxl0->mode == QXL_MODE_VGA) {
+        qemu_spice_display_refresh(&qxl0->ssd);
+    }
+}
+
+static DisplayChangeListener display_listener = {
+    .dpy_update  = display_update,
+    .dpy_resize  = display_resize,
+    .dpy_refresh = display_refresh,
+};
+
+static int qxl_init(PCIDevice *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci, dev);
+    VGACommonState *vga = &qxl->vga;
+    uint8_t* config = qxl->pci.config;
+    ram_addr_t ram_size = msb_mask(qxl->vga.vram_size * 2 - 1);
+    uint32_t pci_device_id;
+    uint32_t pci_device_rev;
+    uint32_t io_size;
+
+    if (device_id == 0 && dev->qdev.hotplugged) {
+        device_id++;
+    }
+
+    qxl->id = device_id;
+    qxl->mode = QXL_MODE_UNDEFINED;
+    qxl->generation = 1;
+    qxl->num_memslots = NUM_MEMSLOTS;
+    qxl->num_surfaces = NUM_SURFACES;
+
+    switch (qxl->revision) {
+    case 1: /* spice 0.4 -- qxl-1 */
+        pci_device_id  = QXL_DEVICE_ID_STABLE;
+        pci_device_rev = QXL_REVISION_STABLE_V04;
+        break;
+    case 2: /* spice 0.6 -- qxl-2 */
+        pci_device_id  = QXL_DEVICE_ID_STABLE;
+        pci_device_rev = QXL_REVISION_STABLE_V06;
+        break;
+    default: /* experimental */
+        pci_device_id  = QXL_DEVICE_ID_DEVEL;
+        pci_device_rev = 1;
+        break;
+    }
+
+    if (!qxl->id) {
+        if (ram_size < 32 * 1024 * 1024)
+            ram_size = 32 * 1024 * 1024;
+        vga_common_init(vga, ram_size);
+        vga_init(vga);
+        register_ioport_write(0x3c0, 16, 1, qxl_vga_ioport_write, vga);
+        register_ioport_write(0x3b4,  2, 1, qxl_vga_ioport_write, vga);
+        register_ioport_write(0x3d4,  2, 1, qxl_vga_ioport_write, vga);
+        register_ioport_write(0x3ba,  1, 1, qxl_vga_ioport_write, vga);
+        register_ioport_write(0x3da,  1, 1, qxl_vga_ioport_write, vga);
+
+        vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
+                                       qxl_hw_screen_dump, qxl_hw_text_update, qxl);
+        qxl->ssd.ds = vga->ds;
+        qxl->ssd.bufsize = (16 * 1024 * 1024);
+        qxl->ssd.buf = qemu_malloc(qxl->ssd.bufsize);
+
+        qxl0 = qxl;
+        register_displaychangelistener(vga->ds, &display_listener);
+
+        if (qxl->pci.romfile == NULL) {
+            if (pci_device_id == 0x01ff) {
+                qxl->pci.romfile = qemu_strdup("vgabios-qxldev.bin");
+            } else {
+                qxl->pci.romfile = qemu_strdup("vgabios-qxl.bin");
+            }
+        }
+        pci_config_set_class(config, PCI_CLASS_DISPLAY_VGA);
+    } else {
+        if (ram_size < 16 * 1024 * 1024)
+            ram_size = 16 * 1024 * 1024;
+        qxl->vga.vram_size = ram_size;
+        qxl->vga.vram_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vgavram",
+                                              qxl->vga.vram_size);
+        qxl->vga.vram_ptr = qemu_get_ram_ptr(qxl->vga.vram_offset);
+
+        pci_config_set_class(config, PCI_CLASS_DISPLAY_OTHER);
+    }
+
+    pci_config_set_vendor_id(config, REDHAT_PCI_VENDOR_ID);
+    pci_config_set_device_id(config, pci_device_id);
+    pci_set_byte(&config[PCI_REVISION_ID], pci_device_rev);
+    pci_set_byte(&config[PCI_INTERRUPT_PIN], 1);
+
+    qxl->rom_size = qxl_rom_size();
+    qxl->rom_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vrom", qxl->rom_size);
+    init_qxl_rom(qxl);
+    init_qxl_ram(qxl);
+
+    if (qxl->vram_size < 16 * 1024 * 1024) {
+        qxl->vram_size = 16 * 1024 * 1024;
+    }
+    if (qxl->revision == 1) {
+        qxl->vram_size = 4096;
+    }
+    qxl->vram_size = msb_mask(qxl->vram_size * 2 - 1);
+    qxl->vram_offset = qemu_ram_alloc(&qxl->pci.qdev, "qxl.vram", qxl->vram_size);
+
+    io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+    if (qxl->revision == 1) {
+        io_size = 8;
+    }
+
+    pci_register_bar(&qxl->pci, QXL_IO_RANGE_INDEX,
+                     io_size, PCI_BASE_ADDRESS_SPACE_IO, qxl_map);
+
+    pci_register_bar(&qxl->pci, QXL_ROM_RANGE_INDEX,
+                     qxl->rom_size, PCI_BASE_ADDRESS_SPACE_MEMORY,
+                     qxl_map);
+
+    pci_register_bar(&qxl->pci, QXL_RAM_RANGE_INDEX,
+                     qxl->vga.vram_size, PCI_BASE_ADDRESS_SPACE_MEMORY,
+                     qxl_map);
+
+    pci_register_bar(&qxl->pci, QXL_VRAM_RANGE_INDEX, qxl->vram_size,
+                     PCI_BASE_ADDRESS_SPACE_MEMORY, qxl_map);
+
+    qxl->ssd.qxl.base.sif = &qxl_interface.base;
+    qxl->ssd.qxl.id = qxl->id;
+    qemu_spice_add_interface(&qxl->ssd.qxl.base);
+    qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
+
+    init_pipe_signaling(qxl);
+    qxl_reset_state(qxl);
+
+    device_id++;
+    return 0;
+}
+
+static void qxl_pre_save(void *opaque)
+{
+    PCIQXLDevice* d = opaque;
+    uint8_t *ram_start = d->vga.vram_ptr;
+
+    dprint(d, 1, "%s:\n", __FUNCTION__);
+    if (d->last_release == NULL) {
+        d->last_release_offset = 0;
+    } else {
+        d->last_release_offset = (uint8_t *)d->last_release - ram_start;
+    }
+    assert(d->last_release_offset < d->vga.vram_size);
+}
+
+static int qxl_pre_load(void *opaque)
+{
+    PCIQXLDevice* d = opaque;
+
+    dprint(d, 1, "%s: start\n", __FUNCTION__);
+    qxl_hard_reset(d, 1);
+    qxl_exit_vga_mode(d);
+    dprint(d, 1, "%s: done\n", __FUNCTION__);
+    return 0;
+}
+
+static int qxl_post_load(void *opaque, int version)
+{
+    PCIQXLDevice* d = opaque;
+    uint8_t *ram_start = d->vga.vram_ptr;
+    QXLCommandExt *cmds;
+    int in, out, i, newmode;
+
+    dprint(d, 1, "%s: start\n", __FUNCTION__);
+
+    assert(d->last_release_offset < d->vga.vram_size);
+    if (d->last_release_offset == 0) {
+        d->last_release = NULL;
+    } else {
+        d->last_release = (QXLReleaseInfo *)(ram_start + d->last_release_offset);
+    }
+
+    d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset);
+
+    dprint(d, 1, "%s: restore mode\n", __FUNCTION__);
+    newmode = d->mode;
+    d->mode = QXL_MODE_UNDEFINED;
+    switch (newmode) {
+    case QXL_MODE_UNDEFINED:
+        break;
+    case QXL_MODE_VGA:
+        qxl_enter_vga_mode(d);
+        break;
+    case QXL_MODE_NATIVE:
+        for (i = 0; i < NUM_MEMSLOTS; i++) {
+            if (!d->guest_slots[i].active) {
+                continue;
+            }
+            qxl_add_memslot(d, i, 0);
+        }
+        qxl_create_guest_primary(d, 1);
+
+        /* replay surface-create and cursor-set commands */
+        cmds = qemu_mallocz(sizeof(QXLCommandExt) * (NUM_SURFACES + 1));
+        for (in = 0, out = 0; in < NUM_SURFACES; in++) {
+            if (d->guest_surfaces.cmds[in] == 0) {
+                continue;
+            }
+            cmds[out].cmd.data = d->guest_surfaces.cmds[in];
+            cmds[out].cmd.type = QXL_CMD_SURFACE;
+            cmds[out].group_id = MEMSLOT_GROUP_GUEST;
+            out++;
+        }
+        cmds[out].cmd.data = d->guest_cursor;
+        cmds[out].cmd.type = QXL_CMD_CURSOR;
+        cmds[out].group_id = MEMSLOT_GROUP_GUEST;
+        out++;
+        d->ssd.worker->loadvm_commands(d->ssd.worker, cmds, out);
+        qemu_free(cmds);
+
+        break;
+    case QXL_MODE_COMPAT:
+        qxl_set_mode(d, d->shadow_rom.mode, 1);
+        break;
+    }
+    dprint(d, 1, "%s: done\n", __FUNCTION__);
+
+    /* spice 0.4 compatibility -- accept but ignore */
+    qemu_free(d->worker_data);
+    d->worker_data = NULL;
+    d->worker_data_size = 0;
+
+    return 0;
+}
+
+#define QXL_SAVE_VERSION 20
+
+static bool qxl_test_worker_data(void *opaque, int version_id)
+{
+    PCIQXLDevice* d = opaque;
+
+    if (d->revision != 1) {
+        return false;
+    }
+    if (!d->worker_data_size) {
+        return false;
+    }
+    if (!d->worker_data) {
+        d->worker_data = qemu_malloc(d->worker_data_size);
+    }
+    return true;
+}
+
+static bool qxl_test_spice04(void *opaque, int version_id)
+{
+    PCIQXLDevice* d = opaque;
+    return d->revision == 1;
+}
+
+static bool qxl_test_spice06(void *opaque)
+{
+    PCIQXLDevice* d = opaque;
+    return d->revision > 1;
+}
+
+static VMStateDescription qxl_memslot = {
+    .name               = "qxl-memslot",
+    .version_id         = QXL_SAVE_VERSION,
+    .minimum_version_id = QXL_SAVE_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(slot.mem_start, struct guest_slots),
+        VMSTATE_UINT64(slot.mem_end,   struct guest_slots),
+        VMSTATE_UINT32(active,         struct guest_slots),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static VMStateDescription qxl_surface = {
+    .name               = "qxl-surface",
+    .version_id         = QXL_SAVE_VERSION,
+    .minimum_version_id = QXL_SAVE_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(width,      QXLSurfaceCreate),
+        VMSTATE_UINT32(height,     QXLSurfaceCreate),
+        VMSTATE_INT32(stride,      QXLSurfaceCreate),
+        VMSTATE_UINT32(format,     QXLSurfaceCreate),
+        VMSTATE_UINT32(position,   QXLSurfaceCreate),
+        VMSTATE_UINT32(mouse_mode, QXLSurfaceCreate),
+        VMSTATE_UINT32(flags,      QXLSurfaceCreate),
+        VMSTATE_UINT32(type,       QXLSurfaceCreate),
+        VMSTATE_UINT64(mem,        QXLSurfaceCreate),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static VMStateDescription qxl_vmstate_spice06 = {
+    .name               = "qxl/spice06",
+    .version_id         = QXL_SAVE_VERSION,
+    .minimum_version_id = QXL_SAVE_VERSION,
+    .fields = (VMStateField []) {
+        VMSTATE_INT32_EQUAL(num_memslots, PCIQXLDevice),
+        VMSTATE_STRUCT_ARRAY(guest_slots, PCIQXLDevice, NUM_MEMSLOTS, 0,
+                             qxl_memslot, struct guest_slots),
+        VMSTATE_STRUCT(guest_primary.surface, PCIQXLDevice, 0,
+                       qxl_surface, QXLSurfaceCreate),
+        VMSTATE_INT32_EQUAL(num_surfaces, PCIQXLDevice),
+        VMSTATE_ARRAY(guest_surfaces.cmds, PCIQXLDevice, NUM_SURFACES, 0,
+                      vmstate_info_uint64, uint64_t),
+        VMSTATE_UINT64(guest_cursor, PCIQXLDevice),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static VMStateDescription qxl_vmstate = {
+    .name               = "qxl",
+    .version_id         = QXL_SAVE_VERSION,
+    .minimum_version_id = QXL_SAVE_VERSION,
+    .pre_save           = qxl_pre_save,
+    .pre_load           = qxl_pre_load,
+    .post_load          = qxl_post_load,
+    .fields = (VMStateField []) {
+        VMSTATE_PCI_DEVICE(pci, PCIQXLDevice),
+        VMSTATE_STRUCT(vga, PCIQXLDevice, 0, vmstate_vga_common, VGACommonState),
+        VMSTATE_UINT32(shadow_rom.mode, PCIQXLDevice),
+        VMSTATE_UINT32(num_free_res, PCIQXLDevice),
+        VMSTATE_UINT32(last_release_offset, PCIQXLDevice),
+        VMSTATE_UINT32(mode, PCIQXLDevice),
+        VMSTATE_UINT32(ssd.unique, PCIQXLDevice),
+
+        /* spice 0.4 sends/expects them */
+        VMSTATE_VBUFFER_UINT32(vga.vram_ptr, PCIQXLDevice, 0, qxl_test_spice04, 0,
+                               vga.vram_size),
+        VMSTATE_UINT32_TEST(worker_data_size, PCIQXLDevice, qxl_test_spice04),
+        VMSTATE_VBUFFER_UINT32(worker_data, PCIQXLDevice, 0, qxl_test_worker_data, 0,
+                               worker_data_size),
+
+        VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            /* additional spice 0.6 state */
+            .vmsd   = &qxl_vmstate_spice06,
+            .needed = qxl_test_spice06,
+        },{
+            /* end of list */
+        },
+    },
+};
+
+static PCIDeviceInfo qxl_info = {
+    .qdev.name    = "qxl",
+    .qdev.desc    = "Spice QXL GPU",
+    .qdev.size    = sizeof(PCIQXLDevice),
+    .qdev.reset   = qxl_reset_handler,
+    .qdev.vmsd    = &qxl_vmstate,
+    .init         = qxl_init,
+    .config_write = qxl_write_config,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT32("ram_size", PCIQXLDevice, vga.vram_size, 64 * 1024 * 1024),
+        DEFINE_PROP_UINT32("vram_size", PCIQXLDevice, vram_size, 64 * 1024 * 1024),
+        DEFINE_PROP_UINT32("revision", PCIQXLDevice, revision, 2),
+        DEFINE_PROP_UINT32("debug", PCIQXLDevice, debug, 0),
+        DEFINE_PROP_UINT32("guestdebug", PCIQXLDevice, guestdebug, 0),
+        DEFINE_PROP_UINT32("cmdlog", PCIQXLDevice, cmdlog, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void qxl_register(void)
+{
+    pci_qdev_register(&qxl_info);
+}
+
+device_init(qxl_register);
diff --git a/hw/qxl.h b/hw/qxl.h
new file mode 100644
index 0000000..6733ec2
--- /dev/null
+++ b/hw/qxl.h
@@ -0,0 +1,110 @@ 
+#include "console.h"
+#include "hw.h"
+#include "pci.h"
+#include "vga_int.h"
+
+#include "ui/qemu-spice.h"
+#include "ui/spice-display.h"
+
+enum qxl_mode {
+    QXL_MODE_UNDEFINED,
+    QXL_MODE_VGA,
+    QXL_MODE_COMPAT, /* spice 0.4.x */
+    QXL_MODE_NATIVE,
+};
+
+typedef struct PCIQXLDevice {
+    PCIDevice          pci;
+    SimpleSpiceDisplay ssd;
+    int                id;
+    uint32_t           debug;
+    uint32_t           guestdebug;
+    uint32_t           cmdlog;
+    enum qxl_mode      mode;
+    uint32_t           cmdflags;
+    int                generation;
+    uint32_t           revision;
+
+    int32_t            num_memslots;
+    int32_t            num_surfaces;
+
+    struct guest_slots {
+        QXLMemSlot     slot;
+        void           *ptr;
+        uint64_t       size;
+        uint64_t       delta;
+        uint32_t       active;
+    } guest_slots[NUM_MEMSLOTS];
+
+    struct guest_primary {
+        QXLSurfaceCreate surface;
+        uint32_t       commands;
+        uint32_t       resized;
+        int32_t        stride;
+        uint32_t       bits_pp;
+        uint32_t       bytes_pp;
+        uint8_t        *data, *flipped;
+    } guest_primary;
+
+    struct surfaces {
+        QXLPHYSICAL    cmds[NUM_SURFACES];
+        uint32_t       count;
+        uint32_t       max;
+    } guest_surfaces;
+    QXLPHYSICAL        guest_cursor;
+
+    /* thread signaling */
+    pthread_t          main;
+    int                pipe[2];
+
+    /* ram pci bar */
+    QXLRam             *ram;
+    VGACommonState     vga;
+    uint32_t           num_free_res;
+    QXLReleaseInfo     *last_release;
+    uint32_t           last_release_offset;
+    uint32_t           oom_running;
+
+    /* rom pci bar */
+    QXLRom             shadow_rom;
+    QXLRom             *rom;
+    QXLModes           *modes;
+    uint32_t           rom_size;
+    uint64_t           rom_offset;
+
+    /* vram pci bar */
+    uint32_t           vram_size;
+    uint64_t           vram_offset;
+
+    /* io bar */
+    uint32_t           io_base;
+
+    /* spice 0.4 loadvm compatibility */
+    void               *worker_data;
+    uint32_t           worker_data_size;
+} PCIQXLDevice;
+
+#define PANIC_ON(x) if ((x)) {                         \
+    printf("%s: PANIC %s failed\n", __FUNCTION__, #x); \
+    exit(-1);                                          \
+}
+
+#define dprint(_qxl, _level, _fmt, ...)                                 \
+    do {                                                                \
+        if (_qxl->debug >= _level) {                                    \
+            fprintf(stderr, "qxl-%d: ", _qxl->id);                      \
+            fprintf(stderr, _fmt, ## __VA_ARGS__);                      \
+        }                                                               \
+    } while (0)
+
+/* qxl.c */
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
+
+/* qxl-logger.c */
+void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id);
+void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext);
+
+/* qxl-render.c */
+void qxl_render_resize(PCIQXLDevice *qxl);
+void qxl_render_update(PCIQXLDevice *qxl);
+void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext);
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 6a46a43..beb5423 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -106,7 +106,7 @@  typedef void (* vga_update_retrace_info_fn)(struct VGACommonState *s);
 typedef struct VGACommonState {
     uint8_t *vram_ptr;
     ram_addr_t vram_offset;
-    unsigned int vram_size;
+    uint32_t vram_size;
     uint32_t lfb_addr;
     uint32_t lfb_end;
     uint32_t map_addr;
diff --git a/qemu-options.hx b/qemu-options.hx
index 4d99a58..fd73210 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -751,7 +751,7 @@  Rotate graphical output 90 deg left (only PXA LCD).
 ETEXI
 
 DEF("vga", HAS_ARG, QEMU_OPTION_vga,
-    "-vga [std|cirrus|vmware|xenfb|none]\n"
+    "-vga [std|cirrus|vmware|qxl|xenfb|none]\n"
     "                select video card type\n", QEMU_ARCH_ALL)
 STEXI
 @item -vga @var{type}
@@ -772,6 +772,10 @@  this option.
 VMWare SVGA-II compatible adapter. Use it if you have sufficiently
 recent XFree86/XOrg server or Windows guest with a driver for this
 card.
+@item qxl
+QXL paravirtual graphic card.  It is VGA compatible (including VESA
+2.0 VBE support).  Works best with qxl guest drivers installed though.
+Recommended choice when using the spice protocol.
 @item none
 Disable VGA card.
 @end table
diff --git a/sysemu.h b/sysemu.h
index b81a70e..d9b445b 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -102,7 +102,7 @@  extern int incoming_expected;
 extern int bios_size;
 
 typedef enum {
-    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB
+    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
 } VGAInterfaceType;
 
 extern int vga_interface_type;
@@ -110,6 +110,7 @@  extern int vga_interface_type;
 #define std_vga_enabled (vga_interface_type == VGA_STD)
 #define xenfb_enabled (vga_interface_type == VGA_XENFB)
 #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
+#define qxl_enabled (vga_interface_type == VGA_QXL)
 
 extern int graphic_width;
 extern int graphic_height;
diff --git a/ui/spice-core.c b/ui/spice-core.c
index e97a72d..82fbaec 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -369,6 +369,21 @@  void qemu_spice_init(void)
 
 int qemu_spice_add_interface(SpiceBaseInstance *sin)
 {
+    if (!spice_server) {
+        if (QTAILQ_FIRST(&qemu_spice_opts.head) != NULL) {
+            fprintf(stderr, "Oops: spice configured but not active\n");
+            exit(1);
+        }
+        /*
+         * Create a spice server instance.
+         * It does *not* listen on the network.
+         * It handles QXL local rendering only.
+         *
+         * With a command line like '-vnc :0 -vga qxl' you'll end up here.
+         */
+        spice_server = spice_server_new();
+        spice_server_init(spice_server, &core_interface);
+    }
     return spice_server_add_interface(spice_server, sin);
 }
 
diff --git a/vl.c b/vl.c
index 7038952..d7c1762 100644
--- a/vl.c
+++ b/vl.c
@@ -1410,6 +1410,8 @@  static void select_vgahw (const char *p)
         vga_interface_type = VGA_VMWARE;
     } else if (strstart(p, "xenfb", &opts)) {
         vga_interface_type = VGA_XENFB;
+    } else if (strstart(p, "qxl", &opts)) {
+        vga_interface_type = VGA_QXL;
     } else if (!strstart(p, "none", &opts)) {
     invalid_vga:
         fprintf(stderr, "Unknown vga type: %s\n", p);
@@ -2952,7 +2954,7 @@  int main(int argc, char **argv, char **envp)
         }
     }
 #ifdef CONFIG_SPICE
-    if (using_spice) {
+    if (using_spice && !qxl_enabled) {
         qemu_spice_display_init(ds);
     }
 #endif