Re: [Qemu-devel] [PATCH v8 14/27] target-arm: respect SCR.FW, SCR.AW and SCTLR.NMFI

[Greg Bellows]

On 31 October 2014 09:18, Peter Maydell <address@hidden> wrote:

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> bits when modifying CPSR.

I prefer it if we don't continue sentences from the subject
line into the main commit message body like this, it makes
them rather odd to read.

I agree and I thought I fixed this with the other one you pointed out.  Fixed in v9.

 

>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v7 -> v8
> - Fixed incorrect use of env->uncached_cpsr A/I/F to use env->daif instead.
> - Removed incorrect statement about SPSR to CPSR copies being affected by
>   SCR.AW/FW.
> - Fix typo in comment.
> - Simpified cpsr_write logic
>
> v3 -> v4
> - Fixed up conditions for ignoring CPSR.A/F updates by isolating to v7 and
>   checking for the existence of EL3 and non-existence of EL2.
> ---
>  target-arm/helper.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 466459b..03e6b62 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3666,9 +3666,6 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>          env->GE = (val >> 16) & 0xf;
>      }
>
> -    env->daif &= ~(CPSR_AIF & mask);
> -    env->daif |= val & CPSR_AIF & mask;
> -
>      if ((env->uncached_cpsr ^ val) & mask & CPSR_M) {
>          if (bad_mode_switch(env, val & CPSR_M)) {
>              /* Attempt to switch to an invalid mode: this is UNPREDICTABLE.
> @@ -3680,6 +3677,50 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>              switch_mode(env, val & CPSR_M);
>          }
>      }
> +

You've put this code hunk below the section of this function
which updates the mode bits in the CPU state. That means we'll
do the arm_is_secure() and BANKED_CURRENT_REG_GET below as
if from the mode we're going to, not the mode we started out in.
This is wrong -- compare the CPSRWriteByInstr pseudocode function,
which updates the mode field as the last thing it does.

Good catch.  I moved the AIF masking logic above the mode switch conditional in v9.

 

> +    /* In a V7 implementation that includes the security extensions but does
> +     * not include Virtualization Extensions the SCR.FW and SCR.AW bits control
> +     * whether non-secure software is allowed to change the CPSR_F and CPSR_A
> +     * bits respectively.
> +     *
> +     * In a V8 implementation, it is permitted for privileged software to
> +     * change the CPSR A/F bits regardless of the SCR.AW/FW bits.
> +     */
> +    if (!arm_feature(env, ARM_FEATURE_V8) &&
> +        arm_feature(env, ARM_FEATURE_EL3) &&
> +        !arm_feature(env, ARM_FEATURE_EL2) &&
> +        !arm_is_secure(env)) {
> +        if (!(env->cp15.scr_el3 & SCR_AW)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "Ignoring attempt to switch CPSR_A flag from "
> +                          "non-secure world with SCR.AW bit clear\n");

This logging is now incorrect, because it will trigger even if the
guest wasn't attempting to change the value of CPSR.A. You could
either just drop the logging or alternatively only log
if ((env->daif ^ val) & mask & CPSR_A)
I guess.

True.  So the change to take out what we thought was pre-optimization was somewhat needed.  Adding the check back in as you suggested basically puts the code back where it was.   As you mentioned before, this is not a bottleneck path, so I'll add back in the conditionals and add comments in v9.

 

> +            mask &= ~CPSR_A;
> +        }
> +
> +        if (!(env->cp15.scr_el3 & SCR_FW)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "Ignoring attempt to switch CPSR_F flag from "
> +                          "non-secure world with SCR.FW bit clear\n");
> +            mask &= ~CPSR_F;
> +        }
> +
> +        /* Check whether non-maskable FIQ (NMFI) support is enabled.
> +         * If this bit is set software is not allowed to mask
> +         * FIQs, but is allowed to set CPSR_F to 0.
> +         */
> +        if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_NMFI) &&
> +            (val & CPSR_F)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "Ignoring attempt to enable CPSR_F flag "
> +                          "(non-maskable FIQ [NMFI] support "
> +                          "enabled)\n");
> +            mask &= ~CPSR_F;
> +        }
> +    }
> +
> +    env->daif &= ~(CPSR_AIF & mask);
> +    env->daif |= val & CPSR_AIF & mask;
> +
>      mask &= ~CACHED_CPSR_BITS;
>      env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
>  }
> --
> 1.8.3.2

thanks
-- PMM