Re: [RFC PATCH v8 09/23] target/arm: Handle PSTATE.ALLINT on taking an exception
On Thu, 28 Mar 2024 at 08:57, Jinjie Ruan via wrote:
>
>
>
> On 2024/3/20 0:47, Peter Maydell wrote:
> > On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan wrote:
> >>
> >> Set or clear PSTATE.ALLINT on taking an exception to ELx according to the
> >> SCTLR_ELx.SPINTMASK bit.
> >>
> >> Signed-off-by: Jinjie Ruan
> >> Reviewed-by: Richard Henderson
> >> ---
> >> 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
Re: [RFC PATCH v8 09/23] target/arm: Handle PSTATE.ALLINT on taking an exception
On 2024/3/20 0:47, Peter Maydell wrote:
> On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan wrote:
>>
>> Set or clear PSTATE.ALLINT on taking an exception to ELx according to the
>> SCTLR_ELx.SPINTMASK bit.
>>
>> Signed-off-by: Jinjie Ruan
>> Reviewed-by: Richard Henderson
>> ---
>> 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
Re: [RFC PATCH v8 09/23] target/arm: Handle PSTATE.ALLINT on taking an exception
On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan wrote:
>
> Set or clear PSTATE.ALLINT on taking an exception to ELx according to the
> SCTLR_ELx.SPINTMASK bit.
>
> Signed-off-by: Jinjie Ruan
> Reviewed-by: Richard Henderson
> ---
> 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
[RFC PATCH v8 09/23] target/arm: Handle PSTATE.ALLINT on taking an exception
Set or clear PSTATE.ALLINT on taking an exception to ELx according to the
SCTLR_ELx.SPINTMASK bit.
Signed-off-by: Jinjie Ruan
Reviewed-by: Richard Henderson
---
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)) {
+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
