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();
             }
         } else {
             /*
-- 
2.25.1

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