On Fri, 3 Jul 2026, Peter Maydell wrote:
On Tue, 12 May 2026 at 07:30, Philippe Mathieu-Daudé <[email protected]> wrote:
From: Chad Jablonski <[email protected]>
This fixes three bugs with the ati_set_dirty address calculation.
First, vbe_start_addr is a word offset. All other values in the
calculation are byte offsets. It must be converted to bytes.
Second, when setting the dirty region with memory_region_set_dirty
the vbe_start_addr is used to calculate the start of the dirty region.
This is a problem because the vbe_start_addr is the offset at which scan out
begins. This puts it in the visible screen coordinate system. The dirty
region however is in the virtual screen coordinate system. This can cause both
overmarking and missed updates. This is removed from the calculation.
Third, when the start address of a blit is outside of the bounds check
the entire blit is missed and not set to dirty. This happens even if the
blit does partially overlap with the visible screen. The fix here is to
find the intersection of the visible screen and the blit and mark only
that region as dirty.
This does not attempt to apply clipping to the blit. So there will be
overmarking in some cases.
Signed-off-by: Chad Jablonski <[email protected]>
[balaton: drop excess parenthesis, use offsets instead of pointers]
Reviewed-by: BALATON Zoltan <[email protected]>
Tested-by: BALATON Zoltan <[email protected]>
Signed-off-by: BALATON Zoltan <[email protected]>
Message-ID: <[email protected]>
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Hi; Coverity warns here about some "we did a 32 x 32 multiply
and put the result into a 64 bit calculation" issue here (CID 1660048):
I've got the warnings but didn't have time to look at it yet.
---
hw/display/ati_2d.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 504d1c57085..48498677c7e 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -69,18 +69,28 @@ typedef struct {
static void ati_set_dirty(VGACommonState *vga, const ATI2DCtx *ctx)
{
DisplaySurface *ds = qemu_console_surface(vga->con);
+ unsigned int bypp = ctx->bpp / 8;
+ hwaddr dirty_start = ctx->dst_offset + ctx->dst.x * bypp +
+ ctx->dst.y * ctx->dst_stride;
dst.y is uint16_t and dst_stride is int, so we do the multiply
at 32-bits, and then the whole set of additions at 32-bits before
putting it into a 64-bit hwaddr.
Should we cast one of them to 64-bits to avoid the overflow,
or do we know here that these values can't actually be large
enough to overflow ?
I think since the x,y values are limited (at about 16384 or so, we usually
apply mask when writing the regs) these should never overflow but we could
try to cast something to silence the warnings. I can try to make a patch
eventually unless someone else does it faster.
Regards,
BALATON Zoltan
