Another SIGFPE in display code, now in cirrus

Another SIGFPE in display code, now in cirrus

[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
(gdb) p *s
$3 = {vga = {
     vram_ptr = 0xd5e42000 
"\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\37
 7"..., 
vram_offset = 537133056,
     vram_size = 16777216, lfb_addr = 4026531840, lfb_end = 4043309056,
     map_addr = 4026531840, map_end = 4043309056, lfb_vram_mapped = 1,
     bios_offset = 0, bios_size = 0, latch = 3876589584, sr_index = 19 '\023',
     sr = "\003!\017\000\016\000\022\027\000\000\030####\230\000\000\000?\000\004\017$\000\000\000\024\024\024\024-", '\000' <repeats 223 times>,
     gr_index = 56 '8',
     gr = "\000\000\000\000\000@\005\017\377\000\000$", '\000' <repeats 12 times>, "\017\000\000\000\000\000\000\000\001\000\b\000\001\000\000\000\000\246\016\000\000\000\000\000\000\201\016", '\000' <repeats 204 times>, ar_index = 32 ' ',
     ar = "\000\001\002\003\004\005\024\a89:;<=>?\005\000\017\b",
     ar_flip_flop = 1, cr_index = 39 '\'',
     cr = "}cc\200k\032\230\360\000`\016\017\000\000\000\000}#W\310@W\230\303\377\000\000\"", '\000' <repeats 11 times>"\270, ", '\000' <repeats 215 times>,
     msr = 103 'g', fcr = 0 '\000', st00 = 0 '\000', st01 = 0 '\000',
     dac_state = 0 '\000', dac_sub_index = 0 '\000',
     dac_read_index = 16 '\020', dac_write_index = 16 '\020',
     dac_cache = "**?", dac_8bit = 0,
     palette = "\000\000\000\000\000*\000*\000\000***\000\000*\000***\000***\000\000\025\000\000?\000*\025\000*?*\000\025*\000?**\025**?\000\025\000\000\025*\000?\000\000?**\025\000*\025**?\000*?*\000\025\025\000\025?\000?\025\000??*\025\025*\025?*?\025*??\025\000\000\025\000*\025*\000\025**?\000\000?\000*?*\000?**\025\000\025\025\000?\025*\025\025*??\000\025?\000??*\025?*?\025\025\000\025\025*\025?\000\025?*?\025\000?\025*??\000??*\025\025\025\025\025?\025?\025\025???\025\025?\025???\025???", '\000' <repeats 575 times>, bank_offset = 0,
     vga_io_memory = 56, get_bpp = 0x80c70e0 <cirrus_get_bpp>,
     get_offsets = 0x80c6f9e <cirrus_get_offsets>,
     get_resolution = 0x80c717e <cirrus_get_resolution>, vbe_index = 0,
     vbe_regs = {45248, 0, 0, 0, 0, 0, 0, 0, 0, 0}, vbe_start_addr = 0,
     vbe_line_offset = 0, vbe_bank_mask = 255, vbe_mapped = 0, ds = 0x8489fb0,
     font_offsets = {2, 2}, graphic_mode = 1, shift_control = 2 '\002',
     double_scan = 0 '\000', line_offset = 1600, line_compare = 1023,
     start_addr = 0, plane_updated = 0, last_line_offset = 1600,
     last_cw = 9 '\t', last_ch = 16 '\020', last_width = 800,
     last_height = 600, last_scr_width = 800, last_scr_height = 600,
     last_depth = 16, cursor_start = 14 '\016', cursor_end = 15 '\017',
     cursor_offset = 0, rgb_to_pixel = 0x809fadb <rgb_to_pixel16_dup>,
     update = 0x80a19f4 <vga_update_display>,
     invalidate = 0x80a1ac1 <vga_invalidate_display>,
     screen_dump = 0x80a2fda <vga_screen_dump>,
     text_update = 0x80a1e83 <vga_update_text>, invalidated_y_table = {
       0 <repeats 64 times>},
     cursor_invalidate = 0x80c8b9c <cirrus_cursor_invalidate>,
     cursor_draw_line = 0x80c8e33 <cirrus_cursor_draw_line>, last_palette = {0,
       168, 43008, 43176, 11010048, 11010216, 11032320, 11053224, 5723991,
       5724159, 5766999, 5767167, 16734039, 16734207, 16777047, 16777215,
       0 <repeats 240 times>}, last_ch_attr = {0 <repeats 10160 times>,
       4294967295, 0 <repeats 5839 times>},
     retrace = 0x809b64c <vga_dumb_retrace>,
     update_retrace_info = 0x809b298 <vga_dumb_update_retrace_info>,
     retrace_info = {precise = {ticks_per_char = 0, total_chars = 0,
         htotal = 0, hstart = 0, hend = 0, vstart = 0, vend = 0, freq = 0}},
     is_vbe_vmstate = 1 '\001'}, cirrus_linear_io_addr = 64,
   cirrus_linear_bitblt_io_addr = 72, cirrus_mmio_io_addr = 80,
   cirrus_addr_mask = 4194303, linear_mmio_mask = 4194048,
   cirrus_shadow_gr0 = 0 '\000', cirrus_shadow_gr1 = 0 '\000',
   cirrus_hidden_dac_lockindex = 0 '\000', cirrus_hidden_dac_data = 225 '\341',
   cirrus_bank_base = {0, 32768}, cirrus_bank_limit = {4194304, 4161536},
   cirrus_hidden_palette = '\000' <repeats 45 times>"\377, \377\377",
   hw_cursor_x = 0, hw_cursor_y = 0, cirrus_blt_pixelwidth = 1,
   cirrus_blt_width = 2, cirrus_blt_height = 9, cirrus_blt_dstpitch = 1,
   cirrus_blt_srcpitch = 0, cirrus_blt_fgcol = 0, cirrus_blt_bgcol = 0,
   cirrus_blt_dstaddr = 960000, cirrus_blt_srcaddr = 0,
   cirrus_blt_mode = 0 '\000', cirrus_blt_modeext = 0 '\000',
   cirrus_rop = 0x80b60f5 <cirrus_bitblt_rop_fwd_1>,
   cirrus_bltbuf = '\000' <repeats 8191 times>, cirrus_srcptr = 0x8623b94 "",
   cirrus_srcptr_end = 0x8623b94 "", cirrus_srccounter = 0,
   last_hw_cursor_size = 0, last_hw_cursor_x = 0, last_hw_cursor_y = 0,
   last_hw_cursor_y_start = 0, last_hw_cursor_y_end = 0,
   real_vram_size = 4194304, device_id = 184, bustype = 32}

(gdb) bt
#0  0x080c5e45 in cirrus_do_copy (s=0x86134dc, dst=960000, src=0, w=2, h=9)
     at hw/cirrus_vga.c:687
#1  0x080c6226 in cirrus_bitblt_videotovideo_copy (s=0x86134dc)
     at hw/cirrus_vga.c:748
#2  0x080c6692 in cirrus_bitblt_videotovideo (s=0x86134dc)
     at hw/cirrus_vga.c:870
#3  0x080c6ccc in cirrus_bitblt_start (s=0x86134dc)
     at hw/cirrus_vga.c:1011
#4  0x080c7b3c in cirrus_vga_write_gr (s=0x86134dc, reg_index=42, reg_value=14)
     at hw/cirrus_vga.c:1526
#5  0x080c82d1 in cirrus_mmio_blt_write (s=0x86134dc, address=18,
     value=14 '\016') at hw/cirrus_vga.c:1848
#6  0x080c8a79 in cirrus_vga_mem_writeb (opaque=0x86134dc, addr=98322,
     mem_value=14) at hw/cirrus_vga.c:2089
#7  0x080c8b6f in cirrus_vga_mem_writel (opaque=0x86134dc, addr=98320,
     val=960000) at hw/cirrus_vga.c:2120
#8  0x0816b41e in cpu_physical_memory_rw (addr=753680, buf=0xf7fdc270 "",
     len=4, is_write=1) at exec.c:3207
#9  0x08073198 in kvm_run (env=0x847cff0)
     at qemu-kvm.c:937
#10 0x0807454f in kvm_cpu_exec (env=0x847cff0)
     at qemu-kvm.c:1651
#11 0x08074ceb in kvm_main_loop_cpu (env=0x847cff0)
     at qemu-kvm.c:1893
#12 0x08074e36 in ap_main_loop (_env=0x847cff0)
     at qemu-kvm.c:1943
#13 0xf7fad3d0 in start_thread () from /lib/libpthread.so.0
#14 0xf7bb010e in clone () from /lib/libc.so.6


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!

/mjt

Re: Another SIGFPE in display code, now in cirrus

[Michael Tokarev]

07.05.2010 00:07, Michael Tokarev wrote:
> 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.

I just tried current qemu-kvm/master, it crashes at exactly
the same place:

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0xf79dfb70 (LWP 10840)]
0x0821b4ca in cirrus_do_copy (s=0x85dc7ac)
     at hw/cirrus_vga.c:687
687	    sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;

(gdb) bt
#0  0x0821b4ca in cirrus_do_copy (s=0x85dc7ac) at hw/cirrus_vga.c:687
#1  cirrus_bitblt_videotovideo_copy (s=0x85dc7ac) at hw/cirrus_vga.c:748
#2  cirrus_bitblt_videotovideo (s=0x85dc7ac) at hw/cirrus_vga.c:870
#3  cirrus_bitblt_start (s=0x85dc7ac) at hw/cirrus_vga.c:1011
#4  0x0821d009 in cirrus_vga_mem_writel (opaque=0x85dc7ac, addr=98320,
     val=960000) at hw/cirrus_vga.c:2120
#5  0x0811d147 in cpu_physical_memory_rw (addr=753680, buf=0xf7fdc390 "",
     len=4, is_write=1) at exec.c:3475
#6  0x0807b462 in cpu_physical_memory_write () at cpu-common.h:67
#7  kvm_flush_coalesced_mmio_buffer () at kvm-all.c:808
#8  0x0807cc2e in kvm_run (env=0x84c4650) at qemu-kvm.c:575
#9  0x0807d10c in kvm_cpu_exec (env=0x84c4650) at qemu-kvm.c:1192
#10 0x0807ec0a in kvm_main_loop_cpu (_env=0x84c4650) at qemu-kvm.c:1449
#11 ap_main_loop (_env=0x84c4650) at qemu-kvm.c:1495
#12 0xf7fad3d0 in start_thread () from /lib/libpthread.so.0
#13 0xf7cbf10e in clone () from /lib/libc.so.6

> Anything can be done with it?
>
> Thanks!

/mjt

Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

On 05/06/2010 11:07 PM, Michael Tokarev wrote:
> 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?

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

Need to find the spec for the chip.

-- 
error compiling committee.c: too many arguments to function

Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

On 05/10/2010 10:41 AM, Avi Kivity wrote:
> On 05/06/2010 11:07 PM, Michael Tokarev wrote:
>> 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?
>
> 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:

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

-- 
error compiling committee.c: too many arguments to function

Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

On Mon, 10 May 2010, Avi Kivity wrote:
> On 05/10/2010 10:41 AM, Avi Kivity wrote:
> > On 05/06/2010 11:07 PM, Michael Tokarev wrote:
> >> 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?
> >
> > 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.
> 

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.

> Further:
> 
> >
> >     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.
> 

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.

Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

On 05/12/2010 03:20 PM, Stefano Stabellini wrote:
> On Mon, 10 May 2010, Avi Kivity wrote:
>    
>> On 05/10/2010 10:41 AM, Avi Kivity wrote:
>>      
>>> On 05/06/2010 11:07 PM, Michael Tokarev wrote:
>>>        
>>>> 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?
>>>>          
>>> 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.
>>
>>      
> 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.

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

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

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

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

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

On Wed, 12 May 2010, Avi Kivity wrote:
> On 05/12/2010 03:20 PM, Stefano Stabellini wrote:
> > On Mon, 10 May 2010, Avi Kivity wrote:
> >    
> >> On 05/10/2010 10:41 AM, Avi Kivity wrote:
> >>      
> >>> On 05/06/2010 11:07 PM, Michael Tokarev wrote:
> >>>        
> >>>> 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?
> >>>>          
> >>> 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.
> >>
> >>      
> > 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.
>
> > 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.
> 

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.



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


> >> 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.
> >    
> 
> Well, we can't just revert 31c05501c.  There's probably another bug 
> involved.
> 

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

Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

On 05/12/2010 04:45 PM, Stefano Stabellini wrote:
>
>    
>> 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.
>    

Yes.

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

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

Because the region is not a rectangle anymore.  We could compute exactly 
what needs to be invalidated, but since it will never happen, there's no 
point in optimizing it.

>>>> 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.
>>>
>>>        
>> Well, we can't just revert 31c05501c.  There's probably another bug
>> involved.
>>
>>      
> Sure, we have to fix the other one first :)
>    

And find it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

On Wed, 12 May 2010, Avi Kivity wrote:
> > 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.
> 

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.

Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

On 05/12/2010 06:57 PM, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Avi Kivity wrote:
>    
>>> 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.
>>
>>      
> I see: you want to support the case when abs(src_blt_pitch) != display_pitch,
> while I though it is some sort of error.
>    

When blting withinh the screen, it is an error from the point of view of 
writing a sane driver.  My guess is that it's a bug in the NT driver.

It's not an error from the point of view of the hardware, or when blting 
offscreen bitmaps (which can have different geomeries than the display).

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

cirrus_rop() and its arguments don't need to change.  They're correctly 
using the blt pitch.

My point is: x[xy] and d[xy] are only valid if both source and 
destination overlap the display, and if both src_pitch and dst_pitch are 
absequal to the display pitch.  When they aren't valid, there's no point 
in calculating them or using them in anything.  The raster operation is 
still valid though.


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

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

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

On Wed, 12 May 2010, Avi Kivity wrote:
> > 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.
> >    
> 
> 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 :)

Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

On 05/12/2010 07:55 PM, Stefano Stabellini wrote:
>
>>> 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.
>>>
>>>        
>> 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).
>    

My guess is that the src or dst address simply doesn't increment.  I 
think it's also fine to ignore the operation completely.

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


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Jamie Lokier]

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.

-- Jamie

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

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

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Michael Tokarev]

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;
[...]

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

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?



-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

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

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Michael Tokarev]

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

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Stefano Stabellini]

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

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Jamie Lokier]

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

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Michael Tokarev]

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

Re: [Qemu-devel] Re: Another SIGFPE in display code, now in cirrus

[Avi Kivity]

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


-- 
error compiling committee.c: too many arguments to function

Re: Another SIGFPE in display code, now in cirrus

[Paolo Bonzini]

On 05/12/2010 05:57 PM, Stefano Stabellini wrote:
> 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;
> }

Note that here srcpitch == 0 is actually srcpitch == bltwidth, which 
_is_ obviously useful.

The "real" srcpitch == 0 case would result in srcpitch == -bltwidth, and 
it is actually quite useful if you want to stretch a Nx1 bitmap to NxN.

Paolo