[RFC,v1,09/12] translate-all: introduces tb_flush_safe.

Commit Message

Alex Bennée April 15, 2016, 2:23 p.m. UTC

From: KONRAD Frederic <[e-mail]>

tb_flush is not thread safe we definitely need to exit VCPUs to do that.
This introduces tb_flush_safe which just creates an async safe work which will
do a tb_flush later. This is called when we run out of space.

Signed-off-by: KONRAD Frederic <[e-mail]>
[AJB: merge the various tb_flush commits]
Signed-off-by: Alex Bennée <[e-mail]>

---
v1:
  - add more comments
  - fix build for linux-user
  - add log warning on linux-user
---
 include/exec/exec-all.h |  1 +
 translate-all.c         | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)

Comments

Sergey Fedorov June 5, 2016, 4:48 p.m. UTC | #1

On 15/04/16 17:23, Alex Bennée wrote:
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index f695577..858055b 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -307,6 +307,7 @@ struct TBContext {
>  
>  void tb_free(TranslationBlock *tb);
>  void tb_flush(CPUState *cpu);
> +void tb_flush_safe(CPUState *cpu);

Do we really want to have both tb_flush_safe() and tb_flush()?

Kind regards,
Sergey

>  void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
>  
>  #if defined(USE_DIRECT_JUMP)

Alex Bennée June 6, 2016, 8:54 a.m. UTC | #2

Sergey Fedorov <[e-mail]> writes:

> On 15/04/16 17:23, Alex Bennée wrote:
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index f695577..858055b 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -307,6 +307,7 @@ struct TBContext {
>>
>>  void tb_free(TranslationBlock *tb);
>>  void tb_flush(CPUState *cpu);
>> +void tb_flush_safe(CPUState *cpu);
>
> Do we really want to have both tb_flush_safe() and tb_flush()?

I guess if we are going to include user-mode in the party ;-)

>
> Kind regards,
> Sergey
>
>>  void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
>>
>>  #if defined(USE_DIRECT_JUMP)


--
Alex Bennée

Sergey Fedorov June 6, 2016, 10:06 a.m. UTC | #3

On 06/06/16 11:54, Alex Bennée wrote:
> Sergey Fedorov <[e-mail]> writes:
>
>> On 15/04/16 17:23, Alex Bennée wrote:
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index f695577..858055b 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -307,6 +307,7 @@ struct TBContext {
>>>
>>>  void tb_free(TranslationBlock *tb);
>>>  void tb_flush(CPUState *cpu);
>>> +void tb_flush_safe(CPUState *cpu);
>> Do we really want to have both tb_flush_safe() and tb_flush()?
> I guess if we are going to include user-mode in the party ;-)
>
>

User-mode should be fine; we take care of it:

+void tb_flush_safe(CPUState *cpu)
+{
+#ifdef CONFIG_SOFTMMU
+    async_safe_run_on_cpu(cpu, tb_flush_work, NULL);
+#else
+    qemu_log("Safe flushing of TBs not implemented for linux-user\n");
+    tb_flush(cpu);
+#endif
+}
+

Kind regards,
Sergey

Patch

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index f695577..858055b 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -307,6 +307,7 @@  struct TBContext {
 
 void tb_free(TranslationBlock *tb);
 void tb_flush(CPUState *cpu);
+void tb_flush_safe(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
 
 #if defined(USE_DIRECT_JUMP)
diff --git a/translate-all.c b/translate-all.c
index 8e70583..874dc56 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -825,9 +825,30 @@  static void page_flush_tb(void)
     }
 }
 
-/* flush all the translation blocks */
-/* XXX: tb_flush is currently not thread safe.  System emulation calls it only
- * with tb_lock taken or from safe_work, so no need to take tb_lock here.
+#ifdef CONFIG_SOFTMMU
+static void tb_flush_work(CPUState *cpu, void *opaque)
+{
+    tb_flush(cpu);
+}
+#endif
+
+void tb_flush_safe(CPUState *cpu)
+{
+#ifdef CONFIG_SOFTMMU
+    async_safe_run_on_cpu(cpu, tb_flush_work, NULL);
+#else
+    qemu_log("Safe flushing of TBs not implemented for linux-user\n");
+    tb_flush(cpu);
+#endif
+}
+
+/* Flush *all* translations
+ *
+ * This invalidates all translations, lookups and caches.
+ *
+ * This is not thread save for linux-user. For softmmu targets the
+ * flushing is done when all vCPUs are quiescent via the
+ * async_safe_work mechanism
  */
 void tb_flush(CPUState *cpu)
 {
@@ -1183,10 +1204,9 @@  TranslationBlock *tb_gen_code(CPUState *cpu,
     if (unlikely(!tb)) {
  buffer_overflow:
         /* flush must be done */
-        tb_flush(cpu);
-        /* cannot fail at this point */
-        tb = tb_alloc(pc);
-        assert(tb != NULL);
+        tb_flush_safe(cpu);
+        tb_unlock();
+        cpu_loop_exit(cpu);
     }
 
     gen_code_buf = tcg_ctx.code_gen_ptr;