From: Alex Bennée <alex.bennee@linaro.org>

The bug describes a race whereby cpu_exec_step_atomic can acquire a TB

which is invalidated by a tb_flush before we execute it. This doesn't

affect the other cpu_exec modes as a tb_flush by it's nature can only

occur on a quiescent system. The race was described as:

B2. tcg_cpu_exec => cpu_exec => tb_find => tb_gen_code

B3. tcg_tb_alloc obtains a new TB

C3. TB obtained with tb_lookup__cpu_state or tb_gen_code

(same TB as B2)

A3. start_exclusive critical section entered

A4. do_tb_flush is called, TB memory freed/re-allocated

A5. end_exclusive exits critical section

B2. tcg_cpu_exec => cpu_exec => tb_find => tb_gen_code

B3. tcg_tb_alloc reallocates TB from B2

C4. start_exclusive critical section entered

C5. cpu_tb_exec executes the TB code that was free in A4

The simplest fix is to widen the exclusive period to include the TB

lookup. As a result we can drop the complication of checking we are in

the exclusive region before we end it.

Cc: Yifan <me@yifanlu.com>

Buglink: https://bugs.launchpad.net/qemu/+bug/1863025

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Message-Id: <20200214144952.15502-1-alex.bennee@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---

accel/tcg/cpu-exec.c | 21 +++++++++++----------

1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c

index XXXXXXX..XXXXXXX 100644

--- a/accel/tcg/cpu-exec.c

+++ b/accel/tcg/cpu-exec.c

@@ -XXX,XX +XXX,XX @@ void cpu_exec_step_atomic(CPUState *cpu)

uint32_t cf_mask = cflags & CF_HASH_MASK;

if (sigsetjmp(cpu->jmp_env, 0) == 0) {

+ start_exclusive();

+

tb = tb_lookup__cpu_state(cpu, &pc, &cs_base, &flags, cf_mask);

if (tb == NULL) {

mmap_lock();

@@ -XXX,XX +XXX,XX @@ void cpu_exec_step_atomic(CPUState *cpu)

mmap_unlock();

}

- start_exclusive();

-

/* Since we got here, we know that parallel_cpus must be true. */

parallel_cpus = false;

cc->cpu_exec_enter(cpu);

@@ -XXX,XX +XXX,XX @@ void cpu_exec_step_atomic(CPUState *cpu)

qemu_plugin_disable_mem_helpers(cpu);

}

- if (cpu_in_exclusive_context(cpu)) {

- /* We might longjump out of either the codegen or the

- * execution, so must make sure we only end the exclusive

- * region if we started it.

- */

- parallel_cpus = true;

- end_exclusive();

- }

+

+ /*

+ * As we start the exclusive region before codegen we must still

+ * be in the region if we longjump out of either the codegen or

+ * the execution.

+ */

+ g_assert(cpu_in_exclusive_context(cpu));

+ parallel_cpus = true;

+ end_exclusive();

}

struct tb_desc {

--

2.20.1

From: Zenghui Yu <yuzenghui@huawei.com>

Our robot reported the following compile-time warning while compiling

Qemu with -fno-inline cflags:

In function 'load_memop',

inlined from 'load_helper' at /qemu/accel/tcg/cputlb.c:1578:20,

inlined from 'full_ldub_mmu' at /qemu/accel/tcg/cputlb.c:1624:12:

/qemu/accel/tcg/cputlb.c:1502:9: error: call to 'qemu_build_not_reached' declared with attribute error: code path is reachable

qemu_build_not_reached();

^~~~~~~~~~~~~~~~~~~~~~~~

[...]

It looks like a false-positive because only (MO_UB ^ MO_BSWAP) will

hit the default case in load_memop() while need_swap (size > 1) has

already ensured that MO_UB is not involved.

So the thing is that compilers get confused by the -fno-inline and

just can't accurately evaluate memop_size(op) at compile time, and

then the qemu_build_not_reached() is wrongly triggered by (MO_UB ^

MO_BSWAP). Let's carefully don't use the compile-time assert when

no functions will be inlined into their callers.

Reported-by: Euler Robot <euler.robot@huawei.com>

Suggested-by: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>

Message-Id: <20200205141545.180-1-yuzenghui@huawei.com>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---

include/qemu/compiler.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/compiler.h

+++ b/include/qemu/compiler.h

@@ -XXX,XX +XXX,XX @@

* supports QEMU_ERROR, this will be reported at compile time; otherwise

* this will be reported at link time due to the missing symbol.

*/

-#ifdef __OPTIMIZE__

+#if defined(__OPTIMIZE__) && !defined(__NO_INLINE__)

extern void QEMU_NORETURN QEMU_ERROR("code path is reachable")

qemu_build_not_reached(void);

#else

--

2.20.1

From: Richard Henderson <rth@twiddle.net>

We will shortly use this function from tcg_out_op as well.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Signed-off-by: Richard Henderson <rth@twiddle.net>

---

tcg/arm/tcg-target.inc.c | 19 +++++++++++--------

1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c

index XXXXXXX..XXXXXXX 100644

--- a/tcg/arm/tcg-target.inc.c

+++ b/tcg/arm/tcg-target.inc.c

@@ -XXX,XX +XXX,XX @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is64)

}

static tcg_insn_unit *tb_ret_addr;

+static void tcg_out_epilogue(TCGContext *s);

static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,

const TCGArg *args, const int *const_args)

@@ -XXX,XX +XXX,XX @@ static void tcg_out_nop_fill(tcg_insn_unit *p, int count)

+ TCG_TARGET_STACK_ALIGN - 1) \

& -TCG_TARGET_STACK_ALIGN)

+#define STACK_ADDEND (FRAME_SIZE - PUSH_SIZE)

+

static void tcg_target_qemu_prologue(TCGContext *s)

{

- int stack_addend;

-

/* Calling convention requires us to save r4-r11 and lr. */

/* stmdb sp!, { r4 - r11, lr } */

tcg_out32(s, (COND_AL << 28) | 0x092d4ff0);

/* Reserve callee argument and tcg temp space. */

- stack_addend = FRAME_SIZE - PUSH_SIZE;

-

tcg_out_dat_rI(s, COND_AL, ARITH_SUB, TCG_REG_CALL_STACK,

- TCG_REG_CALL_STACK, stack_addend, 1);

+ TCG_REG_CALL_STACK, STACK_ADDEND, 1);

tcg_set_frame(s, TCG_REG_CALL_STACK, TCG_STATIC_CALL_ARGS_SIZE,

CPU_TEMP_BUF_NLONGS * sizeof(long));

@@ -XXX,XX +XXX,XX @@ static void tcg_target_qemu_prologue(TCGContext *s)

*/

s->code_gen_epilogue = s->code_ptr;

tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R0, 0);

-

- /* TB epilogue */

tb_ret_addr = s->code_ptr;

+ tcg_out_epilogue(s);

+}

+

+static void tcg_out_epilogue(TCGContext *s)

+{

+ /* Release local stack frame. */

tcg_out_dat_rI(s, COND_AL, ARITH_ADD, TCG_REG_CALL_STACK,

- TCG_REG_CALL_STACK, stack_addend, 1);

+ TCG_REG_CALL_STACK, STACK_ADDEND, 1);

/* ldmia sp!, { r4 - r11, pc } */

tcg_out32(s, (COND_AL << 28) | 0x08bd8ff0);

--

2.20.1

From: Richard Henderson <rth@twiddle.net>

It is, after all, just two instructions.

Profiling on a cortex-a15, using -d nochain to increase the number

of exit_tb that are executed, shows a minor improvement of 0.5%.

Signed-off-by: Richard Henderson <rth@twiddle.net>

---

tcg/arm/tcg-target.inc.c | 12 ++----------

1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c

index XXXXXXX..XXXXXXX 100644

--- a/tcg/arm/tcg-target.inc.c

+++ b/tcg/arm/tcg-target.inc.c

@@ -XXX,XX +XXX,XX @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is64)

#endif

}

-static tcg_insn_unit *tb_ret_addr;

static void tcg_out_epilogue(TCGContext *s);

static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,

@@ -XXX,XX +XXX,XX @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,

switch (opc) {

case INDEX_op_exit_tb:

- /* Reuse the zeroing that exists for goto_ptr. */

- a0 = args[0];

- if (a0 == 0) {

- tcg_out_goto(s, COND_AL, s->code_gen_epilogue);

- } else {

- tcg_out_movi32(s, COND_AL, TCG_REG_R0, args[0]);

- tcg_out_goto(s, COND_AL, tb_ret_addr);

- }

+ tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R0, args[0]);

+ tcg_out_epilogue(s);

break;

case INDEX_op_goto_tb:

{

@@ -XXX,XX +XXX,XX @@ static void tcg_target_qemu_prologue(TCGContext *s)

*/

s->code_gen_epilogue = s->code_ptr;

tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R0, 0);

- tb_ret_addr = s->code_ptr;

tcg_out_epilogue(s);

}

--

2.20.1

From: Alex Bennée <alex.bennee@linaro.org>

It's easier to read.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-Id: <20200228192415.19867-2-alex.bennee@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---

accel/tcg/translate-all.c | 19 ++++++++++---------

1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c

index XXXXXXX..XXXXXXX 100644

--- a/accel/tcg/translate-all.c

+++ b/accel/tcg/translate-all.c

@@ -XXX,XX +XXX,XX @@

*/

#include "qemu/osdep.h"

+#include "qemu/units.h"

#include "qemu-common.h"

#define NO_CPU_IO_DEFS

@@ -XXX,XX +XXX,XX @@ static void page_lock_pair(PageDesc **ret_p1, tb_page_addr_t phys1,

/* Minimum size of the code gen buffer. This number is randomly chosen,

but not so small that we can't have a fair number of TB's live. */

-#define MIN_CODE_GEN_BUFFER_SIZE (1024u * 1024)

+#define MIN_CODE_GEN_BUFFER_SIZE (1 * MiB)

/* Maximum size of the code gen buffer we'd like to use. Unless otherwise

indicated, this is constrained by the range of direct branches on the

host cpu, as used by the TCG implementation of goto_tb. */

#if defined(__x86_64__)

-# define MAX_CODE_GEN_BUFFER_SIZE (2ul * 1024 * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (2 * GiB)

#elif defined(__sparc__)

-# define MAX_CODE_GEN_BUFFER_SIZE (2ul * 1024 * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (2 * GiB)

#elif defined(__powerpc64__)

-# define MAX_CODE_GEN_BUFFER_SIZE (2ul * 1024 * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (2 * GiB)

#elif defined(__powerpc__)

-# define MAX_CODE_GEN_BUFFER_SIZE (32u * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (32 * MiB)

#elif defined(__aarch64__)

-# define MAX_CODE_GEN_BUFFER_SIZE (2ul * 1024 * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (2 * GiB)

#elif defined(__s390x__)

/* We have a +- 4GB range on the branches; leave some slop. */

-# define MAX_CODE_GEN_BUFFER_SIZE (3ul * 1024 * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (3 * GiB)

#elif defined(__mips__)

/* We have a 256MB branch region, but leave room to make sure the

main executable is also within that region. */

-# define MAX_CODE_GEN_BUFFER_SIZE (128ul * 1024 * 1024)

+# define MAX_CODE_GEN_BUFFER_SIZE (128 * MiB)

#else

# define MAX_CODE_GEN_BUFFER_SIZE ((size_t)-1)

#endif

-#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (32u * 1024 * 1024)

+#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (32 * MiB)

#define DEFAULT_CODE_GEN_BUFFER_SIZE \

(DEFAULT_CODE_GEN_BUFFER_SIZE_1 < MAX_CODE_GEN_BUFFER_SIZE \

--

2.20.1

From: Alex Bennée <alex.bennee@linaro.org>

Basing the TB cache size on the ram_size was always a little heuristic

and was broken by a1b18df9a4 which caused ram_size not to be fully

realised at the time we initialise the TCG translation cache.

The current DEFAULT_CODE_GEN_BUFFER_SIZE may still be a little small

but follow-up patches will address that.

Fixes: a1b18df9a4

Cc: Igor Mammedov <imammedo@redhat.com>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Message-Id: <20200228192415.19867-3-alex.bennee@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---

accel/tcg/translate-all.c | 8 --------

1 file changed, 8 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c

index XXXXXXX..XXXXXXX 100644

--- a/accel/tcg/translate-all.c

+++ b/accel/tcg/translate-all.c

@@ -XXX,XX +XXX,XX @@ static inline size_t size_code_gen_buffer(size_t tb_size)

{

/* Size the buffer. */

if (tb_size == 0) {

-#ifdef USE_STATIC_CODE_GEN_BUFFER

tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE;

-#else

- /* ??? Needs adjustments. */

- /* ??? If we relax the requirement that CONFIG_USER_ONLY use the

- static buffer, we could size this on RESERVED_VA, on the text

- segment size of the executable, or continue to use the default. */

- tb_size = (unsigned long)(ram_size / 4);

-#endif

}

if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) {

tb_size = MIN_CODE_GEN_BUFFER_SIZE;

--

2.20.1