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

[Richard Henderson]

On 1/17/24 02:56, Peter Maydell 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
Signed-off-by: Peter Maydell
---
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(-)


Reviewed-by: Richard Henderson 

r~

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

[Jonathan Cameron via]

On Tue, 16 Jan 2024 16:56:05 +
Peter Maydell  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 
> Signed-off-by: Peter Maydell 

Matches what I have locally from discussion earlier.

Tested-by: Jonathan Cameron 
Reviewed-by: Jonathan Cameron 

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) {

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

[Peter Maydell]

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 
Signed-off-by: Peter Maydell 
---
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) {
-- 
2.34.1