target/arm: Fix CNTPOFF_EL2 trap to missing EL3

[PATCH] target/arm: Fix CNTPOFF_EL2 trap to missing EL3

Posted by Pierre-Clément Tosi 7 months, 3 weeks ago

EL2 accesses to CNTPOFF_EL2 should only ever trap to EL3 if EL3 is
present, as described by the reference manual (for MRS):

  /* ... */
  elsif PSTATE.EL == EL2 then
      if Halted() && HaveEL(EL3) && /*...*/ then
          UNDEFINED;
      elsif HaveEL(EL3) && SCR_EL3.ECVEn == '0' then
          /* ... */
      else
          X[t, 64] = CNTPOFF_EL2;

However, the existing implementation of gt_cntpoff_access() always
returns CP_ACCESS_TRAP_EL3 for EL2 accesses with SCR_EL3.ECVEn unset. In
pseudo-code terminology, this corresponds to assuming that HaveEL(EL3)
is always true, which is wrong. As a result, QEMU panics in
access_check_cp_reg() when started without EL3 and running EL2 code
accessing the register (e.g. any recent KVM booting a guest).

Therefore, add the HaveEL(EL3) check to gt_cntpoff_access().

Cc: qemu-stable@nongnu.org
Fixes: 2808d3b38a52 ("target/arm: Implement FEAT_ECV CNTPOFF_EL2 handling")
Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 target/arm/helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3f3a5b55d4..13ad90cac1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3452,7 +3452,8 @@ static CPAccessResult gt_cntpoff_access(CPUARMState *env,
                                         const ARMCPRegInfo *ri,
                                         bool isread)
 {
-    if (arm_current_el(env) == 2 && !(env->cp15.scr_el3 & SCR_ECVEN)) {
+    if (arm_current_el(env) == 2 && arm_feature(env, ARM_FEATURE_EL3) &&
+        !(env->cp15.scr_el3 & SCR_ECVEN)) {
         return CP_ACCESS_TRAP_EL3;
     }
     return CP_ACCESS_OK;
-- 
2.44.0.478.gd926399ef9-goog


-- 
Pierre

Re: [PATCH] target/arm: Fix CNTPOFF_EL2 trap to missing EL3

Posted by Peter Maydell 7 months, 3 weeks ago

On Thu, 4 Apr 2024 at 17:36, Pierre-Clément Tosi <ptosi@google.com> wrote:
>
> EL2 accesses to CNTPOFF_EL2 should only ever trap to EL3 if EL3 is
> present, as described by the reference manual (for MRS):
>
>   /* ... */
>   elsif PSTATE.EL == EL2 then
>       if Halted() && HaveEL(EL3) && /*...*/ then
>           UNDEFINED;
>       elsif HaveEL(EL3) && SCR_EL3.ECVEn == '0' then
>           /* ... */
>       else
>           X[t, 64] = CNTPOFF_EL2;
>
> However, the existing implementation of gt_cntpoff_access() always
> returns CP_ACCESS_TRAP_EL3 for EL2 accesses with SCR_EL3.ECVEn unset. In
> pseudo-code terminology, this corresponds to assuming that HaveEL(EL3)
> is always true, which is wrong. As a result, QEMU panics in
> access_check_cp_reg() when started without EL3 and running EL2 code
> accessing the register (e.g. any recent KVM booting a guest).
>
> Therefore, add the HaveEL(EL3) check to gt_cntpoff_access().
>
> Cc: qemu-stable@nongnu.org
> Fixes: 2808d3b38a52 ("target/arm: Implement FEAT_ECV CNTPOFF_EL2 handling")
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>

Oops, thanks for the fix. I'll get this in for the 9.0
release, so we won't need to backport it to stable branches
(the commit breaking this only went in in this cycle).



Applied to target-arm.next, thanks.

-- PMM