Patchwork [1/2] Move graphic-related coalesced MMIO flushes to affected device models

login
register
mail settings
Submitter Jan Kiszka
Date Sept. 30, 2011, 10:31 a.m.
Message ID <4E859A72.9040007@siemens.com>
Download mbox | patch
Permalink /patch/117076/
State New
Headers show

Comments

Jan Kiszka - Sept. 30, 2011, 10:31 a.m.
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 <jan.kiszka@siemens.com>
---
 hw/g364fb.c |    4 ++++
 hw/vga.c    |    4 ++++
 vl.c        |    2 --
 3 files changed, 8 insertions(+), 2 deletions(-)
Blue Swirl - Oct. 15, 2011, 9:35 p.m.
Thanks, applied both. I tested ARM and Sparc32 nographic modes which
IIRC were the problem previously, both seem to work fine.

On Fri, Sep 30, 2011 at 10:31 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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 <jan.kiszka@siemens.com>
> ---
>  hw/g364fb.c |    4 ++++
>  hw/vga.c    |    4 ++++
>  vl.c        |    2 --
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/g364fb.c b/hw/g364fb.c
> index b43341f..f00ee27 100644
> --- a/hw/g364fb.c
> +++ b/hw/g364fb.c
> @@ -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;
> diff --git a/hw/vga.c b/hw/vga.c
> index f9a6014..8712269 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -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 {
> diff --git a/vl.c b/vl.c
> index bd4a5ce..54d4dd9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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));
>  }
>
> --
> 1.7.3.4
>
>
Avi Kivity - Oct. 18, 2011, 2 p.m.
On 09/30/2011 01:31 PM, Jan Kiszka wrote:
> 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.
Jan Kiszka - Oct. 18, 2011, 2:05 p.m.
On 2011-10-18 16:00, Avi Kivity wrote:
> On 09/30/2011 01:31 PM, Jan Kiszka wrote:
>> 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
Avi Kivity - Oct. 18, 2011, 2:08 p.m.
On 10/18/2011 04:05 PM, Jan Kiszka wrote:
> On 2011-10-18 16:00, Avi Kivity wrote:
> > On 09/30/2011 01:31 PM, Jan Kiszka wrote:
> >> 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.
>

That's true for sure, but flushing the buffer should never be wrong.
Jan Kiszka - Oct. 18, 2011, 2:10 p.m.
On 2011-10-18 16:08, Avi Kivity wrote:
> On 10/18/2011 04:05 PM, Jan Kiszka wrote:
>> On 2011-10-18 16:00, Avi Kivity wrote:
>>> On 09/30/2011 01:31 PM, Jan Kiszka wrote:
>>>> 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.
>>
> 
> 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
Avi Kivity - Oct. 18, 2011, 2:30 p.m.
On 10/18/2011 04:10 PM, Jan Kiszka wrote:
> On 2011-10-18 16:08, Avi Kivity wrote:
> > On 10/18/2011 04:05 PM, Jan Kiszka wrote:
> >> On 2011-10-18 16:00, Avi Kivity wrote:
> >>> On 09/30/2011 01:31 PM, Jan Kiszka wrote:
> >>>> 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.
> >>
> > 
> > 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.

> 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.
Avi Kivity - Oct. 18, 2011, 4:40 p.m.
On 10/18/2011 04:30 PM, Avi Kivity wrote:
> 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.
Jan Kiszka - Oct. 18, 2011, 4:49 p.m.
On 2011-10-18 18:40, Avi Kivity wrote:
> On 10/18/2011 04:30 PM, Avi Kivity wrote:
>> 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?

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

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

Jan
Avi Kivity - Oct. 18, 2011, 5:34 p.m.
On 10/18/2011 06:49 PM, Jan Kiszka wrote:
> On 2011-10-18 18:40, Avi Kivity wrote:
> > On 10/18/2011 04:30 PM, Avi Kivity wrote:
> >> 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.

> 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
Alon Levy - Oct. 18, 2011, 6:06 p.m.
On Tue, Oct 18, 2011 at 06:49:57PM +0200, Jan Kiszka wrote:
> On 2011-10-18 18:40, Avi Kivity wrote:
> > On 10/18/2011 04:30 PM, Avi Kivity wrote:
> >> 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?

> 
> Do you think that only cirrus is affected by this pattern?
> 
> > 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.
> > 
> 
> Agreed. Unless we can avoid that recursion at devices level, there is
> likely no alternative.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
>
Jan Kiszka - Oct. 18, 2011, 7:50 p.m.
On 2011-10-18 19:34, Avi Kivity wrote:
> On 10/18/2011 06:49 PM, Jan Kiszka wrote:
>> On 2011-10-18 18:40, Avi Kivity wrote:
>>> On 10/18/2011 04:30 PM, Avi Kivity wrote:
>>>> 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.

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

> 
>> 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 - Oct. 19, 2011, 9:04 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/18/2011 09:50 PM, Jan Kiszka wrote:
> On 2011-10-18 19:34, Avi Kivity wrote:
> > On 10/18/2011 06:49 PM, Jan Kiszka wrote:
> >> On 2011-10-18 18:40, Avi Kivity wrote:
> >>> On 10/18/2011 04:30 PM, Avi Kivity wrote:
> >>>> 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.
>
> 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.

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

Er, none.

> 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 - Oct. 19, 2011, 11:18 a.m.
On 2011-10-19 11:04, Avi Kivity wrote:
> 
> On 10/18/2011 09:50 PM, Jan Kiszka wrote:
>> On 2011-10-18 19:34, Avi Kivity wrote:
>>> On 10/18/2011 06:49 PM, Jan Kiszka wrote:
>>>> On 2011-10-18 18:40, Avi Kivity wrote:
>>>>> On 10/18/2011 04:30 PM, Avi Kivity wrote:
>>>>>> 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.
> 
>> 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.

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

Patch

diff --git a/hw/g364fb.c b/hw/g364fb.c
index b43341f..f00ee27 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -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;
diff --git a/hw/vga.c b/hw/vga.c
index f9a6014..8712269 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -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 {
diff --git a/vl.c b/vl.c
index bd4a5ce..54d4dd9 100644
--- a/vl.c
+++ b/vl.c
@@ -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));
 }