From patchwork Thu May 13 13:48:24 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 52483 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id DE793B7E30 for ; Thu, 13 May 2010 23:48:47 +1000 (EST) Received: from localhost ([127.0.0.1]:47911 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OCYmG-0000M0-Tw for incoming@patchwork.ozlabs.org; Thu, 13 May 2010 09:48:40 -0400 Received: from [140.186.70.92] (port=57812 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OCYjR-00078N-7f for qemu-devel@nongnu.org; Thu, 13 May 2010 09:45:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OCYjH-0008IJ-79 for qemu-devel@nongnu.org; Thu, 13 May 2010 09:45:44 -0400 Received: from smtp.citrix.com ([66.165.176.89]:53575) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OCYjG-0008HA-VQ for qemu-devel@nongnu.org; Thu, 13 May 2010 09:45:35 -0400 X-IronPort-AV: E=Sophos;i="4.53,222,1272859200"; d="scan'208";a="8464541" Received: from ftlpmailmx02.citrite.net ([10.9.154.224]) by FTLPIPO01.CITRIX.COM with ESMTP/TLS/RC4-MD5; 13 May 2010 09:45:32 -0400 Received: from LONPMAILMX01.citrite.net (10.30.224.162) by FTLPMAILMX02.citrite.net (10.9.154.224) with Microsoft SMTP Server (TLS) id 8.2.254.0; Thu, 13 May 2010 09:45:32 -0400 Received: from kaball.uk.xensource.com (10.80.2.59) by smtprelay.citrix.com (10.30.224.162) with Microsoft SMTP Server id 8.2.254.0; Thu, 13 May 2010 14:45:30 +0100 Date: Thu, 13 May 2010 14:48:24 +0100 From: Stefano Stabellini X-X-Sender: sstabellini@kaball-desktop To: Avi Kivity Subject: Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus In-Reply-To: <4BEBA0ED.9010009@redhat.com> Message-ID: References: <4BE32178.2090103@msgid.tls.msk.ru> <4BE7B8C1.9060807@redhat.com> <4BE7C0A5.3090909@redhat.com> <4BEAA0CC.4090906@redhat.com> <4BEABABC.6080305@redhat.com> <4BEAD232.2040700@redhat.com> <20100512170702.GE19314@shareable.org> <4BEBA0ED.9010009@redhat.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. Cc: KVM list , Stefano Stabellini , Michael Tokarev , qemu-devel , Brian Kress X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Thu, 13 May 2010, Avi Kivity wrote: > > /* extra x, y */ > > - sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth; > > - sy = (src / ABS(s->cirrus_blt_srcpitch)); > > + sx = (src % line_offset) / depth; > > + sy = (src / line_offset); > > > > Does anything prevent the guest from programming the CRTC display pitch > to 0? > Nope, I'll use line_offset there too to prevent possible divisions by 0. > > dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth; > > dy = (dst / ABS(s->cirrus_blt_dstpitch)); > > > > @@ -725,18 +727,23 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) > > s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch, > > s->cirrus_blt_width, s->cirrus_blt_height); > > > > - if (notify) > > - qemu_console_copy(s->vga.ds, > > - sx, sy, dx, dy, > > - s->cirrus_blt_width / depth, > > - s->cirrus_blt_height); > > - > > - /* we don't have to notify the display that this portion has > > - changed since qemu_console_copy implies this */ > > - > > - cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, > > - s->cirrus_blt_dstpitch, s->cirrus_blt_width, > > - s->cirrus_blt_height); > > + if (ABS(s->cirrus_blt_dstpitch) != line_offset || > > + ABS(s->cirrus_blt_srcpitch) != line_offset) { > > + /* this is not going to happen very often */ > > + vga_hw_invalidate(); > > > > I think we need to consider only dstpitch for a full invalidate. We > might be copying an offscreen bitmap into the screen, and srcpitch is > likely to be the bitmap width instead of the screen pitch. > Agreed. > > > + } else { > > + if (notify) > > + /* we don't have to notify the display that this portion has > > + changed since qemu_console_copy implies this */ > > + qemu_console_copy(s->vga.ds, > > + sx, sy, dx, dy, > > + s->cirrus_blt_width / depth, > > + s->cirrus_blt_height); > > + else > > + cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, > > + s->cirrus_blt_dstpitch, s->cirrus_blt_width, > > + s->cirrus_blt_height); > > + } > > } > > > > static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s) > > diff --git a/hw/cirrus_vga_rop.h b/hw/cirrus_vga_rop.h > > index 39a7b72..80f135b 100644 > > --- a/hw/cirrus_vga_rop.h > > +++ b/hw/cirrus_vga_rop.h > > @@ -32,10 +32,10 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s, > > dstpitch -= bltwidth; > > srcpitch -= bltwidth; > > > > - if (dstpitch< 0 || srcpitch< 0) { > > - /* is 0 valid? srcpitch == 0 could be useful */ > > + if (dstpitch< 0) > > return; > > - } > > + if (srcpitch< 0) > > + srcpitch = 0; > > > > Why? I wouldn't want an operation that is supposed to be a forward copy to become a backward copy instead. With the way the code is currently written in cirrus_vga it shouldn't be possible, but it might be a good idea to have the checks anyway. Actually the limit I wrote is too strict, I'll fix it. diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c index 9f61a01..81c443b 100644 --- a/hw/cirrus_vga.c +++ b/hw/cirrus_vga.c @@ -676,17 +676,19 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) int sx, sy; int dx, dy; int width, height; + uint32_t start_addr, line_offset, line_compare; int depth; int notify = 0; depth = s->vga.get_bpp(&s->vga) / 8; s->vga.get_resolution(&s->vga, &width, &height); + s->vga.get_offsets(&s->vga, &line_offset, &start_addr, &line_compare); /* extra x, y */ - sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth; - sy = (src / ABS(s->cirrus_blt_srcpitch)); - dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth; - dy = (dst / ABS(s->cirrus_blt_dstpitch)); + sx = (src % line_offset) / depth; + sy = (src / line_offset); + dx = (dst % line_offset) / depth; + dy = (dst / line_offset); /* normalize width */ w /= depth; @@ -725,18 +727,22 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch, s->cirrus_blt_width, s->cirrus_blt_height); - if (notify) - qemu_console_copy(s->vga.ds, - sx, sy, dx, dy, - s->cirrus_blt_width / depth, - s->cirrus_blt_height); - - /* we don't have to notify the display that this portion has - changed since qemu_console_copy implies this */ - - cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, - s->cirrus_blt_dstpitch, s->cirrus_blt_width, - s->cirrus_blt_height); + if (ABS(s->cirrus_blt_dstpitch) != line_offset) { + /* this is not going to happen very often */ + vga_hw_invalidate(); + } else { + if (ABS(s->cirrus_blt_srcpitch) == line_offset && notify) + /* we don't have to notify the display that this portion has + changed since qemu_console_copy implies this */ + qemu_console_copy(s->vga.ds, + sx, sy, dx, dy, + s->cirrus_blt_width / depth, + s->cirrus_blt_height); + else + cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, + s->cirrus_blt_dstpitch, s->cirrus_blt_width, + s->cirrus_blt_height); + } } static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s) diff --git a/hw/cirrus_vga_rop.h b/hw/cirrus_vga_rop.h index 39a7b72..3d41d41 100644 --- a/hw/cirrus_vga_rop.h +++ b/hw/cirrus_vga_rop.h @@ -29,13 +29,11 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s, int bltwidth,int bltheight) { int x,y; - dstpitch -= bltwidth; - srcpitch -= bltwidth; - if (dstpitch < 0 || srcpitch < 0) { - /* is 0 valid? srcpitch == 0 could be useful */ + if (dstpitch < 0 || srcpitch < 0) return; - } + dstpitch -= bltwidth; + srcpitch -= bltwidth; for (y = 0; y < bltheight; y++) { for (x = 0; x < bltwidth; x++) { @@ -55,6 +53,9 @@ glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s, int bltwidth,int bltheight) { int x,y; + + if (dstpitch > 0 || srcpitch > 0) + return; dstpitch += bltwidth; srcpitch += bltwidth; for (y = 0; y < bltheight; y++) { @@ -76,6 +77,9 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s, { int x,y; uint8_t p; + + if (dstpitch < 0 || srcpitch < 0) + return; dstpitch -= bltwidth; srcpitch -= bltwidth; for (y = 0; y < bltheight; y++) { @@ -99,6 +103,9 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s, { int x,y; uint8_t p; + + if (dstpitch > 0 || srcpitch > 0) + return; dstpitch += bltwidth; srcpitch += bltwidth; for (y = 0; y < bltheight; y++) { @@ -122,6 +129,9 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s, { int x,y; uint8_t p1, p2; + + if (dstpitch < 0 || srcpitch < 0) + return; dstpitch -= bltwidth; srcpitch -= bltwidth; for (y = 0; y < bltheight; y++) { @@ -150,6 +160,9 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s, { int x,y; uint8_t p1, p2; + + if (dstpitch > 0 || srcpitch > 0) + return; dstpitch += bltwidth; srcpitch += bltwidth; for (y = 0; y < bltheight; y++) {