On Fri, 3 Jul 2026 at 14:37, BALATON Zoltan <[email protected]> wrote: > > 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.
Ah, yeah, I see now that dst_stride comes from dst_pitch and that gets clamped to 0x3fff on writes. So I'm happy to mark this one as a false positive. You don't need to send a patch unless you think it makes the code clearer for a human reader. Since all that we're doing here is calculating guest addresses that we pass to memory_region_set_dirty(), the guest can't get us to do anything particularly bad even in the worst case. -- PMM
