target/arm: Implement emulation of nested virtualization

[PATCH 24/35] target/arm: Handle FEAT_NV2 changes to when SPSR_EL1.M reports EL2

Posted by Peter Maydell 11 months, 2 weeks ago

With FEAT_NV2, the condition for when SPSR_EL1.M should report that
an exception was taken from EL2 changes.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 45444360f95..38e16c2f8a5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11405,10 +11405,18 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
         aarch64_save_sp(env, arm_current_el(env));
         env->elr_el[new_el] = env->pc;
 
-        if (cur_el == 1 && new_el == 1 &&
-            ((arm_hcr_el2_eff(env) & (HCR_NV | HCR_NV1)) == HCR_NV)) {
-            /* I_ZJRNN: report EL2 in the SPSR by setting M[3:2] to 0b10 */
-            old_mode = deposit32(old_mode, 2, 2, 2);
+        if (cur_el == 1 && new_el == 1) {
+            uint64_t hcr = arm_hcr_el2_eff(env);
+            if ((hcr & (HCR_NV | HCR_NV1 | HCR_NV2)) == HCR_NV ||
+                (hcr & (HCR_NV | HCR_NV2)) == (HCR_NV | HCR_NV2)) {
+                /*
+                 * FEAT_NV, FEAT_NV2 may need to report EL2 in the SPSR
+                 * by setting M[3:2] to 0b10.
+                 * If NV2 is disabled, change SPSR when NV,NV1 == 1,0 (I_ZJRNN)
+                 * If NV2 is enabled, change SPSR when NV is 1 (I_DBTLM)
+                 */
+                old_mode = deposit32(old_mode, 2, 2, 2);
+            }
         }
     } else {
         old_mode = cpsr_read_for_spsr_elx(env);
-- 
2.34.1

Re: [PATCH 24/35] target/arm: Handle FEAT_NV2 changes to when SPSR_EL1.M reports EL2

Posted by Richard Henderson 11 months ago

On 12/18/23 22:32, Peter Maydell wrote:
> With FEAT_NV2, the condition for when SPSR_EL1.M should report that
> an exception was taken from EL2 changes.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 45444360f95..38e16c2f8a5 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11405,10 +11405,18 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>           aarch64_save_sp(env, arm_current_el(env));
>           env->elr_el[new_el] = env->pc;
>   
> -        if (cur_el == 1 && new_el == 1 &&
> -            ((arm_hcr_el2_eff(env) & (HCR_NV | HCR_NV1)) == HCR_NV)) {
> -            /* I_ZJRNN: report EL2 in the SPSR by setting M[3:2] to 0b10 */
> -            old_mode = deposit32(old_mode, 2, 2, 2);
> +        if (cur_el == 1 && new_el == 1) {
> +            uint64_t hcr = arm_hcr_el2_eff(env);
> +            if ((hcr & (HCR_NV | HCR_NV1 | HCR_NV2)) == HCR_NV ||
> +                (hcr & (HCR_NV | HCR_NV2)) == (HCR_NV | HCR_NV2)) {

Maybe clearer as

	if ((hcr & HCR_NV) && ((hcr & HCR_NV2) || !(hcr & HCR_NV1)))

?

Anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> +                /*
> +                 * FEAT_NV, FEAT_NV2 may need to report EL2 in the SPSR
> +                 * by setting M[3:2] to 0b10.
> +                 * If NV2 is disabled, change SPSR when NV,NV1 == 1,0 (I_ZJRNN)
> +                 * If NV2 is enabled, change SPSR when NV is 1 (I_DBTLM)
> +                 */
> +                old_mode = deposit32(old_mode, 2, 2, 2);
> +            }
>           }
>       } else {
>           old_mode = cpsr_read_for_spsr_elx(env);

Re: [PATCH 24/35] target/arm: Handle FEAT_NV2 changes to when SPSR_EL1.M reports EL2

Posted by Peter Maydell 10 months, 3 weeks ago

On Wed, 27 Dec 2023 at 23:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/18/23 22:32, Peter Maydell wrote:
> > With FEAT_NV2, the condition for when SPSR_EL1.M should report that
> > an exception was taken from EL2 changes.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   target/arm/helper.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 45444360f95..38e16c2f8a5 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -11405,10 +11405,18 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
> >           aarch64_save_sp(env, arm_current_el(env));
> >           env->elr_el[new_el] = env->pc;
> >
> > -        if (cur_el == 1 && new_el == 1 &&
> > -            ((arm_hcr_el2_eff(env) & (HCR_NV | HCR_NV1)) == HCR_NV)) {
> > -            /* I_ZJRNN: report EL2 in the SPSR by setting M[3:2] to 0b10 */
> > -            old_mode = deposit32(old_mode, 2, 2, 2);
> > +        if (cur_el == 1 && new_el == 1) {
> > +            uint64_t hcr = arm_hcr_el2_eff(env);
> > +            if ((hcr & (HCR_NV | HCR_NV1 | HCR_NV2)) == HCR_NV ||
> > +                (hcr & (HCR_NV | HCR_NV2)) == (HCR_NV | HCR_NV2)) {
>
> Maybe clearer as
>
>         if ((hcr & HCR_NV) && ((hcr & HCR_NV2) || !(hcr & HCR_NV1)))
>
> ?

I dunno; I went back and forth a bit on how to write this, but
I think what I have is a little closer to how the Arm ARM
defines it (as separate FEAT_NV vs FEAT_NV2 conditions). At any rate,
I don't think your suggestion sufficiently better to do the
work to make the change :-)

-- PMM