[Qemu-devel] [PATCH] qxl: Default to PCI revision 4 when spice-server is new enough

[Qemu-devel] [PATCH] qxl: Default to PCI revision 4 when spice-server is new enough

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

When spice-server is newer than 0.12.0, the default revision should be
QXL_REVISION_STABLE_V12.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 hw/qxl.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.h b/hw/qxl.h
index 5553824..4460bb2 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -129,8 +129,7 @@ typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
-#if 0
-/* spice-server 0.12 is still in development */
+#if SPICE_SERVER_VERSION >= 0x000c00 /* 0x000c00 == 0.12.0 */
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
 #else
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
-- 
1.7.4

Re: [Qemu-devel] [PATCH] qxl: Default to PCI revision 4 when spice-server is new enough

[Paolo Bonzini]

Il 03/10/2012 19:57, Søren Sandmann ha scritto:
> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> When spice-server is newer than 0.12.0, the default revision should be
> QXL_REVISION_STABLE_V12.
> 
> Signed-off-by: Soren Sandmann <ssp@redhat.com>
> ---
>  hw/qxl.h |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 5553824..4460bb2 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -129,8 +129,7 @@ typedef struct PCIQXLDevice {
>          }                                                               \
>      } while (0)
>  
> -#if 0
> -/* spice-server 0.12 is still in development */
> +#if SPICE_SERVER_VERSION >= 0x000c00 /* 0x000c00 == 0.12.0 */
>  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
>  #else
>  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> 

No, that's not possible.  The revision must be the same for all machines
started with the same "-M" argument.  So your only choice is to make
0.12 a requirement for QEMU 1.3.  Then you can use compatibility
properties (see the long list of strings in hw/pc_piix.c) to ensure that
pc-1.2 and earlier machines use the previous revision.

If you cannot release 0.12 in time for QEMU 1.3, your only choice is to
wait for 1.4.  In the meanwhile, use "-global qxl.revision=4 -global
qxl-vga.revision=4" to create a machine with revision 4.

Paolo

Re: [Qemu-devel] [PATCH] qxl: Default to PCI revision 4 when spice-server is new enough

[Søren Sandmann]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 03/10/2012 19:57, Søren Sandmann ha scritto:
>> From: Søren Sandmann Pedersen <ssp@redhat.com>
>> 
>> When spice-server is newer than 0.12.0, the default revision should be
>> QXL_REVISION_STABLE_V12.
>> 
>> Signed-off-by: Soren Sandmann <ssp@redhat.com>
>> ---
>>  hw/qxl.h |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/qxl.h b/hw/qxl.h
>> index 5553824..4460bb2 100644
>> --- a/hw/qxl.h
>> +++ b/hw/qxl.h
>> @@ -129,8 +129,7 @@ typedef struct PCIQXLDevice {
>>          }                                                               \
>>      } while (0)
>>  
>> -#if 0
>> -/* spice-server 0.12 is still in development */
>> +#if SPICE_SERVER_VERSION >= 0x000c00 /* 0x000c00 == 0.12.0 */
>>  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
>>  #else
>>  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
>> 
>
> No, that's not possible.  The revision must be the same for all machines
> started with the same "-M" argument.  So your only choice is to make
> 0.12 a requirement for QEMU 1.3.  Then you can use compatibility
> properties (see the long list of strings in hw/pc_piix.c) to ensure that
> pc-1.2 and earlier machines use the previous revision.

Spice server 0.12 is already, so I'll send a patch to require that
unconditionally. I'm assuming that later strings override earlier ones
so that the revision=3 I added won't take effect for previous machines.


Søren

[Qemu-devel] [PATCH] Require spice-server 0.12.0 and spice-protocol 0.12.2

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

Set the default PCI revision to QXL_REVISION_STABLE_V12, and remove
the various #ifs on various subversions of spice-server 0.11.x.

Also set the revision to 3 for the PC 1.2 machine for compatibility.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 configure    |   10 ++--------
 hw/pc_piix.c |    8 ++++++++
 hw/qxl.c     |   29 -----------------------------
 hw/qxl.h     |    5 -----
 4 files changed, 10 insertions(+), 42 deletions(-)

diff --git a/configure b/configure
index 8f99b7b..317a6b0 100755
--- a/configure
+++ b/configure
@@ -2685,20 +2685,14 @@ int main(void) { spice_server_new(); return 0; }
 EOF
   spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null)
   spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
-  if $pkg_config --atleast-version=0.8.2 spice-server >/dev/null 2>&1 && \
-     $pkg_config --atleast-version=0.8.1 spice-protocol > /dev/null 2>&1 && \
+  if $pkg_config --atleast-version=0.12.0 spice-server >/dev/null 2>&1 && \
+     $pkg_config --atleast-version=0.12.2 spice-protocol > /dev/null 2>&1 && \
      compile_prog "$spice_cflags" "$spice_libs" ; then
     spice="yes"
     libs_softmmu="$libs_softmmu $spice_libs"
     QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
     spice_protocol_version=$($pkg_config --modversion spice-protocol)
     spice_server_version=$($pkg_config --modversion spice-server)
-    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
-        spice_qxl_io_monitors_config_async="yes"
-    fi
-    if $pkg_config --atleast-version=0.12.2 spice-protocol > /dev/null 2>&1; then
-        spice_qxl_client_monitors_config="yes"
-    fi
   else
     if test "$spice" = "yes" ; then
       feature_not_found "spice"
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index fd5898f..82364ab 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -371,6 +371,14 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "ivshmem",\
             .property = "use64",\
             .value    = "0",\
+        },{\
+            .driver   = "qxl",\
+            .property = "revision",\
+            .value    = stringify(3),\
+        },{\
+            .driver   = "qxl-vga",\
+            .property = "revision",\
+            .value    = stringify(3),\
         }
 
 static QEMUMachine pc_machine_v1_2 = {
diff --git a/hw/qxl.c b/hw/qxl.c
index 33169f3..c256bda 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -29,11 +29,6 @@
 
 #include "qxl.h"
 
-#ifndef CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC
-/* spice-protocol is too old, add missing definitions */
-#define QXL_IO_MONITORS_CONFIG_ASYNC (QXL_IO_FLUSH_RELEASE + 1)
-#endif
-
 /*
  * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
  * such can be changed by the guest, so to avoid a guest trigerrable
@@ -262,9 +257,6 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
 static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
 {
     trace_qxl_spice_monitors_config(qxl->id);
-/* 0x000b01 == 0.11.1 */
-#if SPICE_SERVER_VERSION >= 0x000b01 && \
-    defined(CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC)
     if (replay) {
         /*
          * don't use QXL_COOKIE_TYPE_IO:
@@ -286,10 +278,6 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
                 (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
                                           QXL_IO_MONITORS_CONFIG_ASYNC));
     }
-#else
-    fprintf(stderr, "qxl: too old spice-protocol/spice-server for "
-            "QXL_IO_MONITORS_CONFIG_ASYNC\n");
-#endif
 }
 
 void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
@@ -948,8 +936,6 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     }
 }
 
-#if SPICE_SERVER_VERSION >= 0x000b04
-
 /* called from spice server thread context only */
 static void interface_set_client_capabilities(QXLInstance *sin,
                                               uint8_t client_present,
@@ -971,11 +957,6 @@ static void interface_set_client_capabilities(QXLInstance *sin,
     qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
 }
 
-#endif
-
-#if defined(CONFIG_QXL_CLIENT_MONITORS_CONFIG) \
-    && SPICE_SERVER_VERSION >= 0x000b05
-
 static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
 {
     /*
@@ -1044,7 +1025,6 @@ static int interface_client_monitors_config(QXLInstance *sin,
     qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
     return 1;
 }
-#endif
 
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
@@ -1067,13 +1047,8 @@ static const QXLInterface qxl_interface = {
     .flush_resources         = interface_flush_resources,
     .async_complete          = interface_async_complete,
     .update_area_complete    = interface_update_area_complete,
-#if SPICE_SERVER_VERSION >= 0x000b04
     .set_client_capabilities = interface_set_client_capabilities,
-#endif
-#if SPICE_SERVER_VERSION >= 0x000b05 && \
-    defined(CONFIG_QXL_CLIENT_MONITORS_CONFIG)
     .client_monitors_config = interface_client_monitors_config,
-#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1971,14 +1946,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
         pci_device_rev = QXL_REVISION_STABLE_V10;
         io_size = 32; /* PCI region size must be pow2 */
         break;
-/* 0x000b01 == 0.11.1 */
-#if SPICE_SERVER_VERSION >= 0x000b01 && \
-        defined(CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC)
     case 4: /* qxl-4 */
         pci_device_rev = QXL_REVISION_STABLE_V12;
         io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
         break;
-#endif
     default:
         error_report("Invalid revision %d for qxl device (max %d)",
                      qxl->revision, QXL_DEFAULT_REVISION);
diff --git a/hw/qxl.h b/hw/qxl.h
index 5553824..e583cfb 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -129,12 +129,7 @@ typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
-#if 0
-/* spice-server 0.12 is still in development */
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
-#else
-#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
-#endif
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-- 
1.7.4

Re: [Qemu-devel] [PATCH] qxl: Default to PCI revision 4 when spice-server is new enough

[Paolo Bonzini]

Il 04/10/2012 19:31, Søren Sandmann ha scritto:
>> >
>> > No, that's not possible.  The revision must be the same for all machines
>> > started with the same "-M" argument.  So your only choice is to make
>> > 0.12 a requirement for QEMU 1.3.  Then you can use compatibility
>> > properties (see the long list of strings in hw/pc_piix.c) to ensure that
>> > pc-1.2 and earlier machines use the previous revision.
> Spice server 0.12 is already, so I'll send a patch to require that
> unconditionally. I'm assuming that later strings override earlier ones
> so that the revision=3 I added won't take effect for previous machines.

There is a PC_COMPAT_1_2 macro, just add the

        {\
            .driver   = "qxl-vga",\
            .property = "revision",\
            .value    = "3",\
        },{\
        {\
            .driver   = "qxl",\
            .property = "revision",\
            .value    = "3",\
        },{\

there.

Paolo

Re: [Qemu-devel] [PATCH] Require spice-server 0.12.0 and spice-protocol 0.12.2

[Paolo Bonzini]

Il 04/10/2012 19:37, Søren Sandmann ha scritto:
> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> Set the default PCI revision to QXL_REVISION_STABLE_V12, and remove
> the various #ifs on various subversions of spice-server 0.11.x.
> 
> Also set the revision to 3 for the PC 1.2 machine for compatibility.
> 
> Signed-off-by: Soren Sandmann <ssp@redhat.com>
> ---
>  configure    |   10 ++--------
>  hw/pc_piix.c |    8 ++++++++
>  hw/qxl.c     |   29 -----------------------------
>  hw/qxl.h     |    5 -----
>  4 files changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/configure b/configure
> index 8f99b7b..317a6b0 100755
> --- a/configure
> +++ b/configure
> @@ -2685,20 +2685,14 @@ int main(void) { spice_server_new(); return 0; }
>  EOF
>    spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null)
>    spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
> -  if $pkg_config --atleast-version=0.8.2 spice-server >/dev/null 2>&1 && \
> -     $pkg_config --atleast-version=0.8.1 spice-protocol > /dev/null 2>&1 && \
> +  if $pkg_config --atleast-version=0.12.0 spice-server >/dev/null 2>&1 && \
> +     $pkg_config --atleast-version=0.12.2 spice-protocol > /dev/null 2>&1 && \
>       compile_prog "$spice_cflags" "$spice_libs" ; then
>      spice="yes"
>      libs_softmmu="$libs_softmmu $spice_libs"
>      QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
>      spice_protocol_version=$($pkg_config --modversion spice-protocol)
>      spice_server_version=$($pkg_config --modversion spice-server)
> -    if $pkg_config --atleast-version=0.12.0 spice-protocol >/dev/null 2>&1; then
> -        spice_qxl_io_monitors_config_async="yes"
> -    fi
> -    if $pkg_config --atleast-version=0.12.2 spice-protocol > /dev/null 2>&1; then
> -        spice_qxl_client_monitors_config="yes"
> -    fi
>    else
>      if test "$spice" = "yes" ; then
>        feature_not_found "spice"
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index fd5898f..82364ab 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -371,6 +371,14 @@ static QEMUMachine pc_machine_v1_3 = {
>              .driver   = "ivshmem",\
>              .property = "use64",\
>              .value    = "0",\
> +        },{\
> +            .driver   = "qxl",\
> +            .property = "revision",\
> +            .value    = stringify(3),\
> +        },{\
> +            .driver   = "qxl-vga",\
> +            .property = "revision",\
> +            .value    = stringify(3),\
>          }
>  
>  static QEMUMachine pc_machine_v1_2 = {
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 33169f3..c256bda 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -29,11 +29,6 @@
>  
>  #include "qxl.h"
>  
> -#ifndef CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC
> -/* spice-protocol is too old, add missing definitions */
> -#define QXL_IO_MONITORS_CONFIG_ASYNC (QXL_IO_FLUSH_RELEASE + 1)
> -#endif
> -
>  /*
>   * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
>   * such can be changed by the guest, so to avoid a guest trigerrable
> @@ -262,9 +257,6 @@ static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl, qxl_async_io async)
>  static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
>  {
>      trace_qxl_spice_monitors_config(qxl->id);
> -/* 0x000b01 == 0.11.1 */
> -#if SPICE_SERVER_VERSION >= 0x000b01 && \
> -    defined(CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC)
>      if (replay) {
>          /*
>           * don't use QXL_COOKIE_TYPE_IO:
> @@ -286,10 +278,6 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
>                  (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO,
>                                            QXL_IO_MONITORS_CONFIG_ASYNC));
>      }
> -#else
> -    fprintf(stderr, "qxl: too old spice-protocol/spice-server for "
> -            "QXL_IO_MONITORS_CONFIG_ASYNC\n");
> -#endif
>  }
>  
>  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
> @@ -948,8 +936,6 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
>      }
>  }
>  
> -#if SPICE_SERVER_VERSION >= 0x000b04
> -
>  /* called from spice server thread context only */
>  static void interface_set_client_capabilities(QXLInstance *sin,
>                                                uint8_t client_present,
> @@ -971,11 +957,6 @@ static void interface_set_client_capabilities(QXLInstance *sin,
>      qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
>  }
>  
> -#endif
> -
> -#if defined(CONFIG_QXL_CLIENT_MONITORS_CONFIG) \
> -    && SPICE_SERVER_VERSION >= 0x000b05
> -
>  static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
>  {
>      /*
> @@ -1044,7 +1025,6 @@ static int interface_client_monitors_config(QXLInstance *sin,
>      qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
>      return 1;
>  }
> -#endif
>  
>  static const QXLInterface qxl_interface = {
>      .base.type               = SPICE_INTERFACE_QXL,
> @@ -1067,13 +1047,8 @@ static const QXLInterface qxl_interface = {
>      .flush_resources         = interface_flush_resources,
>      .async_complete          = interface_async_complete,
>      .update_area_complete    = interface_update_area_complete,
> -#if SPICE_SERVER_VERSION >= 0x000b04
>      .set_client_capabilities = interface_set_client_capabilities,
> -#endif
> -#if SPICE_SERVER_VERSION >= 0x000b05 && \
> -    defined(CONFIG_QXL_CLIENT_MONITORS_CONFIG)
>      .client_monitors_config = interface_client_monitors_config,
> -#endif
>  };
>  
>  static void qxl_enter_vga_mode(PCIQXLDevice *d)
> @@ -1971,14 +1946,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>          pci_device_rev = QXL_REVISION_STABLE_V10;
>          io_size = 32; /* PCI region size must be pow2 */
>          break;
> -/* 0x000b01 == 0.11.1 */
> -#if SPICE_SERVER_VERSION >= 0x000b01 && \
> -        defined(CONFIG_QXL_IO_MONITORS_CONFIG_ASYNC)
>      case 4: /* qxl-4 */
>          pci_device_rev = QXL_REVISION_STABLE_V12;
>          io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
>          break;
> -#endif
>      default:
>          error_report("Invalid revision %d for qxl device (max %d)",
>                       qxl->revision, QXL_DEFAULT_REVISION);
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 5553824..e583cfb 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -129,12 +129,7 @@ typedef struct PCIQXLDevice {
>          }                                                               \
>      } while (0)
>  
> -#if 0
> -/* spice-server 0.12 is still in development */
>  #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
> -#else
> -#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> -#endif
>  
>  /* qxl.c */
>  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
> 

Nice cleanup, thanks!

As a followup, there is some confusion between values of the revision
property (1-4) and PCI device revisions (QXL_REVISION_STABLE_Vxy).  They
are the same right now, but this means that you can do one of the following:

1) if they are always going to be the same, eliminate pci_device_rev
from qxl_init_common, just use qxl->revision instead;

2) if they might diverge, change the value of QXL_DEFAULT_REVISION from
QXL_REVISION_STABLE_V12 to 4.

Paolo

Re: [Qemu-devel] [PATCH] Require spice-server 0.12.0 and spice-protocol 0.12.2

[Søren Sandmann]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Nice cleanup, thanks!
>
> As a followup, there is some confusion between values of the revision
> property (1-4) and PCI device revisions (QXL_REVISION_STABLE_Vxy).  They
> are the same right now, but this means that you can do one of the following:
>
> 1) if they are always going to be the same, eliminate pci_device_rev
> from qxl_init_common, just use qxl->revision instead;
>
> 2) if they might diverge, change the value of QXL_DEFAULT_REVISION from
> QXL_REVISION_STABLE_V12 to 4.

Without knowing why these names are separate, it's difficult to say
whether (1) or (2) is right. In any case, it seems like an orthogonal
issue from the revision bump itself.


Søren

Re: [Qemu-devel] [PATCH] Require spice-server 0.12.0 and spice-protocol 0.12.2

[Gerd Hoffmann]

On 10/04/12 19:37, Søren Sandmann wrote:
> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> Set the default PCI revision to QXL_REVISION_STABLE_V12, and remove
> the various #ifs on various subversions of spice-server 0.11.x.

Simliar patch is in spice-next (see
http://cgit.freedesktop.org/spice/qemu/log/?h=rebase/spice-next) already.

> Also set the revision to 3 for the PC 1.2 machine for compatibility.

The revision change should be a separate patch.  I'll go handle it when
sending out the next spice pull request today or tomorrow.

cheers,
  Gerd