[Qemu-devel] [PATCH 0/9] initial spice support.

[Qemu-devel] [PATCH 0/9] initial spice support.

[Gerd Hoffmann]

  Hi,

/me wants getting the spice support merging started, here is the first
batch of patches.  It brings just the very basic bits:

 * Detect spice in configure, Makefile windup.
 * Support for keyboard, mouse and tablet.
 * Support for simple display output (works as DisplayChangeListener,
   plays with any gfx card, sends simple draw commands to update
   dirty regions).

Note that this patch series does *not* yet contain the qxl paravirtual
gfx card.  That will come as part of a additional patch series after
sorting the vgabios support.

The patches are also available in the git repository at:
  git://anongit.freedesktop.org/spice/qemu submit.2

cheers,
  Gerd

Gerd Hoffmann (9):
  add pflib: PixelFormat conversion library.
  configure: add logging
  add spice into the configure file
  configure: require spice 0.5.3
  spice: core bits
  spice: add keyboard
  spice: add mouse
  spice: simple display
  spice: add tablet support

 Makefile.objs   |    3 +
 configure       |   42 ++++++-
 pflib.c         |  213 ++++++++++++++++++++++++++++++
 pflib.h         |   20 +++
 qemu-config.c   |   23 ++++
 qemu-config.h   |    1 +
 qemu-options.hx |    8 +
 qemu-spice.h    |   24 ++++
 spice-display.c |  387 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 spice-display.h |   52 ++++++++
 spice-input.c   |  173 +++++++++++++++++++++++++
 spice.c         |  153 ++++++++++++++++++++++
 vl.c            |   22 +++-
 13 files changed, 1118 insertions(+), 3 deletions(-)
 create mode 100644 pflib.c
 create mode 100644 pflib.h
 create mode 100644 qemu-spice.h
 create mode 100644 spice-display.c
 create mode 100644 spice-display.h
 create mode 100644 spice-input.c
 create mode 100644 spice.c

[Qemu-devel] [PATCH 1/9] add pflib: PixelFormat conversion library.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs |    1 +
 pflib.c       |  213 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pflib.h       |   20 ++++++
 3 files changed, 234 insertions(+), 0 deletions(-)
 create mode 100644 pflib.c
 create mode 100644 pflib.h

diff --git a/Makefile.objs b/Makefile.objs
index 4a1eaa1..edfca87 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -83,6 +83,7 @@ common-obj-y += qemu-char.o savevm.o #aio.o
 common-obj-y += msmouse.o ps2.o
 common-obj-y += qdev.o qdev-properties.o
 common-obj-y += block-migration.o
+common-obj-y += pflib.o
 
 common-obj-$(CONFIG_BRLAPI) += baum.o
 common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
diff --git a/pflib.c b/pflib.c
new file mode 100644
index 0000000..1154d0c
--- /dev/null
+++ b/pflib.c
@@ -0,0 +1,213 @@
+/*
+ * PixelFormat conversion library.
+ *
+ * Author: Gerd Hoffmann <kraxel@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#include "qemu-common.h"
+#include "console.h"
+#include "pflib.h"
+
+typedef struct QemuPixel QemuPixel;
+
+typedef void (*pf_convert)(QemuPfConv *conv,
+                           void *dst, void *src, uint32_t cnt);
+typedef void (*pf_convert_from)(PixelFormat *pf,
+                                QemuPixel *dst, void *src, uint32_t cnt);
+typedef void (*pf_convert_to)(PixelFormat *pf,
+                              void *dst, QemuPixel *src, uint32_t cnt);
+
+struct QemuPfConv {
+    pf_convert        convert;
+    PixelFormat       src;
+    PixelFormat       dst;
+
+    /* for copy_generic() */
+    pf_convert_from   conv_from;
+    pf_convert_to     conv_to;
+    QemuPixel         *conv_buf;
+    uint32_t          conv_cnt;
+};
+
+struct QemuPixel {
+    uint8_t red;
+    uint8_t green;
+    uint8_t blue;
+    uint8_t alpha;
+};
+
+/* ----------------------------------------------------------------------- */
+/* PixelFormat -> QemuPixel conversions                                    */
+
+static void conv_16_to_pixel(PixelFormat *pf,
+                             QemuPixel *dst, void *src, uint32_t cnt)
+{
+    uint16_t *src16 = src;
+
+    while (cnt > 0) {
+        dst->red   = ((*src16 & pf->rmask) >> pf->rshift) << (8 - pf->rbits);
+        dst->green = ((*src16 & pf->gmask) >> pf->gshift) << (8 - pf->gbits);
+        dst->blue  = ((*src16 & pf->bmask) >> pf->bshift) << (8 - pf->bbits);
+        dst->alpha = ((*src16 & pf->amask) >> pf->ashift) << (8 - pf->abits);
+        dst++, src16++, cnt--;
+    }
+}
+
+/* assumes pf->{r,g,b,a}bits == 8 */
+static void conv_32_to_pixel_fast(PixelFormat *pf,
+                                  QemuPixel *dst, void *src, uint32_t cnt)
+{
+    uint32_t *src32 = src;
+
+    while (cnt > 0) {
+        dst->red   = (*src32 & pf->rmask) >> pf->rshift;
+        dst->green = (*src32 & pf->gmask) >> pf->gshift;
+        dst->blue  = (*src32 & pf->bmask) >> pf->bshift;
+        dst->alpha = (*src32 & pf->amask) >> pf->ashift;
+        dst++, src32++, cnt--;
+    }
+}
+
+static void conv_32_to_pixel_generic(PixelFormat *pf,
+                                     QemuPixel *dst, void *src, uint32_t cnt)
+{
+    uint32_t *src32 = src;
+
+    while (cnt > 0) {
+        if (pf->rbits < 8) {
+            dst->red   = ((*src32 & pf->rmask) >> pf->rshift) << (8 - pf->rbits);
+        } else {
+            dst->red   = ((*src32 & pf->rmask) >> pf->rshift) >> (pf->rbits - 8);
+        }
+        if (pf->gbits < 8) {
+            dst->green = ((*src32 & pf->gmask) >> pf->gshift) << (8 - pf->gbits);
+        } else {
+            dst->green = ((*src32 & pf->gmask) >> pf->gshift) >> (pf->gbits - 8);
+        }
+        if (pf->bbits < 8) {
+            dst->blue  = ((*src32 & pf->bmask) >> pf->bshift) << (8 - pf->bbits);
+        } else {
+            dst->blue  = ((*src32 & pf->bmask) >> pf->bshift) >> (pf->bbits - 8);
+        }
+        if (pf->abits < 8) {
+            dst->alpha = ((*src32 & pf->amask) >> pf->ashift) << (8 - pf->abits);
+        } else {
+            dst->alpha = ((*src32 & pf->amask) >> pf->ashift) >> (pf->abits - 8);
+        }
+        dst++, src32++, cnt--;
+    }
+}
+
+/* ----------------------------------------------------------------------- */
+/* QemuPixel -> PixelFormat conversions                                    */
+
+static void conv_pixel_to_16(PixelFormat *pf,
+                             void *dst, QemuPixel *src, uint32_t cnt)
+{
+    uint16_t *dst16 = dst;
+
+    while (cnt > 0) {
+        *dst16  = ((uint16_t)src->red   >> (8 - pf->rbits)) << pf->rshift;
+        *dst16 |= ((uint16_t)src->green >> (8 - pf->gbits)) << pf->gshift;
+        *dst16 |= ((uint16_t)src->blue  >> (8 - pf->bbits)) << pf->bshift;
+        *dst16 |= ((uint16_t)src->alpha >> (8 - pf->abits)) << pf->ashift;
+        dst16++, src++, cnt--;
+    }
+}
+
+static void conv_pixel_to_32(PixelFormat *pf,
+                             void *dst, QemuPixel *src, uint32_t cnt)
+{
+    uint32_t *dst32 = dst;
+
+    while (cnt > 0) {
+        *dst32  = ((uint32_t)src->red   >> (8 - pf->rbits)) << pf->rshift;
+        *dst32 |= ((uint32_t)src->green >> (8 - pf->gbits)) << pf->gshift;
+        *dst32 |= ((uint32_t)src->blue  >> (8 - pf->bbits)) << pf->bshift;
+        *dst32 |= ((uint32_t)src->alpha >> (8 - pf->abits)) << pf->ashift;
+        dst32++, src++, cnt--;
+    }
+}
+
+/* ----------------------------------------------------------------------- */
+/* PixelFormat -> PixelFormat conversions                                  */
+
+static void convert_copy(QemuPfConv *conv, void *dst, void *src, uint32_t cnt)
+{
+    uint32_t bytes = cnt * conv->src.bytes_per_pixel;
+    memcpy(dst, src, bytes);
+}
+
+static void convert_generic(QemuPfConv *conv, void *dst, void *src, uint32_t cnt)
+{
+    if (conv->conv_cnt < cnt) {
+        conv->conv_cnt = cnt;
+        conv->conv_buf = qemu_realloc(conv->conv_buf, sizeof(QemuPixel) * conv->conv_cnt);
+    }
+    conv->conv_from(&conv->src, conv->conv_buf, src, cnt);
+    conv->conv_to(&conv->dst, dst, conv->conv_buf, cnt);
+}
+
+/* ----------------------------------------------------------------------- */
+/* public interface                                                        */
+
+QemuPfConv *qemu_pf_conv_get(PixelFormat *dst, PixelFormat *src)
+{
+    QemuPfConv *conv = qemu_mallocz(sizeof(QemuPfConv));
+
+    conv->src = *src;
+    conv->dst = *dst;
+
+    if (memcmp(&conv->src, &conv->dst, sizeof(PixelFormat)) == 0) {
+        /* formats identical, can simply copy */
+        conv->convert = convert_copy;
+    } else {
+        /* generic two-step conversion: src -> QemuPixel -> dst  */
+        switch (conv->src.bytes_per_pixel) {
+        case 2:
+            conv->conv_from = conv_16_to_pixel;
+            break;
+        case 4:
+            if (conv->src.rbits == 8 && conv->src.gbits == 8 && conv->src.bbits == 8) {
+                conv->conv_from = conv_32_to_pixel_fast;
+            } else {
+                conv->conv_from = conv_32_to_pixel_generic;
+            }
+            break;
+        default:
+            goto err;
+        }
+        switch (conv->dst.bytes_per_pixel) {
+        case 2:
+            conv->conv_to = conv_pixel_to_16;
+            break;
+        case 4:
+            conv->conv_to = conv_pixel_to_32;
+            break;
+        default:
+            goto err;
+        }
+        conv->convert = convert_generic;
+    }
+    return conv;
+
+err:
+    qemu_free(conv);
+    return NULL;
+}
+
+void qemu_pf_conv_run(QemuPfConv *conv, void *dst, void *src, uint32_t cnt)
+{
+    conv->convert(conv, dst, src, cnt);
+}
+
+void qemu_pf_conv_put(QemuPfConv *conv)
+{
+    if (conv) {
+        qemu_free(conv->conv_buf);
+        qemu_free(conv);
+    }
+}
diff --git a/pflib.h b/pflib.h
new file mode 100644
index 0000000..b70c313
--- /dev/null
+++ b/pflib.h
@@ -0,0 +1,20 @@
+#ifndef __QEMU_PFLIB_H
+#define __QEMU_PFLIB_H
+
+/*
+ * PixelFormat conversion library.
+ *
+ * Author: Gerd Hoffmann <kraxel@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+typedef struct QemuPfConv QemuPfConv;
+
+QemuPfConv *qemu_pf_conv_get(PixelFormat *dst, PixelFormat *src);
+void qemu_pf_conv_run(QemuPfConv *conv, void *dst, void *src, uint32_t cnt);
+void qemu_pf_conv_put(QemuPfConv *conv);
+
+#endif
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/9] add pflib: PixelFormat conversion library.

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>    

This code is more or less stand alone so it's a good candidate for 
having some in-tree unit tests.

I can provide some examples of how it should be done if you like, but 
just something that lives in tests/ that exercises the functionality and 
can be run under valgrind to verify that we aren't leaking memory.

Using libcheck is also reasonable (take a look at the various check-q* 
in the top level).

Regards,

Anthony Liguori

> ---
>   Makefile.objs |    1 +
>   pflib.c       |  213 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   pflib.h       |   20 ++++++
>   3 files changed, 234 insertions(+), 0 deletions(-)
>   create mode 100644 pflib.c
>   create mode 100644 pflib.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 4a1eaa1..edfca87 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -83,6 +83,7 @@ common-obj-y += qemu-char.o savevm.o #aio.o
>   common-obj-y += msmouse.o ps2.o
>   common-obj-y += qdev.o qdev-properties.o
>   common-obj-y += block-migration.o
> +common-obj-y += pflib.o
>
>   common-obj-$(CONFIG_BRLAPI) += baum.o
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
> diff --git a/pflib.c b/pflib.c
> new file mode 100644
> index 0000000..1154d0c
> --- /dev/null
> +++ b/pflib.c
> @@ -0,0 +1,213 @@
> +/*
> + * PixelFormat conversion library.
> + *
> + * Author: Gerd Hoffmann<kraxel@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +#include "qemu-common.h"
> +#include "console.h"
> +#include "pflib.h"
> +
> +typedef struct QemuPixel QemuPixel;
> +
> +typedef void (*pf_convert)(QemuPfConv *conv,
> +                           void *dst, void *src, uint32_t cnt);
> +typedef void (*pf_convert_from)(PixelFormat *pf,
> +                                QemuPixel *dst, void *src, uint32_t cnt);
> +typedef void (*pf_convert_to)(PixelFormat *pf,
> +                              void *dst, QemuPixel *src, uint32_t cnt);
> +
> +struct QemuPfConv {
> +    pf_convert        convert;
> +    PixelFormat       src;
> +    PixelFormat       dst;
> +
> +    /* for copy_generic() */
> +    pf_convert_from   conv_from;
> +    pf_convert_to     conv_to;
> +    QemuPixel         *conv_buf;
> +    uint32_t          conv_cnt;
> +};
> +
> +struct QemuPixel {
> +    uint8_t red;
> +    uint8_t green;
> +    uint8_t blue;
> +    uint8_t alpha;
> +};
> +
> +/* ----------------------------------------------------------------------- */
> +/* PixelFormat ->  QemuPixel conversions                                    */
> +
> +static void conv_16_to_pixel(PixelFormat *pf,
> +                             QemuPixel *dst, void *src, uint32_t cnt)
> +{
> +    uint16_t *src16 = src;
> +
> +    while (cnt>  0) {
> +        dst->red   = ((*src16&  pf->rmask)>>  pf->rshift)<<  (8 - pf->rbits);
> +        dst->green = ((*src16&  pf->gmask)>>  pf->gshift)<<  (8 - pf->gbits);
> +        dst->blue  = ((*src16&  pf->bmask)>>  pf->bshift)<<  (8 - pf->bbits);
> +        dst->alpha = ((*src16&  pf->amask)>>  pf->ashift)<<  (8 - pf->abits);
> +        dst++, src16++, cnt--;
> +    }
> +}
> +
> +/* assumes pf->{r,g,b,a}bits == 8 */
> +static void conv_32_to_pixel_fast(PixelFormat *pf,
> +                                  QemuPixel *dst, void *src, uint32_t cnt)
> +{
> +    uint32_t *src32 = src;
> +
> +    while (cnt>  0) {
> +        dst->red   = (*src32&  pf->rmask)>>  pf->rshift;
> +        dst->green = (*src32&  pf->gmask)>>  pf->gshift;
> +        dst->blue  = (*src32&  pf->bmask)>>  pf->bshift;
> +        dst->alpha = (*src32&  pf->amask)>>  pf->ashift;
> +        dst++, src32++, cnt--;
> +    }
> +}
> +
> +static void conv_32_to_pixel_generic(PixelFormat *pf,
> +                                     QemuPixel *dst, void *src, uint32_t cnt)
> +{
> +    uint32_t *src32 = src;
> +
> +    while (cnt>  0) {
> +        if (pf->rbits<  8) {
> +            dst->red   = ((*src32&  pf->rmask)>>  pf->rshift)<<  (8 - pf->rbits);
> +        } else {
> +            dst->red   = ((*src32&  pf->rmask)>>  pf->rshift)>>  (pf->rbits - 8);
> +        }
> +        if (pf->gbits<  8) {
> +            dst->green = ((*src32&  pf->gmask)>>  pf->gshift)<<  (8 - pf->gbits);
> +        } else {
> +            dst->green = ((*src32&  pf->gmask)>>  pf->gshift)>>  (pf->gbits - 8);
> +        }
> +        if (pf->bbits<  8) {
> +            dst->blue  = ((*src32&  pf->bmask)>>  pf->bshift)<<  (8 - pf->bbits);
> +        } else {
> +            dst->blue  = ((*src32&  pf->bmask)>>  pf->bshift)>>  (pf->bbits - 8);
> +        }
> +        if (pf->abits<  8) {
> +            dst->alpha = ((*src32&  pf->amask)>>  pf->ashift)<<  (8 - pf->abits);
> +        } else {
> +            dst->alpha = ((*src32&  pf->amask)>>  pf->ashift)>>  (pf->abits - 8);
> +        }
> +        dst++, src32++, cnt--;
> +    }
> +}
> +
> +/* ----------------------------------------------------------------------- */
> +/* QemuPixel ->  PixelFormat conversions                                    */
> +
> +static void conv_pixel_to_16(PixelFormat *pf,
> +                             void *dst, QemuPixel *src, uint32_t cnt)
> +{
> +    uint16_t *dst16 = dst;
> +
> +    while (cnt>  0) {
> +        *dst16  = ((uint16_t)src->red>>  (8 - pf->rbits))<<  pf->rshift;
> +        *dst16 |= ((uint16_t)src->green>>  (8 - pf->gbits))<<  pf->gshift;
> +        *dst16 |= ((uint16_t)src->blue>>  (8 - pf->bbits))<<  pf->bshift;
> +        *dst16 |= ((uint16_t)src->alpha>>  (8 - pf->abits))<<  pf->ashift;
> +        dst16++, src++, cnt--;
> +    }
> +}
> +
> +static void conv_pixel_to_32(PixelFormat *pf,
> +                             void *dst, QemuPixel *src, uint32_t cnt)
> +{
> +    uint32_t *dst32 = dst;
> +
> +    while (cnt>  0) {
> +        *dst32  = ((uint32_t)src->red>>  (8 - pf->rbits))<<  pf->rshift;
> +        *dst32 |= ((uint32_t)src->green>>  (8 - pf->gbits))<<  pf->gshift;
> +        *dst32 |= ((uint32_t)src->blue>>  (8 - pf->bbits))<<  pf->bshift;
> +        *dst32 |= ((uint32_t)src->alpha>>  (8 - pf->abits))<<  pf->ashift;
> +        dst32++, src++, cnt--;
> +    }
> +}
> +
> +/* ----------------------------------------------------------------------- */
> +/* PixelFormat ->  PixelFormat conversions                                  */
> +
> +static void convert_copy(QemuPfConv *conv, void *dst, void *src, uint32_t cnt)
> +{
> +    uint32_t bytes = cnt * conv->src.bytes_per_pixel;
> +    memcpy(dst, src, bytes);
> +}
> +
> +static void convert_generic(QemuPfConv *conv, void *dst, void *src, uint32_t cnt)
> +{
> +    if (conv->conv_cnt<  cnt) {
> +        conv->conv_cnt = cnt;
> +        conv->conv_buf = qemu_realloc(conv->conv_buf, sizeof(QemuPixel) * conv->conv_cnt);
> +    }
> +    conv->conv_from(&conv->src, conv->conv_buf, src, cnt);
> +    conv->conv_to(&conv->dst, dst, conv->conv_buf, cnt);
> +}
> +
> +/* ----------------------------------------------------------------------- */
> +/* public interface                                                        */
> +
> +QemuPfConv *qemu_pf_conv_get(PixelFormat *dst, PixelFormat *src)
> +{
> +    QemuPfConv *conv = qemu_mallocz(sizeof(QemuPfConv));
> +
> +    conv->src = *src;
> +    conv->dst = *dst;
> +
> +    if (memcmp(&conv->src,&conv->dst, sizeof(PixelFormat)) == 0) {
> +        /* formats identical, can simply copy */
> +        conv->convert = convert_copy;
> +    } else {
> +        /* generic two-step conversion: src ->  QemuPixel ->  dst  */
> +        switch (conv->src.bytes_per_pixel) {
> +        case 2:
> +            conv->conv_from = conv_16_to_pixel;
> +            break;
> +        case 4:
> +            if (conv->src.rbits == 8&&  conv->src.gbits == 8&&  conv->src.bbits == 8) {
> +                conv->conv_from = conv_32_to_pixel_fast;
> +            } else {
> +                conv->conv_from = conv_32_to_pixel_generic;
> +            }
> +            break;
> +        default:
> +            goto err;
> +        }
> +        switch (conv->dst.bytes_per_pixel) {
> +        case 2:
> +            conv->conv_to = conv_pixel_to_16;
> +            break;
> +        case 4:
> +            conv->conv_to = conv_pixel_to_32;
> +            break;
> +        default:
> +            goto err;
> +        }
> +        conv->convert = convert_generic;
> +    }
> +    return conv;
> +
> +err:
> +    qemu_free(conv);
> +    return NULL;
> +}
> +
> +void qemu_pf_conv_run(QemuPfConv *conv, void *dst, void *src, uint32_t cnt)
> +{
> +    conv->convert(conv, dst, src, cnt);
> +}
> +
> +void qemu_pf_conv_put(QemuPfConv *conv)
> +{
> +    if (conv) {
> +        qemu_free(conv->conv_buf);
> +        qemu_free(conv);
> +    }
> +}
> diff --git a/pflib.h b/pflib.h
> new file mode 100644
> index 0000000..b70c313
> --- /dev/null
> +++ b/pflib.h
> @@ -0,0 +1,20 @@
> +#ifndef __QEMU_PFLIB_H
> +#define __QEMU_PFLIB_H
> +
> +/*
> + * PixelFormat conversion library.
> + *
> + * Author: Gerd Hoffmann<kraxel@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +typedef struct QemuPfConv QemuPfConv;
> +
> +QemuPfConv *qemu_pf_conv_get(PixelFormat *dst, PixelFormat *src);
> +void qemu_pf_conv_run(QemuPfConv *conv, void *dst, void *src, uint32_t cnt);
> +void qemu_pf_conv_put(QemuPfConv *conv);
> +
> +#endif
>    

Re: [Qemu-devel] [PATCH 1/9] add pflib: PixelFormat conversion library.

[Gerd Hoffmann]

On 08/19/10 16:04, Anthony Liguori wrote:
> On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>
> This code is more or less stand alone so it's a good candidate for
> having some in-tree unit tests.
>
> I can provide some examples of how it should be done if you like, but
> just something that lives in tests/ that exercises the functionality and
> can be run under valgrind to verify that we aren't leaking memory.

Yea, some example would be great.  Commented tests/example.c maybe?

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 1/9] add pflib: PixelFormat conversion library.

[Anthony Liguori]

On 08/19/2010 10:34 AM, Gerd Hoffmann wrote:
> On 08/19/10 16:04, Anthony Liguori wrote:
>> On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>
>> This code is more or less stand alone so it's a good candidate for
>> having some in-tree unit tests.
>>
>> I can provide some examples of how it should be done if you like, but
>> just something that lives in tests/ that exercises the functionality and
>> can be run under valgrind to verify that we aren't leaking memory.
>
> Yea, some example would be great.  Commented tests/example.c maybe?

This is one of a few that I've written:

http://repo.or.cz/w/qemu/aliguori-queue.git/blob/refs/heads/qpi:/tests/test-buffer.c

You can see the rest of the plumbing in the tree.  The libcheck tests 
are also pretty reasonable although to be honest I dislike libcheck.  I 
don't find it to offer any real value compared to just something that 
succeeds silently or fails upon error.

I'm working on isolating the serial device right now to write tests 
against it but decoupling it from everything else is a fair bit of 
work.  I'd eventually like to get to the point where all new code had 
corresponding in tree unit tests.

Regards,

Anthony Liguori

> thanks,
>   Gerd
>

Re: [Qemu-devel] [PATCH 1/9] add pflib: PixelFormat conversion library.

[Gerd Hoffmann]

   Hi,

> You can see the rest of the plumbing in the tree.

Hmm, "make test" (and trying to build anything in tests/) fails with 
separate source/build dirs.  But it isn't obvious to me why it does. 
The Makefile takes care to set vpath, so I think it should have picked 
up the source files just fine ...

Ideas anyone?

cheers,
   Gerd

[Qemu-devel] [PATCH 2/9] configure: add logging

[Gerd Hoffmann]

Write compile commands and messages to config.log.
Useful for debugging configure.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 configure |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index a20371c..13d8be0 100755
--- a/configure
+++ b/configure
@@ -16,15 +16,18 @@ TMPO="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.o"
 TMPE="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.exe"
 
 trap "rm -f $TMPC $TMPO $TMPE ; exit" EXIT INT QUIT TERM
+rm -f config.log
 
 compile_object() {
-  $cc $QEMU_CFLAGS -c -o $TMPO $TMPC > /dev/null 2> /dev/null
+  echo $cc $QEMU_CFLAGS -c -o $TMPO $TMPC >> config.log
+  $cc $QEMU_CFLAGS -c -o $TMPO $TMPC >> config.log 2>&1
 }
 
 compile_prog() {
   local_cflags="$1"
   local_ldflags="$2"
-  $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags > /dev/null 2> /dev/null
+  echo $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags >> config.log
+  $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags >> config.log 2>&1
 }
 
 # check whether a command is available to this shell (may be either an
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/9] configure: add logging

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Write compile commands and messages to config.log.
> Useful for debugging configure.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>    

Indeed.

Acked-by: Anthony Liguori <aliguori@us.ibm.com>

Given the large nature of spice, I suggest that you setup a subtree for 
it and do a pull request when the first round is ready.

Regards,

Anthony Liguori

> ---
>   configure |    7 +++++--
>   1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index a20371c..13d8be0 100755
> --- a/configure
> +++ b/configure
> @@ -16,15 +16,18 @@ TMPO="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.o"
>   TMPE="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.exe"
>
>   trap "rm -f $TMPC $TMPO $TMPE ; exit" EXIT INT QUIT TERM
> +rm -f config.log
>
>   compile_object() {
> -  $cc $QEMU_CFLAGS -c -o $TMPO $TMPC>  /dev/null 2>  /dev/null
> +  echo $cc $QEMU_CFLAGS -c -o $TMPO $TMPC>>  config.log
> +  $cc $QEMU_CFLAGS -c -o $TMPO $TMPC>>  config.log 2>&1
>   }
>
>   compile_prog() {
>     local_cflags="$1"
>     local_ldflags="$2"
> -  $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags>  /dev/null 2>  /dev/null
> +  echo $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags>>  config.log
> +  $cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $LDFLAGS $local_ldflags>>  config.log 2>&1
>   }
>
>   # check whether a command is available to this shell (may be either an
>    

Re: [Qemu-devel] [PATCH 2/9] configure: add logging

[Gerd Hoffmann]

On 08/19/10 16:05, Anthony Liguori wrote:
> On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
>> Write compile commands and messages to config.log.
>> Useful for debugging configure.
>>
>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>
> Indeed.
>
> Acked-by: Anthony Liguori <aliguori@us.ibm.com>
>
> Given the large nature of spice, I suggest that you setup a subtree for
> it and do a pull request when the first round is ready.

There *is* a git tree (referenced in the cover letter) to pull from. 
Will do that for the next version too ;)

cheers,
   Gerd

[Qemu-devel] [PATCH 3/9] add spice into the configure file

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 configure |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 13d8be0..56e7084 100755
--- a/configure
+++ b/configure
@@ -318,6 +318,7 @@ pkgversion=""
 check_utests="no"
 user_pie="no"
 zero_malloc=""
+spice=""
 
 # OS specific
 if check_define __linux__ ; then
@@ -619,6 +620,10 @@ for opt do
   ;;
   --enable-kvm) kvm="yes"
   ;;
+  --disable-spice) spice="no"
+  ;;
+  --enable-spice) spice="yes"
+  ;;
   --enable-profiler) profiler="yes"
   ;;
   --enable-cocoa)
@@ -898,6 +903,8 @@ echo "  --enable-docs            enable documentation build"
 echo "  --disable-docs           disable documentation build"
 echo "  --disable-vhost-net      disable vhost-net acceleration support"
 echo "  --enable-vhost-net       enable vhost-net acceleration support"
+echo "  --disable-spice          disable spice"
+echo "  --enable-spice           enable spice"
 echo ""
 echo "NOTE: The object files are built at the place where configure is launched"
 exit 1
@@ -2048,6 +2055,30 @@ if compile_prog "" ""; then
     gcc_attribute_warn_unused_result=yes
 fi
 
+# spice probe
+if test "$spice" != "no" ; then
+  cat > $TMPC << EOF
+#include <spice.h>
+int main(void) { spice_server_new(); return 0; }
+EOF
+  spice_proto_ver=$($pkgconfig --modversion spice-protocol 2>/dev/null)
+  spice_server_ver=$($pkgconfig --modversion spice-server 2>/dev/null)
+  spice_cflags=$($pkgconfig --cflags spice-protocol spice-server 2>/dev/null)
+  spice_libs=$($pkgconfig --libs spice-protocol spice-server 2>/dev/null)
+  if compile_prog "$spice_cflags" "$spice_libs" ; then
+    spice="yes"
+    libs_softmmu="$libs_softmmu $spice_libs"
+    QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
+  else
+    if test "$spice" = "yes" ; then
+      feature_not_found "spice"
+    fi
+    spice="no"
+  fi
+fi
+
+##########################################
+
 ##########################################
 # check if we have fdatasync
 
@@ -2190,6 +2221,7 @@ echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
+echo "spice support     $spice"
 
 if test $sdl_too_old = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -2427,6 +2459,10 @@ if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
 
+if test "$spice" = "yes" ; then
+  echo "CONFIG_SPICE=y" >> $config_host_mak
+fi
+
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
   echo "CONFIG_BSD=y" >> $config_host_mak
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/9] add spice into the configure file

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>    

Acked-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   configure |   36 ++++++++++++++++++++++++++++++++++++
>   1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/configure b/configure
> index 13d8be0..56e7084 100755
> --- a/configure
> +++ b/configure
> @@ -318,6 +318,7 @@ pkgversion=""
>   check_utests="no"
>   user_pie="no"
>   zero_malloc=""
> +spice=""
>
>   # OS specific
>   if check_define __linux__ ; then
> @@ -619,6 +620,10 @@ for opt do
>     ;;
>     --enable-kvm) kvm="yes"
>     ;;
> +  --disable-spice) spice="no"
> +  ;;
> +  --enable-spice) spice="yes"
> +  ;;
>     --enable-profiler) profiler="yes"
>     ;;
>     --enable-cocoa)
> @@ -898,6 +903,8 @@ echo "  --enable-docs            enable documentation build"
>   echo "  --disable-docs           disable documentation build"
>   echo "  --disable-vhost-net      disable vhost-net acceleration support"
>   echo "  --enable-vhost-net       enable vhost-net acceleration support"
> +echo "  --disable-spice          disable spice"
> +echo "  --enable-spice           enable spice"
>   echo ""
>   echo "NOTE: The object files are built at the place where configure is launched"
>   exit 1
> @@ -2048,6 +2055,30 @@ if compile_prog "" ""; then
>       gcc_attribute_warn_unused_result=yes
>   fi
>
> +# spice probe
> +if test "$spice" != "no" ; then
> +  cat>  $TMPC<<  EOF
> +#include<spice.h>
> +int main(void) { spice_server_new(); return 0; }
> +EOF
> +  spice_proto_ver=$($pkgconfig --modversion spice-protocol 2>/dev/null)
> +  spice_server_ver=$($pkgconfig --modversion spice-server 2>/dev/null)
> +  spice_cflags=$($pkgconfig --cflags spice-protocol spice-server 2>/dev/null)
> +  spice_libs=$($pkgconfig --libs spice-protocol spice-server 2>/dev/null)
> +  if compile_prog "$spice_cflags" "$spice_libs" ; then
> +    spice="yes"
> +    libs_softmmu="$libs_softmmu $spice_libs"
> +    QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
> +  else
> +    if test "$spice" = "yes" ; then
> +      feature_not_found "spice"
> +    fi
> +    spice="no"
> +  fi
> +fi
> +
> +##########################################
> +
>   ##########################################
>   # check if we have fdatasync
>
> @@ -2190,6 +2221,7 @@ echo "preadv support    $preadv"
>   echo "fdatasync         $fdatasync"
>   echo "uuid support      $uuid"
>   echo "vhost-net support $vhost_net"
> +echo "spice support     $spice"
>
>   if test $sdl_too_old = "yes"; then
>   echo "->  Your SDL version is too old - please upgrade to have SDL support"
> @@ -2427,6 +2459,10 @@ if test "$fdatasync" = "yes" ; then
>     echo "CONFIG_FDATASYNC=y">>  $config_host_mak
>   fi
>
> +if test "$spice" = "yes" ; then
> +  echo "CONFIG_SPICE=y">>  $config_host_mak
> +fi
> +
>   # XXX: suppress that
>   if [ "$bsd" = "yes" ] ; then
>     echo "CONFIG_BSD=y">>  $config_host_mak
>    

[Qemu-devel] [PATCH 4/9] configure: require spice 0.5.3

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 configure |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 56e7084..0998e86 100755
--- a/configure
+++ b/configure
@@ -2061,11 +2061,10 @@ if test "$spice" != "no" ; then
 #include <spice.h>
 int main(void) { spice_server_new(); return 0; }
 EOF
-  spice_proto_ver=$($pkgconfig --modversion spice-protocol 2>/dev/null)
-  spice_server_ver=$($pkgconfig --modversion spice-server 2>/dev/null)
   spice_cflags=$($pkgconfig --cflags spice-protocol spice-server 2>/dev/null)
   spice_libs=$($pkgconfig --libs spice-protocol spice-server 2>/dev/null)
-  if compile_prog "$spice_cflags" "$spice_libs" ; then
+  if $pkgconfig --atleast-version=0.5.3 spice-server &&\
+     compile_prog "$spice_cflags" "$spice_libs" ; then
     spice="yes"
     libs_softmmu="$libs_softmmu $spice_libs"
     QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
-- 
1.7.1

[Qemu-devel] [PATCH 5/9] spice: core bits

[Gerd Hoffmann]

Add -spice command line switch.  Has support setting passwd and port for
now.  With this patch applied the spice client can successfully connect
to qemu.  You can't do anything useful yet though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs   |    2 +
 qemu-config.c   |   23 ++++++++
 qemu-config.h   |    1 +
 qemu-options.hx |    8 +++
 qemu-spice.h    |   22 ++++++++
 spice.c         |  151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c            |   15 ++++++
 7 files changed, 222 insertions(+), 0 deletions(-)
 create mode 100644 qemu-spice.h
 create mode 100644 spice.c

diff --git a/Makefile.objs b/Makefile.objs
index edfca87..021067b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -88,6 +88,8 @@ 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
+
 audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
 audio-obj-$(CONFIG_SDL) += sdlaudio.o
 audio-obj-$(CONFIG_OSS) += ossaudio.o
diff --git a/qemu-config.c b/qemu-config.c
index 95abe61..4cf3b53 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -342,6 +342,26 @@ QemuOptsList qemu_cpudef_opts = {
     },
 };
 
+#ifdef CONFIG_SPICE
+QemuOptsList qemu_spice_opts = {
+    .name = "spice",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
+    .desc = {
+        {
+            .name = "port",
+            .type = QEMU_OPT_NUMBER,
+        },{
+            .name = "password",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "disable-ticketing",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end if list */ }
+    },
+};
+#endif
+
 static QemuOptsList *vm_config_groups[] = {
     &qemu_drive_opts,
     &qemu_chardev_opts,
@@ -352,6 +372,9 @@ static QemuOptsList *vm_config_groups[] = {
     &qemu_global_opts,
     &qemu_mon_opts,
     &qemu_cpudef_opts,
+#ifdef CONFIG_SPICE
+    &qemu_spice_opts,
+#endif
     NULL,
 };
 
diff --git a/qemu-config.h b/qemu-config.h
index dca69d4..3a90213 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -14,6 +14,7 @@ extern QemuOptsList qemu_rtc_opts;
 extern QemuOptsList qemu_global_opts;
 extern QemuOptsList qemu_mon_opts;
 extern QemuOptsList qemu_cpudef_opts;
+extern QemuOptsList qemu_spice_opts;
 
 QemuOptsList *qemu_find_opts(const char *group);
 int qemu_set_option(const char *str);
diff --git a/qemu-options.hx b/qemu-options.hx
index db86feb..c05c219 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -674,6 +674,14 @@ STEXI
 Enable SDL.
 ETEXI
 
+#ifdef CONFIG_SPICE
+DEF("spice", HAS_ARG, QEMU_OPTION_spice,
+    "-spice <args>   use spice\n", QEMU_ARCH_ALL)
+STEXI
+Use Spice.
+ETEXI
+#endif
+
 DEF("portrait", 0, QEMU_OPTION_portrait,
     "-portrait       rotate graphical output 90 deg left (only PXA LCD)\n",
     QEMU_ARCH_ALL)
diff --git a/qemu-spice.h b/qemu-spice.h
new file mode 100644
index 0000000..5597576
--- /dev/null
+++ b/qemu-spice.h
@@ -0,0 +1,22 @@
+#ifndef QEMU_SPICE_H
+#define QEMU_SPICE_H
+
+#ifdef CONFIG_SPICE
+
+#include <spice.h>
+
+#include "qemu-option.h"
+#include "qemu-config.h"
+
+extern SpiceServer *spice_server;
+extern int using_spice;
+
+void qemu_spice_init(void);
+
+#else  /* CONFIG_SPICE */
+
+#define using_spice 0
+
+#endif /* CONFIG_SPICE */
+
+#endif /* QEMU_SPICE_H */
diff --git a/spice.c b/spice.c
new file mode 100644
index 0000000..50fa5ca
--- /dev/null
+++ b/spice.c
@@ -0,0 +1,151 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <spice.h>
+#include <spice-experimental.h>
+
+#include "qemu-common.h"
+#include "qemu-spice.h"
+#include "qemu-timer.h"
+#include "qemu-queue.h"
+#include "monitor.h"
+
+/* core bits */
+
+SpiceServer *spice_server;
+int using_spice = 0;
+
+struct SpiceTimer {
+    QEMUTimer *timer;
+    QTAILQ_ENTRY(SpiceTimer) next;
+};
+static QTAILQ_HEAD(, SpiceTimer) timers = QTAILQ_HEAD_INITIALIZER(timers);
+
+static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
+{
+    SpiceTimer *timer;
+
+    timer = qemu_mallocz(sizeof(*timer));
+    timer->timer = qemu_new_timer(rt_clock, func, opaque);
+    QTAILQ_INSERT_TAIL(&timers, timer, next);
+    return timer;
+}
+
+static void timer_start(SpiceTimer *timer, uint32_t ms)
+{
+    qemu_mod_timer(timer->timer, qemu_get_clock(rt_clock) + ms);
+}
+
+static void timer_cancel(SpiceTimer *timer)
+{
+    qemu_del_timer(timer->timer);
+}
+
+static void timer_remove(SpiceTimer *timer)
+{
+    qemu_del_timer(timer->timer);
+    qemu_free_timer(timer->timer);
+    QTAILQ_REMOVE(&timers, timer, next);
+    free(timer);
+}
+
+struct SpiceWatch {
+    int fd;
+    int event_mask;
+    SpiceWatchFunc func;
+    void *opaque;
+    QTAILQ_ENTRY(SpiceWatch) next;
+};
+static QTAILQ_HEAD(, SpiceWatch) watches = QTAILQ_HEAD_INITIALIZER(watches);
+
+static void watch_read(void *opaque)
+{
+    SpiceWatch *watch = opaque;
+    watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
+}
+
+static void watch_write(void *opaque)
+{
+    SpiceWatch *watch = opaque;
+    watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
+}
+
+static void watch_update_mask(SpiceWatch *watch, int event_mask)
+{
+    IOHandler *on_read = NULL;
+    IOHandler *on_write = NULL;
+
+    watch->event_mask = event_mask;
+    if (watch->event_mask & SPICE_WATCH_EVENT_READ)
+        on_read = watch_read;
+    if (watch->event_mask & SPICE_WATCH_EVENT_WRITE)
+        on_read = watch_write;
+    qemu_set_fd_handler(watch->fd, on_read, on_write, watch);
+}
+
+static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
+{
+    SpiceWatch *watch;
+
+    watch = qemu_mallocz(sizeof(*watch));
+    watch->fd     = fd;
+    watch->func   = func;
+    watch->opaque = opaque;
+    QTAILQ_INSERT_TAIL(&watches, watch, next);
+
+    watch_update_mask(watch, event_mask);
+    return watch;
+}
+
+static void watch_remove(SpiceWatch *watch)
+{
+    watch_update_mask(watch, 0);
+    QTAILQ_REMOVE(&watches, watch, next);
+    qemu_free(watch);
+}
+
+static SpiceCoreInterface core_interface = {
+    .base.type          = SPICE_INTERFACE_CORE,
+    .base.description   = "qemu core services",
+    .base.major_version = SPICE_INTERFACE_CORE_MAJOR,
+    .base.minor_version = SPICE_INTERFACE_CORE_MINOR,
+
+    .timer_add          = timer_add,
+    .timer_start        = timer_start,
+    .timer_cancel       = timer_cancel,
+    .timer_remove       = timer_remove,
+
+    .watch_add          = watch_add,
+    .watch_update_mask  = watch_update_mask,
+    .watch_remove       = watch_remove,
+};
+
+/* functions for the rest of qemu */
+
+void qemu_spice_init(void)
+{
+    QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
+    const char *password;
+    int port;
+
+    if (!opts)
+        return;
+    port = qemu_opt_get_number(opts, "port", 0);
+    if (!port)
+        return;
+    password = qemu_opt_get(opts, "password");
+
+    spice_server = spice_server_new();
+    spice_server_set_port(spice_server, port);
+    if (password)
+        spice_server_set_ticket(spice_server, password, 0, 0, 0);
+    if (qemu_opt_get_bool(opts, "disable-ticketing", 0))
+        spice_server_set_noauth(spice_server);
+
+    /* TODO: make configurable via cmdline */
+    spice_server_set_image_compression(spice_server, SPICE_IMAGE_COMPRESS_AUTO_GLZ);
+
+    spice_server_init(spice_server, &core_interface);
+    using_spice = 1;
+}
diff --git a/vl.c b/vl.c
index b3e3676..7f391c0 100644
--- a/vl.c
+++ b/vl.c
@@ -161,6 +161,8 @@ int main(int argc, char **argv)
 #include "cpus.h"
 #include "arch_init.h"
 
+#include "qemu-spice.h"
+
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
@@ -2600,6 +2602,15 @@ int main(int argc, char **argv, char **envp)
                     }
                     break;
                 }
+#ifdef CONFIG_SPICE
+            case QEMU_OPTION_spice:
+                opts = qemu_opts_parse(&qemu_spice_opts, optarg, 0);
+                if (!opts) {
+                    fprintf(stderr, "parse error: %s\n", optarg);
+                    exit(1);
+                }
+                break;
+#endif
             case QEMU_OPTION_writeconfig:
                 {
                     FILE *fp;
@@ -2868,6 +2879,10 @@ int main(int argc, char **argv, char **envp)
     }
     qemu_add_globals();
 
+#ifdef CONFIG_SPICE
+    qemu_spice_init();
+#endif
+
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 5/9] spice: core bits

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Add -spice command line switch.  Has support setting passwd and port for
> now.  With this patch applied the spice client can successfully connect
> to qemu.  You can't do anything useful yet though.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   Makefile.objs   |    2 +
>   qemu-config.c   |   23 ++++++++
>   qemu-config.h   |    1 +
>   qemu-options.hx |    8 +++
>   qemu-spice.h    |   22 ++++++++
>   spice.c         |  151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   vl.c            |   15 ++++++
>   7 files changed, 222 insertions(+), 0 deletions(-)
>   create mode 100644 qemu-spice.h
>   create mode 100644 spice.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index edfca87..021067b 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -88,6 +88,8 @@ 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
> +
>   audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
>   audio-obj-$(CONFIG_SDL) += sdlaudio.o
>   audio-obj-$(CONFIG_OSS) += ossaudio.o
> diff --git a/qemu-config.c b/qemu-config.c
> index 95abe61..4cf3b53 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -342,6 +342,26 @@ QemuOptsList qemu_cpudef_opts = {
>       },
>   };
>
> +#ifdef CONFIG_SPICE
>    

Now's probably a good time to make vm_config_groups a list that can be 
dynamically added to so that you can register the option group from 
within spice.c.

This would require a new qemu_opts_parse() that took a name of an 
QemuOptsList instead of the pointer to it but then you can return 
failure if spice is unsupported.

The result would be that we wouldn't need an #ifdef here, -spice would 
always be a command line option, but if we didn't support spice, we'd 
return an error if we tried to use it.

> diff --git a/qemu-config.h b/qemu-config.h
> index dca69d4..3a90213 100644
> --- a/qemu-config.h
> +++ b/qemu-config.h
> @@ -14,6 +14,7 @@ extern QemuOptsList qemu_rtc_opts;
>   extern QemuOptsList qemu_global_opts;
>   extern QemuOptsList qemu_mon_opts;
>   extern QemuOptsList qemu_cpudef_opts;
> +extern QemuOptsList qemu_spice_opts;
>
>   QemuOptsList *qemu_find_opts(const char *group);
>   int qemu_set_option(const char *str);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index db86feb..c05c219 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -674,6 +674,14 @@ STEXI
>   Enable SDL.
>   ETEXI
>
> +#ifdef CONFIG_SPICE
> +DEF("spice", HAS_ARG, QEMU_OPTION_spice,
> +    "-spice<args>    use spice\n", QEMU_ARCH_ALL)
> +STEXI
> +Use Spice.
> +ETEXI
> +#endif
> +
>    

As mentioned above, I prefer run time failure instead of build time 
failure.  One reason is that we build documentation based on this file 
and it's awkward to have different documentation based on what's 
available in your build.

For instance, if I don't have libspice installed and I regenerate the 
doc on qemu.org, no one will be able to find out about spice from the help.

BTW, you could have better documentation than 'Use Spice.' :-)

>   DEF("portrait", 0, QEMU_OPTION_portrait,
>       "-portrait       rotate graphical output 90 deg left (only PXA LCD)\n",
>       QEMU_ARCH_ALL)
> diff --git a/qemu-spice.h b/qemu-spice.h
> new file mode 100644
> index 0000000..5597576
> --- /dev/null
> +++ b/qemu-spice.h
> @@ -0,0 +1,22 @@
> +#ifndef QEMU_SPICE_H
> +#define QEMU_SPICE_H
> +
> +#ifdef CONFIG_SPICE
> +
> +#include<spice.h>
> +
> +#include "qemu-option.h"
> +#include "qemu-config.h"
> +
> +extern SpiceServer *spice_server;
> +extern int using_spice;
> +
> +void qemu_spice_init(void);
>    

Could you avoid this with a module_init()?

Please make sure to have copyright/licenses in all files too.

> +
> +#else  /* CONFIG_SPICE */
> +
> +#define using_spice 0
>    

Please make this at least a macro.

> +#endif /* CONFIG_SPICE */
> +
> +#endif /* QEMU_SPICE_H */
> diff --git a/spice.c b/spice.c
> new file mode 100644
> index 0000000..50fa5ca
> --- /dev/null
> +++ b/spice.c
> @@ -0,0 +1,151 @@
>    

Copyrights.

> +#include<stdlib.h>
> +#include<stdio.h>
> +#include<string.h>
>    

This all comes in qemu-common.h

> +#include<spice.h>
> +#include<spice-experimental.h>
> +
> +#include "qemu-common.h"
> +#include "qemu-spice.h"
> +#include "qemu-timer.h"
> +#include "qemu-queue.h"
> +#include "monitor.h"
> +
> +/* core bits */
> +
> +SpiceServer *spice_server;
> +int using_spice = 0;
> +
> +struct SpiceTimer {
> +    QEMUTimer *timer;
> +    QTAILQ_ENTRY(SpiceTimer) next;
> +};
> +static QTAILQ_HEAD(, SpiceTimer) timers = QTAILQ_HEAD_INITIALIZER(timers);
> +
> +static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
> +{
> +    SpiceTimer *timer;
> +
> +    timer = qemu_mallocz(sizeof(*timer));
> +    timer->timer = qemu_new_timer(rt_clock, func, opaque);
> +    QTAILQ_INSERT_TAIL(&timers, timer, next);
> +    return timer;
> +}
> +
> +static void timer_start(SpiceTimer *timer, uint32_t ms)
> +{
> +    qemu_mod_timer(timer->timer, qemu_get_clock(rt_clock) + ms);
> +}
> +
> +static void timer_cancel(SpiceTimer *timer)
> +{
> +    qemu_del_timer(timer->timer);
> +}
> +
> +static void timer_remove(SpiceTimer *timer)
> +{
> +    qemu_del_timer(timer->timer);
> +    qemu_free_timer(timer->timer);
> +    QTAILQ_REMOVE(&timers, timer, next);
> +    free(timer);
> +}
>    
qemu_malloc mixed with free().

This can probably be a non-Spice function too.  I want to propose a new 
Timer wrapper that behaves much like what you have above.  There's a 
couple things I'd suggest to do though.  Let me post a quick patch as a 
follow up.

> +struct SpiceWatch {
> +    int fd;
> +    int event_mask;
> +    SpiceWatchFunc func;
> +    void *opaque;
> +    QTAILQ_ENTRY(SpiceWatch) next;
> +};
> +static QTAILQ_HEAD(, SpiceWatch) watches = QTAILQ_HEAD_INITIALIZER(watches);
> +
> +static void watch_read(void *opaque)
> +{
> +    SpiceWatch *watch = opaque;
> +    watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
> +}
> +
> +static void watch_write(void *opaque)
> +{
> +    SpiceWatch *watch = opaque;
> +    watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
> +}
> +
> +static void watch_update_mask(SpiceWatch *watch, int event_mask)
> +{
> +    IOHandler *on_read = NULL;
> +    IOHandler *on_write = NULL;
> +
> +    watch->event_mask = event_mask;
> +    if (watch->event_mask&  SPICE_WATCH_EVENT_READ)
> +        on_read = watch_read;
> +    if (watch->event_mask&  SPICE_WATCH_EVENT_WRITE)
> +        on_read = watch_write;
> +    qemu_set_fd_handler(watch->fd, on_read, on_write, watch);
> +}
> +
> +static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> +{
> +    SpiceWatch *watch;
> +
> +    watch = qemu_mallocz(sizeof(*watch));
> +    watch->fd     = fd;
> +    watch->func   = func;
> +    watch->opaque = opaque;
> +    QTAILQ_INSERT_TAIL(&watches, watch, next);
> +
> +    watch_update_mask(watch, event_mask);
> +    return watch;
> +}
> +
> +static void watch_remove(SpiceWatch *watch)
> +{
> +    watch_update_mask(watch, 0);
> +    QTAILQ_REMOVE(&watches, watch, next);
> +    qemu_free(watch);
> +}
>    

I understand the value of these wrappers but it should be common code IMHO.

> +static SpiceCoreInterface core_interface = {
> +    .base.type          = SPICE_INTERFACE_CORE,
> +    .base.description   = "qemu core services",
> +    .base.major_version = SPICE_INTERFACE_CORE_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_CORE_MINOR,
> +
> +    .timer_add          = timer_add,
> +    .timer_start        = timer_start,
> +    .timer_cancel       = timer_cancel,
> +    .timer_remove       = timer_remove,
> +
> +    .watch_add          = watch_add,
> +    .watch_update_mask  = watch_update_mask,
> +    .watch_remove       = watch_remove,
> +};
>    

Ah, the interface is dictated by libspice.  Okay, ignore the bit about 
common code.

> +/* functions for the rest of qemu */
> +
> +void qemu_spice_init(void)
> +{
> +    QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> +    const char *password;
> +    int port;
>    

Perhaps qemu_spice_init() should be passed the option list instead of 
relying on the global here.

> +    if (!opts)
> +        return;
> +    port = qemu_opt_get_number(opts, "port", 0);
> +    if (!port)
> +        return;
> +    password = qemu_opt_get(opts, "password");
> +
> +    spice_server = spice_server_new();
> +    spice_server_set_port(spice_server, port);
> +    if (password)
> +        spice_server_set_ticket(spice_server, password, 0, 0, 0);
> +    if (qemu_opt_get_bool(opts, "disable-ticketing", 0))
> +        spice_server_set_noauth(spice_server);
> +
> +    /* TODO: make configurable via cmdline */
> +    spice_server_set_image_compression(spice_server, SPICE_IMAGE_COMPRESS_AUTO_GLZ);
> +
> +    spice_server_init(spice_server,&core_interface);
> +    using_spice = 1;
> +}
> diff --git a/vl.c b/vl.c
> index b3e3676..7f391c0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -161,6 +161,8 @@ int main(int argc, char **argv)
>   #include "cpus.h"
>   #include "arch_init.h"
>
> +#include "qemu-spice.h"
> +
>   //#define DEBUG_NET
>   //#define DEBUG_SLIRP
>
> @@ -2600,6 +2602,15 @@ int main(int argc, char **argv, char **envp)
>                       }
>                       break;
>                   }
> +#ifdef CONFIG_SPICE
> +            case QEMU_OPTION_spice:
> +                opts = qemu_opts_parse(&qemu_spice_opts, optarg, 0);
> +                if (!opts) {
> +                    fprintf(stderr, "parse error: %s\n", optarg);
> +                    exit(1);
> +                }
> +                break;
> +#endif
>               case QEMU_OPTION_writeconfig:
>                   {
>                       FILE *fp;
> @@ -2868,6 +2879,10 @@ int main(int argc, char **argv, char **envp)
>       }
>       qemu_add_globals();
>
> +#ifdef CONFIG_SPICE
> +    qemu_spice_init();
> +#endif
>    

I think there's two paths we can take here.  We can either use 
module_init() with a new class for this location or we can be more 
explicit and pass the option list here.

I think I'd be happy with either one.  In any event, I'd like to avoid 
an #ifdef here.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 5/9] spice: core bits

[Gerd Hoffmann]

>> +#ifdef CONFIG_SPICE
>
> Now's probably a good time to make vm_config_groups a list that can be
> dynamically added to so that you can register the option group from
> within spice.c.
>
> This would require a new qemu_opts_parse() that took a name of an
> QemuOptsList instead of the pointer to it but then you can return
> failure if spice is unsupported.

Ah, you want me fix virtfs too ;)

Patches are on the list.  Well, at least a start.  For further cleanups 
we'll need some more *_init() magic for the non-devices stuff.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 5/9] spice: core bits

[Gerd Hoffmann]

   Hi,

> I think there's two paths we can take here. We can either use
> module_init() with a new class for this location or we can be more
> explicit and pass the option list here.

Actually the device_init() hook does just fine here.  Not sure this is a 
good idea to hook there though as this is meant for guest device 
backends and it could be moved at some point in the future.

I basically need a hook to do host-side setup after all config 
processing is done and before guest devices are created.

cheers,
   Gerd

[Qemu-devel] [PATCH 6/9] spice: add keyboard

[Gerd Hoffmann]

Open keyboard channel.  Now you can type into the spice client and the
keyboard events are sent to your guest.  You'll need some other display
like vnc to actually see the guest responding to them though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs |    2 +-
 qemu-spice.h  |    1 +
 spice-input.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 spice.c       |    2 ++
 4 files changed, 61 insertions(+), 1 deletions(-)
 create mode 100644 spice-input.c

diff --git a/Makefile.objs b/Makefile.objs
index 021067b..6ddb373 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
+common-obj-$(CONFIG_SPICE) += spice.o spice-input.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 5597576..ceb3db2 100644
--- a/qemu-spice.h
+++ b/qemu-spice.h
@@ -12,6 +12,7 @@ extern SpiceServer *spice_server;
 extern int using_spice;
 
 void qemu_spice_init(void);
+void qemu_spice_input_init(void);
 
 #else  /* CONFIG_SPICE */
 
diff --git a/spice-input.c b/spice-input.c
new file mode 100644
index 0000000..e1014d7
--- /dev/null
+++ b/spice-input.c
@@ -0,0 +1,57 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <spice.h>
+
+#include "qemu-common.h"
+#include "qemu-spice.h"
+#include "console.h"
+
+/* keyboard bits */
+
+typedef struct QemuSpiceKbd {
+    SpiceKbdInstance sin;
+    int ledstate;
+} QemuSpiceKbd;
+
+static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag);
+static uint8_t kbd_get_leds(SpiceKbdInstance *sin);
+static void kbd_leds(void *opaque, int l);
+
+static const SpiceKbdInterface kbd_interface = {
+    .base.type          = SPICE_INTERFACE_KEYBOARD,
+    .base.description   = "qemu keyboard",
+    .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
+    .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
+    .push_scan_freg     = kbd_push_key,
+    .get_leds           = kbd_get_leds,
+};
+
+static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
+{
+    kbd_put_keycode(frag);
+}
+
+static uint8_t kbd_get_leds(SpiceKbdInstance *sin)
+{
+    QemuSpiceKbd *kbd = container_of(sin, QemuSpiceKbd, sin);
+    return kbd->ledstate;
+}
+
+static void kbd_leds(void *opaque, int ledstate)
+{
+    QemuSpiceKbd *kbd = opaque;
+    kbd->ledstate = ledstate;
+    spice_server_kbd_leds(&kbd->sin, ledstate);
+}
+
+void qemu_spice_input_init(void)
+{
+    QemuSpiceKbd *kbd;
+
+    kbd = qemu_mallocz(sizeof(*kbd));
+    kbd->sin.base.sif = &kbd_interface.base;
+    spice_server_add_interface(spice_server, &kbd->sin.base);
+    qemu_add_led_event_handler(kbd_leds, kbd);
+}
diff --git a/spice.c b/spice.c
index 50fa5ca..c763d52 100644
--- a/spice.c
+++ b/spice.c
@@ -148,4 +148,6 @@ void qemu_spice_init(void)
 
     spice_server_init(spice_server, &core_interface);
     using_spice = 1;
+
+    qemu_spice_input_init();
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Open keyboard channel.  Now you can type into the spice client and the
> keyboard events are sent to your guest.  You'll need some other display
> like vnc to actually see the guest responding to them though.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   Makefile.objs |    2 +-
>   qemu-spice.h  |    1 +
>   spice-input.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   spice.c       |    2 ++
>   4 files changed, 61 insertions(+), 1 deletions(-)
>   create mode 100644 spice-input.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 021067b..6ddb373 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
> +common-obj-$(CONFIG_SPICE) += spice.o spice-input.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 5597576..ceb3db2 100644
> --- a/qemu-spice.h
> +++ b/qemu-spice.h
> @@ -12,6 +12,7 @@ extern SpiceServer *spice_server;
>   extern int using_spice;
>
>   void qemu_spice_init(void);
> +void qemu_spice_input_init(void);
>
>   #else  /* CONFIG_SPICE */
>
> diff --git a/spice-input.c b/spice-input.c
> new file mode 100644
> index 0000000..e1014d7
> --- /dev/null
> +++ b/spice-input.c
> @@ -0,0 +1,57 @@
> +#include<stdlib.h>
> +#include<stdio.h>
> +#include<string.h>
>    

copyright and extra headers.

> +
> +#include<spice.h>
> +
> +#include "qemu-common.h"
> +#include "qemu-spice.h"
> +#include "console.h"
> +
> +/* keyboard bits */
> +
> +typedef struct QemuSpiceKbd {
> +    SpiceKbdInstance sin;
> +    int ledstate;
> +} QemuSpiceKbd;
> +
> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag);
> +static uint8_t kbd_get_leds(SpiceKbdInstance *sin);
> +static void kbd_leds(void *opaque, int l);
> +
> +static const SpiceKbdInterface kbd_interface = {
> +    .base.type          = SPICE_INTERFACE_KEYBOARD,
> +    .base.description   = "qemu keyboard",
> +    .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
> +    .push_scan_freg     = kbd_push_key,
> +    .get_leds           = kbd_get_leds,
> +};
> +
> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
> +{
> +    kbd_put_keycode(frag);
> +}
>    

Instead of using this interface which includes multi-byte sequences, it 
would probably make sense to use the same compressed encoding that VNC 
uses and introduce a new function that takes this encoding type.  The 
advantage is that one call == one keycode so it's far less prone to 
goofiness.

> +
> +static uint8_t kbd_get_leds(SpiceKbdInstance *sin)
> +{
> +    QemuSpiceKbd *kbd = container_of(sin, QemuSpiceKbd, sin);
> +    return kbd->ledstate;
> +}
> +
> +static void kbd_leds(void *opaque, int ledstate)
> +{
> +    QemuSpiceKbd *kbd = opaque;
> +    kbd->ledstate = ledstate;
> +    spice_server_kbd_leds(&kbd->sin, ledstate);
> +}
>    

It's probably more robust in the long term to explicitly convert the 
ledstate to from the QEMU format to the libspice format even if they 
both happen to be the same.

Regards,

Anthony Liguori

> +void qemu_spice_input_init(void)
> +{
> +    QemuSpiceKbd *kbd;
> +
> +    kbd = qemu_mallocz(sizeof(*kbd));
> +    kbd->sin.base.sif =&kbd_interface.base;
> +    spice_server_add_interface(spice_server,&kbd->sin.base);
> +    qemu_add_led_event_handler(kbd_leds, kbd);
> +}
> diff --git a/spice.c b/spice.c
> index 50fa5ca..c763d52 100644
> --- a/spice.c
> +++ b/spice.c
> @@ -148,4 +148,6 @@ void qemu_spice_init(void)
>
>       spice_server_init(spice_server,&core_interface);
>       using_spice = 1;
> +
> +    qemu_spice_input_init();
>   }
>    

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Gerd Hoffmann]

>> +static const SpiceKbdInterface kbd_interface = {
>> + .base.type = SPICE_INTERFACE_KEYBOARD,
>> + .base.description = "qemu keyboard",
>> + .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
>> + .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
>> + .push_scan_freg = kbd_push_key,
>> + .get_leds = kbd_get_leds,
>> +};
>> +
>> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
>> +{
>> + kbd_put_keycode(frag);
>> +}
>
> Instead of using this interface which includes multi-byte sequences, it
> would probably make sense to use the same compressed encoding that VNC
> uses and introduce a new function that takes this encoding type. The
> advantage is that one call == one keycode so it's far less prone to
> goofiness.

Hmm?  Not sure what you want here ...

kbd_push_key is called by libspice which feeds us with a ps/2 data 
stream for the keyboard.  Yes, one of those x86-isms in spice.  Yes, the 
ps/2 data stream also travels over the wire, i.e. the spice client has 
to hop through loops to convert whatever it gets into a ps/2 data stream 
for us.

Luckily ps/2 data is exactly what qemu expects to get via 
kbd_put_keycode, so I can simply pass on what I get, and it is IMHO 
pretty pointless to do something else ...

> It's probably more robust in the long term to explicitly convert the
> ledstate to from the QEMU format to the libspice format even if they
> both happen to be the same.

Ok.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Anthony Liguori]

On 08/20/2010 07:34 AM, Gerd Hoffmann wrote:
>
>>> +static const SpiceKbdInterface kbd_interface = {
>>> + .base.type = SPICE_INTERFACE_KEYBOARD,
>>> + .base.description = "qemu keyboard",
>>> + .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
>>> + .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
>>> + .push_scan_freg = kbd_push_key,
>>> + .get_leds = kbd_get_leds,
>>> +};
>>> +
>>> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
>>> +{
>>> + kbd_put_keycode(frag);
>>> +}
>>
>> Instead of using this interface which includes multi-byte sequences, it
>> would probably make sense to use the same compressed encoding that VNC
>> uses and introduce a new function that takes this encoding type. The
>> advantage is that one call == one keycode so it's far less prone to
>> goofiness.
>
> Hmm?  Not sure what you want here ...
>
> kbd_push_key is called by libspice which feeds us with a ps/2 data 
> stream for the keyboard.  Yes, one of those x86-isms in spice.  Yes, 
> the ps/2 data stream also travels over the wire, i.e. the spice client 
> has to hop through loops to convert whatever it gets into a ps/2 data 
> stream for us.
>
> Luckily ps/2 data is exactly what qemu expects to get via 
> kbd_put_keycode, so I can simply pass on what I get, and it is IMHO 
> pretty pointless to do something else ...

It's not actually ps/2 data.  It's AT scan codes plus an internal 
encoding to indicate press vs. release using the high bit.  
Additionally, some special keys are encoded with two calls to 
kbd_put_keycode using the 0xe0 prefix (the grey code).

That gets converted to ps/2 data by decoding the high bit and generating 
a special PS/2 keyboard sequence if necessary.  The grey sequence just 
happens to work because the logic in ps2_put_keycode is simple.  IOW, 
the ps/2 protocol looks like:

[0xe0,][0xf0,]raw_keycode

0xe0 indicates that the keycode is a "grey" code and 0xf0 indicates it's 
a key release event.  raw_keycode is a PS/2 raw keycode that has a 1-1 
mapping from AT scan codes.

To generate the possible sequences, we would do:

// normal key press; PS/2: at2raw(at_keycode)
kbd_put_keycode(at_keycode);

// grey key press; PS/2 0xe0, at2raw(at_keycode)
kbd_put_keycode(0xe0);
kbd_put_keycode(at_keycode);

// normal key release; PS/2 0xf0, at2raw(at_keycode)
kbd_put_keycode(at_keycode | 0x80);

// grey key release; PS/2 0xe0, 0xf0, at2raw(at_keycode)
kbd_put_keycode(0xe0);
kbd_put_keycode(at_keycode | 0x80);

In the VNC encoding, instead of using the high bit to represent the key 
down vs. key up event, we use separate messages for key down vs. key 
up.  We then use the high bit to encode whether the grey code is needed 
which let's us send keycodes in a single message with a fixed keycode 
size of 8 bits.

So what I'm proposing is that we modify kbd_put_keycode to also reflect 
this:

// normal keys
kbd_keycode_press(at_keycode);      // PS/2 at2raw(at_keycode)
kbd_keycode_release(at_keycode);   // PS/2 0xf0, at2raw(at_keycode)

// grey keys; PS/2 0xe0, at2raw(at_keycode)
kbd_keycode_press(0x80 | at_keycode);      // PS/2 0xe0, 0xf0, 
at2raw(at_keycode)
kbd_keycode_release(0x80 | at_keycode);   // PS/2 0xe0, 0xf0, 
at2raw(at_keycode)

If it's not already too late, I'd suggest making this the Spice protocol 
interface.  The current kbd_put_keycode interface is just a QEMU 
implementation detail and having different size byte sequences is a very 
awkward interface.

Regards,

Anthony Liguori

>> It's probably more robust in the long term to explicitly convert the
>> ledstate to from the QEMU format to the libspice format even if they
>> both happen to be the same.
>
> Ok.
>
> cheers,
>   Gerd
>

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Gerd Hoffmann]

   Hi,

> It's not actually ps/2 data. It's AT scan codes plus an internal
> encoding to indicate press vs. release using the high bit. Additionally,
> some special keys are encoded with two calls to kbd_put_keycode using
> the 0xe0 prefix (the grey code).

Wheee.  From a brief look at the code it seems this *is* the spice wire 
protocol.  One more place where spice uses knowledge about qemu 
internals.  Unfortunaly this one escaped my attention until now, so it 
didn't got fixed :-(

> So what I'm proposing is that we modify kbd_put_keycode to also reflect
> this:
>
> // normal keys
> kbd_keycode_press(at_keycode); // PS/2 at2raw(at_keycode)
> kbd_keycode_release(at_keycode); // PS/2 0xf0, at2raw(at_keycode)
>
> // grey keys; PS/2 0xe0, at2raw(at_keycode)
> kbd_keycode_press(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> at2raw(at_keycode)
> kbd_keycode_release(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> at2raw(at_keycode)
>
> If it's not already too late, I'd suggest making this the Spice protocol
> interface.

No.  I think for now I have to deal with the mess in case qemu decides 
to change the internal interface.  And when ever touching the spice wire 
protocol to fixup this mess I will *not* use AT keycodes.  Handling 
anything with extra internet / multimedia / whatever keys in a sane way 
is simply impossible with AT keycodes.  linux input layer key codes 
should do.  maybe usb hid is usable too, need to check.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Daniel P. Berrange]

On Fri, Aug 20, 2010 at 03:56:52PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >It's not actually ps/2 data. It's AT scan codes plus an internal
> >encoding to indicate press vs. release using the high bit. Additionally,
> >some special keys are encoded with two calls to kbd_put_keycode using
> >the 0xe0 prefix (the grey code).
> 
> Wheee.  From a brief look at the code it seems this *is* the spice wire 
> protocol.  One more place where spice uses knowledge about qemu 
> internals.  Unfortunaly this one escaped my attention until now, so it 
> didn't got fixed :-(
> 
> >So what I'm proposing is that we modify kbd_put_keycode to also reflect
> >this:
> >
> >// normal keys
> >kbd_keycode_press(at_keycode); // PS/2 at2raw(at_keycode)
> >kbd_keycode_release(at_keycode); // PS/2 0xf0, at2raw(at_keycode)
> >
> >// grey keys; PS/2 0xe0, at2raw(at_keycode)
> >kbd_keycode_press(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> >at2raw(at_keycode)
> >kbd_keycode_release(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> >at2raw(at_keycode)
> >
> >If it's not already too late, I'd suggest making this the Spice protocol
> >interface.
> 
> No.  I think for now I have to deal with the mess in case qemu decides 
> to change the internal interface.  And when ever touching the spice wire 
> protocol to fixup this mess I will *not* use AT keycodes.  Handling 
> anything with extra internet / multimedia / whatever keys in a sane way 
> is simply impossible with AT keycodes.  linux input layer key codes 
> should do.  maybe usb hid is usable too, need to check.

AT (well XT) keycodes aren't that bad a choice, at least if you go for the
extended mapping used by the Linux keyboard driver. This supports pretty
much all of the internet/multimedia keys, AFAICT, more than USB hid
does (at least in the Linux USB HID driver). In order to properly support
the VNC keycode extension with GTK-VNC under  Xorg + Win32, OS-X and Linux,
as well as native OS-X & Win32, I've created a giant CSV mapping file for
all keycode sets that I've encountered so far.

The master mapping is from Linux keycodes to other sets:

  http://git.gnome.org/browse/gtk-vnc/tree/src/keymaps.csv

And a tool that can then create you C arrays for mapping between
arbitrary keycode sets in any direction (potentially lossy
of course, depending on choice of keycode sets):

  http://git.gnome.org/browse/gtk-vnc/tree/src/keymap-gen.pl

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Gerd Hoffmann]

   Hi,

> AT (well XT) keycodes aren't that bad a choice, at least if you go for the
> extended mapping used by the Linux keyboard driver.

Hmm, as far I know those extended mappings are not standardized.  Uses 
linux this just as internal representation?  Or can you actually feed a 
linux guest with them and expect it to work?  How about non-linux guests?

When using it as wire protocol guest compatibility doesn't matter that 
much though ...

> The master mapping is from Linux keycodes to other sets:
>
>    http://git.gnome.org/browse/gtk-vnc/tree/src/keymaps.csv
>
> And a tool that can then create you C arrays for mapping between
> arbitrary keycode sets in any direction (potentially lossy
> of course, depending on choice of keycode sets):
>
>    http://git.gnome.org/browse/gtk-vnc/tree/src/keymap-gen.pl

Nice.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Daniel P. Berrange]

On Fri, Aug 20, 2010 at 04:49:20PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >AT (well XT) keycodes aren't that bad a choice, at least if you go for the
> >extended mapping used by the Linux keyboard driver.
> 
> Hmm, as far I know those extended mappings are not standardized.  Uses 
> linux this just as internal representation?  Or can you actually feed a 
> linux guest with them and expect it to work?  How about non-linux guests?

Internally Linux uses Linux keycodes throughout. The keyboard driver
(drivers/char/keyboard.c) has a table that maps Linux keycodes to XT 
keycodes when running in RAW mode that has entries for many of the
internet/multimedia keys. 

IIUC, Microsoft sets out standard XT mappings for internet, multimedia keys,
etc as part of WHQL tests for keyboards. There is a doc hiding somewhere
on the web that details these, but its location escapes me currently. 
I've not  checked whether the extended mappings used by the Linux 
keyboard driver in RAW mode, because it wasn't relevant for my immediate
needs - I just needed to know what the mapping was, not who standardized
it :-)

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] [PATCH 7/9] spice: add mouse

[Gerd Hoffmann]

Open mouse channel.  Now you can move the guests mouse pointer.
No tablet / absolute positioning (yet) though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 spice-input.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/spice-input.c b/spice-input.c
index e1014d7..8f3deb4 100644
--- a/spice-input.c
+++ b/spice-input.c
@@ -46,12 +46,43 @@ static void kbd_leds(void *opaque, int ledstate)
     spice_server_kbd_leds(&kbd->sin, ledstate);
 }
 
+/* mouse bits */
+
+typedef struct QemuSpiceMouse {
+    SpiceMouseInstance sin;
+} QemuSpiceMouse;
+
+static void mouse_motion(SpiceMouseInstance *sin, int dx, int dy, int dz,
+                         uint32_t buttons_state)
+{
+    kbd_mouse_event(dx, dy, dz, buttons_state);
+}
+
+static void mouse_buttons(SpiceMouseInstance *sin, uint32_t buttons_state)
+{
+    kbd_mouse_event(0, 0, 0, buttons_state);
+}
+
+static const SpiceMouseInterface mouse_interface = {
+    .base.type          = SPICE_INTERFACE_MOUSE,
+    .base.description   = "mouse",
+    .base.major_version = SPICE_INTERFACE_MOUSE_MAJOR,
+    .base.minor_version = SPICE_INTERFACE_MOUSE_MINOR,
+    .motion             = mouse_motion,
+    .buttons            = mouse_buttons,
+};
+
 void qemu_spice_input_init(void)
 {
     QemuSpiceKbd *kbd;
+    QemuSpiceMouse *mouse;
 
     kbd = qemu_mallocz(sizeof(*kbd));
     kbd->sin.base.sif = &kbd_interface.base;
     spice_server_add_interface(spice_server, &kbd->sin.base);
     qemu_add_led_event_handler(kbd_leds, kbd);
+
+    mouse = qemu_mallocz(sizeof(*mouse));
+    mouse->sin.base.sif = &mouse_interface.base;
+    spice_server_add_interface(spice_server, &mouse->sin.base);
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 7/9] spice: add mouse

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Open mouse channel.  Now you can move the guests mouse pointer.
> No tablet / absolute positioning (yet) though.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   spice-input.c |   31 +++++++++++++++++++++++++++++++
>   1 files changed, 31 insertions(+), 0 deletions(-)
>
> diff --git a/spice-input.c b/spice-input.c
> index e1014d7..8f3deb4 100644
> --- a/spice-input.c
> +++ b/spice-input.c
> @@ -46,12 +46,43 @@ static void kbd_leds(void *opaque, int ledstate)
>       spice_server_kbd_leds(&kbd->sin, ledstate);
>   }
>
> +/* mouse bits */
> +
> +typedef struct QemuSpiceMouse {
> +    SpiceMouseInstance sin;
> +} QemuSpiceMouse;
> +
> +static void mouse_motion(SpiceMouseInstance *sin, int dx, int dy, int dz,
> +                         uint32_t buttons_state)
> +{
> +    kbd_mouse_event(dx, dy, dz, buttons_state);
>    

dz is an odd interface.  We use it to represent additional buttons which 
really makes no sense.  If you still can, I'd suggest moving dz into 
buttons_state.

Previous comment still applies though, you should explicitly convert the 
button_states from QEMU format to Spice format to future proof.

Regards,

Anthony Liguori

> +}
> +
> +static void mouse_buttons(SpiceMouseInstance *sin, uint32_t buttons_state)
> +{
> +    kbd_mouse_event(0, 0, 0, buttons_state);
> +}
> +
> +static const SpiceMouseInterface mouse_interface = {
> +    .base.type          = SPICE_INTERFACE_MOUSE,
> +    .base.description   = "mouse",
> +    .base.major_version = SPICE_INTERFACE_MOUSE_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_MOUSE_MINOR,
> +    .motion             = mouse_motion,
> +    .buttons            = mouse_buttons,
> +};
> +
>   void qemu_spice_input_init(void)
>   {
>       QemuSpiceKbd *kbd;
> +    QemuSpiceMouse *mouse;
>
>       kbd = qemu_mallocz(sizeof(*kbd));
>       kbd->sin.base.sif =&kbd_interface.base;
>       spice_server_add_interface(spice_server,&kbd->sin.base);
>       qemu_add_led_event_handler(kbd_leds, kbd);
> +
> +    mouse = qemu_mallocz(sizeof(*mouse));
> +    mouse->sin.base.sif =&mouse_interface.base;
> +    spice_server_add_interface(spice_server,&mouse->sin.base);
>   }
>    

Re: [Qemu-devel] [PATCH 7/9] spice: add mouse

[Gerd Hoffmann]

   Hi,

>> +static void mouse_motion(SpiceMouseInstance *sin, int dx, int dy, int
>> dz,
>> + uint32_t buttons_state)
>> +{
>> + kbd_mouse_event(dx, dy, dz, buttons_state);
>
> dz is an odd interface. We use it to represent additional buttons which
> really makes no sense. If you still can, I'd suggest moving dz into
> buttons_state.

Again this is libspice interface (and I think also wire protocol), so I 
can't change it.  I can convert dz into some button_mask bits before 
calling kbd_mouse_event, but looking at the vnc code it seems qemu 
expects the mouse wheel events being passed via dz not button_state.

> Previous comment still applies though, you should explicitly convert the
> button_states from QEMU format to Spice format to future proof.

Ok.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 7/9] spice: add mouse

[Anthony Liguori]

On 08/20/2010 07:42 AM, Gerd Hoffmann wrote:
>   Hi,
>
>>> +static void mouse_motion(SpiceMouseInstance *sin, int dx, int dy, int
>>> dz,
>>> + uint32_t buttons_state)
>>> +{
>>> + kbd_mouse_event(dx, dy, dz, buttons_state);
>>
>> dz is an odd interface. We use it to represent additional buttons which
>> really makes no sense. If you still can, I'd suggest moving dz into
>> buttons_state.
>
> Again this is libspice interface (and I think also wire protocol), so 
> I can't change it.  I can convert dz into some button_mask bits before 
> calling kbd_mouse_event, but looking at the vnc code it seems qemu 
> expects the mouse wheel events being passed via dz not button_state.

That's unfortunate but understood.  Is Spice considered a stable 
API/wire protocol at this point?

Regards,

Anthony Liguori

>
>> Previous comment still applies though, you should explicitly convert the
>> button_states from QEMU format to Spice format to future proof.
>
> Ok.
>
> cheers,
>   Gerd
>

Re: [Qemu-devel] [PATCH 7/9] spice: add mouse

[Gerd Hoffmann]

   Hi,

>> Again this is libspice interface (and I think also wire protocol), so
>> I can't change it. I can convert dz into some button_mask bits before
>> calling kbd_mouse_event, but looking at the vnc code it seems qemu
>> expects the mouse wheel events being passed via dz not button_state.
>
> That's unfortunate but understood. Is Spice considered a stable API/wire
> protocol at this point?

Yes.  There are version numbers though, so we can add new messages and 
use them in case both server and client are new enough.  Unfortunaly 
this makes the keyboard f*ckup only slightly less messy as we'll have to 
support both old+new way for compatibility reasons.  Oh well.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 7/9] spice: add mouse

[Anthony Liguori]

On 08/20/2010 09:03 AM, Gerd Hoffmann wrote:
>   Hi,
>
>>> Again this is libspice interface (and I think also wire protocol), so
>>> I can't change it. I can convert dz into some button_mask bits before
>>> calling kbd_mouse_event, but looking at the vnc code it seems qemu
>>> expects the mouse wheel events being passed via dz not button_state.
>>
>> That's unfortunate but understood. Is Spice considered a stable API/wire
>> protocol at this point?
>
> Yes.  There are version numbers though, so we can add new messages and 
> use them in case both server and client are new enough.  Unfortunaly 
> this makes the keyboard f*ckup only slightly less messy as we'll have 
> to support both old+new way for compatibility reasons.  Oh well.

One of the things to consider is that we only emulate a 2-axis 5 button 
mouse.  Buttons one through three are bits in button_state and buttons 
4/5 are represented through dz.  My mouse IRL is actually a 9 button mouse.

We encode buttons 4/5 as a -1, 0, 1 in dz which corresponds to 10, 00, 
and 11.  We cannot represent 11 with dz which corresponds to having 
buttons 4 and 5 pressed simultaneously.  Most mice have a scroll wheel 
for 4/5 but there's no strict requirement that that's the case.

So in terms of building a long term stable wire protocol, you'd be much 
better off using button_mask to encode buttons 4/5 because it's just a 
matter of time before we drop the dz hack in qemu.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

[Qemu-devel] [PATCH 8/9] spice: simple display

[Gerd Hoffmann]

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>
+#include <pthread.h>
+
+#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);
+    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);
+
+    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;
+}
+
+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__);
+
+    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
     dpy_resize(ds);
 
     dcl = ds->listeners;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Anthony Liguori]

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;
>    

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[malc]

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..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[malc]

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..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Anthony Liguori]

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..]
>
>    

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[malc]

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..]
> > 
> >    

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Gerd Hoffmann]

> 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

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Gerd Hoffmann]

   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

Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Anthony Liguori]

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
>

[Qemu-devel] [PATCH 9/9] spice: add tablet support

[Gerd Hoffmann]

Add support for the spice tablet interface.  The tablet interface will
be registered (and then used by the spice client) as soon as a absolute
pointing device is available and used by the guest, i.e. you'll have to
configure your guest with '-usbdevice tablet'.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 spice-display.c |    2 +-
 spice-input.c   |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/spice-display.c b/spice-display.c
index 1e6d939..fec2432 100644
--- a/spice-display.c
+++ b/spice-display.c
@@ -143,7 +143,7 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
     surface.width      = ds_get_width(ssd->ds);
     surface.height     = ds_get_height(ssd->ds);
     surface.stride     = -surface.width * 4;
-    surface.mouse_mode = 0;
+    surface.mouse_mode = true;
     surface.flags      = 0;
     surface.type       = 0;
     surface.mem        = (intptr_t)ssd->buf;
diff --git a/spice-input.c b/spice-input.c
index 8f3deb4..5646ff9 100644
--- a/spice-input.c
+++ b/spice-input.c
@@ -1,5 +1,6 @@
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdbool.h>
 #include <string.h>
 
 #include <spice.h>
@@ -48,9 +49,13 @@ static void kbd_leds(void *opaque, int ledstate)
 
 /* mouse bits */
 
-typedef struct QemuSpiceMouse {
-    SpiceMouseInstance sin;
-} QemuSpiceMouse;
+typedef struct QemuSpicePointer {
+    SpiceMouseInstance  mouse;
+    SpiceTabletInstance tablet;
+    int width, height, x, y;
+    Notifier mouse_mode;
+    bool absolute;
+} QemuSpicePointer;
 
 static void mouse_motion(SpiceMouseInstance *sin, int dx, int dy, int dz,
                          uint32_t buttons_state)
@@ -72,17 +77,97 @@ static const SpiceMouseInterface mouse_interface = {
     .buttons            = mouse_buttons,
 };
 
+static void tablet_set_logical_size(SpiceTabletInstance* sin, int width, int height)
+{
+    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
+
+    fprintf(stderr, "%s: %dx%d\n", __FUNCTION__, width, height);
+    if (height < 16)
+        height = 16;
+    if (width < 16)
+        width = 16;
+    pointer->width  = width;
+    pointer->height = height;
+}
+
+static void tablet_position(SpiceTabletInstance* sin, int x, int y,
+                            uint32_t buttons_state)
+{
+    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
+
+    pointer->x = x * 0x7FFF / (pointer->width - 1);
+    pointer->y = y * 0x7FFF / (pointer->height - 1);
+    kbd_mouse_event(pointer->x, pointer->y, 0, buttons_state);
+}
+
+
+static void tablet_wheel(SpiceTabletInstance* sin, int wheel,
+                         uint32_t buttons_state)
+{
+    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
+
+    kbd_mouse_event(pointer->x, pointer->y, wheel, buttons_state);
+}
+
+static void tablet_buttons(SpiceTabletInstance *sin,
+                           uint32_t buttons_state)
+{
+    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
+
+    kbd_mouse_event(pointer->x, pointer->y, 0, buttons_state);
+}
+
+static const SpiceTabletInterface tablet_interface = {
+    .base.type          = SPICE_INTERFACE_TABLET,
+    .base.description   = "tablet",
+    .base.major_version = SPICE_INTERFACE_TABLET_MAJOR,
+    .base.minor_version = SPICE_INTERFACE_TABLET_MINOR,
+    .set_logical_size   = tablet_set_logical_size,
+    .position           = tablet_position,
+    .wheel              = tablet_wheel,
+    .buttons            = tablet_buttons,
+};
+
+static void mouse_mode_notifier(Notifier *notifier)
+{
+    QemuSpicePointer *pointer = container_of(notifier, QemuSpicePointer, mouse_mode);
+    bool is_absolute  = kbd_mouse_is_absolute();
+    bool has_absolute = kbd_mouse_has_absolute();
+
+    fprintf(stderr, "%s: absolute pointer: %s%s\n", __FUNCTION__,
+            has_absolute ? "present" : "not available",
+            is_absolute  ? "+active" : "");
+
+    if (pointer->absolute == is_absolute)
+        return;
+
+    if (is_absolute) {
+        fprintf(stderr, "%s: using absolute pointer (client mode)\n", __FUNCTION__);
+        spice_server_add_interface(spice_server, &pointer->tablet.base);
+    } else {
+        fprintf(stderr, "%s: using relative pointer (server mode)\n", __FUNCTION__);
+        spice_server_remove_interface(&pointer->tablet.base);
+    }
+    pointer->absolute = is_absolute;
+}
+
 void qemu_spice_input_init(void)
 {
     QemuSpiceKbd *kbd;
-    QemuSpiceMouse *mouse;
+    QemuSpicePointer *pointer;
 
     kbd = qemu_mallocz(sizeof(*kbd));
     kbd->sin.base.sif = &kbd_interface.base;
     spice_server_add_interface(spice_server, &kbd->sin.base);
     qemu_add_led_event_handler(kbd_leds, kbd);
 
-    mouse = qemu_mallocz(sizeof(*mouse));
-    mouse->sin.base.sif = &mouse_interface.base;
-    spice_server_add_interface(spice_server, &mouse->sin.base);
+    pointer = qemu_mallocz(sizeof(*pointer));
+    pointer->mouse.base.sif  = &mouse_interface.base;
+    pointer->tablet.base.sif = &tablet_interface.base;
+    spice_server_add_interface(spice_server, &pointer->mouse.base);
+
+    pointer->absolute = false;
+    pointer->mouse_mode.notify = mouse_mode_notifier;
+    qemu_add_mouse_mode_change_notifier(&pointer->mouse_mode);
+    mouse_mode_notifier(&pointer->mouse_mode);
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 9/9] spice: add tablet support

[Anthony Liguori]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Add support for the spice tablet interface.  The tablet interface will
> be registered (and then used by the spice client) as soon as a absolute
> pointing device is available and used by the guest, i.e. you'll have to
> configure your guest with '-usbdevice tablet'.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   spice-display.c |    2 +-
>   spice-input.c   |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>   2 files changed, 93 insertions(+), 8 deletions(-)
>
> diff --git a/spice-display.c b/spice-display.c
> index 1e6d939..fec2432 100644
> --- a/spice-display.c
> +++ b/spice-display.c
> @@ -143,7 +143,7 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>       surface.width      = ds_get_width(ssd->ds);
>       surface.height     = ds_get_height(ssd->ds);
>       surface.stride     = -surface.width * 4;
> -    surface.mouse_mode = 0;
> +    surface.mouse_mode = true;
>       surface.flags      = 0;
>       surface.type       = 0;
>       surface.mem        = (intptr_t)ssd->buf;
> diff --git a/spice-input.c b/spice-input.c
> index 8f3deb4..5646ff9 100644
> --- a/spice-input.c
> +++ b/spice-input.c
> @@ -1,5 +1,6 @@
>   #include<stdlib.h>
>   #include<stdio.h>
> +#include<stdbool.h>
>   #include<string.h>
>
>   #include<spice.h>
> @@ -48,9 +49,13 @@ static void kbd_leds(void *opaque, int ledstate)
>
>   /* mouse bits */
>
> -typedef struct QemuSpiceMouse {
> -    SpiceMouseInstance sin;
> -} QemuSpiceMouse;
> +typedef struct QemuSpicePointer {
> +    SpiceMouseInstance  mouse;
> +    SpiceTabletInstance tablet;
> +    int width, height, x, y;
> +    Notifier mouse_mode;
> +    bool absolute;
> +} QemuSpicePointer;
>
>   static void mouse_motion(SpiceMouseInstance *sin, int dx, int dy, int dz,
>                            uint32_t buttons_state)
> @@ -72,17 +77,97 @@ static const SpiceMouseInterface mouse_interface = {
>       .buttons            = mouse_buttons,
>   };
>
> +static void tablet_set_logical_size(SpiceTabletInstance* sin, int width, int height)
> +{
> +    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
> +
> +    fprintf(stderr, "%s: %dx%d\n", __FUNCTION__, width, height);
> +    if (height<  16)
> +        height = 16;
> +    if (width<  16)
> +        width = 16;
> +    pointer->width  = width;
> +    pointer->height = height;
> +}
> +
> +static void tablet_position(SpiceTabletInstance* sin, int x, int y,
> +                            uint32_t buttons_state)
> +{
> +    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
> +
> +    pointer->x = x * 0x7FFF / (pointer->width - 1);
> +    pointer->y = y * 0x7FFF / (pointer->height - 1);
> +    kbd_mouse_event(pointer->x, pointer->y, 0, buttons_state);
> +}
> +
> +
> +static void tablet_wheel(SpiceTabletInstance* sin, int wheel,
> +                         uint32_t buttons_state)
> +{
> +    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
> +
> +    kbd_mouse_event(pointer->x, pointer->y, wheel, buttons_state);
> +}
> +
> +static void tablet_buttons(SpiceTabletInstance *sin,
> +                           uint32_t buttons_state)
> +{
> +    QemuSpicePointer *pointer = container_of(sin, QemuSpicePointer, tablet);
> +
> +    kbd_mouse_event(pointer->x, pointer->y, 0, buttons_state);
> +}
> +
> +static const SpiceTabletInterface tablet_interface = {
> +    .base.type          = SPICE_INTERFACE_TABLET,
> +    .base.description   = "tablet",
> +    .base.major_version = SPICE_INTERFACE_TABLET_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_TABLET_MINOR,
> +    .set_logical_size   = tablet_set_logical_size,
> +    .position           = tablet_position,
> +    .wheel              = tablet_wheel,
> +    .buttons            = tablet_buttons,
> +};
> +
> +static void mouse_mode_notifier(Notifier *notifier)
> +{
> +    QemuSpicePointer *pointer = container_of(notifier, QemuSpicePointer, mouse_mode);
> +    bool is_absolute  = kbd_mouse_is_absolute();
> +    bool has_absolute = kbd_mouse_has_absolute();
> +
> +    fprintf(stderr, "%s: absolute pointer: %s%s\n", __FUNCTION__,
> +            has_absolute ? "present" : "not available",
> +            is_absolute  ? "+active" : "");
> +
> +    if (pointer->absolute == is_absolute)
> +        return;
> +
> +    if (is_absolute) {
> +        fprintf(stderr, "%s: using absolute pointer (client mode)\n", __FUNCTION__);
> +        spice_server_add_interface(spice_server,&pointer->tablet.base);
> +    } else {
> +        fprintf(stderr, "%s: using relative pointer (server mode)\n", __FUNCTION__);
> +        spice_server_remove_interface(&pointer->tablet.base);
> +    }
> +    pointer->absolute = is_absolute;
> +}
> +
>    

Minus the fprintf()s and the lack of conversion of button formats, this 
all looks pretty reasonable.

Regards,

Anthony Liguori

>   void qemu_spice_input_init(void)
>   {
>       QemuSpiceKbd *kbd;
> -    QemuSpiceMouse *mouse;
> +    QemuSpicePointer *pointer;
>
>       kbd = qemu_mallocz(sizeof(*kbd));
>       kbd->sin.base.sif =&kbd_interface.base;
>       spice_server_add_interface(spice_server,&kbd->sin.base);
>       qemu_add_led_event_handler(kbd_leds, kbd);
>
> -    mouse = qemu_mallocz(sizeof(*mouse));
> -    mouse->sin.base.sif =&mouse_interface.base;
> -    spice_server_add_interface(spice_server,&mouse->sin.base);
> +    pointer = qemu_mallocz(sizeof(*pointer));
> +    pointer->mouse.base.sif  =&mouse_interface.base;
> +    pointer->tablet.base.sif =&tablet_interface.base;
> +    spice_server_add_interface(spice_server,&pointer->mouse.base);
> +
> +    pointer->absolute = false;
> +    pointer->mouse_mode.notify = mouse_mode_notifier;
> +    qemu_add_mouse_mode_change_notifier(&pointer->mouse_mode);
> +    mouse_mode_notifier(&pointer->mouse_mode);
>   }
>