Move graphic-related coalesced MMIO flushes to affected device models

PATCH In review Jan Kiszka 5 participants 14 messages Sep 30, 2011 – Oct 19, 2011

Share & tools

Jan Kiszka

author

2011-09-30 10:31 UTC #0 lore raw

PATCH 1/2 Move graphic-related coalesced MMIO flushes to affected device models

This is conceptually cleaner and will allow us to drop the nographic timer. Moreover, it will be mandatory to fully exploit future per-device coalesced MMIO rings.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/g364fb.c |    4 ++++
 hw/vga.c    |    4 ++++
 vl.c        |    2 --
 3 files changed, 8 insertions(+), 2 deletions(-)

hw/g364fb.c +4 -0

index b43341f..f00ee27 100644
@@ -245,6 +245,8 @@ static void g364fb_update_display(void *opaque)
 {
     G364State *s = opaque;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     if (s->width == 0 || s->height == 0)
         return;

@@ -297,6 +299,8 @@ static void g364fb_screen_dump(void *opaque, const char *filename)
     uint8_t *data_buffer;
     FILE *f;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     if (s->depth != 8) {
         error_report("g364: unknown guest depth %d", s->depth);
         return;

hw/vga.c +4 -0

index f9a6014..8712269 100644
@@ -1838,6 +1838,8 @@ static void vga_update_display(void *opaque)
     VGACommonState *s = opaque;
     int full_update, graphic_mode;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     if (ds_get_bits_per_pixel(s->ds) == 0) {
         /* nothing to do */
     } else {

@@ -1958,6 +1960,8 @@ static void vga_update_text(void *opaque, console_ch_t *chardata)
     char msg_buffer[80];
     int full_update = 0;
 
+    qemu_flush_coalesced_mmio_buffer();
+
     if (!(s->ar_index & 0x20)) {
         graphic_mode = GMODE_BLANK;
     } else {

vl.c +0 -2

index bd4a5ce..54d4dd9 100644
@@ -1210,7 +1210,6 @@ static void gui_update(void *opaque)
     DisplayState *ds = opaque;
     DisplayChangeListener *dcl = ds->listeners;
 
-    qemu_flush_coalesced_mmio_buffer();
     dpy_refresh(ds);
 
     while (dcl != NULL) {

@@ -1226,7 +1225,6 @@ static void nographic_update(void *opaque)
 {
     uint64_t interval = GUI_REFRESH_INTERVAL;
 
-    qemu_flush_coalesced_mmio_buffer();
     qemu_mod_timer(nographic_timer, interval + qemu_get_clock_ms(rt_clock));
 }

Signature · 1 line

1.7.3.4

Avi Kivity

2011-10-18 14:01 UTC #2 lore raw

In reply to Jan Kiszka

Quoted 4 lines · Jan Kiszka

This is conceptually cleaner and will allow us to drop the nographic timer. Moreover, it will be mandatory to fully exploit future per-device coalesced MMIO rings.

Appears to break winxp installation - the guest enters an infinite bitblt loop. Trying to find out why.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-10-18 14:06 UTC #3 lore raw

Quoted 8 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka
This is conceptually cleaner and will allow us to drop the nographic timer. Moreover, it will be mandatory to fully exploit future per-device coalesced MMIO rings.
Appears to break winxp installation - the guest enters an infinite bitblt loop. Trying to find out why.

Hmm, maybe there are side effects in certain modes that actually disallow coalescing.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-10-18 14:08 UTC #4 lore raw

Quoted 7 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity
Quoted 4 lines · Jan Kiszka
This is conceptually cleaner and will allow us to drop the nographic timer. Moreover, it will be mandatory to fully exploit future per-device coalesced MMIO rings.

Quoted 6 lines

Quoted 2 lines
Appears to break winxp installation - the guest enters an infinite bitblt loop. Trying to find out why.
Hmm, maybe there are side effects in certain modes that actually disallow coalescing.

That's true for sure, but flushing the buffer should never be wrong.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-10-18 14:10 UTC #5 lore raw

Quoted 8 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Jan Kiszka
This is conceptually cleaner and will allow us to drop the nographic timer. Moreover, it will be mandatory to fully exploit future per-device coalesced MMIO rings.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Appears to break winxp installation - the guest enters an infinite bitblt loop. Trying to find out why.
Hmm, maybe there are side effects in certain modes that actually disallow coalescing.
That's true for sure, but flushing the buffer should never be wrong.

Err, you mean we are not flushing "too often"? I was under the impression winxp is missing our periodic flushes. Do things work again when you do not flush at all?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-10-18 14:30 UTC #6 lore raw

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Quoted 8 lines · Avi Kivity
Quoted 4 lines · Jan Kiszka
This is conceptually cleaner and will allow us to drop the nographic timer. Moreover, it will be mandatory to fully exploit future per-device coalesced MMIO rings.
Appears to break winxp installation - the guest enters an infinite bitblt loop. Trying to find out why.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Hmm, maybe there are side effects in certain modes that actually disallow coalescing.
That's true for sure, but flushing the buffer should never be wrong.
Err, you mean we are not flushing "too often"?

No, I don't know what the exact problem is. What I mean is that an extra flush should never hurt; a missing flush degrades the user experience but shouldn't cause the infinite loops I'm seeing.

Quoted 3 lines

I was under the
impression winxp is missing our periodic flushes. Do things work again when you do not flush at all?

This takes a while to reproduce, let me talk to gdb for a bit.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2011-10-18 16:40 UTC #7 lore raw

Quoted 2 lines · Avi Kivity

This takes a while to reproduce, let me talk to gdb for a bit.

a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which does vga_update_display(), which calls qemu_flush_coalesced_mmio_buffer(), which is not reentrant.

It's easy to make qemu_flush_coalesced_mmio_buffer reentrant:

  if (s->coalesced_flush_in_progress) {
      return;
  }

it isn't very pretty and is also a lie. Other ideas?

I'll probably commit this soon to avoid the regression, to be replaced by a better fix when we find it.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-10-18 16:50 UTC #8 lore raw

Quoted 6 lines · Avi Kivity

Quoted 2 lines · Avi Kivity
This takes a while to reproduce, let me talk to gdb for a bit.
a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which

Why does it have to do vga_hw_update? Why can't it set some flag for the next requested screen update or so? Just thinking, haven't looked at the code yet.

Do you think that only cirrus is affected by this pattern?

Quoted 5 lines

does vga_update_display(), which calls qemu_flush_coalesced_mmio_buffer(), which is not reentrant.
It's easy to make qemu_flush_coalesced_mmio_buffer reentrant:

Quoted 9 lines

  if (s->coalesced_flush_in_progress) {
      return;
  }
it isn't very pretty and is also a lie. Other ideas?
I'll probably commit this soon to avoid the regression, to be replaced by a better fix when we find it.

Agreed. Unless we can avoid that recursion at devices level, there is likely no alternative.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-10-18 17:34 UTC #9 lore raw

Quoted 11 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity
Quoted 2 lines · Avi Kivity
This takes a while to reproduce, let me talk to gdb for a bit.
a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which
Why does it have to do vga_hw_update? Why can't it set some flag for the next requested screen update or so? Just thinking, haven't looked at the code yet.

Maybe it's a remnant from the days where it asked the host hardware to do the blt.

Quoted 1 line

Do you think that only cirrus is affected by this pattern?

It's also possible for hotunplug:

- hotunplug
- unregister coalesced regions
- flush mmios
- call back into same device

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2011-10-18 19:50 UTC #10 lore raw

Quoted 9 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Avi Kivity
This takes a while to reproduce, let me talk to gdb for a bit.
a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which

Quoted 6 lines

Quoted 3 lines
Why does it have to do vga_hw_update? Why can't it set some flag for the next requested screen update or so? Just thinking, haven't looked at the code yet.
Maybe it's a remnant from the days where it asked the host hardware to do the blt.

If it's no longer needed - drop it? Already for other reasons like efficiency.

Quoted 9 lines

Quoted 2 lines
Do you think that only cirrus is affected by this pattern?
It's also possible for hotunplug:
- hotunplug
- unregister coalesced regions
- flush mmios
- call back into same device

Which device triggers hotunplug via a coalesced mmio region?

Anyway, if we want to avoid other surprises like that, better make kvm_flush_coalesced_mmio_buffer reentrance-safe. If we think that this remains an odd scenario, issue a warning to the console that some device may require fixing.

Jan

Avi Kivity

2011-10-19 09:04 UTC #11 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 14 lines · Jan Kiszka

Quoted 13 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Avi Kivity
This takes a while to reproduce, let me talk to gdb for a bit.
a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which
Why does it have to do vga_hw_update? Why can't it set some flag for the next requested screen update or so? Just thinking, haven't looked at the code yet.

Quoted 5 lines

Quoted 2 lines
Maybe it's a remnant from the days where it asked the host hardware to do the blt.
If it's no longer needed - drop it? Already for other reasons like efficiency.

I think it actually is needed - it calls qemu_console_copy() to do the copy. Which incidentally means the the coalesced flush, had it worked, would be a bug: it would bring pending mmio writes in front of a currently executing bitblt. I don't think we can regard my hack as a fix for that. Maybe we need to revert the original patch. Or make sure the flush only happens from the display thread.

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Do you think that only cirrus is affected by this pattern?

Quoted 8 lines

Quoted 6 lines
It's also possible for hotunplug:
- hotunplug
- unregister coalesced regions
- flush mmios
- call back into same device
Which device triggers hotunplug via a coalesced mmio region?

Er, none.

Quoted 5 lines

Anyway, if we want to avoid other surprises like that, better make kvm_flush_coalesced_mmio_buffer reentrance-safe. If we think that this remains an odd scenario, issue a warning to the console that some device may require fixing.

- --
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJOnpKTAAoJEI7yEDeUysxlWjkP/3KubuwPQXYV6Fh8EjLXYSNG ClxiqOvGIy8lHqdpZON2iuv5RiFcReeUZlFVXhRcjJ28tSb2ZYh8qxb/mRE82U8p 1sPS+kEH8k+lvzF38LjUc9XwWdjLLPiXrm3xWX/3uGvotbMKezS+2WGqc5hN7l8U bc1pKLXQJ8YTIPq1seMv+ncVUcS3yaYSHMagkwjnQ4MDo8mhyPadfkkyv2BBLMM6 lTI9w5QvZ6JoWAHy/7KEEqyzs06ssfXOc/rSQLcHXn0S4CyVsmuGu3B/grHNqKIp 8OE0gZgDcA00E+YvXdOUGPMJ+XHEn/BCH2vrRf1TAlWq+zRPwXTrY3SZbauuBV/+ x0991xE2fMPF3o03OybwqnbQFrqtPhhYKXu9Dt/mBeITOkcvEVJjnO6ooCeqirmA GrA26ls0I5+oZxfC3qmwJnO/WmGhZCEBS4YAav/4o+t1Ae2bjj/A23kcW/W+RUW7 5I2WTcbhe/+zqtalJg68F//8PSWmCnF4njxdHR3sRyhkzuDS1Ue99bGi0eugkYrM 71q0r17SnOK8Mo7tPn4FP0eeSEpTnxzS5vScf60IV0p3wIzgTPqeDgs+73v6AzO1 Yu3efGmh2q+UFVgUOhkDiFkPoQeaUSnpNEhpwBDGctGjqMyKpildkZB6DFY/+hle 9+Id+qSPeQNzoO62bfGi
=n5PD
-----END PGP SIGNATURE-----

Jan Kiszka

author

2011-10-19 11:19 UTC #12 lore raw

Quoted 22 lines · Avi Kivity

Quoted 14 lines · Jan Kiszka
Quoted 13 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 6 lines · Avi Kivity
Quoted 2 lines · Avi Kivity
This takes a while to reproduce, let me talk to gdb for a bit.
a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which
Why does it have to do vga_hw_update? Why can't it set some flag for the next requested screen update or so? Just thinking, haven't looked at the code yet.
Quoted 5 lines
Quoted 2 lines
Maybe it's a remnant from the days where it asked the host hardware to do the blt.
If it's no longer needed - drop it? Already for other reasons like efficiency.

Quoted 6 lines

I think it actually is needed - it calls qemu_console_copy() to do the copy. Which incidentally means the the coalesced flush, had it worked, would be a bug: it would bring pending mmio writes in front of a currently executing bitblt. I don't think we can regard my hack as a fix for that. Maybe we need to revert the original patch. Or make sure the flush only happens from the display thread.

I hope we can avoid the old scheme as it hurts when trying to make progress /wrt scalability. Will have a look if we can avoid the recursion in some reasonable way at device level here.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Alon Levy

2011-10-18 18:09 UTC #13 lore raw

In reply to Jan Kiszka

Quoted 11 lines · Jan Kiszka

Quoted 6 lines · Avi Kivity
Quoted 2 lines · Avi Kivity
This takes a while to reproduce, let me talk to gdb for a bit.
a vcpu exit causes kvm_flush_coalesced_mmio_buffer() to run, which does a bitblt, which is cirrus_do_copy(), which goes to vga_hw_update, which
Why does it have to do vga_hw_update? Why can't it set some flag for the next requested screen update or so? Just thinking, haven't looked at the code yet.

bottomhalf?

Quoted 27 lines

Do you think that only cirrus is affected by this pattern?
Quoted 5 lines
does vga_update_display(), which calls qemu_flush_coalesced_mmio_buffer(), which is not reentrant.
It's easy to make qemu_flush_coalesced_mmio_buffer reentrant:
Quoted 9 lines
  if (s->coalesced_flush_in_progress) {
      return;
  }
it isn't very pretty and is also a lie. Other ideas?
I'll probably commit this soon to avoid the regression, to be replaced by a better fix when we find it.
Agreed. Unless we can avoid that recursion at devices level, there is likely no alternative.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux