Message ID | 1491486152-24304-1-git-send-email-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
On Thu, Apr 06, 2017 at 02:42:32PM +0100, Peter Maydell wrote: > In tlb_fill() we construct a syndrome register value from a > fault status register value which is filled in by arm_tlb_fill(). > arm_tlb_fill() returns FSR values which might be in the format > used with short-format page descriptors, or the format used > with long-format (LPAE) descriptors. The syndrome register > always uses LPAE-format FSR status codes. > > It isn't actually possible to end up delivering a syndrome > register value to the guest for a fault which is reported > with a short-format FSR (that kind of stage 1 fault will only > happen for an AArch32 translation regime which doesn't have > a syndrome register, and can never be redirected to an AArch64 > or Hyp exception level). Add an assertion which checks this, > and adjust the code so that we construct a syndrome with > an invalid status code, rather than allowing set bits in > the FSR input to randomly corrupt other fields in the syndrome. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Tricky but it looks correct as far as I can follow the specs... Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > It took me a little while to convince myself that you'd > never take a short-format FSR to a using-syndrome EL :-) > --- > target/arm/op_helper.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c > index d64c867..156b825 100644 > --- a/target/arm/op_helper.c > +++ b/target/arm/op_helper.c > @@ -130,7 +130,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, > if (unlikely(ret)) { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > - uint32_t syn, exc; > + uint32_t syn, exc, fsc; > unsigned int target_el; > bool same_el; > > @@ -145,19 +145,32 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, > env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4; > } > same_el = arm_current_el(env) == target_el; > - /* AArch64 syndrome does not have an LPAE bit */ > - syn = fsr & ~(1 << 9); > + > + if (fsr & (1 << 9)) { > + /* LPAE format fault status register : bottom 6 bits are > + * status code in the same form as needed for syndrome > + */ > + fsc = extract32(fsr, 0, 6); > + } else { > + /* Short format FSR : this fault will never actually be reported > + * to an EL that uses a syndrome register. Check that here, > + * and use a (currently) reserved FSR code in case the constructed > + * syndrome does leak into the guest somehow. > + */ > + assert(target_el != 2 && !arm_el_is_aa64(env, target_el)); > + fsc = 0x3f; > + } > > /* For insn and data aborts we assume there is no instruction syndrome > * information; this is always true for exceptions reported to EL1. > */ > if (access_type == MMU_INST_FETCH) { > - syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn); > + syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc); > exc = EXCP_PREFETCH_ABORT; > } else { > syn = merge_syn_data_abort(env->exception.syndrome, target_el, > same_el, fi.s1ptw, > - access_type == MMU_DATA_STORE, syn); > + access_type == MMU_DATA_STORE, fsc); > if (access_type == MMU_DATA_STORE > && arm_feature(env, ARM_FEATURE_V6)) { > fsr |= (1 << 11); > -- > 2.7.4 > >
On Thu, Apr 06, 2017 at 02:42:32PM +0100, Peter Maydell wrote: > In tlb_fill() we construct a syndrome register value from a > fault status register value which is filled in by arm_tlb_fill(). > arm_tlb_fill() returns FSR values which might be in the format > used with short-format page descriptors, or the format used > with long-format (LPAE) descriptors. The syndrome register > always uses LPAE-format FSR status codes. > > It isn't actually possible to end up delivering a syndrome > register value to the guest for a fault which is reported > with a short-format FSR (that kind of stage 1 fault will only > happen for an AArch32 translation regime which doesn't have > a syndrome register, and can never be redirected to an AArch64 > or Hyp exception level). Add an assertion which checks this, > and adjust the code so that we construct a syndrome with > an invalid status code, rather than allowing set bits in > the FSR input to randomly corrupt other fields in the syndrome. BTW, Have you seen something suspicous when running code or was this a theoretical issue? Cheers, Edgar > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > It took me a little while to convince myself that you'd > never take a short-format FSR to a using-syndrome EL :-) > --- > target/arm/op_helper.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c > index d64c867..156b825 100644 > --- a/target/arm/op_helper.c > +++ b/target/arm/op_helper.c > @@ -130,7 +130,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, > if (unlikely(ret)) { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > - uint32_t syn, exc; > + uint32_t syn, exc, fsc; > unsigned int target_el; > bool same_el; > > @@ -145,19 +145,32 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, > env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4; > } > same_el = arm_current_el(env) == target_el; > - /* AArch64 syndrome does not have an LPAE bit */ > - syn = fsr & ~(1 << 9); > + > + if (fsr & (1 << 9)) { > + /* LPAE format fault status register : bottom 6 bits are > + * status code in the same form as needed for syndrome > + */ > + fsc = extract32(fsr, 0, 6); > + } else { > + /* Short format FSR : this fault will never actually be reported > + * to an EL that uses a syndrome register. Check that here, > + * and use a (currently) reserved FSR code in case the constructed > + * syndrome does leak into the guest somehow. > + */ > + assert(target_el != 2 && !arm_el_is_aa64(env, target_el)); > + fsc = 0x3f; > + } > > /* For insn and data aborts we assume there is no instruction syndrome > * information; this is always true for exceptions reported to EL1. > */ > if (access_type == MMU_INST_FETCH) { > - syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn); > + syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc); > exc = EXCP_PREFETCH_ABORT; > } else { > syn = merge_syn_data_abort(env->exception.syndrome, target_el, > same_el, fi.s1ptw, > - access_type == MMU_DATA_STORE, syn); > + access_type == MMU_DATA_STORE, fsc); > if (access_type == MMU_DATA_STORE > && arm_feature(env, ARM_FEATURE_V6)) { > fsr |= (1 << 11); > -- > 2.7.4 > >
On 10 April 2017 at 09:44, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > Have you seen something suspicous when running code or was > this a theoretical issue? No, I was looking at the code for a different reason and was surprised that the syndrome-generation code wasn't doing anything to handle short-format FSRs. I started out thinking that this was just a corner case we'd forgotten since it only affects AArch32 guests (and even wrote some code to do short-to-long status code conversion), but after some thought I realised that it's not possible to get to a point where we would need the syndrome after such a fault. It's a tricky enough point that it seems worth having an assert and some comments to assure future readers that it isn't just something we forgot to handle... thanks -- PMM
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index d64c867..156b825 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -130,7 +130,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, if (unlikely(ret)) { ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; - uint32_t syn, exc; + uint32_t syn, exc, fsc; unsigned int target_el; bool same_el; @@ -145,19 +145,32 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type, env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4; } same_el = arm_current_el(env) == target_el; - /* AArch64 syndrome does not have an LPAE bit */ - syn = fsr & ~(1 << 9); + + if (fsr & (1 << 9)) { + /* LPAE format fault status register : bottom 6 bits are + * status code in the same form as needed for syndrome + */ + fsc = extract32(fsr, 0, 6); + } else { + /* Short format FSR : this fault will never actually be reported + * to an EL that uses a syndrome register. Check that here, + * and use a (currently) reserved FSR code in case the constructed + * syndrome does leak into the guest somehow. + */ + assert(target_el != 2 && !arm_el_is_aa64(env, target_el)); + fsc = 0x3f; + } /* For insn and data aborts we assume there is no instruction syndrome * information; this is always true for exceptions reported to EL1. */ if (access_type == MMU_INST_FETCH) { - syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn); + syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc); exc = EXCP_PREFETCH_ABORT; } else { syn = merge_syn_data_abort(env->exception.syndrome, target_el, same_el, fi.s1ptw, - access_type == MMU_DATA_STORE, syn); + access_type == MMU_DATA_STORE, fsc); if (access_type == MMU_DATA_STORE && arm_feature(env, ARM_FEATURE_V6)) { fsr |= (1 << 11);
In tlb_fill() we construct a syndrome register value from a fault status register value which is filled in by arm_tlb_fill(). arm_tlb_fill() returns FSR values which might be in the format used with short-format page descriptors, or the format used with long-format (LPAE) descriptors. The syndrome register always uses LPAE-format FSR status codes. It isn't actually possible to end up delivering a syndrome register value to the guest for a fault which is reported with a short-format FSR (that kind of stage 1 fault will only happen for an AArch32 translation regime which doesn't have a syndrome register, and can never be redirected to an AArch64 or Hyp exception level). Add an assertion which checks this, and adjust the code so that we construct a syndrome with an invalid status code, rather than allowing set bits in the FSR input to randomly corrupt other fields in the syndrome. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- It took me a little while to convince myself that you'd never take a short-format FSR to a using-syndrome EL :-) --- target/arm/op_helper.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) -- 2.7.4