| Message ID | 20200731125127.30866-3-robert.foley@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path | expand |
On 31/07/20 14:51, Robert Foley wrote: > This change removes the implied BQL from the cpu_handle_interrupt, > and cpu_handle_exception paths. We can now select per-arch if > the BQL is needed or not by using the bql_interrupt flag. > By default, the core code holds the BQL. > One benefit of this change is that it leaves it up to the arch > to make the change to remove BQL when it makes sense. > > Signed-off-by: Robert Foley <robert.foley@linaro.org> No, please just modify all implementation to do lock/unlock. It's a simpler patch than this on. Paolo > --- > accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++++-------- > 1 file changed, 26 insertions(+), 8 deletions(-) > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index 80d0e649b2..cde27ee0bf 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -517,9 +517,13 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret) > #else > if (replay_exception()) { > CPUClass *cc = CPU_GET_CLASS(cpu); > - qemu_mutex_lock_iothread(); > + if (cc->bql_interrupt) { > + qemu_mutex_lock_iothread(); > + } > cc->do_interrupt(cpu); > - qemu_mutex_unlock_iothread(); > + if (cc->bql_interrupt) { > + qemu_mutex_unlock_iothread(); > + } > cpu->exception_index = -1; > > if (unlikely(cpu->singlestep_enabled)) { > @@ -558,7 +562,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > if (unlikely(cpu_interrupt_request(cpu))) { > int interrupt_request; > > - qemu_mutex_lock_iothread(); > + cpu_mutex_lock(cpu); > interrupt_request = cpu_interrupt_request(cpu); > if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { > /* Mask out external interrupts for this step. */ > @@ -567,7 +571,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > if (interrupt_request & CPU_INTERRUPT_DEBUG) { > cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG); > cpu->exception_index = EXCP_DEBUG; > - qemu_mutex_unlock_iothread(); > + cpu_mutex_unlock(cpu); > return true; > } > if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) { > @@ -577,13 +581,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT); > cpu_halted_set(cpu, 1); > cpu->exception_index = EXCP_HLT; > - qemu_mutex_unlock_iothread(); > + cpu_mutex_unlock(cpu); > return true; > } > #if defined(TARGET_I386) > else if (interrupt_request & CPU_INTERRUPT_INIT) { > X86CPU *x86_cpu = X86_CPU(cpu); > CPUArchState *env = &x86_cpu->env; > + cpu_mutex_unlock(cpu); > + qemu_mutex_lock_iothread(); > replay_interrupt(); > cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0); > do_cpu_init(x86_cpu); > @@ -595,7 +601,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > else if (interrupt_request & CPU_INTERRUPT_RESET) { > replay_interrupt(); > cpu_reset(cpu); > - qemu_mutex_unlock_iothread(); > + cpu_mutex_unlock(cpu); > return true; > } > #endif > @@ -604,7 +610,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > True when it is, and we should restart on a new TB, > and via longjmp via cpu_loop_exit. */ > else { > + cpu_mutex_unlock(cpu); > + if (cc->bql_interrupt) { > + qemu_mutex_lock_iothread(); > + } > if (cc->cpu_exec_interrupt(cpu, interrupt_request)) { > + if (cc->bql_interrupt) { > + qemu_mutex_unlock_iothread(); > + } > + cpu_mutex_lock(cpu); > replay_interrupt(); > /* > * After processing the interrupt, ensure an EXCP_DEBUG is > @@ -614,6 +628,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > cpu->exception_index = > (cpu->singlestep_enabled ? EXCP_DEBUG : -1); > *last_tb = NULL; > + } else { > + if (cc->bql_interrupt) { > + qemu_mutex_unlock_iothread(); > + } > + cpu_mutex_lock(cpu); > } > /* The target hook may have updated the 'cpu->interrupt_request'; > * reload the 'interrupt_request' value */ > @@ -627,7 +646,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > } > > /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */ > - qemu_mutex_unlock_iothread(); > + cpu_mutex_unlock(cpu); > } > > /* Finally, check if we need to exit to the main loop. */ > @@ -691,7 +710,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, > } > #endif > } > - > /* main execution loop */ > > int cpu_exec(CPUState *cpu) >
On Fri, 31 Jul 2020 at 14:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 31/07/20 14:51, Robert Foley wrote: > > This change removes the implied BQL from the cpu_handle_interrupt, > > and cpu_handle_exception paths. We can now select per-arch if > > the BQL is needed or not by using the bql_interrupt flag. > > By default, the core code holds the BQL. > > One benefit of this change is that it leaves it up to the arch > > to make the change to remove BQL when it makes sense. > > > > Signed-off-by: Robert Foley <robert.foley@linaro.org> > > No, please just modify all implementation to do lock/unlock. It's a > simpler patch than this on. Sure, we will update the patch based on this. To clarify, the suggestion here is to remove the bql_interrupt flag that we added and change all the per-arch interrupt callback code to do the lock/unlock of the BQL? So for example change x86_cpu_exec_interrupt, and arm_cpu_exec_interrupt, etc to lock/unlock BQL? Thanks, -Rob > > Paolo > > > --- > > accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++++-------- > > 1 file changed, 26 insertions(+), 8 deletions(-) > > > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > > index 80d0e649b2..cde27ee0bf 100644 > > --- a/accel/tcg/cpu-exec.c > > +++ b/accel/tcg/cpu-exec.c > > @@ -517,9 +517,13 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret) > > #else > > if (replay_exception()) { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > - qemu_mutex_lock_iothread(); > > + if (cc->bql_interrupt) { > > + qemu_mutex_lock_iothread(); > > + } > > cc->do_interrupt(cpu); > > - qemu_mutex_unlock_iothread(); > > + if (cc->bql_interrupt) { > > + qemu_mutex_unlock_iothread(); > > + } > > cpu->exception_index = -1; > > > > if (unlikely(cpu->singlestep_enabled)) { > > @@ -558,7 +562,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > if (unlikely(cpu_interrupt_request(cpu))) { > > int interrupt_request; > > > > - qemu_mutex_lock_iothread(); > > + cpu_mutex_lock(cpu); > > interrupt_request = cpu_interrupt_request(cpu); > > if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { > > /* Mask out external interrupts for this step. */ > > @@ -567,7 +571,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > if (interrupt_request & CPU_INTERRUPT_DEBUG) { > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG); > > cpu->exception_index = EXCP_DEBUG; > > - qemu_mutex_unlock_iothread(); > > + cpu_mutex_unlock(cpu); > > return true; > > } > > if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) { > > @@ -577,13 +581,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT); > > cpu_halted_set(cpu, 1); > > cpu->exception_index = EXCP_HLT; > > - qemu_mutex_unlock_iothread(); > > + cpu_mutex_unlock(cpu); > > return true; > > } > > #if defined(TARGET_I386) > > else if (interrupt_request & CPU_INTERRUPT_INIT) { > > X86CPU *x86_cpu = X86_CPU(cpu); > > CPUArchState *env = &x86_cpu->env; > > + cpu_mutex_unlock(cpu); > > + qemu_mutex_lock_iothread(); > > replay_interrupt(); > > cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0); > > do_cpu_init(x86_cpu); > > @@ -595,7 +601,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > else if (interrupt_request & CPU_INTERRUPT_RESET) { > > replay_interrupt(); > > cpu_reset(cpu); > > - qemu_mutex_unlock_iothread(); > > + cpu_mutex_unlock(cpu); > > return true; > > } > > #endif > > @@ -604,7 +610,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > True when it is, and we should restart on a new TB, > > and via longjmp via cpu_loop_exit. */ > > else { > > + cpu_mutex_unlock(cpu); > > + if (cc->bql_interrupt) { > > + qemu_mutex_lock_iothread(); > > + } > > if (cc->cpu_exec_interrupt(cpu, interrupt_request)) { > > + if (cc->bql_interrupt) { > > + qemu_mutex_unlock_iothread(); > > + } > > + cpu_mutex_lock(cpu); > > replay_interrupt(); > > /* > > * After processing the interrupt, ensure an EXCP_DEBUG is > > @@ -614,6 +628,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > cpu->exception_index = > > (cpu->singlestep_enabled ? EXCP_DEBUG : -1); > > *last_tb = NULL; > > + } else { > > + if (cc->bql_interrupt) { > > + qemu_mutex_unlock_iothread(); > > + } > > + cpu_mutex_lock(cpu); > > } > > /* The target hook may have updated the 'cpu->interrupt_request'; > > * reload the 'interrupt_request' value */ > > @@ -627,7 +646,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, > > } > > > > /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */ > > - qemu_mutex_unlock_iothread(); > > + cpu_mutex_unlock(cpu); > > } > > > > /* Finally, check if we need to exit to the main loop. */ > > @@ -691,7 +710,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, > > } > > #endif > > } > > - > > /* main execution loop */ > > > > int cpu_exec(CPUState *cpu) > > >
Yes, that is correct. It's more work but also more maintainable. Thanks, Paolo Il ven 31 lug 2020, 22:09 Robert Foley <robert.foley@linaro.org> ha scritto: > On Fri, 31 Jul 2020 at 14:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > On 31/07/20 14:51, Robert Foley wrote: > > > This change removes the implied BQL from the cpu_handle_interrupt, > > > and cpu_handle_exception paths. We can now select per-arch if > > > the BQL is needed or not by using the bql_interrupt flag. > > > By default, the core code holds the BQL. > > > One benefit of this change is that it leaves it up to the arch > > > to make the change to remove BQL when it makes sense. > > > > > > Signed-off-by: Robert Foley <robert.foley@linaro.org> > > > > No, please just modify all implementation to do lock/unlock. It's a > > simpler patch than this on. > > Sure, we will update the patch based on this. > > To clarify, the suggestion here is to remove the bql_interrupt flag > that we added and change all the per-arch interrupt callback code to > do the lock/unlock of the BQL? So for example change > x86_cpu_exec_interrupt, and arm_cpu_exec_interrupt, etc to lock/unlock BQL? > > Thanks, > -Rob > > > > > > Paolo > > > > > --- > > > accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++++-------- > > > 1 file changed, 26 insertions(+), 8 deletions(-) > > > > > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > > > index 80d0e649b2..cde27ee0bf 100644 > > > --- a/accel/tcg/cpu-exec.c > > > +++ b/accel/tcg/cpu-exec.c > > > @@ -517,9 +517,13 @@ static inline bool cpu_handle_exception(CPUState > *cpu, int *ret) > > > #else > > > if (replay_exception()) { > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > - qemu_mutex_lock_iothread(); > > > + if (cc->bql_interrupt) { > > > + qemu_mutex_lock_iothread(); > > > + } > > > cc->do_interrupt(cpu); > > > - qemu_mutex_unlock_iothread(); > > > + if (cc->bql_interrupt) { > > > + qemu_mutex_unlock_iothread(); > > > + } > > > cpu->exception_index = -1; > > > > > > if (unlikely(cpu->singlestep_enabled)) { > > > @@ -558,7 +562,7 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > if (unlikely(cpu_interrupt_request(cpu))) { > > > int interrupt_request; > > > > > > - qemu_mutex_lock_iothread(); > > > + cpu_mutex_lock(cpu); > > > interrupt_request = cpu_interrupt_request(cpu); > > > if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { > > > /* Mask out external interrupts for this step. */ > > > @@ -567,7 +571,7 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > if (interrupt_request & CPU_INTERRUPT_DEBUG) { > > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG); > > > cpu->exception_index = EXCP_DEBUG; > > > - qemu_mutex_unlock_iothread(); > > > + cpu_mutex_unlock(cpu); > > > return true; > > > } > > > if (replay_mode == REPLAY_MODE_PLAY && > !replay_has_interrupt()) { > > > @@ -577,13 +581,15 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT); > > > cpu_halted_set(cpu, 1); > > > cpu->exception_index = EXCP_HLT; > > > - qemu_mutex_unlock_iothread(); > > > + cpu_mutex_unlock(cpu); > > > return true; > > > } > > > #if defined(TARGET_I386) > > > else if (interrupt_request & CPU_INTERRUPT_INIT) { > > > X86CPU *x86_cpu = X86_CPU(cpu); > > > CPUArchState *env = &x86_cpu->env; > > > + cpu_mutex_unlock(cpu); > > > + qemu_mutex_lock_iothread(); > > > replay_interrupt(); > > > cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0); > > > do_cpu_init(x86_cpu); > > > @@ -595,7 +601,7 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > else if (interrupt_request & CPU_INTERRUPT_RESET) { > > > replay_interrupt(); > > > cpu_reset(cpu); > > > - qemu_mutex_unlock_iothread(); > > > + cpu_mutex_unlock(cpu); > > > return true; > > > } > > > #endif > > > @@ -604,7 +610,15 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > True when it is, and we should restart on a new TB, > > > and via longjmp via cpu_loop_exit. */ > > > else { > > > + cpu_mutex_unlock(cpu); > > > + if (cc->bql_interrupt) { > > > + qemu_mutex_lock_iothread(); > > > + } > > > if (cc->cpu_exec_interrupt(cpu, interrupt_request)) { > > > + if (cc->bql_interrupt) { > > > + qemu_mutex_unlock_iothread(); > > > + } > > > + cpu_mutex_lock(cpu); > > > replay_interrupt(); > > > /* > > > * After processing the interrupt, ensure an > EXCP_DEBUG is > > > @@ -614,6 +628,11 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > cpu->exception_index = > > > (cpu->singlestep_enabled ? EXCP_DEBUG : -1); > > > *last_tb = NULL; > > > + } else { > > > + if (cc->bql_interrupt) { > > > + qemu_mutex_unlock_iothread(); > > > + } > > > + cpu_mutex_lock(cpu); > > > } > > > /* The target hook may have updated the > 'cpu->interrupt_request'; > > > * reload the 'interrupt_request' value */ > > > @@ -627,7 +646,7 @@ static inline bool cpu_handle_interrupt(CPUState > *cpu, > > > } > > > > > > /* If we exit via cpu_loop_exit/longjmp it is reset in > cpu_exec */ > > > - qemu_mutex_unlock_iothread(); > > > + cpu_mutex_unlock(cpu); > > > } > > > > > > /* Finally, check if we need to exit to the main loop. */ > > > @@ -691,7 +710,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, > TranslationBlock *tb, > > > } > > > #endif > > > } > > > - > > > /* main execution loop */ > > > > > > int cpu_exec(CPUState *cpu) > > > > > > > <div dir="auto">Yes, that is correct. It's more work but also more maintainable.<div dir="auto"><br></div><div dir="auto">Thanks,</div><div dir="auto"><br></div><div dir="auto">Paolo</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il ven 31 lug 2020, 22:09 Robert Foley <<a href="mailto:robert.foley@linaro.org">robert.foley@linaro.org</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 31 Jul 2020 at 14:02, Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank" rel="noreferrer">pbonzini@redhat.com</a>> wrote:<br> ><br> > On 31/07/20 14:51, Robert Foley wrote:<br> > > This change removes the implied BQL from the cpu_handle_interrupt,<br> > > and cpu_handle_exception paths. We can now select per-arch if<br> > > the BQL is needed or not by using the bql_interrupt flag.<br> > > By default, the core code holds the BQL.<br> > > One benefit of this change is that it leaves it up to the arch<br> > > to make the change to remove BQL when it makes sense.<br> > ><br> > > Signed-off-by: Robert Foley <<a href="mailto:robert.foley@linaro.org" target="_blank" rel="noreferrer">robert.foley@linaro.org</a>><br> ><br> > No, please just modify all implementation to do lock/unlock. It's a<br> > simpler patch than this on.<br> <br> Sure, we will update the patch based on this.<br> <br> To clarify, the suggestion here is to remove the bql_interrupt flag<br> that we added and change all the per-arch interrupt callback code to<br> do the lock/unlock of the BQL? So for example change<br> x86_cpu_exec_interrupt, and arm_cpu_exec_interrupt, etc to lock/unlock BQL?<br> <br> Thanks,<br> -Rob<br> <br> <br> ><br> > Paolo<br> ><br> > > ---<br> > > accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++++--------<br> > > 1 file changed, 26 insertions(+), 8 deletions(-)<br> > ><br> > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c<br> > > index 80d0e649b2..cde27ee0bf 100644<br> > > --- a/accel/tcg/cpu-exec.c<br> > > +++ b/accel/tcg/cpu-exec.c<br> > > @@ -517,9 +517,13 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)<br> > > #else<br> > > if (replay_exception()) {<br> > > CPUClass *cc = CPU_GET_CLASS(cpu);<br> > > - qemu_mutex_lock_iothread();<br> > > + if (cc->bql_interrupt) {<br> > > + qemu_mutex_lock_iothread();<br> > > + }<br> > > cc->do_interrupt(cpu);<br> > > - qemu_mutex_unlock_iothread();<br> > > + if (cc->bql_interrupt) {<br> > > + qemu_mutex_unlock_iothread();<br> > > + }<br> > > cpu->exception_index = -1;<br> > ><br> > > if (unlikely(cpu->singlestep_enabled)) {<br> > > @@ -558,7 +562,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > if (unlikely(cpu_interrupt_request(cpu))) {<br> > > int interrupt_request;<br> > ><br> > > - qemu_mutex_lock_iothread();<br> > > + cpu_mutex_lock(cpu);<br> > > interrupt_request = cpu_interrupt_request(cpu);<br> > > if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {<br> > > /* Mask out external interrupts for this step. */<br> > > @@ -567,7 +571,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > if (interrupt_request & CPU_INTERRUPT_DEBUG) {<br> > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG);<br> > > cpu->exception_index = EXCP_DEBUG;<br> > > - qemu_mutex_unlock_iothread();<br> > > + cpu_mutex_unlock(cpu);<br> > > return true;<br> > > }<br> > > if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {<br> > > @@ -577,13 +581,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);<br> > > cpu_halted_set(cpu, 1);<br> > > cpu->exception_index = EXCP_HLT;<br> > > - qemu_mutex_unlock_iothread();<br> > > + cpu_mutex_unlock(cpu);<br> > > return true;<br> > > }<br> > > #if defined(TARGET_I386)<br> > > else if (interrupt_request & CPU_INTERRUPT_INIT) {<br> > > X86CPU *x86_cpu = X86_CPU(cpu);<br> > > CPUArchState *env = &x86_cpu->env;<br> > > + cpu_mutex_unlock(cpu);<br> > > + qemu_mutex_lock_iothread();<br> > > replay_interrupt();<br> > > cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);<br> > > do_cpu_init(x86_cpu);<br> > > @@ -595,7 +601,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > else if (interrupt_request & CPU_INTERRUPT_RESET) {<br> > > replay_interrupt();<br> > > cpu_reset(cpu);<br> > > - qemu_mutex_unlock_iothread();<br> > > + cpu_mutex_unlock(cpu);<br> > > return true;<br> > > }<br> > > #endif<br> > > @@ -604,7 +610,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > True when it is, and we should restart on a new TB,<br> > > and via longjmp via cpu_loop_exit. */<br> > > else {<br> > > + cpu_mutex_unlock(cpu);<br> > > + if (cc->bql_interrupt) {<br> > > + qemu_mutex_lock_iothread();<br> > > + }<br> > > if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {<br> > > + if (cc->bql_interrupt) {<br> > > + qemu_mutex_unlock_iothread();<br> > > + }<br> > > + cpu_mutex_lock(cpu);<br> > > replay_interrupt();<br> > > /*<br> > > * After processing the interrupt, ensure an EXCP_DEBUG is<br> > > @@ -614,6 +628,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > cpu->exception_index =<br> > > (cpu->singlestep_enabled ? EXCP_DEBUG : -1);<br> > > *last_tb = NULL;<br> > > + } else {<br> > > + if (cc->bql_interrupt) {<br> > > + qemu_mutex_unlock_iothread();<br> > > + }<br> > > + cpu_mutex_lock(cpu);<br> > > }<br> > > /* The target hook may have updated the 'cpu->interrupt_request';<br> > > * reload the 'interrupt_request' value */<br> > > @@ -627,7 +646,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,<br> > > }<br> > ><br> > > /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */<br> > > - qemu_mutex_unlock_iothread();<br> > > + cpu_mutex_unlock(cpu);<br> > > }<br> > ><br> > > /* Finally, check if we need to exit to the main loop. */<br> > > @@ -691,7 +710,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,<br> > > }<br> > > #endif<br> > > }<br> > > -<br> > > /* main execution loop */<br> > ><br> > > int cpu_exec(CPUState *cpu)<br> > ><br> ><br> <br> </blockquote></div>
Paolo Bonzini <pbonzini@redhat.com> writes: > Yes, that is correct. It's more work but also more maintainable. I originally suggested keeping the locking choice up in the main loop because I suspect most guests will stick to BQL IRQs until they find it is a bottle neck. cpu_handle_interrupt/exception have never been my favourite functions but perhaps there is a way to re-factor and clean them up to keep this in core code? I do worry that hiding BQL activity in the guest code makes it harder to reason about what locks are currently held when reading the code. > > Thanks, > > Paolo > > Il ven 31 lug 2020, 22:09 Robert Foley <robert.foley@linaro.org> ha scritto: > >> On Fri, 31 Jul 2020 at 14:02, Paolo Bonzini <pbonzini@redhat.com> wrote: >> > >> > On 31/07/20 14:51, Robert Foley wrote: >> > > This change removes the implied BQL from the cpu_handle_interrupt, >> > > and cpu_handle_exception paths. We can now select per-arch if >> > > the BQL is needed or not by using the bql_interrupt flag. >> > > By default, the core code holds the BQL. >> > > One benefit of this change is that it leaves it up to the arch >> > > to make the change to remove BQL when it makes sense. >> > > >> > > Signed-off-by: Robert Foley <robert.foley@linaro.org> >> > >> > No, please just modify all implementation to do lock/unlock. It's a >> > simpler patch than this on. >> >> Sure, we will update the patch based on this. >> >> To clarify, the suggestion here is to remove the bql_interrupt flag >> that we added and change all the per-arch interrupt callback code to >> do the lock/unlock of the BQL? So for example change >> x86_cpu_exec_interrupt, and arm_cpu_exec_interrupt, etc to lock/unlock BQL? >> >> Thanks, >> -Rob >> >> >> > >> > Paolo >> > >> > > --- >> > > accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++++-------- >> > > 1 file changed, 26 insertions(+), 8 deletions(-) >> > > >> > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >> > > index 80d0e649b2..cde27ee0bf 100644 >> > > --- a/accel/tcg/cpu-exec.c >> > > +++ b/accel/tcg/cpu-exec.c >> > > @@ -517,9 +517,13 @@ static inline bool cpu_handle_exception(CPUState >> *cpu, int *ret) >> > > #else >> > > if (replay_exception()) { >> > > CPUClass *cc = CPU_GET_CLASS(cpu); >> > > - qemu_mutex_lock_iothread(); >> > > + if (cc->bql_interrupt) { >> > > + qemu_mutex_lock_iothread(); >> > > + } >> > > cc->do_interrupt(cpu); >> > > - qemu_mutex_unlock_iothread(); >> > > + if (cc->bql_interrupt) { >> > > + qemu_mutex_unlock_iothread(); >> > > + } >> > > cpu->exception_index = -1; >> > > >> > > if (unlikely(cpu->singlestep_enabled)) { >> > > @@ -558,7 +562,7 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > if (unlikely(cpu_interrupt_request(cpu))) { >> > > int interrupt_request; >> > > >> > > - qemu_mutex_lock_iothread(); >> > > + cpu_mutex_lock(cpu); >> > > interrupt_request = cpu_interrupt_request(cpu); >> > > if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { >> > > /* Mask out external interrupts for this step. */ >> > > @@ -567,7 +571,7 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > if (interrupt_request & CPU_INTERRUPT_DEBUG) { >> > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG); >> > > cpu->exception_index = EXCP_DEBUG; >> > > - qemu_mutex_unlock_iothread(); >> > > + cpu_mutex_unlock(cpu); >> > > return true; >> > > } >> > > if (replay_mode == REPLAY_MODE_PLAY && >> !replay_has_interrupt()) { >> > > @@ -577,13 +581,15 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT); >> > > cpu_halted_set(cpu, 1); >> > > cpu->exception_index = EXCP_HLT; >> > > - qemu_mutex_unlock_iothread(); >> > > + cpu_mutex_unlock(cpu); >> > > return true; >> > > } >> > > #if defined(TARGET_I386) >> > > else if (interrupt_request & CPU_INTERRUPT_INIT) { >> > > X86CPU *x86_cpu = X86_CPU(cpu); >> > > CPUArchState *env = &x86_cpu->env; >> > > + cpu_mutex_unlock(cpu); >> > > + qemu_mutex_lock_iothread(); >> > > replay_interrupt(); >> > > cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0); >> > > do_cpu_init(x86_cpu); >> > > @@ -595,7 +601,7 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > else if (interrupt_request & CPU_INTERRUPT_RESET) { >> > > replay_interrupt(); >> > > cpu_reset(cpu); >> > > - qemu_mutex_unlock_iothread(); >> > > + cpu_mutex_unlock(cpu); >> > > return true; >> > > } >> > > #endif >> > > @@ -604,7 +610,15 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > True when it is, and we should restart on a new TB, >> > > and via longjmp via cpu_loop_exit. */ >> > > else { >> > > + cpu_mutex_unlock(cpu); >> > > + if (cc->bql_interrupt) { >> > > + qemu_mutex_lock_iothread(); >> > > + } >> > > if (cc->cpu_exec_interrupt(cpu, interrupt_request)) { >> > > + if (cc->bql_interrupt) { >> > > + qemu_mutex_unlock_iothread(); >> > > + } >> > > + cpu_mutex_lock(cpu); >> > > replay_interrupt(); >> > > /* >> > > * After processing the interrupt, ensure an >> EXCP_DEBUG is >> > > @@ -614,6 +628,11 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > cpu->exception_index = >> > > (cpu->singlestep_enabled ? EXCP_DEBUG : -1); >> > > *last_tb = NULL; >> > > + } else { >> > > + if (cc->bql_interrupt) { >> > > + qemu_mutex_unlock_iothread(); >> > > + } >> > > + cpu_mutex_lock(cpu); >> > > } >> > > /* The target hook may have updated the >> 'cpu->interrupt_request'; >> > > * reload the 'interrupt_request' value */ >> > > @@ -627,7 +646,7 @@ static inline bool cpu_handle_interrupt(CPUState >> *cpu, >> > > } >> > > >> > > /* If we exit via cpu_loop_exit/longjmp it is reset in >> cpu_exec */ >> > > - qemu_mutex_unlock_iothread(); >> > > + cpu_mutex_unlock(cpu); >> > > } >> > > >> > > /* Finally, check if we need to exit to the main loop. */ >> > > @@ -691,7 +710,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, >> TranslationBlock *tb, >> > > } >> > > #endif >> > > } >> > > - >> > > /* main execution loop */ >> > > >> > > int cpu_exec(CPUState *cpu) >> > > >> > >> >> -- Alex Bennée
On 02/08/20 18:09, Alex Bennée wrote: > I originally suggested keeping the locking choice up in the main loop > because I suspect most guests will stick to BQL IRQs until they find it > is a bottle neck. True, but the main loop is already complicated and conditional locking is pretty much impossible to reason on and (in the future) do static analysis on. Paolo
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index 80d0e649b2..cde27ee0bf 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -517,9 +517,13 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret) #else if (replay_exception()) { CPUClass *cc = CPU_GET_CLASS(cpu); - qemu_mutex_lock_iothread(); + if (cc->bql_interrupt) { + qemu_mutex_lock_iothread(); + } cc->do_interrupt(cpu); - qemu_mutex_unlock_iothread(); + if (cc->bql_interrupt) { + qemu_mutex_unlock_iothread(); + } cpu->exception_index = -1; if (unlikely(cpu->singlestep_enabled)) { @@ -558,7 +562,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, if (unlikely(cpu_interrupt_request(cpu))) { int interrupt_request; - qemu_mutex_lock_iothread(); + cpu_mutex_lock(cpu); interrupt_request = cpu_interrupt_request(cpu); if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) { /* Mask out external interrupts for this step. */ @@ -567,7 +571,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, if (interrupt_request & CPU_INTERRUPT_DEBUG) { cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG); cpu->exception_index = EXCP_DEBUG; - qemu_mutex_unlock_iothread(); + cpu_mutex_unlock(cpu); return true; } if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) { @@ -577,13 +581,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT); cpu_halted_set(cpu, 1); cpu->exception_index = EXCP_HLT; - qemu_mutex_unlock_iothread(); + cpu_mutex_unlock(cpu); return true; } #if defined(TARGET_I386) else if (interrupt_request & CPU_INTERRUPT_INIT) { X86CPU *x86_cpu = X86_CPU(cpu); CPUArchState *env = &x86_cpu->env; + cpu_mutex_unlock(cpu); + qemu_mutex_lock_iothread(); replay_interrupt(); cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0); do_cpu_init(x86_cpu); @@ -595,7 +601,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, else if (interrupt_request & CPU_INTERRUPT_RESET) { replay_interrupt(); cpu_reset(cpu); - qemu_mutex_unlock_iothread(); + cpu_mutex_unlock(cpu); return true; } #endif @@ -604,7 +610,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, True when it is, and we should restart on a new TB, and via longjmp via cpu_loop_exit. */ else { + cpu_mutex_unlock(cpu); + if (cc->bql_interrupt) { + qemu_mutex_lock_iothread(); + } if (cc->cpu_exec_interrupt(cpu, interrupt_request)) { + if (cc->bql_interrupt) { + qemu_mutex_unlock_iothread(); + } + cpu_mutex_lock(cpu); replay_interrupt(); /* * After processing the interrupt, ensure an EXCP_DEBUG is @@ -614,6 +628,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, cpu->exception_index = (cpu->singlestep_enabled ? EXCP_DEBUG : -1); *last_tb = NULL; + } else { + if (cc->bql_interrupt) { + qemu_mutex_unlock_iothread(); + } + cpu_mutex_lock(cpu); } /* The target hook may have updated the 'cpu->interrupt_request'; * reload the 'interrupt_request' value */ @@ -627,7 +646,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, } /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */ - qemu_mutex_unlock_iothread(); + cpu_mutex_unlock(cpu); } /* Finally, check if we need to exit to the main loop. */ @@ -691,7 +710,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, } #endif } - /* main execution loop */ int cpu_exec(CPUState *cpu)
This change removes the implied BQL from the cpu_handle_interrupt, and cpu_handle_exception paths. We can now select per-arch if the BQL is needed or not by using the bql_interrupt flag. By default, the core code holds the BQL. One benefit of this change is that it leaves it up to the arch to make the change to remove BQL when it makes sense. Signed-off-by: Robert Foley <robert.foley@linaro.org> --- accel/tcg/cpu-exec.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) -- 2.17.1