[Qemu-devel] [PATCH v2 0/4] spice: minor improvements

[Qemu-devel] [PATCH v2 0/4] spice: minor improvements

[Radim Krčmář]

Series contains two logical parts:
[1-2/4] fix a segfault after user misconfiguration (too big vgamem_mb),
[3-4/4] improve code that was modified by [2/4].

v2 summary:
- added [1/4] to explain the size bump in [2/4]
- modified Gerd's proposed solution as [2/4]
- added [3/4] to have at least one nice patch
- removed assert and kept rounding+lamping in [4/4]


Radim Krčmář (4):
  qxl: document minimal video memory for new modes
  spice: fix invalid memory access to vga.vram
  qxl: refactor rounding up to a nearest power of 2
  vga: refactor vram_size clamping and rounding

 hw/display/qxl.c      | 34 ++++++++++++++++------------------
 hw/display/vga.c      | 22 +++++++++++++++-------
 include/qemu-common.h |  3 +++
 util/cutils.c         | 14 ++++++++++++++
 4 files changed, 48 insertions(+), 25 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH v2 1/4] qxl: document minimal video memory for new modes

[Radim Krčmář]

The alternative to removing existing comments.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new
 
 hw/display/qxl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 61df47726481..6e9079783e27 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -120,9 +120,12 @@ static QXLMode qxl_modes[] = {
     QXL_MODE_EX(2560, 2048),
     QXL_MODE_EX(2800, 2100),
     QXL_MODE_EX(3200, 2400),
+    /* these modes need more than 32 MB video memory */
     QXL_MODE_EX(3840, 2160), /* 4k mainstream */
     QXL_MODE_EX(4096, 2160), /* 4k            */
+    /* these modes need more than 64 MB video memory */
     QXL_MODE_EX(7680, 4320), /* 8k mainstream */
+    /* these modes need more than 128 MB video memory */
     QXL_MODE_EX(8192, 4320), /* 8k            */
 };
 
-- 
2.3.0

[Qemu-devel] [PATCH v2 2/4] spice: fix invalid memory access to vga.vram

[Radim Krčmář]

vga_common_init() doesn't allow more than 256 MiB vram size and silently
shrinks any larger value.  qxl_dirty_surfaces() used the unshrinked size
via qxl->shadow_rom.surface0_area_size when accessing the memory, which
resulted in segfault.

Add a workaround for this case and an assert if it happens again.

We have to bump the vga memory limit too, because 256 MiB wouldn't have
allowed 8k (it requires more than 128 MiB).
1024 MiB doesn't work, but 512 MiB seems fine.

Proposed-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: changed numbers in Gerd's solution, added an assert and a comment
     (was [v1 2/2])
 
 hw/display/qxl.c | 8 ++++++++
 hw/display/vga.c | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6e9079783e27..92f2d5025d70 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -370,6 +370,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
     num_pages         -= surface0_area_size;
     num_pages          = num_pages / QXL_PAGE_SIZE;
 
+    assert(ram_header_size + surface0_area_size <= d->vga.vram_size);
+
     rom->draw_area_offset   = cpu_to_le32(0);
     rom->surface0_area_size = cpu_to_le32(surface0_area_size);
     rom->pages_offset       = cpu_to_le32(surface0_area_size);
@@ -1883,6 +1885,12 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
     if (qxl->vgamem_size_mb < 8) {
         qxl->vgamem_size_mb = 8;
     }
+    /* XXX: we round vgamem_size_mb up to a nearest power of two and it must be
+     * less than vga_common_init()'s maximum on qxl->vga.vram_size (512 now).
+     */
+    if (qxl->vgamem_size_mb > 256) {
+        qxl->vgamem_size_mb = 256;
+    }
     qxl->vgamem_size = qxl->vgamem_size_mb * 1024 * 1024;
 
     /* vga ram (bar 0, total) */
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 9c62fbf48823..13f34f950d5a 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2122,10 +2122,10 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         expand4to8[i] = v;
     }
 
-    /* valid range: 1 MB -> 256 MB */
+    /* valid range: 1 MB -> 512 MB */
     s->vram_size = 1024 * 1024;
     while (s->vram_size < (s->vram_size_mb << 20) &&
-           s->vram_size < (256 << 20)) {
+           s->vram_size < (512 << 20)) {
         s->vram_size <<= 1;
     }
     s->vram_size_mb = s->vram_size >> 20;
-- 
2.3.0

[Qemu-devel] [PATCH v2 3/4] qxl: refactor rounding up to a nearest power of 2

[Radim Krčmář]

We already have pow2floor, mirror it and use instead of a function with
similar results (same in used domain), to clarify our intent.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new
 
 hw/display/qxl.c      | 23 +++++------------------
 include/qemu-common.h |  3 +++
 util/cutils.c         | 14 ++++++++++++++
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 92f2d5025d70..94ff52a959d4 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -300,19 +300,6 @@ void qxl_spice_reset_cursor(PCIQXLDevice *qxl)
     qxl->ssd.cursor = cursor_builtin_hidden();
 }
 
-
-static inline uint32_t msb_mask(uint32_t val)
-{
-    uint32_t mask;
-
-    do {
-        mask = ~(val - 1) & val;
-        val &= ~mask;
-    } while (mask < val);
-
-    return mask;
-}
-
 static ram_addr_t qxl_rom_size(void)
 {
     uint32_t required_rom_size = sizeof(QXLRom) + sizeof(QXLModes) +
@@ -1921,10 +1908,10 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl)
         qxl->vram32_size = 4096;
         qxl->vram_size = 4096;
     }
-    qxl->vgamem_size = msb_mask(qxl->vgamem_size * 2 - 1);
-    qxl->vga.vram_size = msb_mask(qxl->vga.vram_size * 2 - 1);
-    qxl->vram32_size = msb_mask(qxl->vram32_size * 2 - 1);
-    qxl->vram_size = msb_mask(qxl->vram_size * 2 - 1);
+    qxl->vgamem_size = pow2ceil(qxl->vgamem_size);
+    qxl->vga.vram_size = pow2ceil(qxl->vga.vram_size);
+    qxl->vram32_size = pow2ceil(qxl->vram32_size);
+    qxl->vram_size = pow2ceil(qxl->vram_size);
 }
 
 static int qxl_init_common(PCIQXLDevice *qxl)
@@ -1956,7 +1943,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
         break;
     case 4: /* qxl-4 */
         pci_device_rev = QXL_REVISION_STABLE_V12;
-        io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+        io_size = pow2ceil(QXL_IO_RANGE_SIZE);
         break;
     default:
         error_report("Invalid revision %d for qxl device (max %d)",
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 644b46dcddf3..1b5cffb4033b 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -418,6 +418,9 @@ static inline bool is_power_of_2(uint64_t value)
 /* round down to the nearest power of 2*/
 int64_t pow2floor(int64_t value);
 
+/* round up to the nearest power of 2 (0 if overflow) */
+uint64_t pow2ceil(uint64_t value);
+
 #include "qemu/module.h"
 
 /*
diff --git a/util/cutils.c b/util/cutils.c
index dbe7412bd88c..c2250d1ba574 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -483,6 +483,20 @@ int64_t pow2floor(int64_t value)
     return value;
 }
 
+/* round up to the nearest power of 2 (0 if overflow) */
+uint64_t pow2ceil(uint64_t value)
+{
+    uint8_t nlz = clz64(value);
+
+    if (is_power_of_2(value)) {
+        return value;
+    }
+    if (!nlz) {
+        return 0;
+    }
+    return 1ULL << (64 - nlz);
+}
+
 /*
  * Implementation of  ULEB128 (http://en.wikipedia.org/wiki/LEB128)
  * Input is limited to 14-bit numbers
-- 
2.3.0

[Qemu-devel] [PATCH v2 4/4] vga: refactor vram_size clamping and rounding

[Radim Krčmář]

Make the code a bit more obvious.

We don't have min/max, so a general helper for clamp probably isn't
acceptable either.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: kept rounding and clamping (was [v1 1/2])

 hw/display/vga.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 13f34f950d5a..7e0c01dc57fa 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2095,6 +2095,17 @@ static const GraphicHwOps vga_ops = {
     .text_update = vga_update_text,
 };
 
+static inline uint32_t uint_clamp(uint32_t val, uint32_t vmin, uint32_t vmax)
+{
+    if (val < vmin) {
+        return vmin;
+    }
+    if (val > vmax) {
+        return vmax;
+    }
+    return val;
+}
+
 void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
 {
     int i, j, v, b;
@@ -2122,13 +2133,10 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         expand4to8[i] = v;
     }
 
-    /* valid range: 1 MB -> 512 MB */
-    s->vram_size = 1024 * 1024;
-    while (s->vram_size < (s->vram_size_mb << 20) &&
-           s->vram_size < (512 << 20)) {
-        s->vram_size <<= 1;
-    }
-    s->vram_size_mb = s->vram_size >> 20;
+    s->vram_size_mb = uint_clamp(s->vram_size_mb, 1, 512);
+    s->vram_size_mb = pow2ceil(s->vram_size_mb);
+    s->vram_size = s->vram_size_mb << 20;
+
     if (!s->vbe_size) {
         s->vbe_size = s->vram_size;
     }
-- 
2.3.0