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

[Richard Henderson]

On 10/4/22 09:23, Peter Maydell wrote:
> >   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".

Correct.

> > @@ -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 ?

gen_a64_update_pc trashes pc_save.

Off of the top of my head, I can't remember what conditionally uses exceptions (single 
step?).  But the usage pattern that is interesting is

    brcond(x, y, L1)
    update_pc(disp1);
    exit-or-exception.
L1:
    update_pc(disp2);
    exit-or-exception.

where at L1 we should have the same pc_save value as we did at the brcond.  Saving and 
restoring around (at least some of) the DISAS_NORETURN points achieves that.


r~