Ping for code review, please? thanks -- PMM
On Fri, 5 Jul 2019 at 10:48, Peter Maydell <[email protected]> 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 <[email protected]> > --- > 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
