| Message ID | 1530806837-5416-1-git-send-email-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | translate-all: honour CF_NOCACHE in tb_gen_code | expand |
On 07/05/2018 09:07 AM, Emilio G. Cota wrote: > This fixes a record-replay regression introduced by 95590e2 > ("translate-all: discard TB when tb_link_page returns an existing > matching TB", 2018-06-15). The problem is that code using CF_NOCACHE > assumes that the TB returned from tb_gen_code is always a > newly-generated one. This assumption, however, was broken in > the aforementioned commit. > > Fix it by honouring CF_NOCACHE, so that tb_gen_code always > returns a newly-generated TB when CF_NOCACHE is passed to it. > Do this by avoiding the TB hash table if CF_NOCACHE is set. > > Reported-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Tested-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > accel/tcg/translate-all.c | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 5 July 2018 at 17:07, Emilio G. Cota <cota@braap.org> wrote: > This fixes a record-replay regression introduced by 95590e2 > ("translate-all: discard TB when tb_link_page returns an existing > matching TB", 2018-06-15). The problem is that code using CF_NOCACHE > assumes that the TB returned from tb_gen_code is always a > newly-generated one. This assumption, however, was broken in > the aforementioned commit. > > Fix it by honouring CF_NOCACHE, so that tb_gen_code always > returns a newly-generated TB when CF_NOCACHE is passed to it. > Do this by avoiding the TB hash table if CF_NOCACHE is set. > > Reported-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Tested-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Signed-off-by: Emilio G. Cota <cota@braap.org> This bug can also cause crashes when using -icount, without record-replay; I hit it looking at a test case in LP:1774677. This patch seems to fix the crash. thanks -- PMM
On Fri, Jul 6, 2018 at 6:05 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 July 2018 at 17:07, Emilio G. Cota <cota@braap.org> wrote: >> This fixes a record-replay regression introduced by 95590e2 >> ("translate-all: discard TB when tb_link_page returns an existing >> matching TB", 2018-06-15). The problem is that code using CF_NOCACHE >> assumes that the TB returned from tb_gen_code is always a >> newly-generated one. This assumption, however, was broken in >> the aforementioned commit. >> >> Fix it by honouring CF_NOCACHE, so that tb_gen_code always >> returns a newly-generated TB when CF_NOCACHE is passed to it. >> Do this by avoiding the TB hash table if CF_NOCACHE is set. >> >> Reported-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> >> Tested-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> >> Signed-off-by: Emilio G. Cota <cota@braap.org> > > This bug can also cause crashes when using -icount, without > record-replay; I hit it looking at a test case in LP:1774677. > This patch seems to fix the crash. This fixes the icount crash for me as well. Tested-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > thanks > -- PMM >
On 5 July 2018 at 17:07, Emilio G. Cota <cota@braap.org> wrote: > This fixes a record-replay regression introduced by 95590e2 > ("translate-all: discard TB when tb_link_page returns an existing > matching TB", 2018-06-15). The problem is that code using CF_NOCACHE > assumes that the TB returned from tb_gen_code is always a > newly-generated one. This assumption, however, was broken in > the aforementioned commit. > > Fix it by honouring CF_NOCACHE, so that tb_gen_code always > returns a newly-generated TB when CF_NOCACHE is passed to it. > Do this by avoiding the TB hash table if CF_NOCACHE is set. > > Reported-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Tested-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- Applied to master ready for our rc0 tag tomorrow. thanks -- PMM
On 5 July 2018 at 17:07, Emilio G. Cota <cota@braap.org> wrote: > This fixes a record-replay regression introduced by 95590e2 > ("translate-all: discard TB when tb_link_page returns an existing > matching TB", 2018-06-15). The problem is that code using CF_NOCACHE > assumes that the TB returned from tb_gen_code is always a > newly-generated one. This assumption, however, was broken in > the aforementioned commit. > > Fix it by honouring CF_NOCACHE, so that tb_gen_code always > returns a newly-generated TB when CF_NOCACHE is passed to it. > Do this by avoiding the TB hash table if CF_NOCACHE is set. > > Reported-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Tested-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> > Signed-off-by: Emilio G. Cota <cota@braap.org> > @@ -1604,8 +1605,6 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc, > { > PageDesc *p; > PageDesc *p2 = NULL; > - void *existing_tb = NULL; > - uint32_t h; > > assert_memory_lock(); > > @@ -1625,20 +1624,25 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc, > tb->page_addr[1] = -1; > } > > - /* add in the hash table */ > - h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & CF_HASH_MASK, > - tb->trace_vcpu_dstate); > - qht_insert(&tb_ctx.htable, tb, h, &existing_tb); > + if (!(tb->cflags & CF_NOCACHE)) { > + void *existing_tb = NULL; > + uint32_t h; > > - /* remove TB from the page(s) if we couldn't insert it */ > - if (unlikely(existing_tb)) { > - tb_page_remove(p, tb); > - invalidate_page_bitmap(p); > - if (p2) { > - tb_page_remove(p2, tb); > - invalidate_page_bitmap(p2); > + /* add in the hash table */ > + h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & CF_HASH_MASK, > + tb->trace_vcpu_dstate); > + qht_insert(&tb_ctx.htable, tb, h, &existing_tb); > + > + /* remove TB from the page(s) if we couldn't insert it */ > + if (unlikely(existing_tb)) { > + tb_page_remove(p, tb); > + invalidate_page_bitmap(p); > + if (p2) { > + tb_page_remove(p2, tb); > + invalidate_page_bitmap(p2); > + } > + tb = existing_tb; > } > - tb = existing_tb; > } > > if (p2 && p2 != p) { Hi -- I've been having a look at getting QEMU to support execution from MMIO regions by doing a "generate a single-insn CF_NOCACHE TB". As part of that, rth and I ran into a question about this change: why does tb_link_page() still add a CF_NOCACHE TB to the page lists? That is, why does this patch guard the "add to the hash table" part of the function, rather than just saying "return doing nothing if CF_NOCACHE"? I'm pretty clueless about this bit of the code, so I'm quite probably missing something. thanks -- PMM
On Mon, Jul 09, 2018 at 17:59:02 +0100, Peter Maydell wrote: > Hi -- I've been having a look at getting QEMU to support execution > from MMIO regions by doing a "generate a single-insn CF_NOCACHE TB". > As part of that, rth and I ran into a question about this change: > why does tb_link_page() still add a CF_NOCACHE TB to the page lists? > That is, why does this patch guard the "add to the hash table" part > of the function, rather than just saying "return doing nothing > if CF_NOCACHE"? I think either way would work, although I have not tried the alternative you suggest. I'd say from a correctness viewpoint it seems "more correct" to add the TB to the TB list, if only for the serialization imposed by the page locks in softmmu mode. Think for instance of MTTCG, where you might have a NOCACHE generation concurrent with an invalidation of the same page where guest code will be read from. Having to go through the page locks at least enforces some ordering, which seems reasonable. That said, currently CF_NOCACHE is only used in icount (i.e. !MTTCG), so the above is not a practical concern. But for your MMIO use case, which AFAIK will be abled for all TCG modes, I'd rather keep the serialization in. > I'm pretty clueless about this bit of the code, so I'm quite > probably missing something. The original design was changed to support parallel code generation. docs/devel/multi-thread-tcg.txt documents those changes, although it assumes that the original design is known by the reader. That design can be sketched as follows. TBs are tracked by: 1. pc_ptr: host address of the translated code, which is managed with tcg_tb_lookup/remove/insert, with lookups usually being called from cpu_restore_state. These lookups are rare; insertions are a lot more common (one per generated TB). 2. phys_pc, pc, flags, etc. (all from the guest's viewpoint). These guest TB lookups are very common, so we have private per-vCPU caches plus a global hash table. 3. PageDesc: each guest page is tracked in a radix tree, whose root is l1_map in translate-all.c. PageDesc's also keep a list of the associated guest TBs, so that we can selectively invalidate TBs at a page granularity (eventually calling tb_phys_invalidate for each TB). Most of tb_gen_code is spent in code generation, which in softmmu works exclusively on thread-local data since commit 3468b59 ("tcg: enable multiple TCG contexts in softmmu", 2017-10-24). (in user-mode we have a single context protected by mmap_lock.) Once the code is generated, tb_add_page does 2 and 3 above, and then tb_gen_code does 1 at the very end. Hope that helps a little bit. Thanks, Emilio
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 170b957..49d77fa 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1446,7 +1446,8 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list) phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK); h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb_cflags(tb) & CF_HASH_MASK, tb->trace_vcpu_dstate); - if (!qht_remove(&tb_ctx.htable, tb, h)) { + if (!(tb->cflags & CF_NOCACHE) && + !qht_remove(&tb_ctx.htable, tb, h)) { return; } @@ -1604,8 +1605,6 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc, { PageDesc *p; PageDesc *p2 = NULL; - void *existing_tb = NULL; - uint32_t h; assert_memory_lock(); @@ -1625,20 +1624,25 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc, tb->page_addr[1] = -1; } - /* add in the hash table */ - h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & CF_HASH_MASK, - tb->trace_vcpu_dstate); - qht_insert(&tb_ctx.htable, tb, h, &existing_tb); + if (!(tb->cflags & CF_NOCACHE)) { + void *existing_tb = NULL; + uint32_t h; - /* remove TB from the page(s) if we couldn't insert it */ - if (unlikely(existing_tb)) { - tb_page_remove(p, tb); - invalidate_page_bitmap(p); - if (p2) { - tb_page_remove(p2, tb); - invalidate_page_bitmap(p2); + /* add in the hash table */ + h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & CF_HASH_MASK, + tb->trace_vcpu_dstate); + qht_insert(&tb_ctx.htable, tb, h, &existing_tb); + + /* remove TB from the page(s) if we couldn't insert it */ + if (unlikely(existing_tb)) { + tb_page_remove(p, tb); + invalidate_page_bitmap(p); + if (p2) { + tb_page_remove(p2, tb); + invalidate_page_bitmap(p2); + } + tb = existing_tb; } - tb = existing_tb; } if (p2 && p2 != p) {
This fixes a record-replay regression introduced by 95590e2 ("translate-all: discard TB when tb_link_page returns an existing matching TB", 2018-06-15). The problem is that code using CF_NOCACHE assumes that the TB returned from tb_gen_code is always a newly-generated one. This assumption, however, was broken in the aforementioned commit. Fix it by honouring CF_NOCACHE, so that tb_gen_code always returns a newly-generated TB when CF_NOCACHE is passed to it. Do this by avoiding the TB hash table if CF_NOCACHE is set. Reported-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> Tested-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> Signed-off-by: Emilio G. Cota <cota@braap.org> --- accel/tcg/translate-all.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)