Re: [Qemu-devel] [PATCH v3 02/10] tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> From: Sergey Fedorov <address@hidden>
>
> These fields do not contain pure pointers to a TranslationBlock
> structure. So uintptr_t is the most appropriate type for them.
> Also put an assert to assure that the two least significant bits of the
> pointer are zero before assigning it to jmp_list_first.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Sergey Fedorov <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

> ---
>
> Changes in v2:
>  * Eliminated duplicate dereference of 'ptb' in tb_jmp_remove()
>
>  include/exec/exec-all.h | 13 ++++++++-----
>  translate-all.c         | 38 ++++++++++++++++++++------------------
>  2 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 38a149cc5e0c..b055716ed690 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -275,14 +275,16 @@ struct TranslationBlock {
>       * jmp_list_first points to the first TB jumping to this one.
>       * jmp_list_next is used to point to the next TB in a list.
>       * Since each TB can have two jumps, it can participate in two lists.
> -     * The two least significant bits of a pointer are used to choose which
> -     * data field holds a pointer to the next TB:
> +     * jmp_list_first and jmp_list_next are 4-byte aligned pointers to a
> +     * TranslationBlock structure, but the two least significant bits of
> +     * them are used to encode which data field of the pointed TB should
> +     * be used to traverse the list further from that TB:
>       * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>       * In other words, 0/1 tells which jump is used in the pointed TB,
>       * and 2 means that this is a pointer back to the target TB of this list.
>       */
> -    struct TranslationBlock *jmp_list_next[2];
> -    struct TranslationBlock *jmp_list_first;
> +    uintptr_t jmp_list_next[2];
> +    uintptr_t jmp_list_first;
>  };
>
>  #include "qemu/thread.h"
> @@ -401,7 +403,8 @@ static inline void tb_add_jump(TranslationBlock *tb, int 
> n,
>
>          /* add in TB jmp circular list */
>          tb->jmp_list_next[n] = tb_next->jmp_list_first;
> -        tb_next->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | n);
> +        assert(((uintptr_t)tb & 3) == 0);
> +        tb_next->jmp_list_first = (uintptr_t)tb | n;
>      }
>  }
>
> diff --git a/translate-all.c b/translate-all.c
> index 33ca06c663d4..ba71ff73f55f 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -927,17 +927,17 @@ static inline void tb_page_remove(TranslationBlock 
> **ptb, TranslationBlock *tb)
>
>  static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>  {
> -    TranslationBlock *tb1, **ptb;
> +    TranslationBlock *tb1;
> +    uintptr_t *ptb, ntb;
>      unsigned int n1;
>
>      ptb = &tb->jmp_list_next[n];
> -    tb1 = *ptb;
> -    if (tb1) {
> +    if (*ptb) {
>          /* find tb(n) in circular list */
>          for (;;) {
> -            tb1 = *ptb;
> -            n1 = (uintptr_t)tb1 & 3;
> -            tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
> +            ntb = *ptb;
> +            n1 = ntb & 3;
> +            tb1 = (TranslationBlock *)(ntb & ~3);
>              if (n1 == n && tb1 == tb) {
>                  break;
>              }
> @@ -950,7 +950,7 @@ static inline void tb_jmp_remove(TranslationBlock *tb, 
> int n)
>          /* now we can suppress tb(n) from the list */
>          *ptb = tb->jmp_list_next[n];
>
> -        tb->jmp_list_next[n] = NULL;
> +        tb->jmp_list_next[n] = (uintptr_t)NULL;
>      }
>  }
>
> @@ -969,7 +969,7 @@ void tb_phys_invalidate(TranslationBlock *tb, 
> tb_page_addr_t page_addr)
>      PageDesc *p;
>      unsigned int h, n1;
>      tb_page_addr_t phys_pc;
> -    TranslationBlock *tb1, *tb2;
> +    uintptr_t tb1, tb2;
>
>      /* remove the TB from the hash list */
>      phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
> @@ -1005,19 +1005,20 @@ void tb_phys_invalidate(TranslationBlock *tb, 
> tb_page_addr_t page_addr)
>      /* suppress any remaining jumps to this TB */
>      tb1 = tb->jmp_list_first;
>      for (;;) {
> -        n1 = (uintptr_t)tb1 & 3;
> +        TranslationBlock *tmp_tb;
> +        n1 = tb1 & 3;
>          if (n1 == 2) {
>              break;
>          }
> -        tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
> -        tb2 = tb1->jmp_list_next[n1];
> -        tb_reset_jump(tb1, n1);
> -        tb1->jmp_list_next[n1] = NULL;
> +        tmp_tb = (TranslationBlock *)(tb1 & ~3);
> +        tb2 = tmp_tb->jmp_list_next[n1];
> +        tb_reset_jump(tmp_tb, n1);
> +        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
>          tb1 = tb2;
>      }
>
> -    /* fail safe */
> -    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
>
>      tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
>  }
> @@ -1491,9 +1492,10 @@ static void tb_link_page(TranslationBlock *tb, 
> tb_page_addr_t phys_pc,
>          tb->page_addr[1] = -1;
>      }
>
> -    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
> -    tb->jmp_list_next[0] = NULL;
> -    tb->jmp_list_next[1] = NULL;
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2;
> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
>
>      /* init original jump addresses */
>      if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {


--
Alex Bennée