On Fri, 30 Sept 2022 at 23:10, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> target/arm/cpu-param.h | 1 +
> target/arm/translate.h | 19 ++++++++++++
> target/arm/cpu.c | 23 +++++++-------
> target/arm/translate-a64.c | 37 ++++++++++++++++++-----
> target/arm/translate.c | 62 ++++++++++++++++++++++++++++++--------
> 5 files changed, 112 insertions(+), 30 deletions(-)
>
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index 68ffb12427..29c5fc4241 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -30,6 +30,7 @@
> */
> # define TARGET_PAGE_BITS_VARY
> # define TARGET_PAGE_BITS_MIN 10
> +# define TARGET_TB_PCREL 1
> #endif
>
> #define NB_MMU_MODES 15
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 4aa239e23c..41d14cc067 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -12,6 +12,25 @@ typedef struct DisasContext {
>
> /* The address of the current instruction being translated. */
> target_ulong pc_curr;
> + /*
> + * For TARGET_TB_PCREL, the full value of cpu_pc is not known
> + * (although the page offset is known). For convenience, the
> + * translation loop uses the full virtual address that triggered
> + * the translation is used, from base.pc_start through pc_curr.
s/ is used//
> + * For efficiency, we do not update cpu_pc for every instruction.
> + * Instead, pc_save has the value of pc_curr at the time of the
> + * last update to cpu_pc, which allows us to compute the addend
> + * needed to bring cpu_pc current: pc_curr - pc_save.
> + * If cpu_pc now contains the destiation of an indirect branch,
"destination"
> + * pc_save contains -1 to indicate that relative updates are no
> + * longer possible.
> + */
> + target_ulong pc_save;
> + /*
> + * Similarly, pc_cond_save contains the value of pc_save at the
> + * beginning of an AArch32 conditional instruction.
> + */
> + target_ulong pc_cond_save;
> target_ulong page_start;
> uint32_t insn;
> /* Nonzero if this instruction has been conditionally skipped. */
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 94ca6f163f..0bc5e9b125 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -76,17 +76,18 @@ static vaddr arm_cpu_get_pc(CPUState *cs)
> void arm_cpu_synchronize_from_tb(CPUState *cs,
> const TranslationBlock *tb)
> {
> - ARMCPU *cpu = ARM_CPU(cs);
> - CPUARMState *env = &cpu->env;
> -
> - /*
> - * It's OK to look at env for the current mode here, because it's
> - * never possible for an AArch64 TB to chain to an AArch32 TB.
> - */
> - if (is_a64(env)) {
> - env->pc = tb_pc(tb);
> - } else {
> - env->regs[15] = tb_pc(tb);
> + /* The program counter is always up to date with TARGET_TB_PCREL. */
I was confused for a bit about this, but it works because
although the synchronize_from_tb hook has a name that implies
it's comparatively general purpose, in fact we use it only
in the special case of "we abandoned execution at the start of
this TB without executing any of it".
> + if (!TARGET_TB_PCREL) {
> + CPUARMState *env = cs->env_ptr;
> + /*
> + * It's OK to look at env for the current mode here, because it's
> + * never possible for an AArch64 TB to chain to an AArch32 TB.
> + */
> + if (is_a64(env)) {
> + env->pc = tb_pc(tb);
> + } else {
> + env->regs[15] = tb_pc(tb);
> + }
> }
> }
> #endif /* CONFIG_TCG */
> @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
>
> static void gen_exception_internal_insn(DisasContext *s, int excp)
> {
> + target_ulong pc_save = s->pc_save;
> +
> gen_a64_update_pc(s, 0);
> gen_exception_internal(excp);
> s->base.is_jmp = DISAS_NORETURN;
> + s->pc_save = pc_save;
What is trashing s->pc_save that we have to work around like this,
here and in the other similar changes ?
thanks
-- PMM
