Patchwork Re: Another SIGFPE in display code, now in cirrus

login
register
mail settings
Submitter Stefano Stabellini
Date May 13, 2010, 1:48 p.m.
Message ID <alpine.DEB.2.00.1005131329230.11380@kaball-desktop>
Download mbox | patch
Permalink /patch/52483/
State New
Headers show

Comments

Stefano Stabellini - May 13, 2010, 1:48 p.m.
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.
 

---
Michael Tokarev - May 13, 2010, 2:13 p.m.
Stefano Stabellini wrote:
[]
> 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

The same as with previous patch: Yellow screen
(instead of crashing), and two lines on the
stderr:

BUG: kvm_dirty_pages_log_enable_slot: invalid parameters
BUG: kvm_dirty_pages_log_enable_slot: invalid parameters

Thanks!

/mjt
Jamie Lokier - May 13, 2010, 4:04 p.m.
Stefano Stabellini wrote:
> > 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.

Even when copying on-screen (or partially on-screen), the srcpitch
does not affect the invalidated area.  The source area might be
strange (parallelogram, single line repeated), but srcpitch should
only affect whether qemu_console_copy can be used, not the
invalidation.

-- Jamie
Stefano Stabellini - May 13, 2010, 6:03 p.m.
On Thu, 13 May 2010, Michael Tokarev wrote:
> Stefano Stabellini wrote:
> []
> > 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
> 
> The same as with previous patch: Yellow screen
> (instead of crashing), and two lines on the
> stderr:
> 
> BUG: kvm_dirty_pages_log_enable_slot: invalid parameters
> BUG: kvm_dirty_pages_log_enable_slot: invalid parameters
> 

I tried to do the same thing on WinNT with qemu (thanks to
Michael for providing me the image) and it works OK with the
patch applied.

I think there must be another bug in the kvm dirty page handling...

Patch

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++) {