[PATCH 0/4] ati-vga fixes

[PATCH 0/4] ati-vga fixes

[BALATON Zoltan]

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)

Patch4 tries to avoid Coverity warning matching what other places
already do

BALATON Zoltan (4):
  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 warning about sign extension

 hw/display/ati.c    |  5 +++--
 hw/display/ati_2d.c | 29 +++++++++++++++++------------
 2 files changed, 20 insertions(+), 14 deletions(-)

-- 
2.41.3

[PATCH 1/4] 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>
---
 hw/display/ati_2d.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 37fe6c17ee..accc7b12b4 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;
@@ -268,10 +270,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 +329,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 +361,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 = s->regs.dp_src_frgd_clr;
+        uint32_t bg = 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 2/4] 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>
---
 hw/display/ati.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 05cf507bd4..1a6a5ad4fd 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -617,6 +617,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 3/4] 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>
---
 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 accc7b12b4..fc09acf97c 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 4/4] ati-vga: Avoid warning about sign extension

[BALATON Zoltan]

Coverity reports possible sign extension error (CID 1645615) when
reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
values are limited when writing the register but change the code to
match other similar registers and to avoid this warning.

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

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 1a6a5ad4fd..d194f15002 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -513,8 +513,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

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

[Philippe Mathieu-Daudé]

On 16/3/26 01:05, BALATON Zoltan wrote:
> Coverity reports possible sign extension error (CID 1645615) when
> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
> values are limited when writing the register but change the code to
> match other similar registers and to avoid this warning.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/display/ati.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

[Peter Maydell]

On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> Coverity reports possible sign extension error (CID 1645615) when
> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
> values are limited when writing the register but change the code to
> match other similar registers and to avoid this warning.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/ati.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 1a6a5ad4fd..d194f15002 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -513,8 +513,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;

I don't think this is going to make Coverity any happier, because
we're still doing:
 * take a value in a 16-bit unsigned type
 * shift it without a cast, giving a 'signed int' (32-bit)
 * promote that to 64-bits to do the |=, because 'val' is
   64 bits, causing a potential sign extension
The rephrasing of the code doesn't avoid the sign-extension.

Indeed there are other (previously dismissed as false positives
years ago) issues in the coverity DB for the other cases in this
switch where we put together values like this without a cast.

But as you say this is a "can't happen" case for this register,
so I've already marked CID 1645615 as a false positive.
We should do whatever we think is reasonable and readable for
this code.

-- PMM

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

[BALATON Zoltan]

On Tue, 17 Mar 2026, Peter Maydell wrote:
> On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> Coverity reports possible sign extension error (CID 1645615) when
>> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
>> values are limited when writing the register but change the code to
>> match other similar registers and to avoid this warning.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/ati.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index 1a6a5ad4fd..d194f15002 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -513,8 +513,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;
>
> I don't think this is going to make Coverity any happier, because
> we're still doing:
> * take a value in a 16-bit unsigned type
> * shift it without a cast, giving a 'signed int' (32-bit)
> * promote that to 64-bits to do the |=, because 'val' is
>   64 bits, causing a potential sign extension
> The rephrasing of the code doesn't avoid the sign-extension.

OK then I'd additionally change val to uint32_t which should avoid it 
then. Would that work?

> Indeed there are other (previously dismissed as false positives
> years ago) issues in the coverity DB for the other cases in this
> switch where we put together values like this without a cast.

I'll try to have a look but I'm new to Coverity and have not checked it 
yet.

> But as you say this is a "can't happen" case for this register,
> so I've already marked CID 1645615 as a false positive.
> We should do whatever we think is reasonable and readable for
> this code.

At least this matches what other places in this device do so good for 
consistency and we generally avoid casts in QEMU, I think changing val to 
uint32_t would help all of these as it does not have to be 64-bit, it's 
just what the ops callback interface has.

Regards,
BALATON Zoltan

Re: [PATCH 4/4] ati-vga: Avoid warning about sign extension

[Peter Maydell]

On Tue, 17 Mar 2026 at 10:34, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 17 Mar 2026, Peter Maydell wrote:
> > On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >>
> >> Coverity reports possible sign extension error (CID 1645615) when
> >> reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the
> >> values are limited when writing the register but change the code to
> >> match other similar registers and to avoid this warning.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  hw/display/ati.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/display/ati.c b/hw/display/ati.c
> >> index 1a6a5ad4fd..d194f15002 100644
> >> --- a/hw/display/ati.c
> >> +++ b/hw/display/ati.c
> >> @@ -513,8 +513,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;
> >
> > I don't think this is going to make Coverity any happier, because
> > we're still doing:
> > * take a value in a 16-bit unsigned type
> > * shift it without a cast, giving a 'signed int' (32-bit)
> > * promote that to 64-bits to do the |=, because 'val' is
> >   64 bits, causing a potential sign extension
> > The rephrasing of the code doesn't avoid the sign-extension.
>
> OK then I'd additionally change val to uint32_t which should avoid it
> then. Would that work?

Yes, I think that using uint32_t for val would silence the
coverity warnings.

thanks
-- PMM

[PATCH v2 4/4] 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>
---
Rest of series unchanged so only sending this patch.
 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

Re: [PATCH v2 4/4] ati-vga: Avoid warnings about sign extension

[Peter Maydell]

On Tue, 17 Mar 2026 at 15:18, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> 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>
> ---
> Rest of series unchanged so only sending this patch.
>  hw/display/ati.c | 8 ++++----

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Chad Jablonski]

This works in the 24/32bpp case. Before this I see a yellow background in
fluxbox, after I see gray.

I did notice that the existing big_endian_fb boolean in ati_vga_switch_mode
doesn't distinguish between big endian 32-bit and 16-bit swapping. In the 16bpp
case I see a black background in fluxbox. I did a quick and dirty test and
replaced the bswap32s with a 16-bit byte swap and then 16bpp displays the gray
background correctly.

This is a pre-existing issue but I thought it was worth pointing out. That said
this fixes things for 24/32bpp and is an improvement.

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

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

[BALATON Zoltan]

On Tue, 17 Mar 2026, Chad Jablonski wrote:
> This works in the 24/32bpp case. Before this I see a yellow background in
> fluxbox, after I see gray.
>
> I did notice that the existing big_endian_fb boolean in ati_vga_switch_mode
> doesn't distinguish between big endian 32-bit and 16-bit swapping. In the 16bpp
> case I see a black background in fluxbox. I did a quick and dirty test and
> replaced the bswap32s with a 16-bit byte swap and then 16bpp displays the gray
> background correctly.
>
> This is a pre-existing issue but I thought it was worth pointing out. That said
> this fixes things for 24/32bpp and is an improvement.
>
> Tested-by: Chad Jablonski <chad@jablonski.xyz>
> Reviewed-by: Chad Jablonski <chad@jablonski.xyz>

Thanks for testing and review. Yes you're right I should have take into 
account bpp too. I'll have a look to see if I can fix this and send a new 
version.

Regards,
BALATON Zoltan

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

[Chad Jablonski]

Tested this on x86_64 Solaris 10. Prior to the patch xterm under XOrg is
drawn incorrectly. It looks like scanlines are drawn every other line.
After, xterm is drawn correctly.

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