Re: [PATCH v2 5/6] target/arm/helper: Check SCR_EL3.{NSE, NS} encoding for AT instructions

[Peter Maydell]

On Mon, 7 Aug 2023 at 15:03, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Mon, Aug 07, 2023 at 10:54:05AM +0100, Peter Maydell wrote:
> > On Fri, 4 Aug 2023 at 19:08, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Wed, 2 Aug 2023 at 18:02, Jean-Philippe Brucker
> > > <jean-philippe@linaro.org> wrote:
> > > >
> > > > The AT instruction is UNDEFINED if the {NSE,NS} configuration is
> > > > invalid. Add a function to check this on all AT instructions that apply
> > > > to an EL lower than 3.
> > > >
> > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > > ---
> > > >  target/arm/helper.c | 36 +++++++++++++++++++++++++-----------
> > > >  1 file changed, 25 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > > index fbb03c364b..77dd80ad28 100644
> > > > --- a/target/arm/helper.c
> > > > +++ b/target/arm/helper.c
> > > > @@ -3616,6 +3616,20 @@ static void ats1h_write(CPUARMState *env, const 
> > > > ARMCPRegInfo *ri,
> > > >  #endif /* CONFIG_TCG */
> > > >  }
> > > >
> > > > +static CPAccessResult at_e012_access(CPUARMState *env, const 
> > > > ARMCPRegInfo *ri,
> > > > +                                     bool isread)
> > > > +{
> > > > +    /*
> > > > +     * R_NYXTL: instruction is UNDEFINED if it applies to an Exception 
> > > > level
> > > > +     * lower than EL3 and the combination SCR_EL3.{NSE,NS} is reserved.
> > > > +     */
> > > > +    if (cpu_isar_feature(aa64_rme, env_archcpu(env)) &&
> > > > +        (env->cp15.scr_el3 & (SCR_NSE | SCR_NS)) == SCR_NSE) {
> > > > +        return CP_ACCESS_TRAP;
> > > > +    }
> > >
> > > The AArch64.AT() pseudocode and the text in the individual
> > > AT insn descriptions ("When FEAT_RME is implemented, if the Effective
> > > value of SCR_EL3.{NSE, NS} is a reserved value, this instruction is
> > > UNDEFINED at EL3") say that this check needs an "arm_current_el(env) == 3"
> > > condition too.
> >
> > It's been pointed out to me that since trying to return from
> > EL3 with SCR_EL3.{NSE,NS} == {1,0} is an illegal exception return,
> > it's not actually possible to try to execute these insns in this
> > state from any other EL than EL3. So we don't actually need
> > to check for EL3 here.
> >
> > QEMU's implementation of exception return is missing that
> > check for illegal-exception-return on bad {NSE,NS}, though.
>
> I can add a patch to check that exception return condition, and add a
> comment here explaining that this can only happen when executing at EL3

I just sent a patch to do the illegal-exc-ret check; I agree a
comment here would probably help.

With the comment
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM