When we added Secure EL2 support, we missed that this needs an update
to the access code for the EL3 physical timer registers.  These are
supposed to UNDEF from Secure EL1 when Secure EL2 is enabled.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ac8cb428925..7ec1e6cfaab 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2387,6 +2387,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
         if (!arm_is_secure(env)) {
             return CP_ACCESS_UNDEFINED;
         }
+        if (arm_is_el2_enabled(env)) {
+            return CP_ACCESS_UNDEFINED;
+        }
         if (!(env->cp15.scr_el3 & SCR_ST)) {
             return CP_ACCESS_TRAP_EL3;
         }
-- 
2.34.1

Peter Maydell <peter.maydell@linaro.org> writes:

> When we added Secure EL2 support, we missed that this needs an update
> to the access code for the EL3 physical timer registers.  These are
> supposed to UNDEF from Secure EL1 when Secure EL2 is enabled.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ac8cb428925..7ec1e6cfaab 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2387,6 +2387,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
>          if (!arm_is_secure(env)) {
>              return CP_ACCESS_UNDEFINED;
>          }

Hmm this failed to apply as b4d3978c2f (target-arm: Add the AArch64 view
of the Secure physical timer) has the above as CP_ACCESS_TRAP. I guess
because I didn't apply 20250130182309.717346-1-peter.maydell@linaro.org.
I guess this needs fixing up for stable.


> +        if (arm_is_el2_enabled(env)) {
> +            return CP_ACCESS_UNDEFINED;
> +        }
>          if (!(env->cp15.scr_el3 & SCR_ST)) {
>              return CP_ACCESS_TRAP_EL3;
>          }


Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On Fri, 21 Feb 2025 at 18:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > When we added Secure EL2 support, we missed that this needs an update
> > to the access code for the EL3 physical timer registers.  These are
> > supposed to UNDEF from Secure EL1 when Secure EL2 is enabled.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/helper.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index ac8cb428925..7ec1e6cfaab 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2387,6 +2387,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
> >          if (!arm_is_secure(env)) {
> >              return CP_ACCESS_UNDEFINED;
> >          }
>
> Hmm this failed to apply as b4d3978c2f (target-arm: Add the AArch64 view
> of the Secure physical timer) has the above as CP_ACCESS_TRAP. I guess
> because I didn't apply 20250130182309.717346-1-peter.maydell@linaro.org.
> I guess this needs fixing up for stable.

There is a Based-on: tag in the cover letter which will tell you
what this series should be based on if you want to apply it.

Yes, we'll need to either tweak this commit for stable
(i.e. use CP_ACCESS_TRAP_UNCATEGORIZED instead of UNDEFINED)
or else pull in the refactoring patches it depends on.

thanks
-- PMM