[Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Alvise Rigo]

As in the case of tlb_flush(), also tlb_flush_by_mmuidx has to query the
TLB flush if it targets another VCPU. To accomplish this, a new async
work has been added, together with a new TLBFlushByMMUIdxParams. A
bitmap is used to track the MMU indexes to flush.

This patch applies to the multi_tcg_v8 branch.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 cputlb.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 12 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 29252d1..1eeeccb 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -103,9 +103,11 @@ void tlb_flush(CPUState *cpu, int flush_global)
     }
 }
 
-static inline void v_tlb_flush_by_mmuidx(CPUState *cpu, va_list argp)
+/* Flush tlb_table[] and tlb_v_table[] of @cpu at MMU indexes given by @bitmap.
+ * Flush also tb_jmp_cache. */
+static inline void tlb_tables_flush_bitmap(CPUState *cpu, unsigned long *bitmap)
 {
-    CPUArchState *env = cpu->env_ptr;
+    int mmu_idx;
 
 #if defined(DEBUG_TLB)
     printf("tlb_flush_by_mmuidx:");
@@ -114,6 +116,41 @@ static inline void v_tlb_flush_by_mmuidx(CPUState *cpu, va_list argp)
        links while we are modifying them */
     cpu->current_tb = NULL;
 
+    for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
+        if (test_bit(mmu_idx, bitmap)) {
+            CPUArchState *env = cpu->env_ptr;
+#if defined(DEBUG_TLB)
+            printf(" %d", mmu_idx);
+#endif
+            memset(env->tlb_table[mmu_idx], -1, sizeof(env->tlb_table[0]));
+            memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
+        }
+    }
+    memset(cpu->tb_jmp_cache, 0, sizeof(cpu->tb_jmp_cache));
+
+#if defined(DEBUG_TLB)
+    printf("\n");
+#endif
+}
+
+struct TLBFlushByMMUIdxParams {
+    CPUState *cpu;
+    DECLARE_BITMAP(idx_to_flush, NB_MMU_MODES);
+};
+
+static void tlb_flush_by_mmuidx_async_work(void *opaque)
+{
+    struct TLBFlushByMMUIdxParams *params = opaque;
+
+    tlb_tables_flush_bitmap(params->cpu, params->idx_to_flush);
+
+    g_free(params);
+}
+
+static inline void v_tlb_flush_by_mmuidx(CPUState *cpu, va_list argp)
+{
+    DECLARE_BITMAP(idxmap, NB_MMU_MODES) = { 0 };
+
     for (;;) {
         int mmu_idx = va_arg(argp, int);
 
@@ -121,19 +158,23 @@ static inline void v_tlb_flush_by_mmuidx(CPUState *cpu, va_list argp)
             break;
         }
 
-#if defined(DEBUG_TLB)
-        printf(" %d", mmu_idx);
-#endif
-
-        memset(env->tlb_table[mmu_idx], -1, sizeof(env->tlb_table[0]));
-        memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
+        set_bit(mmu_idx, idxmap);
     }
 
-#if defined(DEBUG_TLB)
-    printf("\n");
-#endif
+    if (!qemu_cpu_is_self(cpu)) {
+        /* We do not set the pendind_tlb_flush bit, only a global flush
+         * does that. */
+        if (!atomic_read(&cpu->pending_tlb_flush)) {
+             struct TLBFlushByMMUIdxParams *params;
 
-    memset(cpu->tb_jmp_cache, 0, sizeof(cpu->tb_jmp_cache));
+             params = g_malloc(sizeof(struct TLBFlushByMMUIdxParams));
+             params->cpu = cpu;
+             memcpy(params->idx_to_flush, idxmap, sizeof(idxmap));
+             async_run_on_cpu(cpu, tlb_flush_by_mmuidx_async_work, params);
+         }
+    } else {
+        tlb_tables_flush_bitmap(cpu, idxmap);
+    }
 }
 
 void tlb_flush_by_mmuidx(CPUState *cpu, ...)
-- 
2.7.2

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Peter Maydell]

On 29 February 2016 at 13:16, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> As in the case of tlb_flush(), also tlb_flush_by_mmuidx has to query the
> TLB flush if it targets another VCPU. To accomplish this, a new async
> work has been added, together with a new TLBFlushByMMUIdxParams. A
> bitmap is used to track the MMU indexes to flush.
>
> This patch applies to the multi_tcg_v8 branch.

What's the API for a target CPU emulation to say "and now I must
wait for the TLB op to finish" before completing this guest
instruction?

thanks
-- PMM

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Paolo Bonzini]

On 29/02/2016 14:21, Peter Maydell wrote:
> On 29 February 2016 at 13:16, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
>> > As in the case of tlb_flush(), also tlb_flush_by_mmuidx has to query the
>> > TLB flush if it targets another VCPU. To accomplish this, a new async
>> > work has been added, together with a new TLBFlushByMMUIdxParams. A
>> > bitmap is used to track the MMU indexes to flush.
>> >
>> > This patch applies to the multi_tcg_v8 branch.
> What's the API for a target CPU emulation to say "and now I must
> wait for the TLB op to finish" before completing this guest
> instruction?

My proposal has been for a while for DMB to put the CPU in a halted
state (remote TLB callbacks then can decrement a counter and signal
cpu_halt_cond when it's zero), but no one has implemented this.

Paolo

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Peter Maydell]

On 29 February 2016 at 13:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 29/02/2016 14:21, Peter Maydell wrote:
>> On 29 February 2016 at 13:16, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
>>> > As in the case of tlb_flush(), also tlb_flush_by_mmuidx has to query the
>>> > TLB flush if it targets another VCPU. To accomplish this, a new async
>>> > work has been added, together with a new TLBFlushByMMUIdxParams. A
>>> > bitmap is used to track the MMU indexes to flush.
>>> >
>>> > This patch applies to the multi_tcg_v8 branch.
>> What's the API for a target CPU emulation to say "and now I must
>> wait for the TLB op to finish" before completing this guest
>> instruction?
>
> My proposal has been for a while for DMB to put the CPU in a halted
> state (remote TLB callbacks then can decrement a counter and signal
> cpu_halt_cond when it's zero), but no one has implemented this.

Yeah, that's the other approach -- really split the things that can
be async and do real "wait for completion" at points which must
synchronize. (Needs a little care since DMB is not the only such point.)
An initial implementation that does an immediate wait-for-completion
is probably simpler to review though, and add the real asynchrony
later. And either way you need an API for the target to wait for
completion.

thanks
-- PMM

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[alvise rigo]

On Mon, Feb 29, 2016 at 2:55 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 29 February 2016 at 13:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 29/02/2016 14:21, Peter Maydell wrote:
>>> On 29 February 2016 at 13:16, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
>>>> > As in the case of tlb_flush(), also tlb_flush_by_mmuidx has to query the
>>>> > TLB flush if it targets another VCPU. To accomplish this, a new async
>>>> > work has been added, together with a new TLBFlushByMMUIdxParams. A
>>>> > bitmap is used to track the MMU indexes to flush.
>>>> >
>>>> > This patch applies to the multi_tcg_v8 branch.
>>> What's the API for a target CPU emulation to say "and now I must
>>> wait for the TLB op to finish" before completing this guest
>>> instruction?
>>
>> My proposal has been for a while for DMB to put the CPU in a halted
>> state (remote TLB callbacks then can decrement a counter and signal
>> cpu_halt_cond when it's zero), but no one has implemented this.
>
> Yeah, that's the other approach -- really split the things that can
> be async and do real "wait for completion" at points which must
> synchronize. (Needs a little care since DMB is not the only such point.)
> An initial implementation that does an immediate wait-for-completion
> is probably simpler to review though, and add the real asynchrony
> later. And either way you need an API for the target to wait for
> completion.

OK, so basically being sure that the target CPU performs the flush
before executing the next TB is not enough. We need a sort of feedback
that the flush has been done before emulating the next guest
instruction. Did I get it right?

Thank you,
alvise

>
> thanks
> -- PMM

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Paolo Bonzini]

On 29/02/2016 15:02, alvise rigo wrote:
> > Yeah, that's the other approach -- really split the things that can
> > be async and do real "wait for completion" at points which must
> > synchronize. (Needs a little care since DMB is not the only such point.)
> > An initial implementation that does an immediate wait-for-completion
> > is probably simpler to review though, and add the real asynchrony
> > later. And either way you need an API for the target to wait for
> > completion.
> OK, so basically being sure that the target CPU performs the flush
> before executing the next TB is not enough. We need a sort of feedback
> that the flush has been done before emulating the next guest
> instruction. Did I get it right?

That risks getting deadlocks if CPU A asks B to flush the TLB and vice
versa.  Using a halted state means that the VCPU thread goes through the
cpus.c loop and can for example service other CPUs' TLB flush requests.

Paolo

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[alvise rigo]

I see the risk. I will come back with something and let you know.

Thank you,
alvise

On Mon, Feb 29, 2016 at 3:06 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 29/02/2016 15:02, alvise rigo wrote:
>> > Yeah, that's the other approach -- really split the things that can
>> > be async and do real "wait for completion" at points which must
>> > synchronize. (Needs a little care since DMB is not the only such point.)
>> > An initial implementation that does an immediate wait-for-completion
>> > is probably simpler to review though, and add the real asynchrony
>> > later. And either way you need an API for the target to wait for
>> > completion.
>> OK, so basically being sure that the target CPU performs the flush
>> before executing the next TB is not enough. We need a sort of feedback
>> that the flush has been done before emulating the next guest
>> instruction. Did I get it right?
>
> That risks getting deadlocks if CPU A asks B to flush the TLB and vice
> versa.  Using a halted state means that the VCPU thread goes through the
> cpus.c loop and can for example service other CPUs' TLB flush requests.
>
> Paolo

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[alvise rigo]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

A small update on this. I have a working implementation of the "halted
state" mechanism for waiting all the pending flushes to be completed.
However, the way I'm going back to the cpus.c loop (the while(1) in
qemu_tcg_cpu_thread_fn) is a bit convoluted. In the case of the TLB ops
that always end the TB, a simple cpu_exit() allows me to go back to the
main loop. I think in this case we can also use the cpu_loop_exit(), though
making the code a bit more complicated since the PC would require some
adjustments.

I wanted then to apply the same "halted state" to the LoadLink helper,
since also this one might cause some flush requests. In this case, we can
not just call cpu_loop_exit() in that the guest code would miss the
returned value. Forcing the LDREX instruction to also end the TB through an
empty 'is_jmp' condition did the trick allowing once again to use
cpu_exit(). Is there another better solution?

Thank you,
alvise

On Mon, Feb 29, 2016 at 3:18 PM, alvise rigo <a.rigo@virtualopensystems.com>
wrote:

> I see the risk. I will come back with something and let you know.
>
> Thank you,
> alvise
>
> On Mon, Feb 29, 2016 at 3:06 PM, Paolo Bonzini <pbonzini@redhat.com>
> wrote:
> >
> >
> > On 29/02/2016 15:02, alvise rigo wrote:
> >> > Yeah, that's the other approach -- really split the things that can
> >> > be async and do real "wait for completion" at points which must
> >> > synchronize. (Needs a little care since DMB is not the only such
> point.)
> >> > An initial implementation that does an immediate wait-for-completion
> >> > is probably simpler to review though, and add the real asynchrony
> >> > later. And either way you need an API for the target to wait for
> >> > completion.
> >> OK, so basically being sure that the target CPU performs the flush
> >> before executing the next TB is not enough. We need a sort of feedback
> >> that the flush has been done before emulating the next guest
> >> instruction. Did I get it right?
> >
> > That risks getting deadlocks if CPU A asks B to flush the TLB and vice
> > versa.  Using a halted state means that the VCPU thread goes through the
> > cpus.c loop and can for example service other CPUs' TLB flush requests.
> >
> > Paolo
>

[-- Attachment #2: Type: text/html, Size: 2915 bytes --]

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Alex Bennée]

alvise rigo <a.rigo@virtualopensystems.com> writes:

> A small update on this. I have a working implementation of the "halted
> state" mechanism for waiting all the pending flushes to be completed.
> However, the way I'm going back to the cpus.c loop (the while(1) in
> qemu_tcg_cpu_thread_fn) is a bit convoluted. In the case of the TLB ops
> that always end the TB, a simple cpu_exit() allows me to go back to the
> main loop. I think in this case we can also use the cpu_loop_exit(), though
> making the code a bit more complicated since the PC would require some
> adjustments.
>
> I wanted then to apply the same "halted state" to the LoadLink helper,
> since also this one might cause some flush requests. In this case, we can
> not just call cpu_loop_exit() in that the guest code would miss the
> returned value. Forcing the LDREX instruction to also end the TB through an
> empty 'is_jmp' condition did the trick allowing once again to use
> cpu_exit(). Is there another better solution?

Have you looked at Emilio's tree where he replaced the async_safe_work
mechanism with a mechanism to do the work in the vCPU run loop but halt
all other vCPUs first?

>
> Thank you,
> alvise
>
> On Mon, Feb 29, 2016 at 3:18 PM, alvise rigo <a.rigo@virtualopensystems.com>
> wrote:
>
>> I see the risk. I will come back with something and let you know.
>>
>> Thank you,
>> alvise
>>
>> On Mon, Feb 29, 2016 at 3:06 PM, Paolo Bonzini <pbonzini@redhat.com>
>> wrote:
>> >
>> >
>> > On 29/02/2016 15:02, alvise rigo wrote:
>> >> > Yeah, that's the other approach -- really split the things that can
>> >> > be async and do real "wait for completion" at points which must
>> >> > synchronize. (Needs a little care since DMB is not the only such
>> point.)
>> >> > An initial implementation that does an immediate wait-for-completion
>> >> > is probably simpler to review though, and add the real asynchrony
>> >> > later. And either way you need an API for the target to wait for
>> >> > completion.
>> >> OK, so basically being sure that the target CPU performs the flush
>> >> before executing the next TB is not enough. We need a sort of feedback
>> >> that the flush has been done before emulating the next guest
>> >> instruction. Did I get it right?
>> >
>> > That risks getting deadlocks if CPU A asks B to flush the TLB and vice
>> > versa.  Using a halted state means that the VCPU thread goes through the
>> > cpus.c loop and can for example service other CPUs' TLB flush requests.
>> >
>> > Paolo
>>


--
Alex Bennée

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[Paolo Bonzini]

On 04/03/2016 15:28, alvise rigo wrote:
> A small update on this. I have a working implementation of the "halted
> state" mechanism for waiting all the pending flushes to be completed.
> However, the way I'm going back to the cpus.c loop (the while(1) in
> qemu_tcg_cpu_thread_fn) is a bit convoluted. In the case of the TLB ops
> that always end the TB, a simple cpu_exit() allows me to go back to the
> main loop. I think in this case we can also use the cpu_loop_exit(),
> though making the code a bit more complicated since the PC would require
> some adjustments.

I think in both cases the function to use is cpu_loop_exit_restore.  It
will restart execution of the current instruction so it should be fine
as long as you don't call it unconditionally.

If you're not calling it directly from the helper, you need to save
GETPC() in the helper and propagate it down to the call site.  Then the
call site can use it as the last argument.  For an example see
helper_ljmp_protected's call to switch_tss_ra in target-i386/seg_helper.c.

> I wanted then to apply the same "halted state" to the LoadLink helper,
> since also this one might cause some flush requests.

Interesting, where is this documented in the ARM ARM?

> In this case, we
> can not just call cpu_loop_exit() in that the guest code would miss the
> returned value. Forcing the LDREX instruction to also end the TB through
> an empty 'is_jmp' condition did the trick allowing once again to use
> cpu_exit(). Is there another better solution?

Perhaps cpu_loop_exit_restore()?

Paolo

Re: [Qemu-devel] [mttcg] cputlb: Use async tlb_flush_by_mmuidx

[alvise rigo]

Hi Paolo,

On Mon, Mar 7, 2016 at 10:18 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 04/03/2016 15:28, alvise rigo wrote:
>> A small update on this. I have a working implementation of the "halted
>> state" mechanism for waiting all the pending flushes to be completed.
>> However, the way I'm going back to the cpus.c loop (the while(1) in
>> qemu_tcg_cpu_thread_fn) is a bit convoluted. In the case of the TLB ops
>> that always end the TB, a simple cpu_exit() allows me to go back to the
>> main loop. I think in this case we can also use the cpu_loop_exit(),
>> though making the code a bit more complicated since the PC would require
>> some adjustments.
>
> I think in both cases the function to use is cpu_loop_exit_restore.  It
> will restart execution of the current instruction so it should be fine
> as long as you don't call it unconditionally.

Indeed, cpu_loop_exit_restore() works just fine for those helpers that
do not return any value, thank you.

>
> If you're not calling it directly from the helper, you need to save
> GETPC() in the helper and propagate it down to the call site.  Then the
> call site can use it as the last argument.  For an example see
> helper_ljmp_protected's call to switch_tss_ra in target-i386/seg_helper.c.
>
>> I wanted then to apply the same "halted state" to the LoadLink helper,
>> since also this one might cause some flush requests.
>
> Interesting, where is this documented in the ARM ARM?

I'm referring to the usual flush requests that a LL(x) operation might
issue in order to have all the VCPUs agreeing on "x is an exclusive
address". Adding the halted state we ensure that the calling VCPU
resumes its execution after all the other VCPUs have set the TLB_EXCL
flag (this should also fix the race condition you were worried about).

>
>> In this case, we
>> can not just call cpu_loop_exit() in that the guest code would miss the
>> returned value. Forcing the LDREX instruction to also end the TB through
>> an empty 'is_jmp' condition did the trick allowing once again to use
>> cpu_exit(). Is there another better solution?
>
> Perhaps cpu_loop_exit_restore()?

For some reason this is not working to exit from helper_ldlink_name in
softmmu_llsc_template.h (the method returns a "WORD_TYPE"). The
execution in the guest is brought to an infinite loop, most likely
because of a deadlock due to an improper emulation of LDREX and STREX.
In any case the cpu_exit() solution still works with the downside of a
slightly bigger overhead in exiting/entering the TB.

Thank you,
alvise

>
> Paolo