* [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h"
@ 2021-02-07 23:23 Philippe Mathieu-Daudé
2021-02-07 23:23 ` [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() Philippe Mathieu-Daudé
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato,
Sagar Karandikar, Bastian Koppelmann, Richard Henderson,
Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm,
Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo,
Aurelien Jarno
Since v1:
- Do not move tlb_vaddr_to_host()
Hi,
I wondered why changing something in "tcg/tcg.h" would trigger
rebuilding the whole tree and figured the inclusion in
"exec/cpu_ldst.h".
By making tlb_addr_write() static to accel/tcg/cputlb.c we can
remove the "tcg/tcg.h" inclusion and reduce the number of objects
to rebuild.
I added tlb_assert_iotlb_entry_for_ptr_present() but there is
this comment in target/arm/mte_helper.c which I don't understand
much (so have no clue how to fix this TODO) but I suppose this
would be to add a proper implementation and not need this ugly
tlb_assert_iotlb_entry_for_ptr_present():
* TODO: Perhaps there should be a cputlb helper that returns a
* matching tlb entry + iotlb entry.
Regards,
Phil.
Philippe Mathieu-Daudé (6):
target: Replace tcg_debug_assert() by assert()
target/m68k: Include missing "tcg/tcg.h" header
target/mips: Include missing "tcg/tcg.h" header
accel/tcg: Include missing "tcg/tcg.h" header
accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present()
exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h"
include/exec/cpu_ldst.h | 28 -------------------
include/exec/exec-all.h | 25 +++++++++++++++++
target/arm/translate.h | 4 +--
accel/tcg/cputlb.c | 23 ++++++++++++++++
accel/tcg/tcg-accel-ops-mttcg.c | 1 +
accel/tcg/tcg-accel-ops-rr.c | 1 +
target/arm/mte_helper.c | 15 +++--------
target/arm/sve_helper.c | 18 +++++--------
target/arm/translate-a64.c | 12 ++++-----
target/arm/translate-sve.c | 4 +--
target/arm/translate.c | 36 ++++++++++++-------------
target/hppa/translate.c | 4 +--
target/m68k/op_helper.c | 1 +
target/mips/msa_helper.c | 1 +
target/rx/op_helper.c | 6 ++---
target/rx/translate.c | 14 +++++-----
target/sh4/translate.c | 4 +--
target/riscv/insn_trans/trans_rvv.c.inc | 2 +-
18 files changed, 105 insertions(+), 94 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé @ 2021-02-07 23:23 ` Philippe Mathieu-Daudé 2021-02-08 13:54 ` Alex Bennée 2021-02-07 23:23 ` [PATCH v2 2/6] target/m68k: Include missing "tcg/tcg.h" header Philippe Mathieu-Daudé ` (4 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm, Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo, Aurelien Jarno Since commit 262a69f4282 ("osdep.h: Prohibit disabling assert() in supported builds") we can not build QEMU with assert() disabled. tcg_debug_assert() does nothing until QEMU is configured with --enable-debug-tcg. Since there is no obvious logic whether to use tcg_debug_assert() or assert() for files under target/, simplify by using plain assert() everywhere. Keep tcg_debug_assert() for the tcg/ and accel/ directories. Patch created mechanically using: $ sed -i s/tcg_debug_assert/assert/ \ $(git grep -l tcg_debug_assert target/) Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- If there is a logic, we should document it, and include "tcg/tcg.h" in these files. --- target/arm/translate.h | 4 +-- target/arm/mte_helper.c | 4 +-- target/arm/sve_helper.c | 8 +++--- target/arm/translate-a64.c | 12 ++++----- target/arm/translate-sve.c | 4 +-- target/arm/translate.c | 36 ++++++++++++------------- target/hppa/translate.c | 4 +-- target/rx/op_helper.c | 6 ++--- target/rx/translate.c | 14 +++++----- target/sh4/translate.c | 4 +-- target/riscv/insn_trans/trans_rvv.c.inc | 2 +- 11 files changed, 49 insertions(+), 49 deletions(-) diff --git a/target/arm/translate.h b/target/arm/translate.h index 423b0e08df0..e2ddf87629c 100644 --- a/target/arm/translate.h +++ b/target/arm/translate.h @@ -220,7 +220,7 @@ static inline void set_pstate_bits(uint32_t bits) { TCGv_i32 p = tcg_temp_new_i32(); - tcg_debug_assert(!(bits & CACHED_PSTATE_BITS)); + assert(!(bits & CACHED_PSTATE_BITS)); tcg_gen_ld_i32(p, cpu_env, offsetof(CPUARMState, pstate)); tcg_gen_ori_i32(p, p, bits); @@ -233,7 +233,7 @@ static inline void clear_pstate_bits(uint32_t bits) { TCGv_i32 p = tcg_temp_new_i32(); - tcg_debug_assert(!(bits & CACHED_PSTATE_BITS)); + assert(!(bits & CACHED_PSTATE_BITS)); tcg_gen_ld_i32(p, cpu_env, offsetof(CPUARMState, pstate)); tcg_gen_andi_i32(p, p, ~bits); diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c index 153bd1e9df8..6cea9d1b506 100644 --- a/target/arm/mte_helper.c +++ b/target/arm/mte_helper.c @@ -166,8 +166,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx, * not set in the cputlb lookup above. */ mr = memory_region_from_host(host, &ptr_ra); - tcg_debug_assert(mr != NULL); - tcg_debug_assert(memory_region_is_ram(mr)); + assert(mr != NULL); + assert(memory_region_is_ram(mr)); ptr_paddr = ptr_ra; do { ptr_paddr += mr->addr; diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c index 844db08bd57..c8cdf7618eb 100644 --- a/target/arm/sve_helper.c +++ b/target/arm/sve_helper.c @@ -4030,7 +4030,7 @@ static intptr_t find_next_active(uint64_t *vg, intptr_t reg_off, reg_off += ctz64(pg); /* We should never see an out of range predicate bit set. */ - tcg_debug_assert(reg_off < reg_max); + assert(reg_off < reg_max); return reg_off; } @@ -4186,7 +4186,7 @@ static bool sve_cont_ldst_elements(SVEContLdSt *info, target_ulong addr, /* No active elements, no pages touched. */ return false; } - tcg_debug_assert(reg_off_last >= 0 && reg_off_last < reg_max); + assert(reg_off_last >= 0 && reg_off_last < reg_max); info->reg_off_first[0] = reg_off_first; info->mem_off_first[0] = (reg_off_first >> esz) * msize; @@ -4235,7 +4235,7 @@ static bool sve_cont_ldst_elements(SVEContLdSt *info, target_ulong addr, * this may affect the address reported in an exception. */ reg_off_split = find_next_active(vg, reg_off_split, reg_max, esz); - tcg_debug_assert(reg_off_split <= reg_off_last); + assert(reg_off_split <= reg_off_last); info->reg_off_first[1] = reg_off_split; info->mem_off_first[1] = (reg_off_split >> esz) * msize; info->reg_off_last[1] = reg_off_last; @@ -4794,7 +4794,7 @@ void sve_ldnfff1_r(CPUARMState *env, void *vg, const target_ulong addr, /* Probe the page(s). */ if (!sve_cont_ldst_pages(&info, fault, env, addr, MMU_DATA_LOAD, retaddr)) { /* Fault on first element. */ - tcg_debug_assert(fault == FAULT_NO); + assert(fault == FAULT_NO); memset(vd, 0, reg_max); goto do_fault; } diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index ffc060e5d70..f570506133c 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -144,7 +144,7 @@ static void set_btype(DisasContext *s, int val) TCGv_i32 tcg_val; /* BTYPE is a 2-bit field, and 0 should be done with reset_btype. */ - tcg_debug_assert(val >= 1 && val <= 3); + assert(val >= 1 && val <= 3); tcg_val = tcg_const_i32(val); tcg_gen_st_i32(tcg_val, cpu_env, offsetof(CPUARMState, btype)); @@ -10659,7 +10659,7 @@ static void handle_vec_simd_shri(DisasContext *s, bool is_q, bool is_u, unallocated_encoding(s); return; } - tcg_debug_assert(size <= 3); + assert(size <= 3); if (!fp_access_check(s)) { return; @@ -12812,7 +12812,7 @@ static void disas_simd_two_reg_misc(DisasContext *s, uint32_t insn) /* Coverity claims (size == 3 && !is_q) has been eliminated * from all paths leading to here. */ - tcg_debug_assert(is_q); + assert(is_q); for (pass = 0; pass < 2; pass++) { TCGv_i64 tcg_op = tcg_temp_new_i64(); TCGv_i64 tcg_res = tcg_temp_new_i64(); @@ -14615,7 +14615,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s) s->guarded_page = is_guarded_page(env, s); /* First insn can have btype set to non-zero. */ - tcg_debug_assert(s->btype >= 0); + assert(s->btype >= 0); /* * Note that the Branch Target Exception has fairly high @@ -14633,7 +14633,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s) } } else { /* Not the first insn: btype must be 0. */ - tcg_debug_assert(s->btype == 0); + assert(s->btype == 0); } } @@ -14733,7 +14733,7 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase, #ifdef CONFIG_USER_ONLY /* In sve_probe_page, we assume TBI is enabled. */ - tcg_debug_assert(dc->tbid & 1); + assert(dc->tbid & 1); #endif /* Single step state. The code-generation logic here is: diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c index 27402af23c0..a1e327f863e 100644 --- a/target/arm/translate-sve.c +++ b/target/arm/translate-sve.c @@ -3938,8 +3938,8 @@ static bool trans_FCMLA_zzxz(DisasContext *s, arg_FCMLA_zzxz *a) gen_helper_gvec_fcmlas_idx, }; - tcg_debug_assert(a->esz == 1 || a->esz == 2); - tcg_debug_assert(a->rd == a->ra); + assert(a->esz == 1 || a->esz == 2); + assert(a->rd == a->ra); if (sve_access_check(s)) { unsigned vsz = vec_full_reg_size(s); TCGv_ptr status = fpstatus_ptr(a->esz == MO_16 ? FPST_FPCR_F16 : FPST_FPCR); diff --git a/target/arm/translate.c b/target/arm/translate.c index 1653cca1aaa..04ebfcc0d6d 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -2972,7 +2972,7 @@ void gen_gvec_sqrdmlah_qc(unsigned vece, uint32_t rd_ofs, uint32_t rn_ofs, static gen_helper_gvec_3_ptr * const fns[2] = { gen_helper_gvec_qrdmlah_s16, gen_helper_gvec_qrdmlah_s32 }; - tcg_debug_assert(vece >= 1 && vece <= 2); + assert(vece >= 1 && vece <= 2); gen_gvec_fn3_qc(rd_ofs, rn_ofs, rm_ofs, opr_sz, max_sz, fns[vece - 1]); } @@ -2982,7 +2982,7 @@ void gen_gvec_sqrdmlsh_qc(unsigned vece, uint32_t rd_ofs, uint32_t rn_ofs, static gen_helper_gvec_3_ptr * const fns[2] = { gen_helper_gvec_qrdmlsh_s16, gen_helper_gvec_qrdmlsh_s32 }; - tcg_debug_assert(vece >= 1 && vece <= 2); + assert(vece >= 1 && vece <= 2); gen_gvec_fn3_qc(rd_ofs, rn_ofs, rm_ofs, opr_sz, max_sz, fns[vece - 1]); } @@ -3105,8 +3105,8 @@ void gen_gvec_ssra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize]. */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); /* * Shifts larger than the element size are architecturally valid. @@ -3181,8 +3181,8 @@ void gen_gvec_usra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize]. */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); /* * Shifts larger than the element size are architecturally valid. @@ -3290,8 +3290,8 @@ void gen_gvec_srshr(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize] */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); if (shift == (8 << vece)) { /* @@ -3386,8 +3386,8 @@ void gen_gvec_srsra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize] */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); /* * Shifts larger than the element size are architecturally valid. @@ -3491,8 +3491,8 @@ void gen_gvec_urshr(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize] */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); if (shift == (8 << vece)) { /* @@ -3606,8 +3606,8 @@ void gen_gvec_ursra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize] */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); tcg_gen_gvec_2i(rd_ofs, rm_ofs, opr_sz, max_sz, shift, &ops[vece]); } @@ -3695,8 +3695,8 @@ void gen_gvec_sri(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [1..esize]. */ - tcg_debug_assert(shift > 0); - tcg_debug_assert(shift <= (8 << vece)); + assert(shift > 0); + assert(shift <= (8 << vece)); /* Shift of esize leaves destination unchanged. */ if (shift < (8 << vece)) { @@ -3788,8 +3788,8 @@ void gen_gvec_sli(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, }; /* tszimm encoding produces immediates in the range [0..esize-1]. */ - tcg_debug_assert(shift >= 0); - tcg_debug_assert(shift < (8 << vece)); + assert(shift >= 0); + assert(shift < (8 << vece)); if (shift == 0) { tcg_gen_gvec_mov(vece, rd_ofs, rm_ofs, opr_sz, max_sz); diff --git a/target/hppa/translate.c b/target/hppa/translate.c index 64af1e0d5cc..ceb3bacc7dd 100644 --- a/target/hppa/translate.c +++ b/target/hppa/translate.c @@ -1945,8 +1945,8 @@ static bool do_ibranch(DisasContext *ctx, TCGv_reg dest, for the indirect branch consumes no special resources, we can (conditionally) skip B and continue execution. */ /* The use_nullify_skip test implies we have a known control path. */ - tcg_debug_assert(ctx->iaoq_b != -1); - tcg_debug_assert(ctx->iaoq_n != -1); + assert(ctx->iaoq_b != -1); + assert(ctx->iaoq_n != -1); /* We do have to handle the non-local temporary, DEST, before branching. Since IOAQ_F is not really live at this point, we diff --git a/target/rx/op_helper.c b/target/rx/op_helper.c index 4d315b44492..03d285fbafe 100644 --- a/target/rx/op_helper.c +++ b/target/rx/op_helper.c @@ -234,7 +234,7 @@ static void (* const cpu_stfn[])(CPUArchState *env, void helper_sstr(CPURXState *env, uint32_t sz) { - tcg_debug_assert(sz < 3); + assert(sz < 3); while (env->regs[3] != 0) { cpu_stfn[sz](env, env->regs[1], env->regs[2], GETPC()); env->regs[1] += 1 << sz; @@ -283,7 +283,7 @@ void helper_smovb(CPURXState *env) void helper_suntil(CPURXState *env, uint32_t sz) { uint32_t tmp; - tcg_debug_assert(sz < 3); + assert(sz < 3); if (env->regs[3] == 0) { return ; } @@ -302,7 +302,7 @@ void helper_suntil(CPURXState *env, uint32_t sz) void helper_swhile(CPURXState *env, uint32_t sz) { uint32_t tmp; - tcg_debug_assert(sz < 3); + assert(sz < 3); if (env->regs[3] == 0) { return ; } diff --git a/target/rx/translate.c b/target/rx/translate.c index 9ea941c6302..ff12af4f7f8 100644 --- a/target/rx/translate.c +++ b/target/rx/translate.c @@ -87,7 +87,7 @@ static uint32_t li(DisasContext *ctx, int sz) CPURXState *env = ctx->env; addr = ctx->base.pc_next; - tcg_debug_assert(sz < 4); + assert(sz < 4); switch (sz) { case 1: ctx->base.pc_next += 1; @@ -201,7 +201,7 @@ static inline TCGv rx_index_addr(DisasContext *ctx, TCGv mem, { uint32_t dsp; - tcg_debug_assert(ld < 3); + assert(ld < 3); switch (ld) { case 0: return cpu_regs[reg]; @@ -222,7 +222,7 @@ static inline TCGv rx_index_addr(DisasContext *ctx, TCGv mem, static inline MemOp mi_to_mop(unsigned mi) { static const MemOp mop[5] = { MO_SB, MO_SW, MO_UL, MO_UW, MO_UB }; - tcg_debug_assert(mi < 5); + assert(mi < 5); return mop[mi]; } @@ -258,7 +258,7 @@ static int is_privileged(DisasContext *ctx, int is_exception) /* generate QEMU condition */ static void psw_cond(DisasCompare *dc, uint32_t cond) { - tcg_debug_assert(cond < 16); + assert(cond < 16); switch (cond) { case 0: /* z */ dc->cond = TCG_COND_EQ; @@ -1401,7 +1401,7 @@ static inline void shiftr_imm(uint32_t rd, uint32_t rs, uint32_t imm, static void (* const gen_sXri[])(TCGv ret, TCGv arg1, int arg2) = { tcg_gen_shri_i32, tcg_gen_sari_i32, }; - tcg_debug_assert(alith < 2); + assert(alith < 2); if (imm) { gen_sXri[alith](cpu_regs[rd], cpu_regs[rs], imm - 1); tcg_gen_andi_i32(cpu_psw_c, cpu_regs[rd], 0x00000001); @@ -1425,7 +1425,7 @@ static inline void shiftr_reg(uint32_t rd, uint32_t rs, unsigned int alith) static void (* const gen_sXr[])(TCGv ret, TCGv arg1, TCGv arg2) = { tcg_gen_shr_i32, tcg_gen_sar_i32, }; - tcg_debug_assert(alith < 2); + assert(alith < 2); noshift = gen_new_label(); done = gen_new_label(); count = tcg_temp_new(); @@ -2282,7 +2282,7 @@ static bool trans_INT(DisasContext *ctx, arg_INT *a) { TCGv vec; - tcg_debug_assert(a->imm < 0x100); + assert(a->imm < 0x100); vec = tcg_const_i32(a->imm); tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next); gen_helper_rxint(cpu_env, vec); diff --git a/target/sh4/translate.c b/target/sh4/translate.c index 93127906237..f12cc0830bf 100644 --- a/target/sh4/translate.c +++ b/target/sh4/translate.c @@ -339,7 +339,7 @@ static void gen_delayed_conditional_jump(DisasContext * ctx) static inline void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg) { /* We have already signaled illegal instruction for odd Dr. */ - tcg_debug_assert((reg & 1) == 0); + assert((reg & 1) == 0); reg ^= ctx->fbank; tcg_gen_concat_i32_i64(t, cpu_fregs[reg + 1], cpu_fregs[reg]); } @@ -347,7 +347,7 @@ static inline void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg) static inline void gen_store_fpr64(DisasContext *ctx, TCGv_i64 t, int reg) { /* We have already signaled illegal instruction for odd Dr. */ - tcg_debug_assert((reg & 1) == 0); + assert((reg & 1) == 0); reg ^= ctx->fbank; tcg_gen_extr_i64_i32(cpu_fregs[reg + 1], cpu_fregs[reg], t); } diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc index 887c6b88831..f0e6e844f5f 100644 --- a/target/riscv/insn_trans/trans_rvv.c.inc +++ b/target/riscv/insn_trans/trans_rvv.c.inc @@ -992,7 +992,7 @@ static void tcg_gen_gvec_rsubs(unsigned vece, uint32_t dofs, uint32_t aofs, .vece = MO_64 }, }; - tcg_debug_assert(vece <= MO_64); + assert(vece <= MO_64); tcg_gen_gvec_2s(dofs, aofs, oprsz, maxsz, c, &rsub_op[vece]); } -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() 2021-02-07 23:23 ` [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() Philippe Mathieu-Daudé @ 2021-02-08 13:54 ` Alex Bennée 0 siblings, 0 replies; 15+ messages in thread From: Alex Bennée @ 2021-02-08 13:54 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, qemu-devel, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > Since commit 262a69f4282 ("osdep.h: Prohibit disabling assert() > in supported builds") we can not build QEMU with assert() disabled. > > tcg_debug_assert() does nothing until QEMU is configured with > --enable-debug-tcg. > > Since there is no obvious logic whether to use tcg_debug_assert() > or assert() for files under target/, simplify by using plain > assert() everywhere. Keep tcg_debug_assert() for the tcg/ and > accel/ directories. I think generally tcg_debug_asserts are those that: - assert the TCG logic is working correctly - would impose a cost on the hotpath if run in "production" Arguably all of these are on the translation front-end side and are guards against changes being made to the front-end that violate the API contract with the rest of the code. They are unlikely to be triggered by running different code. Usually these mask or zero checks are fairly cheap on modern hardware but we can hit these checks fairly frequently. Have you measured any performance change? > > Patch created mechanically using: > > $ sed -i s/tcg_debug_assert/assert/ \ > $(git grep -l tcg_debug_assert target/) > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > If there is a logic, we should document it, and include "tcg/tcg.h" > in these files. > --- > target/arm/translate.h | 4 +-- > target/arm/mte_helper.c | 4 +-- > target/arm/sve_helper.c | 8 +++--- > target/arm/translate-a64.c | 12 ++++----- > target/arm/translate-sve.c | 4 +-- > target/arm/translate.c | 36 ++++++++++++------------- > target/hppa/translate.c | 4 +-- > target/rx/op_helper.c | 6 ++--- > target/rx/translate.c | 14 +++++----- > target/sh4/translate.c | 4 +-- > target/riscv/insn_trans/trans_rvv.c.inc | 2 +- > 11 files changed, 49 insertions(+), 49 deletions(-) > > diff --git a/target/arm/translate.h b/target/arm/translate.h > index 423b0e08df0..e2ddf87629c 100644 > --- a/target/arm/translate.h > +++ b/target/arm/translate.h > @@ -220,7 +220,7 @@ static inline void set_pstate_bits(uint32_t bits) > { > TCGv_i32 p = tcg_temp_new_i32(); > > - tcg_debug_assert(!(bits & CACHED_PSTATE_BITS)); > + assert(!(bits & CACHED_PSTATE_BITS)); > > tcg_gen_ld_i32(p, cpu_env, offsetof(CPUARMState, pstate)); > tcg_gen_ori_i32(p, p, bits); > @@ -233,7 +233,7 @@ static inline void clear_pstate_bits(uint32_t bits) > { > TCGv_i32 p = tcg_temp_new_i32(); > > - tcg_debug_assert(!(bits & CACHED_PSTATE_BITS)); > + assert(!(bits & CACHED_PSTATE_BITS)); > > tcg_gen_ld_i32(p, cpu_env, offsetof(CPUARMState, pstate)); > tcg_gen_andi_i32(p, p, ~bits); > diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c > index 153bd1e9df8..6cea9d1b506 100644 > --- a/target/arm/mte_helper.c > +++ b/target/arm/mte_helper.c > @@ -166,8 +166,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx, > * not set in the cputlb lookup above. > */ > mr = memory_region_from_host(host, &ptr_ra); > - tcg_debug_assert(mr != NULL); > - tcg_debug_assert(memory_region_is_ram(mr)); > + assert(mr != NULL); > + assert(memory_region_is_ram(mr)); > ptr_paddr = ptr_ra; > do { > ptr_paddr += mr->addr; > diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c > index 844db08bd57..c8cdf7618eb 100644 > --- a/target/arm/sve_helper.c > +++ b/target/arm/sve_helper.c > @@ -4030,7 +4030,7 @@ static intptr_t find_next_active(uint64_t *vg, intptr_t reg_off, > reg_off += ctz64(pg); > > /* We should never see an out of range predicate bit set. */ > - tcg_debug_assert(reg_off < reg_max); > + assert(reg_off < reg_max); > return reg_off; > } > > @@ -4186,7 +4186,7 @@ static bool sve_cont_ldst_elements(SVEContLdSt *info, target_ulong addr, > /* No active elements, no pages touched. */ > return false; > } > - tcg_debug_assert(reg_off_last >= 0 && reg_off_last < reg_max); > + assert(reg_off_last >= 0 && reg_off_last < reg_max); > > info->reg_off_first[0] = reg_off_first; > info->mem_off_first[0] = (reg_off_first >> esz) * msize; > @@ -4235,7 +4235,7 @@ static bool sve_cont_ldst_elements(SVEContLdSt *info, target_ulong addr, > * this may affect the address reported in an exception. > */ > reg_off_split = find_next_active(vg, reg_off_split, reg_max, esz); > - tcg_debug_assert(reg_off_split <= reg_off_last); > + assert(reg_off_split <= reg_off_last); > info->reg_off_first[1] = reg_off_split; > info->mem_off_first[1] = (reg_off_split >> esz) * msize; > info->reg_off_last[1] = reg_off_last; > @@ -4794,7 +4794,7 @@ void sve_ldnfff1_r(CPUARMState *env, void *vg, const target_ulong addr, > /* Probe the page(s). */ > if (!sve_cont_ldst_pages(&info, fault, env, addr, MMU_DATA_LOAD, retaddr)) { > /* Fault on first element. */ > - tcg_debug_assert(fault == FAULT_NO); > + assert(fault == FAULT_NO); > memset(vd, 0, reg_max); > goto do_fault; > } > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index ffc060e5d70..f570506133c 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -144,7 +144,7 @@ static void set_btype(DisasContext *s, int val) > TCGv_i32 tcg_val; > > /* BTYPE is a 2-bit field, and 0 should be done with reset_btype. */ > - tcg_debug_assert(val >= 1 && val <= 3); > + assert(val >= 1 && val <= 3); > > tcg_val = tcg_const_i32(val); > tcg_gen_st_i32(tcg_val, cpu_env, offsetof(CPUARMState, btype)); > @@ -10659,7 +10659,7 @@ static void handle_vec_simd_shri(DisasContext *s, bool is_q, bool is_u, > unallocated_encoding(s); > return; > } > - tcg_debug_assert(size <= 3); > + assert(size <= 3); > > if (!fp_access_check(s)) { > return; > @@ -12812,7 +12812,7 @@ static void disas_simd_two_reg_misc(DisasContext *s, uint32_t insn) > /* Coverity claims (size == 3 && !is_q) has been eliminated > * from all paths leading to here. > */ > - tcg_debug_assert(is_q); > + assert(is_q); > for (pass = 0; pass < 2; pass++) { > TCGv_i64 tcg_op = tcg_temp_new_i64(); > TCGv_i64 tcg_res = tcg_temp_new_i64(); > @@ -14615,7 +14615,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s) > s->guarded_page = is_guarded_page(env, s); > > /* First insn can have btype set to non-zero. */ > - tcg_debug_assert(s->btype >= 0); > + assert(s->btype >= 0); > > /* > * Note that the Branch Target Exception has fairly high > @@ -14633,7 +14633,7 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s) > } > } else { > /* Not the first insn: btype must be 0. */ > - tcg_debug_assert(s->btype == 0); > + assert(s->btype == 0); > } > } > > @@ -14733,7 +14733,7 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase, > > #ifdef CONFIG_USER_ONLY > /* In sve_probe_page, we assume TBI is enabled. */ > - tcg_debug_assert(dc->tbid & 1); > + assert(dc->tbid & 1); > #endif > > /* Single step state. The code-generation logic here is: > diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c > index 27402af23c0..a1e327f863e 100644 > --- a/target/arm/translate-sve.c > +++ b/target/arm/translate-sve.c > @@ -3938,8 +3938,8 @@ static bool trans_FCMLA_zzxz(DisasContext *s, arg_FCMLA_zzxz *a) > gen_helper_gvec_fcmlas_idx, > }; > > - tcg_debug_assert(a->esz == 1 || a->esz == 2); > - tcg_debug_assert(a->rd == a->ra); > + assert(a->esz == 1 || a->esz == 2); > + assert(a->rd == a->ra); > if (sve_access_check(s)) { > unsigned vsz = vec_full_reg_size(s); > TCGv_ptr status = fpstatus_ptr(a->esz == MO_16 ? FPST_FPCR_F16 : FPST_FPCR); > diff --git a/target/arm/translate.c b/target/arm/translate.c > index 1653cca1aaa..04ebfcc0d6d 100644 > --- a/target/arm/translate.c > +++ b/target/arm/translate.c > @@ -2972,7 +2972,7 @@ void gen_gvec_sqrdmlah_qc(unsigned vece, uint32_t rd_ofs, uint32_t rn_ofs, > static gen_helper_gvec_3_ptr * const fns[2] = { > gen_helper_gvec_qrdmlah_s16, gen_helper_gvec_qrdmlah_s32 > }; > - tcg_debug_assert(vece >= 1 && vece <= 2); > + assert(vece >= 1 && vece <= 2); > gen_gvec_fn3_qc(rd_ofs, rn_ofs, rm_ofs, opr_sz, max_sz, fns[vece - 1]); > } > > @@ -2982,7 +2982,7 @@ void gen_gvec_sqrdmlsh_qc(unsigned vece, uint32_t rd_ofs, uint32_t rn_ofs, > static gen_helper_gvec_3_ptr * const fns[2] = { > gen_helper_gvec_qrdmlsh_s16, gen_helper_gvec_qrdmlsh_s32 > }; > - tcg_debug_assert(vece >= 1 && vece <= 2); > + assert(vece >= 1 && vece <= 2); > gen_gvec_fn3_qc(rd_ofs, rn_ofs, rm_ofs, opr_sz, max_sz, fns[vece - 1]); > } > > @@ -3105,8 +3105,8 @@ void gen_gvec_ssra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize]. */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > /* > * Shifts larger than the element size are architecturally valid. > @@ -3181,8 +3181,8 @@ void gen_gvec_usra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize]. */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > /* > * Shifts larger than the element size are architecturally valid. > @@ -3290,8 +3290,8 @@ void gen_gvec_srshr(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize] */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > if (shift == (8 << vece)) { > /* > @@ -3386,8 +3386,8 @@ void gen_gvec_srsra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize] */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > /* > * Shifts larger than the element size are architecturally valid. > @@ -3491,8 +3491,8 @@ void gen_gvec_urshr(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize] */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > if (shift == (8 << vece)) { > /* > @@ -3606,8 +3606,8 @@ void gen_gvec_ursra(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize] */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > tcg_gen_gvec_2i(rd_ofs, rm_ofs, opr_sz, max_sz, shift, &ops[vece]); > } > @@ -3695,8 +3695,8 @@ void gen_gvec_sri(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [1..esize]. */ > - tcg_debug_assert(shift > 0); > - tcg_debug_assert(shift <= (8 << vece)); > + assert(shift > 0); > + assert(shift <= (8 << vece)); > > /* Shift of esize leaves destination unchanged. */ > if (shift < (8 << vece)) { > @@ -3788,8 +3788,8 @@ void gen_gvec_sli(unsigned vece, uint32_t rd_ofs, uint32_t rm_ofs, > }; > > /* tszimm encoding produces immediates in the range [0..esize-1]. */ > - tcg_debug_assert(shift >= 0); > - tcg_debug_assert(shift < (8 << vece)); > + assert(shift >= 0); > + assert(shift < (8 << vece)); > > if (shift == 0) { > tcg_gen_gvec_mov(vece, rd_ofs, rm_ofs, opr_sz, max_sz); > diff --git a/target/hppa/translate.c b/target/hppa/translate.c > index 64af1e0d5cc..ceb3bacc7dd 100644 > --- a/target/hppa/translate.c > +++ b/target/hppa/translate.c > @@ -1945,8 +1945,8 @@ static bool do_ibranch(DisasContext *ctx, TCGv_reg dest, > for the indirect branch consumes no special resources, we > can (conditionally) skip B and continue execution. */ > /* The use_nullify_skip test implies we have a known control path. */ > - tcg_debug_assert(ctx->iaoq_b != -1); > - tcg_debug_assert(ctx->iaoq_n != -1); > + assert(ctx->iaoq_b != -1); > + assert(ctx->iaoq_n != -1); > > /* We do have to handle the non-local temporary, DEST, before > branching. Since IOAQ_F is not really live at this point, we > diff --git a/target/rx/op_helper.c b/target/rx/op_helper.c > index 4d315b44492..03d285fbafe 100644 > --- a/target/rx/op_helper.c > +++ b/target/rx/op_helper.c > @@ -234,7 +234,7 @@ static void (* const cpu_stfn[])(CPUArchState *env, > > void helper_sstr(CPURXState *env, uint32_t sz) > { > - tcg_debug_assert(sz < 3); > + assert(sz < 3); > while (env->regs[3] != 0) { > cpu_stfn[sz](env, env->regs[1], env->regs[2], GETPC()); > env->regs[1] += 1 << sz; > @@ -283,7 +283,7 @@ void helper_smovb(CPURXState *env) > void helper_suntil(CPURXState *env, uint32_t sz) > { > uint32_t tmp; > - tcg_debug_assert(sz < 3); > + assert(sz < 3); > if (env->regs[3] == 0) { > return ; > } > @@ -302,7 +302,7 @@ void helper_suntil(CPURXState *env, uint32_t sz) > void helper_swhile(CPURXState *env, uint32_t sz) > { > uint32_t tmp; > - tcg_debug_assert(sz < 3); > + assert(sz < 3); > if (env->regs[3] == 0) { > return ; > } > diff --git a/target/rx/translate.c b/target/rx/translate.c > index 9ea941c6302..ff12af4f7f8 100644 > --- a/target/rx/translate.c > +++ b/target/rx/translate.c > @@ -87,7 +87,7 @@ static uint32_t li(DisasContext *ctx, int sz) > CPURXState *env = ctx->env; > addr = ctx->base.pc_next; > > - tcg_debug_assert(sz < 4); > + assert(sz < 4); > switch (sz) { > case 1: > ctx->base.pc_next += 1; > @@ -201,7 +201,7 @@ static inline TCGv rx_index_addr(DisasContext *ctx, TCGv mem, > { > uint32_t dsp; > > - tcg_debug_assert(ld < 3); > + assert(ld < 3); > switch (ld) { > case 0: > return cpu_regs[reg]; > @@ -222,7 +222,7 @@ static inline TCGv rx_index_addr(DisasContext *ctx, TCGv mem, > static inline MemOp mi_to_mop(unsigned mi) > { > static const MemOp mop[5] = { MO_SB, MO_SW, MO_UL, MO_UW, MO_UB }; > - tcg_debug_assert(mi < 5); > + assert(mi < 5); > return mop[mi]; > } > > @@ -258,7 +258,7 @@ static int is_privileged(DisasContext *ctx, int is_exception) > /* generate QEMU condition */ > static void psw_cond(DisasCompare *dc, uint32_t cond) > { > - tcg_debug_assert(cond < 16); > + assert(cond < 16); > switch (cond) { > case 0: /* z */ > dc->cond = TCG_COND_EQ; > @@ -1401,7 +1401,7 @@ static inline void shiftr_imm(uint32_t rd, uint32_t rs, uint32_t imm, > static void (* const gen_sXri[])(TCGv ret, TCGv arg1, int arg2) = { > tcg_gen_shri_i32, tcg_gen_sari_i32, > }; > - tcg_debug_assert(alith < 2); > + assert(alith < 2); > if (imm) { > gen_sXri[alith](cpu_regs[rd], cpu_regs[rs], imm - 1); > tcg_gen_andi_i32(cpu_psw_c, cpu_regs[rd], 0x00000001); > @@ -1425,7 +1425,7 @@ static inline void shiftr_reg(uint32_t rd, uint32_t rs, unsigned int alith) > static void (* const gen_sXr[])(TCGv ret, TCGv arg1, TCGv arg2) = { > tcg_gen_shr_i32, tcg_gen_sar_i32, > }; > - tcg_debug_assert(alith < 2); > + assert(alith < 2); > noshift = gen_new_label(); > done = gen_new_label(); > count = tcg_temp_new(); > @@ -2282,7 +2282,7 @@ static bool trans_INT(DisasContext *ctx, arg_INT *a) > { > TCGv vec; > > - tcg_debug_assert(a->imm < 0x100); > + assert(a->imm < 0x100); > vec = tcg_const_i32(a->imm); > tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next); > gen_helper_rxint(cpu_env, vec); > diff --git a/target/sh4/translate.c b/target/sh4/translate.c > index 93127906237..f12cc0830bf 100644 > --- a/target/sh4/translate.c > +++ b/target/sh4/translate.c > @@ -339,7 +339,7 @@ static void gen_delayed_conditional_jump(DisasContext * ctx) > static inline void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg) > { > /* We have already signaled illegal instruction for odd Dr. */ > - tcg_debug_assert((reg & 1) == 0); > + assert((reg & 1) == 0); > reg ^= ctx->fbank; > tcg_gen_concat_i32_i64(t, cpu_fregs[reg + 1], cpu_fregs[reg]); > } > @@ -347,7 +347,7 @@ static inline void gen_load_fpr64(DisasContext *ctx, TCGv_i64 t, int reg) > static inline void gen_store_fpr64(DisasContext *ctx, TCGv_i64 t, int reg) > { > /* We have already signaled illegal instruction for odd Dr. */ > - tcg_debug_assert((reg & 1) == 0); > + assert((reg & 1) == 0); > reg ^= ctx->fbank; > tcg_gen_extr_i64_i32(cpu_fregs[reg + 1], cpu_fregs[reg], t); > } > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc > index 887c6b88831..f0e6e844f5f 100644 > --- a/target/riscv/insn_trans/trans_rvv.c.inc > +++ b/target/riscv/insn_trans/trans_rvv.c.inc > @@ -992,7 +992,7 @@ static void tcg_gen_gvec_rsubs(unsigned vece, uint32_t dofs, uint32_t aofs, > .vece = MO_64 }, > }; > > - tcg_debug_assert(vece <= MO_64); > + assert(vece <= MO_64); > tcg_gen_gvec_2s(dofs, aofs, oprsz, maxsz, c, &rsub_op[vece]); > } -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/6] target/m68k: Include missing "tcg/tcg.h" header 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé 2021-02-07 23:23 ` [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() Philippe Mathieu-Daudé @ 2021-02-07 23:23 ` Philippe Mathieu-Daudé 2021-02-07 23:23 ` [PATCH v2 3/6] target/mips: " Philippe Mathieu-Daudé ` (3 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm, Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo, Aurelien Jarno Commit 14f944063af ("target-m68k: add cas/cas2 ops") introduced use of typedef/prototypes declared in "tcg/tcg.h" without including it. This was not a problem because "tcg/tcg.h" is pulled in by "exec/cpu_ldst.h". To be able to remove this header there, we first need to include it here in op_helper.c, else we get: [953/1018] Compiling C object libqemu-m68k-softmmu.fa.p/target_m68k_op_helper.c.o target/m68k/op_helper.c: In function ‘do_cas2l’: target/m68k/op_helper.c:774:5: error: unknown type name ‘TCGMemOpIdx’ 774 | TCGMemOpIdx oi; | ^~~~~~~~~~~ target/m68k/op_helper.c:787:18: error: implicit declaration of function ‘make_memop_idx’ [-Werror=implicit-function-declaration] 787 | oi = make_memop_idx(MO_BEQ, mmu_idx); | ^~~~~~~~~~~~~~ target/m68k/op_helper.c:787:18: error: nested extern declaration of ‘make_memop_idx’ [-Werror=nested-externs] target/m68k/op_helper.c:788:17: error: implicit declaration of function ‘helper_atomic_cmpxchgq_be_mmu’; did you mean ‘helper_atomic_cmpxchgq_be’? [-Werror=implicit-function-declaration] 788 | l = helper_atomic_cmpxchgq_be_mmu(env, a1, c, u, oi, ra); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | helper_atomic_cmpxchgq_be target/m68k/op_helper.c:788:17: error: nested extern declaration of ‘helper_atomic_cmpxchgq_be_mmu’ [-Werror=nested-externs] cc1: all warnings being treated as errors Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- target/m68k/op_helper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c index 202498deb51..36b68fd318f 100644 --- a/target/m68k/op_helper.c +++ b/target/m68k/op_helper.c @@ -18,6 +18,7 @@ */ #include "qemu/osdep.h" #include "cpu.h" +#include "tcg/tcg.h" #include "exec/helper-proto.h" #include "exec/exec-all.h" #include "exec/cpu_ldst.h" -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/6] target/mips: Include missing "tcg/tcg.h" header 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé 2021-02-07 23:23 ` [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() Philippe Mathieu-Daudé 2021-02-07 23:23 ` [PATCH v2 2/6] target/m68k: Include missing "tcg/tcg.h" header Philippe Mathieu-Daudé @ 2021-02-07 23:23 ` Philippe Mathieu-Daudé 2021-02-20 20:07 ` Philippe Mathieu-Daudé 2021-02-07 23:23 ` [PATCH v2 4/6] accel/tcg: " Philippe Mathieu-Daudé ` (2 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm, Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo, Aurelien Jarno Commit 83be6b54123 ("Fix MSA instructions LD.<B|H|W|D> on big endian host") introduced use of typedef/prototypes declared in "tcg/tcg.h" without including it. This was not a problem because "tcg/tcg.h" is pulled in by "exec/cpu_ldst.h". To be able to remove this header there, we first need to include it here in op_helper.c, else we get: [222/337] Compiling C object libqemu-mips-softmmu.fa.p/target_mips_msa_helper.c.o target/mips/msa_helper.c: In function ‘helper_msa_ld_b’: target/mips/msa_helper.c:8214:9: error: unknown type name ‘TCGMemOpIdx’ 8214 | TCGMemOpIdx oi = make_memop_idx(MO_TE | DF | MO_UNALN, \ | ^~~~~~~~~~~ target/mips/msa_helper.c:8224:5: note: in expansion of macro ‘MEMOP_IDX’ 8224 | MEMOP_IDX(DF_BYTE) | ^~~~~~~~~ target/mips/msa_helper.c:8214:26: error: implicit declaration of function ‘make_memop_idx’ [-Werror=implicit-function-declaration] 8214 | TCGMemOpIdx oi = make_memop_idx(MO_TE | DF | MO_UNALN, \ | ^~~~~~~~~~~~~~ target/mips/msa_helper.c:8227:18: error: implicit declaration of function ‘helper_ret_ldub_mmu’ [-Werror=implicit-function-declaration] 8227 | pwd->b[0] = helper_ret_ldub_mmu(env, addr + (0 << DF_BYTE), oi, GETPC()); | ^~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- target/mips/msa_helper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target/mips/msa_helper.c b/target/mips/msa_helper.c index 1298a1917ce..4caefe29ad7 100644 --- a/target/mips/msa_helper.c +++ b/target/mips/msa_helper.c @@ -20,6 +20,7 @@ #include "qemu/osdep.h" #include "cpu.h" #include "internal.h" +#include "tcg/tcg.h" #include "exec/exec-all.h" #include "exec/helper-proto.h" #include "exec/memop.h" -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/6] target/mips: Include missing "tcg/tcg.h" header 2021-02-07 23:23 ` [PATCH v2 3/6] target/mips: " Philippe Mathieu-Daudé @ 2021-02-20 20:07 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-20 20:07 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno On 2/8/21 12:23 AM, Philippe Mathieu-Daudé wrote: > Commit 83be6b54123 ("Fix MSA instructions LD.<B|H|W|D> on big endian > host") introduced use of typedef/prototypes declared in "tcg/tcg.h" > without including it. This was not a problem because "tcg/tcg.h" is > pulled in by "exec/cpu_ldst.h". To be able to remove this header > there, we first need to include it here in op_helper.c, else we get: > > [222/337] Compiling C object libqemu-mips-softmmu.fa.p/target_mips_msa_helper.c.o > target/mips/msa_helper.c: In function ‘helper_msa_ld_b’: > target/mips/msa_helper.c:8214:9: error: unknown type name ‘TCGMemOpIdx’ > 8214 | TCGMemOpIdx oi = make_memop_idx(MO_TE | DF | MO_UNALN, \ > | ^~~~~~~~~~~ > target/mips/msa_helper.c:8224:5: note: in expansion of macro ‘MEMOP_IDX’ > 8224 | MEMOP_IDX(DF_BYTE) > | ^~~~~~~~~ > target/mips/msa_helper.c:8214:26: error: implicit declaration of function ‘make_memop_idx’ [-Werror=implicit-function-declaration] > 8214 | TCGMemOpIdx oi = make_memop_idx(MO_TE | DF | MO_UNALN, \ > | ^~~~~~~~~~~~~~ > target/mips/msa_helper.c:8227:18: error: implicit declaration of function ‘helper_ret_ldub_mmu’ [-Werror=implicit-function-declaration] > 8227 | pwd->b[0] = helper_ret_ldub_mmu(env, addr + (0 << DF_BYTE), oi, GETPC()); > | ^~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/mips/msa_helper.c | 1 + > 1 file changed, 1 insertion(+) Thanks, applied to mips-next. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/6] accel/tcg: Include missing "tcg/tcg.h" header 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2021-02-07 23:23 ` [PATCH v2 3/6] target/mips: " Philippe Mathieu-Daudé @ 2021-02-07 23:23 ` Philippe Mathieu-Daudé 2021-02-08 14:36 ` Alex Bennée 2021-02-07 23:23 ` [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() Philippe Mathieu-Daudé 2021-02-07 23:23 ` [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h" Philippe Mathieu-Daudé 5 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm, Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo, Aurelien Jarno Commit 3468b59e18b ("tcg: enable multiple TCG contexts in softmmu") introduced use of typedef/prototypes declared in "tcg/tcg.h" without including it. This was not a problem because "tcg/tcg.h" is pulled in by "exec/cpu_ldst.h". To be able to remove this header there, we first need to include it here in op_helper.c, else we get: accel/tcg/tcg-accel-ops-mttcg.c: In function ‘mttcg_cpu_thread_fn’: accel/tcg/tcg-accel-ops-mttcg.c:52:5: error: implicit declaration of function ‘tcg_register_thread’; did you mean ‘rcu_register_thread’? [-Werror=implicit-function-declaration] 52 | tcg_register_thread(); | ^~~~~~~~~~~~~~~~~~~ | rcu_register_thread accel/tcg/tcg-accel-ops-mttcg.c:52:5: error: nested extern declaration of ‘tcg_register_thread’ [-Werror=nested-externs] cc1: all warnings being treated as errors accel/tcg/tcg-accel-ops-rr.c: In function ‘rr_cpu_thread_fn’: accel/tcg/tcg-accel-ops-rr.c:153:5: error: implicit declaration of function ‘tcg_register_thread’; did you mean ‘rcu_register_thread’? [-Werror=implicit-function-declaration] 153 | tcg_register_thread(); | ^~~~~~~~~~~~~~~~~~~ | rcu_register_thread accel/tcg/tcg-accel-ops-rr.c:153:5: error: nested extern declaration of ‘tcg_register_thread’ [-Werror=nested-externs] cc1: all warnings being treated as errors Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- accel/tcg/tcg-accel-ops-mttcg.c | 1 + accel/tcg/tcg-accel-ops-rr.c | 1 + 2 files changed, 2 insertions(+) diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c index 42973fb062b..ddbca6c5b8c 100644 --- a/accel/tcg/tcg-accel-ops-mttcg.c +++ b/accel/tcg/tcg-accel-ops-mttcg.c @@ -32,6 +32,7 @@ #include "exec/exec-all.h" #include "hw/boards.h" +#include "tcg/tcg.h" #include "tcg-accel-ops.h" #include "tcg-accel-ops-mttcg.h" diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c index 4a66055e0d7..1bb1d0f8f1c 100644 --- a/accel/tcg/tcg-accel-ops-rr.c +++ b/accel/tcg/tcg-accel-ops-rr.c @@ -32,6 +32,7 @@ #include "exec/exec-all.h" #include "hw/boards.h" +#include "tcg/tcg.h" #include "tcg-accel-ops.h" #include "tcg-accel-ops-rr.h" #include "tcg-accel-ops-icount.h" -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/6] accel/tcg: Include missing "tcg/tcg.h" header 2021-02-07 23:23 ` [PATCH v2 4/6] accel/tcg: " Philippe Mathieu-Daudé @ 2021-02-08 14:36 ` Alex Bennée 0 siblings, 0 replies; 15+ messages in thread From: Alex Bennée @ 2021-02-08 14:36 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, qemu-devel, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > Commit 3468b59e18b ("tcg: enable multiple TCG contexts in softmmu") > introduced use of typedef/prototypes declared in "tcg/tcg.h" without > including it. This was not a problem because "tcg/tcg.h" is pulled > in by "exec/cpu_ldst.h". To be able to remove this header there, we > first need to include it here in op_helper.c, else we get: > > accel/tcg/tcg-accel-ops-mttcg.c: In function ‘mttcg_cpu_thread_fn’: > accel/tcg/tcg-accel-ops-mttcg.c:52:5: error: implicit declaration of function ‘tcg_register_thread’; did you mean ‘rcu_register_thread’? [-Werror=implicit-function-declaration] > 52 | tcg_register_thread(); > | ^~~~~~~~~~~~~~~~~~~ > | rcu_register_thread > accel/tcg/tcg-accel-ops-mttcg.c:52:5: error: nested extern declaration of ‘tcg_register_thread’ [-Werror=nested-externs] > cc1: all warnings being treated as errors > > accel/tcg/tcg-accel-ops-rr.c: In function ‘rr_cpu_thread_fn’: > accel/tcg/tcg-accel-ops-rr.c:153:5: error: implicit declaration of function ‘tcg_register_thread’; did you mean ‘rcu_register_thread’? [-Werror=implicit-function-declaration] > 153 | tcg_register_thread(); > | ^~~~~~~~~~~~~~~~~~~ > | rcu_register_thread > accel/tcg/tcg-accel-ops-rr.c:153:5: error: nested extern declaration of ‘tcg_register_thread’ [-Werror=nested-externs] > cc1: all warnings being treated as errors > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > accel/tcg/tcg-accel-ops-mttcg.c | 1 + > accel/tcg/tcg-accel-ops-rr.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c > index 42973fb062b..ddbca6c5b8c 100644 > --- a/accel/tcg/tcg-accel-ops-mttcg.c > +++ b/accel/tcg/tcg-accel-ops-mttcg.c > @@ -32,6 +32,7 @@ > #include "exec/exec-all.h" > #include "hw/boards.h" > > +#include "tcg/tcg.h" > #include "tcg-accel-ops.h" > #include "tcg-accel-ops-mttcg.h" > > diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c > index 4a66055e0d7..1bb1d0f8f1c 100644 > --- a/accel/tcg/tcg-accel-ops-rr.c > +++ b/accel/tcg/tcg-accel-ops-rr.c > @@ -32,6 +32,7 @@ > #include "exec/exec-all.h" > #include "hw/boards.h" > > +#include "tcg/tcg.h" > #include "tcg-accel-ops.h" > #include "tcg-accel-ops-rr.h" > #include "tcg-accel-ops-icount.h" Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé ` (3 preceding siblings ...) 2021-02-07 23:23 ` [PATCH v2 4/6] accel/tcg: " Philippe Mathieu-Daudé @ 2021-02-07 23:23 ` Philippe Mathieu-Daudé 2021-02-08 8:42 ` Alex Bennée 2021-02-07 23:23 ` [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h" Philippe Mathieu-Daudé 5 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm, Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo, Aurelien Jarno Refactor debug code as tlb_assert_iotlb_entry_for_ptr_present() helper. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- What this code does is out of my league, but refactoring it allow keeping tlb_addr_write() local to accel/tcg/cputlb.c in the next patch. --- include/exec/exec-all.h | 9 +++++++++ accel/tcg/cputlb.c | 14 ++++++++++++++ target/arm/mte_helper.c | 11 ++--------- target/arm/sve_helper.c | 10 ++-------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index f933c74c446..c5e8e355b7f 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -296,6 +296,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, void tlb_set_page(CPUState *cpu, target_ulong vaddr, hwaddr paddr, int prot, int mmu_idx, target_ulong size); + +/* + * Find the iotlbentry for ptr. This *must* be present in the TLB + * because we just found the mapping. + */ +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, + uint64_t ptr, + MMUAccessType ptr_access, + uintptr_t index); #else static inline void tlb_init(CPUState *cpu) { diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 8a7b779270a..a6247da34a0 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -429,6 +429,20 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu) tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS); } +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, + uint64_t ptr, + MMUAccessType ptr_access, + uintptr_t index) +{ +#ifdef CONFIG_DEBUG_TCG + CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); + target_ulong comparator = (ptr_access == MMU_DATA_LOAD + ? entry->addr_read + : tlb_addr_write(entry)); + g_assert(tlb_hit(comparator, ptr)); +#endif +} + static bool tlb_hit_page_mask_anyprot(CPUTLBEntry *tlb_entry, target_ulong page, target_ulong mask) { diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c index 6cea9d1b506..f47d3b4570e 100644 --- a/target/arm/mte_helper.c +++ b/target/arm/mte_helper.c @@ -111,15 +111,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx, * matching tlb entry + iotlb entry. */ index = tlb_index(env, ptr_mmu_idx, ptr); -# ifdef CONFIG_DEBUG_TCG - { - CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); - target_ulong comparator = (ptr_access == MMU_DATA_LOAD - ? entry->addr_read - : tlb_addr_write(entry)); - g_assert(tlb_hit(comparator, ptr)); - } -# endif + tlb_assert_iotlb_entry_for_ptr_present(env, ptr_mmu_idx, ptr, + ptr_access, index); iotlbentry = &env_tlb(env)->d[ptr_mmu_idx].iotlb[index]; /* If the virtual page MemAttr != Tagged, access unchecked. */ diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c index c8cdf7618eb..a5708da0f2f 100644 --- a/target/arm/sve_helper.c +++ b/target/arm/sve_helper.c @@ -4089,14 +4089,8 @@ static bool sve_probe_page(SVEHostPage *info, bool nofault, { uintptr_t index = tlb_index(env, mmu_idx, addr); -# ifdef CONFIG_DEBUG_TCG - CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); - target_ulong comparator = (access_type == MMU_DATA_LOAD - ? entry->addr_read - : tlb_addr_write(entry)); - g_assert(tlb_hit(comparator, addr)); -# endif - + tlb_assert_iotlb_entry_for_ptr_present(env, mmu_idx, addr, + access_type, index); CPUIOTLBEntry *iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index]; info->attrs = iotlbentry->attrs; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() 2021-02-07 23:23 ` [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() Philippe Mathieu-Daudé @ 2021-02-08 8:42 ` Alex Bennée 2021-02-08 13:52 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 15+ messages in thread From: Alex Bennée @ 2021-02-08 8:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, qemu-devel, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > Refactor debug code as tlb_assert_iotlb_entry_for_ptr_present() helper. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > What this code does is out of my league, but refactoring it allow > keeping tlb_addr_write() local to accel/tcg/cputlb.c in the next > patch. The assertion that the table entry is current is just a simple housekeeping one. The details of how the MTE implementation uses (abuses?) the iotlb entries requires a closer reading of the code. > --- > include/exec/exec-all.h | 9 +++++++++ > accel/tcg/cputlb.c | 14 ++++++++++++++ > target/arm/mte_helper.c | 11 ++--------- > target/arm/sve_helper.c | 10 ++-------- > 4 files changed, 27 insertions(+), 17 deletions(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index f933c74c446..c5e8e355b7f 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -296,6 +296,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, > void tlb_set_page(CPUState *cpu, target_ulong vaddr, > hwaddr paddr, int prot, > int mmu_idx, target_ulong size); > + > +/* > + * Find the iotlbentry for ptr. This *must* be present in the TLB > + * because we just found the mapping. > + */ > +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, > + uint64_t ptr, > + MMUAccessType ptr_access, > + uintptr_t index); Probably worth making this an empty inline for the non CONFIG_DEBUG_TCG case so we can eliminate the call to an empty function. > #else > static inline void tlb_init(CPUState *cpu) > { > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index 8a7b779270a..a6247da34a0 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -429,6 +429,20 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu) > tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS); > } > > +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, > + uint64_t ptr, > + MMUAccessType ptr_access, > + uintptr_t index) > +{ > +#ifdef CONFIG_DEBUG_TCG > + CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); > + target_ulong comparator = (ptr_access == MMU_DATA_LOAD > + ? entry->addr_read > + : tlb_addr_write(entry)); > + g_assert(tlb_hit(comparator, ptr)); > +#endif > +} > + > static bool tlb_hit_page_mask_anyprot(CPUTLBEntry *tlb_entry, > target_ulong page, target_ulong mask) > { > diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c > index 6cea9d1b506..f47d3b4570e 100644 > --- a/target/arm/mte_helper.c > +++ b/target/arm/mte_helper.c > @@ -111,15 +111,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx, > * matching tlb entry + iotlb entry. > */ > index = tlb_index(env, ptr_mmu_idx, ptr); > -# ifdef CONFIG_DEBUG_TCG > - { > - CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); > - target_ulong comparator = (ptr_access == MMU_DATA_LOAD > - ? entry->addr_read > - : tlb_addr_write(entry)); > - g_assert(tlb_hit(comparator, ptr)); > - } > -# endif > + tlb_assert_iotlb_entry_for_ptr_present(env, ptr_mmu_idx, ptr, > + ptr_access, index); > iotlbentry = &env_tlb(env)->d[ptr_mmu_idx].iotlb[index]; > > /* If the virtual page MemAttr != Tagged, access unchecked. */ > diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c > index c8cdf7618eb..a5708da0f2f 100644 > --- a/target/arm/sve_helper.c > +++ b/target/arm/sve_helper.c > @@ -4089,14 +4089,8 @@ static bool sve_probe_page(SVEHostPage *info, bool nofault, > { > uintptr_t index = tlb_index(env, mmu_idx, addr); > > -# ifdef CONFIG_DEBUG_TCG > - CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); > - target_ulong comparator = (access_type == MMU_DATA_LOAD > - ? entry->addr_read > - : tlb_addr_write(entry)); > - g_assert(tlb_hit(comparator, addr)); > -# endif > - > + tlb_assert_iotlb_entry_for_ptr_present(env, mmu_idx, addr, > + access_type, index); > CPUIOTLBEntry *iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index]; > info->attrs = iotlbentry->attrs; > } with the inline fix: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() 2021-02-08 8:42 ` Alex Bennée @ 2021-02-08 13:52 ` Philippe Mathieu-Daudé 2021-02-08 14:48 ` Alex Bennée 2021-02-08 22:34 ` Richard Henderson 0 siblings, 2 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-08 13:52 UTC (permalink / raw) To: Alex Bennée Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, qemu-devel, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno On 2/8/21 9:42 AM, Alex Bennée wrote: > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > >> Refactor debug code as tlb_assert_iotlb_entry_for_ptr_present() helper. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> What this code does is out of my league, but refactoring it allow >> keeping tlb_addr_write() local to accel/tcg/cputlb.c in the next >> patch. > > The assertion that the table entry is current is just a simple > housekeeping one. The details of how the MTE implementation uses > (abuses?) the iotlb entries requires a closer reading of the code. > >> --- >> include/exec/exec-all.h | 9 +++++++++ >> accel/tcg/cputlb.c | 14 ++++++++++++++ >> target/arm/mte_helper.c | 11 ++--------- >> target/arm/sve_helper.c | 10 ++-------- >> 4 files changed, 27 insertions(+), 17 deletions(-) >> >> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >> index f933c74c446..c5e8e355b7f 100644 >> --- a/include/exec/exec-all.h >> +++ b/include/exec/exec-all.h >> @@ -296,6 +296,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >> void tlb_set_page(CPUState *cpu, target_ulong vaddr, >> hwaddr paddr, int prot, >> int mmu_idx, target_ulong size); >> + >> +/* >> + * Find the iotlbentry for ptr. This *must* be present in the TLB >> + * because we just found the mapping. >> + */ >> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, >> + uint64_t ptr, >> + MMUAccessType ptr_access, >> + uintptr_t index); > > Probably worth making this an empty inline for the non CONFIG_DEBUG_TCG > case so we can eliminate the call to an empty function. But then we can't make tlb_addr_write() static (next patch) and we still have to include "tcg/tcg.h" for the TCG_OVERSIZED_GUEST definition... > >> #else >> static inline void tlb_init(CPUState *cpu) >> { >> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >> index 8a7b779270a..a6247da34a0 100644 >> --- a/accel/tcg/cputlb.c >> +++ b/accel/tcg/cputlb.c >> @@ -429,6 +429,20 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu) >> tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS); >> } >> >> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, >> + uint64_t ptr, >> + MMUAccessType ptr_access, >> + uintptr_t index) >> +{ >> +#ifdef CONFIG_DEBUG_TCG >> + CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); >> + target_ulong comparator = (ptr_access == MMU_DATA_LOAD >> + ? entry->addr_read >> + : tlb_addr_write(entry)); >> + g_assert(tlb_hit(comparator, ptr)); >> +#endif >> +} >> + >> static bool tlb_hit_page_mask_anyprot(CPUTLBEntry *tlb_entry, >> target_ulong page, target_ulong mask) >> { >> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c >> index 6cea9d1b506..f47d3b4570e 100644 >> --- a/target/arm/mte_helper.c >> +++ b/target/arm/mte_helper.c >> @@ -111,15 +111,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx, >> * matching tlb entry + iotlb entry. >> */ >> index = tlb_index(env, ptr_mmu_idx, ptr); >> -# ifdef CONFIG_DEBUG_TCG >> - { >> - CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); >> - target_ulong comparator = (ptr_access == MMU_DATA_LOAD >> - ? entry->addr_read >> - : tlb_addr_write(entry)); >> - g_assert(tlb_hit(comparator, ptr)); >> - } >> -# endif >> + tlb_assert_iotlb_entry_for_ptr_present(env, ptr_mmu_idx, ptr, >> + ptr_access, index); >> iotlbentry = &env_tlb(env)->d[ptr_mmu_idx].iotlb[index]; >> >> /* If the virtual page MemAttr != Tagged, access unchecked. */ >> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c >> index c8cdf7618eb..a5708da0f2f 100644 >> --- a/target/arm/sve_helper.c >> +++ b/target/arm/sve_helper.c >> @@ -4089,14 +4089,8 @@ static bool sve_probe_page(SVEHostPage *info, bool nofault, >> { >> uintptr_t index = tlb_index(env, mmu_idx, addr); >> >> -# ifdef CONFIG_DEBUG_TCG >> - CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); >> - target_ulong comparator = (access_type == MMU_DATA_LOAD >> - ? entry->addr_read >> - : tlb_addr_write(entry)); >> - g_assert(tlb_hit(comparator, addr)); >> -# endif >> - >> + tlb_assert_iotlb_entry_for_ptr_present(env, mmu_idx, addr, >> + access_type, index); >> CPUIOTLBEntry *iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index]; >> info->attrs = iotlbentry->attrs; >> } > > with the inline fix: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() 2021-02-08 13:52 ` Philippe Mathieu-Daudé @ 2021-02-08 14:48 ` Alex Bennée 2021-02-08 22:34 ` Richard Henderson 1 sibling, 0 replies; 15+ messages in thread From: Alex Bennée @ 2021-02-08 14:48 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, qemu-devel, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 2/8/21 9:42 AM, Alex Bennée wrote: >> >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >>> Refactor debug code as tlb_assert_iotlb_entry_for_ptr_present() helper. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> What this code does is out of my league, but refactoring it allow >>> keeping tlb_addr_write() local to accel/tcg/cputlb.c in the next >>> patch. >> >> The assertion that the table entry is current is just a simple >> housekeeping one. The details of how the MTE implementation uses >> (abuses?) the iotlb entries requires a closer reading of the code. >> >>> --- >>> include/exec/exec-all.h | 9 +++++++++ >>> accel/tcg/cputlb.c | 14 ++++++++++++++ >>> target/arm/mte_helper.c | 11 ++--------- >>> target/arm/sve_helper.c | 10 ++-------- >>> 4 files changed, 27 insertions(+), 17 deletions(-) >>> >>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >>> index f933c74c446..c5e8e355b7f 100644 >>> --- a/include/exec/exec-all.h >>> +++ b/include/exec/exec-all.h >>> @@ -296,6 +296,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >>> void tlb_set_page(CPUState *cpu, target_ulong vaddr, >>> hwaddr paddr, int prot, >>> int mmu_idx, target_ulong size); >>> + >>> +/* >>> + * Find the iotlbentry for ptr. This *must* be present in the TLB >>> + * because we just found the mapping. >>> + */ >>> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, >>> + uint64_t ptr, >>> + MMUAccessType ptr_access, >>> + uintptr_t index); >> >> Probably worth making this an empty inline for the non CONFIG_DEBUG_TCG >> case so we can eliminate the call to an empty function. > > But then we can't make tlb_addr_write() static (next patch) and > we still have to include "tcg/tcg.h" for the TCG_OVERSIZED_GUEST > definition... Hmm - yeah. I'm not keen on turning something into a function call when the compiler should have all the information it needs with it. On the other hand maybe we don't care for a debug assert. Richard WDYT? > >> >>> #else >>> static inline void tlb_init(CPUState *cpu) >>> { >>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c >>> index 8a7b779270a..a6247da34a0 100644 >>> --- a/accel/tcg/cputlb.c >>> +++ b/accel/tcg/cputlb.c >>> @@ -429,6 +429,20 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu) >>> tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS); >>> } >>> >>> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, >>> + uint64_t ptr, >>> + MMUAccessType ptr_access, >>> + uintptr_t index) >>> +{ >>> +#ifdef CONFIG_DEBUG_TCG >>> + CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); >>> + target_ulong comparator = (ptr_access == MMU_DATA_LOAD >>> + ? entry->addr_read >>> + : tlb_addr_write(entry)); >>> + g_assert(tlb_hit(comparator, ptr)); >>> +#endif >>> +} >>> + >>> static bool tlb_hit_page_mask_anyprot(CPUTLBEntry *tlb_entry, >>> target_ulong page, target_ulong mask) >>> { >>> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c >>> index 6cea9d1b506..f47d3b4570e 100644 >>> --- a/target/arm/mte_helper.c >>> +++ b/target/arm/mte_helper.c >>> @@ -111,15 +111,8 @@ static uint8_t *allocation_tag_mem(CPUARMState *env, int ptr_mmu_idx, >>> * matching tlb entry + iotlb entry. >>> */ >>> index = tlb_index(env, ptr_mmu_idx, ptr); >>> -# ifdef CONFIG_DEBUG_TCG >>> - { >>> - CPUTLBEntry *entry = tlb_entry(env, ptr_mmu_idx, ptr); >>> - target_ulong comparator = (ptr_access == MMU_DATA_LOAD >>> - ? entry->addr_read >>> - : tlb_addr_write(entry)); >>> - g_assert(tlb_hit(comparator, ptr)); >>> - } >>> -# endif >>> + tlb_assert_iotlb_entry_for_ptr_present(env, ptr_mmu_idx, ptr, >>> + ptr_access, index); >>> iotlbentry = &env_tlb(env)->d[ptr_mmu_idx].iotlb[index]; >>> >>> /* If the virtual page MemAttr != Tagged, access unchecked. */ >>> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c >>> index c8cdf7618eb..a5708da0f2f 100644 >>> --- a/target/arm/sve_helper.c >>> +++ b/target/arm/sve_helper.c >>> @@ -4089,14 +4089,8 @@ static bool sve_probe_page(SVEHostPage *info, bool nofault, >>> { >>> uintptr_t index = tlb_index(env, mmu_idx, addr); >>> >>> -# ifdef CONFIG_DEBUG_TCG >>> - CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr); >>> - target_ulong comparator = (access_type == MMU_DATA_LOAD >>> - ? entry->addr_read >>> - : tlb_addr_write(entry)); >>> - g_assert(tlb_hit(comparator, addr)); >>> -# endif >>> - >>> + tlb_assert_iotlb_entry_for_ptr_present(env, mmu_idx, addr, >>> + access_type, index); >>> CPUIOTLBEntry *iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index]; >>> info->attrs = iotlbentry->attrs; >>> } >> >> with the inline fix: >> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() 2021-02-08 13:52 ` Philippe Mathieu-Daudé 2021-02-08 14:48 ` Alex Bennée @ 2021-02-08 22:34 ` Richard Henderson 1 sibling, 0 replies; 15+ messages in thread From: Richard Henderson @ 2021-02-08 22:34 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Alex Bennée Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, qemu-devel, Laurent Vivier, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno On 2/8/21 5:52 AM, Philippe Mathieu-Daudé wrote: > On 2/8/21 9:42 AM, Alex Bennée wrote: >> >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >>> Refactor debug code as tlb_assert_iotlb_entry_for_ptr_present() helper. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> What this code does is out of my league, but refactoring it allow >>> keeping tlb_addr_write() local to accel/tcg/cputlb.c in the next >>> patch. >> >> The assertion that the table entry is current is just a simple >> housekeeping one. The details of how the MTE implementation uses >> (abuses?) the iotlb entries requires a closer reading of the code. >> >>> --- >>> include/exec/exec-all.h | 9 +++++++++ >>> accel/tcg/cputlb.c | 14 ++++++++++++++ >>> target/arm/mte_helper.c | 11 ++--------- >>> target/arm/sve_helper.c | 10 ++-------- >>> 4 files changed, 27 insertions(+), 17 deletions(-) >>> >>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h >>> index f933c74c446..c5e8e355b7f 100644 >>> --- a/include/exec/exec-all.h >>> +++ b/include/exec/exec-all.h >>> @@ -296,6 +296,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, >>> void tlb_set_page(CPUState *cpu, target_ulong vaddr, >>> hwaddr paddr, int prot, >>> int mmu_idx, target_ulong size); >>> + >>> +/* >>> + * Find the iotlbentry for ptr. This *must* be present in the TLB >>> + * because we just found the mapping. >>> + */ >>> +void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, >>> + uint64_t ptr, >>> + MMUAccessType ptr_access, >>> + uintptr_t index); >> >> Probably worth making this an empty inline for the non CONFIG_DEBUG_TCG >> case so we can eliminate the call to an empty function. > > But then we can't make tlb_addr_write() static (next patch) and > we still have to include "tcg/tcg.h" for the TCG_OVERSIZED_GUEST > definition... Certainly you can, though it's not especially pretty: #ifdef CONFIG_DEBUG_TCG void tlb_assert_iotlb_entry_for_ptr_present (CPUArchState *env, int ptr_mmu_idx, uint64_t ptr, MMUAccessType ptr_access, uintptr_t index); #else static inline void tlb_assert_iotlb_entry_for_ptr_present (CPUArchState *env, int ptr_mmu_idx, uint64_t ptr, MMUAccessType ptr_access, uintptr_t index) { } #endif r~ ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h" 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé ` (4 preceding siblings ...) 2021-02-07 23:23 ` [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() Philippe Mathieu-Daudé @ 2021-02-07 23:23 ` Philippe Mathieu-Daudé 2021-02-08 14:40 ` Alex Bennée 5 siblings, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-07 23:23 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Alistair Francis, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Philippe Mathieu-Daudé, Laurent Vivier, qemu-arm, Palmer Dabbelt, Claudio Fontana, Paolo Bonzini, Aleksandar Rikalo, Aurelien Jarno Keep MMU functions in "exec/cpu_ldst.h", and move TLB functions to "exec/exec-all.h". As tlb_addr_write() is only called in accel/tcg/cputlb.c, make move it there as a static function. Doing so we removed the "tcg/tcg.h" dependency on "exec/cpu_ldst.h". Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/exec/cpu_ldst.h | 28 ---------------------------- include/exec/exec-all.h | 16 ++++++++++++++++ accel/tcg/cputlb.c | 9 +++++++++ 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h index ef54cb7e1f8..c1753a64dfd 100644 --- a/include/exec/cpu_ldst.h +++ b/include/exec/cpu_ldst.h @@ -291,34 +291,6 @@ static inline void cpu_stq_le_mmuidx_ra(CPUArchState *env, abi_ptr addr, #else -/* Needed for TCG_OVERSIZED_GUEST */ -#include "tcg/tcg.h" - -static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry) -{ -#if TCG_OVERSIZED_GUEST - return entry->addr_write; -#else - return qatomic_read(&entry->addr_write); -#endif -} - -/* Find the TLB index corresponding to the mmu_idx + address pair. */ -static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx, - target_ulong addr) -{ - uintptr_t size_mask = env_tlb(env)->f[mmu_idx].mask >> CPU_TLB_ENTRY_BITS; - - return (addr >> TARGET_PAGE_BITS) & size_mask; -} - -/* Find the TLB entry corresponding to the mmu_idx + address pair. */ -static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx, - target_ulong addr) -{ - return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)]; -} - uint32_t cpu_ldub_mmuidx_ra(CPUArchState *env, abi_ptr addr, int mmu_idx, uintptr_t ra); int cpu_ldsb_mmuidx_ra(CPUArchState *env, abi_ptr addr, diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index c5e8e355b7f..8e54b537189 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -297,6 +297,22 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr, hwaddr paddr, int prot, int mmu_idx, target_ulong size); +/* Find the TLB index corresponding to the mmu_idx + address pair. */ +static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx, + target_ulong addr) +{ + uintptr_t size_mask = env_tlb(env)->f[mmu_idx].mask >> CPU_TLB_ENTRY_BITS; + + return (addr >> TARGET_PAGE_BITS) & size_mask; +} + +/* Find the TLB entry corresponding to the mmu_idx + address pair. */ +static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx, + target_ulong addr) +{ + return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)]; +} + /* * Find the iotlbentry for ptr. This *must* be present in the TLB * because we just found the mapping. diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index a6247da34a0..084d19b52d7 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -429,6 +429,15 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu) tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS); } +static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry) +{ +#if TCG_OVERSIZED_GUEST + return entry->addr_write; +#else + return qatomic_read(&entry->addr_write); +#endif +} + void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, uint64_t ptr, MMUAccessType ptr_access, -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h" 2021-02-07 23:23 ` [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h" Philippe Mathieu-Daudé @ 2021-02-08 14:40 ` Alex Bennée 0 siblings, 0 replies; 15+ messages in thread From: Alex Bennée @ 2021-02-08 14:40 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Aleksandar Rikalo, qemu-riscv, Yoshinori Sato, Sagar Karandikar, Bastian Koppelmann, Richard Henderson, Laurent Vivier, qemu-devel, qemu-arm, Alistair Francis, Claudio Fontana, Paolo Bonzini, Palmer Dabbelt, Aurelien Jarno Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > Keep MMU functions in "exec/cpu_ldst.h", and move TLB functions > to "exec/exec-all.h". As tlb_addr_write() is only called in > accel/tcg/cputlb.c, make move it there as a static function. > > Doing so we removed the "tcg/tcg.h" dependency on "exec/cpu_ldst.h". > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/exec/cpu_ldst.h | 28 ---------------------------- > include/exec/exec-all.h | 16 ++++++++++++++++ > accel/tcg/cputlb.c | 9 +++++++++ > 3 files changed, 25 insertions(+), 28 deletions(-) > > diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h > index ef54cb7e1f8..c1753a64dfd 100644 > --- a/include/exec/cpu_ldst.h > +++ b/include/exec/cpu_ldst.h > @@ -291,34 +291,6 @@ static inline void cpu_stq_le_mmuidx_ra(CPUArchState *env, abi_ptr addr, > > #else > > -/* Needed for TCG_OVERSIZED_GUEST */ > -#include "tcg/tcg.h" > - > -static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry) > -{ > -#if TCG_OVERSIZED_GUEST > - return entry->addr_write; > -#else > - return qatomic_read(&entry->addr_write); > -#endif > -} > - > -/* Find the TLB index corresponding to the mmu_idx + address pair. */ > -static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx, > - target_ulong addr) > -{ > - uintptr_t size_mask = env_tlb(env)->f[mmu_idx].mask >> CPU_TLB_ENTRY_BITS; > - > - return (addr >> TARGET_PAGE_BITS) & size_mask; > -} > - > -/* Find the TLB entry corresponding to the mmu_idx + address pair. */ > -static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx, > - target_ulong addr) > -{ > - return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)]; > -} > - > uint32_t cpu_ldub_mmuidx_ra(CPUArchState *env, abi_ptr addr, > int mmu_idx, uintptr_t ra); > int cpu_ldsb_mmuidx_ra(CPUArchState *env, abi_ptr addr, > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index c5e8e355b7f..8e54b537189 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -297,6 +297,22 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr, > hwaddr paddr, int prot, > int mmu_idx, target_ulong size); > > +/* Find the TLB index corresponding to the mmu_idx + address pair. */ > +static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx, > + target_ulong addr) > +{ > + uintptr_t size_mask = env_tlb(env)->f[mmu_idx].mask >> CPU_TLB_ENTRY_BITS; > + > + return (addr >> TARGET_PAGE_BITS) & size_mask; > +} > + > +/* Find the TLB entry corresponding to the mmu_idx + address pair. */ > +static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx, > + target_ulong addr) > +{ > + return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)]; > +} > + I wonder if throwing these into exec-all is worth it, could we not use the cputlb.h so we avoid too much kitchen sink for a header (after all we are trying to avoid recompilations and not everything needs detailed access to the tlb structures). > /* > * Find the iotlbentry for ptr. This *must* be present in the TLB > * because we just found the mapping. > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index a6247da34a0..084d19b52d7 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -429,6 +429,15 @@ void tlb_flush_all_cpus_synced(CPUState *src_cpu) > tlb_flush_by_mmuidx_all_cpus_synced(src_cpu, ALL_MMUIDX_BITS); > } > > +static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry) > +{ > +#if TCG_OVERSIZED_GUEST > + return entry->addr_write; > +#else > + return qatomic_read(&entry->addr_write); > +#endif > +} You can drop the inline, compiler should know best what to do in this case. > void tlb_assert_iotlb_entry_for_ptr_present(CPUArchState *env, int ptr_mmu_idx, > uint64_t ptr, > MMUAccessType ptr_access, -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-20 20:09 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-07 23:23 [RFC PATCH v2 0/6] exec: Remove "tcg/tcg.h" from "exec/cpu_ldst.h" Philippe Mathieu-Daudé 2021-02-07 23:23 ` [RFC PATCH v2 1/6] target: Replace tcg_debug_assert() by assert() Philippe Mathieu-Daudé 2021-02-08 13:54 ` Alex Bennée 2021-02-07 23:23 ` [PATCH v2 2/6] target/m68k: Include missing "tcg/tcg.h" header Philippe Mathieu-Daudé 2021-02-07 23:23 ` [PATCH v2 3/6] target/mips: " Philippe Mathieu-Daudé 2021-02-20 20:07 ` Philippe Mathieu-Daudé 2021-02-07 23:23 ` [PATCH v2 4/6] accel/tcg: " Philippe Mathieu-Daudé 2021-02-08 14:36 ` Alex Bennée 2021-02-07 23:23 ` [RFC PATCH v2 5/6] accel/tcg: Refactor debugging tlb_assert_iotlb_entry_for_ptr_present() Philippe Mathieu-Daudé 2021-02-08 8:42 ` Alex Bennée 2021-02-08 13:52 ` Philippe Mathieu-Daudé 2021-02-08 14:48 ` Alex Bennée 2021-02-08 22:34 ` Richard Henderson 2021-02-07 23:23 ` [PATCH v2 6/6] exec/cpu_ldst: Move tlb* declarations to "exec/exec-all.h" Philippe Mathieu-Daudé 2021-02-08 14:40 ` Alex Bennée
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).