* [PATCH 0/2] HYP boot mode fixes
@ 2013-01-04 17:44 Marc Zyngier
2013-01-04 17:44 ` [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point Marc Zyngier
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Marc Zyngier @ 2013-01-04 17:44 UTC (permalink / raw)
To: linux-arm-kernel
Here's a couple of patches fixing issues that have been recently
brought to my attention.
The first patch only affects configurations that are already broken
(CPUs entering the kernel with different modes). The second one is a
bit more serious, and potentially affects setups where the secondary
core is brought up using an interrupt that is still pending when the
kernel is entered.
Both patches have been tested on TC2, and are based on v3.8-rc2.
Marc Zyngier (2):
ARM: hyp: boot secondary CPUs through the right entry point
ARM: hyp: simplify __hyp_stub_install epilog
arch/arm/kernel/head.S | 2 +-
arch/arm/kernel/hyp-stub.S | 12 +++---------
2 files changed, 4 insertions(+), 10 deletions(-)
--
1.8.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point 2013-01-04 17:44 [PATCH 0/2] HYP boot mode fixes Marc Zyngier @ 2013-01-04 17:44 ` Marc Zyngier 2013-01-07 12:26 ` Dave Martin 2013-01-04 17:44 ` [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog Marc Zyngier 2013-01-09 14:46 ` [PATCH 0/2] HYP boot mode fixes Will Deacon 2 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2013-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel Secondary CPUs should use the __hyp_stub_install_secondary entry point, so boot mode inconsistencies can be detected. Reported-by: Ian Molton <ian.molton@collabora.co.uk> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/kernel/head.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S index 4eee351..16abc83 100644 --- a/arch/arm/kernel/head.S +++ b/arch/arm/kernel/head.S @@ -331,7 +331,7 @@ ENTRY(secondary_startup) * as it has already been validated by the primary processor. */ #ifdef CONFIG_ARM_VIRT_EXT - bl __hyp_stub_install + bl __hyp_stub_install_secondary #endif safe_svcmode_maskall r9 -- 1.8.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point 2013-01-04 17:44 ` [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point Marc Zyngier @ 2013-01-07 12:26 ` Dave Martin 0 siblings, 0 replies; 9+ messages in thread From: Dave Martin @ 2013-01-07 12:26 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jan 04, 2013 at 05:44:14PM +0000, Marc Zyngier wrote: > Secondary CPUs should use the __hyp_stub_install_secondary entry > point, so boot mode inconsistencies can be detected. > > Reported-by: Ian Molton <ian.molton@collabora.co.uk> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> I was just going to report this, but I see I was beaten to it... Acked-by: Dave Martin <dave.martin@linaro.org> Cheers ---Dave > --- > arch/arm/kernel/head.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S > index 4eee351..16abc83 100644 > --- a/arch/arm/kernel/head.S > +++ b/arch/arm/kernel/head.S > @@ -331,7 +331,7 @@ ENTRY(secondary_startup) > * as it has already been validated by the primary processor. > */ > #ifdef CONFIG_ARM_VIRT_EXT > - bl __hyp_stub_install > + bl __hyp_stub_install_secondary > #endif > safe_svcmode_maskall r9 > > -- > 1.8.1 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog 2013-01-04 17:44 [PATCH 0/2] HYP boot mode fixes Marc Zyngier 2013-01-04 17:44 ` [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point Marc Zyngier @ 2013-01-04 17:44 ` Marc Zyngier 2013-01-07 12:18 ` Dave Martin 2013-01-09 14:46 ` [PATCH 0/2] HYP boot mode fixes Will Deacon 2 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2013-01-04 17:44 UTC (permalink / raw) To: linux-arm-kernel __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(-) 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog 2013-01-04 17:44 ` [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog Marc Zyngier @ 2013-01-07 12:18 ` Dave Martin 2013-01-07 13:27 ` Marc Zyngier 0 siblings, 1 reply; 9+ messages in thread From: Dave Martin @ 2013-01-07 12:18 UTC (permalink / raw) To: linux-arm-kernel 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog 2013-01-07 12:18 ` Dave Martin @ 2013-01-07 13:27 ` Marc Zyngier 2013-01-07 13:49 ` Dave Martin 0 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2013-01-07 13:27 UTC (permalink / raw) To: linux-arm-kernel 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. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog 2013-01-07 13:27 ` Marc Zyngier @ 2013-01-07 13:49 ` Dave Martin 0 siblings, 0 replies; 9+ messages in thread From: Dave Martin @ 2013-01-07 13:49 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 0/2] HYP boot mode fixes 2013-01-04 17:44 [PATCH 0/2] HYP boot mode fixes Marc Zyngier 2013-01-04 17:44 ` [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point Marc Zyngier 2013-01-04 17:44 ` [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog Marc Zyngier @ 2013-01-09 14:46 ` Will Deacon 2013-01-09 14:48 ` Marc Zyngier 2 siblings, 1 reply; 9+ messages in thread From: Will Deacon @ 2013-01-09 14:46 UTC (permalink / raw) To: linux-arm-kernel Hi Marc, On Fri, Jan 04, 2013 at 05:44:13PM +0000, Marc Zyngier wrote: > Here's a couple of patches fixing issues that have been recently > brought to my attention. > > The first patch only affects configurations that are already broken > (CPUs entering the kernel with different modes). The second one is a > bit more serious, and potentially affects setups where the secondary > core is brought up using an interrupt that is still pending when the > kernel is entered. > > Both patches have been tested on TC2, and are based on v3.8-rc2. I've been collecting hyp-boot patches since before Christmas so, if it's alright with you, I'll take these into the branch I currently have and send that lot via Russell. Check my virt/hyp-boot branch if you're interested. Cheers, Will ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 0/2] HYP boot mode fixes 2013-01-09 14:46 ` [PATCH 0/2] HYP boot mode fixes Will Deacon @ 2013-01-09 14:48 ` Marc Zyngier 0 siblings, 0 replies; 9+ messages in thread From: Marc Zyngier @ 2013-01-09 14:48 UTC (permalink / raw) To: linux-arm-kernel On 09/01/13 14:46, Will Deacon wrote: > Hi Marc, > > On Fri, Jan 04, 2013 at 05:44:13PM +0000, Marc Zyngier wrote: >> Here's a couple of patches fixing issues that have been recently >> brought to my attention. >> >> The first patch only affects configurations that are already broken >> (CPUs entering the kernel with different modes). The second one is a >> bit more serious, and potentially affects setups where the secondary >> core is brought up using an interrupt that is still pending when the >> kernel is entered. >> >> Both patches have been tested on TC2, and are based on v3.8-rc2. > > I've been collecting hyp-boot patches since before Christmas so, if it's > alright with you, I'll take these into the branch I currently have and send > that lot via Russell. Check my virt/hyp-boot branch if you're interested. Please do. You may want to Cc stable on the second one, as it fixes a observable issue. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-01-09 14:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-04 17:44 [PATCH 0/2] HYP boot mode fixes Marc Zyngier 2013-01-04 17:44 ` [PATCH 1/2] ARM: hyp: boot secondary CPUs through the right entry point Marc Zyngier 2013-01-07 12:26 ` Dave Martin 2013-01-04 17:44 ` [PATCH 2/2] ARM: hyp: simplify __hyp_stub_install epilog Marc Zyngier 2013-01-07 12:18 ` Dave Martin 2013-01-07 13:27 ` Marc Zyngier 2013-01-07 13:49 ` Dave Martin 2013-01-09 14:46 ` [PATCH 0/2] HYP boot mode fixes Will Deacon 2013-01-09 14:48 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).