Another SIGFPE in display code, now in cirrus

There was a bug recently fixed in vnc code. Apparently there's something similar in the cirrus emulation as well. Here it triggers _always_ (including old versions of kvm) when running windows NT and hitting "test" button in its display resolution dialog. Here's what gdb is to say:

Program received signal SIGFPE, Arithmetic exception. [Switching to Thread 0xf76cab70 (LWP 580)]
0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9) at hw/cirrus_vga.c:687
687 sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
(gdb) p s->cirrus_blt_srcpitch
$2 = 0

This qemu-kvm-0.12.3 - actually a debian package of it, but there's no patches relevant to video applied.

Anything can be done with it?

Thanks!

There was a bug recently fixed in vnc code. Apparently there's something similar in the cirrus emulation as well. Here it triggers _always_ (including old versions of kvm) when running windows NT and hitting "test" button in its display resolution dialog. Here's what gdb is to say:

Program received signal SIGFPE, Arithmetic exception. [Switching to Thread 0xf76cab70 (LWP 580)]
0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9)

    at hw/cirrus_vga.c:687

687 sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
(gdb) p depth
$1 = 2
(gdb) p s->cirrus_blt_srcpitch
$2 = 0

This qemu-kvm-0.12.3 - actually a debian package of it, but there's no patches relevant to video applied.

Anything can be done with it?

Quoted 17 lines · Michael Tokarev

There was a bug recently fixed in vnc code. Apparently there's something similar in the cirrus emulation as well. Here it triggers _always_ (including old versions of kvm) when running windows NT and hitting "test" button in its display resolution dialog. Here's what gdb is to say:

Program received signal SIGFPE, Arithmetic exception. [Switching to Thread 0xf76cab70 (LWP 580)]
0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9)

    at hw/cirrus_vga.c:687

687 sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
(gdb) p depth
$1 = 2
(gdb) p s->cirrus_blt_srcpitch
$2 = 0

Quoted 4 lines

This qemu-kvm-0.12.3 - actually a debian package of it, but there's no patches relevant to video applied.

Anything can be done with it?

Well, it's trivial to check for the condition, but how to handle it?

    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);

Quoted 21 lines · Avi Kivity

Quoted 20 lines · Michael Tokarev

There was a bug recently fixed in vnc code. Apparently there's something similar in the cirrus emulation as well. Here it triggers _always_ (including old versions of kvm) when running windows NT and hitting "test" button in its display resolution dialog. Here's what gdb is to say:

Program received signal SIGFPE, Arithmetic exception. [Switching to Thread 0xf76cab70 (LWP 580)]
0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9)

    at hw/cirrus_vga.c:687

687 sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
(gdb) p depth
$1 = 2
(gdb) p s->cirrus_blt_srcpitch
$2 = 0

This qemu-kvm-0.12.3 - actually a debian package of it, but there's no patches relevant to video applied.

Quoted 4 lines

Quoted 1 line

Anything can be done with it?

Well, it's trivial to check for the condition, but how to handle it?

That code is wrong. It shouldn't be using the bitblt source pitch, but the actual display pitch. If the two are different, then the blt doesn't actually affect a rectangular region, but instead a parallelogram.

Further:

Quoted 13 lines

    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);

Shouldn't we avoid the invalidate if notify != 0, as the comment says?

31c05501c says this breaks bitblt, but I can't see why this is true. The copy should update the display. This is probably due to a miscalculation of the affected region, and now we have two invalidates instead of one, reducing performance.

Quoted 29 lines · Avi Kivity

Quoted 27 lines · Avi Kivity

Quoted 17 lines · Michael Tokarev

There was a bug recently fixed in vnc code. Apparently there's something similar in the cirrus emulation as well. Here it triggers _always_ (including old versions of kvm) when running windows NT and hitting "test" button in its display resolution dialog. Here's what gdb is to say:

Program received signal SIGFPE, Arithmetic exception. [Switching to Thread 0xf76cab70 (LWP 580)]
0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9)

     at hw/cirrus_vga.c:687

687 sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
(gdb) p depth
$1 = 2
(gdb) p s->cirrus_blt_srcpitch
$2 = 0

Quoted 5 lines

This qemu-kvm-0.12.3 - actually a debian package of it, but there's no patches relevant to video applied.

Anything can be done with it?

Well, it's trivial to check for the condition, but how to handle it?

Quoted 5 lines

That code is wrong. It shouldn't be using the bitblt source pitch, but the actual display pitch. If the two are different, then the blt doesn't actually affect a rectangular region, but instead a parallelogram.

Reading some documention about the Cirrus card we are emulating, it seems that the source pitch is ignored in video to video operations, so I think you are right here.

The Windows NT driver probably relies on this behaviour and doesn't set the source pitch properly before the bitblt.

Quoted 6 lines

31c05501c says this breaks bitblt, but I can't see why this is true. The copy should update the display. This is probably due to a miscalculation of the affected region, and now we have two invalidates instead of one, reducing performance.

I agree with you: qemu_console_copy does imply that the copied portion of the screen changed, so there is no reason to invalidate it again if qemu_console_copy is called.

Quoted 31 lines · Stefano Stabellini

Quoted 29 lines · Avi Kivity

Quoted 27 lines · Avi Kivity

Quoted 17 lines · Michael Tokarev

There was a bug recently fixed in vnc code. Apparently there's something similar in the cirrus emulation as well. Here it triggers _always_ (including old versions of kvm) when running windows NT and hitting "test" button in its display resolution dialog. Here's what gdb is to say:

Program received signal SIGFPE, Arithmetic exception. [Switching to Thread 0xf76cab70 (LWP 580)]
0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9)

     at hw/cirrus_vga.c:687

687 sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
(gdb) p depth
$1 = 2
(gdb) p s->cirrus_blt_srcpitch
$2 = 0

Quoted 5 lines

This qemu-kvm-0.12.3 - actually a debian package of it, but there's no patches relevant to video applied.

Anything can be done with it?

Well, it's trivial to check for the condition, but how to handle it?

Quoted 9 lines

Quoted 5 lines

That code is wrong. It shouldn't be using the bitblt source pitch, but the actual display pitch. If the two are different, then the blt doesn't actually affect a rectangular region, but instead a parallelogram.

Reading some documention about the Cirrus card we are emulating, it seems that the source pitch is ignored in video to video operations, so I think you are right here.

From what I've read I don't think it's ignored. Rather there are two
separate pitches, one for bitblt and one for display.

Quoted 3 lines

The Windows NT driver probably relies on this behaviour and doesn't set the source pitch properly before the bitblt.

Note it's just during mode changes. During normal operation I'm sure the pitches are equal.

I think the code should be something like

if bitblt destination intersects display memory:
if bitblt pitch == display pitch
use console_copy
else
invalidate entire display

Quoted 6 lines

Quoted 6 lines

31c05501c says this breaks bitblt, but I can't see why this is true. The copy should update the display. This is probably due to a miscalculation of the affected region, and now we have two invalidates instead of one, reducing performance.

Quoted 4 lines

I agree with you: qemu_console_copy does imply that the copied portion of the screen changed, so there is no reason to invalidate it again if qemu_console_copy is called.

Well, we can't just revert 31c05501c. There's probably another bug involved.

Quoted 6 lines

Note it's just during mode changes. During normal operation I'm sure the pitches are equal.

The source blt pitch as set by the driver is always equal to the display pitch (apart from the case reported above).
However cirrus_blt_srcpitch is not always equal to the display pitch because of CIRRUS_BLTMODE_BACKWARDS: cirrus_blt_srcpitch can also be negative and equal to -display_pitch.

I suggest to start using the display pitch (with the proper sign) instead of cirrus_blt_srcpitch in cirrus_do_copy at least when cirrus_blt_srcpitch doesn't have a proper value.

Quoted 9 lines

I think the code should be something like

if bitblt destination intersects display memory:
if bitblt pitch == display pitch
use console_copy
else
invalidate entire display

I think the following should be enough:

if bitblt destination intersects display memory:
qemu_console_copy
else
invalidate region

why do we need if bitblt pitch == display pitch or to invalidate everything?

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

31c05501c says this breaks bitblt, but I can't see why this is true. The copy should update the display. This is probably due to a miscalculation of the affected region, and now we have two invalidates instead of one, reducing performance.

I agree with you: qemu_console_copy does imply that the copied portion of the screen changed, so there is no reason to invalidate it again if qemu_console_copy is called.

Quoted 4 lines

Well, we can't just revert 31c05501c. There's probably another bug involved.

Sure, we have to fix the other one first :)

Quoted 4 lines

I suggest to start using the display pitch (with the proper sign) instead of cirrus_blt_srcpitch in cirrus_do_copy at least when cirrus_blt_srcpitch doesn't have a proper value.

Why switch from one bug to the other?

It's perfectly possible to take into account both values. Of course when abs(blt_pitch) != display pitch we can't use console_copy, but so what.

Quoted 5 lines · Avi Kivity

Quoted 5 lines

I suggest to start using the display pitch (with the proper sign) instead of cirrus_blt_srcpitch in cirrus_do_copy at least when cirrus_blt_srcpitch doesn't have a proper value.

Quoted 6 lines

Why switch from one bug to the other?

It's perfectly possible to take into account both values. Of course when abs(blt_pitch) != display pitch we can't use console_copy, but so what.

I see: you want to support the case when abs(src_blt_pitch) != display_pitch, while I though it is some sort of error.

Considering that src_blt_pitch can even be 0 (as in the bug report), we would still need to calculate sx and sy using display_pitch instead, so the only place in which it would make a difference is when src_blt_pitch is passed as an argument to cirrus_rop.

I guess even a src blt pitch of 0 could be useful there, however in practice I think the only rop function that was written with this case in mind has:

dstpitch -= bltwidth;
srcpitch -= bltwidth;

if (dstpitch<  0 || srcpitch<  0) {
     /* is 0 valid? srcpitch == 0 could be useful */
     return;
}

at the beginning and the others probably just don't deal with the case with possibly buggy consequences.
Also the documentation I have states:

3CEh index 26h W(R/W): BLT Source Pitch (5426 +)
bit 0-11 (5426-28) Number of bytes in a scanline at the source.

     0-12  (5429 +)  do

if the source BLT is supposed to be the number of bytes in a scanline at the source, then 0 is not a correct value for it.

Quoted 20 lines

I guess even a src blt pitch of 0 could be useful there, however in practice I think the only rop function that was written with this case in mind has:

dstpitch -= bltwidth;
srcpitch -= bltwidth;

if (dstpitch<  0 || srcpitch<  0) {
     /* is 0 valid? srcpitch == 0 could be useful */
     return;
}

at the beginning and the others probably just don't deal with the case with possibly buggy consequences.
Also the documentation I have states:

3CEh index 26h W(R/W): BLT Source Pitch (5426 +)
bit 0-11 (5426-28) Number of bytes in a scanline at the source.

     0-12  (5429 +)  do

Quoted 3 lines

if the source BLT is supposed to be the number of bytes in a scanline at the source, then 0 is not a correct value for it.

It's useful if you have a one-line horizontal pattern you want to propagate all over.

Quoted 5 lines

Quoted 5 lines

3CEh index 26h W(R/W): BLT Source Pitch (5426 +)
bit 0-11 (5426-28) Number of bytes in a scanline at the source.

      0-12  (5429 +)  do

Quoted 8 lines

Quoted 4 lines

if the source BLT is supposed to be the number of bytes in a scanline at the source, then 0 is not a correct value for it.

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.

Without any clear indication of what a Cirrus Logic graphic card would have done here, I would choose the safest answer that is disregard the "delicate" case (if it doesn't break Windows NT).

However I don't mind if we try to handle this case too, provided that we handle it well, without SIGFPEs that is :)

Quoted 2 lines · Avi Kivity

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.

Quoted 7 lines · Stefano Stabellini

Quoted 2 lines · Avi Kivity

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.

Quoted 9 lines · Jamie Lokier

Quoted 7 lines · Stefano Stabellini

Quoted 2 lines · Avi Kivity

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.

Quoted 2 lines

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.

hw/cirrus_vga.c +1 -0

index 9f61a01..a7f0d3c 100644
@@ -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;

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Stefano Stabellini

Quoted 3 lines · Avi Kivity

It's useful if you have a one-line horizontal pattern you want to propagate all over.

Quoted 7 lines

Quoted 4 lines

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<[email protected]>

---

hw/cirrus_vga.c +1 -0

index 9f61a01..a7f0d3c 100644
@@ -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;
 

hw/cirrus_vga.c +0 -7

      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

hw/cirrus_vga.c +13 -0

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

hw/cirrus_vga_rop.h +0 -0

index 39a7b72..80f135b 100644
@@ -32,10 +32,10 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
      dstpitch -= bltwidth;

Review

Quoted 6 lines

      /* extra x, y */

(quoted diff) +2 -2

-    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?

Quoted 20 lines

      dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
      dy = (dst / ABS(s->cirrus_blt_dstpitch));

(quoted diff) +1 -12

@@ -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 ||

Review static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) · line 734

Quoted 4 lines

(quoted diff) +3 -0

+             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.

Quoted 25 lines

(quoted diff) +13 -0

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

hw/cirrus_vga_rop.h +0 -0

index 39a7b72..80f135b 100644
@@ -32,10 +32,10 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
      dstpitch -= bltwidth;
      srcpitch -= bltwidth;
 

hw/cirrus_vga_rop.h glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s, · line 39

Quoted 8 lines

hw/cirrus_vga_rop.h +3 -3

-    if (dstpitch<  0 || srcpitch<  0) {
-        /* is 0 valid? srcpitch == 0 could be useful */
+    if (dstpitch<  0)
          return;
-    }
+    if (srcpitch<  0)
+        srcpitch = 0;
 

Why?

Stefano Stabellini wrote:
[]

hw/cirrus_vga.c

Quoted 4 lines

hw/cirrus_vga.c +0 -0

index 9f61a01..81c443b 100644

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

Quoted 3 lines

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.

Quoted 11 lines · Jamie Lokier

Quoted 7 lines · Stefano Stabellini

Quoted 2 lines · Avi Kivity

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<[email protected]>

hw/cirrus_vga.c +21 -14

index 9f61a01..a7f0d3c 100644
@@ -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)

hw/cirrus_vga_rop.h +33 -4

index 39a7b72..80f135b 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Stefano Stabellini

Quoted 3 lines · Avi Kivity

It's useful if you have a one-line horizontal pattern you want to propagate all over.

Quoted 7 lines

Quoted 4 lines

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<[email protected]>

---

hw/cirrus_vga.c +1 -0

index 9f61a01..a7f0d3c 100644
@@ -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);

I guess even a src blt pitch of 0 could be useful there, however in practice I think the only rop function that was written with this case in mind has:

dstpitch -= bltwidth;
srcpitch -= bltwidth;

if (dstpitch<  0 || srcpitch<  0) {
     /* is 0 valid? srcpitch == 0 could be useful */
     return;
}