[Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Anthony PERARD]

The qxl driver expect NULL for QXLRam.memory_configs, but this is never
initialized.

If memory is set to 0xc2c2.., it leads to a spice-critical error when
trying to start qxl.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/display/qxl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c475cb1..937a402 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -387,6 +387,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
     d->ram->int_pending = cpu_to_le32(0);
     d->ram->int_mask    = cpu_to_le32(0);
     d->ram->update_surface = 0;
+    d->ram->monitors_config = 0;
     SPICE_RING_INIT(&d->ram->cmd_ring);
     SPICE_RING_INIT(&d->ram->cursor_ring);
     SPICE_RING_INIT(&d->ram->release_ring);
-- 
Anthony PERARD

[Qemu-devel] [PATCH] qxl: Fix QXLRam initialisation.

[Anthony PERARD]

The qxl driver expect NULL for QXLRam.memory_configs, but this is never
initialized.

If memory is set to 0xc2c2.., it leads to a spice-critical error when
trying to start qxl.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/display/qxl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c475cb1..937a402 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -387,6 +387,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
     d->ram->int_pending = cpu_to_le32(0);
     d->ram->int_mask    = cpu_to_le32(0);
     d->ram->update_surface = 0;
+    d->ram->monitors_config = 0;
     SPICE_RING_INIT(&d->ram->cmd_ring);
     SPICE_RING_INIT(&d->ram->cursor_ring);
     SPICE_RING_INIT(&d->ram->release_ring);
-- 
Anthony PERARD

Re: [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Michael Tokarev]

17.06.2013 20:38, Anthony PERARD wrote:
> The qxl driver expect NULL for QXLRam.memory_configs, but this is never
> initialized.
> 
> If memory is set to 0xc2c2.., it leads to a spice-critical error when
> trying to start qxl.

This bug has been introduced by:

 commit 020af1c45fec664d5d4cf3b8e5117f8bc1d691f2
 Author: Alon Levy <alevy@redhat.com>
 Date:   Wed Aug 22 11:16:25 2012 +0300

    qxl: add QXL_IO_MONITORS_CONFIG_ASYNC

which went in past 1.2.0, so it might be a good candidate for -stable
as well.

The issue looks real indeed, and is a good catch.

But maybe instead of doing it this way, we should just memset the
whole thing to 0, to avoid similar bugs in the future?  Are there
any arches out there which have cpu_to_le32(0) != 0 ?

Thanks,

/mjt

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/display/qxl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c475cb1..937a402 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -387,6 +387,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
>      d->ram->int_pending = cpu_to_le32(0);
>      d->ram->int_mask    = cpu_to_le32(0);
>      d->ram->update_surface = 0;
> +    d->ram->monitors_config = 0;
>      SPICE_RING_INIT(&d->ram->cmd_ring);
>      SPICE_RING_INIT(&d->ram->cursor_ring);
>      SPICE_RING_INIT(&d->ram->release_ring);
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Michael Tokarev]

17.06.2013 20:38, Anthony PERARD wrote:
> The qxl driver expect NULL for QXLRam.memory_configs, but this is never
> initialized.
> 
> If memory is set to 0xc2c2.., it leads to a spice-critical error when
> trying to start qxl.

This bug has been introduced by:

 commit 020af1c45fec664d5d4cf3b8e5117f8bc1d691f2
 Author: Alon Levy <alevy@redhat.com>
 Date:   Wed Aug 22 11:16:25 2012 +0300

    qxl: add QXL_IO_MONITORS_CONFIG_ASYNC

which went in past 1.2.0, so it might be a good candidate for -stable
as well.

The issue looks real indeed, and is a good catch.

But maybe instead of doing it this way, we should just memset the
whole thing to 0, to avoid similar bugs in the future?  Are there
any arches out there which have cpu_to_le32(0) != 0 ?

Thanks,

/mjt

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/display/qxl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c475cb1..937a402 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -387,6 +387,7 @@ static void init_qxl_ram(PCIQXLDevice *d)
>      d->ram->int_pending = cpu_to_le32(0);
>      d->ram->int_mask    = cpu_to_le32(0);
>      d->ram->update_surface = 0;
> +    d->ram->monitors_config = 0;
>      SPICE_RING_INIT(&d->ram->cmd_ring);
>      SPICE_RING_INIT(&d->ram->cursor_ring);
>      SPICE_RING_INIT(&d->ram->release_ring);
> 

Re: [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Gerd Hoffmann]

  Hi,

>> If memory is set to 0xc2c2.., it leads to a spice-critical error when
>> trying to start qxl.

> But maybe instead of doing it this way, we should just memset the
> whole thing to 0, to avoid similar bugs in the future?  Are there
> any arches out there which have cpu_to_le32(0) != 0 ?

On qemu + kvm guest memory (including pci device memory where this lives
in) _is_ zero initialized.

IIRC xen has a debug mode where it fills memory with non-zero values
instead, for debugging / testing purposes, and this is where things blow up.

So while the fix is clearly correct, the patch isn't as critical as it
initially looks like.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

Re: [Qemu-devel] [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Gerd Hoffmann]

  Hi,

>> If memory is set to 0xc2c2.., it leads to a spice-critical error when
>> trying to start qxl.

> But maybe instead of doing it this way, we should just memset the
> whole thing to 0, to avoid similar bugs in the future?  Are there
> any arches out there which have cpu_to_le32(0) != 0 ?

On qemu + kvm guest memory (including pci device memory where this lives
in) _is_ zero initialized.

IIRC xen has a debug mode where it fills memory with non-zero values
instead, for debugging / testing purposes, and this is where things blow up.

So while the fix is clearly correct, the patch isn't as critical as it
initially looks like.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

Re: [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Anthony PERARD]

Hi,

On 18/06/13 06:37, Gerd Hoffmann wrote:
> IIRC xen has a debug mode where it fills memory with non-zero values
> instead, for debugging / testing purposes, and this is where things blow up.
>
> So while the fix is clearly correct, the patch isn't as critical as it
> initially looks like.

Indeed, it's not critical. There is other issue with Xen before to get
QXL running.

Regards,

-- 
Anthony PERARD

Re: [Qemu-devel] [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Anthony PERARD]

Hi,

On 18/06/13 06:37, Gerd Hoffmann wrote:
> IIRC xen has a debug mode where it fills memory with non-zero values
> instead, for debugging / testing purposes, and this is where things blow up.
>
> So while the fix is clearly correct, the patch isn't as critical as it
> initially looks like.

Indeed, it's not critical. There is other issue with Xen before to get
QXL running.

Regards,

-- 
Anthony PERARD

Re: [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Michael Tokarev]

17.06.2013 20:38, Anthony PERARD wrote:> The qxl driver expect NULL for QXLRam.memory_configs, but this is never
> initialized.

Thanks, applied to the trivial patches queue.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] qxl: Fix QXLRam initialisation.

[Michael Tokarev]

17.06.2013 20:38, Anthony PERARD wrote:> The qxl driver expect NULL for QXLRam.memory_configs, but this is never
> initialized.

Thanks, applied to the trivial patches queue.

/mjt