Re: [PATCH] target/arm: Fix VNCR fault detection logic

[Jonathan Cameron]

On Tue, 16 Jan 2024 16:56:05 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> In arm_deliver_fault() we check for whether the fault is caused
> by a data abort due to an access to a FEAT_NV2 sysreg in the
> memory pointed to by the VNCR. Unfortunately part of the
> condition checks the wrong argument to the function, meaning
> that it would spuriously trigger, resulting in some instruction
> aborts being taken to the wrong EL and reported incorrectly.
> 
> Use the right variable in the condition.
> 
> Fixes: 674e5345275d425 ("target/arm: Report VNCR_EL2 based faults correctly")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Matches what I have locally from discussion earlier.

Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks

> ---
> In less lax languages the compiler might have pointed out that
> the type of the LHS and the RHS in the comparison didn't match :-)
> ---
>  target/arm/tcg/tlb_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/tcg/tlb_helper.c b/target/arm/tcg/tlb_helper.c
> index dd5de74ffb7..5477c7fb7dc 100644
> --- a/target/arm/tcg/tlb_helper.c
> +++ b/target/arm/tcg/tlb_helper.c
> @@ -184,7 +184,7 @@ void arm_deliver_fault(ARMCPU *cpu, vaddr addr,
>       * (and indeed syndrome does not have the EC field in it,
>       * because we masked that out in disas_set_insn_syndrome())
>       */
> -    bool is_vncr = (mmu_idx != MMU_INST_FETCH) &&
> +    bool is_vncr = (access_type != MMU_INST_FETCH) &&
>          (env->exception.syndrome & ARM_EL_VNCR);
>  
>      if (is_vncr) {