Re: [Qemu-devel] [PATCH] translate-all: honour CF_NOCACHE in tb_gen_code

[Peter Maydell]

On 5 July 2018 at 17:07, Emilio G. Cota <address@hidden> 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 <address@hidden>
> Tested-by: Pavel Dovgalyuk <address@hidden>
> Signed-off-by: Emilio G. Cota <address@hidden>

> @@ -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