Message ID | 20221017092432.546881-1-ake@igel.co.jp |
---|---|
State | New |
Headers | show |
Series | target/arm: honor HCR_E2H and HCR_TGE in arm_excp_unmasked() | expand |
On Mon, 17 Oct 2022 at 10:29, Ake Koomsin <ake@igel.co.jp> wrote: > > An exception targeting EL2 from lower EL is actually maskable when > HCR_E2H and HCR_TGE are both set. This applies to both secure and > non-secure Security state. > > Signed-off-by: Ake Koomsin <ake@igel.co.jp> > --- > target/arm/cpu.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 94ca6f163f..86d3377d3f 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -561,14 +561,24 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, > if ((target_el > cur_el) && (target_el != 1)) { > /* Exceptions targeting a higher EL may not be maskable */ > if (arm_feature(env, ARM_FEATURE_AARCH64)) { > - /* > - * 64-bit masking rules are simple: exceptions to EL3 > - * can't be masked, and exceptions to EL2 can only be > - * masked from Secure state. The HCR and SCR settings > - * don't affect the masking logic, only the interrupt routing. > - */ > - if (target_el == 3 || !secure || (env->cp15.scr_el3 & SCR_EEL2)) { > + switch (target_el) { > + case 2: > + /* > + * According to ARM DDI 0487H.a, an interrupt can be masked > + * when HCR_E2H and HCR_TGE are both set regardless of the > + * current Security state. Note that We need to revisit this > + * part again once we need to support NMI. > + */ > + if ((hcr_el2 & (HCR_E2H | HCR_TGE)) != (HCR_E2H | HCR_TGE)) { > + unmasked = true; > + } > + break; > + case 3: > + /* Interrupt cannot be masked when the target EL is 3 */ > unmasked = true; > + break; > + default: > + g_assert_not_reached(); Hi; thanks for this patch. You're correct that there is a bug here, but it took me a while to work out why it's OK to remove the "don't allow masking if we're in Secure EL0/EL1 and the exception is going to NS EL2" check. The answer to that turns out to be "the target_el could never be 2 anyway in that case": because arm_phys_excp_target_el() uses arm_hcr_el2_eff(), if we're Secure and SEL2 isn't enabled then the effective HCR_EL2 bits will all be zero, which forces us into the "targets EL1" or "targets EL3" cases. So I think that's worth mentioning in the commit message: "We can remove the conditions that try to suppress masking of interrupts when we are Secure and the exception targets EL2 and Secure EL2 is disabled, because in that case arm_phys_excp_target_el() will never return 2 as the target EL. The 'not if secure' check in this function was originally written before arm_hcr_el2_eff(), and back then the target EL could be 2 even if we were in Secure EL0/EL1; it is no longer needed." I'll apply this to target-arm.next, with the commit message updated to include that. thanks -- PMM
On Tue, Oct 25, 2022 at 1:37 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 17 Oct 2022 at 10:29, Ake Koomsin <ake@igel.co.jp> wrote: > > > > An exception targeting EL2 from lower EL is actually maskable when > > HCR_E2H and HCR_TGE are both set. This applies to both secure and > > non-secure Security state. > > > > Signed-off-by: Ake Koomsin <ake@igel.co.jp> > > --- > > target/arm/cpu.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 94ca6f163f..86d3377d3f 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -561,14 +561,24 @@ static inline bool arm_excp_unmasked(CPUState *cs, > unsigned int excp_idx, > > if ((target_el > cur_el) && (target_el != 1)) { > > /* Exceptions targeting a higher EL may not be maskable */ > > if (arm_feature(env, ARM_FEATURE_AARCH64)) { > > - /* > > - * 64-bit masking rules are simple: exceptions to EL3 > > - * can't be masked, and exceptions to EL2 can only be > > - * masked from Secure state. The HCR and SCR settings > > - * don't affect the masking logic, only the interrupt > routing. > > - */ > > - if (target_el == 3 || !secure || (env->cp15.scr_el3 & > SCR_EEL2)) { > > + switch (target_el) { > > + case 2: > > + /* > > + * According to ARM DDI 0487H.a, an interrupt can be > masked > > + * when HCR_E2H and HCR_TGE are both set regardless of > the > > + * current Security state. Note that We need to revisit > this > > + * part again once we need to support NMI. > > + */ > > + if ((hcr_el2 & (HCR_E2H | HCR_TGE)) != (HCR_E2H | > HCR_TGE)) { > > + unmasked = true; > > + } > > + break; > > + case 3: > > + /* Interrupt cannot be masked when the target EL is 3 */ > > unmasked = true; > > + break; > > + default: > > + g_assert_not_reached(); > > Hi; thanks for this patch. You're correct that there is a bug here, but > it took me a while to work out why it's OK to remove the "don't allow > masking if we're in Secure EL0/EL1 and the exception is going to NS EL2" > check. The answer to that turns out to be "the target_el could never be > 2 anyway in that case": because arm_phys_excp_target_el() uses > arm_hcr_el2_eff(), > if we're Secure and SEL2 isn't enabled then the effective HCR_EL2 bits > will all be zero, which forces us into the "targets EL1" or "targets EL3" > cases. > > So I think that's worth mentioning in the commit message: > > "We can remove the conditions that try to suppress masking of > interrupts when we are Secure and the exception targets EL2 and > Secure EL2 is disabled, because in that case arm_phys_excp_target_el() > will never return 2 as the target EL. The 'not if secure' check > in this function was originally written before arm_hcr_el2_eff(), and > back then the target EL could be 2 even if we were in Secure EL0/EL1; > it is no longer needed." > > I'll apply this to target-arm.next, with the commit message > updated to include that. > > thanks > -- PMM > Thank you very much for the review and additional commit commit message for clarity. --- Ake Koomsin
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 94ca6f163f..86d3377d3f 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -561,14 +561,24 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx, if ((target_el > cur_el) && (target_el != 1)) { /* Exceptions targeting a higher EL may not be maskable */ if (arm_feature(env, ARM_FEATURE_AARCH64)) { - /* - * 64-bit masking rules are simple: exceptions to EL3 - * can't be masked, and exceptions to EL2 can only be - * masked from Secure state. The HCR and SCR settings - * don't affect the masking logic, only the interrupt routing. - */ - if (target_el == 3 || !secure || (env->cp15.scr_el3 & SCR_EEL2)) { + switch (target_el) { + case 2: + /* + * According to ARM DDI 0487H.a, an interrupt can be masked + * when HCR_E2H and HCR_TGE are both set regardless of the + * current Security state. Note that We need to revisit this + * part again once we need to support NMI. + */ + if ((hcr_el2 & (HCR_E2H | HCR_TGE)) != (HCR_E2H | HCR_TGE)) { + unmasked = true; + } + break; + case 3: + /* Interrupt cannot be masked when the target EL is 3 */ unmasked = true; + break; + default: + g_assert_not_reached(); } } else { /*
An exception targeting EL2 from lower EL is actually maskable when HCR_E2H and HCR_TGE are both set. This applies to both secure and non-secure Security state. Signed-off-by: Ake Koomsin <ake@igel.co.jp> --- target/arm/cpu.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)