[PATCH v6 0/8] ati-vga fixes

[PATCH v6 0/8] ati-vga fixes

[BALATON Zoltan]

OK I think these are all the fixes I'd like to add for 11.0 unless
some more bugs are found. Could you please check and merge?

v6:
- revert part of mouse pointer fix patch from v5 as it broke MacOS;
since I'm not sure the Linux image has no issues on real machine I'm
not trying to fix that now

v5:
- fix typo on Patch 6
- add more patches to fix mouse pointer with big endian frame buffer

v4:
- compile fix in patch 5
- new patch 6 to work around fuloong2e issue:
https://lists.nongnu.org/archive/html/qemu-devel/2026-01/msg06308.html
v3:
- fix patch 1 to work with 8 and 16 bit modes too
- new patch 5 to fix display updates with 8 and 16 bit modes
v2:
- fix patch 4 to really avoid warnings

These are some fixes to ati-vga for 11.0.

Patch 1 fixes problem reported in
https://lists.nongnu.org/archive/html/qemu-ppc/2026-01/msg00105.html

Patch 2 fixes output from Solaris install iso
Rendering still not always correct as it uses some unimplemented
features but it's now readable. I did not test full install though.

Patch 3 fixes vram overrun errors seen with Solaris and MorphOS when
crtc offset is non-zero (this happens on MorphOS when additional
screen is opened)

Patch 4 tries to avoid Coverity warning matching what other places
already do

Patch 5 fixes dirty region tracking when guest display depth is not
the same as the host's

Patch 6 adds work around for fuloong2e to keep working with partial
machine model

BALATON Zoltan (8):
  ati-vga: Fix colors when frame buffer endianness does not match host
  ati-vga: Also switch mode on HW cursor enable bit change
  ati-vga: Do not add crtc offset to src and dst data address
  ati-vga: Avoid warnings about sign extension
  ati-vga: Fix display updates in non-32 bit modes
  ati-vga: Add work around for fuloong2e
  ati-vga: Simplify pointer image handling
  ati-vga: Make sure hardware cursor data is within vram

 hw/display/ati.c     | 85 +++++++++++++++++++++++++-------------------
 hw/display/ati_2d.c  | 47 ++++++++++++++++--------
 hw/display/ati_int.h |  1 +
 hw/mips/fuloong2e.c  |  1 +
 4 files changed, 82 insertions(+), 52 deletions(-)

-- 
2.41.3

[PATCH v6 1/8] ati-vga: Fix colors when frame buffer endianness does not match host

[BALATON Zoltan]

When writing pixels we have to take into account if the frame buffer
endianness matches the host endianness or we need to swap to correct
endianness. This caused wrong colors e.g. with PPC Linux guest that
uses big endian frame buffer when running on little endian host.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: Chad Jablonski <chad@jablonski.xyz>
Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati_2d.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 37fe6c17ee..0cbbdc33f4 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -50,6 +50,7 @@ typedef struct {
     bool host_data_active;
     bool left_to_right;
     bool top_to_bottom;
+    bool need_swap;
     uint32_t frgd_clr;
     const uint8_t *palette;
     const uint8_t *vram_end;
@@ -89,6 +90,7 @@ static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
     ctx->host_data_active = s->host_data.active;
     ctx->left_to_right = s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT;
     ctx->top_to_bottom = s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM;
+    ctx->need_swap = HOST_BIG_ENDIAN != s->vga.big_endian_fb ? true : false;
     ctx->frgd_clr = s->regs.dp_brush_frgd_clr;
     ctx->palette = s->vga.palette;
     ctx->dst_offset = s->regs.dst_offset;
@@ -131,6 +133,17 @@ static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
             (ctx->top_to_bottom ? 'v' : '^'));
 }
 
+static uint32_t make_filler(int bpp, uint32_t color)
+{
+    if (bpp < 24) {
+        color |= color << 16;
+        if (bpp < 15) {
+            color |= color << 8;
+        }
+    }
+    return color;
+}
+
 static bool ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
 {
     QemuRect vis_src, vis_dst;
@@ -255,7 +268,7 @@ static bool ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
 
         switch (ctx->rop3) {
         case ROP3_PATCOPY:
-            filler = ctx->frgd_clr;
+            filler = make_filler(ctx->bpp, ctx->frgd_clr);
             break;
         case ROP3_BLACKNESS:
             filler = 0xffUL << 24 | rgb_to_pixel32(ctx->palette[0],
@@ -268,10 +281,12 @@ static bool ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
                                                    ctx->palette[5]);
             break;
         }
-
         DPRINTF("pixman_fill(%p, %ld, %d, %d, %d, %d, %d, %x)\n",
                 ctx->dst_bits, ctx->dst_stride / sizeof(uint32_t), ctx->bpp,
                 vis_dst.x, vis_dst.y, vis_dst.width, vis_dst.height, filler);
+        if (ctx->need_swap) {
+            bswap32s(&filler);
+        }
 #ifdef CONFIG_PIXMAN
         if (!(use_pixman & BIT(0)) ||
             !pixman_fill((uint32_t *)ctx->dst_bits,
@@ -325,11 +340,8 @@ void ati_2d_blt(ATIVGAState *s)
 bool ati_host_data_flush(ATIVGAState *s)
 {
     ATI2DCtx ctx, chunk;
-    uint32_t fg = s->regs.dp_src_frgd_clr;
-    uint32_t bg = s->regs.dp_src_bkgd_clr;
     unsigned bypp, pix_count, row, col, idx;
     uint8_t pix_buf[ATI_HOST_DATA_ACC_BITS * sizeof(uint32_t)];
-    uint32_t byte_pix_order = s->regs.dp_datatype & DP_BYTE_PIX_ORDER;
     uint32_t src_source = s->regs.dp_mix & DP_SRC_SOURCE;
     uint32_t src_datatype = s->regs.dp_datatype & DP_SRC_DATATYPE;
 
@@ -360,21 +372,27 @@ bool ati_host_data_flush(ATIVGAState *s)
     }
 
     bypp = ctx.bpp / 8;
-
+    pix_count = ATI_HOST_DATA_ACC_BITS;
     if (src_datatype == SRC_COLOR) {
-        pix_count = ATI_HOST_DATA_ACC_BITS / ctx.bpp;
-        memcpy(pix_buf, &s->host_data.acc[0], sizeof(s->host_data.acc));
+        pix_count /= ctx.bpp;
+        memcpy(pix_buf, s->host_data.acc, sizeof(s->host_data.acc));
     } else {
-        pix_count = ATI_HOST_DATA_ACC_BITS;
         /* Expand monochrome bits to color pixels */
+        uint32_t byte_pix_order = s->regs.dp_datatype & DP_BYTE_PIX_ORDER;
+        uint32_t fg = make_filler(ctx.bpp, s->regs.dp_src_frgd_clr);
+        uint32_t bg = make_filler(ctx.bpp, s->regs.dp_src_bkgd_clr);
+
+        if (ctx.need_swap) {
+            bswap32s(&fg);
+            bswap32s(&bg);
+        }
         idx = 0;
         for (int word = 0; word < 4; word++) {
             for (int byte = 0; byte < 4; byte++) {
                 uint8_t byte_val = s->host_data.acc[word] >> (byte * 8);
                 for (int i = 0; i < 8; i++) {
                     bool is_fg = byte_val & BIT(byte_pix_order ? i : 7 - i);
-                    uint32_t color = is_fg ? fg : bg;
-                    stn_he_p(&pix_buf[idx], bypp, color);
+                    stn_he_p(&pix_buf[idx], bypp, is_fg ? fg : bg);
                     idx += bypp;
                 }
             }
-- 
2.41.3

[PATCH v6 2/8] ati-vga: Also switch mode on HW cursor enable bit change

[BALATON Zoltan]

This does nothing for most drivers but works around issue and fixes
output with the Solaris R128 driver that only sets display parameters
after enabling CRT controller which we would miss otherwise.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: Chad Jablonski <chad@jablonski.xyz>
Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index c165434938..8286f67c1c 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -632,6 +632,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
         ati_reg_write_offs(&s->regs.crtc_gen_cntl,
                            addr - CRTC_GEN_CNTL, data, size);
         if ((val & CRTC2_CUR_EN) != (s->regs.crtc_gen_cntl & CRTC2_CUR_EN)) {
+            ati_vga_switch_mode(s);
             if (s->cursor_guest_mode) {
                 s->vga.force_shadow = !!(s->regs.crtc_gen_cntl & CRTC2_CUR_EN);
             } else {
-- 
2.41.3

[PATCH v6 3/8] ati-vga: Do not add crtc offset to src and dst data address

[BALATON Zoltan]

Drivers seem to program these registers with values that already
include the crtc offset so this is not needed. This fixes blit outside
of vram errors with non-0 crtc offset.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati_2d.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 0cbbdc33f4..cf2d4a08e2 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -110,7 +110,6 @@ static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
     ctx->dst_stride = s->regs.dst_pitch;
     ctx->dst_bits = s->vga.vram_ptr + s->regs.dst_offset;
     if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
-        ctx->dst_bits += s->regs.crtc_offset & 0x07ffffff;
         ctx->dst_stride *= ctx->bpp;
     }
 
@@ -121,7 +120,6 @@ static void setup_2d_blt_ctx(const ATIVGAState *s, ATI2DCtx *ctx)
     ctx->src_stride = s->regs.src_pitch;
     ctx->src_bits = s->vga.vram_ptr + s->regs.src_offset;
     if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
-        ctx->src_bits += s->regs.crtc_offset & 0x07ffffff;
         ctx->src_stride *= ctx->bpp;
     }
     DPRINTF("%d %d %d, %d %d %d, (%d,%d) -> (%d,%d) %dx%d %c %c\n",
-- 
2.41.3

[PATCH v6 4/8] ati-vga: Avoid warnings about sign extension

[BALATON Zoltan]

Coverity reports several possible sign extension errors (latest is CID
1645615). These cannot happen because the values are limited when
writing the registers and only 32 bits of the return value matter but
change type of the variable storing the return value to uint32_t to
avoid these warnings. Also change DEFAULT_SC_BOTTOM_RIGHT register
read to match what other similar registers do for consistency.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/ati.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 8286f67c1c..705e4db6e4 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -266,7 +266,7 @@ static void ati_vga_vblank_irq(void *opaque)
     ati_vga_update_irq(s);
 }
 
-static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
+static inline uint32_t ati_reg_read_offs(uint32_t reg, int offs,
                                          unsigned int size)
 {
     if (offs == 0 && size == 4) {
@@ -279,7 +279,7 @@ static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
 static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
 {
     ATIVGAState *s = opaque;
-    uint64_t val = 0;
+    uint32_t val = 0;
 
     switch (addr) {
     case MM_INDEX:
@@ -514,8 +514,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
         val |= s->regs.default_tile << 16;
         break;
     case DEFAULT_SC_BOTTOM_RIGHT:
-        val = (s->regs.default_sc_bottom << 16) |
-              s->regs.default_sc_right;
+        val = s->regs.default_sc_right;
+        val |= s->regs.default_sc_bottom << 16;
         break;
     case SC_TOP:
         val = s->regs.sc_top;
-- 
2.41.3

[PATCH v6 5/8] ati-vga: Fix display updates in non-32 bit modes

[BALATON Zoltan]

The memory_region_set_dirty used to mark changes should use stride
value in vram which is normally only the same as surface_stride in 32
bit modes. This caused missed updates in 8 and 16 bit modes.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati_2d.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index cf2d4a08e2..23527b2c50 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -70,6 +70,7 @@ static void ati_set_dirty(VGACommonState *vga, const ATI2DCtx *ctx)
 {
     DisplaySurface *ds = qemu_console_surface(vga->con);
 
+    (void)ds;
     DPRINTF("%p %u ds: %p %d %d rop: %x\n", vga->vram_ptr, vga->vbe_start_addr,
             surface_data(ds), surface_stride(ds), surface_bits_per_pixel(ds),
             ctx->rop3 >> 16);
@@ -78,8 +79,8 @@ static void ati_set_dirty(VGACommonState *vga, const ATI2DCtx *ctx)
         vga->vbe_regs[VBE_DISPI_INDEX_YRES] * vga->vbe_line_offset) {
         memory_region_set_dirty(&vga->vram,
                                 vga->vbe_start_addr + ctx->dst_offset +
-                                ctx->dst.y * surface_stride(ds),
-                                ctx->dst.height * surface_stride(ds));
+                                ctx->dst.y * ctx->dst_stride,
+                                ctx->dst.height * ctx->dst_stride);
     }
 }
 
-- 
2.41.3

[PATCH v6 6/8] ati-vga: Add work around for fuloong2e

[BALATON Zoltan]

With the linear aperture size fixed to match real card fuloong2e no
longer works due to running out of PCI memory because only one PCI bus
is emulated on that machine. Add a property to allow fuloong2e to set
a smaller linear aperture size to work around that problem until the
machine model is improved.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati.c     | 17 +++++++++++++----
 hw/display/ati_int.h |  1 +
 hw/mips/fuloong2e.c  |  1 +
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 705e4db6e4..fba73a9956 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -1074,7 +1074,6 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     ATIVGAState *s = ATI_VGA(dev);
     VGACommonState *vga = &s->vga;
     I2CBus *i2cbus;
-    uint64_t aper_size;
 
 #ifndef CONFIG_PIXMAN
     if (s->use_pixman != 0) {
@@ -1138,10 +1137,19 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
      * Rage128 the upper half of the aperture is reserved for an AGP
      * window (which we do not emulate.)
      */
-    aper_size = s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
-                ATI_RAGE128_LINEAR_APER_SIZE : ATI_R100_LINEAR_APER_SIZE;
+    if (!s->linear_aper_sz) {
+        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            s->linear_aper_sz = ATI_RAGE128_LINEAR_APER_SIZE;
+        } else {
+            s->linear_aper_sz = ATI_R100_LINEAR_APER_SIZE;
+        }
+    }
+    if (s->linear_aper_sz < 16 * MiB) {
+        error_setg(errp, "x-linear-aper-size is too small (minimum 16 MiB)");
+        return;
+    }
     memory_region_init(&s->linear_aper, OBJECT(dev), "ati-linear-aperture0",
-                       aper_size);
+                       s->linear_aper_sz);
     memory_region_add_subregion(&s->linear_aper, 0, &vga->vram);
 
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->linear_aper);
@@ -1186,6 +1194,7 @@ static const Property ati_vga_properties[] = {
     DEFINE_PROP_BOOL("guest_hwcursor", ATIVGAState, cursor_guest_mode, false),
     /* this is a debug option, prefer PROP_UINT over PROP_BIT for simplicity */
     DEFINE_PROP_UINT8("x-pixman", ATIVGAState, use_pixman, DEFAULT_X_PIXMAN),
+    DEFINE_PROP_UINT64("x-linear-aper-size", ATIVGAState, linear_aper_sz, 0),
     DEFINE_EDID_PROPERTIES(ATIVGAState, i2cddc.edid_info),
 };
 
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 21b74511e0..0c48934d33 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -119,6 +119,7 @@ struct ATIVGAState {
     QEMUTimer vblank_timer;
     bitbang_i2c_interface bbi2c;
     I2CDDCState i2cddc;
+    uint64_t linear_aper_sz;
     MemoryRegion linear_aper;
     MemoryRegion io;
     MemoryRegion mm;
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index d0efe36f7c..72ad4507df 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -316,6 +316,7 @@ static void mips_fuloong2e_init(MachineState *machine)
         dev = DEVICE(pci_dev);
         qdev_prop_set_uint32(dev, "vgamem_mb", 16);
         qdev_prop_set_uint16(dev, "x-device-id", 0x5159);
+        qdev_prop_set_uint64(dev, "x-linear-aper-size", 16 * MiB);
         pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
     }
 
-- 
2.41.3

[PATCH v6 7/8] ati-vga: Simplify pointer image handling

[BALATON Zoltan]

Rewrite reading of mouse pointer image. I am not sure this is entirely
correct but appears to work at least on little endian host with PPC
guests using little or big endian frame buffer (MorphOS and MacOS) but
still produces broken pointer image with Linux where I am not sure if
it is a guest driver bug or still missing something.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati.c | 53 ++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index fba73a9956..e1616dc3b3 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -142,27 +142,24 @@ static void ati_vga_switch_mode(ATIVGAState *s)
 /* Used by host side hardware cursor */
 static void ati_cursor_define(ATIVGAState *s)
 {
-    uint8_t data[1024];
+    uint64_t data[128];
     uint32_t srcoff;
-    int i, j, idx = 0;
 
     if ((s->regs.cur_offset & BIT(31)) || s->cursor_guest_mode) {
         return; /* Do not update cursor if locked or rendered by guest */
     }
     /* FIXME handle cur_hv_offs correctly */
-    srcoff = s->regs.cur_offset -
-        (s->regs.cur_hv_offs >> 16) - (s->regs.cur_hv_offs & 0xffff) * 16;
-    for (i = 0; i < 64; i++) {
-        for (j = 0; j < 8; j++, idx++) {
-            data[idx] = vga_read_byte(&s->vga, srcoff + i * 16 + j);
-            data[512 + idx] = vga_read_byte(&s->vga, srcoff + i * 16 + j + 8);
-        }
+    srcoff = s->regs.cur_offset - (s->regs.cur_hv_offs >> 16) -
+             (s->regs.cur_hv_offs & 0xffff) * 16;
+    for (int i = 0; i < 64; i++, srcoff += 16) {
+        data[i] = ldq_le_p(&s->vga.vram_ptr[srcoff]);
+        data[i + 64] = ldq_le_p(&s->vga.vram_ptr[srcoff + 8]);
     }
     if (!s->cursor) {
         s->cursor = cursor_alloc(64, 64);
     }
     cursor_set_mono(s->cursor, s->regs.cur_color1, s->regs.cur_color0,
-                    &data[512], 1, &data[0]);
+                    (uint8_t *)&data[64], 1, (uint8_t *)&data[0]);
     dpy_cursor_define(s->vga.con, s->cursor);
 }
 
@@ -197,9 +194,9 @@ static void ati_cursor_invalidate(VGACommonState *vga)
 static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
 {
     ATIVGAState *s = container_of(vga, ATIVGAState, vga);
-    uint32_t srcoff;
+    uint32_t h, srcoff, color;
+    uint64_t abits, xbits, mask;
     uint32_t *dp = (uint32_t *)d;
-    int i, j, h, idx = 0;
 
     if (!(s->regs.crtc_gen_cntl & CRTC2_CUR_EN) ||
         scr_y < vga->hw_cursor_y || scr_y >= vga->hw_cursor_y + 64 ||
@@ -210,26 +207,24 @@ static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
     srcoff = s->cursor_offset + (scr_y - vga->hw_cursor_y) * 16;
     dp = &dp[vga->hw_cursor_x];
     h = ((s->regs.crtc_h_total_disp >> 16) + 1) * 8;
-    for (i = 0; i < 8; i++) {
-        uint32_t color;
-        uint8_t abits = vga_read_byte(vga, srcoff + i);
-        uint8_t xbits = vga_read_byte(vga, srcoff + i + 8);
-        for (j = 0; j < 8; j++, abits <<= 1, xbits <<= 1, idx++) {
-            if (vga->hw_cursor_x + idx >= h) {
-                return; /* end of screen, don't span to next line */
-            }
-            if (abits & BIT(7)) {
-                if (xbits & BIT(7)) {
-                    color = dp[idx] ^ 0xffffffff; /* complement */
-                } else {
-                    continue; /* transparent, no change */
-                }
+    abits = ldq_be_p(&vga->vram_ptr[srcoff]);
+    xbits = ldq_be_p(&vga->vram_ptr[srcoff + 8]);
+    mask = BIT(63);
+    for (int i = 0; i < 64; i++, mask >>= 1) {
+        if (vga->hw_cursor_x + i >= h) {
+            return; /* end of screen, don't span to next line */
+        }
+        if (abits & mask) {
+            if (xbits & mask) {
+                color = dp[i] ^ 0xffffffff; /* complement */
             } else {
-                color = (xbits & BIT(7) ? s->regs.cur_color1 :
-                                          s->regs.cur_color0) | 0xff000000;
+                continue; /* transparent, no change */
             }
-            dp[idx] = color;
+        } else {
+            color = (xbits & mask ? s->regs.cur_color1 :
+                                    s->regs.cur_color0) | 0xff000000;
         }
+        dp[i] = color;
     }
 }
 
-- 
2.41.3

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[Philippe Mathieu-Daudé]

On 21/3/26 17:30, BALATON Zoltan wrote:
> Rewrite reading of mouse pointer image. I am not sure this is entirely
> correct but appears to work at least on little endian host with PPC
> guests using little or big endian frame buffer (MorphOS and MacOS) but
> still produces broken pointer image with Linux where I am not sure if
> it is a guest driver bug or still missing something.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/display/ati.c | 53 ++++++++++++++++++++++--------------------------
>   1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index fba73a9956..e1616dc3b3 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -142,27 +142,24 @@ static void ati_vga_switch_mode(ATIVGAState *s)
>   /* Used by host side hardware cursor */
>   static void ati_cursor_define(ATIVGAState *s)
>   {
> -    uint8_t data[1024];
> +    uint64_t data[128];
>       uint32_t srcoff;
> -    int i, j, idx = 0;
>   
>       if ((s->regs.cur_offset & BIT(31)) || s->cursor_guest_mode) {
>           return; /* Do not update cursor if locked or rendered by guest */
>       }
>       /* FIXME handle cur_hv_offs correctly */
> -    srcoff = s->regs.cur_offset -
> -        (s->regs.cur_hv_offs >> 16) - (s->regs.cur_hv_offs & 0xffff) * 16;
> -    for (i = 0; i < 64; i++) {
> -        for (j = 0; j < 8; j++, idx++) {
> -            data[idx] = vga_read_byte(&s->vga, srcoff + i * 16 + j);
> -            data[512 + idx] = vga_read_byte(&s->vga, srcoff + i * 16 + j + 8);
> -        }
> +    srcoff = s->regs.cur_offset - (s->regs.cur_hv_offs >> 16) -
> +             (s->regs.cur_hv_offs & 0xffff) * 16;
> +    for (int i = 0; i < 64; i++, srcoff += 16) {
> +        data[i] = ldq_le_p(&s->vga.vram_ptr[srcoff]);
> +        data[i + 64] = ldq_le_p(&s->vga.vram_ptr[srcoff + 8]);
>       }
>       if (!s->cursor) {
>           s->cursor = cursor_alloc(64, 64);
>       }
>       cursor_set_mono(s->cursor, s->regs.cur_color1, s->regs.cur_color0,
> -                    &data[512], 1, &data[0]);
> +                    (uint8_t *)&data[64], 1, (uint8_t *)&data[0]);
>       dpy_cursor_define(s->vga.con, s->cursor);
>   }
>   
> @@ -197,9 +194,9 @@ static void ati_cursor_invalidate(VGACommonState *vga)
>   static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
>   {
>       ATIVGAState *s = container_of(vga, ATIVGAState, vga);
> -    uint32_t srcoff;
> +    uint32_t h, srcoff, color;
> +    uint64_t abits, xbits, mask;
>       uint32_t *dp = (uint32_t *)d;
> -    int i, j, h, idx = 0;
>   
>       if (!(s->regs.crtc_gen_cntl & CRTC2_CUR_EN) ||
>           scr_y < vga->hw_cursor_y || scr_y >= vga->hw_cursor_y + 64 ||
> @@ -210,26 +207,24 @@ static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
>       srcoff = s->cursor_offset + (scr_y - vga->hw_cursor_y) * 16;
>       dp = &dp[vga->hw_cursor_x];
>       h = ((s->regs.crtc_h_total_disp >> 16) + 1) * 8;
> -    for (i = 0; i < 8; i++) {
> -        uint32_t color;
> -        uint8_t abits = vga_read_byte(vga, srcoff + i);
> -        uint8_t xbits = vga_read_byte(vga, srcoff + i + 8);
> -        for (j = 0; j < 8; j++, abits <<= 1, xbits <<= 1, idx++) {
> -            if (vga->hw_cursor_x + idx >= h) {
> -                return; /* end of screen, don't span to next line */
> -            }
> -            if (abits & BIT(7)) {
> -                if (xbits & BIT(7)) {
> -                    color = dp[idx] ^ 0xffffffff; /* complement */
> -                } else {
> -                    continue; /* transparent, no change */
> -                }
> +    abits = ldq_be_p(&vga->vram_ptr[srcoff]);
> +    xbits = ldq_be_p(&vga->vram_ptr[srcoff + 8]);
> +    mask = BIT(63);

This triggers:

In file included from include/qemu/timer.h:4,
                  from ../hw/display/ati_int.h:12,
                  from ../hw/display/ati.c:20:
../hw/display/ati.c: In function 'ati_cursor_draw_line':
include/qemu/bitops.h:24:38: error: left shift count >= width of type 
[-Werror=shift-count-overflow]
    24 | #define BIT(nr)                 (1UL << (nr))
       |                                      ^~
../hw/display/ati.c:217:12: note: in expansion of macro 'BIT'
   217 |     mask = BIT(63);
       |            ^~~

I suppose we want BIT_ULL() here, I'll test again.

> +    for (int i = 0; i < 64; i++, mask >>= 1) {
> +        if (vga->hw_cursor_x + i >= h) {
> +            return; /* end of screen, don't span to next line */
> +        }
> +        if (abits & mask) {
> +            if (xbits & mask) {
> +                color = dp[i] ^ 0xffffffff; /* complement */
>               } else {
> -                color = (xbits & BIT(7) ? s->regs.cur_color1 :
> -                                          s->regs.cur_color0) | 0xff000000;
> +                continue; /* transparent, no change */
>               }
> -            dp[idx] = color;
> +        } else {
> +            color = (xbits & mask ? s->regs.cur_color1 :
> +                                    s->regs.cur_color0) | 0xff000000;
>           }
> +        dp[i] = color;
>       }
>   }
>   

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[Philippe Mathieu-Daudé]

On 23/3/26 13:21, Philippe Mathieu-Daudé wrote:
> On 21/3/26 17:30, BALATON Zoltan wrote:
>> Rewrite reading of mouse pointer image. I am not sure this is entirely
>> correct but appears to work at least on little endian host with PPC
>> guests using little or big endian frame buffer (MorphOS and MacOS) but
>> still produces broken pointer image with Linux where I am not sure if
>> it is a guest driver bug or still missing something.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/display/ati.c | 53 ++++++++++++++++++++++--------------------------
>>   1 file changed, 24 insertions(+), 29 deletions(-)


>> @@ -197,9 +194,9 @@ static void ati_cursor_invalidate(VGACommonState 
>> *vga)
>>   static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, 
>> int scr_y)
>>   {
>>       ATIVGAState *s = container_of(vga, ATIVGAState, vga);
>> -    uint32_t srcoff;
>> +    uint32_t h, srcoff, color;
>> +    uint64_t abits, xbits, mask;
>>       uint32_t *dp = (uint32_t *)d;
>> -    int i, j, h, idx = 0;
>>       if (!(s->regs.crtc_gen_cntl & CRTC2_CUR_EN) ||
>>           scr_y < vga->hw_cursor_y || scr_y >= vga->hw_cursor_y + 64 ||
>> @@ -210,26 +207,24 @@ static void ati_cursor_draw_line(VGACommonState 
>> *vga, uint8_t *d, int scr_y)
>>       srcoff = s->cursor_offset + (scr_y - vga->hw_cursor_y) * 16;
>>       dp = &dp[vga->hw_cursor_x];
>>       h = ((s->regs.crtc_h_total_disp >> 16) + 1) * 8;
>> -    for (i = 0; i < 8; i++) {
>> -        uint32_t color;
>> -        uint8_t abits = vga_read_byte(vga, srcoff + i);
>> -        uint8_t xbits = vga_read_byte(vga, srcoff + i + 8);
>> -        for (j = 0; j < 8; j++, abits <<= 1, xbits <<= 1, idx++) {
>> -            if (vga->hw_cursor_x + idx >= h) {
>> -                return; /* end of screen, don't span to next line */
>> -            }
>> -            if (abits & BIT(7)) {
>> -                if (xbits & BIT(7)) {
>> -                    color = dp[idx] ^ 0xffffffff; /* complement */
>> -                } else {
>> -                    continue; /* transparent, no change */
>> -                }
>> +    abits = ldq_be_p(&vga->vram_ptr[srcoff]);
>> +    xbits = ldq_be_p(&vga->vram_ptr[srcoff + 8]);
>> +    mask = BIT(63);
> 
> This triggers:
> 
> In file included from include/qemu/timer.h:4,
>                   from ../hw/display/ati_int.h:12,
>                   from ../hw/display/ati.c:20:
> ../hw/display/ati.c: In function 'ati_cursor_draw_line':
> include/qemu/bitops.h:24:38: error: left shift count >= width of type [- 
> Werror=shift-count-overflow]
>     24 | #define BIT(nr)                 (1UL << (nr))
>        |                                      ^~
> ../hw/display/ati.c:217:12: note: in expansion of macro 'BIT'
>    217 |     mask = BIT(63);
>        |            ^~~

BTW you can check that yourself by running our CI jobs on GitLab
(https://www.qemu.org/docs/master/devel/testing/ci.html).

This is the output of the 'cross-win64-system' job:
https://gitlab.com/philmd/qemu/-/jobs/13605781786

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[BALATON Zoltan]

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

On Mon, 23 Mar 2026, Philippe Mathieu-Daudé wrote:
> On 23/3/26 13:21, Philippe Mathieu-Daudé wrote:
>> On 21/3/26 17:30, BALATON Zoltan wrote:
>>> Rewrite reading of mouse pointer image. I am not sure this is entirely
>>> correct but appears to work at least on little endian host with PPC
>>> guests using little or big endian frame buffer (MorphOS and MacOS) but
>>> still produces broken pointer image with Linux where I am not sure if
>>> it is a guest driver bug or still missing something.
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/display/ati.c | 53 ++++++++++++++++++++++--------------------------
>>>   1 file changed, 24 insertions(+), 29 deletions(-)
>
>
>>> @@ -197,9 +194,9 @@ static void ati_cursor_invalidate(VGACommonState *vga)
>>>   static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int 
>>> scr_y)
>>>   {
>>>       ATIVGAState *s = container_of(vga, ATIVGAState, vga);
>>> -    uint32_t srcoff;
>>> +    uint32_t h, srcoff, color;
>>> +    uint64_t abits, xbits, mask;
>>>       uint32_t *dp = (uint32_t *)d;
>>> -    int i, j, h, idx = 0;
>>>       if (!(s->regs.crtc_gen_cntl & CRTC2_CUR_EN) ||
>>>           scr_y < vga->hw_cursor_y || scr_y >= vga->hw_cursor_y + 64 ||
>>> @@ -210,26 +207,24 @@ static void ati_cursor_draw_line(VGACommonState 
>>> *vga, uint8_t *d, int scr_y)
>>>       srcoff = s->cursor_offset + (scr_y - vga->hw_cursor_y) * 16;
>>>       dp = &dp[vga->hw_cursor_x];
>>>       h = ((s->regs.crtc_h_total_disp >> 16) + 1) * 8;
>>> -    for (i = 0; i < 8; i++) {
>>> -        uint32_t color;
>>> -        uint8_t abits = vga_read_byte(vga, srcoff + i);
>>> -        uint8_t xbits = vga_read_byte(vga, srcoff + i + 8);
>>> -        for (j = 0; j < 8; j++, abits <<= 1, xbits <<= 1, idx++) {
>>> -            if (vga->hw_cursor_x + idx >= h) {
>>> -                return; /* end of screen, don't span to next line */
>>> -            }
>>> -            if (abits & BIT(7)) {
>>> -                if (xbits & BIT(7)) {
>>> -                    color = dp[idx] ^ 0xffffffff; /* complement */
>>> -                } else {
>>> -                    continue; /* transparent, no change */
>>> -                }
>>> +    abits = ldq_be_p(&vga->vram_ptr[srcoff]);
>>> +    xbits = ldq_be_p(&vga->vram_ptr[srcoff + 8]);
>>> +    mask = BIT(63);
>> 
>> This triggers:
>> 
>> In file included from include/qemu/timer.h:4,
>>                   from ../hw/display/ati_int.h:12,
>>                   from ../hw/display/ati.c:20:
>> ../hw/display/ati.c: In function 'ati_cursor_draw_line':
>> include/qemu/bitops.h:24:38: error: left shift count >= width of type [- 
>> Werror=shift-count-overflow]
>>     24 | #define BIT(nr)                 (1UL << (nr))
>>        |                                      ^~
>> ../hw/display/ati.c:217:12: note: in expansion of macro 'BIT'
>>    217 |     mask = BIT(63);
>>        |            ^~~
>
> BTW you can check that yourself by running our CI jobs on GitLab
> (https://www.qemu.org/docs/master/devel/testing/ci.html).
>
> This is the output of the 'cross-win64-system' job:
> https://gitlab.com/philmd/qemu/-/jobs/13605781786

I find Gitlab to be very inconvenient to use so I avoided it so far. If 
needed I can try but I could do without it.

Regards,
BALATON Zoltan

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[BALATON Zoltan]

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

On Mon, 23 Mar 2026, Philippe Mathieu-Daudé wrote:
> On 21/3/26 17:30, BALATON Zoltan wrote:
>> Rewrite reading of mouse pointer image. I am not sure this is entirely
>> correct but appears to work at least on little endian host with PPC
>> guests using little or big endian frame buffer (MorphOS and MacOS) but
>> still produces broken pointer image with Linux where I am not sure if
>> it is a guest driver bug or still missing something.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/display/ati.c | 53 ++++++++++++++++++++++--------------------------
>>   1 file changed, 24 insertions(+), 29 deletions(-)
>> 
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index fba73a9956..e1616dc3b3 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -142,27 +142,24 @@ static void ati_vga_switch_mode(ATIVGAState *s)
>>   /* Used by host side hardware cursor */
>>   static void ati_cursor_define(ATIVGAState *s)
>>   {
>> -    uint8_t data[1024];
>> +    uint64_t data[128];
>>       uint32_t srcoff;
>> -    int i, j, idx = 0;
>>         if ((s->regs.cur_offset & BIT(31)) || s->cursor_guest_mode) {
>>           return; /* Do not update cursor if locked or rendered by guest */
>>       }
>>       /* FIXME handle cur_hv_offs correctly */
>> -    srcoff = s->regs.cur_offset -
>> -        (s->regs.cur_hv_offs >> 16) - (s->regs.cur_hv_offs & 0xffff) * 16;
>> -    for (i = 0; i < 64; i++) {
>> -        for (j = 0; j < 8; j++, idx++) {
>> -            data[idx] = vga_read_byte(&s->vga, srcoff + i * 16 + j);
>> -            data[512 + idx] = vga_read_byte(&s->vga, srcoff + i * 16 + j + 
>> 8);
>> -        }
>> +    srcoff = s->regs.cur_offset - (s->regs.cur_hv_offs >> 16) -
>> +             (s->regs.cur_hv_offs & 0xffff) * 16;
>> +    for (int i = 0; i < 64; i++, srcoff += 16) {
>> +        data[i] = ldq_le_p(&s->vga.vram_ptr[srcoff]);
>> +        data[i + 64] = ldq_le_p(&s->vga.vram_ptr[srcoff + 8]);
>>       }
>>       if (!s->cursor) {
>>           s->cursor = cursor_alloc(64, 64);
>>       }
>>       cursor_set_mono(s->cursor, s->regs.cur_color1, s->regs.cur_color0,
>> -                    &data[512], 1, &data[0]);
>> +                    (uint8_t *)&data[64], 1, (uint8_t *)&data[0]);
>>       dpy_cursor_define(s->vga.con, s->cursor);
>>   }
>>   @@ -197,9 +194,9 @@ static void ati_cursor_invalidate(VGACommonState 
>> *vga)
>>   static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int 
>> scr_y)
>>   {
>>       ATIVGAState *s = container_of(vga, ATIVGAState, vga);
>> -    uint32_t srcoff;
>> +    uint32_t h, srcoff, color;
>> +    uint64_t abits, xbits, mask;
>>       uint32_t *dp = (uint32_t *)d;
>> -    int i, j, h, idx = 0;
>>         if (!(s->regs.crtc_gen_cntl & CRTC2_CUR_EN) ||
>>           scr_y < vga->hw_cursor_y || scr_y >= vga->hw_cursor_y + 64 ||
>> @@ -210,26 +207,24 @@ static void ati_cursor_draw_line(VGACommonState *vga, 
>> uint8_t *d, int scr_y)
>>       srcoff = s->cursor_offset + (scr_y - vga->hw_cursor_y) * 16;
>>       dp = &dp[vga->hw_cursor_x];
>>       h = ((s->regs.crtc_h_total_disp >> 16) + 1) * 8;
>> -    for (i = 0; i < 8; i++) {
>> -        uint32_t color;
>> -        uint8_t abits = vga_read_byte(vga, srcoff + i);
>> -        uint8_t xbits = vga_read_byte(vga, srcoff + i + 8);
>> -        for (j = 0; j < 8; j++, abits <<= 1, xbits <<= 1, idx++) {
>> -            if (vga->hw_cursor_x + idx >= h) {
>> -                return; /* end of screen, don't span to next line */
>> -            }
>> -            if (abits & BIT(7)) {
>> -                if (xbits & BIT(7)) {
>> -                    color = dp[idx] ^ 0xffffffff; /* complement */
>> -                } else {
>> -                    continue; /* transparent, no change */
>> -                }
>> +    abits = ldq_be_p(&vga->vram_ptr[srcoff]);
>> +    xbits = ldq_be_p(&vga->vram_ptr[srcoff + 8]);
>> +    mask = BIT(63);
>
> This triggers:
>
> In file included from include/qemu/timer.h:4,
>                 from ../hw/display/ati_int.h:12,
>                 from ../hw/display/ati.c:20:
> ../hw/display/ati.c: In function 'ati_cursor_draw_line':
> include/qemu/bitops.h:24:38: error: left shift count >= width of type 
> [-Werror=shift-count-overflow]
>   24 | #define BIT(nr)                 (1UL << (nr))
>      |                                      ^~
> ../hw/display/ati.c:217:12: note: in expansion of macro 'BIT'
>  217 |     mask = BIT(63);
>      |            ^~~
>
> I suppose we want BIT_ULL() here, I'll test again.

Yes should be BIT_ULL, Previously it was only 8 bits and I forgot to 
change the BIT macro and did not get a warning with the compiler I have. 
Sorry for that, I hope you can fix up or drop the last two patches until I 
can update it.

Regards,
BALATON Zoltan

>> +    for (int i = 0; i < 64; i++, mask >>= 1) {
>> +        if (vga->hw_cursor_x + i >= h) {
>> +            return; /* end of screen, don't span to next line */
>> +        }
>> +        if (abits & mask) {
>> +            if (xbits & mask) {
>> +                color = dp[i] ^ 0xffffffff; /* complement */
>>               } else {
>> -                color = (xbits & BIT(7) ? s->regs.cur_color1 :
>> -                                          s->regs.cur_color0) | 
>> 0xff000000;
>> +                continue; /* transparent, no change */
>>               }
>> -            dp[idx] = color;
>> +        } else {
>> +            color = (xbits & mask ? s->regs.cur_color1 :
>> +                                    s->regs.cur_color0) | 0xff000000;
>>           }
>> +        dp[i] = color;
>>       }
>>   }
>> 
>
>

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[Chad Jablonski]

>      /* FIXME handle cur_hv_offs correctly */
> +    srcoff = s->regs.cur_offset - (s->regs.cur_hv_offs >> 16) -
> +             (s->regs.cur_hv_offs & 0xffff) * 16;

This is pre-existing (and a FIXME!) but do we want to be subtracting the
horizontal cursor offset (s->regs.cur_hv_offs >> 16) here? I think when
it's non-zero it would mean that the first row of the cursor would be offset
and rows would contain bytes from the next row at the end. The way I read the
register reference is that the horizontal offset is the number of columns the
cursor is shifted. So I don't think this would affect the source, just how it's drawn.

> +    for (int i = 0; i < 64; i++, srcoff += 16) {
> +        data[i] = ldq_le_p(&s->vga.vram_ptr[srcoff]);
> +        data[i + 64] = ldq_le_p(&s->vga.vram_ptr[srcoff + 8]);

Is this ldq_le_p correct here? Wouldn't this cause issues on BE hosts?

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[BALATON Zoltan]

On Tue, 24 Mar 2026, Chad Jablonski wrote:
>>      /* FIXME handle cur_hv_offs correctly */
>> +    srcoff = s->regs.cur_offset - (s->regs.cur_hv_offs >> 16) -
>> +             (s->regs.cur_hv_offs & 0xffff) * 16;
>
> This is pre-existing (and a FIXME!) but do we want to be subtracting the
> horizontal cursor offset (s->regs.cur_hv_offs >> 16) here? I think when
> it's non-zero it would mean that the first row of the cursor would be offset
> and rows would contain bytes from the next row at the end. The way I read the
> register reference is that the horizontal offset is the number of columns the
> cursor is shifted. So I don't think this would affect the source, just how it's drawn.

As you say it's preexisting and I did not try to fix that and left the 
FIXME comment there. I haven't seen anything using hv_offset so far except 
the Linux test image at the very left and top of the screen but that also 
has other problems with the mouse pointer so unless it's first confirmed 
on real hardware that it's supposed to work I won't try to fix that 
because I tested MacOS that also uses big endian frame buffer and that 
works now so it might be the Linux driver is buggy or there's some other 
swap bit somewhere we should take into account. As nothing else seems to 
use non-0 hv_offset this should not matter for most drivers so until we 
see something that needs it and we can test with I leave it a FIXME.

>> +    for (int i = 0; i < 64; i++, srcoff += 16) {
>> +        data[i] = ldq_le_p(&s->vga.vram_ptr[srcoff]);
>> +        data[i + 64] = ldq_le_p(&s->vga.vram_ptr[srcoff + 8]);
>
> Is this ldq_le_p correct here? Wouldn't this cause issues on BE hosts?

I'm not sure as I noted in commit message. According to the docs I've seen 
hw cursor data should always be little endian and if we pass these on as 
host endian data here then le should be correct but I could not test on 
big endian host so can't say for sure.

Regards,
BALATON Zoltan

Re: [PATCH v6 7/8] ati-vga: Simplify pointer image handling

[Chad Jablonski]

Reviewed-by: Chad Jablonski <chad@jablonski.xyz>

[PATCH v6 8/8] ati-vga: Make sure hardware cursor data is within vram

[BALATON Zoltan]

Add check to make sure we don't read past the end of vram when getting
mouse pointer image.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index e1616dc3b3..d7b131d633 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -151,6 +151,9 @@ static void ati_cursor_define(ATIVGAState *s)
     /* FIXME handle cur_hv_offs correctly */
     srcoff = s->regs.cur_offset - (s->regs.cur_hv_offs >> 16) -
              (s->regs.cur_hv_offs & 0xffff) * 16;
+    if (srcoff + 64 * 16 > s->vga.vram_size) {
+        return;
+    }
     for (int i = 0; i < 64; i++, srcoff += 16) {
         data[i] = ldq_le_p(&s->vga.vram_ptr[srcoff]);
         data[i + 64] = ldq_le_p(&s->vga.vram_ptr[srcoff + 8]);
@@ -205,6 +208,9 @@ static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
     }
     /* FIXME handle cur_hv_offs correctly */
     srcoff = s->cursor_offset + (scr_y - vga->hw_cursor_y) * 16;
+    if (srcoff + 16 > s->vga.vram_size) {
+        return;
+    }
     dp = &dp[vga->hw_cursor_x];
     h = ((s->regs.crtc_h_total_disp >> 16) + 1) * 8;
     abits = ldq_be_p(&vga->vram_ptr[srcoff]);
-- 
2.41.3

Re: [PATCH v6 8/8] ati-vga: Make sure hardware cursor data is within vram

[Chad Jablonski]

Reviewed-by: Chad Jablonski <chad@jablonski.xyz>

Re: [PATCH v6 0/8] ati-vga fixes

[BALATON Zoltan]

On Sat, 21 Mar 2026, BALATON Zoltan wrote:
> OK I think these are all the fixes I'd like to add for 11.0 unless
> some more bugs are found. Could you please check and merge?

Philippe are you queueing these too for 11.0? The last ati-vga patches 
went through your tree because of fuloong2e and I don't think there's 
anybody else who maintains hw/display.

(Last too patches don't yet have review but you could take the rest at 
least.)

Regards,
BALATON Zoltan

> v6:
> - revert part of mouse pointer fix patch from v5 as it broke MacOS;
> since I'm not sure the Linux image has no issues on real machine I'm
> not trying to fix that now
>
> v5:
> - fix typo on Patch 6
> - add more patches to fix mouse pointer with big endian frame buffer
>
> v4:
> - compile fix in patch 5
> - new patch 6 to work around fuloong2e issue:
> https://lists.nongnu.org/archive/html/qemu-devel/2026-01/msg06308.html
> v3:
> - fix patch 1 to work with 8 and 16 bit modes too
> - new patch 5 to fix display updates with 8 and 16 bit modes
> v2:
> - fix patch 4 to really avoid warnings
>
> These are some fixes to ati-vga for 11.0.
>
> Patch 1 fixes problem reported in
> https://lists.nongnu.org/archive/html/qemu-ppc/2026-01/msg00105.html
>
> Patch 2 fixes output from Solaris install iso
> Rendering still not always correct as it uses some unimplemented
> features but it's now readable. I did not test full install though.
>
> Patch 3 fixes vram overrun errors seen with Solaris and MorphOS when
> crtc offset is non-zero (this happens on MorphOS when additional
> screen is opened)
>
> Patch 4 tries to avoid Coverity warning matching what other places
> already do
>
> Patch 5 fixes dirty region tracking when guest display depth is not
> the same as the host's
>
> Patch 6 adds work around for fuloong2e to keep working with partial
> machine model
>
> BALATON Zoltan (8):
>  ati-vga: Fix colors when frame buffer endianness does not match host
>  ati-vga: Also switch mode on HW cursor enable bit change
>  ati-vga: Do not add crtc offset to src and dst data address
>  ati-vga: Avoid warnings about sign extension
>  ati-vga: Fix display updates in non-32 bit modes
>  ati-vga: Add work around for fuloong2e
>  ati-vga: Simplify pointer image handling
>  ati-vga: Make sure hardware cursor data is within vram
>
> hw/display/ati.c     | 85 +++++++++++++++++++++++++-------------------
> hw/display/ati_2d.c  | 47 ++++++++++++++++--------
> hw/display/ati_int.h |  1 +
> hw/mips/fuloong2e.c  |  1 +
> 4 files changed, 82 insertions(+), 52 deletions(-)
>
>

Re: [PATCH v6 0/8] ati-vga fixes

[Michael Tokarev]

On 21.03.2026 19:30, BALATON Zoltan wrote:
> OK I think these are all the fixes I'd like to add for 11.0 unless
> some more bugs are found. Could you please check and merge?
> 
> v6:
> - revert part of mouse pointer fix patch from v5 as it broke MacOS;
> since I'm not sure the Linux image has no issues on real machine I'm
> not trying to fix that now
> 
> v5:
> - fix typo on Patch 6
> - add more patches to fix mouse pointer with big endian frame buffer
> 
> v4:
> - compile fix in patch 5
> - new patch 6 to work around fuloong2e issue:
> https://lists.nongnu.org/archive/html/qemu-devel/2026-01/msg06308.html
> v3:
> - fix patch 1 to work with 8 and 16 bit modes too
> - new patch 5 to fix display updates with 8 and 16 bit modes
> v2:
> - fix patch 4 to really avoid warnings
> 
> These are some fixes to ati-vga for 11.0.
> 
> Patch 1 fixes problem reported in
> https://lists.nongnu.org/archive/html/qemu-ppc/2026-01/msg00105.html
> 
> Patch 2 fixes output from Solaris install iso
> Rendering still not always correct as it uses some unimplemented
> features but it's now readable. I did not test full install though.
> 
> Patch 3 fixes vram overrun errors seen with Solaris and MorphOS when
> crtc offset is non-zero (this happens on MorphOS when additional
> screen is opened)
> 
> Patch 4 tries to avoid Coverity warning matching what other places
> already do
> 
> Patch 5 fixes dirty region tracking when guest display depth is not
> the same as the host's
> 
> Patch 6 adds work around for fuloong2e to keep working with partial
> machine model
> 
> BALATON Zoltan (8):
>    ati-vga: Fix colors when frame buffer endianness does not match host
>    ati-vga: Also switch mode on HW cursor enable bit change
>    ati-vga: Do not add crtc offset to src and dst data address
>    ati-vga: Avoid warnings about sign extension
>    ati-vga: Fix display updates in non-32 bit modes
>    ati-vga: Add work around for fuloong2e
>    ati-vga: Simplify pointer image handling
>    ati-vga: Make sure hardware cursor data is within vram

This stuff feels like it should be picked up for qemu-stable too,
what do you think?

Thanks,

/mjt

>   hw/display/ati.c     | 85 +++++++++++++++++++++++++-------------------
>   hw/display/ati_2d.c  | 47 ++++++++++++++++--------
>   hw/display/ati_int.h |  1 +
>   hw/mips/fuloong2e.c  |  1 +
>   4 files changed, 82 insertions(+), 52 deletions(-)
> 

Re: [PATCH v6 0/8] ati-vga fixes

[Michael Tokarev]

On 24.03.2026 16:38, Michael Tokarev wrote:
> On 21.03.2026 19:30, BALATON Zoltan wrote:

>> BALATON Zoltan (8):
>>    ati-vga: Fix colors when frame buffer endianness does not match host
>>    ati-vga: Also switch mode on HW cursor enable bit change
>>    ati-vga: Do not add crtc offset to src and dst data address
>>    ati-vga: Avoid warnings about sign extension
>>    ati-vga: Fix display updates in non-32 bit modes
>>    ati-vga: Add work around for fuloong2e
>>    ati-vga: Simplify pointer image handling
>>    ati-vga: Make sure hardware cursor data is within vram
> 
> This stuff feels like it should be picked up for qemu-stable too,
> what do you think?

Um, this is a bit difficult, since there were quite some
refactoring of this area for 11.0, so for previous qemu
releases we should either pick up these refactorings or
back-port the new changes.  Neither of which seems very
important.

Thanks,

/mjt

Re: [PATCH v6 0/8] ati-vga fixes

[BALATON Zoltan]

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

On Tue, 24 Mar 2026, Michael Tokarev wrote:
> On 24.03.2026 16:38, Michael Tokarev wrote:
>> On 21.03.2026 19:30, BALATON Zoltan wrote:
>
>>> BALATON Zoltan (8):
>>>    ati-vga: Fix colors when frame buffer endianness does not match host
>>>    ati-vga: Also switch mode on HW cursor enable bit change
>>>    ati-vga: Do not add crtc offset to src and dst data address
>>>    ati-vga: Avoid warnings about sign extension
>>>    ati-vga: Fix display updates in non-32 bit modes
>>>    ati-vga: Add work around for fuloong2e
>>>    ati-vga: Simplify pointer image handling
>>>    ati-vga: Make sure hardware cursor data is within vram
>> 
>> This stuff feels like it should be picked up for qemu-stable too,
>> what do you think?

Some of it might be but there were changes to ati-vga which likely cause
that these won't apply to older versions. Since ati-vga is still in
development and little used I would not bother backporting these.

> Um, this is a bit difficult, since there were quite some
> refactoring of this area for 11.0, so for previous qemu
> releases we should either pick up these refactorings or
> back-port the new changes.  Neither of which seems very
> important.

Yes, since ati-vga still needs more features to be emulated for most 
drivers it only works with a few simple and obscure ones (e.g. MorphOS) so 
those who want to use those should just update to latest QEMU. Most other 
drivers need the command processor which we don't emulate yet so they 
won't work anyway so it's not stable material yet in general.

Regards,
BALATON Zoltan