[v2,2/3] tcg: reorganize tb_find_physical loop

Commit Message

[e-mail] March 29, 2016, 7:48 p.m. UTC

From: Alex Bennée <[e-mail]>

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

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

Comments

Richard Henderson March 29, 2016, 8:26 p.m. UTC | #1

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~

Richard Henderson March 29, 2016, 8:27 p.m. UTC | #2

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

Oh, right.

Reviewed-by: Richard Henderson  <[e-mail]>


r~

Patch

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;
 }