From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58110) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aj6jJ-0000IU-8v for qemu-devel@nongnu.org; Thu, 24 Mar 2016 10:59:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aj6jG-00077Z-2P for qemu-devel@nongnu.org; Thu, 24 Mar 2016 10:59:21 -0400 Received: from mail-wm0-x22f.google.com ([2a00:1450:400c:c09::22f]:32955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aj6jF-00077T-KB for qemu-devel@nongnu.org; Thu, 24 Mar 2016 10:59:17 -0400 Received: by mail-wm0-x22f.google.com with SMTP id l68so278650036wml.0 for ; Thu, 24 Mar 2016 07:59:17 -0700 (PDT) References: <1458815961-31979-1-git-send-email-sergey.fedorov@linaro.org> <1458815961-31979-3-git-send-email-sergey.fedorov@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1458815961-31979-3-git-send-email-sergey.fedorov@linaro.org> Date: Thu, 24 Mar 2016 14:58:56 +0000 Message-ID: <87oaa3rkgf.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/8] tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergey.fedorov@linaro.org Cc: Sergey Fedorov , Richard Henderson , Peter Crosthwaite , qemu-devel@nongnu.org, Paolo Bonzini sergey.fedorov@linaro.org writes: > From: Sergey Fedorov > > 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 > Signed-off-by: Sergey Fedorov > --- > include/exec/exec-all.h | 12 +++++++----- > translate-all.c | 37 +++++++++++++++++++------------------ > 2 files changed, 26 insertions(+), 23 deletions(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index cc3d2ca25917..cd96219a89e7 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -275,14 +275,15 @@ 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, and the two least significant bits of them > + * are used to encode which data field holds a pointer to the next 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. > */ Ahh I see you anticipate my previous confusion. Does this mean each time a jump is resolved for a particular chain the next tb could be in a different entry in the next TB? > - 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" > @@ -396,7 +397,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 31cdf8ae217e..7c008927e3f3 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -926,17 +926,16 @@ 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; > 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); > + n1 = *ptb & 3; > + tb1 = (TranslationBlock *)(*ptb & ~3); > if (n1 == n && tb1 == tb) { > break; > } > @@ -949,7 +948,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; > } > } > > @@ -968,7 +967,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); > @@ -1004,19 +1003,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++; > } > @@ -1489,9 +1489,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