Re: [PATCH v4 9/9] target/arm: Enable TARGET_TB_PCREL

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 6 Sept 2022 at 11:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu-param.h     |  2 ++
>  target/arm/translate.h     |  6 ++++
>  target/arm/cpu.c           | 23 +++++++-------
>  target/arm/translate-a64.c | 37 ++++++++++++++++++-----
>  target/arm/translate.c     | 62 ++++++++++++++++++++++++++++++--------
>  5 files changed, 100 insertions(+), 30 deletions(-)
>
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index 68ffb12427..ef62371d8f 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -34,4 +34,6 @@
>
>  #define NB_MMU_MODES 15
>
> +#define TARGET_TB_PCREL 1
> +
>  #endif
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index d42059aa1d..7717ea3f45 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -12,6 +12,12 @@ typedef struct DisasContext {
>
>      /* The address of the current instruction being translated. */
>      target_ulong pc_curr;
> +    /*
> +     * For TARGET_TB_PCREL, the value relative to pc_curr against which
> +     * offsets must be computed for cpu_pc.  -1 if unknown due to jump.
> +     */

I'm not really sure what this comment is trying to tell me. Could
you expand it a bit ?

> +    target_ulong pc_save;
> +    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 047bf3f4ab..f5e74b6c3b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -64,17 +64,18 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>  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. */

Is it?

Is there some documentation in one of the other patchsets about
how TARGET_TB_PCREL works in general and what targets need to do to
support it?


thanks
-- PMM