[8/9] spice: simple display

With that patch applied you'll actually see the guests screen in the
spice client.  This does *not* bring qxl and full spice support though.
This is basically the qxl vga mode made more generic, so it plays
together with any qemu-emulated gfx card.  You can display stdvga or
cirrus via spice client.  You can have both vnc and spice enabled and
clients connected at the same time.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs   |    2 +-
 qemu-spice.h    |    1 +
 spice-display.c |  387 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 spice-display.h |   52 ++++++++
 vl.c            |    7 +-
 5 files changed, 447 insertions(+), 2 deletions(-)
 create mode 100644 spice-display.c
 create mode 100644 spice-display.h

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> With that patch applied you'll actually see the guests screen in the
> spice client.  This does *not* bring qxl and full spice support though.
> This is basically the qxl vga mode made more generic, so it plays
> together with any qemu-emulated gfx card.  You can display stdvga or
> cirrus via spice client.  You can have both vnc and spice enabled and
> clients connected at the same time.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   Makefile.objs   |    2 +-
>   qemu-spice.h    |    1 +
>   spice-display.c |  387 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   spice-display.h |   52 ++++++++
>   vl.c            |    7 +-
>   5 files changed, 447 insertions(+), 2 deletions(-)
>   create mode 100644 spice-display.c
>   create mode 100644 spice-display.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 6ddb373..2f40529 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -88,7 +88,7 @@ common-obj-y += pflib.o
>   common-obj-$(CONFIG_BRLAPI) += baum.o
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
>
> -common-obj-$(CONFIG_SPICE) += spice.o spice-input.o
> +common-obj-$(CONFIG_SPICE) += spice.o spice-input.o spice-display.o
>
>   audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
>   audio-obj-$(CONFIG_SDL) += sdlaudio.o
> diff --git a/qemu-spice.h b/qemu-spice.h
> index ceb3db2..f061004 100644
> --- a/qemu-spice.h
> +++ b/qemu-spice.h
> @@ -13,6 +13,7 @@ extern int using_spice;
>
>   void qemu_spice_init(void);
>   void qemu_spice_input_init(void);
> +void qemu_spice_display_init(DisplayState *ds);
>
>   #else  /* CONFIG_SPICE */
>
> diff --git a/spice-display.c b/spice-display.c
> new file mode 100644
> index 0000000..1e6d939
> --- /dev/null
> +++ b/spice-display.c
> @@ -0,0 +1,387 @@
> +#include<stdio.h>
> +#include<stdlib.h>
> +#include<stdbool.h>
> +#include<stdint.h>
> +#include<string.h>
>    

Copyright + unnecessary headers.

> +#include<pthread.h>
>    

Hopefully this isn't actually needed.  If it is, this needs to be a bit 
more clever.

> +
> +#include "qemu-common.h"
> +#include "qemu-spice.h"
> +#include "qemu-timer.h"
> +#include "qemu-queue.h"
> +#include "monitor.h"
> +#include "console.h"
> +#include "sysemu.h"
> +
> +#include "spice-display.h"
> +
> +static int debug = 0;
> +
> +int qemu_spice_rect_is_empty(const QXLRect* r)
> +{
> +    return r->top == r->bottom || r->left == r->right;
> +}
> +
> +void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r)
> +{
> +    if (qemu_spice_rect_is_empty(r)) {
> +        return;
> +    }
> +
> +    if (qemu_spice_rect_is_empty(dest)) {
> +        *dest = *r;
> +        return;
> +    }
> +
> +    dest->top = MIN(dest->top, r->top);
> +    dest->left = MIN(dest->left, r->left);
> +    dest->bottom = MAX(dest->bottom, r->bottom);
> +    dest->right = MAX(dest->right, r->right);
> +}
> +
> +SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
> +{
> +    SimpleSpiceUpdate *update;
> +    QXLDrawable *drawable;
> +    QXLImage *image;
> +    QXLCommand *cmd;
> +    uint8_t *src, *dst;
> +    int by, bw, bh;
> +
> +    if (qemu_spice_rect_is_empty(&ssd->dirty)) {
> +        return NULL;
> +    };
> +
> +    pthread_mutex_lock(&ssd->lock);
>    

The locking worries me here.

It only makes sense if the spice interface_* callbacks can be invoked 
from threads independently of any of the QEMU threads.

If that's the case, that means that the interface_* code is potentially 
running in parallel to another QEMU thread that's holding the qemu_mutex.

So just acquiring a private lock in the interface_* code and then 
calling into common QEMU code could result in re-entrance in interfaces 
that aren't re-entrant.

> +    if (debug>  1)
> +        fprintf(stderr, "%s: lr %d ->  %d,  tb ->  %d ->  %d\n", __FUNCTION__,
> +                ssd->dirty.left, ssd->dirty.right,
> +                ssd->dirty.top, ssd->dirty.bottom);
> +
> +    update   = qemu_mallocz(sizeof(*update));
> +    drawable =&update->drawable;
> +    image    =&update->image;
> +    cmd      =&update->ext.cmd;
> +
> +    bw       = ssd->dirty.right - ssd->dirty.left;
> +    bh       = ssd->dirty.bottom - ssd->dirty.top;
> +    update->bitmap = qemu_malloc(bw * bh * 4);
>    

While not really unsafe, the qemu_malloc functions are not guaranteed to 
be re-entrant by the interfaces today.  It's also terribly subtle to 
have to rely on implicit re-entrance safety.

> +
> +    drawable->bbox            = ssd->dirty;
> +    drawable->clip.type       = SPICE_CLIP_TYPE_NONE;
> +    drawable->effect          = QXL_EFFECT_OPAQUE;
> +    drawable->release_info.id = (intptr_t)update;
> +    drawable->type            = QXL_DRAW_COPY;
> +
> +    drawable->u.copy.rop_descriptor  = SPICE_ROPD_OP_PUT;
> +    drawable->u.copy.src_bitmap      = (intptr_t)image;
> +    drawable->u.copy.src_area.right  = bw;
> +    drawable->u.copy.src_area.bottom = bh;
> +
> +    QXL_SET_IMAGE_ID(image, QXL_IMAGE_GROUP_DEVICE, ssd->unique++);
> +    image->descriptor.type   = SPICE_IMAGE_TYPE_BITMAP;
> +    image->bitmap.flags      = QXL_BITMAP_DIRECT | QXL_BITMAP_TOP_DOWN;
> +    image->bitmap.stride     = bw * 4;
> +    image->descriptor.width  = image->bitmap.x = bw;
> +    image->descriptor.height = image->bitmap.y = bh;
> +    image->bitmap.data = (intptr_t)(update->bitmap);
> +    image->bitmap.palette = 0;
> +    image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
> +
> +    if (ssd->conv == NULL) {
> +        PixelFormat dst = qemu_default_pixelformat(32);
> +        ssd->conv = qemu_pf_conv_get(&dst,&ssd->ds->surface->pf);
> +        assert(ssd->conv);
> +    }
> +
> +    src = ds_get_data(ssd->ds) +
> +        ssd->dirty.top * ds_get_linesize(ssd->ds) +
> +        ssd->dirty.left * ds_get_bytes_per_pixel(ssd->ds);
> +    dst = update->bitmap;
> +    for (by = 0; by<  bh; by++) {
> +        qemu_pf_conv_run(ssd->conv, dst, src, bw);
> +        src += ds_get_linesize(ssd->ds);
> +        dst += image->bitmap.stride;
> +    }
> +
> +    cmd->type = QXL_CMD_DRAW;
> +    cmd->data = (intptr_t)drawable;
> +
> +    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> +    pthread_mutex_unlock(&ssd->lock);
> +    return update;
> +}
>    

As we move to better support for re-entrance, we have to be careful we 
don't create a scenario where some code relies on certain functions to 
be re-entrant without it clearly being part of the contract.

So in the very least, any function that gets touched by something not 
carrying the qemu_mutex needs to have a comment in the definition and 
declaration that the functions are required to be re-entrant.  Also, the 
spice-display code would benefit from clearly stating where you were 
holding the qemu_mutex and where you weren't.

> +
> +void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate *update)
> +{
> +    qemu_free(update->bitmap);
> +    qemu_free(update);
> +}
> +
> +void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
> +{
> +    QXLDevMemSlot memslot;
> +
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
>    

a dprintf() would better fit qemu's style.

> +    memset(&memslot, 0, sizeof(memslot));
> +    memslot.slot_group_id = MEMSLOT_GROUP_HOST;
> +    memslot.virt_end = ~0;
> +    ssd->worker->add_memslot(ssd->worker,&memslot);
> +}
> +
> +void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
> +{
> +    QXLDevSurfaceCreate surface;
> +
> +    if (debug)
> +        fprintf(stderr, "%s: %dx%d\n", __FUNCTION__,
> +                ds_get_width(ssd->ds), ds_get_height(ssd->ds));
> +
> +    surface.format     = SPICE_SURFACE_FMT_32_xRGB;
> +    surface.width      = ds_get_width(ssd->ds);
> +    surface.height     = ds_get_height(ssd->ds);
> +    surface.stride     = -surface.width * 4;
> +    surface.mouse_mode = 0;
> +    surface.flags      = 0;
> +    surface.type       = 0;
> +    surface.mem        = (intptr_t)ssd->buf;
> +    surface.group_id   = MEMSLOT_GROUP_HOST;
> +    ssd->worker->create_primary_surface(ssd->worker, 0,&surface);
> +}
> +
> +void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
> +{
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +
> +    ssd->worker->destroy_primary_surface(ssd->worker, 0);
> +}
> +
> +void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
> +{
> +    SimpleSpiceDisplay *ssd = opaque;
> +
> +    if (running) {
> +        ssd->worker->start(ssd->worker);
> +    } else {
> +        ssd->worker->stop(ssd->worker);
> +    }
> +    ssd->running = running;
> +}
> +
> +/* display listener callbacks */
> +
> +void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
> +                               int x, int y, int w, int h)
> +{
> +    QXLRect update_area;
> +
> +    if (debug>  1)
> +        fprintf(stderr, "%s: x %d y %d w %d h %d\n", __FUNCTION__, x, y, w, h);
> +    update_area.left = x,
> +    update_area.right = x + w;
> +    update_area.top = y;
> +    update_area.bottom = y + h;
> +
> +    pthread_mutex_lock(&ssd->lock);
> +    if (qemu_spice_rect_is_empty(&ssd->dirty)) {
> +        ssd->notify++;
> +    }
> +    qemu_spice_rect_union(&ssd->dirty,&update_area);
> +    pthread_mutex_unlock(&ssd->lock);
> +}
> +
> +void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
> +{
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +
> +    pthread_mutex_lock(&ssd->lock);
> +    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> +    pthread_mutex_unlock(&ssd->lock);
> +
> +    qemu_spice_destroy_host_primary(ssd);
> +    qemu_spice_create_host_primary(ssd);
> +    qemu_pf_conv_put(ssd->conv);
> +    ssd->conv = NULL;
> +
> +    pthread_mutex_lock(&ssd->lock);
> +    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> +    ssd->notify++;
> +    pthread_mutex_unlock(&ssd->lock);
> +}
> +
> +void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
> +{
> +    if (debug>  2)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    vga_hw_update();
> +    if (ssd->notify) {
> +        ssd->notify = 0;
> +        ssd->worker->wakeup(ssd->worker);
> +        if (debug>  1)
> +            fprintf(stderr, "%s: notify\n", __FUNCTION__);
> +    }
> +}
> +
> +/* spice display interface callbacks */
> +
> +static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
> +{
> +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> +
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    ssd->worker = qxl_worker;
> +}
> +
> +static void interface_set_compression_level(QXLInstance *sin, int level)
> +{
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    /* nothing to do */
> +}
> +
> +static void interface_set_mm_time(QXLInstance *sin, uint32_t mm_time)
> +{
> +    if (debug>  2)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    /* nothing to do */
> +}
> +
> +static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
> +{
> +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> +
> +    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 = ssd->bufsize;
> +    info->n_surfaces = NUM_SURFACES;
> +}
> +
> +static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
> +{
> +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> +    SimpleSpiceUpdate *update;
> +
> +    if (debug>  2)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    update = qemu_spice_create_update(ssd);
> +    if (update == NULL) {
> +        return false;
> +    }
> +    *ext = update->ext;
> +    return true;
> +}
> +
> +static int interface_req_cmd_notification(QXLInstance *sin)
> +{
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    return 1;
> +}
> +
> +static void interface_release_resource(QXLInstance *sin,
> +                                       struct QXLReleaseInfoExt ext)
> +{
> +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> +    uintptr_t id;
> +
> +    if (debug>  1)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    id = ext.info->id;
> +    qemu_spice_destroy_update(ssd, (void*)id);
> +}
> +
> +static int interface_get_cursor_command(QXLInstance *sin, struct QXLCommandExt *ext)
> +{
> +    if (debug>  2)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    return false;
> +}
> +
> +static int interface_req_cursor_notification(QXLInstance *sin)
> +{
> +    if (debug)
> +        fprintf(stderr, "%s:\n", __FUNCTION__);
> +    return 1;
> +}
> +
> +static void interface_notify_update(QXLInstance *sin, uint32_t update_id)
> +{
> +    fprintf(stderr, "%s: abort()\n", __FUNCTION__);
> +    abort();
> +}
> +
> +static int interface_flush_resources(QXLInstance *sin)
> +{
> +    fprintf(stderr, "%s: abort()\n", __FUNCTION__);
> +    abort();
> +    return 0;
> +}
> +
> +static const QXLInterface dpy_interface = {
> +    .base.type               = SPICE_INTERFACE_QXL,
> +    .base.description        = "qemu simple display",
> +    .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,
> +    .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 SimpleSpiceDisplay sdpy;
> +
> +static void display_update(struct DisplayState *ds, int x, int y, int w, int h)
> +{
> +    qemu_spice_display_update(&sdpy, x, y, w, h);
> +}
> +
> +static void display_resize(struct DisplayState *ds)
> +{
> +    qemu_spice_display_resize(&sdpy);
> +}
> +
> +static void display_refresh(struct DisplayState *ds)
> +{
> +    qemu_spice_display_refresh(&sdpy);
> +}
> +
> +static DisplayChangeListener display_listener = {
> +    .dpy_update  = display_update,
> +    .dpy_resize  = display_resize,
> +    .dpy_refresh = display_refresh,
> +};
> +
> +void qemu_spice_display_init(DisplayState *ds)
> +{
> +    assert(sdpy.ds == NULL);
> +    sdpy.ds = ds;
> +    sdpy.bufsize = (16 * 1024 * 1024);
> +    sdpy.buf = qemu_malloc(sdpy.bufsize);
> +    pthread_mutex_init(&sdpy.lock, NULL);
> +    register_displaychangelistener(ds,&display_listener);
> +
> +    sdpy.qxl.base.sif =&dpy_interface.base;
> +    spice_server_add_interface(spice_server,&sdpy.qxl.base);
> +    assert(sdpy.worker);
> +
> +    qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy);
> +    qemu_spice_create_host_memslot(&sdpy);
> +    qemu_spice_create_host_primary(&sdpy);
> +}
> diff --git a/spice-display.h b/spice-display.h
> new file mode 100644
> index 0000000..b55e7ea
> --- /dev/null
> +++ b/spice-display.h
> @@ -0,0 +1,52 @@
> +#include<spice/ipc_ring.h>
> +#include<spice/enums.h>
> +#include<spice/qxl_dev.h>
> +
> +#include "pflib.h"
> +
> +#define NUM_MEMSLOTS 8
> +#define MEMSLOT_GENERATION_BITS 8
> +#define MEMSLOT_SLOT_BITS 8
> +
> +#define MEMSLOT_GROUP_HOST  0
> +#define MEMSLOT_GROUP_GUEST 1
> +#define NUM_MEMSLOTS_GROUPS 2
> +
> +#define NUM_SURFACES 1024
> +
> +typedef struct SimpleSpiceDisplay {
> +    DisplayState *ds;
> +    void *buf;
> +    int bufsize;
> +    QXLWorker *worker;
> +    QXLInstance qxl;
> +    uint32_t unique;
> +    QemuPfConv *conv;
> +
> +    pthread_mutex_t lock;
> +    QXLRect dirty;
> +    int notify;
> +    int running;
> +} SimpleSpiceDisplay;
> +
> +typedef struct SimpleSpiceUpdate {
> +    QXLDrawable drawable;
> +    QXLImage image;
> +    QXLCommandExt ext;
> +    uint8_t *bitmap;
> +} SimpleSpiceUpdate;
> +
> +int qemu_spice_rect_is_empty(const QXLRect* r);
> +void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
> +
> +SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *sdpy);
> +void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate *update);
> +void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd);
> +void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd);
> +void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd);
> +void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason);
> +
> +void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
> +                               int x, int y, int w, int h);
> +void qemu_spice_display_resize(SimpleSpiceDisplay *ssd);
> +void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd);
> diff --git a/vl.c b/vl.c
> index 7f391c0..f68f8e8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2910,7 +2910,7 @@ int main(int argc, char **argv, char **envp)
>       /* just use the first displaystate for the moment */
>       ds = get_displaystate();
>
> -    if (display_type == DT_DEFAULT) {
> +    if (display_type == DT_DEFAULT&&  !using_spice) {
>   #if defined(CONFIG_SDL) || defined(CONFIG_COCOA)
>           display_type = DT_SDL;
>   #else
> @@ -2950,6 +2950,11 @@ int main(int argc, char **argv, char **envp)
>       default:
>           break;
>       }
> +#ifdef CONFIG_SPICE
> +    if (using_spice) {
> +        qemu_spice_display_init(ds);
> +    }
> +#endif
>    

Having spice as an independent interface to the current display_type 
switching seems awkward to me.

Regards,

Anthony Liguori

>       dpy_resize(ds);
>
>       dcl = ds->listeners;
>

On Thu, 19 Aug 2010, Anthony Liguori wrote:

> On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> > With that patch applied you'll actually see the guests screen in the
> > spice client.  This does *not* bring qxl and full spice support though.
> > This is basically the qxl vga mode made more generic, so it plays
> > together with any qemu-emulated gfx card.  You can display stdvga or
> > cirrus via spice client.  You can have both vnc and spice enabled and
> > clients connected at the same time.
[..snip..]

> > +
> > +void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate
> > *update)
> > +{
> > +    qemu_free(update->bitmap);
> > +    qemu_free(update);
> > +}
> > +
> > +void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
> > +{
> > +    QXLDevMemSlot memslot;
> > +
> > +    if (debug)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> >    
> 
> a dprintf() would better fit qemu's style.

A dprintf is a POSIX function.

> 
> > +    memset(&memslot, 0, sizeof(memslot));
> > +    memslot.slot_group_id = MEMSLOT_GROUP_HOST;
> > +    memslot.virt_end = ~0;
> > +    ssd->worker->add_memslot(ssd->worker,&memslot);
> > +}
> > +
> > +void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
> > +{
> > +    QXLDevSurfaceCreate surface;
> > +
> > +    if (debug)
> > +        fprintf(stderr, "%s: %dx%d\n", __FUNCTION__,
> > +                ds_get_width(ssd->ds), ds_get_height(ssd->ds));
> > +
> > +    surface.format     = SPICE_SURFACE_FMT_32_xRGB;
> > +    surface.width      = ds_get_width(ssd->ds);
> > +    surface.height     = ds_get_height(ssd->ds);
> > +    surface.stride     = -surface.width * 4;
> > +    surface.mouse_mode = 0;
> > +    surface.flags      = 0;
> > +    surface.type       = 0;
> > +    surface.mem        = (intptr_t)ssd->buf;
> > +    surface.group_id   = MEMSLOT_GROUP_HOST;
> > +    ssd->worker->create_primary_surface(ssd->worker, 0,&surface);
> > +}
> > +
> > +void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
> > +{
> > +    if (debug)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +
> > +    ssd->worker->destroy_primary_surface(ssd->worker, 0);
> > +}
> > +
> > +void qemu_spice_vm_change_state_handler(void *opaque, int running, int
> > reason)
> > +{
> > +    SimpleSpiceDisplay *ssd = opaque;
> > +
> > +    if (running) {
> > +        ssd->worker->start(ssd->worker);
> > +    } else {
> > +        ssd->worker->stop(ssd->worker);
> > +    }
> > +    ssd->running = running;
> > +}
> > +
> > +/* display listener callbacks */
> > +
> > +void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
> > +                               int x, int y, int w, int h)
> > +{
> > +    QXLRect update_area;
> > +
> > +    if (debug>  1)
> > +        fprintf(stderr, "%s: x %d y %d w %d h %d\n", __FUNCTION__, x, y, w,
> > h);
> > +    update_area.left = x,
> > +    update_area.right = x + w;
> > +    update_area.top = y;
> > +    update_area.bottom = y + h;
> > +
> > +    pthread_mutex_lock(&ssd->lock);
> > +    if (qemu_spice_rect_is_empty(&ssd->dirty)) {
> > +        ssd->notify++;
> > +    }
> > +    qemu_spice_rect_union(&ssd->dirty,&update_area);
> > +    pthread_mutex_unlock(&ssd->lock);
> > +}
> > +
> > +void qemu_spice_display_resize(SimpleSpiceDisplay *ssd)
> > +{
> > +    if (debug)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +
> > +    pthread_mutex_lock(&ssd->lock);
> > +    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> > +    pthread_mutex_unlock(&ssd->lock);
> > +
> > +    qemu_spice_destroy_host_primary(ssd);
> > +    qemu_spice_create_host_primary(ssd);
> > +    qemu_pf_conv_put(ssd->conv);
> > +    ssd->conv = NULL;
> > +
> > +    pthread_mutex_lock(&ssd->lock);
> > +    memset(&ssd->dirty, 0, sizeof(ssd->dirty));
> > +    ssd->notify++;
> > +    pthread_mutex_unlock(&ssd->lock);
> > +}
> > +
> > +void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
> > +{
> > +    if (debug>  2)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +    vga_hw_update();
> > +    if (ssd->notify) {
> > +        ssd->notify = 0;
> > +        ssd->worker->wakeup(ssd->worker);
> > +        if (debug>  1)
> > +            fprintf(stderr, "%s: notify\n", __FUNCTION__);
> > +    }
> > +}
> > +
> > +/* spice display interface callbacks */
> > +
> > +static void interface_attach_worker(QXLInstance *sin, QXLWorker
> > *qxl_worker)
> > +{
> > +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> > +
> > +    if (debug)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +    ssd->worker = qxl_worker;
> > +}
> > +
> > +static void interface_set_compression_level(QXLInstance *sin, int level)
> > +{
> > +    if (debug)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +    /* nothing to do */
> > +}
> > +
> > +static void interface_set_mm_time(QXLInstance *sin, uint32_t mm_time)
> > +{
> > +    if (debug>  2)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +    /* nothing to do */
> > +}
> > +
> > +static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
> > +{
> > +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> > +
> > +    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 = ssd->bufsize;
> > +    info->n_surfaces = NUM_SURFACES;
> > +}
> > +
> > +static int interface_get_command(QXLInstance *sin, struct QXLCommandExt
> > *ext)
> > +{
> > +    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> > +    SimpleSpiceUpdate *update;
> > +
> > +    if (debug>  2)
> > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > +    update = qemu_spice_create_update(ssd);
[..snip..]

On 08/19/2010 10:23 AM, malc wrote:
>
>>> +
>>> +void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate
>>> *update)
>>> +{
>>> +    qemu_free(update->bitmap);
>>> +    qemu_free(update);
>>> +}
>>> +
>>> +void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
>>> +{
>>> +    QXLDevMemSlot memslot;
>>> +
>>> +    if (debug)
>>> +        fprintf(stderr, "%s:\n", __FUNCTION__);
>>>
>>>        
>> a dprintf() would better fit qemu's style.
>>      
> A dprintf is a POSIX function.
>    

Apparently a recent POSIX function...

Preferred alternatives?  I really dislike all caps macros.

Regards,

Anthony Liguori

On Thu, 19 Aug 2010, Anthony Liguori wrote:

> On 08/19/2010 10:23 AM, malc wrote:
> > 
> > > > +
> > > > +void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy,
> > > > SimpleSpiceUpdate
> > > > *update)
> > > > +{
> > > > +    qemu_free(update->bitmap);
> > > > +    qemu_free(update);
> > > > +}
> > > > +
> > > > +void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
> > > > +{
> > > > +    QXLDevMemSlot memslot;
> > > > +
> > > > +    if (debug)
> > > > +        fprintf(stderr, "%s:\n", __FUNCTION__);
> > > > 
> > > >        
> > > a dprintf() would better fit qemu's style.
> > >      
> > A dprintf is a POSIX function.
> >    
> 
> Apparently a recent POSIX function...
> 
> Preferred alternatives?  I really dislike all caps macros.

Macros should be all capps.

[..snip..]

On 19.08.2010, at 17:34, Anthony Liguori wrote:

> On 08/19/2010 10:23 AM, malc wrote:
>> 
>>>> +
>>>> +void qemu_spice_destroy_update(SimpleSpiceDisplay *sdpy, SimpleSpiceUpdate
>>>> *update)
>>>> +{
>>>> +    qemu_free(update->bitmap);
>>>> +    qemu_free(update);
>>>> +}
>>>> +
>>>> +void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
>>>> +{
>>>> +    QXLDevMemSlot memslot;
>>>> +
>>>> +    if (debug)
>>>> +        fprintf(stderr, "%s:\n", __FUNCTION__);
>>>> 
>>>>       
>>> a dprintf() would better fit qemu's style.
>>>     
>> A dprintf is a POSIX function.
>>   
> 
> Apparently a recent POSIX function...

In my man page it's declared as

       #define _GNU_SOURCE
       #include <stdio.h>

       int dprintf(int fd, const char *format, ...);

but maybe it did become POSIX recently. Either way, it's used already :).

> Preferred alternatives?  I really dislike all caps macros.

printd should be fine.

Alex

Hi,

>> + pthread_mutex_lock(&ssd->lock);
>
> The locking worries me here.
>
> It only makes sense if the spice interface_* callbacks can be invoked
> from threads independently of any of the QEMU threads.
>
> If that's the case, that means that the interface_* code is potentially
> running in parallel to another QEMU thread that's holding the qemu_mutex.

Yes, interface_* code can be called back from spice server thread context.

> So just acquiring a private lock in the interface_* code and then
> calling into common QEMU code could result in re-entrance in interfaces
> that aren't re-entrant.

I think there are no such calls.

> While not really unsafe, the qemu_malloc functions are not guaranteed to
> be re-entrant by the interfaces today. It's also terribly subtle to have
> to rely on implicit re-entrance safety.

The underlying malloc() is re-entrant, isn't it?
pflib (which is called too) is re-entrant too.
Otherwise only private data is accessed (under lock when needed).

> So in the very least, any function that gets touched by something not
> carrying the qemu_mutex needs to have a comment in the definition and
> declaration that the functions are required to be re-entrant. Also, the
> spice-display code would benefit from clearly stating where you were
> holding the qemu_mutex and where you weren't.

Yep.

>> +#ifdef CONFIG_SPICE
>> + if (using_spice) {
>> + qemu_spice_display_init(ds);
>> + }
>> +#endif
>
> Having spice as an independent interface to the current display_type
> switching seems awkward to me.

Having remote desktop protocols as DT_something seems awkward to me.  It 
makes sense for the local display (being none, curses, sdl, fbdev, 
whatever).  For remote display protocols I see no reason why we 
shouldn't have multiple of them enabled at the same time, so the user 
can connect with whatever he wants.  And that even in parallel to a 
local display if needed.

The state the patch introduces is a bit inconsistent though.  But I'd 
rather drop DT_VNC instead of adding DT_SPICE.

cheers,
   Gerd

On 08/19/2010 11:05 AM, Gerd Hoffmann wrote:
>> While not really unsafe, the qemu_malloc functions are not guaranteed to
>> be re-entrant by the interfaces today. It's also terribly subtle to have
>> to rely on implicit re-entrance safety.
>
> The underlying malloc() is re-entrant, isn't it?
> pflib (which is called too) is re-entrant too.
> Otherwise only private data is accessed (under lock when needed).

Yes, I looked too and agree that it's safe now.  But we're sloppy as 
hell in qemu about depending on global state and I can imagine someone 
adding something to these functions that would create an issue.

I think documentation would be sufficient, at least for now.

>> Having spice as an independent interface to the current display_type
>> switching seems awkward to me.
>
> Having remote desktop protocols as DT_something seems awkward to me.  
> It makes sense for the local display (being none, curses, sdl, fbdev, 
> whatever).  For remote display protocols I see no reason why we 
> shouldn't have multiple of them enabled at the same time, so the user 
> can connect with whatever he wants.  And that even in parallel to a 
> local display if needed.
>
> The state the patch introduces is a bit inconsistent though.  But I'd 
> rather drop DT_VNC instead of adding DT_SPICE.

Yes, I would think that would be reasonable.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

On 08/19/2010 10:36 AM, malc wrote:
>> Apparently a recent POSIX function...
>>
>> Preferred alternatives?  I really dislike all caps macros.
>>      
> Macros should be all capps.
>    

If you like angry code.

But honestly, just make printd() a static inline and then every one's 
happy :-)

Regards,

Anthony Liguori

> [..snip..]
>
>

On Thu, 19 Aug 2010, Anthony Liguori wrote:

> On 08/19/2010 10:36 AM, malc wrote:
> > > Apparently a recent POSIX function...
> > > 
> > > Preferred alternatives?  I really dislike all caps macros.
> > >      
> > Macros should be all capps.
> >    
> 
> If you like angry code.

I'm a self-flagelation kind of person.

> 
> But honestly, just make printd() a static inline and then every one's happy
> :-)

That much is true.

> 
> Regards,
> 
> Anthony Liguori
> 
> > [..snip..]
> > 
> >

> In my man page it's declared as
>
>         #define _GNU_SOURCE
>         #include<stdio.h>
>
>         int dprintf(int fd, const char *format, ...);
>
> but maybe it did become POSIX recently. Either way, it's used already :).

Same man-page says:

CONFORMING TO
        These functions are GNU extensions that are nowadays
        specified in POSIX.1-2008

cheers,
   Gerd