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 
> 
> Put some comments and improve code structure. This should help reading
> the code.
> 
> Signed-off-by: Alex Bennée 
> [Sergey Fedorov: provide commit message; bring back resetting of
> tb_invalidated_flag]
> Signed-off-by: Sergey Fedorov 

Oh, right.

Reviewed-by: Richard Henderson  


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~

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

[Sergey Fedorov]

From: Alex Bennée 

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

Signed-off-by: Alex Bennée 
[Sergey Fedorov: provide commit message; bring back resetting of
tb_invalidated_flag]
Signed-off-by: Sergey Fedorov 
---
 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 = _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 = _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 = >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