| Message ID | 20240318093546.2786144-10-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI | expand |
On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan <ruanjinjie@huawei.com> wrote: > > Set or clear PSTATE.ALLINT on taking an exception to ELx according to the > SCTLR_ELx.SPINTMASK bit. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > v3: > - Add Reviewed-by. > --- > target/arm/helper.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 4bc63bf7ca..81f4a8f194 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -11705,6 +11705,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) > } > } > > + if (cpu_isar_feature(aa64_nmi, cpu) && > + (env->cp15.sctlr_el[new_el] & SCTLR_NMI)) { This shouldn't be checking the value of SCTLR_NMI here: the new PSTATE.ALLINT is set to !SPINTMASK even if NMI == 0. (The SPINTMASK bit description is a bit confusing, but the correct behaviour is clear in the AArch64.TakeException() pseudocode.) > + if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPINTMASK)) { > + new_mode |= PSTATE_ALLINT; > + } else { > + new_mode &= ~PSTATE_ALLINT; > + } > + } > + > pstate_write(env, PSTATE_DAIF | new_mode); > env->aarch64 = true; > aarch64_restore_sp(env, new_el); > -- > 2.34.1 > thanks -- PMM
On 2024/3/20 0:47, Peter Maydell wrote: > On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan <ruanjinjie@huawei.com> wrote: >> >> Set or clear PSTATE.ALLINT on taking an exception to ELx according to the >> SCTLR_ELx.SPINTMASK bit. >> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> v3: >> - Add Reviewed-by. >> --- >> target/arm/helper.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index 4bc63bf7ca..81f4a8f194 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -11705,6 +11705,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) >> } >> } >> >> + if (cpu_isar_feature(aa64_nmi, cpu) && >> + (env->cp15.sctlr_el[new_el] & SCTLR_NMI)) { > > This shouldn't be checking the value of SCTLR_NMI here: > the new PSTATE.ALLINT is set to !SPINTMASK even if NMI == 0. > (The SPINTMASK bit description is a bit confusing, but > the correct behaviour is clear in the AArch64.TakeException() > pseudocode.) It seems unreasonable to remove the SCTLR_NMI check, because if the hardware supports FEAT_NMI but the kernel do not enable it, the ALLINT bit in pstate will also set or clear when an exception is caught, which seems unreasonable. > >> + if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPINTMASK)) { >> + new_mode |= PSTATE_ALLINT; >> + } else { >> + new_mode &= ~PSTATE_ALLINT; >> + } >> + } >> + >> pstate_write(env, PSTATE_DAIF | new_mode); >> env->aarch64 = true; >> aarch64_restore_sp(env, new_el); >> -- >> 2.34.1 >> > > thanks > -- PMM
On Thu, 28 Mar 2024 at 08:57, Jinjie Ruan via <qemu-arm@nongnu.org> wrote: > > > > On 2024/3/20 0:47, Peter Maydell wrote: > > On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan <ruanjinjie@huawei.com> wrote: > >> > >> Set or clear PSTATE.ALLINT on taking an exception to ELx according to the > >> SCTLR_ELx.SPINTMASK bit. > >> > >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> v3: > >> - Add Reviewed-by. > >> --- > >> target/arm/helper.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/target/arm/helper.c b/target/arm/helper.c > >> index 4bc63bf7ca..81f4a8f194 100644 > >> --- a/target/arm/helper.c > >> +++ b/target/arm/helper.c > >> @@ -11705,6 +11705,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) > >> } > >> } > >> > >> + if (cpu_isar_feature(aa64_nmi, cpu) && > >> + (env->cp15.sctlr_el[new_el] & SCTLR_NMI)) { > > > > This shouldn't be checking the value of SCTLR_NMI here: > > the new PSTATE.ALLINT is set to !SPINTMASK even if NMI == 0. > > (The SPINTMASK bit description is a bit confusing, but > > the correct behaviour is clear in the AArch64.TakeException() > > pseudocode.) > > It seems unreasonable to remove the SCTLR_NMI check, because if the > hardware supports FEAT_NMI but the kernel do not enable it, the ALLINT > bit in pstate will also set or clear when an exception is caught, which > seems unreasonable. Whether we personally think it is "unreasonable" or not does not matter here. The architecture says that we must not check SCTLR_NMI, and therefore we must not check SCTLR_NMI, or we will be implementing the wrong behaviour. thanks -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index 4bc63bf7ca..81f4a8f194 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -11705,6 +11705,15 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) } } + if (cpu_isar_feature(aa64_nmi, cpu) && + (env->cp15.sctlr_el[new_el] & SCTLR_NMI)) { + if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPINTMASK)) { + new_mode |= PSTATE_ALLINT; + } else { + new_mode &= ~PSTATE_ALLINT; + } + } + pstate_write(env, PSTATE_DAIF | new_mode); env->aarch64 = true; aarch64_restore_sp(env, new_el);