Re: [Qemu-arm] [PATCH v2 for-4.1] target/arm: NS BusFault on vector table fetch escalates to NS HardFault

[Peter Maydell]

Ping for code review, please?

thanks
-- PMM

On Fri, 5 Jul 2019 at 10:48, Peter Maydell <address@hidden> wrote:
>
> In the M-profile architecture, when we do a vector table fetch and it
> fails, we need to report a HardFault.  Whether this is a Secure HF or
> a NonSecure HF depends on several things.  If AIRCR.BFHFNMINS is 0
> then HF is always Secure, because there is no NonSecure HardFault.
> Otherwise, the answer depends on whether the 'underlying exception'
> (MemManage, BusFault, SecureFault) targets Secure or NonSecure.  (In
> the pseudocode, this is handled in the Vector() function: the final
> exc.isSecure is calculated by looking at the exc.isSecure from the
> exception returned from the memory access, not the isSecure input
> argument.)
>
> We weren't doing this correctly, because we were looking at
> the target security domain of the exception we were trying to
> load the vector table entry for. This produces errors of two kinds:
>  * a load from the NS vector table which hits the "NS access
>    to S memory" SecureFault should end up as a Secure HardFault,
>    but we were raising an NS HardFault
>  * a load from the S vector table which causes a BusFault
>    should raise an NS HardFault if BFHFNMINS == 1 (because
>    in that case all BusFaults are NonSecure), but we were raising
>    a Secure HardFault
>
> Correct the logic.
>
> We also fix a comment error where we claimed that we might
> be escalating MemManage to HardFault, and forgot about SecureFault.
> (Vector loads can never hit MPU access faults, because they're
> always aligned and always use the default address map.)
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This is the one remaining patch from my earlier 'minor M-profile
> bugfixes' series; the rest are in master now.
>
> Changes v1->v2:
>  * rebased on master (function has moved to m_helper.c)
>  * fixed logic bug pointed out by rth
> ---
>  target/arm/m_helper.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
> index 1867435db7d..84609f446e6 100644
> --- a/target/arm/m_helper.c
> +++ b/target/arm/m_helper.c
> @@ -624,7 +624,11 @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, 
> bool targets_secure,
>          if (sattrs.ns) {
>              attrs.secure = false;
>          } else if (!targets_secure) {
> -            /* NS access to S memory */
> +            /*
> +             * NS access to S memory: the underlying exception which we 
> escalate
> +             * to HardFault is SecureFault, which always targets Secure.
> +             */
> +            exc_secure = true;
>              goto load_fail;
>          }
>      }
> @@ -632,6 +636,11 @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, 
> bool targets_secure,
>      vector_entry = address_space_ldl(arm_addressspace(cs, attrs), addr,
>                                       attrs, &result);
>      if (result != MEMTX_OK) {
> +        /*
> +         * Underlying exception is BusFault: its target security state
> +         * depends on BFHFNMINS.
> +         */
> +        exc_secure = !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
>          goto load_fail;
>      }
>      *pvec = vector_entry;
> @@ -641,13 +650,17 @@ load_fail:
>      /*
>       * All vector table fetch fails are reported as HardFault, with
>       * HFSR.VECTTBL and .FORCED set. (FORCED is set because
> -     * technically the underlying exception is a MemManage or BusFault
> +     * technically the underlying exception is a SecureFault or BusFault
>       * that is escalated to HardFault.) This is a terminal exception,
>       * so we will either take the HardFault immediately or else enter
>       * lockup (the latter case is handled in 
> armv7m_nvic_set_pending_derived()).
> +     * The HardFault is Secure if BFHFNMINS is 0 (meaning that all HFs are
> +     * secure); otherwise it targets the same security state as the
> +     * underlying exception.
>       */
> -    exc_secure = targets_secure ||
> -        !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
> +    if (!(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK)) {
> +        exc_secure = true;
> +    }
>      env->v7m.hfsr |= R_V7M_HFSR_VECTTBL_MASK | R_V7M_HFSR_FORCED_MASK;
>      armv7m_nvic_set_pending_derived(env->nvic, ARMV7M_EXCP_HARD, exc_secure);
>      return false;
> --
> 2.20.1