Re: [PATCH v9 15/18] ati-vga: Move source bounds validation to ati_2d_blt

[BALATON Zoltan]

On Fri, 27 Feb 2026, Chad Jablonski wrote:
A call to ati_2d_blt implies that the source will be vram. Checking
bounds is useful in that case. Other sources (HOST_DATA) will not make
sense to check against vram bounds.
Signed-off-by: Chad Jablonski <c...@jablonski.xyz>
Reviewed-by: BALATON Zoltan <bala...@eik.bme.hu>

---

Changes from v8:

The source bound validation was being performed in all cases, even for
blits that weren't using the source. It now limits the validation to the
ROP3_SRCCOPY case.
---
hw/display/ati_2d.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 440c1d159a..dbc8791824 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -138,9 +138,9 @@ static void ati_2d_do_blt(ATI2DCtx *ctx, uint8_t use_pixman)
        return;
    }
    int dst_stride_words = ctx->dst_stride / sizeof(uint32_t);
-    if (ctx->dst.x > 0x3fff || ctx->dst.y > 0x3fff
-        || ctx->dst_bits >= ctx->vram_end || ctx->dst_bits + ctx->dst.x
-         + (ctx->dst.y + ctx->dst.height) * ctx->dst_stride >= ctx->vram_end) {
+    if (ctx->dst.x > 0x3fff || ctx->dst.y > 0x3fff ||
+        ctx->dst_bits >= ctx->vram_end || ctx->dst_bits + ctx->dst.x +
+        (ctx->dst.y + ctx->dst.height) * ctx->dst_stride >= ctx->vram_end) {
        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
        return;
    }
@@ -153,13 +153,6 @@ static void ati_2d_do_blt(ATI2DCtx *ctx, uint8_t 
use_pixman)
            return;
        }
        int src_stride_words = ctx->src_stride / sizeof(uint32_t);
-        if (ctx->src.x > 0x3fff || ctx->src.y > 0x3fff
-            || ctx->src_bits >= ctx->vram_end
-            || ctx->src_bits + ctx->src.x + (ctx->src.y + ctx->dst.height)
-             * ctx->src_stride >= ctx->vram_end) {
-            qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
-            return;
-        }

        DPRINTF("pixman_blt(%p, %p, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d)\n",
                ctx->src_bits, ctx->dst_bits, src_stride_words,
@@ -268,6 +261,12 @@ void ati_2d_blt(ATIVGAState *s)
{
    ATI2DCtx ctx;
    setup_2d_blt_ctx(s, &ctx);
+    if (ctx.rop3 == ROP3_SRCCOPY && (ctx.src.x > 0x3fff || ctx.src.y > 0x3fff 
||
+        ctx.src_bits >= ctx.vram_end || ctx.src_bits + ctx.src.x +
+        (ctx.src.y + ctx.dst.height) * ctx.src_stride >= ctx.vram_end)) {
+        qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
+        return;
+    }

In second look I'm not sure about this, also because the next patch 
changes src values based on clipping so this check should be done after 
that to make sure it's still within vram. So maybe we should leave it 
where it is and if need to be disabled for host data then add a check at 
the front if (!host_data_is_active && (...)) or so to only do the check 
if source is vram.

Regards,
BALATON Zoltan

    ati_2d_do_blt(&ctx, s->use_pixman);
    ati_set_dirty(&s->vga, &ctx);
}