diff mbox series

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

Message ID 20230802170157.401491-6-jean-philippe@linaro.org
State Superseded
Headers show
Series target/arm: Fixes for RME | expand

Commit Message

Jean-Philippe Brucker Aug. 2, 2023, 5:01 p.m. UTC 
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(-)

Comments

Peter Maydell Aug. 4, 2023, 6:08 p.m. UTC | #1 
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.

> +    return CP_ACCESS_OK;
> +}
> +
>  static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                       bool isread)
>  {

thanks
-- PMM
Peter Maydell Aug. 7, 2023, 9:54 a.m. UTC | #2 
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.

thanks
-- PMM
Jean-Philippe Brucker Aug. 7, 2023, 2:03 p.m. UTC | #3 
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

Thanks,
Jean
Peter Maydell Aug. 7, 2023, 3:08 p.m. UTC | #4 
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
diff mbox series

Patch

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;
+    }
+    return CP_ACCESS_OK;
+}
+
 static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                      bool isread)
 {
@@ -3623,7 +3637,7 @@  static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
         !(env->cp15.scr_el3 & (SCR_NS | SCR_EEL2))) {
         return CP_ACCESS_TRAP;
     }
-    return CP_ACCESS_OK;
+    return at_e012_access(env, ri, isread);
 }
 
 static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -5505,38 +5519,38 @@  static const ARMCPRegInfo v8_cp_reginfo[] = {
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 0,
       .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
       .fgt = FGT_ATS1E1R,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S1E1W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 1,
       .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
       .fgt = FGT_ATS1E1W,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S1E0R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 2,
       .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
       .fgt = FGT_ATS1E0R,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S1E0W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 3,
       .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
       .fgt = FGT_ATS1E0W,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S12E1R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 4,
       .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S12E1W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 5,
       .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S12E0R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 6,
       .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S12E0W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 7,
       .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     /* AT S1E2* are elsewhere as they UNDEF from EL3 if EL2 is not present */
     { .name = "AT_S1E3R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 6, .crn = 7, .crm = 8, .opc2 = 0,
@@ -8078,12 +8092,12 @@  static const ARMCPRegInfo ats1e1_reginfo[] = {
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 9, .opc2 = 0,
       .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
       .fgt = FGT_ATS1E1RP,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
     { .name = "AT_S1E1WP", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 9, .opc2 = 1,
       .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
       .fgt = FGT_ATS1E1WP,
-      .writefn = ats_write64 },
+      .accessfn = at_e012_access, .writefn = ats_write64 },
 };
 
 static const ARMCPRegInfo ats1cp_reginfo[] = {