[PATCH for-9.0 0/4] vga: fix assertion failure with 4- and 16-color modes

[PATCH for-9.0 0/4] vga: fix assertion failure with 4- and 16-color modes

[Paolo Bonzini]

These patches (the first three especially) fix an assertion failure
introduced by horizontal pel panning support in VGA.  The assertion
triggers with legacy 4- and 16-color modes, due to a mismatch between
the addresses visited by vga_draw_graphic() and the region that is
passed to memory_region_snapshot_and_clear_dirty().

Patches 1 and 2 reorganize the code so that the "bits" value
(used in turn to check if horizontal pel panning is taken into
account) is available where the dirty memory region is computed.

Patch 3 is the actual bug fix.

Patch 4 is a small optimization that would also hide the bug, by
treating pel panning as disabled in the common case where the
register is set to 8 (bit 3 is ignored in graphics mode).
This one could be suitable for QEMU 9.0 but is not necessary.

Patches 5 and 6 are larger cleanups and optimizations in how the dirty
memory region is computed.  This is enabled by the availability of "bits"
where the dirty memory region is computed; it is now possible for 8-
and 15-bit modes to skip the slow path and only read dirty bits for a
small part of VRAM.

Paolo Bonzini (6):
  vga: merge conditionals on shift control register
  vga: move computation of dirty memory region later
  vga: adjust dirty memory region if pel panning is active
  vga: do not treat horiz pel panning value of 8 as "enabled"
  vga: optimize computation of dirty memory region
  vga: move dirty memory region code together

 hw/display/vga.c     | 152 ++++++++++++++++++++-----------------------
 1 file changed, 71 insertions(+), 81 deletions(-)

-- 
2.44.0

[PATCH for-9.0 1/4] vga: merge conditionals on shift control register

[Paolo Bonzini]

There are two sets of conditionals using the shift control bits: one to
verify the palette and adjust disp_width, one to compute the "v" and
"bits" variables.  Merge them into one, with the extra benefit that
we now have the "bits" value available early and can use it to
compute region_end.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 89 +++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index bc5b83421bf..4795a0012e2 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1546,12 +1546,54 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     }
 
     if (shift_control == 0) {
+        full_update |= update_palette16(s);
         if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
             disp_width <<= 1;
+            v = VGA_DRAW_LINE4D2;
+        } else {
+            v = VGA_DRAW_LINE4;
         }
+        bits = 4;
+
     } else if (shift_control == 1) {
+        full_update |= update_palette16(s);
         if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
             disp_width <<= 1;
+            v = VGA_DRAW_LINE2D2;
+        } else {
+            v = VGA_DRAW_LINE2;
+        }
+        bits = 4;
+
+    } else {
+        switch (depth) {
+        default:
+        case 0:
+            full_update |= update_palette256(s);
+            v = VGA_DRAW_LINE8D2;
+            bits = 4;
+            break;
+        case 8:
+            full_update |= update_palette256(s);
+            v = VGA_DRAW_LINE8;
+            bits = 8;
+            break;
+        case 15:
+            v = s->big_endian_fb ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
+            bits = 16;
+            break;
+        case 16:
+            v = s->big_endian_fb ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
+            bits = 16;
+            break;
+        case 24:
+            v = s->big_endian_fb ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
+            bits = 24;
+            break;
+        case 32:
+            v = s->big_endian_fb ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
+            bits = 32;
+            break;
         }
     }
 
@@ -1607,53 +1649,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         }
     }
 
-    if (shift_control == 0) {
-        full_update |= update_palette16(s);
-        if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
-            v = VGA_DRAW_LINE4D2;
-        } else {
-            v = VGA_DRAW_LINE4;
-        }
-        bits = 4;
-    } else if (shift_control == 1) {
-        full_update |= update_palette16(s);
-        if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) {
-            v = VGA_DRAW_LINE2D2;
-        } else {
-            v = VGA_DRAW_LINE2;
-        }
-        bits = 4;
-    } else {
-        switch(s->get_bpp(s)) {
-        default:
-        case 0:
-            full_update |= update_palette256(s);
-            v = VGA_DRAW_LINE8D2;
-            bits = 4;
-            break;
-        case 8:
-            full_update |= update_palette256(s);
-            v = VGA_DRAW_LINE8;
-            bits = 8;
-            break;
-        case 15:
-            v = s->big_endian_fb ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
-            bits = 16;
-            break;
-        case 16:
-            v = s->big_endian_fb ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
-            bits = 16;
-            break;
-        case 24:
-            v = s->big_endian_fb ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
-            bits = 24;
-            break;
-        case 32:
-            v = s->big_endian_fb ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
-            bits = 32;
-            break;
-        }
-    }
     vga_draw_line = vga_draw_line_table[v];
 
     if (!is_buffer_shared(surface) && s->cursor_invalidate) {
-- 
2.44.0

[PATCH for-9.0 2/4] vga: move computation of dirty memory region later

[Paolo Bonzini]

Move the computation of region_start and region_end after the value of
"bits" is known.  This makes it possible to distinguish modes that
support horizontal pel panning from modes that do not.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 50 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 4795a0012e2..b4ceff70eb8 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1501,31 +1501,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     disp_width = width;
     depth = s->get_bpp(s);
 
-    region_start = (s->params.start_addr * 4);
-    region_end = region_start + (ram_addr_t)s->params.line_offset * height;
-    region_end += width * depth / 8; /* scanline length */
-    region_end -= s->params.line_offset;
-    if (region_end > s->vbe_size || depth == 0 || depth == 15) {
-        /*
-         * We land here on:
-         *  - wraps around (can happen with cirrus vbe modes)
-         *  - depth == 0 (256 color palette video mode)
-         *  - depth == 15
-         *
-         * Take the safe and slow route:
-         *   - create a dirty bitmap snapshot for all vga memory.
-         *   - force shadowing (so all vga memory access goes
-         *     through vga_read_*() helpers).
-         *
-         * Given this affects only vga features which are pretty much
-         * unused by modern guests there should be no performance
-         * impact.
-         */
-        region_start = 0;
-        region_end = s->vbe_size;
-        force_shadow = true;
-    }
-
     /* bits 5-6: 0 = 16-color mode, 1 = 4-color mode, 2 = 256-color mode.  */
     shift_control = (s->gr[VGA_GFX_MODE] >> 5) & 3;
     double_scan = (s->cr[VGA_CRTC_MAX_SCAN] >> 7);
@@ -1597,6 +1572,31 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         }
     }
 
+    region_start = (s->params.start_addr * 4);
+    region_end = region_start + (ram_addr_t)s->params.line_offset * height;
+    region_end += width * depth / 8; /* scanline length */
+    region_end -= s->params.line_offset;
+    if (region_end > s->vbe_size || depth == 0 || depth == 15) {
+        /*
+         * We land here on:
+         *  - wraps around (can happen with cirrus vbe modes)
+         *  - depth == 0 (256 color palette video mode)
+         *  - depth == 15
+         *
+         * Take the safe and slow route:
+         *   - create a dirty bitmap snapshot for all vga memory.
+         *   - force shadowing (so all vga memory access goes
+         *     through vga_read_*() helpers).
+         *
+         * Given this affects only vga features which are pretty much
+         * unused by modern guests there should be no performance
+         * impact.
+         */
+        region_start = 0;
+        region_end = s->vbe_size;
+        force_shadow = true;
+    }
+
     /*
      * Check whether we can share the surface with the backend
      * or whether we need a shadow surface. We share native
-- 
2.44.0

[PATCH for-9.0 3/4] vga: adjust dirty memory region if pel panning is active

[Paolo Bonzini]

When pel panning is active, one more byte is read from each of the VGA
memory planes.  This has to be accounted in the computation of region_end,
otherwise vga_draw_graphic() fails an assertion:

qemu-system-i386: ../system/physmem.c:946: cpu_physical_memory_snapshot_get_dirty: Assertion `start + length <= snap->end' failed.

Reported-by: Helge Konetzka <hk@zapateado.de>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2244
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index b4ceff70eb8..40acd19e72a 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1571,11 +1571,15 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             break;
         }
     }
+    hpel = bits <= 8 ? s->params.hpel : 0;
 
     region_start = (s->params.start_addr * 4);
     region_end = region_start + (ram_addr_t)s->params.line_offset * height;
     region_end += width * depth / 8; /* scanline length */
     region_end -= s->params.line_offset;
+    if (hpel) {
+        region_end += 4;
+    }
     if (region_end > s->vbe_size || depth == 0 || depth == 15) {
         /*
          * We land here on:
@@ -1660,7 +1664,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
            width, height, v, line_offset, s->cr[9], s->cr[VGA_CRTC_MODE],
            s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
 #endif
-    hpel = bits <= 8 ? s->params.hpel : 0;
     addr1 = (s->params.start_addr * 4);
     bwidth = DIV_ROUND_UP(width * bits, 8);
     if (hpel) {
-- 
2.44.0

[PATCH for-9.0 4/4] vga: do not treat horiz pel panning value of 8 as "enabled"

[Paolo Bonzini]

Horizontal pel panning bit 3 is only used in text mode.  In graphics
mode, it can be treated as if it was zero, thus not extending the
dirty memory region.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 40acd19e72a..77f59e8c113 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1571,7 +1571,9 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             break;
         }
     }
-    hpel = bits <= 8 ? s->params.hpel : 0;
+
+    /* Horizontal pel panning bit 3 is only used in text mode.  */
+    hpel = bits <= 8 ? s->params.hpel & 7 : 0;
 
     region_start = (s->params.start_addr * 4);
     region_end = region_start + (ram_addr_t)s->params.line_offset * height;
-- 
2.44.0

[PATCH for-9.1 5/4] vga: optimize computation of dirty memory region

[Paolo Bonzini]

The depth == 0 and depth == 15 have to be special cased because
width * depth / 8 does not provide the correct scanline length.
However, thanks to the recent reorganization of vga_draw_graphic()
the correct value of VRAM bits per pixel is available in "bits".

Use it (via the same "bwidth" computation that is used later in
the function, and that already takes into account pel panning),
so that the slow path is restricted to the wraparound case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 77f59e8c113..77d709a3d69 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1574,22 +1574,16 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
 
     /* Horizontal pel panning bit 3 is only used in text mode.  */
     hpel = bits <= 8 ? s->params.hpel & 7 : 0;
+    bwidth = DIV_ROUND_UP(width * bits, 8); /* scanline length */
+    if (hpel) {
+        bwidth += 4;
+    }
 
     region_start = (s->params.start_addr * 4);
-    region_end = region_start + (ram_addr_t)s->params.line_offset * height;
-    region_end += width * depth / 8; /* scanline length */
-    region_end -= s->params.line_offset;
-    if (hpel) {
-        region_end += 4;
-    }
-    if (region_end > s->vbe_size || depth == 0 || depth == 15) {
+    region_end = region_start + (ram_addr_t)s->params.line_offset * (height - 1) + bwidth;
+    if (region_end > s->vbe_size) {
         /*
-         * We land here on:
-         *  - wraps around (can happen with cirrus vbe modes)
-         *  - depth == 0 (256 color palette video mode)
-         *  - depth == 15
-         *
-         * Take the safe and slow route:
+         * On wrap around take the safe and slow route:
          *   - create a dirty bitmap snapshot for all vga memory.
          *   - force shadowing (so all vga memory access goes
          *     through vga_read_*() helpers).
@@ -1667,10 +1661,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
            s->params.line_compare, sr(s, VGA_SEQ_CLOCK_MODE));
 #endif
     addr1 = (s->params.start_addr * 4);
-    bwidth = DIV_ROUND_UP(width * bits, 8);
-    if (hpel) {
-        bwidth += 4;
-    }
     y_start = -1;
     d = surface_data(surface);
     linesize = surface_stride(surface);
-- 
2.44.0

[PATCH for-9.1 6/4] vga: move dirty memory region code together

[Paolo Bonzini]

Take into account split screen mode close to wrap around, which is the
other special case for dirty memory region computation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/vga.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 77d709a3d69..e91a76bf76b 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1596,6 +1596,10 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         region_end = s->vbe_size;
         force_shadow = true;
     }
+    if (s->params.line_compare < height) {
+        /* split screen mode */
+        region_start = 0;
+    }
 
     /*
      * Check whether we can share the surface with the backend
@@ -1667,10 +1671,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     y1 = 0;
 
     if (!full_update) {
-        if (s->params.line_compare < height) {
-            /* split screen mode */
-            region_start = 0;
-        }
         snap = memory_region_snapshot_and_clear_dirty(&s->vram, region_start,
                                                       region_end - region_start,
                                                       DIRTY_MEMORY_VGA);
-- 
2.44.0

Re: [PATCH for-9.0 1/4] vga: merge conditionals on shift control register

[Philippe Mathieu-Daudé]

On 2/4/24 13:34, Paolo Bonzini wrote:
> There are two sets of conditionals using the shift control bits: one to
> verify the palette and adjust disp_width, one to compute the "v" and
> "bits" variables.  Merge them into one, with the extra benefit that
> we now have the "bits" value available early and can use it to
> compute region_end.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/display/vga.c | 89 +++++++++++++++++++++++-------------------------
>   1 file changed, 42 insertions(+), 47 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-9.0 2/4] vga: move computation of dirty memory region later

[Philippe Mathieu-Daudé]

On 2/4/24 13:34, Paolo Bonzini wrote:
> Move the computation of region_start and region_end after the value of
> "bits" is known.  This makes it possible to distinguish modes that
> support horizontal pel panning from modes that do not.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/display/vga.c | 50 ++++++++++++++++++++++++------------------------
>   1 file changed, 25 insertions(+), 25 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-9.0 3/4] vga: adjust dirty memory region if pel panning is active

[Philippe Mathieu-Daudé]

On 2/4/24 13:34, Paolo Bonzini wrote:
> When pel panning is active, one more byte is read from each of the VGA
> memory planes.  This has to be accounted in the computation of region_end,
> otherwise vga_draw_graphic() fails an assertion:
> 
> qemu-system-i386: ../system/physmem.c:946: cpu_physical_memory_snapshot_get_dirty: Assertion `start + length <= snap->end' failed.
> 
> Reported-by: Helge Konetzka <hk@zapateado.de>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2244
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/display/vga.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>