[PULL 17/27] ati-vga: Fix colors when frame buffer endianness does not match host

Philippe Mathieu-Daudé posted 27 patches 1 week, 3 days ago
Maintainers: Yi Liu <yi.l.liu@intel.com>, Eric Auger <eric.auger@redhat.com>, Zhenzhong Duan <zhenzhong.duan@intel.com>, Richard Henderson <richard.henderson@linaro.org>, Alistair Francis <Alistair.Francis@wdc.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Vijai Kumar K <vijai@behindbytes.com>, Palmer Dabbelt <palmer@dabbelt.com>, "Michael S. Tsirkin" <mst@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Amit Shah <amit@kernel.org>, Jonathan Cameron <jonathan.cameron@huawei.com>, Fan Ni <fan.ni@samsung.com>, Helge Deller <deller@gmx.de>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>, Joe Komlodi <komlodi@google.com>, "Cédric Le Goater" <clg@kaod.org>, Jamin Lin <jamin_lin@aspeedtech.com>, Nabih Estefan <nabihestefan@google.com>, Corey Minyard <minyard@acm.org>, Thomas Huth <th.huth+qemu@posteo.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Peter Maydell <peter.maydell@linaro.org>, Steven Lee <steven_lee@aspeedtech.com>, Troy Lee <leetroy@gmail.com>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Jason Wang <jasowang@redhat.com>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>, Jiri Pirko <jiri@resnulli.us>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Chao Liu <chao.liu.zevorn@gmail.com>, Fam Zheng <fam@euphon.net>, Cornelia Huck <cohuck@redhat.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Alex Williamson <alex@shazbot.org>, Stefano Garzarella <sgarzare@redhat.com>, Magnus Kulke <magnuskulke@linux.microsoft.com>, Wei Liu <wei.liu@kernel.org>, "Dr. David Alan Gilbert" <dave@treblig.org>, Marcelo Tosatti <mtosatti@redhat.com>
[PULL 17/27] ati-vga: Fix colors when frame buffer endianness does not match host
Posted by Philippe Mathieu-Daudé 1 week, 3 days ago
From: BALATON Zoltan <balaton@eik.bme.hu>

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>
Message-ID: <759ed5e3b019cce94e9a4ef003f1fc2e0cea2ec1.1774110169.git.balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 37fe6c17ee9..0cbbdc33f47 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.53.0


Re: [PULL 17/27] ati-vga: Fix colors when frame buffer endianness does not match host
Posted by Peter Maydell 1 week, 2 days ago
On Mon, 23 Mar 2026 at 16:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> From: BALATON Zoltan <balaton@eik.bme.hu>
>
> 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>
> Message-ID: <759ed5e3b019cce94e9a4ef003f1fc2e0cea2ec1.1774110169.git.balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Hi; Coverity flags this as possibly-wrong code (CID 1645969:

> @@ -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;

The issue is that != is higher priority than ?:, and people often
write "X != A ? B : C" when they wanted "X != (A ? B : C)" but they
get "(X != A) ? B : C". Either way, using parentheses helps clarify
for readers.

The priority is actually right for this particular case, but the
expression is unnecessarily complex: we could write more simply:

  ctx->need_swap = (HOST_BIG_ENDIAN != s->vga.big_endian_fb);

I've marked the issue in Coverity as false-positive, but we might
consider changing the code also.

-- PMM
Re: [PULL 17/27] ati-vga: Fix colors when frame buffer endianness does not match host
Posted by BALATON Zoltan 1 week, 2 days ago
On Tue, 24 Mar 2026, Peter Maydell wrote:
> On Mon, 23 Mar 2026 at 16:54, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> From: BALATON Zoltan <balaton@eik.bme.hu>
>>
>> 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>
>> Message-ID: <759ed5e3b019cce94e9a4ef003f1fc2e0cea2ec1.1774110169.git.balaton@eik.bme.hu>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Hi; Coverity flags this as possibly-wrong code (CID 1645969:

You probably meant to cc me too but now I'm registered for QEMU in 
coverity as well so I got the warning too.

>> @@ -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;
>
> The issue is that != is higher priority than ?:, and people often
> write "X != A ? B : C" when they wanted "X != (A ? B : C)" but they
> get "(X != A) ? B : C". Either way, using parentheses helps clarify
> for readers.
>
> The priority is actually right for this particular case, but the

Yes I omitted the parenthesis as it's not needed here but will include it 
in the future.

> expression is unnecessarily complex: we could write more simply:
>
>  ctx->need_swap = (HOST_BIG_ENDIAN != s->vga.big_endian_fb);

I thought about that but too late and did not want to redo the series. 
I'll send a patch and check the other issue reported as well if it's a 
problem or can be avoided.

> I've marked the issue in Coverity as false-positive, but we might
> consider changing the code also.

Thank you,
BALATON Zoltan