[Qemu-devel] [PATCH 1/2] hw/display/qxl: improve framebuffer error message

[Qemu-devel] [PATCH 1/2] hw/display/qxl: improve framebuffer error message

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/display/qxl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e4f172e..f6af470 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1367,7 +1367,8 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
     size = abs(requested_stride) * requested_height;
     if (size > qxl->vgamem_size) {
         qxl_set_guest_bug(qxl, "%s: requested primary larger then framebuffer"
-                               " size", __func__);
+                               " size %d > %d", __func__, size,
+                               qxl->vgamem_size);
         return;
     }
 
-- 
1.8.4.2

[Qemu-devel] [PATCH 2/2] qxl: clear irq on reset

[Alon Levy]

Without this we occasionally trigger an assert at
hw/pci/pci.c:pcibus_reset that asserts the irq_count is zero on reset.

This has become a problem with the new drm driver for linux, since doing
a reboot from console causes a race between console updates that set the
irq and the reset assertion that the irq is clear.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/display/qxl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index f6af470..e164d96 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1126,6 +1126,7 @@ static void qxl_reset_state(PCIQXLDevice *d)
     d->num_free_res = 0;
     d->last_release = NULL;
     memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty));
+    qxl_update_irq(d);
 }
 
 static void qxl_soft_reset(PCIQXLDevice *d)
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH 2/2] qxl: clear irq on reset

[Gerd Hoffmann]

On Mo, 2014-01-20 at 12:44 +0200, Alon Levy wrote:
> Without this we occasionally trigger an assert at
> hw/pci/pci.c:pcibus_reset that asserts the irq_count is zero on reset.
> 
> This has become a problem with the new drm driver for linux, since
> doing
> a reboot from console causes a race between console updates that set
> the
> irq and the reset assertion that the irq is clear.
> 

Added to spice patch queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] hw/display/qxl: improve framebuffer error message

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  hw/display/qxl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index e4f172e..f6af470 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1367,7 +1367,8 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>      size = abs(requested_stride) * requested_height;
>      if (size > qxl->vgamem_size) {
>          qxl_set_guest_bug(qxl, "%s: requested primary larger then framebuffer"
> -                               " size", __func__);
> +                               " size %d > %d", __func__, size,
> +                               qxl->vgamem_size);
>          return;
>      }

Shouldn't you use %u or %PRIu32 to print uint32_t qxl->vgamem_size?

Hmm, we're comparing int size to uint32_t vgamem_size, not nice.  Should
size be unsigned?

Since you touch the message anyway, you could fix "larger then" to
"larger than".

[Qemu-devel] [PATCH v2] hw/display/qxl: fix signed to unsigned comparison

[Alon Levy]

Fix signed to unsigned comparison in qxl_create_guest_primary and add
the size of the framebuffer to the error message used when setting the
guest bug state (which causes a complete guess blackout until reset, so
it helps if it is verbose).

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/display/qxl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e4f172e..b799b51 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1360,14 +1360,15 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
 {
     QXLDevSurfaceCreate surface;
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
-    int size;
+    uint32_t size;
     int requested_height = le32_to_cpu(sc->height);
     int requested_stride = le32_to_cpu(sc->stride);
 
     size = abs(requested_stride) * requested_height;
     if (size > qxl->vgamem_size) {
-        qxl_set_guest_bug(qxl, "%s: requested primary larger then framebuffer"
-                               " size", __func__);
+        qxl_set_guest_bug(qxl, "%s: requested primary larger than framebuffer"
+                               " size %u > %u", __func__, size,
+                               qxl->vgamem_size);
         return;
     }
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v2] hw/display/qxl: fix signed to unsigned comparison

[Markus Armbruster]

Alon Levy <alevy@redhat.com> writes:

> Fix signed to unsigned comparison in qxl_create_guest_primary and add
> the size of the framebuffer to the error message used when setting the
> guest bug state (which causes a complete guess blackout until reset, so
> it helps if it is verbose).
>
> Signed-off-by: Alon Levy <alevy@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2] hw/display/qxl: fix signed to unsigned comparison

[Peter Maydell]

On 20 January 2014 13:29, Alon Levy <alevy@redhat.com> wrote:
> Fix signed to unsigned comparison in qxl_create_guest_primary and add
> the size of the framebuffer to the error message used when setting the
> guest bug state (which causes a complete guess blackout until reset, so
> it helps if it is verbose).
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  hw/display/qxl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index e4f172e..b799b51 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1360,14 +1360,15 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>  {
>      QXLDevSurfaceCreate surface;
>      QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
> -    int size;
> +    uint32_t size;
>      int requested_height = le32_to_cpu(sc->height);
>      int requested_stride = le32_to_cpu(sc->stride);
>
>      size = abs(requested_stride) * requested_height;

This looks a bit dubious -- the multiply is still going
to be done with signed arithmetic, so if it's possible
we might overflow so as to require a uint32_t size
then we've already hit undefined behaviour. Also, if
the multiply overflows 32 bits the check will not
catch this. Probably better to do this as a 64 bit
multiply.

Is requested_height really supposed to be signed?
Why is requested_stride an int that needs to go
through abs()? What is the behaviour supposed to be
if it is the minimum integer (in which case abs(x)
is undefined)?

>      if (size > qxl->vgamem_size) {
> -        qxl_set_guest_bug(qxl, "%s: requested primary larger then framebuffer"
> -                               " size", __func__);
> +        qxl_set_guest_bug(qxl, "%s: requested primary larger than framebuffer"
> +                               " size %u > %u", __func__, size,
> +                               qxl->vgamem_size);
>          return;
>      }

PRIu32 is more portable for printing uint32_t types.

thanks
-- PMM