Re: [Qemu-devel] [RFC v3] translate-all: protect code_gen_buffer with RCU

["Alex Bennée" <alex.bennee@linaro.org>]

Emilio G. Cota <cota@braap.org> writes:

> Context:
>   https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04658.html
>   https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06942.html
>
> This seems to half-work[*] although I'm uneasy about the while idea.
> I see two major hurdles:
>
> * If the TB size is too small, this breaks badly, because we're not
>   out of the RCU read critical section when another call to tb_flush
>   is made. For instance, when booting ARM with this patch applied,
>   I need to at least pass -tb-size 10 for it to fully boot debian
>   jessie.
> * We have different tb_flush callers that should be audited:
>  $ git grep '\stb_flush(' | grep -v '//' | grep -v 'CPUState'
>  exec.c:            tb_flush(cpu);
>  gdbstub.c:        tb_flush(cpu);
>  gdbstub.c:    tb_flush(cpu);
>  hw/ppc/spapr_hcall.c:            tb_flush(CPU(cpu));
>  target-alpha/sys_helper.c:    tb_flush(CPU(alpha_env_get_cpu(env)));
>  target-i386/translate.c:        tb_flush(CPU(x86_env_get_cpu(env)));
>  translate-all.c:        tb_flush(cpu);
>
> With two code_gen "halves", if two tb_flush calls are done in the same
> RCU read critical section, we're screwed. I added a cpu_exit at the end
> of tb_flush to try to mitigate this, but I haven't audited all the callers
> (for instance, what does the gdbstub do?).

I'm not sure we are going to get much from this approach. The tb_flush
is a fairly rare occurrence its not like its on the critical performance
path (although of course pathological cases are possible).

I still think there are possibilities with a smaller TranslationRegion
approach but this is more aimed at solving problems like bulk
invalidations of a page of translations at a time and safer inter-block
patching. It doesn't do much to make the tb_flush easier though.

>
> If we end up having a mechanism to "stop all  CPUs to do something", as
> I think we'll end up needing for correct LL/SC emulation, we'll probably
> be better off using that mechanism for tb_flush as well -- plus, we'll avoid
> wasting memory.

I'm fairly certain there will need to be a "stop everything" mode for
some things - I'm less certain of the best way of doing it. Did you get
a chance to look at my version of the async_safe_work mechanism?

>
> Other issues:
> - This could be split into at least 2 patches, one that touches
>   tcg/ and another to deal with translate-all.
>   Note that in translate-all, the initial allocation of code_gen doesn't
>   allocate extra space for the guard page; reserving guard page space is
>   done instead by the added split_code_gen_buffer function.
> - Windows: not even compile-tested.
>
> [*] "Seems to half-work". At least it boots ARM OK with MTTCG and -smp 4.
>     Alex' tests, however, sometimes fail with:
>
> Unhandled exception 3 (pabt)
> Exception frame registers:
> pc : [<fffffb44>]    lr : [<00000001>]    psr: 20000173
> sp : 4004f528  ip : 40012048  fp : 40032ca8
> r10: 40032ca8  r9 : 00000000  r8 : 00000000
> r7 : 0000000e  r6 : 40030000  r5 : 40032ca8  r4 : 00001ac6
> r3 : 40012030  r2 : 40012030  r1 : d5ffffe7  r0 : 00000028
> Flags: nzCv  IRQs on  FIQs off  Mode SVC_32
> Control: 00c5107d  Table: 40060000  DAC: 00000000
> IFAR: fffffb44    IFSR: 00000205
>
> or with:
>
> CPU0: 16986 irqs (0 races, 11 slow,  1322 ticks avg latency)
> FAIL: smc: irq: 17295 IRQs sent, 16986 received
>
> Unhandled exception 6 (irq)
> Exception frame registers:
> pc : [<00000020>]    lr : [<40010800>]    psr: 60000153
> sp : 400b45c0  ip : 400b34e8  fp : 40032ca8
> r10: 00000000  r9 : 00000000  r8 : 00000000
> r7 : 00000000  r6 : 00000000  r5 : 00000000  r4 : 00000000
> r3 : 00000000  r2 : 00000000  r1 : 000000ff  r0 : 00000000
> Flags: nZCv  IRQs on  FIQs off  Mode SVC_32
> Control: 00c5107d  Table: 40060000  DAC: 00000000
>
> I built with --enable-tcg-debug.

I'll have another go at reproducing. I could only get the asserts to
fire which was odd because AFAICT the prologue generation which
triggered it was serialised with tb_lock.

>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  tcg/tcg.c       |  22 +++++---
>  tcg/tcg.h       |   1 +
>  translate-all.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  3 files changed, 144 insertions(+), 34 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index b46bf1a..7db8ce9 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -380,6 +380,20 @@ void tcg_context_init(TCGContext *s)
>      }
>  }
>
> +void tcg_code_gen_init(TCGContext *s, void *buf, size_t prologue_size)
> +{
> +    size_t size = s->code_gen_buffer_size - prologue_size;
> +
> +    s->code_gen_ptr = buf;
> +    s->code_gen_buffer = buf;
> +    s->code_buf = buf;
> +
> +    /* Compute a high-water mark, at which we voluntarily flush the buffer
> +       and start over.  The size here is arbitrary, significantly larger
> +       than we expect the code generation for any one opcode to require.  */
> +    s->code_gen_highwater = s->code_buf + (size - 1024);
> +}
> +
>  void tcg_prologue_init(TCGContext *s)
>  {
>      size_t prologue_size, total_size;
> @@ -398,16 +412,10 @@ void tcg_prologue_init(TCGContext *s)
>
>      /* Deduct the prologue from the buffer.  */
>      prologue_size = tcg_current_code_size(s);
> -    s->code_gen_ptr = buf1;
> -    s->code_gen_buffer = buf1;
> -    s->code_buf = buf1;
>      total_size = s->code_gen_buffer_size - prologue_size;
>      s->code_gen_buffer_size = total_size;
>
> -    /* Compute a high-water mark, at which we voluntarily flush the buffer
> -       and start over.  The size here is arbitrary, significantly larger
> -       than we expect the code generation for any one opcode to require.  */
> -    s->code_gen_highwater = s->code_gen_buffer + (total_size - 1024);
> +    tcg_code_gen_init(s, buf1, prologue_size);
>
>      tcg_register_jit(s->code_gen_buffer, total_size);
>
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 40c8fbe..e849d3e 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -634,6 +634,7 @@ static inline void *tcg_malloc(int size)
>
>  void tcg_context_init(TCGContext *s);
>  void tcg_prologue_init(TCGContext *s);
> +void tcg_code_gen_init(TCGContext *s, void *buf, size_t prologue_size);
>  void tcg_func_start(TCGContext *s);
>
>  int tcg_gen_code(TCGContext *s, TranslationBlock *tb);
> diff --git a/translate-all.c b/translate-all.c
> index bba9b62..7a83176 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -485,6 +485,11 @@ static inline PageDesc *page_find(tb_page_addr_t index)
>    (DEFAULT_CODE_GEN_BUFFER_SIZE_1 < MAX_CODE_GEN_BUFFER_SIZE \
>     ? DEFAULT_CODE_GEN_BUFFER_SIZE_1 : MAX_CODE_GEN_BUFFER_SIZE)
>
> +static int code_gen_buf_mask;
> +static void *code_gen_buf1;
> +static void *code_gen_buf2;
> +static size_t code_gen_prologue_size;
> +
>  static inline size_t size_code_gen_buffer(size_t tb_size)
>  {
>      /* Size the buffer.  */
> @@ -508,6 +513,43 @@ static inline size_t size_code_gen_buffer(size_t tb_size)
>      return tb_size;
>  }
>
> +static void *split_code_gen_buffer(void *buf, size_t size)
> +{
> +    /* enforce alignment of the buffer to a page boundary */
> +    if (unlikely((uintptr_t)buf & ~qemu_real_host_page_mask)) {
> +        uintptr_t b;
> +
> +        b = QEMU_ALIGN_UP((uintptr_t)buf, qemu_real_host_page_size);
> +        size -= b - (uintptr_t)buf;
> +        buf = (void *)b;
> +    }
> +    /*
> +     * Make sure the size is an even number of pages so that both halves will
> +     * end on a page boundary.
> +     */
> +    size = QEMU_ALIGN_DOWN(size, 2 * qemu_real_host_page_size);
> +
> +    /* split in half, making room for 2 guard pages */
> +    size -= 2 * qemu_real_host_page_size;
> +    size /= 2;
> +    code_gen_buf1 = buf;
> +    code_gen_buf2 = buf + size + qemu_real_host_page_size;
> +
> +    /*
> +     * write the prologue into buf2. This is safe because we'll later call
> +     * tcg_prologue_init on buf1, from which we'll start execution.
> +     */
> +    tcg_ctx.code_gen_buffer = code_gen_buf2;
> +    tcg_prologue_init(&tcg_ctx);
> +    code_gen_prologue_size = (void *)tcg_ctx.code_ptr - code_gen_buf2;
> +
> +    tcg_ctx.code_gen_buffer_size = size;
> +
> +    /* start execution from buf1 */
> +    code_gen_buf_mask = 1;
> +    return code_gen_buf1;
> +}
> +
>  #ifdef __mips__
>  /* In order to use J and JAL within the code_gen_buffer, we require
>     that the buffer not cross a 256MB boundary.  */
> @@ -583,21 +625,17 @@ static inline void map_none(void *addr, long size)
>  static inline void *alloc_code_gen_buffer(void)
>  {
>      void *buf = static_code_gen_buffer;
> -    size_t full_size, size;
> +    size_t size;
>
>      /* The size of the buffer, rounded down to end on a page boundary.  */
> -    full_size = (((uintptr_t)buf + sizeof(static_code_gen_buffer))
> -                 & qemu_real_host_page_mask) - (uintptr_t)buf;
> -
> -    /* Reserve a guard page.  */
> -    size = full_size - qemu_real_host_page_size;
> +    size = (((uintptr_t)buf + sizeof(static_code_gen_buffer))
> +            & qemu_real_host_page_mask) - (uintptr_t)buf;
>
>      /* Honor a command-line option limiting the size of the buffer.  */
>      if (size > tcg_ctx.code_gen_buffer_size) {
>          size = (((uintptr_t)buf + tcg_ctx.code_gen_buffer_size)
>                  & qemu_real_host_page_mask) - (uintptr_t)buf;
>      }
> -    tcg_ctx.code_gen_buffer_size = size;
>
>  #ifdef __mips__
>      if (cross_256mb(buf, size)) {
> @@ -607,27 +645,40 @@ static inline void *alloc_code_gen_buffer(void)
>  #endif
>
>      map_exec(buf, size);
> -    map_none(buf + size, qemu_real_host_page_size);
>      qemu_madvise(buf, size, QEMU_MADV_HUGEPAGE);
>
> +    buf = split_code_gen_buffer(buf, size);
> +    size = tcg_ctx.code_gen_buffer_size;
> +
> +    /* page guards */
> +    map_none(code_gen_buf1 + size, qemu_real_host_page_size);
> +    map_none(code_gen_buf2 + size, qemu_real_host_page_size);
> +
>      return buf;
>  }
>  #elif defined(_WIN32)
>  static inline void *alloc_code_gen_buffer(void)
>  {
>      size_t size = tcg_ctx.code_gen_buffer_size;
> -    void *buf1, *buf2;
> +    void *buf;
> +    void *ret;
>
> -    /* Perform the allocation in two steps, so that the guard page
> -       is reserved but uncommitted.  */
> -    buf1 = VirtualAlloc(NULL, size + qemu_real_host_page_size,
> -                        MEM_RESERVE, PAGE_NOACCESS);
> -    if (buf1 != NULL) {
> -        buf2 = VirtualAlloc(buf1, size, MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> -        assert(buf1 == buf2);
> +    /* Perform the allocation in two steps, so that the guard pages
> +       are reserved but uncommitted.  */
> +    ret = VirtualAlloc(NULL, size, MEM_RESERVE, PAGE_NOACCESS);
> +    if (ret == NULL) {
> +        return NULL;
>      }
>
> -    return buf1;
> +    ret = split_code_gen_buffer(ret, size);
> +    size = tcg_ctx.code_gen_buffer_size;
> +
> +    buf = VirtualAlloc(code_gen_buf1, size, MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> +    assert(buf);
> +    buf = VirtualAlloc(code_gen_buf2, size, MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> +    assert(buf);
> +
> +    return ret;
>  }
>  #else
>  static inline void *alloc_code_gen_buffer(void)
> @@ -665,8 +716,7 @@ static inline void *alloc_code_gen_buffer(void)
>  #  endif
>  # endif
>
> -    buf = mmap((void *)start, size + qemu_real_host_page_size,
> -               PROT_NONE, flags, -1, 0);
> +    buf = mmap((void *)start, size, PROT_NONE, flags, -1, 0);
>      if (buf == MAP_FAILED) {
>          return NULL;
>      }
> @@ -676,24 +726,24 @@ static inline void *alloc_code_gen_buffer(void)
>          /* Try again, with the original still mapped, to avoid re-acquiring
>             that 256mb crossing.  This time don't specify an address.  */
>          size_t size2;
> -        void *buf2 = mmap(NULL, size + qemu_real_host_page_size,
> -                          PROT_NONE, flags, -1, 0);
> +        void *buf2 = mmap(NULL, size, PROT_NONE, flags, -1, 0);
> +
>          switch (buf2 != MAP_FAILED) {
>          case 1:
>              if (!cross_256mb(buf2, size)) {
>                  /* Success!  Use the new buffer.  */
> -                munmap(buf, size + qemu_real_host_page_size);
> +                munmap(buf, size);
>                  break;
>              }
>              /* Failure.  Work with what we had.  */
> -            munmap(buf2, size + qemu_real_host_page_size);
> +            munmap(buf2, size);
>              /* fallthru */
>          default:
>              /* Split the original buffer.  Free the smaller half.  */
>              buf2 = split_cross_256mb(buf, size);
>              size2 = tcg_ctx.code_gen_buffer_size;
>              if (buf == buf2) {
> -                munmap(buf + size2 + qemu_real_host_page_size, size - size2);
> +                munmap(buf + size2, size - size2);
>              } else {
>                  munmap(buf, size - size2);
>              }
> @@ -704,13 +754,19 @@ static inline void *alloc_code_gen_buffer(void)
>      }
>  #endif
>
> -    /* Make the final buffer accessible.  The guard page at the end
> -       will remain inaccessible with PROT_NONE.  */
> +    /* Make the final buffer accessible. */
>      mprotect(buf, size, PROT_WRITE | PROT_READ | PROT_EXEC);
>
>      /* Request large pages for the buffer.  */
>      qemu_madvise(buf, size, QEMU_MADV_HUGEPAGE);
>
> +    buf = split_code_gen_buffer(buf + 1, size);
> +    size = tcg_ctx.code_gen_buffer_size;
> +
> +    /* page guards */
> +    mprotect(code_gen_buf1 + size, qemu_real_host_page_size, PROT_NONE);
> +    mprotect(code_gen_buf2 + size, qemu_real_host_page_size, PROT_NONE);
> +
>      return buf;
>  }
>  #endif /* USE_STATIC_CODE_GEN_BUFFER, WIN32, POSIX */
> @@ -829,10 +885,48 @@ static void page_flush_tb(void)
>      }
>  }
>
> +struct code_gen_desc {
> +    struct rcu_head rcu;
> +    int clear_bit;
> +};
> +
> +static void clear_code_gen_buffer(struct rcu_head *rcu)
> +{
> +    struct code_gen_desc *desc = container_of(rcu, struct code_gen_desc, rcu);
> +
> +    tb_lock();
> +    code_gen_buf_mask &= ~desc->clear_bit;
> +    tb_unlock();
> +    g_free(desc);
> +}
> +
> +static void *replace_code_gen_buffer(void)
> +{
> +    struct code_gen_desc *desc = g_malloc0(sizeof(*desc));
> +
> +    /*
> +     * If both bits are set, we're having two concurrent flushes. This
> +     * can easily happen if the buffers are heavily undersized.
> +     */
> +    assert(code_gen_buf_mask == 1 || code_gen_buf_mask == 2);
> +
> +    desc->clear_bit = code_gen_buf_mask;
> +    call_rcu1(&desc->rcu, clear_code_gen_buffer);
> +
> +    if (code_gen_buf_mask == 1) {
> +        code_gen_buf_mask |= 2;
> +        return code_gen_buf2 + code_gen_prologue_size;
> +    }
> +    code_gen_buf_mask |= 1;
> +    return code_gen_buf1 + code_gen_prologue_size;
> +}
> +
>  /* flush all the translation blocks */
>  /* XXX: tb_flush is currently not thread safe */
>  void tb_flush(CPUState *cpu)
>  {
> +    void *buf;
> +
>  #if defined(DEBUG_FLUSH)
>      printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
>             (unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer),
> @@ -853,10 +947,17 @@ void tb_flush(CPUState *cpu)
>      qht_reset_size(&tcg_ctx.tb_ctx.htable, CODE_GEN_HTABLE_SIZE);
>      page_flush_tb();
>
> -    tcg_ctx.code_gen_ptr = tcg_ctx.code_gen_buffer;
> +    buf = replace_code_gen_buffer();
> +    tcg_code_gen_init(&tcg_ctx, buf, code_gen_prologue_size);
> +
>      /* XXX: flush processor icache at this point if cache flush is
>         expensive */
>      tcg_ctx.tb_ctx.tb_flush_count++;
> +
> +    /* exit all CPUs so that the old buffer is quickly cleared */
> +    CPU_FOREACH(cpu) {
> +        cpu_exit(cpu);
> +    }
>  }
>
>  #ifdef DEBUG_TB_CHECK


--
Alex Bennée