| Message ID | 1330956981-5001-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/05/2012 08:16 AM, Alon Levy wrote: > adds a handler for the following qmp screendump-async command. > > graphics_console_init signature change required touching every user, but > no implementation of the new vga_hw_screen_dump_async_ptr is provided > in this patch. > > Signed-off-by: Alon Levy<alevy@redhat.com> > --- > console.c | 4 ++++ > console.h | 5 +++++ > hw/blizzard.c | 2 +- > hw/cirrus_vga.c | 4 ++-- > hw/exynos4210_fimd.c | 3 ++- > hw/g364fb.c | 2 +- > hw/jazz_led.c | 3 ++- > hw/milkymist-vgafb.c | 2 +- > hw/musicpal.c | 2 +- > hw/omap_dss.c | 4 +++- > hw/omap_lcdc.c | 2 +- > hw/pl110.c | 2 +- > hw/pxa2xx_lcd.c | 2 +- > hw/qxl.c | 3 ++- > hw/sm501.c | 4 ++-- > hw/ssd0303.c | 2 +- > hw/ssd0323.c | 2 +- > hw/tc6393xb.c | 1 + > hw/tcx.c | 4 ++-- > hw/vga-isa-mm.c | 3 ++- > hw/vga-isa.c | 3 ++- > hw/vga-pci.c | 3 ++- > hw/vga_int.h | 1 + > hw/vmware_vga.c | 3 ++- > hw/xenfb.c | 2 ++ > 25 files changed, 45 insertions(+), 23 deletions(-) > > diff --git a/console.c b/console.c > index 25d8acb..9a49e93 100644 > --- a/console.c > +++ b/console.c > @@ -124,6 +124,7 @@ struct TextConsole { > vga_hw_update_ptr hw_update; > vga_hw_invalidate_ptr hw_invalidate; > vga_hw_screen_dump_ptr hw_screen_dump; > + vga_hw_screen_dump_async_ptr hw_screen_dump_async; > vga_hw_text_update_ptr hw_text_update; > void *hw; > > @@ -1403,6 +1404,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, > vga_hw_invalidate_ptr invalidate, > vga_hw_screen_dump_ptr screen_dump, > vga_hw_text_update_ptr text_update, > + vga_hw_screen_dump_async_ptr > + screen_dump_async, > void *opaque) > { > TextConsole *s; > @@ -1421,6 +1424,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, > s->hw_update = update; > s->hw_invalidate = invalidate; > s->hw_screen_dump = screen_dump; > + s->hw_screen_dump_async = screen_dump_async; > s->hw_text_update = text_update; > s->hw = opaque; > > diff --git a/console.h b/console.h > index c22803c..3cf28c0 100644 > --- a/console.h > +++ b/console.h > @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch) > typedef void (*vga_hw_update_ptr)(void *); > typedef void (*vga_hw_invalidate_ptr)(void *); > typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch); > +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename, > + bool cswitch); > typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *); > > DisplayState *graphic_console_init(vga_hw_update_ptr update, > vga_hw_invalidate_ptr invalidate, > vga_hw_screen_dump_ptr screen_dump, > vga_hw_text_update_ptr text_update, > + vga_hw_screen_dump_async_ptr > + screen_dump_async, > void *opaque); async in QEMU doesn't mean "generate a QMP event when you're done". It should mean execute this closure when you finish (function pointer + opaque). The QMP event should be dispatched from the closure such that the screendump code doesn't have to have a direct dependency on QMP. Regards, Anthony Liguori
On Mon, Mar 05, 2012 at 08:33:06AM -0600, Anthony Liguori wrote: > On 03/05/2012 08:16 AM, Alon Levy wrote: > >adds a handler for the following qmp screendump-async command. > > > >graphics_console_init signature change required touching every user, but > >no implementation of the new vga_hw_screen_dump_async_ptr is provided > >in this patch. > > > >Signed-off-by: Alon Levy<alevy@redhat.com> > >--- > > console.c | 4 ++++ > > console.h | 5 +++++ > > hw/blizzard.c | 2 +- > > hw/cirrus_vga.c | 4 ++-- > > hw/exynos4210_fimd.c | 3 ++- > > hw/g364fb.c | 2 +- > > hw/jazz_led.c | 3 ++- > > hw/milkymist-vgafb.c | 2 +- > > hw/musicpal.c | 2 +- > > hw/omap_dss.c | 4 +++- > > hw/omap_lcdc.c | 2 +- > > hw/pl110.c | 2 +- > > hw/pxa2xx_lcd.c | 2 +- > > hw/qxl.c | 3 ++- > > hw/sm501.c | 4 ++-- > > hw/ssd0303.c | 2 +- > > hw/ssd0323.c | 2 +- > > hw/tc6393xb.c | 1 + > > hw/tcx.c | 4 ++-- > > hw/vga-isa-mm.c | 3 ++- > > hw/vga-isa.c | 3 ++- > > hw/vga-pci.c | 3 ++- > > hw/vga_int.h | 1 + > > hw/vmware_vga.c | 3 ++- > > hw/xenfb.c | 2 ++ > > 25 files changed, 45 insertions(+), 23 deletions(-) > > > >diff --git a/console.c b/console.c > >index 25d8acb..9a49e93 100644 > >--- a/console.c > >+++ b/console.c > >@@ -124,6 +124,7 @@ struct TextConsole { > > vga_hw_update_ptr hw_update; > > vga_hw_invalidate_ptr hw_invalidate; > > vga_hw_screen_dump_ptr hw_screen_dump; > >+ vga_hw_screen_dump_async_ptr hw_screen_dump_async; > > vga_hw_text_update_ptr hw_text_update; > > void *hw; > > > >@@ -1403,6 +1404,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, > > vga_hw_invalidate_ptr invalidate, > > vga_hw_screen_dump_ptr screen_dump, > > vga_hw_text_update_ptr text_update, > >+ vga_hw_screen_dump_async_ptr > >+ screen_dump_async, > > void *opaque) > > { > > TextConsole *s; > >@@ -1421,6 +1424,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, > > s->hw_update = update; > > s->hw_invalidate = invalidate; > > s->hw_screen_dump = screen_dump; > >+ s->hw_screen_dump_async = screen_dump_async; > > s->hw_text_update = text_update; > > s->hw = opaque; > > > >diff --git a/console.h b/console.h > >index c22803c..3cf28c0 100644 > >--- a/console.h > >+++ b/console.h > >@@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch) > > typedef void (*vga_hw_update_ptr)(void *); > > typedef void (*vga_hw_invalidate_ptr)(void *); > > typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch); > >+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename, > >+ bool cswitch); > > typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *); > > > > DisplayState *graphic_console_init(vga_hw_update_ptr update, > > vga_hw_invalidate_ptr invalidate, > > vga_hw_screen_dump_ptr screen_dump, > > vga_hw_text_update_ptr text_update, > >+ vga_hw_screen_dump_async_ptr > >+ screen_dump_async, > > void *opaque); > > > async in QEMU doesn't mean "generate a QMP event when you're done". > It should mean execute this closure when you finish (function > pointer + opaque). > > The QMP event should be dispatched from the closure such that the > screendump code doesn't have to have a direct dependency on QMP. Yes, I can add a closure (I could put the filename there, that's the only thing we need to have there right now). Note that there is already a function wrapping the specific QEVENT used. > > Regards, > > Anthony Liguori
v2->v3: pass a closure to hw_screen_dump_async Alon Levy (3): monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE console: add hw_screen_dump_async add qmp screendump-async QMP/qmp-events.txt | 15 +++++++++++ console.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++- console.h | 9 +++++++ hw/blizzard.c | 2 +- hw/cirrus_vga.c | 4 +- hw/exynos4210_fimd.c | 3 +- hw/g364fb.c | 2 +- hw/jazz_led.c | 3 +- hw/milkymist-vgafb.c | 2 +- hw/musicpal.c | 2 +- hw/omap_dss.c | 4 ++- hw/omap_lcdc.c | 2 +- hw/pl110.c | 2 +- hw/pxa2xx_lcd.c | 2 +- hw/qxl.c | 3 +- hw/sm501.c | 4 +- hw/ssd0303.c | 2 +- hw/ssd0323.c | 2 +- hw/tc6393xb.c | 1 + hw/tcx.c | 4 +- hw/vga-isa-mm.c | 3 +- hw/vga-isa.c | 3 +- hw/vga-pci.c | 3 +- hw/vga_int.h | 1 + hw/vmware_vga.c | 3 +- hw/xenfb.c | 2 + monitor.c | 7 +++++ monitor.h | 1 + qapi-schema.json | 20 +++++++++++++++ qmp-commands.hx | 26 +++++++++++++++++++ 30 files changed, 178 insertions(+), 25 deletions(-)
On 03/05/2012 09:56 AM, Alon Levy wrote: > v2->v3: > pass a closure to hw_screen_dump_async > > Alon Levy (3): > monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE > console: add hw_screen_dump_async > add qmp screendump-async Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Regards, Anthony Liguori > > QMP/qmp-events.txt | 15 +++++++++++ > console.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++- > console.h | 9 +++++++ > hw/blizzard.c | 2 +- > hw/cirrus_vga.c | 4 +- > hw/exynos4210_fimd.c | 3 +- > hw/g364fb.c | 2 +- > hw/jazz_led.c | 3 +- > hw/milkymist-vgafb.c | 2 +- > hw/musicpal.c | 2 +- > hw/omap_dss.c | 4 ++- > hw/omap_lcdc.c | 2 +- > hw/pl110.c | 2 +- > hw/pxa2xx_lcd.c | 2 +- > hw/qxl.c | 3 +- > hw/sm501.c | 4 +- > hw/ssd0303.c | 2 +- > hw/ssd0323.c | 2 +- > hw/tc6393xb.c | 1 + > hw/tcx.c | 4 +- > hw/vga-isa-mm.c | 3 +- > hw/vga-isa.c | 3 +- > hw/vga-pci.c | 3 +- > hw/vga_int.h | 1 + > hw/vmware_vga.c | 3 +- > hw/xenfb.c | 2 + > monitor.c | 7 +++++ > monitor.h | 1 + > qapi-schema.json | 20 +++++++++++++++ > qmp-commands.hx | 26 +++++++++++++++++++ > 30 files changed, 178 insertions(+), 25 deletions(-) >
On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > async in QEMU doesn't mean "generate a QMP event when you're done". > It should mean execute this closure when you finish (function pointer > + opaque). > > The QMP event should be dispatched from the closure such that the > screendump code doesn't have to have a direct dependency on QMP. > What about using the parallel execution facility of qmp? It's silly to duplicate every command X with X-async and X-COMPLETED.
On 03/05/2012 11:20 AM, Avi Kivity wrote: > On 03/05/2012 04:33 PM, Anthony Liguori wrote: >> >> >> async in QEMU doesn't mean "generate a QMP event when you're done". >> It should mean execute this closure when you finish (function pointer >> + opaque). >> >> The QMP event should be dispatched from the closure such that the >> screendump code doesn't have to have a direct dependency on QMP. >> > > What about using the parallel execution facility of qmp? It's silly to > duplicate every command X with X-async and X-COMPLETED. We need to switch over to QAPI to get there. We're pretty close to being there. Luiz, about how long do you think before we get there? Regards, Anthony Liguori >
On 03/05/2012 07:27 PM, Anthony Liguori wrote: > On 03/05/2012 11:20 AM, Avi Kivity wrote: >> On 03/05/2012 04:33 PM, Anthony Liguori wrote: >>> >>> >>> async in QEMU doesn't mean "generate a QMP event when you're done". >>> It should mean execute this closure when you finish (function pointer >>> + opaque). >>> >>> The QMP event should be dispatched from the closure such that the >>> screendump code doesn't have to have a direct dependency on QMP. >>> >> >> What about using the parallel execution facility of qmp? It's silly to >> duplicate every command X with X-async and X-COMPLETED. > > We need to switch over to QAPI to get there. Just an implementation detail, yes? No spec/protocol changes? > We're pretty close to being there. Luiz, about how long do you > think before we get there? It's a pity to add new commands along the way.
On Mon, 05 Mar 2012 11:27:07 -0600 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > >> > >> > >> async in QEMU doesn't mean "generate a QMP event when you're done". > >> It should mean execute this closure when you finish (function pointer > >> + opaque). > >> > >> The QMP event should be dispatched from the closure such that the > >> screendump code doesn't have to have a direct dependency on QMP. > >> > > > > What about using the parallel execution facility of qmp? It's silly to > > duplicate every command X with X-async and X-COMPLETED. > > We need to switch over to QAPI to get there. We're pretty close to being there. > Luiz, about how long do you think before we get there? It's a bit hard to tell, because it depends on upstream review plus me not being bogged down with other stuff. But maybe in around two weeks when I resume my work on this.
On Mon, 05 Mar 2012 19:29:14 +0200 Avi Kivity <avi@redhat.com> wrote: > On 03/05/2012 07:27 PM, Anthony Liguori wrote: > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > >> On 03/05/2012 04:33 PM, Anthony Liguori wrote: > >>> > >>> > >>> async in QEMU doesn't mean "generate a QMP event when you're done". > >>> It should mean execute this closure when you finish (function pointer > >>> + opaque). > >>> > >>> The QMP event should be dispatched from the closure such that the > >>> screendump code doesn't have to have a direct dependency on QMP. > >>> > >> > >> What about using the parallel execution facility of qmp? It's silly to > >> duplicate every command X with X-async and X-COMPLETED. > > > > We need to switch over to QAPI to get there. > > Just an implementation detail, yes? No spec/protocol changes? We haven't discussed it yet how to do async commands in detail, so it may or may not have protocol changes. I have a simple proposal in mind, but haven't submitted it yet. > > We're pretty close to being there. Luiz, about how long do you > > think before we get there? > > It's a pity to add new commands along the way. We decided not to block useful features because of the long time that's taking to do async support properly.
On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > On Mon, 05 Mar 2012 11:27:07 -0600 > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > >> > > >> > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > >> It should mean execute this closure when you finish (function pointer > > >> + opaque). > > >> > > >> The QMP event should be dispatched from the closure such that the > > >> screendump code doesn't have to have a direct dependency on QMP. > > >> > > > > > > What about using the parallel execution facility of qmp? It's silly to > > > duplicate every command X with X-async and X-COMPLETED. How would the parallel execution facility be opaque to the implementer? screendump returns, screendump_async needs to pass a closure. You can automatically generate any amount of code, but you can only have a single function implementation with longjmp/coroutine, or having a saparate thread per command but that would mean taking locks for anything not trivial, which avoids the no-change-to-implementation. Is this what you have in mind? > > > > We need to switch over to QAPI to get there. We're pretty close to being there. > > Luiz, about how long do you think before we get there? > > It's a bit hard to tell, because it depends on upstream review plus me not > being bogged down with other stuff. But maybe in around two weeks when I > resume my work on this. >
On 03/05/2012 11:29 AM, Avi Kivity wrote: > On 03/05/2012 07:27 PM, Anthony Liguori wrote: >> On 03/05/2012 11:20 AM, Avi Kivity wrote: >>> On 03/05/2012 04:33 PM, Anthony Liguori wrote: >>>> >>>> >>>> async in QEMU doesn't mean "generate a QMP event when you're done". >>>> It should mean execute this closure when you finish (function pointer >>>> + opaque). >>>> >>>> The QMP event should be dispatched from the closure such that the >>>> screendump code doesn't have to have a direct dependency on QMP. >>>> >>> >>> What about using the parallel execution facility of qmp? It's silly to >>> duplicate every command X with X-async and X-COMPLETED. >> >> We need to switch over to QAPI to get there. > > Just an implementation detail, yes? No spec/protocol changes? Correct. > >> We're pretty close to being there. Luiz, about how long do you >> think before we get there? > > It's a pity to add new commands along the way. It's more complicated than this unfortunately. A client needs to be able to determine whether the 'screendump' command works as expected. Unfortunately, when QXL was introduced, 'screendump' became broken across multiple releases. screendump is the right interface, but since it was broken in multiple releases, we need another command for a client to determine that it's not broken. In the short term, screendump_async is that. After QAPI, it will probably be a screendump2. I don't mind introducing short term commands and then deprecating it particularly when they are marked as such. Regards, Anthony Liguori
On 03/05/2012 08:09 PM, Alon Levy wrote: > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > > On Mon, 05 Mar 2012 11:27:07 -0600 > > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > >> > > > >> > > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > > >> It should mean execute this closure when you finish (function pointer > > > >> + opaque). > > > >> > > > >> The QMP event should be dispatched from the closure such that the > > > >> screendump code doesn't have to have a direct dependency on QMP. > > > >> > > > > > > > > What about using the parallel execution facility of qmp? It's silly to > > > > duplicate every command X with X-async and X-COMPLETED. > > How would the parallel execution facility be opaque to the implementer? > screendump returns, screendump_async needs to pass a closure. You can > automatically generate any amount of code, but you can only have a > single function implementation with longjmp/coroutine, or having a > saparate thread per command but that would mean taking locks for > anything not trivial, which avoids the no-change-to-implementation. Is > this what you have in mind? It would not be opaque to the implementer. But it would avoid introducing new commands and events, instead we have a unified mechanism to signal completion.
On 03/05/2012 08:16 PM, Anthony Liguori wrote: > >> >>> We're pretty close to being there. Luiz, about how long do you >>> think before we get there? >> >> It's a pity to add new commands along the way. > > It's more complicated than this unfortunately. > > A client needs to be able to determine whether the 'screendump' > command works as expected. Unfortunately, when QXL was introduced, > 'screendump' became broken across multiple releases. > > screendump is the right interface, but since it was broken in multiple > releases, we need another command for a client to determine that it's > not broken. In the short term, screendump_async is that. After QAPI, > it will probably be a screendump2. > > I don't mind introducing short term commands and then deprecating it > particularly when they are marked as such. Zooming out from screendump, there are several ways to deal with broken commands: 1. introduce new commands 2. introduce capabilities that say "screendump is fixed wrt bug so-and-so" 3. fix it and backport the fix to a stable release 4. ignore the broken command and hope My preference is 3->2->1->4, or we'll end up with screendump65535 soon.
On Mon, Mar 05, 2012 at 08:17:52PM +0200, Avi Kivity wrote: > On 03/05/2012 08:09 PM, Alon Levy wrote: > > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > > > On Mon, 05 Mar 2012 11:27:07 -0600 > > > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > > >> > > > > >> > > > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > > > >> It should mean execute this closure when you finish (function pointer > > > > >> + opaque). > > > > >> > > > > >> The QMP event should be dispatched from the closure such that the > > > > >> screendump code doesn't have to have a direct dependency on QMP. > > > > >> > > > > > > > > > > What about using the parallel execution facility of qmp? It's silly to > > > > > duplicate every command X with X-async and X-COMPLETED. > > > > How would the parallel execution facility be opaque to the implementer? > > screendump returns, screendump_async needs to pass a closure. You can > > automatically generate any amount of code, but you can only have a > > single function implementation with longjmp/coroutine, or having a > > saparate thread per command but that would mean taking locks for > > anything not trivial, which avoids the no-change-to-implementation. Is > > this what you have in mind? > > It would not be opaque to the implementer. But it would avoid > introducing new commands and events, instead we have a unified mechanism > to signal completion. Yeah, that sounds good. Let's have that for 6.4. > > -- > error compiling committee.c: too many arguments to function > >
On 03/05/2012 12:22 PM, Avi Kivity wrote: > On 03/05/2012 08:16 PM, Anthony Liguori wrote: >> >>> >>>> We're pretty close to being there. Luiz, about how long do you >>>> think before we get there? >>> >>> It's a pity to add new commands along the way. >> >> It's more complicated than this unfortunately. >> >> A client needs to be able to determine whether the 'screendump' >> command works as expected. Unfortunately, when QXL was introduced, >> 'screendump' became broken across multiple releases. >> >> screendump is the right interface, but since it was broken in multiple >> releases, we need another command for a client to determine that it's >> not broken. In the short term, screendump_async is that. After QAPI, >> it will probably be a screendump2. >> >> I don't mind introducing short term commands and then deprecating it >> particularly when they are marked as such. > > Zooming out from screendump, there are several ways to deal with broken > commands: > > 1. introduce new commands This is our only short term options for this specific circumstance. > 2. introduce capabilities that say "screendump is fixed wrt bug so-and-so" We don't have such a mechanism and I generally would prefer to avoid it. There's a certain shame in introducing new commands that hopefully will cause us to be more careful in the future. > 3. fix it and backport the fix to a stable release This is only not practical because it requires such an infrastructure change. > 4. ignore the broken command and hope 5. just tell clients to rely on version information and ignore the existence of downstreams This is really mostly a downstream problem, not an upstream problem. Version numbers can be reliable here for upstream. This is the approach that would be typically taken for a C library. The flip side is that this is also while we end up with autotools and a bunch of compile time tests to determine what broken functions exist. Regards, Anthony Liguori > > My preference is 3->2->1->4, or we'll end up with screendump65535 soon. >
On Mon, 5 Mar 2012 20:58:08 +0200 Alon Levy <alevy@redhat.com> wrote: > On Mon, Mar 05, 2012 at 08:17:52PM +0200, Avi Kivity wrote: > > On 03/05/2012 08:09 PM, Alon Levy wrote: > > > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > > > > On Mon, 05 Mar 2012 11:27:07 -0600 > > > > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > > > > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > > > >> > > > > > >> > > > > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > > > > >> It should mean execute this closure when you finish (function pointer > > > > > >> + opaque). > > > > > >> > > > > > >> The QMP event should be dispatched from the closure such that the > > > > > >> screendump code doesn't have to have a direct dependency on QMP. > > > > > >> > > > > > > > > > > > > What about using the parallel execution facility of qmp? It's silly to > > > > > > duplicate every command X with X-async and X-COMPLETED. > > > > > > How would the parallel execution facility be opaque to the implementer? > > > screendump returns, screendump_async needs to pass a closure. You can > > > automatically generate any amount of code, but you can only have a > > > single function implementation with longjmp/coroutine, or having a > > > saparate thread per command but that would mean taking locks for > > > anything not trivial, which avoids the no-change-to-implementation. Is > > > this what you have in mind? > > > > It would not be opaque to the implementer. But it would avoid > > introducing new commands and events, instead we have a unified mechanism > > to signal completion. > > Yeah, that sounds good. Let's have that for 6.4. That's too far away, qemu will have be re-written a dozen of times when we get there. But if you meant 1.2, yes, I hope to have async support for 1.2. But I must admit that I'm having some trouble delivering things on time...
Hi, >> How would the parallel execution facility be opaque to the implementer? >> screendump returns, screendump_async needs to pass a closure. You can >> automatically generate any amount of code, but you can only have a >> single function implementation with longjmp/coroutine, or having a >> saparate thread per command but that would mean taking locks for >> anything not trivial, which avoids the no-change-to-implementation. Is >> this what you have in mind? > > It would not be opaque to the implementer. But it would avoid > introducing new commands and events, instead we have a unified mechanism > to signal completion. Ok. We have a async mechanism today: .mhandler.cmd_async = ... I know it has its problems like no cancelation and is deprecated and all. But still: how about using it as interim until QAPI-based async monitor support is ready? We could unbreak qxl screendumps without having to introduce a new (but temporary!) screendump_async command + completion event. cheers, Gerd
On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote: > Hi, > > >> How would the parallel execution facility be opaque to the implementer? > >> screendump returns, screendump_async needs to pass a closure. You can > >> automatically generate any amount of code, but you can only have a > >> single function implementation with longjmp/coroutine, or having a > >> saparate thread per command but that would mean taking locks for > >> anything not trivial, which avoids the no-change-to-implementation. Is > >> this what you have in mind? > > > > It would not be opaque to the implementer. But it would avoid > > introducing new commands and events, instead we have a unified mechanism > > to signal completion. > > Ok. We have a async mechanism today: .mhandler.cmd_async = ... > > I know it has its problems like no cancelation and is deprecated and > all. But still: how about using it as interim until QAPI-based async > monitor support is ready? We could unbreak qxl screendumps without > having to introduce a new (but temporary!) screendump_async command + > completion event. With the added benefit of no libvirt changes for the temporary solution. > > cheers, > Gerd >
On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote: > Hi, > > >> How would the parallel execution facility be opaque to the implementer? > >> screendump returns, screendump_async needs to pass a closure. You can > >> automatically generate any amount of code, but you can only have a > >> single function implementation with longjmp/coroutine, or having a > >> saparate thread per command but that would mean taking locks for > >> anything not trivial, which avoids the no-change-to-implementation. Is > >> this what you have in mind? > > > > It would not be opaque to the implementer. But it would avoid > > introducing new commands and events, instead we have a unified mechanism > > to signal completion. > > Ok. We have a async mechanism today: .mhandler.cmd_async = ... > > I know it has its problems like no cancelation and is deprecated and > all. But still: how about using it as interim until QAPI-based async > monitor support is ready? We could unbreak qxl screendumps without > having to introduce a new (but temporary!) screendump_async command + > completion event. Actually, I'm not sure this doesn't reintroduce the original problem (which I haven't been able to reproduce): client: screenshot <-> client libvirt <-> host libvirt host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client > > cheers, > Gerd >
On 03/06/12 08:56, Alon Levy wrote: > On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote: >> Hi, >> >>>> How would the parallel execution facility be opaque to the implementer? >>>> screendump returns, screendump_async needs to pass a closure. You can >>>> automatically generate any amount of code, but you can only have a >>>> single function implementation with longjmp/coroutine, or having a >>>> saparate thread per command but that would mean taking locks for >>>> anything not trivial, which avoids the no-change-to-implementation. Is >>>> this what you have in mind? >>> >>> It would not be opaque to the implementer. But it would avoid >>> introducing new commands and events, instead we have a unified mechanism >>> to signal completion. >> >> Ok. We have a async mechanism today: .mhandler.cmd_async = ... >> >> I know it has its problems like no cancelation and is deprecated and >> all. But still: how about using it as interim until QAPI-based async >> monitor support is ready? We could unbreak qxl screendumps without >> having to introduce a new (but temporary!) screendump_async command + >> completion event. > > Actually, I'm not sure this doesn't reintroduce the original problem > (which I haven't been able to reproduce): > > client: screenshot <-> client libvirt <-> host libvirt > > host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client Hmm? spice client can ask for a screendump via libvirt? /me looks completely puzzled. cheers, Gerd
On Tue, Mar 06, 2012 at 09:10:00AM +0100, Gerd Hoffmann wrote: > On 03/06/12 08:56, Alon Levy wrote: > > On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote: > >> Hi, > >> > >>>> How would the parallel execution facility be opaque to the implementer? > >>>> screendump returns, screendump_async needs to pass a closure. You can > >>>> automatically generate any amount of code, but you can only have a > >>>> single function implementation with longjmp/coroutine, or having a > >>>> saparate thread per command but that would mean taking locks for > >>>> anything not trivial, which avoids the no-change-to-implementation. Is > >>>> this what you have in mind? > >>> > >>> It would not be opaque to the implementer. But it would avoid > >>> introducing new commands and events, instead we have a unified mechanism > >>> to signal completion. > >> > >> Ok. We have a async mechanism today: .mhandler.cmd_async = ... > >> > >> I know it has its problems like no cancelation and is deprecated and > >> all. But still: how about using it as interim until QAPI-based async > >> monitor support is ready? We could unbreak qxl screendumps without > >> having to introduce a new (but temporary!) screendump_async command + > >> completion event. > > > > Actually, I'm not sure this doesn't reintroduce the original problem > > (which I haven't been able to reproduce): > > > > client: screenshot <-> client libvirt <-> host libvirt > > > > host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client > > Hmm? spice client can ask for a screendump via libvirt? > > /me looks completely puzzled. No, ok, bad attempt. "client" is a libvirt and spice client. client as a libvirt client asks for screenshot. libvirt (client side) asks from libvirt (host side) libvirt (host side) issues a screendump monitor command (the new internal async version) qxl_screen_dump asks spice server to render spice server waits for pipe to spice client to empty (lower then 50) but spice client, which is just "client", is blocking on completion of the screendump command. I have two problems with my own explanation: 1. I didn't manage to reproduce it myself, and nor has Marc Andre (who first reported this problem) yesterday. 2. I remember that the async monitor command I sent in October did fix the problem, so something with my description is wrong. > > cheers, > Gerd > >
On Tue, 06 Mar 2012 08:36:34 +0100 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > >> How would the parallel execution facility be opaque to the implementer? > >> screendump returns, screendump_async needs to pass a closure. You can > >> automatically generate any amount of code, but you can only have a > >> single function implementation with longjmp/coroutine, or having a > >> saparate thread per command but that would mean taking locks for > >> anything not trivial, which avoids the no-change-to-implementation. Is > >> this what you have in mind? > > > > It would not be opaque to the implementer. But it would avoid > > introducing new commands and events, instead we have a unified mechanism > > to signal completion. > > Ok. We have a async mechanism today: .mhandler.cmd_async = ... > > I know it has its problems like no cancelation and is deprecated and > all. But still: how about using it as interim until QAPI-based async > monitor support is ready? We could unbreak qxl screendumps without > having to introduce a new (but temporary!) screendump_async command + > completion event. There are a few problems here, but the blocking one is that a command can't go from sync to async. This is an incompatible change. If you mind adding the temporary command and if this issue is so rare that none can reproduce it, then I'd suggest to wait for 1.2.
On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > On Tue, 06 Mar 2012 08:36:34 +0100 > Gerd Hoffmann <kraxel@redhat.com> wrote: > > > Hi, > > > > >> How would the parallel execution facility be opaque to the implementer? > > >> screendump returns, screendump_async needs to pass a closure. You can > > >> automatically generate any amount of code, but you can only have a > > >> single function implementation with longjmp/coroutine, or having a > > >> saparate thread per command but that would mean taking locks for > > >> anything not trivial, which avoids the no-change-to-implementation. Is > > >> this what you have in mind? > > > > > > It would not be opaque to the implementer. But it would avoid > > > introducing new commands and events, instead we have a unified mechanism > > > to signal completion. > > > > Ok. We have a async mechanism today: .mhandler.cmd_async = ... > > > > I know it has its problems like no cancelation and is deprecated and > > all. But still: how about using it as interim until QAPI-based async > > monitor support is ready? We could unbreak qxl screendumps without > > having to introduce a new (but temporary!) screendump_async command + > > completion event. > > There are a few problems here, but the blocking one is that a command > can't go from sync to async. This is an incompatible change. > > If you mind adding the temporary command and if this issue is so rare > that none can reproduce it, then I'd suggest to wait for 1.2. > There are two options really: 1. revert the patches that changed qxl screendump to save the ppm before (possibly) updating the framebuffer. 2. introduce a new command that is really async The third option, what Gerd proposes, doesn't break the blocking chain going from the A, the dual purpose spice client and libvirt client, through libvirt, qemu, spice and back to A. If no one can reproduce the block then it would seem 1 makes sense. But 1 serves a second purpose, which is to allow the guest to do io exits while the server thread is processing the area update request issued for the screendump. So it makes sense regardless. => Option 2.
On 03/06/2012 07:16 AM, Alon Levy wrote: > On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: >> On Tue, 06 Mar 2012 08:36:34 +0100 >> Gerd Hoffmann<kraxel@redhat.com> wrote: >> >>> Hi, >>> >>>>> How would the parallel execution facility be opaque to the implementer? >>>>> screendump returns, screendump_async needs to pass a closure. You can >>>>> automatically generate any amount of code, but you can only have a >>>>> single function implementation with longjmp/coroutine, or having a >>>>> saparate thread per command but that would mean taking locks for >>>>> anything not trivial, which avoids the no-change-to-implementation. Is >>>>> this what you have in mind? >>>> >>>> It would not be opaque to the implementer. But it would avoid >>>> introducing new commands and events, instead we have a unified mechanism >>>> to signal completion. >>> >>> Ok. We have a async mechanism today: .mhandler.cmd_async = ... >>> >>> I know it has its problems like no cancelation and is deprecated and >>> all. But still: how about using it as interim until QAPI-based async >>> monitor support is ready? We could unbreak qxl screendumps without >>> having to introduce a new (but temporary!) screendump_async command + >>> completion event. >> >> There are a few problems here, but the blocking one is that a command >> can't go from sync to async. This is an incompatible change. >> >> If you mind adding the temporary command and if this issue is so rare >> that none can reproduce it, then I'd suggest to wait for 1.2. >> > > There are two options really: > 1. revert the patches that changed qxl screendump to save the ppm > before (possibly) updating the framebuffer. > 2. introduce a new command that is really async > > The third option, what Gerd proposes, doesn't break the blocking chain > going from the A, the dual purpose spice client and libvirt client, > through libvirt, qemu, spice and back to A. > > If no one can reproduce the block then it would seem 1 makes sense. So let's start with a reproducible test case that demonstrates the problem before we start introducing new commands then if there's doubt about the nature of the problem. Regards, Anthony Liguori > > But 1 serves a second purpose, which is to allow the guest to do io > exits while the server thread is processing the area update request > issued for the screendump. So it makes sense regardless. > > => Option 2.
On Tue, 06 Mar 2012 07:51:29 -0600 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/06/2012 07:16 AM, Alon Levy wrote: > > On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > >> On Tue, 06 Mar 2012 08:36:34 +0100 > >> Gerd Hoffmann<kraxel@redhat.com> wrote: > >> > >>> Hi, > >>> > >>>>> How would the parallel execution facility be opaque to the implementer? > >>>>> screendump returns, screendump_async needs to pass a closure. You can > >>>>> automatically generate any amount of code, but you can only have a > >>>>> single function implementation with longjmp/coroutine, or having a > >>>>> saparate thread per command but that would mean taking locks for > >>>>> anything not trivial, which avoids the no-change-to-implementation. Is > >>>>> this what you have in mind? > >>>> > >>>> It would not be opaque to the implementer. But it would avoid > >>>> introducing new commands and events, instead we have a unified mechanism > >>>> to signal completion. > >>> > >>> Ok. We have a async mechanism today: .mhandler.cmd_async = ... > >>> > >>> I know it has its problems like no cancelation and is deprecated and > >>> all. But still: how about using it as interim until QAPI-based async > >>> monitor support is ready? We could unbreak qxl screendumps without > >>> having to introduce a new (but temporary!) screendump_async command + > >>> completion event. > >> > >> There are a few problems here, but the blocking one is that a command > >> can't go from sync to async. This is an incompatible change. > >> > >> If you mind adding the temporary command and if this issue is so rare > >> that none can reproduce it, then I'd suggest to wait for 1.2. > >> > > > > There are two options really: > > 1. revert the patches that changed qxl screendump to save the ppm > > before (possibly) updating the framebuffer. > > 2. introduce a new command that is really async > > > > The third option, what Gerd proposes, doesn't break the blocking chain > > going from the A, the dual purpose spice client and libvirt client, > > through libvirt, qemu, spice and back to A. > > > > If no one can reproduce the block then it would seem 1 makes sense. > > So let's start with a reproducible test case that demonstrates the problem > before we start introducing new commands then if there's doubt about the nature > of the problem. Completely agree, I was going to suggest that too.
On Tue, Mar 06, 2012 at 10:53:42AM -0300, Luiz Capitulino wrote: > On Tue, 06 Mar 2012 07:51:29 -0600 > Anthony Liguori <anthony@codemonkey.ws> wrote: > > > On 03/06/2012 07:16 AM, Alon Levy wrote: > > > On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > > >> On Tue, 06 Mar 2012 08:36:34 +0100 > > >> Gerd Hoffmann<kraxel@redhat.com> wrote: > > >> > > >>> Hi, > > >>> > > >>>>> How would the parallel execution facility be opaque to the implementer? > > >>>>> screendump returns, screendump_async needs to pass a closure. You can > > >>>>> automatically generate any amount of code, but you can only have a > > >>>>> single function implementation with longjmp/coroutine, or having a > > >>>>> saparate thread per command but that would mean taking locks for > > >>>>> anything not trivial, which avoids the no-change-to-implementation. Is > > >>>>> this what you have in mind? > > >>>> > > >>>> It would not be opaque to the implementer. But it would avoid > > >>>> introducing new commands and events, instead we have a unified mechanism > > >>>> to signal completion. > > >>> > > >>> Ok. We have a async mechanism today: .mhandler.cmd_async = ... > > >>> > > >>> I know it has its problems like no cancelation and is deprecated and > > >>> all. But still: how about using it as interim until QAPI-based async > > >>> monitor support is ready? We could unbreak qxl screendumps without > > >>> having to introduce a new (but temporary!) screendump_async command + > > >>> completion event. > > >> > > >> There are a few problems here, but the blocking one is that a command > > >> can't go from sync to async. This is an incompatible change. > > >> > > >> If you mind adding the temporary command and if this issue is so rare > > >> that none can reproduce it, then I'd suggest to wait for 1.2. > > >> > > > > > > There are two options really: > > > 1. revert the patches that changed qxl screendump to save the ppm > > > before (possibly) updating the framebuffer. > > > 2. introduce a new command that is really async > > > > > > The third option, what Gerd proposes, doesn't break the blocking chain > > > going from the A, the dual purpose spice client and libvirt client, > > > through libvirt, qemu, spice and back to A. > > > > > > If no one can reproduce the block then it would seem 1 makes sense. > > > > So let's start with a reproducible test case that demonstrates the problem > > before we start introducing new commands then if there's doubt about the nature > > of the problem. > > Completely agree, I was going to suggest that too. > So cutting off a part of the email is a good way to win arguments? cool trick. I agree a reproducer is a good idea, but as I mentioned in the cut part, doing the update area without keeping the monitor waiting improves performance of the guest by letting it do io exits. Why is that a bad thing?
On 03/06/2012 08:23 AM, Alon Levy wrote: > On Tue, Mar 06, 2012 at 10:53:42AM -0300, Luiz Capitulino wrote: > > So cutting off a part of the email is a good way to win arguments? cool > trick. It doesn't work as well if you acknowledge that was the motivation ;-) (j/k) > I agree a reproducer is a good idea, but as I mentioned in the > cut part, doing the update area without keeping the monitor waiting > improves performance of the guest by letting it do io exits. Why is that > a bad thing? You say, "improves performance", but can you quantify that? And why is screendump on the performance critical path in the first place? Having a small test case that demonstrates the problem (be it functional or performance) will help ground this discussion in concrete terms. I think we need to step back and reexamine what the problem we're trying to solve is. Regards, Anthony Liguori
On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote: > On 03/06/2012 07:16 AM, Alon Levy wrote: > >On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > >>On Tue, 06 Mar 2012 08:36:34 +0100 > >>Gerd Hoffmann<kraxel@redhat.com> wrote: > >> > >>> Hi, > >>> > >>>>>How would the parallel execution facility be opaque to the implementer? > >>>>>screendump returns, screendump_async needs to pass a closure. You can > >>>>>automatically generate any amount of code, but you can only have a > >>>>>single function implementation with longjmp/coroutine, or having a > >>>>>saparate thread per command but that would mean taking locks for > >>>>>anything not trivial, which avoids the no-change-to-implementation. Is > >>>>>this what you have in mind? > >>>> > >>>>It would not be opaque to the implementer. But it would avoid > >>>>introducing new commands and events, instead we have a unified mechanism > >>>>to signal completion. > >>> > >>>Ok. We have a async mechanism today: .mhandler.cmd_async = ... > >>> > >>>I know it has its problems like no cancelation and is deprecated and > >>>all. But still: how about using it as interim until QAPI-based async > >>>monitor support is ready? We could unbreak qxl screendumps without > >>>having to introduce a new (but temporary!) screendump_async command + > >>>completion event. > >> > >>There are a few problems here, but the blocking one is that a command > >>can't go from sync to async. This is an incompatible change. > >> > >>If you mind adding the temporary command and if this issue is so rare > >>that none can reproduce it, then I'd suggest to wait for 1.2. > >> > > > >There are two options really: > > 1. revert the patches that changed qxl screendump to save the ppm > > before (possibly) updating the framebuffer. > > 2. introduce a new command that is really async > > > > The third option, what Gerd proposes, doesn't break the blocking chain > > going from the A, the dual purpose spice client and libvirt client, > > through libvirt, qemu, spice and back to A. > > > >If no one can reproduce the block then it would seem 1 makes sense. > > So let's start with a reproducible test case that demonstrates the > problem before we start introducing new commands then if there's > doubt about the nature of the problem. s/the problem/different problem/: NB: To get this hang I had to disable update_area's from the guest, just because otherwise there is a very small window between suspending the client and qemu waiting on the same stack trace below issued from the guest update_area io out, instead of from the screendump monitor command. spice client, qemu, libvirt, virsh screenshot. libvirt controlled qemu with qxl device and spice connection. qemu ... -vga qxl -spice disable-ticketing,port=5900... spicec -h localhost -p 5900 [boot until qxl driver is loaded, and guest is doing something (Running toms 2d benchmark works), suspend spicec] virsh screenshot <vm-name> /tmp/dump.ppm virsh will hang at: #1 virCondWait #2 qemuMonitorSend #3 qemuMonitorJSONCommandWithFd #4 qemuMonitorJSONCommand #5 qemuMonitorJSONScreendump qemu is hung at: main thread: #0 read #1 read_safe #2 dispatcher_send_message #3 red_dispatcher_update_area #4 qxl_worker_update_area #5 qxl_spice_update_area #6 qxl_render_update #7 qxl_hw_screen_dump #8 vga_hw_screen_dump spice worker thread: #0 nanosleep #1 usleep #2 flush_display_commands #3 handle_dev_update #4 dispatcher_handle_single_read #5 dispatcher_handle_recv_read #6 handle_dev_input #7 red_worker_main #8 start_thread #9 clone So the two problems here are: virsh screenshot is hung if the client is hung / not responsive qemu guest thread will hang the first time it does a vmexit if the client is hung / not responsive. Like Gerd mentioned previously, this issue should be addressed in spice but is hard to correct there, hence the async io patches that landed long ago, and hence the async update_area done for screendump as well that we are discussing reverting. So if I have you convinced it should not be reverted, we are back to needing a asynchronous command for screendump. And Luiz said changing the sync command to async is not an option (though I didn't understand why), so we are left with a new, temporary, command. Any objections? > > Regards, > > Anthony Liguori > > > > >But 1 serves a second purpose, which is to allow the guest to do io > >exits while the server thread is processing the area update request > >issued for the screendump. So it makes sense regardless. > > > >=> Option 2. > >
On Tue, Mar 06, 2012 at 05:56:49PM +0200, Alon Levy wrote: > On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote: > > On 03/06/2012 07:16 AM, Alon Levy wrote: > > >On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > > >>On Tue, 06 Mar 2012 08:36:34 +0100 > > >>Gerd Hoffmann<kraxel@redhat.com> wrote: > > >> > > >>> Hi, > > >>> > > >>>>>How would the parallel execution facility be opaque to the implementer? > > >>>>>screendump returns, screendump_async needs to pass a closure. You can > > >>>>>automatically generate any amount of code, but you can only have a > > >>>>>single function implementation with longjmp/coroutine, or having a > > >>>>>saparate thread per command but that would mean taking locks for > > >>>>>anything not trivial, which avoids the no-change-to-implementation. Is > > >>>>>this what you have in mind? > > >>>> > > >>>>It would not be opaque to the implementer. But it would avoid > > >>>>introducing new commands and events, instead we have a unified mechanism > > >>>>to signal completion. > > >>> > > >>>Ok. We have a async mechanism today: .mhandler.cmd_async = ... > > >>> > > >>>I know it has its problems like no cancelation and is deprecated and > > >>>all. But still: how about using it as interim until QAPI-based async > > >>>monitor support is ready? We could unbreak qxl screendumps without > > >>>having to introduce a new (but temporary!) screendump_async command + > > >>>completion event. > > >> > > >>There are a few problems here, but the blocking one is that a command > > >>can't go from sync to async. This is an incompatible change. > > >> > > >>If you mind adding the temporary command and if this issue is so rare > > >>that none can reproduce it, then I'd suggest to wait for 1.2. > > >> > > > > > >There are two options really: > > > 1. revert the patches that changed qxl screendump to save the ppm > > > before (possibly) updating the framebuffer. > > > 2. introduce a new command that is really async > > > > > > The third option, what Gerd proposes, doesn't break the blocking chain > > > going from the A, the dual purpose spice client and libvirt client, > > > through libvirt, qemu, spice and back to A. > > > > > >If no one can reproduce the block then it would seem 1 makes sense. > > > > So let's start with a reproducible test case that demonstrates the > > problem before we start introducing new commands then if there's > > doubt about the nature of the problem. > > s/the problem/different problem/: > > NB: To get this hang I had to disable update_area's from the guest, just > because otherwise there is a very small window between suspending the > client and qemu waiting on the same stack trace below issued from the > guest update_area io out, instead of from the screendump monitor command. Explanation to the note: 'disable update_area' - I meant disable the implementation, i.e. change the ioport_write switch to return without actually rendering anything. It causes artifacts of course, but if you know how the vm looks by heart it's good enough to reproduce. Nothing changed in the guest. > > spice client, qemu, libvirt, virsh screenshot. > > libvirt controlled qemu with qxl device and spice connection. > qemu ... -vga qxl -spice disable-ticketing,port=5900... > spicec -h localhost -p 5900 > [boot until qxl driver is loaded, and guest is doing something (Running > toms 2d benchmark works), suspend spicec] > virsh screenshot <vm-name> /tmp/dump.ppm > > virsh will hang at: > #1 virCondWait > #2 qemuMonitorSend > #3 qemuMonitorJSONCommandWithFd > #4 qemuMonitorJSONCommand > #5 qemuMonitorJSONScreendump > > qemu is hung at: > main thread: > #0 read > #1 read_safe > #2 dispatcher_send_message > #3 red_dispatcher_update_area > #4 qxl_worker_update_area > #5 qxl_spice_update_area > #6 qxl_render_update > #7 qxl_hw_screen_dump > #8 vga_hw_screen_dump > > spice worker thread: > #0 nanosleep > #1 usleep > #2 flush_display_commands > #3 handle_dev_update > #4 dispatcher_handle_single_read > #5 dispatcher_handle_recv_read > #6 handle_dev_input > #7 red_worker_main > #8 start_thread > #9 clone > > So the two problems here are: > virsh screenshot is hung if the client is hung / not responsive > qemu guest thread will hang the first time it does a vmexit if the > client is hung / not responsive. > > Like Gerd mentioned previously, this issue should be addressed in spice > but is hard to correct there, hence the async io patches that landed > long ago, and hence the async update_area done for screendump as well > that we are discussing reverting. > > So if I have you convinced it should not be reverted, we are back to > needing a asynchronous command for screendump. And Luiz said changing > the sync command to async is not an option (though I didn't understand > why), so we are left with a new, temporary, command. > > Any objections? > > > > Regards, > > > > Anthony Liguori > > > > > > > >But 1 serves a second purpose, which is to allow the guest to do io > > >exits while the server thread is processing the area update request > > >issued for the screendump. So it makes sense regardless. > > > > > >=> Option 2. > > > > >
On 03/06/2012 09:56 AM, Alon Levy wrote: > On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote: >> On 03/06/2012 07:16 AM, Alon Levy wrote: >>> On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: >>>> On Tue, 06 Mar 2012 08:36:34 +0100 >>>> Gerd Hoffmann<kraxel@redhat.com> wrote: >>>> >>>>> Hi, >>>>> >>>>>>> How would the parallel execution facility be opaque to the implementer? >>>>>>> screendump returns, screendump_async needs to pass a closure. You can >>>>>>> automatically generate any amount of code, but you can only have a >>>>>>> single function implementation with longjmp/coroutine, or having a >>>>>>> saparate thread per command but that would mean taking locks for >>>>>>> anything not trivial, which avoids the no-change-to-implementation. Is >>>>>>> this what you have in mind? >>>>>> >>>>>> It would not be opaque to the implementer. But it would avoid >>>>>> introducing new commands and events, instead we have a unified mechanism >>>>>> to signal completion. >>>>> >>>>> Ok. We have a async mechanism today: .mhandler.cmd_async = ... >>>>> >>>>> I know it has its problems like no cancelation and is deprecated and >>>>> all. But still: how about using it as interim until QAPI-based async >>>>> monitor support is ready? We could unbreak qxl screendumps without >>>>> having to introduce a new (but temporary!) screendump_async command + >>>>> completion event. >>>> >>>> There are a few problems here, but the blocking one is that a command >>>> can't go from sync to async. This is an incompatible change. >>>> >>>> If you mind adding the temporary command and if this issue is so rare >>>> that none can reproduce it, then I'd suggest to wait for 1.2. >>>> >>> >>> There are two options really: >>> 1. revert the patches that changed qxl screendump to save the ppm >>> before (possibly) updating the framebuffer. >>> 2. introduce a new command that is really async >>> >>> The third option, what Gerd proposes, doesn't break the blocking chain >>> going from the A, the dual purpose spice client and libvirt client, >>> through libvirt, qemu, spice and back to A. >>> >>> If no one can reproduce the block then it would seem 1 makes sense. >> >> So let's start with a reproducible test case that demonstrates the >> problem before we start introducing new commands then if there's >> doubt about the nature of the problem. > > s/the problem/different problem/: > > NB: To get this hang I had to disable update_area's from the guest, just > because otherwise there is a very small window between suspending the > client and qemu waiting on the same stack trace below issued from the > guest update_area io out, instead of from the screendump monitor command. > > spice client, qemu, libvirt, virsh screenshot. > > libvirt controlled qemu with qxl device and spice connection. > qemu ... -vga qxl -spice disable-ticketing,port=5900... > spicec -h localhost -p 5900 > [boot until qxl driver is loaded, and guest is doing something (Running > toms 2d benchmark works), suspend spicec] > virsh screenshot<vm-name> /tmp/dump.ppm > > virsh will hang at: > #1 virCondWait > #2 qemuMonitorSend > #3 qemuMonitorJSONCommandWithFd > #4 qemuMonitorJSONCommand > #5 qemuMonitorJSONScreendump > > qemu is hung at: > main thread: > #0 read Is qxl doing a blocking read? If so, that's a bug in qxl. You cannot do a blocking read while holding qemu_mutex. Regards, Anthony Liguori
On Tue, Mar 06, 2012 at 10:16:42AM -0600, Anthony Liguori wrote: > On 03/06/2012 09:56 AM, Alon Levy wrote: > >On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote: > >>On 03/06/2012 07:16 AM, Alon Levy wrote: > >>>On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > >>>>On Tue, 06 Mar 2012 08:36:34 +0100 > >>>>Gerd Hoffmann<kraxel@redhat.com> wrote: > >>>> > >>>>> Hi, > >>>>> > >>>>>>>How would the parallel execution facility be opaque to the implementer? > >>>>>>>screendump returns, screendump_async needs to pass a closure. You can > >>>>>>>automatically generate any amount of code, but you can only have a > >>>>>>>single function implementation with longjmp/coroutine, or having a > >>>>>>>saparate thread per command but that would mean taking locks for > >>>>>>>anything not trivial, which avoids the no-change-to-implementation. Is > >>>>>>>this what you have in mind? > >>>>>> > >>>>>>It would not be opaque to the implementer. But it would avoid > >>>>>>introducing new commands and events, instead we have a unified mechanism > >>>>>>to signal completion. > >>>>> > >>>>>Ok. We have a async mechanism today: .mhandler.cmd_async = ... > >>>>> > >>>>>I know it has its problems like no cancelation and is deprecated and > >>>>>all. But still: how about using it as interim until QAPI-based async > >>>>>monitor support is ready? We could unbreak qxl screendumps without > >>>>>having to introduce a new (but temporary!) screendump_async command + > >>>>>completion event. > >>>> > >>>>There are a few problems here, but the blocking one is that a command > >>>>can't go from sync to async. This is an incompatible change. > >>>> > >>>>If you mind adding the temporary command and if this issue is so rare > >>>>that none can reproduce it, then I'd suggest to wait for 1.2. > >>>> > >>> > >>>There are two options really: > >>> 1. revert the patches that changed qxl screendump to save the ppm > >>> before (possibly) updating the framebuffer. > >>> 2. introduce a new command that is really async > >>> > >>> The third option, what Gerd proposes, doesn't break the blocking chain > >>> going from the A, the dual purpose spice client and libvirt client, > >>> through libvirt, qemu, spice and back to A. > >>> > >>>If no one can reproduce the block then it would seem 1 makes sense. > >> > >>So let's start with a reproducible test case that demonstrates the > >>problem before we start introducing new commands then if there's > >>doubt about the nature of the problem. > > > >s/the problem/different problem/: > > > > NB: To get this hang I had to disable update_area's from the guest, just > > because otherwise there is a very small window between suspending the > > client and qemu waiting on the same stack trace below issued from the > > guest update_area io out, instead of from the screendump monitor command. > > > > spice client, qemu, libvirt, virsh screenshot. > > > > libvirt controlled qemu with qxl device and spice connection. > > qemu ... -vga qxl -spice disable-ticketing,port=5900... > > spicec -h localhost -p 5900 > > [boot until qxl driver is loaded, and guest is doing something (Running > > toms 2d benchmark works), suspend spicec] > > virsh screenshot<vm-name> /tmp/dump.ppm > > > >virsh will hang at: > > #1 virCondWait > > #2 qemuMonitorSend > > #3 qemuMonitorJSONCommandWithFd > > #4 qemuMonitorJSONCommand > > #5 qemuMonitorJSONScreendump > > > >qemu is hung at: > > main thread: > > #0 read > > Is qxl doing a blocking read? If so, that's a bug in qxl. You > cannot do a blocking read while holding qemu_mutex. What are you saying, that that read should be fixed? then I guess it's good that patches fixing it have landed. That stack was prior to "qxl: make qxl_render_update async", 81fb6f1504fb9ef71f2382f44af34756668296e8 . > > Regards, > > Anthony Liguori >
On 03/06/2012 10:26 AM, Alon Levy wrote: > On Tue, Mar 06, 2012 at 10:16:42AM -0600, Anthony Liguori wrote: >> On 03/06/2012 09:56 AM, Alon Levy wrote: >>> On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote: >>>> On 03/06/2012 07:16 AM, Alon Levy wrote: >>>>> On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: >>>>>> On Tue, 06 Mar 2012 08:36:34 +0100 >>>>>> Gerd Hoffmann<kraxel@redhat.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>>>> How would the parallel execution facility be opaque to the implementer? >>>>>>>>> screendump returns, screendump_async needs to pass a closure. You can >>>>>>>>> automatically generate any amount of code, but you can only have a >>>>>>>>> single function implementation with longjmp/coroutine, or having a >>>>>>>>> saparate thread per command but that would mean taking locks for >>>>>>>>> anything not trivial, which avoids the no-change-to-implementation. Is >>>>>>>>> this what you have in mind? >>>>>>>> >>>>>>>> It would not be opaque to the implementer. But it would avoid >>>>>>>> introducing new commands and events, instead we have a unified mechanism >>>>>>>> to signal completion. >>>>>>> >>>>>>> Ok. We have a async mechanism today: .mhandler.cmd_async = ... >>>>>>> >>>>>>> I know it has its problems like no cancelation and is deprecated and >>>>>>> all. But still: how about using it as interim until QAPI-based async >>>>>>> monitor support is ready? We could unbreak qxl screendumps without >>>>>>> having to introduce a new (but temporary!) screendump_async command + >>>>>>> completion event. >>>>>> >>>>>> There are a few problems here, but the blocking one is that a command >>>>>> can't go from sync to async. This is an incompatible change. >>>>>> >>>>>> If you mind adding the temporary command and if this issue is so rare >>>>>> that none can reproduce it, then I'd suggest to wait for 1.2. >>>>>> >>>>> >>>>> There are two options really: >>>>> 1. revert the patches that changed qxl screendump to save the ppm >>>>> before (possibly) updating the framebuffer. >>>>> 2. introduce a new command that is really async >>>>> >>>>> The third option, what Gerd proposes, doesn't break the blocking chain >>>>> going from the A, the dual purpose spice client and libvirt client, >>>>> through libvirt, qemu, spice and back to A. >>>>> >>>>> If no one can reproduce the block then it would seem 1 makes sense. >>>> >>>> So let's start with a reproducible test case that demonstrates the >>>> problem before we start introducing new commands then if there's >>>> doubt about the nature of the problem. >>> >>> s/the problem/different problem/: >>> >>> NB: To get this hang I had to disable update_area's from the guest, just >>> because otherwise there is a very small window between suspending the >>> client and qemu waiting on the same stack trace below issued from the >>> guest update_area io out, instead of from the screendump monitor command. >>> >>> spice client, qemu, libvirt, virsh screenshot. >>> >>> libvirt controlled qemu with qxl device and spice connection. >>> qemu ... -vga qxl -spice disable-ticketing,port=5900... >>> spicec -h localhost -p 5900 >>> [boot until qxl driver is loaded, and guest is doing something (Running >>> toms 2d benchmark works), suspend spicec] >>> virsh screenshot<vm-name> /tmp/dump.ppm >>> >>> virsh will hang at: >>> #1 virCondWait >>> #2 qemuMonitorSend >>> #3 qemuMonitorJSONCommandWithFd >>> #4 qemuMonitorJSONCommand >>> #5 qemuMonitorJSONScreendump >>> >>> qemu is hung at: >>> main thread: >>> #0 read >> >> Is qxl doing a blocking read? If so, that's a bug in qxl. You >> cannot do a blocking read while holding qemu_mutex. > > What are you saying, that that read should be fixed? then I guess it's > good that patches fixing it have landed. That stack was prior to "qxl: > make qxl_render_update async", 81fb6f1504fb9ef71f2382f44af34756668296e8 I'm sorry, I'm thoroughly confused by this thread. Can you start a new thread with a clear explanation of the problem you're trying to solve or at least point me to an existing note? Regards, Anthony Liguori > . > >> >> Regards, >> >> Anthony Liguori >>
Hi, >> qemu is hung at: >> main thread: >> #0 read > > Is qxl doing a blocking read? If so, that's a bug in qxl. It used to do that, with the latest spice pull it is gone[1]. And this fix is exactly what broke screendump. Spice does lazy rendering on the server side to avoid burning cpu for nothing, because in the common case there is nothing to render. Thats why there is no up-to-date displaysurface we can just write out when the screendump command comes in. What qxl screendump used to do is this: (1) ask spice server to render the screen (2) blocking read, waiting for spice spice server finish <- BUG (3) write out screendump What qxl screendump does now is: (1) ask spice server to render the screen (2) write out screendump from outdated displaysurface <- BUG (3) spice server finished, callback comes in, arm BH (4) bottom half handler updates displaysurface What we like to do instead is this: (1) ask spice server to render the screen (2) spice server finished, callback comes in, arm BH (3) bottom half handler updates displaysurface and writes the screendump (4) signal screendump monitor command is finished. See our problem now? cheers, Gerd [1] With the exception of guest with old qxl drivers which is pretty much unfixable due to way the old guest <-> qxl interface is designed.
diff --git a/console.c b/console.c index 25d8acb..9a49e93 100644 --- a/console.c +++ b/console.c @@ -124,6 +124,7 @@ struct TextConsole { vga_hw_update_ptr hw_update; vga_hw_invalidate_ptr hw_invalidate; vga_hw_screen_dump_ptr hw_screen_dump; + vga_hw_screen_dump_async_ptr hw_screen_dump_async; vga_hw_text_update_ptr hw_text_update; void *hw; @@ -1403,6 +1404,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, vga_hw_invalidate_ptr invalidate, vga_hw_screen_dump_ptr screen_dump, vga_hw_text_update_ptr text_update, + vga_hw_screen_dump_async_ptr + screen_dump_async, void *opaque) { TextConsole *s; @@ -1421,6 +1424,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, s->hw_update = update; s->hw_invalidate = invalidate; s->hw_screen_dump = screen_dump; + s->hw_screen_dump_async = screen_dump_async; s->hw_text_update = text_update; s->hw = opaque; diff --git a/console.h b/console.h index c22803c..3cf28c0 100644 --- a/console.h +++ b/console.h @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch) typedef void (*vga_hw_update_ptr)(void *); typedef void (*vga_hw_invalidate_ptr)(void *); typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch); +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename, + bool cswitch); typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *); DisplayState *graphic_console_init(vga_hw_update_ptr update, vga_hw_invalidate_ptr invalidate, vga_hw_screen_dump_ptr screen_dump, vga_hw_text_update_ptr text_update, + vga_hw_screen_dump_async_ptr + screen_dump_async, void *opaque); void vga_hw_update(void); void vga_hw_invalidate(void); void vga_hw_screen_dump(const char *filename); +void vga_hw_screen_dump_async(const char *filename); void vga_hw_text_update(console_ch_t *chardata); void monitor_protocol_screen_dump_complete_event(const char *filename); diff --git a/hw/blizzard.c b/hw/blizzard.c index c7d844d..cc51045 100644 --- a/hw/blizzard.c +++ b/hw/blizzard.c @@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int) s->state = graphic_console_init(blizzard_update_display, blizzard_invalidate_display, - blizzard_screen_dump, NULL, s); + blizzard_screen_dump, NULL, NULL, s); switch (ds_get_bits_per_pixel(s->state)) { case 0: diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c index 4edcb94..d2a9c27 100644 --- a/hw/cirrus_vga.c +++ b/hw/cirrus_vga.c @@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev) isa_address_space(dev)); s->ds = graphic_console_init(s->update, s->invalidate, s->screen_dump, s->text_update, - s); + s->screen_dump_async, s); rom_add_vga(VGABIOS_CIRRUS_FILENAME); /* XXX ISA-LFB support */ /* FIXME not qdev yet */ @@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev) cirrus_init_common(s, device_id, 1, pci_address_space(dev)); s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate, s->vga.screen_dump, s->vga.text_update, - &s->vga); + s->vga.screen_dump_async, &s->vga); /* setup PCI */ diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c index 3313f00..2053ed0 100644 --- a/hw/exynos4210_fimd.c +++ b/hw/exynos4210_fimd.c @@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev) "exynos4210.fimd", FIMD_REGS_SIZE); sysbus_init_mmio(dev, &s->iomem); s->console = graphic_console_init(exynos4210_fimd_update, - exynos4210_fimd_invalidate, NULL, NULL, s); + exynos4210_fimd_invalidate, + NULL, NULL, NULL, s); return 0; } diff --git a/hw/g364fb.c b/hw/g364fb.c index 3a0b68f..8ae40d8 100644 --- a/hw/g364fb.c +++ b/hw/g364fb.c @@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s) s->ds = graphic_console_init(g364fb_update_display, g364fb_invalidate_display, - g364fb_screen_dump, NULL, s); + g364fb_screen_dump, NULL, NULL, s); memory_region_init_io(&s->mem_ctrl, &g364fb_ctrl_ops, s, "ctrl", 0x180000); memory_region_init_ram_ptr(&s->mem_vram, "vram", diff --git a/hw/jazz_led.c b/hw/jazz_led.c index 6486523..bb5daf7 100644 --- a/hw/jazz_led.c +++ b/hw/jazz_led.c @@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev) s->ds = graphic_console_init(jazz_led_update_display, jazz_led_invalidate_display, NULL, - jazz_led_text_update, s); + jazz_led_text_update, + NULL, s); return 0; } diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c index 69afd72..ae018d1 100644 --- a/hw/milkymist-vgafb.c +++ b/hw/milkymist-vgafb.c @@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev) s->ds = graphic_console_init(vgafb_update_display, vgafb_invalidate_display, - NULL, NULL, s); + NULL, NULL, NULL, s); return 0; } diff --git a/hw/musicpal.c b/hw/musicpal.c index 187a1ae..2fc391b 100644 --- a/hw/musicpal.c +++ b/hw/musicpal.c @@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev) sysbus_init_mmio(dev, &s->iomem); s->ds = graphic_console_init(lcd_refresh, lcd_invalidate, - NULL, NULL, s); + NULL, NULL, NULL, s); qemu_console_resize(s->ds, 128*3, 64*3); qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3); diff --git a/hw/omap_dss.c b/hw/omap_dss.c index 86ed6ea..0e8243e 100644 --- a/hw/omap_dss.c +++ b/hw/omap_dss.c @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta, #if 0 s->state = graphic_console_init(omap_update_display, - omap_invalidate_display, omap_screen_dump, s); + omap_invalidate_display, + omap_screen_dump, + NULL, NULL, s); #endif return s; diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c index f172093..d91e88d 100644 --- a/hw/omap_lcdc.c +++ b/hw/omap_lcdc.c @@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem, s->state = graphic_console_init(omap_update_display, omap_invalidate_display, - omap_screen_dump, NULL, s); + omap_screen_dump, NULL, NULL, s); return s; } diff --git a/hw/pl110.c b/hw/pl110.c index f94608c..32ee89c 100644 --- a/hw/pl110.c +++ b/hw/pl110.c @@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev) qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1); s->ds = graphic_console_init(pl110_update_display, pl110_invalidate_display, - NULL, NULL, s); + NULL, NULL, NULL, s); return 0; } diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c index fcbdfb3..5345fb4 100644 --- a/hw/pxa2xx_lcd.c +++ b/hw/pxa2xx_lcd.c @@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem, s->ds = graphic_console_init(pxa2xx_update_display, pxa2xx_invalidate_display, - NULL, NULL, s); + NULL, NULL, NULL, s); switch (ds_get_bits_per_pixel(s->ds)) { case 0: diff --git a/hw/qxl.c b/hw/qxl.c index e17b0e3..da0f931 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1763,7 +1763,8 @@ static int qxl_init_primary(PCIDevice *dev) portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0); vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate, - qxl_hw_screen_dump, qxl_hw_text_update, qxl); + qxl_hw_screen_dump, qxl_hw_text_update, + NULL, qxl); qemu_spice_display_init_common(&qxl->ssd, vga->ds); qxl0 = qxl; diff --git a/hw/sm501.c b/hw/sm501.c index 786e076..8b150be 100644 --- a/hw/sm501.c +++ b/hw/sm501.c @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, } /* create qemu graphic console */ - s->ds = graphic_console_init(sm501_update_display, NULL, - NULL, NULL, s); + s->ds = graphic_console_init(sm501_update_display, + NULL, NULL, NULL, NULL, s); } diff --git a/hw/ssd0303.c b/hw/ssd0303.c index 4e1ee6e..05e6686 100644 --- a/hw/ssd0303.c +++ b/hw/ssd0303.c @@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c) s->ds = graphic_console_init(ssd0303_update_display, ssd0303_invalidate_display, - NULL, NULL, s); + NULL, NULL, NULL, s); qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY); return 0; } diff --git a/hw/ssd0323.c b/hw/ssd0323.c index b0b2e94..5e3491f 100644 --- a/hw/ssd0323.c +++ b/hw/ssd0323.c @@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev) s->row_end = 79; s->ds = graphic_console_init(ssd0323_update_display, ssd0323_invalidate_display, - NULL, NULL, s); + NULL, NULL, NULL, s); qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY); qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1); diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c index 420925c..853b180 100644 --- a/hw/tc6393xb.c +++ b/hw/tc6393xb.c @@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq) NULL, /* invalidate */ NULL, /* screen_dump */ NULL, /* text_update */ + NULL, /* screen_dump_async */ s); return s; diff --git a/hw/tcx.c b/hw/tcx.c index ac7dcb4..9214dec 100644 --- a/hw/tcx.c +++ b/hw/tcx.c @@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev) s->ds = graphic_console_init(tcx24_update_display, tcx24_invalidate_display, - tcx24_screen_dump, NULL, s); + tcx24_screen_dump, NULL, NULL, s); } else { /* THC 8 bit (dummy) */ memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8", @@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev) s->ds = graphic_console_init(tcx_update_display, tcx_invalidate_display, - tcx_screen_dump, NULL, s); + tcx_screen_dump, NULL, NULL, s); } qemu_console_resize(s->ds, s->width, s->height); diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c index f8984c6..87b8fc6 100644 --- a/hw/vga-isa-mm.c +++ b/hw/vga-isa-mm.c @@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base, vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space); s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate, - s->vga.screen_dump, s->vga.text_update, s); + s->vga.screen_dump, s->vga.text_update, + s->vga.screen_dump_async, s); vga_init_vbe(&s->vga, address_space); return 0; diff --git a/hw/vga-isa.c b/hw/vga-isa.c index 4bcc4db..a362ed2 100644 --- a/hw/vga-isa.c +++ b/hw/vga-isa.c @@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev) vga_io_memory, 1); memory_region_set_coalescing(vga_io_memory); s->ds = graphic_console_init(s->update, s->invalidate, - s->screen_dump, s->text_update, s); + s->screen_dump, s->text_update, + s->screen_dump_async, s); vga_init_vbe(s, isa_address_space(dev)); /* ROM BIOS */ diff --git a/hw/vga-pci.c b/hw/vga-pci.c index 465b643..8c8dd53 100644 --- a/hw/vga-pci.c +++ b/hw/vga-pci.c @@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev) vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true); s->ds = graphic_console_init(s->update, s->invalidate, - s->screen_dump, s->text_update, s); + s->screen_dump, s->text_update, + s->screen_dump_async, s); /* XXX: VGA_RAM_SIZE must be a power of two */ pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram); diff --git a/hw/vga_int.h b/hw/vga_int.h index 7685b2b..4486cfb 100644 --- a/hw/vga_int.h +++ b/hw/vga_int.h @@ -161,6 +161,7 @@ typedef struct VGACommonState { vga_hw_invalidate_ptr invalidate; vga_hw_screen_dump_ptr screen_dump; vga_hw_text_update_ptr text_update; + vga_hw_screen_dump_async_ptr screen_dump_async; /* hardware mouse cursor support */ uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32]; void (*cursor_invalidate)(struct VGACommonState *s); diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c index 142d9f4..db96e61 100644 --- a/hw/vmware_vga.c +++ b/hw/vmware_vga.c @@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size, s->vga.ds = graphic_console_init(vmsvga_update_display, vmsvga_invalidate_display, vmsvga_screen_dump, - vmsvga_text_update, s); + vmsvga_text_update, + NULL, s); s->fifo_size = SVGA_FIFO_SIZE; diff --git a/hw/xenfb.c b/hw/xenfb.c index 1bcf171..ad9f0d7 100644 --- a/hw/xenfb.c +++ b/hw/xenfb.c @@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev) xenfb_invalidate, NULL, NULL, + NULL, fb); fb->have_console = 1; } @@ -1015,6 +1016,7 @@ wait_more: xenfb_invalidate, NULL, NULL, + NULL, fb); fb->have_console = 1;
adds a handler for the following qmp screendump-async command. graphics_console_init signature change required touching every user, but no implementation of the new vga_hw_screen_dump_async_ptr is provided in this patch. Signed-off-by: Alon Levy <alevy@redhat.com> --- console.c | 4 ++++ console.h | 5 +++++ hw/blizzard.c | 2 +- hw/cirrus_vga.c | 4 ++-- hw/exynos4210_fimd.c | 3 ++- hw/g364fb.c | 2 +- hw/jazz_led.c | 3 ++- hw/milkymist-vgafb.c | 2 +- hw/musicpal.c | 2 +- hw/omap_dss.c | 4 +++- hw/omap_lcdc.c | 2 +- hw/pl110.c | 2 +- hw/pxa2xx_lcd.c | 2 +- hw/qxl.c | 3 ++- hw/sm501.c | 4 ++-- hw/ssd0303.c | 2 +- hw/ssd0323.c | 2 +- hw/tc6393xb.c | 1 + hw/tcx.c | 4 ++-- hw/vga-isa-mm.c | 3 ++- hw/vga-isa.c | 3 ++- hw/vga-pci.c | 3 ++- hw/vga_int.h | 1 + hw/vmware_vga.c | 3 ++- hw/xenfb.c | 2 ++ 25 files changed, 45 insertions(+), 23 deletions(-)