Re: [PATCH v3 3/6] target/arm: Take an exception if PC is misaligned

[Peter Maydell]

On Mon, 20 Sept 2021 at 03:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For A64, any input to an indirect branch can cause this.
>
> For A32, many indirect branch paths force the branch to be aligned,
> but BXWritePC does not.  This includes the BX instruction but also
> other interworking changes to PC.  Prior to v8, this case is UNDEFINED.
> With v8, this is CONSTRAINED UNPREDICTABLE and may either raise an
> exception or force align the PC.
>
> We choose to raise an exception because we have the infrastructure,
> it makes the generated code for gen_bx simpler, and it has the
> possibility of catching more guest bugs.

> +void helper_exception_pc_alignment(CPUARMState *env, target_ulong pc)
> +{
> +    int target_el = exception_target_el(env);
> +
> +    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> +        /*
> +         * To aarch64 and aarch32 el2, pc alignment has a
> +         * special exception class.
> +         */
> +        env->exception.vaddress = pc;
> +        env->exception.fsr = 0;
> +        raise_exception(env, EXCP_PREFETCH_ABORT, syn_pcalignment(), 
> target_el);
> +    } else {
> +        /*
> +         * To aarch32 el1, pc alignment is like data alignment
> +         * except with a prefetch abort.
> +         */
> +        ARMMMUFaultInfo fi = { .type = ARMFault_Alignment };
> +        arm_deliver_fault(env_archcpu(env), pc, MMU_INST_FETCH,
> +                          cpu_mmu_index(env, true), &fi);
> +    }

I still don't believe that you need to special case AArch32-non-Hyp
like this. Can you expand on what you think the difference is?

> +}
> +
>  #if !defined(CONFIG_USER_ONLY)
>
>  /*
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index ab6b346e35..8c72e37de3 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -14752,8 +14752,10 @@ static void 
> aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>  {
>      DisasContext *s = container_of(dcbase, DisasContext, base);
>      CPUARMState *env = cpu->env_ptr;
> +    uint64_t pc = s->base.pc_next;
>      uint32_t insn;
>
> +    /* Singlestep exceptions have the highest priority. */
>      if (s->ss_active && !s->pstate_ss) {
>          /* Singlestep state is Active-pending.
>           * If we're in this state at the start of a TB then either
> @@ -14768,13 +14770,28 @@ static void 
> aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>          assert(s->base.num_insns == 1);
>          gen_swstep_exception(s, 0, 0);
>          s->base.is_jmp = DISAS_NORETURN;
> +        s->base.pc_next = pc + 4;

Why are we adding this in this patch? Isn't this a separate
bugfix ?

>          return;
>      }

thanks
-- PMM