[Qemu-devel] [PATCH v2 0/3] tcg: Misc clean-up patches from Paolo and Alex

[Qemu-devel] [PATCH v2 0/3] tcg: Misc clean-up patches from Paolo and Alex

[Sergey Fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

This patch series consists of various general TCG clean-up patches extracted
from Paolo's MTTCG tree [1] and Alex's MTTCG base enablement tree [2]. The idea
is to review and merge these patches separately from the MTTCG series to cut
the latter and make it easier to review.

The series' tree can be found in a public git repository [3].

[1] https://github.com/bonzini/qemu/tree/mttcg
[2] https://github.com/stsquad/qemu/tree/mttcg/base-patches-v2
[3] https://github.com/sergefdrv/qemu/tree/tcg-cleanup-v2

Summary of changes:
 v2:
  * Complete code_bitmap elimination [PATCH v2 1/3]
  * Take Alex's version of tb_find_physical() reorganization [PATCH v2 2/3]
  * Drop [PATCH 3/5] completely
  * Drop [PATCH 4/5] and [PATCH 5/5] for separate series
  * Take Alex's rebase of Paolo's icount code eliding [PATCH v2 3/3]

Alex Bennée (1):
  tcg: reorganize tb_find_physical loop

Paolo Bonzini (2):
  tcg: code_bitmap is not used by user-mode emulation
  cpu-exec: elide more icount code if CONFIG_USER_ONLY

 cpu-exec.c      | 52 ++++++++++++++++++++++++++++++++--------------------
 translate-all.c | 11 ++++++++---
 2 files changed, 40 insertions(+), 23 deletions(-)

-- 
2.7.3

[Qemu-devel] [PATCH v2 1/3] tcg: code_bitmap is not used by user-mode emulation

[Sergey Fedorov]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[Sergey Fedorov: eliminate the field entirely in user-mode]
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---

Notes:
    Changes in v2:
     * The field is eliminated entirely in user-mode

 translate-all.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index e9f409b762ab..c131ce2f183e 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -75,8 +75,9 @@ typedef struct PageDesc {
     /* in order to optimize self modifying code, we count the number
        of lookups we do to a given page to use a bitmap */
     unsigned int code_write_count;
+#ifdef CONFIG_SOFTMMU
     unsigned long *code_bitmap;
-#if defined(CONFIG_USER_ONLY)
+#else
     unsigned long flags;
 #endif
 } PageDesc;
@@ -784,8 +785,10 @@ void tb_free(TranslationBlock *tb)
 
 static inline void invalidate_page_bitmap(PageDesc *p)
 {
+#ifdef CONFIG_SOFTMMU
     g_free(p->code_bitmap);
     p->code_bitmap = NULL;
+#endif
     p->code_write_count = 0;
 }
 
@@ -1018,6 +1021,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
 }
 
+#ifdef CONFIG_SOFTMMU
 static void build_page_bitmap(PageDesc *p)
 {
     int n, tb_start, tb_end;
@@ -1046,6 +1050,7 @@ static void build_page_bitmap(PageDesc *p)
         tb = tb->page_next[n];
     }
 }
+#endif
 
 /* Called with mmap_lock held for user mode emulation.  */
 TranslationBlock *tb_gen_code(CPUState *cpu,
@@ -1294,6 +1299,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
 #endif
 }
 
+#ifdef CONFIG_SOFTMMU
 /* len must be <= 8 and start must be a multiple of len */
 void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
 {
@@ -1331,8 +1337,7 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
         tb_invalidate_phys_page_range(start, start + len, 1);
     }
 }
-
-#if !defined(CONFIG_SOFTMMU)
+#else
 /* Called with mmap_lock held.  */
 static void tb_invalidate_phys_page(tb_page_addr_t addr,
                                     uintptr_t pc, void *puc,
-- 
2.7.3

[Qemu-devel] [PATCH v2 2/3] tcg: reorganize tb_find_physical loop

[Sergey Fedorov]

From: Alex Bennée <alex.bennee@linaro.org>

Put some comments and improve code structure. This should help reading
the code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
[Sergey Fedorov: provide commit message; bring back resetting of
tb_invalidated_flag]
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 cpu-exec.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index fd92452f16f6..44116f180859 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -214,10 +214,9 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
                                           uint64_t flags)
 {
     CPUArchState *env = (CPUArchState *)cpu->env_ptr;
-    TranslationBlock *tb, **ptb1;
+    TranslationBlock *tb, **tb_hash_head, **ptb1;
     unsigned int h;
     tb_page_addr_t phys_pc, phys_page1;
-    target_ulong virt_page2;
 
     tcg_ctx.tb_ctx.tb_invalidated_flag = 0;
 
@@ -225,37 +224,42 @@ static TranslationBlock *tb_find_physical(CPUState *cpu,
     phys_pc = get_page_addr_code(env, pc);
     phys_page1 = phys_pc & TARGET_PAGE_MASK;
     h = tb_phys_hash_func(phys_pc);
-    ptb1 = &tcg_ctx.tb_ctx.tb_phys_hash[h];
-    for(;;) {
-        tb = *ptb1;
-        if (!tb) {
-            return NULL;
-        }
+
+    /* Start at head of the hash entry */
+    ptb1 = tb_hash_head = &tcg_ctx.tb_ctx.tb_phys_hash[h];
+    tb = *ptb1;
+
+    while (tb) {
         if (tb->pc == pc &&
             tb->page_addr[0] == phys_page1 &&
             tb->cs_base == cs_base &&
             tb->flags == flags) {
-            /* check next page if needed */
-            if (tb->page_addr[1] != -1) {
-                tb_page_addr_t phys_page2;
 
-                virt_page2 = (pc & TARGET_PAGE_MASK) +
-                    TARGET_PAGE_SIZE;
-                phys_page2 = get_page_addr_code(env, virt_page2);
+            if (tb->page_addr[1] == -1) {
+                /* done, we have a match */
+                break;
+            } else {
+                /* check next page if needed */
+                target_ulong virt_page2 = (pc & TARGET_PAGE_MASK) +
+                                          TARGET_PAGE_SIZE;
+                tb_page_addr_t phys_page2 = get_page_addr_code(env, virt_page2);
+
                 if (tb->page_addr[1] == phys_page2) {
                     break;
                 }
-            } else {
-                break;
             }
         }
+
         ptb1 = &tb->phys_hash_next;
+        tb = *ptb1;
     }
 
-    /* Move the TB to the head of the list */
-    *ptb1 = tb->phys_hash_next;
-    tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h];
-    tcg_ctx.tb_ctx.tb_phys_hash[h] = tb;
+    if (tb) {
+        /* Move the TB to the head of the list */
+        *ptb1 = tb->phys_hash_next;
+        tb->phys_hash_next = *tb_hash_head;
+        *tb_hash_head = tb;
+    }
     return tb;
 }
 
-- 
2.7.3

[Qemu-devel] [PATCH v2 3/3] cpu-exec: elide more icount code if CONFIG_USER_ONLY

[Sergey Fedorov]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[Alex Bennée: #ifndef replay code to match elided functions]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 cpu-exec.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/cpu-exec.c b/cpu-exec.c
index 44116f180859..5d1b4c90a687 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -183,6 +183,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t *tb_ptr)
     return next_tb;
 }
 
+#ifndef CONFIG_USER_ONLY
 /* Execute the code without caching the generated code. An interpreter
    could be used if available. */
 static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
@@ -207,6 +208,7 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
     tb_phys_invalidate(tb, -1);
     tb_free(tb);
 }
+#endif
 
 static TranslationBlock *tb_find_physical(CPUState *cpu,
                                           target_ulong pc,
@@ -422,12 +424,14 @@ int cpu_exec(CPUState *cpu)
                     }
 #endif
                 }
+#ifndef CONFIG_USER_ONLY
             } else if (replay_has_exception()
                        && cpu->icount_decr.u16.low + cpu->icount_extra == 0) {
                 /* try to cause an exception pending in the log */
                 cpu_exec_nocache(cpu, 1, tb_find_fast(cpu), true);
                 ret = -1;
                 break;
+#endif
             }
 
             next_tb = 0; /* force lookup of first TB */
@@ -542,6 +546,9 @@ int cpu_exec(CPUState *cpu)
                     case TB_EXIT_ICOUNT_EXPIRED:
                     {
                         /* Instruction counter expired.  */
+#ifdef CONFIG_USER_ONLY
+                        abort();
+#else
                         int insns_left = cpu->icount_decr.u32;
                         if (cpu->icount_extra && insns_left >= 0) {
                             /* Refill decrementer and continue execution.  */
@@ -561,6 +568,7 @@ int cpu_exec(CPUState *cpu)
                             cpu_loop_exit(cpu);
                         }
                         break;
+#endif
                     }
                     default:
                         break;
-- 
2.7.3

Re: [Qemu-devel] [PATCH v2 1/3] tcg: code_bitmap is not used by user-mode emulation

[Richard Henderson]

On 03/29/2016 12:48 PM, Sergey Fedorov wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [Sergey Fedorov: eliminate the field entirely in user-mode]
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
> 
> Notes:
>     Changes in v2:
>      * The field is eliminated entirely in user-mode

Reviewed-by: Richard Henderson  <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH v2 2/3] tcg: reorganize tb_find_physical loop

[Richard Henderson]

On 03/29/2016 12:48 PM, Sergey Fedorov wrote:
> +    while (tb) {
>          if (tb->pc == pc &&
>              tb->page_addr[0] == phys_page1 &&
>              tb->cs_base == cs_base &&
>              tb->flags == flags) {
> -            /* check next page if needed */
> -            if (tb->page_addr[1] != -1) {
> -                tb_page_addr_t phys_page2;
>  
> -                virt_page2 = (pc & TARGET_PAGE_MASK) +
> -                    TARGET_PAGE_SIZE;
> -                phys_page2 = get_page_addr_code(env, virt_page2);
> +            if (tb->page_addr[1] == -1) {
> +                /* done, we have a match */
> +                break;
> +            } else {
> +                /* check next page if needed */
> +                target_ulong virt_page2 = (pc & TARGET_PAGE_MASK) +
> +                                          TARGET_PAGE_SIZE;
> +                tb_page_addr_t phys_page2 = get_page_addr_code(env, virt_page2);
> +
>                  if (tb->page_addr[1] == phys_page2) {
>                      break;
>                  }
> -            } else {
> -                break;
>              }
>          }

FYI,

There's an issue here before and after this patch: calling get_page_addr_code
will cause an exception to be thrown if the page isn't mapped.

Except this is a search routine looking for matching TB's.  We shouldn't be
raising an exception within this loop.  We need a variant of get_page_addr_code
that reloads the TLB, if possible, but without generating a fault.

Something that I don't think we can actually do with the current tlb_fill
target hooks.  So not something for this patch, but something to put on
someone's radar, hopefully...


r~

Re: [Qemu-devel] [PATCH v2 2/3] tcg: reorganize tb_find_physical loop

[Richard Henderson]

On 03/29/2016 12:48 PM, Sergey Fedorov wrote:
> From: Alex Bennée <alex.bennee@linaro.org>
> 
> Put some comments and improve code structure. This should help reading
> the code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> [Sergey Fedorov: provide commit message; bring back resetting of
> tb_invalidated_flag]
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>

Oh, right.

Reviewed-by: Richard Henderson  <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH v2 1/3] tcg: code_bitmap is not used by user-mode emulation

[Alex Bennée]

Sergey Fedorov <sergey.fedorov@linaro.org> writes:

> From: Paolo Bonzini <pbonzini@redhat.com>
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [Sergey Fedorov: eliminate the field entirely in user-mode]
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>
> Notes:
>     Changes in v2:
>      * The field is eliminated entirely in user-mode
>
>  translate-all.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index e9f409b762ab..c131ce2f183e 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -75,8 +75,9 @@ typedef struct PageDesc {
>      /* in order to optimize self modifying code, we count the number
>         of lookups we do to a given page to use a bitmap */
>      unsigned int code_write_count;
> +#ifdef CONFIG_SOFTMMU
>      unsigned long *code_bitmap;
> -#if defined(CONFIG_USER_ONLY)
> +#else
>      unsigned long flags;
>  #endif
>  } PageDesc;
> @@ -784,8 +785,10 @@ void tb_free(TranslationBlock *tb)
>
>  static inline void invalidate_page_bitmap(PageDesc *p)
>  {
> +#ifdef CONFIG_SOFTMMU
>      g_free(p->code_bitmap);
>      p->code_bitmap = NULL;
> +#endif
>      p->code_write_count = 0;
>  }
>
> @@ -1018,6 +1021,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
>  }
>
> +#ifdef CONFIG_SOFTMMU
>  static void build_page_bitmap(PageDesc *p)
>  {
>      int n, tb_start, tb_end;
> @@ -1046,6 +1050,7 @@ static void build_page_bitmap(PageDesc *p)
>          tb = tb->page_next[n];
>      }
>  }
> +#endif
>
>  /* Called with mmap_lock held for user mode emulation.  */
>  TranslationBlock *tb_gen_code(CPUState *cpu,
> @@ -1294,6 +1299,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>  #endif
>  }
>
> +#ifdef CONFIG_SOFTMMU
>  /* len must be <= 8 and start must be a multiple of len */
>  void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
>  {
> @@ -1331,8 +1337,7 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
>          tb_invalidate_phys_page_range(start, start + len, 1);
>      }
>  }
> -
> -#if !defined(CONFIG_SOFTMMU)
> +#else
>  /* Called with mmap_lock held.  */
>  static void tb_invalidate_phys_page(tb_page_addr_t addr,
>                                      uintptr_t pc, void *puc,


--
Alex Bennée