* [Qemu-devel] [PATCH v8 1/9] replay: exception replay fix
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire Pavel Dovgalyuk
` (8 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch fixes replaying the exception when TB cache is full.
It breaks cpu loop execution through setting exception_index
to process such queued work as TB flush.
v8: moved setting of exeption_index to tb_gen_code
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
translate-all.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/translate-all.c b/translate-all.c
index 2026293..abce8f1 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1290,6 +1290,9 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
/* flush must be done */
tb_flush(cpu);
mmap_unlock();
+ /* Set exception index to make sure that the execution loop
+ will exit and go to flushing and other queued work. */
+ cpu->exception_index = EXCP_INTERRUPT;
cpu_loop_exit(cpu);
}
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 1/9] replay: exception replay fix Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 13:02 ` Paolo Bonzini
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag Pavel Dovgalyuk
` (7 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch adds check to break cpu loop when icount expires without
setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
available translated blocks and all instructions were executed.
In icount replay mode unnecessary tb_find will be called (which may
cause an exception) and execution will be non-deterministic.
v8: refactored loop exit code and moved it to separate function
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
cpu-exec.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/cpu-exec.c b/cpu-exec.c
index fa08c73..f9b8ec8 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -523,9 +523,25 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
*last_tb = NULL;
}
}
- if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
+}
+
+
+static void cpu_check_loop_exit(CPUState *cpu)
+{
+ if (unlikely(atomic_read(&cpu->exit_request)
+ /* icount has expired, we need to break the execution loop.
+ This check is needed before tb_find to make execution
+ deterministic - tb_find may cause an exception
+ while translating the code from non-mapped page. */
+ || (use_icount && ((cpu->icount_extra == 0
+ && cpu->icount_decr.u16.low == 0)
+ || (int32_t)cpu->icount_decr.u32 < 0)))) {
atomic_set(&cpu->exit_request, 0);
- cpu->exception_index = EXCP_INTERRUPT;
+ /* If there is an exception that wasn't replayed yet,
+ don't change exception_index. */
+ if (cpu->exception_index == -1) {
+ cpu->exception_index = EXCP_INTERRUPT;
+ }
cpu_loop_exit(cpu);
}
}
@@ -578,9 +594,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
cpu_exec_nocache(cpu, insns_left, *last_tb, false);
align_clocks(sc, cpu);
}
- cpu->exception_index = EXCP_INTERRUPT;
- *last_tb = NULL;
- cpu_loop_exit(cpu);
}
break;
#endif
@@ -634,6 +647,7 @@ int cpu_exec(CPUState *cpu)
for(;;) {
cpu_handle_interrupt(cpu, &last_tb);
+ cpu_check_loop_exit(cpu);
tb = tb_find(cpu, last_tb, tb_exit);
cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
/* Try to align the host and virtual clocks
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire Pavel Dovgalyuk
@ 2017-01-26 13:02 ` Paolo Bonzini
2017-01-26 13:37 ` Pavel Dovgalyuk
0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-26 13:02 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> This patch adds check to break cpu loop when icount expires without
> setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
> available translated blocks and all instructions were executed.
> In icount replay mode unnecessary tb_find will be called (which may
> cause an exception) and execution will be non-deterministic.
>
> v8: refactored loop exit code and moved it to separate function
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
> cpu-exec.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index fa08c73..f9b8ec8 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -523,9 +523,25 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
> *last_tb = NULL;
> }
> }
> - if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
> +}
> +
> +
> +static void cpu_check_loop_exit(CPUState *cpu)
> +{
> + if (unlikely(atomic_read(&cpu->exit_request)
> + /* icount has expired, we need to break the execution loop.
> + This check is needed before tb_find to make execution
> + deterministic - tb_find may cause an exception
> + while translating the code from non-mapped page. */
> + || (use_icount && ((cpu->icount_extra == 0
> + && cpu->icount_decr.u16.low == 0)
> + || (int32_t)cpu->icount_decr.u32 < 0)))) {
Simpler:
use_icount &&
((int32_t)cpu->icount_decr.u32 < 0 ||
cpu->icount_decr.low + cpu->icount_extra == 0)
But I'm not sure that you need to test u32. After all you're not
testing tcg_exit_req, which is the equivalent when icount is disabled.
> atomic_set(&cpu->exit_request, 0);
> - cpu->exception_index = EXCP_INTERRUPT;
> + /* If there is an exception that wasn't replayed yet,
> + don't change exception_index. */
> + if (cpu->exception_index == -1) {
> + cpu->exception_index = EXCP_INTERRUPT;
> + }
> cpu_loop_exit(cpu);
The siglongjmp is effectively the same as exiting the for(;;) loop of
cpu_exec and going back to cpu_handle_exception. So I would just merge
this with cpu_handle_interrupt, which exits a lot with cpu_loop_exit too.
All cpu_loop_exit() calls in cpu_handle_interrupt become "return true",
similar to cpu_handle_exception, and then in cpu_exec you have:
/* if an exception is pending, we execute it here */
while (!cpu_handle_exception(cpu, &ret)) {
/* if an interrupt is pending, inject it and go back
* to cpu_handle_exception.
*/
while (!cpu_handle_interrupt(cpu, &last_tb)) {
tb = tb_find(cpu, last_tb, tb_exit);
cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
/* Try to align the host and virtual clocks
if the guest is in advance */
align_clocks(&sc, cpu);
}
}
break;
> #endif
> @@ -634,6 +647,7 @@ int cpu_exec(CPUState *cpu)
>
> for(;;) {
> cpu_handle_interrupt(cpu, &last_tb);
> + cpu_check_loop_exit(cpu);
if (cpu_should_check_exception_or_exit(cpu)) {
break;
}
> tb = tb_find(cpu, last_tb, tb_exit);
> cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
> /* Try to align the host and virtual clocks
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 13:02 ` Paolo Bonzini
@ 2017-01-26 13:37 ` Pavel Dovgalyuk
2017-01-26 14:28 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 13:37 UTC (permalink / raw)
To: 'Paolo Bonzini', 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> > This patch adds check to break cpu loop when icount expires without
> > setting the TB_EXIT_ICOUNT_EXPIRED flag. It happens when there is no
> > available translated blocks and all instructions were executed.
> > In icount replay mode unnecessary tb_find will be called (which may
> > cause an exception) and execution will be non-deterministic.
> >
> > v8: refactored loop exit code and moved it to separate function
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> > cpu-exec.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index fa08c73..f9b8ec8 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -523,9 +523,25 @@ static inline void cpu_handle_interrupt(CPUState *cpu,
> > *last_tb = NULL;
> > }
> > }
> > - if (unlikely(atomic_read(&cpu->exit_request) || replay_has_interrupt())) {
> > +}
> > +
> > +
> > +static void cpu_check_loop_exit(CPUState *cpu)
> > +{
> > + if (unlikely(atomic_read(&cpu->exit_request)
> > + /* icount has expired, we need to break the execution loop.
> > + This check is needed before tb_find to make execution
> > + deterministic - tb_find may cause an exception
> > + while translating the code from non-mapped page. */
> > + || (use_icount && ((cpu->icount_extra == 0
> > + && cpu->icount_decr.u16.low == 0)
> > + || (int32_t)cpu->icount_decr.u32 < 0)))) {
>
> Simpler:
>
> use_icount &&
> ((int32_t)cpu->icount_decr.u32 < 0 ||
> cpu->icount_decr.low + cpu->icount_extra == 0)
Right.
> But I'm not sure that you need to test u32. After all you're not
Checking u32 is needed, because sometimes it is less than zero.
Consider the code in cpu_loop_exec_tb:
int insns_left = cpu->icount_decr.u32;
if (insns_left > 0) {
It is set to negative in tcg_handle_interrupt.
Ten days ago you also offered setting high bits of u32 in gen_icount:
+ /* Make the next TB exit immediately with TB_EXIT_ICOUNT_EXPIRED. */
+ tcg_gen_st16_i32(-1, cpu_env,
+ -ENV_OFFSET + offsetof(CPUState, icount_decr.u16.high));
> testing tcg_exit_req, which is the equivalent when icount is disabled.
Yes, if we want to refactor the whole loop.
> > atomic_set(&cpu->exit_request, 0);
> > - cpu->exception_index = EXCP_INTERRUPT;
> > + /* If there is an exception that wasn't replayed yet,
> > + don't change exception_index. */
> > + if (cpu->exception_index == -1) {
> > + cpu->exception_index = EXCP_INTERRUPT;
> > + }
> > cpu_loop_exit(cpu);
>
> The siglongjmp is effectively the same as exiting the for(;;) loop of
> cpu_exec and going back to cpu_handle_exception. So I would just merge
> this with cpu_handle_interrupt, which exits a lot with cpu_loop_exit too.
Do you want me to create new version of the patch, which changes the loop this way?
> All cpu_loop_exit() calls in cpu_handle_interrupt become "return true",
> similar to cpu_handle_exception, and then in cpu_exec you have:
>
> /* if an exception is pending, we execute it here */
> while (!cpu_handle_exception(cpu, &ret)) {
> /* if an interrupt is pending, inject it and go back
> * to cpu_handle_exception.
> */
> while (!cpu_handle_interrupt(cpu, &last_tb)) {
> tb = tb_find(cpu, last_tb, tb_exit);
> cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit, &sc);
> /* Try to align the host and virtual clocks
> if the guest is in advance */
> align_clocks(&sc, cpu);
> }
> }
> break;
It might even work faster without excessive cpu_loop_exit calls.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 13:37 ` Pavel Dovgalyuk
@ 2017-01-26 14:28 ` Paolo Bonzini
2017-01-26 14:32 ` Pavel Dovgalyuk
0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-26 14:28 UTC (permalink / raw)
To: Pavel Dovgalyuk, 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>> Simpler:
>>
>> use_icount &&
>> ((int32_t)cpu->icount_decr.u32 < 0 ||
>> cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> Right.
>
>> But I'm not sure that you need to test u32. After all you're not
> Checking u32 is needed, because sometimes it is less than zero.
If cpu->icount_decr.u32 is less than zero, the next translation block
would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
cpu->exception_index = EXCP_INTERRUPT;
*last_tb = NULL;
cpu_loop_exit(cpu);
from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
0, so I don't understand why this part of the patch is necessary.
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 14:28 ` Paolo Bonzini
@ 2017-01-26 14:32 ` Pavel Dovgalyuk
2017-01-26 14:44 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 14:32 UTC (permalink / raw)
To: 'Paolo Bonzini', 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
> >> Simpler:
> >>
> >> use_icount &&
> >> ((int32_t)cpu->icount_decr.u32 < 0 ||
> >> cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> > Right.
> >
> >> But I'm not sure that you need to test u32. After all you're not
> > Checking u32 is needed, because sometimes it is less than zero.
>
> If cpu->icount_decr.u32 is less than zero, the next translation block
> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
>
> cpu->exception_index = EXCP_INTERRUPT;
> *last_tb = NULL;
> cpu_loop_exit(cpu);
>
> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
>
> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
> 0, so I don't understand why this part of the patch is necessary.
I removed that lines because we have to check icount=0 not only when it is expired,
but also when all instructions were executed successfully.
If there are no instructions to execute, calling tb_find (and translation then)
may cause an exception at the wrong moment.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 14:32 ` Pavel Dovgalyuk
@ 2017-01-26 14:44 ` Paolo Bonzini
2017-01-27 6:09 ` Pavel Dovgalyuk
0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-26 14:44 UTC (permalink / raw)
To: Pavel Dovgalyuk, 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>>>> Simpler:
>>>>
>>>> use_icount &&
>>>> ((int32_t)cpu->icount_decr.u32 < 0 ||
>>>> cpu->icount_decr.u16.low + cpu->icount_extra == 0)
>>> Right.
>>>
>>>> But I'm not sure that you need to test u32. After all you're not
>>> Checking u32 is needed, because sometimes it is less than zero.
>>
>> If cpu->icount_decr.u32 is less than zero, the next translation block
>> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
>>
>> cpu->exception_index = EXCP_INTERRUPT;
>> *last_tb = NULL;
>> cpu_loop_exit(cpu);
>>
>> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
>>
>> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
>> 0, so I don't understand why this part of the patch is necessary.
>
> I removed that lines because we have to check icount=0 not only when it is expired,
> but also when all instructions were executed successfully.
> If there are no instructions to execute, calling tb_find (and translation then)
> may cause an exception at the wrong moment.
Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
But for decr.u32 < 0, the same reasoning of this comment is also true:
/* Something asked us to stop executing
* chained TBs; just continue round the main
* loop. Whatever requested the exit will also
* have set something else (eg exit_request or
* interrupt_request) which we will handle
* next time around the loop. But we need to
* ensure the tcg_exit_req read in generated code
* comes before the next read of cpu->exit_request
* or cpu->interrupt_request.
*/
(and by the way, this means that you need an smp_rmb in case
TB_EXIT_ICOUNT_EXPIRED).
Thanks,
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-26 14:44 ` Paolo Bonzini
@ 2017-01-27 6:09 ` Pavel Dovgalyuk
2017-01-27 11:02 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-27 6:09 UTC (permalink / raw)
To: 'Paolo Bonzini', 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
> >>>> Simpler:
> >>>>
> >>>> use_icount &&
> >>>> ((int32_t)cpu->icount_decr.u32 < 0 ||
> >>>> cpu->icount_decr.u16.low + cpu->icount_extra == 0)
> >>> Right.
> >>>
> >>>> But I'm not sure that you need to test u32. After all you're not
> >>> Checking u32 is needed, because sometimes it is less than zero.
> >>
> >> If cpu->icount_decr.u32 is less than zero, the next translation block
> >> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
> >>
> >> cpu->exception_index = EXCP_INTERRUPT;
> >> *last_tb = NULL;
> >> cpu_loop_exit(cpu);
> >>
> >> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
> >>
> >> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
> >> 0, so I don't understand why this part of the patch is necessary.
> >
> > I removed that lines because we have to check icount=0 not only when it is expired,
> > but also when all instructions were executed successfully.
> > If there are no instructions to execute, calling tb_find (and translation then)
> > may cause an exception at the wrong moment.
>
> Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
>
> But for decr.u32 < 0, the same reasoning of this comment is also true:
>
> /* Something asked us to stop executing
> * chained TBs; just continue round the main
> * loop. Whatever requested the exit will also
> * have set something else (eg exit_request or
> * interrupt_request) which we will handle
> * next time around the loop. But we need to
> * ensure the tcg_exit_req read in generated code
> * comes before the next read of cpu->exit_request
> * or cpu->interrupt_request.
> */
Right. If the following lines will not be removed (as opposite to my patch) then checking
decr.u32 < 0 will not be needed.
- cpu->exception_index = EXCP_INTERRUPT;
- *last_tb = NULL;
- cpu_loop_exit(cpu);
What is your point about the new version of that patch?
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire
2017-01-27 6:09 ` Pavel Dovgalyuk
@ 2017-01-27 11:02 ` Paolo Bonzini
0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-27 11:02 UTC (permalink / raw)
To: Pavel Dovgalyuk, 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, kraxel
On 27/01/2017 07:09, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 26/01/2017 15:32, Pavel Dovgalyuk wrote:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> On 26/01/2017 14:37, Pavel Dovgalyuk wrote:
>>>>>> Simpler:
>>>>>>
>>>>>> use_icount &&
>>>>>> ((int32_t)cpu->icount_decr.u32 < 0 ||
>>>>>> cpu->icount_decr.u16.low + cpu->icount_extra == 0)
>>>>> Right.
>>>>>
>>>>>> But I'm not sure that you need to test u32. After all you're not
>>>>> Checking u32 is needed, because sometimes it is less than zero.
>>>>
>>>> If cpu->icount_decr.u32 is less than zero, the next translation block
>>>> would immediately exit with TB_EXIT_ICOUNT_EXPIRED, causing
>>>>
>>>> cpu->exception_index = EXCP_INTERRUPT;
>>>> *last_tb = NULL;
>>>> cpu_loop_exit(cpu);
>>>>
>>>> from cpu_loop_exec_tb's "case TB_EXIT_ICOUNT_EXPIRED".
>>>>
>>>> And the same is true for cpu->icount_decr.u16.low + cpu->icount_extra ==
>>>> 0, so I don't understand why this part of the patch is necessary.
>>>
>>> I removed that lines because we have to check icount=0 not only when it is expired,
>>> but also when all instructions were executed successfully.
>>> If there are no instructions to execute, calling tb_find (and translation then)
>>> may cause an exception at the wrong moment.
>>
>> Ok, that makes sense for cpu->icount_decr.u16.low + cpu->icount_extra == 0.
>>
>> But for decr.u32 < 0, the same reasoning of this comment is also true:
>>
>> /* Something asked us to stop executing
>> * chained TBs; just continue round the main
>> * loop. Whatever requested the exit will also
>> * have set something else (eg exit_request or
>> * interrupt_request) which we will handle
>> * next time around the loop. But we need to
>> * ensure the tcg_exit_req read in generated code
>> * comes before the next read of cpu->exit_request
>> * or cpu->interrupt_request.
>> */
>
> Right. If the following lines will not be removed (as opposite to my patch) then checking
> decr.u32 < 0 will not be needed.
That's what I'm not sure about. u32 < 0 is only true if you have set
the interrupt_request as well, but interrupt requests are processed in
cpu_handle_interrupt and that doesn't require going back to the main loop.
Let me try some cleanups early next week and come back to you with a
patch to base your work on.
Paolo
> - cpu->exception_index = EXCP_INTERRUPT;
> - *last_tb = NULL;
> - cpu_loop_exit(cpu);
>
> What is your point about the new version of that patch?
>
> Pavel Dovgalyuk
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 1/9] replay: exception replay fix Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 2/9] icount: exit cpu loop on expire Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:49 ` Paolo Bonzini
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 4/9] integratorcp: adding vmstate for save/restore Pavel Dovgalyuk
` (6 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch implements saving/restoring of static apic_delivered variable.
v8: saving static variable only for one of the APICs
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
hw/intc/apic_common.c | 37 +++++++++++++++++++++++++++++++++++++
include/hw/i386/apic_internal.h | 2 ++
2 files changed, 39 insertions(+)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d78c885..edacb16 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
return s->wait_for_sipi != 0;
}
+static bool apic_irq_delivered_needed(void *opaque)
+{
+ static APICCommonState *first_apic;
+ APICCommonState *s = APIC_COMMON(opaque);
+ if (!first_apic) {
+ first_apic = s;
+ }
+ return s == first_apic;
+}
+
+static void apic_irq_delivered_pre_save(void *opaque)
+{
+ APICCommonState *s = APIC_COMMON(opaque);
+ s->apic_irq_delivered = apic_irq_delivered;
+}
+
+static int apic_irq_delivered_post_load(void *opaque, int version_id)
+{
+ APICCommonState *s = APIC_COMMON(opaque);
+ apic_irq_delivered = s->apic_irq_delivered;
+ return 0;
+}
+
static const VMStateDescription vmstate_apic_common_sipi = {
.name = "apic_sipi",
.version_id = 1,
@@ -396,6 +419,19 @@ static const VMStateDescription vmstate_apic_common_sipi = {
}
};
+static const VMStateDescription vmstate_apic_irq_delivered = {
+ .name = "apic_irq_delivered",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = apic_irq_delivered_needed,
+ .pre_save = apic_irq_delivered_pre_save,
+ .post_load = apic_irq_delivered_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_INT32(apic_irq_delivered, APICCommonState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_apic_common = {
.name = "apic",
.version_id = 3,
@@ -430,6 +466,7 @@ static const VMStateDescription vmstate_apic_common = {
},
.subsections = (const VMStateDescription*[]) {
&vmstate_apic_common_sipi,
+ &vmstate_apic_irq_delivered,
NULL
}
};
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 1209eb4..20ad28c 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -189,6 +189,8 @@ struct APICCommonState {
DeviceState *vapic;
hwaddr vapic_paddr; /* note: persistence via kvmvapic */
bool legacy_instance_id;
+
+ int apic_irq_delivered; /* for saving static variable */
};
typedef struct VAPICState {
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag Pavel Dovgalyuk
@ 2017-01-26 12:49 ` Paolo Bonzini
2017-01-26 13:03 ` Pavel Dovgalyuk
0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-26 12:49 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, kraxel
On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> This patch implements saving/restoring of static apic_delivered variable.
>
> v8: saving static variable only for one of the APICs
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
> hw/intc/apic_common.c | 37 +++++++++++++++++++++++++++++++++++++
> include/hw/i386/apic_internal.h | 2 ++
> 2 files changed, 39 insertions(+)
>
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index d78c885..edacb16 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
> return s->wait_for_sipi != 0;
> }
>
> +static bool apic_irq_delivered_needed(void *opaque)
> +{
> + static APICCommonState *first_apic;
> + APICCommonState *s = APIC_COMMON(opaque);
> + if (!first_apic) {
> + first_apic = s;
> + }
> + return s == first_apic;
Should also check " && apic_irq_delivered != 0".
Paolo
> +}
> +
> +static void apic_irq_delivered_pre_save(void *opaque)
> +{
> + APICCommonState *s = APIC_COMMON(opaque);
> + s->apic_irq_delivered = apic_irq_delivered;
> +}
> +
> +static int apic_irq_delivered_post_load(void *opaque, int version_id)
> +{
> + APICCommonState *s = APIC_COMMON(opaque);
> + apic_irq_delivered = s->apic_irq_delivered;
> + return 0;
> +}
> +
> static const VMStateDescription vmstate_apic_common_sipi = {
> .name = "apic_sipi",
> .version_id = 1,
> @@ -396,6 +419,19 @@ static const VMStateDescription vmstate_apic_common_sipi = {
> }
> };
>
> +static const VMStateDescription vmstate_apic_irq_delivered = {
> + .name = "apic_irq_delivered",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = apic_irq_delivered_needed,
> + .pre_save = apic_irq_delivered_pre_save,
> + .post_load = apic_irq_delivered_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_INT32(apic_irq_delivered, APICCommonState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_apic_common = {
> .name = "apic",
> .version_id = 3,
> @@ -430,6 +466,7 @@ static const VMStateDescription vmstate_apic_common = {
> },
> .subsections = (const VMStateDescription*[]) {
> &vmstate_apic_common_sipi,
> + &vmstate_apic_irq_delivered,
> NULL
> }
> };
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 1209eb4..20ad28c 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -189,6 +189,8 @@ struct APICCommonState {
> DeviceState *vapic;
> hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> bool legacy_instance_id;
> +
> + int apic_irq_delivered; /* for saving static variable */
> };
>
> typedef struct VAPICState {
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag
2017-01-26 12:49 ` Paolo Bonzini
@ 2017-01-26 13:03 ` Pavel Dovgalyuk
2017-01-26 13:06 ` Paolo Bonzini
0 siblings, 1 reply; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 13:03 UTC (permalink / raw)
To: 'Paolo Bonzini', 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, kraxel
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> > This patch implements saving/restoring of static apic_delivered variable.
> >
> > v8: saving static variable only for one of the APICs
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> > hw/intc/apic_common.c | 37 +++++++++++++++++++++++++++++++++++++
> > include/hw/i386/apic_internal.h | 2 ++
> > 2 files changed, 39 insertions(+)
> >
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index d78c885..edacb16 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
> > return s->wait_for_sipi != 0;
> > }
> >
> > +static bool apic_irq_delivered_needed(void *opaque)
> > +{
> > + static APICCommonState *first_apic;
> > + APICCommonState *s = APIC_COMMON(opaque);
> > + if (!first_apic) {
> > + first_apic = s;
> > + }
> > + return s == first_apic;
>
> Should also check " && apic_irq_delivered != 0".
Reset of this variable when machine reboots is also forgotten.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag
2017-01-26 13:03 ` Pavel Dovgalyuk
@ 2017-01-26 13:06 ` Paolo Bonzini
2017-01-26 13:07 ` Pavel Dovgalyuk
0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-26 13:06 UTC (permalink / raw)
To: Pavel Dovgalyuk, 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
On 26/01/2017 14:03, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
>>> This patch implements saving/restoring of static apic_delivered variable.
>>>
>>> v8: saving static variable only for one of the APICs
>>>
>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>> ---
>>> hw/intc/apic_common.c | 37 +++++++++++++++++++++++++++++++++++++
>>> include/hw/i386/apic_internal.h | 2 ++
>>> 2 files changed, 39 insertions(+)
>>>
>>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>>> index d78c885..edacb16 100644
>>> --- a/hw/intc/apic_common.c
>>> +++ b/hw/intc/apic_common.c
>>> @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
>>> return s->wait_for_sipi != 0;
>>> }
>>>
>>> +static bool apic_irq_delivered_needed(void *opaque)
>>> +{
>>> + static APICCommonState *first_apic;
>>> + APICCommonState *s = APIC_COMMON(opaque);
>>> + if (!first_apic) {
>>> + first_apic = s;
>>> + }
>>> + return s == first_apic;
>>
>> Should also check " && apic_irq_delivered != 0".
>
> Reset of this variable when machine reboots is also forgotten.
Ok, I'll queue this patch with the condition changed, can you send one
for reset?
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag
2017-01-26 13:06 ` Paolo Bonzini
@ 2017-01-26 13:07 ` Pavel Dovgalyuk
0 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 13:07 UTC (permalink / raw)
To: 'Paolo Bonzini', 'Pavel Dovgalyuk', qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> On 26/01/2017 14:03, Pavel Dovgalyuk wrote:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> >>> This patch implements saving/restoring of static apic_delivered variable.
> >>>
> >>> v8: saving static variable only for one of the APICs
> >>>
> >>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> >>> ---
> >>> hw/intc/apic_common.c | 37 +++++++++++++++++++++++++++++++++++++
> >>> include/hw/i386/apic_internal.h | 2 ++
> >>> 2 files changed, 39 insertions(+)
> >>>
> >>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >>> index d78c885..edacb16 100644
> >>> --- a/hw/intc/apic_common.c
> >>> +++ b/hw/intc/apic_common.c
> >>> @@ -384,6 +384,29 @@ static bool apic_common_sipi_needed(void *opaque)
> >>> return s->wait_for_sipi != 0;
> >>> }
> >>>
> >>> +static bool apic_irq_delivered_needed(void *opaque)
> >>> +{
> >>> + static APICCommonState *first_apic;
> >>> + APICCommonState *s = APIC_COMMON(opaque);
> >>> + if (!first_apic) {
> >>> + first_apic = s;
> >>> + }
> >>> + return s == first_apic;
> >>
> >> Should also check " && apic_irq_delivered != 0".
> >
> > Reset of this variable when machine reboots is also forgotten.
>
> Ok, I'll queue this patch with the condition changed, can you send one
> for reset?
Ok, I'll send it with the next version of the series.
Pavel Dovgalyuk
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v8 4/9] integratorcp: adding vmstate for save/restore
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (2 preceding siblings ...)
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 3/9] apic: save apic_delivered flag Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 5/9] block: implement bdrv_snapshot_goto for blkreplay Pavel Dovgalyuk
` (5 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
VMState added by this patch preserves correct
loading of the integratorcp device state.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
hw/arm/integratorcp.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 039812a..ca06e1b 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -53,6 +53,27 @@ static uint8_t integrator_spd[128] = {
0xe, 4, 0x1c, 1, 2, 0x20, 0xc0, 0, 0, 0, 0, 0x30, 0x28, 0x30, 0x28, 0x40
};
+static const VMStateDescription vmstate_integratorcm = {
+ .name = "integratorcm",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(cm_osc, IntegratorCMState),
+ VMSTATE_UINT32(cm_ctrl, IntegratorCMState),
+ VMSTATE_UINT32(cm_lock, IntegratorCMState),
+ VMSTATE_UINT32(cm_auxosc, IntegratorCMState),
+ VMSTATE_UINT32(cm_sdram, IntegratorCMState),
+ VMSTATE_UINT32(cm_init, IntegratorCMState),
+ VMSTATE_UINT32(cm_flags, IntegratorCMState),
+ VMSTATE_UINT32(cm_nvflags, IntegratorCMState),
+ VMSTATE_UINT32(int_level, IntegratorCMState),
+ VMSTATE_UINT32(irq_enabled, IntegratorCMState),
+ VMSTATE_UINT32(fiq_enabled, IntegratorCMState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static uint64_t integratorcm_read(void *opaque, hwaddr offset,
unsigned size)
{
@@ -309,6 +330,19 @@ typedef struct icp_pic_state {
qemu_irq parent_fiq;
} icp_pic_state;
+static const VMStateDescription vmstate_icp_pic = {
+ .name = "icp_pic",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(level, icp_pic_state),
+ VMSTATE_UINT32(irq_enabled, icp_pic_state),
+ VMSTATE_UINT32(fiq_enabled, icp_pic_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void icp_pic_update(icp_pic_state *s)
{
uint32_t flags;
@@ -438,6 +472,17 @@ typedef struct ICPCtrlRegsState {
#define ICP_INTREG_WPROT (1 << 0)
#define ICP_INTREG_CARDIN (1 << 3)
+static const VMStateDescription vmstate_icp_control = {
+ .name = "icp_control",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(intreg_state, ICPCtrlRegsState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static uint64_t icp_control_read(void *opaque, hwaddr offset,
unsigned size)
{
@@ -640,6 +685,21 @@ static void core_class_init(ObjectClass *klass, void *data)
dc->props = core_properties;
dc->realize = integratorcm_realize;
+ dc->vmsd = &vmstate_integratorcm;
+}
+
+static void icp_pic_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->vmsd = &vmstate_icp_pic;
+}
+
+static void icp_control_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->vmsd = &vmstate_icp_control;
}
static const TypeInfo core_info = {
@@ -655,6 +715,7 @@ static const TypeInfo icp_pic_info = {
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(icp_pic_state),
.instance_init = icp_pic_init,
+ .class_init = icp_pic_class_init,
};
static const TypeInfo icp_ctrl_regs_info = {
@@ -662,6 +723,7 @@ static const TypeInfo icp_ctrl_regs_info = {
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(ICPCtrlRegsState),
.instance_init = icp_control_init,
+ .class_init = icp_control_class_init,
};
static void integratorcp_register_types(void)
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v8 5/9] block: implement bdrv_snapshot_goto for blkreplay
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (3 preceding siblings ...)
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 4/9] integratorcp: adding vmstate for save/restore Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 6/9] blkreplay: create temporary overlay for underlaying devices Pavel Dovgalyuk
` (4 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch enables making snapshots with blkreplay used in
block devices.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
block/blkreplay.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/block/blkreplay.c b/block/blkreplay.c
index a741654..8a03d62 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -130,6 +130,12 @@ static int coroutine_fn blkreplay_co_flush(BlockDriverState *bs)
return ret;
}
+static int blkreplay_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id)
+{
+ return bdrv_snapshot_goto(bs->file->bs, snapshot_id);
+}
+
static BlockDriver bdrv_blkreplay = {
.format_name = "blkreplay",
.protocol_name = "blkreplay",
@@ -145,6 +151,8 @@ static BlockDriver bdrv_blkreplay = {
.bdrv_co_pwrite_zeroes = blkreplay_co_pwrite_zeroes,
.bdrv_co_pdiscard = blkreplay_co_pdiscard,
.bdrv_co_flush = blkreplay_co_flush,
+
+ .bdrv_snapshot_goto = blkreplay_snapshot_goto,
};
static void bdrv_blkreplay_init(void)
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v8 6/9] blkreplay: create temporary overlay for underlaying devices
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (4 preceding siblings ...)
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 5/9] block: implement bdrv_snapshot_goto for blkreplay Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 7/9] replay: disable default snapshot for record/replay Pavel Dovgalyuk
` (3 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch allows using '-snapshot' behavior in record/replay mode.
blkreplay layer creates temporary overlays on top of underlaying
disk images. It is needed, because creating an overlay over blkreplay
breaks the determinism.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
block/blkreplay.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
stubs/replay.c | 1 +
vl.c | 2 +
3 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 8a03d62..172642f 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -14,12 +14,76 @@
#include "block/block_int.h"
#include "sysemu/replay.h"
#include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
typedef struct Request {
Coroutine *co;
QEMUBH *bh;
} Request;
+static BlockDriverState *blkreplay_append_snapshot(BlockDriverState *bs,
+ Error **errp)
+{
+ int ret;
+ BlockDriverState *bs_snapshot;
+ int64_t total_size;
+ QemuOpts *opts = NULL;
+ char tmp_filename[PATH_MAX + 1];
+ QDict *snapshot_options = qdict_new();
+
+ /* Prepare options QDict for the overlay file */
+ qdict_put(snapshot_options, "file.driver",
+ qstring_from_str("file"));
+ qdict_put(snapshot_options, "driver",
+ qstring_from_str("qcow2"));
+
+ /* Create temporary file */
+ ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not get temporary filename");
+ goto out;
+ }
+ qdict_put(snapshot_options, "file.filename",
+ qstring_from_str(tmp_filename));
+
+ /* Get the required size from the image */
+ total_size = bdrv_getlength(bs);
+ if (total_size < 0) {
+ error_setg_errno(errp, -total_size, "Could not get image size");
+ goto out;
+ }
+
+ opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0, &error_abort);
+ qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort);
+ ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp);
+ qemu_opts_del(opts);
+ if (ret < 0) {
+ error_prepend(errp, "Could not create temporary overlay '%s': ",
+ tmp_filename);
+ goto out;
+ }
+
+ bs_snapshot = bdrv_open(NULL, NULL, snapshot_options,
+ BDRV_O_RDWR | BDRV_O_TEMPORARY, errp);
+ snapshot_options = NULL;
+ if (!bs_snapshot) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will
+ * call bdrv_unref() on it), so in order to be able to return one, we have
+ * to increase bs_snapshot's refcount here */
+ bdrv_ref(bs_snapshot);
+ bdrv_append(bs_snapshot, bs);
+
+ return bs_snapshot;
+
+out:
+ QDECREF(snapshot_options);
+ return NULL;
+}
+
static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -35,6 +99,14 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
+ /* Add temporary snapshot to preserve the image */
+ if (!replay_snapshot
+ && !blkreplay_append_snapshot(bs->file->bs, &local_err)) {
+ ret = -EINVAL;
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
ret = 0;
fail:
if (ret < 0) {
@@ -45,6 +117,10 @@ fail:
static void blkreplay_close(BlockDriverState *bs)
{
+ if (!replay_snapshot) {
+ /* Unref created snapshot file */
+ bdrv_unref(bs->file->bs);
+ }
}
static int64_t blkreplay_getlength(BlockDriverState *bs)
diff --git a/stubs/replay.c b/stubs/replay.c
index d9a6da9..e43e467 100644
--- a/stubs/replay.c
+++ b/stubs/replay.c
@@ -3,6 +3,7 @@
#include "sysemu/sysemu.h"
ReplayMode replay_mode;
+char *replay_snapshot;
int64_t replay_save_clock(unsigned int kind, int64_t clock)
{
diff --git a/vl.c b/vl.c
index 52a6ac8..def0520 100644
--- a/vl.c
+++ b/vl.c
@@ -4474,7 +4474,7 @@ int main(int argc, char **argv, char **envp)
}
/* open the virtual block devices */
- if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+ if (snapshot) {
qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
NULL, NULL);
}
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v8 7/9] replay: disable default snapshot for record/replay
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (5 preceding siblings ...)
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 6/9] blkreplay: create temporary overlay for underlaying devices Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 8/9] audio: make audio poll timer deterministic Pavel Dovgalyuk
` (2 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch disables setting '-snapshot' option on by default
in record/replay mode. This is needed for creating vmstates in record
and replay modes.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
vl.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index def0520..bddb2bc 100644
--- a/vl.c
+++ b/vl.c
@@ -3198,7 +3198,13 @@ int main(int argc, char **argv, char **envp)
drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
break;
case QEMU_OPTION_snapshot:
- snapshot = 1;
+ {
+ Error *blocker = NULL;
+ snapshot = 1;
+ error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED,
+ "-snapshot");
+ replay_add_blocker(blocker);
+ }
break;
case QEMU_OPTION_hdachs:
{
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v8 8/9] audio: make audio poll timer deterministic
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (6 preceding siblings ...)
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 7/9] replay: disable default snapshot for record/replay Pavel Dovgalyuk
@ 2017-01-26 12:34 ` Pavel Dovgalyuk
2017-01-26 12:35 ` [Qemu-devel] [PATCH v8 9/9] replay: add record/replay for audio passthrough Pavel Dovgalyuk
2017-01-26 13:05 ` [Qemu-devel] [PATCH v8 0/9] replay additions Paolo Bonzini
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch changes resetting strategy of the audio polling timer.
It does not change expiration time if the timer is already set.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
audio/audio.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/audio/audio.c b/audio/audio.c
index c845a44..1ee95a5 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
static void audio_reset_timer (AudioState *s)
{
if (audio_is_timer_needed ()) {
- timer_mod (s->ts,
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+ if (!timer_pending(s->ts)) {
+ timer_mod (s->ts,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+ }
}
else {
timer_del (s->ts);
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v8 9/9] replay: add record/replay for audio passthrough
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (7 preceding siblings ...)
2017-01-26 12:34 ` [Qemu-devel] [PATCH v8 8/9] audio: make audio poll timer deterministic Pavel Dovgalyuk
@ 2017-01-26 12:35 ` Pavel Dovgalyuk
2017-01-26 13:05 ` [Qemu-devel] [PATCH v8 0/9] replay additions Paolo Bonzini
9 siblings, 0 replies; 22+ messages in thread
From: Pavel Dovgalyuk @ 2017-01-26 12:35 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, peter.maydell, mst, jasowang, quintela, dovgaluk, kraxel,
pbonzini
This patch adds recording and replaying audio data. Is saves synchronization
information for audio out and inputs from the microphone.
Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
audio/audio.c | 11 +++++-
audio/audio.h | 5 +++
audio/mixeng.c | 31 ++++++++++++++++++
docs/replay.txt | 7 ++++
include/sysemu/replay.h | 7 ++++
replay/Makefile.objs | 1 +
replay/replay-audio.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++
replay/replay-internal.h | 4 ++
8 files changed, 142 insertions(+), 3 deletions(-)
create mode 100644 replay/replay-audio.c
diff --git a/audio/audio.c b/audio/audio.c
index 1ee95a5..79c0788 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -28,6 +28,7 @@
#include "qemu/timer.h"
#include "sysemu/sysemu.h"
#include "qemu/cutils.h"
+#include "sysemu/replay.h"
#define AUDIO_CAP "audio"
#include "audio_int.h"
@@ -1113,7 +1114,7 @@ static void audio_reset_timer (AudioState *s)
{
if (audio_is_timer_needed ()) {
if (!timer_pending(s->ts)) {
- timer_mod (s->ts,
+ timer_mod(s->ts,
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
}
}
@@ -1389,6 +1390,7 @@ static void audio_run_out (AudioState *s)
prev_rpos = hw->rpos;
played = hw->pcm_ops->run_out (hw, live);
+ replay_audio_out(&played);
if (audio_bug (AUDIO_FUNC, hw->rpos >= hw->samples)) {
dolog ("hw->rpos=%d hw->samples=%d played=%d\n",
hw->rpos, hw->samples, played);
@@ -1452,9 +1454,12 @@ static void audio_run_in (AudioState *s)
while ((hw = audio_pcm_hw_find_any_enabled_in (hw))) {
SWVoiceIn *sw;
- int captured, min;
+ int captured = 0, min;
- captured = hw->pcm_ops->run_in (hw);
+ if (replay_mode != REPLAY_MODE_PLAY) {
+ captured = hw->pcm_ops->run_in(hw);
+ }
+ replay_audio_in(&captured, hw->conv_buf, &hw->wpos, hw->samples);
min = audio_pcm_hw_find_min_in (hw);
hw->total_samples_captured += captured - min;
diff --git a/audio/audio.h b/audio/audio.h
index c3c5198..f4339a1 100644
--- a/audio/audio.h
+++ b/audio/audio.h
@@ -166,4 +166,9 @@ int wav_start_capture (CaptureState *s, const char *path, int freq,
bool audio_is_cleaning_up(void);
void audio_cleanup(void);
+void audio_sample_to_uint64(void *samples, int pos,
+ uint64_t *left, uint64_t *right);
+void audio_sample_from_uint64(void *samples, int pos,
+ uint64_t left, uint64_t right);
+
#endif /* QEMU_AUDIO_H */
diff --git a/audio/mixeng.c b/audio/mixeng.c
index 66c0328..c23508e 100644
--- a/audio/mixeng.c
+++ b/audio/mixeng.c
@@ -267,6 +267,37 @@ f_sample *mixeng_clip[2][2][2][3] = {
}
};
+
+void audio_sample_to_uint64(void *samples, int pos,
+ uint64_t *left, uint64_t *right)
+{
+ struct st_sample *sample = samples;
+ sample += pos;
+#ifdef FLOAT_MIXENG
+ error_report(
+ "Coreaudio and floating point samples are not supported by replay yet");
+ abort();
+#else
+ *left = sample->l;
+ *right = sample->r;
+#endif
+}
+
+void audio_sample_from_uint64(void *samples, int pos,
+ uint64_t left, uint64_t right)
+{
+ struct st_sample *sample = samples;
+ sample += pos;
+#ifdef FLOAT_MIXENG
+ error_report(
+ "Coreaudio and floating point samples are not supported by replay yet");
+ abort();
+#else
+ sample->l = left;
+ sample->r = right;
+#endif
+}
+
/*
* August 21, 1998
* Copyright 1998 Fabrice Bellard.
diff --git a/docs/replay.txt b/docs/replay.txt
index 03e1931..486c1e0 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -225,3 +225,10 @@ recording the virtual machine this filter puts all packets coming from
the outer world into the log. In replay mode packets from the log are
injected into the network device. All interactions with network backend
in replay mode are disabled.
+
+Audio devices
+-------------
+
+Audio data is recorded and replay automatically. The command line for recording
+and replaying must contain identical specifications of audio hardware, e.g.:
+ -soundhw ac97
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index 740b425..073ec59 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -152,6 +152,13 @@ void replay_unregister_net(ReplayNetState *rns);
void replay_net_packet_event(ReplayNetState *rns, unsigned flags,
const struct iovec *iov, int iovcnt);
+/* Audio */
+
+/*! Saves/restores number of played samples of audio out operation. */
+void replay_audio_out(int *played);
+/*! Saves/restores recorded samples of audio in operation. */
+void replay_audio_in(int *recorded, void *samples, int *wpos, int size);
+
/* VM state operations */
/*! Called at the start of execution.
diff --git a/replay/Makefile.objs b/replay/Makefile.objs
index b2afd40..cee6539 100644
--- a/replay/Makefile.objs
+++ b/replay/Makefile.objs
@@ -6,3 +6,4 @@ common-obj-y += replay-input.o
common-obj-y += replay-char.o
common-obj-y += replay-snapshot.o
common-obj-y += replay-net.o
+common-obj-y += replay-audio.o
\ No newline at end of file
diff --git a/replay/replay-audio.c b/replay/replay-audio.c
new file mode 100644
index 0000000..3d83743
--- /dev/null
+++ b/replay/replay-audio.c
@@ -0,0 +1,79 @@
+/*
+ * replay-audio.c
+ *
+ * Copyright (c) 2010-2017 Institute for System Programming
+ * of the Russian Academy of Sciences.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "sysemu/replay.h"
+#include "replay-internal.h"
+#include "sysemu/sysemu.h"
+#include "audio/audio.h"
+
+void replay_audio_out(int *played)
+{
+ if (replay_mode == REPLAY_MODE_RECORD) {
+ replay_save_instructions();
+ replay_mutex_lock();
+ replay_put_event(EVENT_AUDIO_OUT);
+ replay_put_dword(*played);
+ replay_mutex_unlock();
+ } else if (replay_mode == REPLAY_MODE_PLAY) {
+ replay_account_executed_instructions();
+ replay_mutex_lock();
+ if (replay_next_event_is(EVENT_AUDIO_OUT)) {
+ *played = replay_get_dword();
+ replay_finish_event();
+ replay_mutex_unlock();
+ } else {
+ replay_mutex_unlock();
+ error_report("Missing audio out event in the replay log");
+ abort();
+ }
+ }
+}
+
+void replay_audio_in(int *recorded, void *samples, int *wpos, int size)
+{
+ int pos;
+ uint64_t left, right;
+ if (replay_mode == REPLAY_MODE_RECORD) {
+ replay_save_instructions();
+ replay_mutex_lock();
+ replay_put_event(EVENT_AUDIO_IN);
+ replay_put_dword(*recorded);
+ replay_put_dword(*wpos);
+ for (pos = (*wpos - *recorded + size) % size ; pos != *wpos
+ ; pos = (pos + 1) % size) {
+ audio_sample_to_uint64(samples, pos, &left, &right);
+ replay_put_qword(left);
+ replay_put_qword(right);
+ }
+ replay_mutex_unlock();
+ } else if (replay_mode == REPLAY_MODE_PLAY) {
+ replay_account_executed_instructions();
+ replay_mutex_lock();
+ if (replay_next_event_is(EVENT_AUDIO_IN)) {
+ *recorded = replay_get_dword();
+ *wpos = replay_get_dword();
+ for (pos = (*wpos - *recorded + size) % size ; pos != *wpos
+ ; pos = (pos + 1) % size) {
+ left = replay_get_qword();
+ right = replay_get_qword();
+ audio_sample_from_uint64(samples, pos, left, right);
+ }
+ replay_finish_event();
+ replay_mutex_unlock();
+ } else {
+ replay_mutex_unlock();
+ error_report("Missing audio in event in the replay log");
+ abort();
+ }
+ }
+}
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index c26d079..ed66ed8 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -29,6 +29,10 @@ enum ReplayEvents {
/* for character device read all event */
EVENT_CHAR_READ_ALL,
EVENT_CHAR_READ_ALL_ERROR,
+ /* for audio out event */
+ EVENT_AUDIO_OUT,
+ /* for audio in event */
+ EVENT_AUDIO_IN,
/* for clock read/writes */
/* some of greater codes are reserved for clocks */
EVENT_CLOCK,
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v8 0/9] replay additions
2017-01-26 12:34 [Qemu-devel] [PATCH v8 0/9] replay additions Pavel Dovgalyuk
` (8 preceding siblings ...)
2017-01-26 12:35 ` [Qemu-devel] [PATCH v8 9/9] replay: add record/replay for audio passthrough Pavel Dovgalyuk
@ 2017-01-26 13:05 ` Paolo Bonzini
9 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2017-01-26 13:05 UTC (permalink / raw)
To: Pavel Dovgalyuk, qemu-devel
Cc: kwolf, peter.maydell, quintela, jasowang, mst, kraxel
On 26/01/2017 13:34, Pavel Dovgalyuk wrote:
> This set of patches includes several fixes for replay and vmstate.
>
> This patches add rrsnapshot option for icount. rrshapshot option creates
> start snapshot at record and loads it at replay. It allows preserving
> the state of disk images used by virtual machine. This vm state can also
> use used to roll back the execution while replaying.
>
> With these patches operations with audio devices can also be recorded
> and replayed. All interactions with passthrough audio (including
> microphone input) are recorded automatically when -soundhw is specified
> at the command line.
>
> This set of patches includes fixes and additions for icount and
> record/replay implementation:
> - VM start/stop in replay mode
> - overlay creation for blkreplay filter
> - rrsnapshot option for record/replay
> - vmstate fix for integratorcp ARM platform
> - vmstate fixes for apic and rtc
> - fixes for icount record/replay mode
> - record/replay for audio devices
>
> v8 changes:
> - Refined replay exception processing (as suggested by Paolo Bonzini)
> - Saving/restoring static variable for APIC only once (as suggested by Paolo Bonzini)
> - Removed already queued patches
> - Minor fixes
>
> v7 changes:
> - Fixed exception replaying when TB cache is full and
> when tb_find is called when there are no instructions about to execute
> - Added record/replay for audio devices
>
> v6 changes:
> - Added overlay creation for blkreplay driver
> - Fixed vmstate loading for apic and rtc
> - Fixed instruction counting for apic instruction patching
>
> v5 changes:
> - Recording is stopped when initial snapshot cannot be created
> - Minor changes
>
> v4 changes:
> - Overlay option is removed from blkreplay driver (as suggested by Paolo Bonzini)
> - Minor changes
>
> v3 changes:
> - Added rrsnapshot option for specifying the initial snapshot name (as suggested by Paolo Bonzini)
> - Minor changes
>
> ---
>
> Pavel Dovgalyuk (9):
> replay: exception replay fix
> icount: exit cpu loop on expire
> apic: save apic_delivered flag
> integratorcp: adding vmstate for save/restore
> block: implement bdrv_snapshot_goto for blkreplay
> blkreplay: create temporary overlay for underlaying devices
> replay: disable default snapshot for record/replay
> audio: make audio poll timer deterministic
> replay: add record/replay for audio passthrough
>
>
> audio/audio.c | 15 +++++--
> audio/audio.h | 5 ++
> audio/mixeng.c | 31 ++++++++++++++
> block/blkreplay.c | 84 +++++++++++++++++++++++++++++++++++++++
> cpu-exec.c | 24 +++++++++--
> docs/replay.txt | 7 +++
> hw/arm/integratorcp.c | 62 +++++++++++++++++++++++++++++
> hw/intc/apic_common.c | 37 +++++++++++++++++
> include/hw/i386/apic_internal.h | 2 +
> include/sysemu/replay.h | 7 +++
> replay/Makefile.objs | 1
> replay/replay-audio.c | 79 +++++++++++++++++++++++++++++++++++++
> replay/replay-internal.h | 4 ++
> stubs/replay.c | 1
> translate-all.c | 3 +
> vl.c | 10 ++++-
> 16 files changed, 361 insertions(+), 11 deletions(-)
> create mode 100644 replay/replay-audio.c
>
Queued patch 1 too.
Thanks for following my reviews. Record/replay has the potential of
making the code either much better or much worse, and I know we can make
it better because it's what happened for icount. But if it makes the
code worse, at some point it becomes a labyrinth and the only choice is
to remove RR support. :( So let's avoid that!
Paolo
Paolo
^ permalink raw reply [flat|nested] 22+ messages in thread