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

login
register
mail settings
Submitter Stefano Stabellini
Date May 12, 2010, 6:11 p.m.
Message ID <alpine.DEB.2.00.1005121855230.11380@kaball-desktop>
Download mbox | patch
Permalink /patch/52404/
State New
Headers show

Comments

Stefano Stabellini - May 12, 2010, 6:11 p.m.
On Wed, 12 May 2010, Jamie Lokier wrote:
> Stefano Stabellini wrote:
> > On Wed, 12 May 2010, Avi Kivity wrote:
> > > It's useful if you have a one-line horizontal pattern you want to 
> > > propagate all over.
> >  
> > It might be useful all right, but it is not entirely clear what the
> > hardware should do in this situation from the documentation we have, and
> > certainly the current state of the cirrus emulation code doesn't help.
> 
> It's quite a reasonable thing for hardware to do, even if not documented.
> It would be surprising if the hardware didn't copy the one-line pattern.
 
All right then, you convinced me :)

This is my proposed solution, however it is untested with Windows NT.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
Michael Tokarev - May 12, 2010, 7:12 p.m.
12.05.2010 22:11, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Jamie Lokier wrote:
>> Stefano Stabellini wrote:
>>> On Wed, 12 May 2010, Avi Kivity wrote:
>>>> It's useful if you have a one-line horizontal pattern you want to
>>>> propagate all over.
>>>
>>> It might be useful all right, but it is not entirely clear what the
>>> hardware should do in this situation from the documentation we have, and
>>> certainly the current state of the cirrus emulation code doesn't help.
>>
>> It's quite a reasonable thing for hardware to do, even if not documented.
>> It would be surprising if the hardware didn't copy the one-line pattern.
>
> All right then, you convinced me :)
>
> This is my proposed solution, however it is untested with Windows NT.

Well.  At least it does not crash anymore.

With this patch applied, when hitting "Test" (of a new video mode)
button on WindowsNT, the guest window gets resized to correct size,
but is painted with yellow and nothing happens.  The CPU enters
100%, and on the kvm console the following messages are displayed:

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

That's not new, it sometimes displays that shit on attempt to
migrate too, as I mentioned in another thread.

Thanks!

/mjt

> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 9f61a01..a7f0d3c 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -676,15 +676,17 @@ 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;
[...]
Avi Kivity - May 13, 2010, 6:49 a.m.
On 05/12/2010 09:11 PM, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Jamie Lokier wrote:
>    
>> Stefano Stabellini wrote:
>>      
>>> On Wed, 12 May 2010, Avi Kivity wrote:
>>>        
>>>> It's useful if you have a one-line horizontal pattern you want to
>>>> propagate all over.
>>>>          
>>>
>>> It might be useful all right, but it is not entirely clear what the
>>> hardware should do in this situation from the documentation we have, and
>>> certainly the current state of the cirrus emulation code doesn't help.
>>>        
>> It's quite a reasonable thing for hardware to do, even if not documented.
>> It would be surprising if the hardware didn't copy the one-line pattern.
>>      
>
> All right then, you convinced me :)
>
> This is my proposed solution, however it is untested with Windows NT.
>
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> ---
>
>
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 9f61a01..a7f0d3c 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -676,15 +676,17 @@ 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));
> +    sx = (src % line_offset) / depth;
> +    sy = (src / line_offset);
>    

Does anything prevent the guest from programming the CRTC display pitch 
to 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.


> +     } 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?
Michael Tokarev - May 28, 2010, 8:51 p.m.
12.05.2010 22:11, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Jamie Lokier wrote:
>> Stefano Stabellini wrote:
>>> On Wed, 12 May 2010, Avi Kivity wrote:
>>>> It's useful if you have a one-line horizontal pattern you want to
>>>> propagate all over.
>>>
>>> It might be useful all right, but it is not entirely clear what the
>>> hardware should do in this situation from the documentation we have, and
>>> certainly the current state of the cirrus emulation code doesn't help.
>>
>> It's quite a reasonable thing for hardware to do, even if not documented.
>> It would be surprising if the hardware didn't copy the one-line pattern.
>
> All right then, you convinced me :)
>
> This is my proposed solution, however it is untested with Windows NT.
>
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>

So.. what's the status of this, after all? :)
As far as I can tell, it has been agreed that the patch
is good, and verified that it fixes the problem.  Should
we just throw it away and start from scratch, or what? :)

Thanks!

> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 9f61a01..a7f0d3c 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -676,15 +676,17 @@ 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));
> +    sx = (src % line_offset) / depth;
> +    sy = (src / line_offset);
>       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();
> +     } 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;
>
>       for (y = 0; y<  bltheight; y++) {
>           for (x = 0; x<  bltwidth; x++) {
> @@ -57,6 +57,12 @@ glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
>       int x,y;
>       dstpitch += bltwidth;
>       srcpitch += bltwidth;
> +
> +    if (dstpitch>  0)
> +        return;
> +    if (srcpitch>  0)
> +        srcpitch = 0;
> +
>       for (y = 0; y<  bltheight; y++) {
>           for (x = 0; x<  bltwidth; x++) {
>               ROP_OP(*dst, *src);
> @@ -78,6 +84,12 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
>       uint8_t p;
>       dstpitch -= bltwidth;
>       srcpitch -= bltwidth;
> +
> +    if (dstpitch<  0)
> +        return;
> +    if (srcpitch<  0)
> +        srcpitch = 0;
> +
>       for (y = 0; y<  bltheight; y++) {
>           for (x = 0; x<  bltwidth; x++) {
>   	    p = *dst;
> @@ -101,6 +113,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
>       uint8_t p;
>       dstpitch += bltwidth;
>       srcpitch += bltwidth;
> +
> +    if (dstpitch>  0)
> +        return;
> +    if (srcpitch>  0)
> +        srcpitch = 0;
> +
>       for (y = 0; y<  bltheight; y++) {
>           for (x = 0; x<  bltwidth; x++) {
>   	    p = *dst;
> @@ -124,6 +142,12 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
>       uint8_t p1, p2;
>       dstpitch -= bltwidth;
>       srcpitch -= bltwidth;
> +
> +    if (dstpitch<  0)
> +        return;
> +    if (srcpitch<  0)
> +        srcpitch = 0;
> +
>       for (y = 0; y<  bltheight; y++) {
>           for (x = 0; x<  bltwidth; x+=2) {
>   	    p1 = *dst;
> @@ -152,6 +176,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
>       uint8_t p1, p2;
>       dstpitch += bltwidth;
>       srcpitch += bltwidth;
> +
> +    if (dstpitch>  0)
> +        return;
> +    if (srcpitch>  0)
> +        srcpitch = 0;
> +
>       for (y = 0; y<  bltheight; y++) {
>           for (x = 0; x<  bltwidth; x+=2) {
>   	    p1 = *(dst-1);
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - May 30, 2010, 8:24 a.m.
On 05/12/2010 09:11 PM, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Jamie Lokier wrote:
>    
>> Stefano Stabellini wrote:
>>      
>>> On Wed, 12 May 2010, Avi Kivity wrote:
>>>        
>>>> It's useful if you have a one-line horizontal pattern you want to
>>>> propagate all over.
>>>>          
>>>
>>> It might be useful all right, but it is not entirely clear what the
>>> hardware should do in this situation from the documentation we have, and
>>> certainly the current state of the cirrus emulation code doesn't help.
>>>        
>> It's quite a reasonable thing for hardware to do, even if not documented.
>> It would be surprising if the hardware didn't copy the one-line pattern.
>>      
>
> All right then, you convinced me :)
>
> This is my proposed solution, however it is untested with Windows NT.
>
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> ---
>
>
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 9f61a01..a7f0d3c 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -676,15 +676,17 @@ 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));
> +    sx = (src % line_offset) / depth;
> +    sy = (src / line_offset);
>       dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
>       dy = (dst / ABS(s->cirrus_blt_dstpitch));
>    

dx/dy also need to be calculated according to line_offset.

I think the rules are:

if src_byte_range in screen and dst_byte_range in screen
    and srcpitch == dstpitch == line_offset:
        can use qemu_console_copy
elif dst_byte_range overlaps screen:
        if dstpitch == line_offset:
            invalidate rectangle
        else:
            invalidate full screen

Patch

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 9f61a01..a7f0d3c 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -676,15 +676,17 @@  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));
+    sx = (src % line_offset) / depth;
+    sy = (src / line_offset);
     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();
+     } 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;
 
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x++) {
@@ -57,6 +57,12 @@  glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
     int x,y;
     dstpitch += bltwidth;
     srcpitch += bltwidth;
+
+    if (dstpitch > 0)
+        return;
+    if (srcpitch > 0)
+        srcpitch = 0;
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x++) {
             ROP_OP(*dst, *src);
@@ -78,6 +84,12 @@  glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
     uint8_t p;
     dstpitch -= bltwidth;
     srcpitch -= bltwidth;
+
+    if (dstpitch < 0)
+        return;
+    if (srcpitch < 0)
+        srcpitch = 0;
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x++) {
 	    p = *dst;
@@ -101,6 +113,12 @@  glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
     uint8_t p;
     dstpitch += bltwidth;
     srcpitch += bltwidth;
+
+    if (dstpitch > 0)
+        return;
+    if (srcpitch > 0)
+        srcpitch = 0;
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x++) {
 	    p = *dst;
@@ -124,6 +142,12 @@  glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
     uint8_t p1, p2;
     dstpitch -= bltwidth;
     srcpitch -= bltwidth;
+
+    if (dstpitch < 0)
+        return;
+    if (srcpitch < 0)
+        srcpitch = 0;
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x+=2) {
 	    p1 = *dst;
@@ -152,6 +176,12 @@  glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
     uint8_t p1, p2;
     dstpitch += bltwidth;
     srcpitch += bltwidth;
+
+    if (dstpitch > 0)
+        return;
+    if (srcpitch > 0)
+        srcpitch = 0;
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x+=2) {
 	    p1 = *(dst-1);