[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] tcg: Merge GETPC and GETRA
|
From: |
Benjamin Herrenschmidt |
|
Subject: |
Re: [Qemu-devel] [PATCH] tcg: Merge GETPC and GETRA |
|
Date: |
Tue, 26 Jul 2016 11:36:40 +1000 |
On Tue, 2016-07-26 at 06:12 +0530, Richard Henderson wrote:
> The return address argument to the softmmu template helpers was
> confused. In the legacy case, we wanted to indicate that there
> is no return address, and so passed in NULL. However, we then
> immediately subtracted GETPC_ADJ from NULL, resulting in a non-zero
> value, indicating the presence of an (invalid) return address.
>
> Push the GETPC_ADJ subtraction down to the only point it's required:
> immediately before use within cpu_restore_state, after all NULL
> pointer
> checks have been completed. This makes GETPC and GETRA identical.
>
> Remove GETRA as the lesser used macro, replacing all uses with GETPC.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>
> Ben, this should fix the "-2" problem that you reported. Of course,
> as also discussed in that thread, this won't fix the whole issue.
Thanks. I've fixed the rest of the issue here:
https://github.com/ozbenh/qemu/commit/5b84a90aa696a7072eaa7a2f4cf6ade7cdd10358
While I will send to the list later today hopefully, along with the
rest of my cleanups and fixes that are starting to pile up here:
https://github.com/ozbenh/qemu/commits/wip
I haven't tackled the FP exceptions yet though but that can wait.
Cheers,
Ben.
> r~
>
> ---
> cputlb.c | 6 ++----
> include/exec/exec-all.h | 9 +++------
> softmmu_template.h | 32 ++++++--------------------------
> target-arm/helper.c | 6 +++---
> target-mips/op_helper.c | 18 +++++++++---------
> translate-all.c | 1 +
> 6 files changed, 24 insertions(+), 48 deletions(-)
>
> diff --git a/cputlb.c b/cputlb.c
> index d068ee5..3c99c34 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -543,10 +543,8 @@ static bool victim_tlb_hit(CPUArchState *env,
> size_t mmu_idx, size_t index,
> #undef MMUSUFFIX
>
> #define MMUSUFFIX _cmmu
> -#undef GETPC_ADJ
> -#define GETPC_ADJ 0
> -#undef GETRA
> -#define GETRA() ((uintptr_t)0)
> +#undef GETPC
> +#define GETPC() ((uintptr_t)0)
> #define SOFTMMU_CODE_ACCESS
>
> #define SHIFT 0
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index acda7b6..6a317db 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -335,13 +335,12 @@ static inline void tb_add_jump(TranslationBlock
> *tb, int n,
> tb_next->jmp_list_first = (uintptr_t)tb | n;
> }
>
> -/* GETRA is the true target of the return instruction that we'll
> execute,
> - defined here for simplicity of defining the follow-up macros. */
> +/* GETPC is the true target of the return instruction that we'll
> execute. */
> #if defined(CONFIG_TCG_INTERPRETER)
> extern uintptr_t tci_tb_ptr;
> -# define GETRA() tci_tb_ptr
> +# define GETPC() tci_tb_ptr
> #else
> -# define GETRA() \
> +# define GETPC() \
> ((uintptr_t)__builtin_extract_return_addr(__builtin_return_addre
> ss(0)))
> #endif
>
> @@ -354,8 +353,6 @@ extern uintptr_t tci_tb_ptr;
> smaller than 4 bytes, so we don't worry about special-casing
> this. */
> #define GETPC_ADJ 2
>
> -#define GETPC() (GETRA() - GETPC_ADJ)
> -
> #if !defined(CONFIG_USER_ONLY)
>
> struct MemoryRegion *iotlb_to_region(CPUState *cpu,
> diff --git a/softmmu_template.h b/softmmu_template.h
> index 284ab2c..4cbb714 100644
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -150,9 +150,6 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env,
> target_ulong addr,
> uintptr_t haddr;
> DATA_TYPE res;
>
> - /* Adjust the given return address. */
> - retaddr -= GETPC_ADJ;
> -
> if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
> cpu_unaligned_access(ENV_GET_CPU(env), addr,
> READ_ACCESS_TYPE,
> mmu_idx, retaddr);
> @@ -193,10 +190,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env,
> target_ulong addr,
> do_unaligned_access:
> addr1 = addr & ~(DATA_SIZE - 1);
> addr2 = addr1 + DATA_SIZE;
> - /* Note the adjustment at the beginning of the function.
> - Undo that for the recursion. */
> - res1 = helper_le_ld_name(env, addr1, oi, retaddr +
> GETPC_ADJ);
> - res2 = helper_le_ld_name(env, addr2, oi, retaddr +
> GETPC_ADJ);
> + res1 = helper_le_ld_name(env, addr1, oi, retaddr);
> + res2 = helper_le_ld_name(env, addr2, oi, retaddr);
> shift = (addr & (DATA_SIZE - 1)) * 8;
>
> /* Little-endian combine. */
> @@ -224,9 +219,6 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env,
> target_ulong addr,
> uintptr_t haddr;
> DATA_TYPE res;
>
> - /* Adjust the given return address. */
> - retaddr -= GETPC_ADJ;
> -
> if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
> cpu_unaligned_access(ENV_GET_CPU(env), addr,
> READ_ACCESS_TYPE,
> mmu_idx, retaddr);
> @@ -267,10 +259,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env,
> target_ulong addr,
> do_unaligned_access:
> addr1 = addr & ~(DATA_SIZE - 1);
> addr2 = addr1 + DATA_SIZE;
> - /* Note the adjustment at the beginning of the function.
> - Undo that for the recursion. */
> - res1 = helper_be_ld_name(env, addr1, oi, retaddr +
> GETPC_ADJ);
> - res2 = helper_be_ld_name(env, addr2, oi, retaddr +
> GETPC_ADJ);
> + res1 = helper_be_ld_name(env, addr1, oi, retaddr);
> + res2 = helper_be_ld_name(env, addr2, oi, retaddr);
> shift = (addr & (DATA_SIZE - 1)) * 8;
>
> /* Big-endian combine. */
> @@ -334,9 +324,6 @@ void helper_le_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
> int a_bits = get_alignment_bits(get_memop(oi));
> uintptr_t haddr;
>
> - /* Adjust the given return address. */
> - retaddr -= GETPC_ADJ;
> -
> if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
> cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
> mmu_idx, retaddr);
> @@ -391,10 +378,8 @@ void helper_le_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
> for (i = 0; i < DATA_SIZE; ++i) {
> /* Little-endian extract. */
> uint8_t val8 = val >> (i * 8);
> - /* Note the adjustment at the beginning of the function.
> - Undo that for the recursion. */
> glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8,
> - oi, retaddr +
> GETPC_ADJ);
> + oi, retaddr);
> }
> return;
> }
> @@ -417,9 +402,6 @@ void helper_be_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
> int a_bits = get_alignment_bits(get_memop(oi));
> uintptr_t haddr;
>
> - /* Adjust the given return address. */
> - retaddr -= GETPC_ADJ;
> -
> if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
> cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
> mmu_idx, retaddr);
> @@ -474,10 +456,8 @@ void helper_be_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
> for (i = 0; i < DATA_SIZE; ++i) {
> /* Big-endian extract. */
> uint8_t val8 = val >> (((DATA_SIZE - 1) * 8) - (i * 8));
> - /* Note the adjustment at the beginning of the function.
> - Undo that for the recursion. */
> glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8,
> - oi, retaddr +
> GETPC_ADJ);
> + oi, retaddr);
> }
> return;
> }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index bdb842c..915fe0f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -8310,12 +8310,12 @@ void HELPER(dc_zva)(CPUARMState *env,
> uint64_t vaddr_in)
> * this purpose use the actual register value passed to
> us
> * so that we get the fault address right.
> */
> - helper_ret_stb_mmu(env, vaddr_in, 0, oi, GETRA());
> + helper_ret_stb_mmu(env, vaddr_in, 0, oi, GETPC());
> /* Now we can populate the other TLB entries, if any */
> for (i = 0; i < maxidx; i++) {
> uint64_t va = vaddr + TARGET_PAGE_SIZE * i;
> if (va != (vaddr_in & TARGET_PAGE_MASK)) {
> - helper_ret_stb_mmu(env, va, 0, oi, GETRA());
> + helper_ret_stb_mmu(env, va, 0, oi, GETPC());
> }
> }
> }
> @@ -8332,7 +8332,7 @@ void HELPER(dc_zva)(CPUARMState *env, uint64_t
> vaddr_in)
> * bounce buffer was in use
> */
> for (i = 0; i < blocklen; i++) {
> - helper_ret_stb_mmu(env, vaddr + i, 0, oi, GETRA());
> + helper_ret_stb_mmu(env, vaddr + i, 0, oi, GETPC());
> }
> }
> #else
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index ea2f2ab..7af4c2f 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -4122,10 +4122,10 @@ void helper_msa_ld_ ## TYPE(CPUMIPSState
> *env, uint32_t wd, \
> }
>
> #if !defined(CONFIG_USER_ONLY)
> -MSA_LD_DF(DF_BYTE, b, helper_ret_ldub_mmu, oi, GETRA())
> -MSA_LD_DF(DF_HALF, h, helper_ret_lduw_mmu, oi, GETRA())
> -MSA_LD_DF(DF_WORD, w, helper_ret_ldul_mmu, oi, GETRA())
> -MSA_LD_DF(DF_DOUBLE, d, helper_ret_ldq_mmu, oi, GETRA())
> +MSA_LD_DF(DF_BYTE, b, helper_ret_ldub_mmu, oi, GETPC())
> +MSA_LD_DF(DF_HALF, h, helper_ret_lduw_mmu, oi, GETPC())
> +MSA_LD_DF(DF_WORD, w, helper_ret_ldul_mmu, oi, GETPC())
> +MSA_LD_DF(DF_DOUBLE, d, helper_ret_ldq_mmu, oi, GETPC())
> #else
> MSA_LD_DF(DF_BYTE, b, cpu_ldub_data)
> MSA_LD_DF(DF_HALF, h, cpu_lduw_data)
> @@ -4161,17 +4161,17 @@ void helper_msa_st_ ## TYPE(CPUMIPSState
> *env, uint32_t wd, \
> int mmu_idx = cpu_mmu_index(env, false);
> \
> int
> i; \
> MEMOP_IDX(DF)
> \
> - ensure_writable_pages(env, addr, mmu_idx,
> GETRA()); \
> + ensure_writable_pages(env, addr, mmu_idx,
> GETPC()); \
> for (i = 0; i < DF_ELEMENTS(DF); i++)
> { \
> ST_INSN(env, addr + (i << DF), pwd->TYPE[i],
> ##__VA_ARGS__); \
> }
> \
> }
>
> #if !defined(CONFIG_USER_ONLY)
> -MSA_ST_DF(DF_BYTE, b, helper_ret_stb_mmu, oi, GETRA())
> -MSA_ST_DF(DF_HALF, h, helper_ret_stw_mmu, oi, GETRA())
> -MSA_ST_DF(DF_WORD, w, helper_ret_stl_mmu, oi, GETRA())
> -MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETRA())
> +MSA_ST_DF(DF_BYTE, b, helper_ret_stb_mmu, oi, GETPC())
> +MSA_ST_DF(DF_HALF, h, helper_ret_stw_mmu, oi, GETPC())
> +MSA_ST_DF(DF_WORD, w, helper_ret_stl_mmu, oi, GETPC())
> +MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETPC())
> #else
> MSA_ST_DF(DF_BYTE, b, cpu_stb_data)
> MSA_ST_DF(DF_HALF, h, cpu_stw_data)
> diff --git a/translate-all.c b/translate-all.c
> index 0d47c1c..644d081 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -299,6 +299,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t
> retaddr)
> {
> TranslationBlock *tb;
>
> + retaddr -= GETPC_ADJ;
> tb = tb_find_pc(retaddr);
> if (tb) {
> cpu_restore_state_from_tb(cpu, tb, retaddr);