Re: [Qemu-devel] [PULL, 14/18] translate-all: discard TB when tb_link_page returns an existing matching TB

[Pavel Dovgalyuk]

> From: Emilio G. Cota [mailto:address@hidden
> On Tue, Jul 03, 2018 at 08:38:52 +0300, Pavel Dovgalyuk wrote:
> > > From: Emilio G. Cota [mailto:address@hidden
> > > On Mon, Jul 02, 2018 at 08:52:14 +0300, Pavel Dovgalyuk wrote:
> > > > The same failure can be reproduced with linux-0.2.img, which was
> > > > downloaded from QEMU site.
> > > > I can't find it now, but I can upload this file if needed.
> > >
> > > Please upload it somewhere and share the full QEMU invocation
> > > needed to replicate.
> >
> > https://github.com/Dovgalyuk/qemu-images/blob/master/linux-0.2.img
> >
> > qemu-system-i386 -drive file=images/linux-0.2.img,if=none,snapshot,id=img 
> > -drive
> > driver=blkreplay,if=none,id=rr,image=img -device ide-hd,drive=rr -net none 
> > -icount
> > shift=5,rr=record,rrfile=linux02.rr
> 
> The appended patch fixes it for me. Can you please test on your
> windows image?
> 
> The rationale is to honour CF_NOCACHE, so that we always return
> a new TB from tb_gen_code.

Works for me, thank you.

Tested-by: Pavel Dovgalyuk <address@hidden>

Pavel Dovgalyuk

> 
> ---
> 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) {