| Message ID | 1357321455-9264-3-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 04, 2013 at 05:44:15PM +0000, Marc Zyngier wrote: > __hyp_stub_install duplicates quite a bit of safe_svcmode_maskall > by forcing the CPU back to SVC. This is unnecessary, as > safe_svcmode_maskall is called just after. > > Furthermore, the way we build SPSR_hyp is buggy as we fail to mask > the interrupts, leading to interesting behaviours on TC2 + UEFI. > > The fix is to simply remove this code and rely on safe_svcmode_maskall > to do the right thing. > > Cc: Dave Martin <dave.martin@linaro.org> > Reported-by: Harry Liebel <harry.liebel@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Although there is clearly a bug here, it looks like interrupts will promptly get masked afterwards due to save_svcmode_maskall. This would only fail if there is an interrupts asserted during this hazard ...? Anyway, There's certainly no sense in trying to drop down to SVC mode twice, so I agree that it is better to delegate that to the save_svcmode_maskall macro. Reviewed-by: Dave Martin <dave.martin@linaro.org> Cheers ---Dave > --- > arch/arm/kernel/hyp-stub.S | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S > index 65b2417..da7e19f 100644 > --- a/arch/arm/kernel/hyp-stub.S > +++ b/arch/arm/kernel/hyp-stub.S > @@ -120,7 +120,8 @@ ENTRY(__hyp_stub_install_secondary) > * Eventually, CPU-specific code might be needed -- assume not for now > * > * This code relies on the "eret" instruction to synchronize the > - * various coprocessor accesses. > + * various coprocessor accesses. This is done when we switch to SVC > + * (see safe_svcmode_maskall). > */ > @ Now install the hypervisor stub: > adr r7, __hyp_stub_vectors > @@ -155,14 +156,7 @@ THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE > 1: > #endif > > - bic r7, r4, #MODE_MASK > - orr r7, r7, #SVC_MODE > -THUMB( orr r7, r7, #PSR_T_BIT ) > - msr spsr_cxsf, r7 @ This is SPSR_hyp. > - > - __MSR_ELR_HYP(14) @ msr elr_hyp, lr > - __ERET @ return, switching to SVC mode > - @ The boot CPU mode is left in r4. > + bx lr @ The boot CPU mode is left in r4. > ENDPROC(__hyp_stub_install_secondary) > > __hyp_stub_do_trap: > -- > 1.8.1 > >
On 07/01/13 12:18, Dave Martin wrote: > On Fri, Jan 04, 2013 at 05:44:15PM +0000, Marc Zyngier wrote: >> __hyp_stub_install duplicates quite a bit of safe_svcmode_maskall >> by forcing the CPU back to SVC. This is unnecessary, as >> safe_svcmode_maskall is called just after. >> >> Furthermore, the way we build SPSR_hyp is buggy as we fail to mask >> the interrupts, leading to interesting behaviours on TC2 + UEFI. >> >> The fix is to simply remove this code and rely on safe_svcmode_maskall >> to do the right thing. >> >> Cc: Dave Martin <dave.martin@linaro.org> >> Reported-by: Harry Liebel <harry.liebel@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > Although there is clearly a bug here, it looks like interrupts will > promptly get masked afterwards due to save_svcmode_maskall. This would > only fail if there is an interrupts asserted during this hazard ...? That's exactly the failure case. It's been observed on TC2 with UEFI, where the secondaries are woken up with a SGI. When they get out of WFI, the interrupt is still pending. After reaching this code and doing an eret, the interrupt fires immediately, with deadly consequences. > Anyway, There's certainly no sense in trying to drop down to SVC mode > twice, so I agree that it is better to delegate that to the > save_svcmode_maskall macro. > > Reviewed-by: Dave Martin <dave.martin@linaro.org> Thanks! M.
On Mon, Jan 07, 2013 at 01:27:51PM +0000, Marc Zyngier wrote: > On 07/01/13 12:18, Dave Martin wrote: > > On Fri, Jan 04, 2013 at 05:44:15PM +0000, Marc Zyngier wrote: > >> __hyp_stub_install duplicates quite a bit of safe_svcmode_maskall > >> by forcing the CPU back to SVC. This is unnecessary, as > >> safe_svcmode_maskall is called just after. > >> > >> Furthermore, the way we build SPSR_hyp is buggy as we fail to mask > >> the interrupts, leading to interesting behaviours on TC2 + UEFI. > >> > >> The fix is to simply remove this code and rely on safe_svcmode_maskall > >> to do the right thing. > >> > >> Cc: Dave Martin <dave.martin@linaro.org> > >> Reported-by: Harry Liebel <harry.liebel@arm.com> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > > > Although there is clearly a bug here, it looks like interrupts will > > promptly get masked afterwards due to save_svcmode_maskall. This would > > only fail if there is an interrupts asserted during this hazard ...? > > That's exactly the failure case. It's been observed on TC2 with UEFI, > where the secondaries are woken up with a SGI. When they get out of WFI, > the interrupt is still pending. After reaching this code and doing an > eret, the interrupt fires immediately, with deadly consequences. Ah right. In which case we also have good evidence that the proposed fix fixes it. Sounds good Cheers ---Dave
diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S index 65b2417..da7e19f 100644 --- a/arch/arm/kernel/hyp-stub.S +++ b/arch/arm/kernel/hyp-stub.S @@ -120,7 +120,8 @@ ENTRY(__hyp_stub_install_secondary) * Eventually, CPU-specific code might be needed -- assume not for now * * This code relies on the "eret" instruction to synchronize the - * various coprocessor accesses. + * various coprocessor accesses. This is done when we switch to SVC + * (see safe_svcmode_maskall). */ @ Now install the hypervisor stub: adr r7, __hyp_stub_vectors @@ -155,14 +156,7 @@ THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE 1: #endif - bic r7, r4, #MODE_MASK - orr r7, r7, #SVC_MODE -THUMB( orr r7, r7, #PSR_T_BIT ) - msr spsr_cxsf, r7 @ This is SPSR_hyp. - - __MSR_ELR_HYP(14) @ msr elr_hyp, lr - __ERET @ return, switching to SVC mode - @ The boot CPU mode is left in r4. + bx lr @ The boot CPU mode is left in r4. ENDPROC(__hyp_stub_install_secondary) __hyp_stub_do_trap:
__hyp_stub_install duplicates quite a bit of safe_svcmode_maskall by forcing the CPU back to SVC. This is unnecessary, as safe_svcmode_maskall is called just after. Furthermore, the way we build SPSR_hyp is buggy as we fail to mask the interrupts, leading to interesting behaviours on TC2 + UEFI. The fix is to simply remove this code and rely on safe_svcmode_maskall to do the right thing. Cc: Dave Martin <dave.martin@linaro.org> Reported-by: Harry Liebel <harry.liebel@arm.com> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/kernel/hyp-stub.S | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)