[Qemu-devel] [PULL for-2.8 0/4] vga fixes

[Qemu-devel] [PULL for-2.8 0/4] vga fixes

[Gerd Hoffmann]

  Hi,

Here is a last-minute poll for 2.8-rc3, bringing some vga fixes.

Most important one is the qxl fix which is quite user-visible.
Sorry for submitting that late, it lingers in my queue for a while
already and I through I had that in the last vga pull already, but
obviously that isn't the case.  If you feel it is too late now it'll
be -stable instead.

cheers,
  Gerd

The following changes since commit bd8ef5060dd2124a54578241da9a572faf7658dd:

  Merge remote-tracking branch 'dgibson/tags/ppc-for-2.8-20161201' into staging (2016-12-01 13:39:29 +0000)

are available in the git repository at:


  git://git.kraxel.org/qemu tags/pull-vga-20161205-1

for you to fetch changes up to 4299b90e9ba9ce5ca9024572804ba751aa1a7e70:

  display: cirrus: check vga bits per pixel(bpp) value (2016-12-05 11:01:55 +0100)

----------------------------------------------------------------
qxl: fix flickering.
cirrus: avoid devision by zero.
virtio-gpu: fix two leaks.

----------------------------------------------------------------
Christophe Fergeau (1):
      qxl: Only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG on config changes

Li Qiang (2):
      virtio-gpu: fix information leak in getting capset info dispatch
      virtio-gpu: fix memory leak in update_cursor_data_virgl

Prasad J Pandit (1):
      display: cirrus: check vga bits per pixel(bpp) value

 hw/display/cirrus_vga.c    | 14 ++++++++++----
 hw/display/qxl.c           | 37 ++++++++++++++++++++++++++++++++++++-
 hw/display/virtio-gpu-3d.c |  1 +
 hw/display/virtio-gpu.c    |  1 +
 4 files changed, 48 insertions(+), 5 deletions(-)

[Qemu-devel] [PULL 1/4] qxl: Only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG on config changes

[Gerd Hoffmann]

From: Christophe Fergeau <cfergeau@redhat.com>

Currently if the client keeps sending the same monitor config to
QEMU/spice-server, QEMU will always raise
a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG regardless of whether there was a
change or not.
Guest-side (with fedora 25), the kernel QXL KMS driver will also forward the
event to user-space without checking if there were actual changes.
Next in line are gnome-shell/mutter (on a default f25 install), which
will try to reconfigure everything without checking if there is anything
to do.
Where this gets ugly is that when applying the resolution changes,
gnome-shell/mutter will call drmModeRmFB, drmModeAddFB, and
drmModeSetCrtc, which will cause the primary surface to be destroyed and
recreated by the QXL KMS driver. This in turn will cause the client to
resend a client monitors config message, which will cause QEMU to reemit
an interrupt with an unchanged monitors configuration, ...
This causes https://bugzilla.redhat.com/show_bug.cgi?id=1266484

This commit makes sure that we only emit
QXL_INTERRUPT_CLIENT_MONITORS_CONFIG when there are actual configuration
changes the guest should act on.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Message-id: 20161028144840.18326-1-cfergeau@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 0e2682d..62d0c80 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -992,6 +992,34 @@ static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
     return crc32(0xffffffff, p, len) ^ 0xffffffff;
 }
 
+static bool qxl_rom_monitors_config_changed(QXLRom *rom,
+        VDAgentMonitorsConfig *monitors_config,
+        unsigned int max_outputs)
+{
+    int i;
+    unsigned int monitors_count;
+
+    monitors_count = MIN(monitors_config->num_of_monitors, max_outputs);
+
+    if (rom->client_monitors_config.count != monitors_count) {
+        return true;
+    }
+
+    for (i = 0 ; i < rom->client_monitors_config.count ; ++i) {
+        VDAgentMonConfig *monitor = &monitors_config->monitors[i];
+        QXLURect *rect = &rom->client_monitors_config.heads[i];
+        /* monitor->depth ignored */
+        if ((rect->left != monitor->x) ||
+            (rect->top != monitor->y)  ||
+            (rect->right != monitor->x + monitor->width) ||
+            (rect->bottom != monitor->y + monitor->height)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 /* called from main context only */
 static int interface_client_monitors_config(QXLInstance *sin,
                                         VDAgentMonitorsConfig *monitors_config)
@@ -1000,6 +1028,7 @@ static int interface_client_monitors_config(QXLInstance *sin,
     QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
     int i;
     unsigned max_outputs = ARRAY_SIZE(rom->client_monitors_config.heads);
+    bool config_changed = false;
 
     if (qxl->revision < 4) {
         trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
@@ -1030,6 +1059,10 @@ static int interface_client_monitors_config(QXLInstance *sin,
     }
 #endif
 
+    config_changed = qxl_rom_monitors_config_changed(rom,
+                                                     monitors_config,
+                                                     max_outputs);
+
     memset(&rom->client_monitors_config, 0,
            sizeof(rom->client_monitors_config));
     rom->client_monitors_config.count = monitors_config->num_of_monitors;
@@ -1059,7 +1092,9 @@ static int interface_client_monitors_config(QXLInstance *sin,
     trace_qxl_interrupt_client_monitors_config(qxl->id,
                         rom->client_monitors_config.count,
                         rom->client_monitors_config.heads);
-    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+    if (config_changed) {
+        qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+    }
     return 1;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 2/4] virtio-gpu: fix information leak in getting capset info dispatch

[Gerd Hoffmann]

From: Li Qiang <liqiang6-s@360.cn>

In virgl_cmd_get_capset_info dispatch function, the 'resp' hasn't
been full initialized before writing to the guest. This will leak
the 'resp.padding' and 'resp.hdr.padding' fieds to the guest. This
patch fix this issue.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Message-id: 5818661e.0860240a.77264.7a56@mx.google.com
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/virtio-gpu-3d.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index 758d33a..23f39de 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -347,6 +347,7 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
 
     VIRTIO_GPU_FILL_CMD(info);
 
+    memset(&resp, 0, sizeof(resp));
     if (info.capset_index == 0) {
         resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
         virgl_renderer_get_cap_set(resp.capset_id,
-- 
1.8.3.1

[Qemu-devel] [PULL 3/4] virtio-gpu: fix memory leak in update_cursor_data_virgl

[Gerd Hoffmann]

From: Li Qiang <liqiang6-s@360.cn>

In update_cursor_data_virgl function, if the 'width'/ 'height'
is not equal to current cursor's width/height it will return
without free the 'data' allocated previously. This will lead
a memory leak issue. This patch fix this issue.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Message-id: 58187760.41d71c0a.cca75.4cb9@mx.google.com
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/virtio-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 60bce94..5f32e1a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -84,6 +84,7 @@ static void update_cursor_data_virgl(VirtIOGPU *g,
 
     if (width != s->current_cursor->width ||
         height != s->current_cursor->height) {
+        free(data);
         return;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 4/4] display: cirrus: check vga bits per pixel(bpp) value

[Gerd Hoffmann]

From: Prasad J Pandit <pjp@fedoraproject.org>

In Cirrus CLGD 54xx VGA Emulator, if cirrus graphics mode is VGA,
'cirrus_get_bpp' returns zero(0), which could lead to a divide
by zero error in while copying pixel data. The same could occur
via blit pitch values. Add check to avoid it.

Reported-by: Huawei PSIRT <psirt@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Message-id: 1476776717-24807-1-git-send-email-ppandit@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/cirrus_vga.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 3d712d5..bdb092e 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState *s);
 static bool blit_region_is_unsafe(struct CirrusVGAState *s,
                                   int32_t pitch, int32_t addr)
 {
+    if (!pitch) {
+        return true;
+    }
     if (pitch < 0) {
         int64_t min = addr
             + ((int64_t)s->cirrus_blt_height-1) * pitch;
@@ -715,7 +718,7 @@ static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
                                             s->cirrus_addr_mask));
 }
 
-static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
+static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 {
     int sx = 0, sy = 0;
     int dx = 0, dy = 0;
@@ -729,6 +732,9 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
         int width, height;
 
         depth = s->vga.get_bpp(&s->vga) / 8;
+        if (!depth) {
+            return 0;
+        }
         s->vga.get_resolution(&s->vga, &width, &height);
 
         /* extra x, y */
@@ -783,6 +789,8 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
     cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
 				s->cirrus_blt_dstpitch, s->cirrus_blt_width,
 				s->cirrus_blt_height);
+
+    return 1;
 }
 
 static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
@@ -790,11 +798,9 @@ static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
     if (blit_is_unsafe(s))
         return 0;
 
-    cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
+    return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
             s->cirrus_blt_srcaddr - s->vga.start_addr,
             s->cirrus_blt_width, s->cirrus_blt_height);
-
-    return 1;
 }
 
 /***************************************
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.8 0/4] vga fixes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On Mon, Dec 05, 2016 at 12:03:56PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> Here is a last-minute poll for 2.8-rc3, bringing some vga fixes.
> 
> Most important one is the qxl fix which is quite user-visible.
> Sorry for submitting that late, it lingers in my queue for a while
> already and I through I had that in the last vga pull already, but
> obviously that isn't the case.  If you feel it is too late now it'll
> be -stable instead.
> 
> cheers,
>   Gerd
> 
> The following changes since commit bd8ef5060dd2124a54578241da9a572faf7658dd:
> 
>   Merge remote-tracking branch 'dgibson/tags/ppc-for-2.8-20161201' into staging (2016-12-01 13:39:29 +0000)
> 
> are available in the git repository at:
> 
> 
>   git://git.kraxel.org/qemu tags/pull-vga-20161205-1
> 
> for you to fetch changes up to 4299b90e9ba9ce5ca9024572804ba751aa1a7e70:
> 
>   display: cirrus: check vga bits per pixel(bpp) value (2016-12-05 11:01:55 +0100)
> 
> ----------------------------------------------------------------
> qxl: fix flickering.
> cirrus: avoid devision by zero.
> virtio-gpu: fix two leaks.
> 
> ----------------------------------------------------------------
> Christophe Fergeau (1):
>       qxl: Only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG on config changes
> 
> Li Qiang (2):
>       virtio-gpu: fix information leak in getting capset info dispatch
>       virtio-gpu: fix memory leak in update_cursor_data_virgl
> 
> Prasad J Pandit (1):
>       display: cirrus: check vga bits per pixel(bpp) value
> 
>  hw/display/cirrus_vga.c    | 14 ++++++++++----
>  hw/display/qxl.c           | 37 ++++++++++++++++++++++++++++++++++++-
>  hw/display/virtio-gpu-3d.c |  1 +
>  hw/display/virtio-gpu.c    |  1 +
>  4 files changed, 48 insertions(+), 5 deletions(-)
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]