| Message ID | 1367846270-1827-2-git-send-email-andre.przywara@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Andre, On Mon, 6 May 2013 15:17:45 +0200, Andre Przywara <andre.przywara@linaro.org> wrote: > A prerequisite for using virtualization is to be in HYP mode, which > requires the CPU to be in non-secure state. > Introduce a monitor handler routine which switches the CPU to > non-secure state by setting the NS and associated bits. > According to the ARM ARM this should not be done in SVC mode, so we ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is a more precise reference, please provide it. > have to setup a SMC handler for this. We reuse the current vector > table for this and make sure that we only access the MVBAR register > if the CPU supports the security extension and only if we > configured the board to use it, since boards entering u-boot already > in non-secure mode would crash on accessing MVBAR otherwise. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S > index e9e57e6..da48b36 100644 > --- a/arch/arm/cpu/armv7/start.S > +++ b/arch/arm/cpu/armv7/start.S > @@ -155,6 +155,13 @@ reset: > /* Set vector address in CP15 VBAR register */ > ldr r0, =_start > mcr p15, 0, r0, c12, c0, 0 @Set VBAR > + > +#ifdef CONFIG_ARMV7_VIRT > + mrc p15, 0, r1, c0, c1, 1 @ check for security extension > + ands r1, r1, #0x30 > + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR > +#endif > + > #endif > > /* the mask ROM code should have PLL and others stable */ > @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup) > ldr r0, =_start > mcr p15, 0, r0, c12, c0, 0 @Set VBAR > > +#ifdef CONFIG_ARMV7_VIRT > + mrc p15, 0, r1, c0, c1, 1 @ check for security extension > + ands r1, r1, #0x30 > + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR > +#endif > + > bx lr > > ENDPROC(c_runtime_cpu_setup) > @@ -490,11 +503,23 @@ undefined_instruction: > bad_save_user_regs > bl do_undefined_instruction > > +/* > + * software interrupt aka. secure monitor handler > + * This is executed on a "smc" instruction, we use a "smc #0" to switch > + * to non-secure state > + */ > .align 5 > software_interrupt: > - get_bad_stack_swi > - bad_save_user_regs > - bl do_software_interrupt > + mrc p15, 0, r1, c1, c1, 0 @ read SCR > + bic r1, r1, #0x07f > + orr r1, r1, #0x31 @ enable NS, AW, FW > + > + mrc p15, 0, r0, c12, c0, 0 @ save secure copy of VBAR > + mcr p15, 0, r1, c1, c1, 0 @ write SCR, switch to non-sec > + isb > + mcr p15, 0, r0, c12, c0, 0 @ write non-secure copy of VBAR > + > + movs pc, lr > > .align 5 > prefetch_abort: Amicalement,
On 23/05/13 11:52, Albert ARIBAUD wrote: > Hi Andre, > > On Mon, 6 May 2013 15:17:45 +0200, Andre Przywara > <andre.przywara@linaro.org> wrote: > >> A prerequisite for using virtualization is to be in HYP mode, which >> requires the CPU to be in non-secure state. >> Introduce a monitor handler routine which switches the CPU to >> non-secure state by setting the NS and associated bits. >> According to the ARM ARM this should not be done in SVC mode, so we > > ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is > a more precise reference, please provide it. I believe the ARM ARM (as in ARM Architecture Reference Manual) is the correct document here. http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406c/index.html Cheers, M.
Hi Marc, On Thu, 23 May 2013 13:14:01 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 23/05/13 11:52, Albert ARIBAUD wrote: > > Hi Andre, > > > > On Mon, 6 May 2013 15:17:45 +0200, Andre Przywara > > <andre.przywara@linaro.org> wrote: > > > >> A prerequisite for using virtualization is to be in HYP mode, which > >> requires the CPU to be in non-secure state. > >> Introduce a monitor handler routine which switches the CPU to > >> non-secure state by setting the NS and associated bits. > >> According to the ARM ARM this should not be done in SVC mode, so we > > > > ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is > > a more precise reference, please provide it. > > I believe the ARM ARM (as in ARM Architecture Reference Manual) is the > correct document here. > > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406c/index.html Well -- if you form the acronym, you do end with 'A R M' indeed, but this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's A already has ARM in it), confusion (as 'ARM' already bears a quite established meaning) and ambiguous (as there are actually several documents with the title of 'ARM<vXXX> Reference Manual' and which would end up with the same acronym). So if you don't want to use 'TRM' (which I can understand), then at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'. Stating the DDI* reference is not a must, unless you want to specify a given revision (but then I suggest adding it after 'Manual' too). > Cheers, > > M. Amicalement,
On Thu, 23 May 2013 14:34:38 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Marc, > So if you don't want to use 'TRM' (which I can understand), then > at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'. > Stating the DDI* reference is not a must, unless you want to specify > a given revision (but then I suggest adding it after 'Manual' too). My bad: this last paragraph was actually directed at Andrew, not Marc. Amicalement,
On Thu, 23 May 2013 14:40:09 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> My bad: this last paragraph was actually directed at Andrew
Make that Andre.
Apologies,
On 23 May 2013 13:34, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Well -- if you form the acronym, you do end with 'A R M' indeed, but > this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's > A already has ARM in it), confusion (as 'ARM' already bears a quite > established meaning) and ambiguous (as there are actually several > documents with the title of 'ARM<vXXX> Reference Manual' and which > would end up with the same acronym). "ARM ARM" is the standard abbreviated way of referring to the ARM Architecture Reference Manual (and as you can see it's not a redundant acronym, since the A doesn't stand for ARM). A TRM (Technical Reference Manual) is a completely different document type, describing a specific processor. Andre is correct that the restriction in question is architectural (and thus described in the ARM ARM), not implementation specific (which would be what you'd find in a TRM). > So if you don't want to use 'TRM' (which I can understand), then > at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'. > Stating the DDI* reference is not a must, unless you want to specify > a given revision (but then I suggest adding it after 'Manual' too). "ARMv7-AR Reference Manual" is confusing, because you've dropped the "Architecture" bit. Since this is only a git comment, I'd suggest "ARM architecture reference manual" as both clear for non-ARM people and sufficiently unambiguous, or "ARMv7-AR Architecture Reference Manual" if you want to be a bit more formal about it. thanks -- PMM
Hi Peter, (sorry for the duplicate; first reply sent was missing recipients, and I had a fix to do anyway) On Thu, 23 May 2013 14:00:17 +0100, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 May 2013 13:34, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > > Well -- if you form the acronym, you do end with 'A R M' indeed, but > > this is quite unfortunate, as 'ARM ARM' is redundant (as the acronym's > > A already has ARM in it), confusion (as 'ARM' already bears a quite > > established meaning) and ambiguous (as there are actually several > > documents with the title of 'ARM<vXXX> Reference Manual' and which > > would end up with the same acronym). > > "ARM ARM" is the standard abbreviated way of referring to the > ARM Architecture Reference Manual (and as you can see it's > not a redundant acronym, since the A doesn't stand for ARM). Before answering about the double 'ARM', I did google for possible meanings of the acronym 'ARM' and did not find any indication of it meaning 'Architecture Reference Manual' except on www.all-acronyms.com, which is completely foreign to the embedded world and where the definition is not backed by any substantial source. OTOH, the ARM Information Center does not use the acronym ARM to mean 'Architecture Reference Manual', nor does the Glossary section of ARM documents I've read so far - including "the" ARM Glossary (AEG0014E), which lists quite a lot of 'ARM something' but no 'ARM' alone, except in the preamble phrase: 'Where the term ARM is used it means “ARM or any of its subsidiaries as appropriate”'. Of course, I might have missed it, so any actual pointer to the definition is heartily welcome. > A TRM (Technical Reference Manual) is a completely different > document type, describing a specific processor. Andre is correct > that the restriction in question is architectural (and thus > described in the ARM ARM), not implementation specific (which > would be what you'd find in a TRM). You are correct that here the document is not a TRM. > > So if you don't want to use 'TRM' (which I can understand), then > > at least please replace 'ARM ARM' with 'ARMv7-AR Reference Manual'. > > Stating the DDI* reference is not a must, unless you want to specify > > a given revision (but then I suggest adding it after 'Manual' too). > > "ARMv7-AR Reference Manual" is confusing, because you've dropped > the "Architecture" bit. That drop was involuntary. > Since this is only a git comment, I'd suggest "ARM architecture > reference manual" as both clear for non-ARM people and sufficiently > unambiguous, or "ARMv7-AR Architecture Reference Manual" if you > want to be a bit more formal about it. Since a git comment is there for a reason, which includes helping its readers understand the commit, I consider "ARMv7-AR Reference Manual" to help them much more than 'ARM ARM', as it points them unambiguously to the document by stating the exact title under which it is listed in the ARM information center, but I am ok with 'ARMv7-AR Architecture Reference Manual' as this how its title goes. > thanks > -- PMM Amicalement,
On Thu, 23 May 2013 16:08:35 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Since a git comment is there for a reason, which includes helping its > readers understand the commit, I consider "ARMv7-AR Reference Manual" > to help them much more than 'ARM ARM', as it points them unambiguously > to the document by stating the exact title under which it is listed in > the ARM information center, but I am ok with 'ARMv7-AR Architecture > Reference Manual' as this how its title goes. Just to clarify: yes, the document is listed (on the left, in the docs tree as "ARMv7-AR Reference Manual", and yes, its title is 'ARMv7-AR Architecture Reference Manual'. That is not a mistake apparently, or it was a duly repeated one for all ARM architecture reference manuals on the Information Center. Amicalement,
On 05/23/2013 12:52 PM, Albert ARIBAUD wrote: > Hi Andre, > > On Mon, 6 May 2013 15:17:45 +0200, Andre Przywara > <andre.przywara@linaro.org> wrote: > >> A prerequisite for using virtualization is to be in HYP mode, which >> requires the CPU to be in non-secure state. >> Introduce a monitor handler routine which switches the CPU to >> non-secure state by setting the NS and associated bits. >> According to the ARM ARM this should not be done in SVC mode, so we > > ARM *TRM*, I suspect. Also, as there are a lot of ARM TRMs, if there is > a more precise reference, please provide it. Albert, my apologies for the confusion. As Peter already pointed out, the reference is really in the architectural manual. I just picked up that "ARM ARM" phrase lately and assumed that this is common knowledge. I will change it to something more precise in the next revision. Thanks, Andre.
On Mon, May 06, 2013 at 03:17:45PM +0200, Andre Przywara wrote: > A prerequisite for using virtualization is to be in HYP mode, which > requires the CPU to be in non-secure state. > Introduce a monitor handler routine which switches the CPU to > non-secure state by setting the NS and associated bits. > According to the ARM ARM this should not be done in SVC mode, so we > have to setup a SMC handler for this. We reuse the current vector > table for this and make sure that we only access the MVBAR register > if the CPU supports the security extension and only if we > configured the board to use it, since boards entering u-boot already > in non-secure mode would crash on accessing MVBAR otherwise. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S > index e9e57e6..da48b36 100644 > --- a/arch/arm/cpu/armv7/start.S > +++ b/arch/arm/cpu/armv7/start.S > @@ -155,6 +155,13 @@ reset: > /* Set vector address in CP15 VBAR register */ > ldr r0, =_start > mcr p15, 0, r0, c12, c0, 0 @Set VBAR > + > +#ifdef CONFIG_ARMV7_VIRT > + mrc p15, 0, r1, c0, c1, 1 @ check for security extension > + ands r1, r1, #0x30 > + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR Hmm, this smells a bit simplified to me. Support for ARMv7_VIRT should easy to integrate into u-boot even for platforms that do not boot U-boot directly into secure mode (OMAP5 GP platforms are such an example). In this case you cannot assume that you can write the secure monitor mvbar. > +#endif > + > #endif > > /* the mask ROM code should have PLL and others stable */ > @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup) > ldr r0, =_start > mcr p15, 0, r0, c12, c0, 0 @Set VBAR > > +#ifdef CONFIG_ARMV7_VIRT > + mrc p15, 0, r1, c0, c1, 1 @ check for security extension > + ands r1, r1, #0x30 > + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR > +#endif > + > bx lr > > ENDPROC(c_runtime_cpu_setup) > @@ -490,11 +503,23 @@ undefined_instruction: > bad_save_user_regs > bl do_undefined_instruction > > +/* > + * software interrupt aka. secure monitor handler > + * This is executed on a "smc" instruction, we use a "smc #0" to switch > + * to non-secure state > + */ > .align 5 > software_interrupt: > - get_bad_stack_swi > - bad_save_user_regs > - bl do_software_interrupt Why is the following block not conditional on CONFIG_ARMV7_VIRT? Again, it feels a bit funny to modify this generic mechanism to contain this code for boards that boot in NS mode but have a way to enter Hyp mode using an HVC or SMC instruction. > + mrc p15, 0, r1, c1, c1, 0 @ read SCR > + bic r1, r1, #0x07f > + orr r1, r1, #0x31 @ enable NS, AW, FW Are you sure you want to always route FIQ to non-secure here? Don't you need to set the HCE bit? The whole register resets to 0register resets to zero. > + > + mrc p15, 0, r0, c12, c0, 0 @ save secure copy of VBAR > + mcr p15, 0, r1, c1, c1, 0 @ write SCR, switch to non-sec Not quite a "swith to non-sec"; you're still in monitor mode. > + isb > + mcr p15, 0, r0, c12, c0, 0 @ write non-secure copy of VBAR I don't actually think that you are, I think you're writing the secure copy here. In that case, I'm also wondering if the isb is superflous, because we perform an exception return below, but we of course want to make damn sure that the write of the NS bit is set before the exception return, maybe some ARM guys have the right expertise here. > + > + movs pc, lr This movs is pretty drastic, because it changes from secure to non-secure world, and yes, you can tell by looking at the orr instruction above, but I would prefer a (potentially big fat) comment here as well. > > .align 5 > prefetch_abort: > -- > 1.7.12.1 >
On 05/31/2013 03:02 AM, Christoffer Dall wrote: Christoffer, thanks a lot for the thorough review. Comments inline. > On Mon, May 06, 2013 at 03:17:45PM +0200, Andre Przywara wrote: >> A prerequisite for using virtualization is to be in HYP mode, which >> requires the CPU to be in non-secure state. >> Introduce a monitor handler routine which switches the CPU to >> non-secure state by setting the NS and associated bits. >> According to the ARM ARM this should not be done in SVC mode, so we >> have to setup a SMC handler for this. We reuse the current vector >> table for this and make sure that we only access the MVBAR register >> if the CPU supports the security extension and only if we >> configured the board to use it, since boards entering u-boot already >> in non-secure mode would crash on accessing MVBAR otherwise. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++--- >> 1 file changed, 28 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S >> index e9e57e6..da48b36 100644 >> --- a/arch/arm/cpu/armv7/start.S >> +++ b/arch/arm/cpu/armv7/start.S >> @@ -155,6 +155,13 @@ reset: >> /* Set vector address in CP15 VBAR register */ >> ldr r0, =_start >> mcr p15, 0, r0, c12, c0, 0 @Set VBAR >> + >> +#ifdef CONFIG_ARMV7_VIRT >> + mrc p15, 0, r1, c0, c1, 1 @ check for security extension >> + ands r1, r1, #0x30 >> + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR > > Hmm, this smells a bit simplified to me. > > Support for ARMv7_VIRT should easy to integrate into u-boot even for > platforms that do not boot U-boot directly into secure mode (OMAP5 GP > platforms are such an example). In this case you cannot assume that you > can write the secure monitor mvbar. Right, Albert kind of hinted on this already. I already fixed this in my private tree, totally removing these MVBAR writes here and instead copying the values from VBAR later just before we do the smc. Will send out a fixed version. >> +#endif >> + >> #endif >> >> /* the mask ROM code should have PLL and others stable */ >> @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup) >> ldr r0, =_start >> mcr p15, 0, r0, c12, c0, 0 @Set VBAR >> >> +#ifdef CONFIG_ARMV7_VIRT >> + mrc p15, 0, r1, c0, c1, 1 @ check for security extension >> + ands r1, r1, #0x30 >> + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR >> +#endif >> + >> bx lr >> >> ENDPROC(c_runtime_cpu_setup) >> @@ -490,11 +503,23 @@ undefined_instruction: >> bad_save_user_regs >> bl do_undefined_instruction >> >> +/* >> + * software interrupt aka. secure monitor handler >> + * This is executed on a "smc" instruction, we use a "smc #0" to switch >> + * to non-secure state >> + */ >> .align 5 >> software_interrupt: >> - get_bad_stack_swi >> - bad_save_user_regs >> - bl do_software_interrupt > > Why is the following block not conditional on CONFIG_ARMV7_VIRT? > > Again, it feels a bit funny to modify this generic mechanism to contain > this code for boards that boot in NS mode but have a way to enter Hyp > mode using an HVC or SMC instruction. software_interrupt is currently a panic routine. So it is not actually used by u-boot, it's just there to dump some state and eventually call reset_cpu(). So I feel that since I am now the only user of software_interrupt I don't need any special precautions and consider this routine now actually part of the HYP mode switcher. But of course I can retain the original "functionality" if CONFIG_ARMV7_VIRT is not defined. >> + mrc p15, 0, r1, c1, c1, 0 @ read SCR >> + bic r1, r1, #0x07f >> + orr r1, r1, #0x31 @ enable NS, AW, FW > > Are you sure you want to always route FIQ to non-secure here? Since we actually don't install any secure software and just use the secure state to do the HYP switch I don't feel like we should restrict FIQ. Since we don't use it for our own purposes, no one would be able to use it then, right? So my idea was to allow as much as possible. > Don't you need to set the HCE bit? The whole register resets to > 0register resets to zero. This is added later in 5/6. For reviewing purposes I split the patches up to do the non-secure switch only first. Later I add the bits to actually go to HYP mode. >> + >> + mrc p15, 0, r0, c12, c0, 0 @ save secure copy of VBAR >> + mcr p15, 0, r1, c1, c1, 0 @ write SCR, switch to non-sec > > Not quite a "swith to non-sec"; you're still in monitor mode. Right, _non-secure_ monitor mode. If I got this thing correctly, then secure/non-secure is a state, not a mode. Switching from secure to non-secure can only be done in monitor mode. And the state totally depends on the NS bit in SCR, so by setting this we enter non-secure world immediately. >> + isb >> + mcr p15, 0, r0, c12, c0, 0 @ write non-secure copy of VBAR > > I don't actually think that you are, I think you're writing the secure > copy here. With the mcr above I set the NS bit, so I am non-secure from that point on (hence the isb). Writing the VBAR with the NS bit set should set the non-secure copy. Not doing this was a problem I chased down for some days, so I believe this is correct. > In that case, I'm also wondering if the isb is superflous, because we > perform an exception return below, but we of course want to make damn > sure that the write of the NS bit is set before the exception return, > maybe some ARM guys have the right expertise here. > >> + >> + movs pc, lr > > This movs is pretty drastic, because it changes from secure to > non-secure world, and yes, you can tell by looking at the orr > instruction above, but I would prefer a (potentially big fat) comment > here as well. OK. Regards, Andre. > >> >> .align 5 >> prefetch_abort: >> -- >> 1.7.12.1 >>
Hi Andre, On Fri, 31 May 2013 11:23:16 +0200, Andre Przywara <andre.przywara@linaro.org> wrote: > software_interrupt is currently a panic routine. So it is not actually > used by u-boot, it's just there to dump some state and eventually call > reset_cpu(). > So I feel that since I am now the only user of software_interrupt I > don't need any special precautions and consider this routine now > actually part of the HYP mode switcher. But of course I can retain the > original "functionality" if CONFIG_ARMV7_VIRT is not defined. You should, actually. Rule #1 goes (*) 'a new config option should have zero effect on the code when not defined'. > Regards, > Andre. (*) Rule #1 actually goes, 'there is an infinite number of "Rule #1"', but that's slightly beyond the point. Amicalement,
On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote: > On 05/31/2013 03:02 AM, Christoffer Dall wrote: > > Christoffer, > > thanks a lot for the thorough review. Comments inline. > > >On Mon, May 06, 2013 at 03:17:45PM +0200, Andre Przywara wrote: > >>A prerequisite for using virtualization is to be in HYP mode, which > >>requires the CPU to be in non-secure state. > >>Introduce a monitor handler routine which switches the CPU to > >>non-secure state by setting the NS and associated bits. > >>According to the ARM ARM this should not be done in SVC mode, so we > >>have to setup a SMC handler for this. We reuse the current vector > >>table for this and make sure that we only access the MVBAR register > >>if the CPU supports the security extension and only if we > >>configured the board to use it, since boards entering u-boot already > >>in non-secure mode would crash on accessing MVBAR otherwise. > >> > >>Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > >>--- > >> arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++--- > >> 1 file changed, 28 insertions(+), 3 deletions(-) > >> > >>diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S > >>index e9e57e6..da48b36 100644 > >>--- a/arch/arm/cpu/armv7/start.S > >>+++ b/arch/arm/cpu/armv7/start.S > >>@@ -155,6 +155,13 @@ reset: > >> /* Set vector address in CP15 VBAR register */ > >> ldr r0, =_start > >> mcr p15, 0, r0, c12, c0, 0 @Set VBAR > >>+ > >>+#ifdef CONFIG_ARMV7_VIRT > >>+ mrc p15, 0, r1, c0, c1, 1 @ check for security extension > >>+ ands r1, r1, #0x30 > >>+ mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR > > > >Hmm, this smells a bit simplified to me. > > > >Support for ARMv7_VIRT should easy to integrate into u-boot even for > >platforms that do not boot U-boot directly into secure mode (OMAP5 GP > >platforms are such an example). In this case you cannot assume that you > >can write the secure monitor mvbar. > > Right, Albert kind of hinted on this already. I already fixed this > in my private tree, totally removing these MVBAR writes here and > instead copying the values from VBAR later just before we do the > smc. > Will send out a fixed version. > > >>+#endif > >>+ > >> #endif > >> > >> /* the mask ROM code should have PLL and others stable */ > >>@@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup) > >> ldr r0, =_start > >> mcr p15, 0, r0, c12, c0, 0 @Set VBAR > >> > >>+#ifdef CONFIG_ARMV7_VIRT > >>+ mrc p15, 0, r1, c0, c1, 1 @ check for security extension > >>+ ands r1, r1, #0x30 > >>+ mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR > >>+#endif > >>+ > >> bx lr > >> > >> ENDPROC(c_runtime_cpu_setup) > >>@@ -490,11 +503,23 @@ undefined_instruction: > >> bad_save_user_regs > >> bl do_undefined_instruction > >> > >>+/* > >>+ * software interrupt aka. secure monitor handler > >>+ * This is executed on a "smc" instruction, we use a "smc #0" to switch > >>+ * to non-secure state > >>+ */ > >> .align 5 > >> software_interrupt: > >>- get_bad_stack_swi > >>- bad_save_user_regs > >>- bl do_software_interrupt > > > >Why is the following block not conditional on CONFIG_ARMV7_VIRT? > > > >Again, it feels a bit funny to modify this generic mechanism to contain > >this code for boards that boot in NS mode but have a way to enter Hyp > >mode using an HVC or SMC instruction. > > software_interrupt is currently a panic routine. So it is not > actually used by u-boot, it's just there to dump some state and > eventually call reset_cpu(). Which is pretty useful to catch if something went wrong. > So I feel that since I am now the only user of software_interrupt I > don't need any special precautions and consider this routine now > actually part of the HYP mode switcher. But of course I can retain > the original "functionality" if CONFIG_ARMV7_VIRT is not defined. > Yeah, I don't really think it's cool if a software interrupt happens somewhere completely else in the system that we do this dance, which could be super hard to detect, which is why I'm not even a fan of re-using the vector in the first place. > >>+ mrc p15, 0, r1, c1, c1, 0 @ read SCR > >>+ bic r1, r1, #0x07f > >>+ orr r1, r1, #0x31 @ enable NS, AW, FW > > > >Are you sure you want to always route FIQ to non-secure here? > > Since we actually don't install any secure software and just use the > secure state to do the HYP switch I don't feel like we should > restrict FIQ. Since we don't use it for our own purposes, no one > would be able to use it then, right? So my idea was to allow as much > as possible. Yeah, makes sense. > > >Don't you need to set the HCE bit? The whole register resets to > >0register resets to zero. > > This is added later in 5/6. For reviewing purposes I split the > patches up to do the non-secure switch only first. Later I add the > bits to actually go to HYP mode. The splitting of patches is fine, but it would be helpful to explain the scope a little more in the commit text perhaps, maybe I'm just being silly. > > >>+ > >>+ mrc p15, 0, r0, c12, c0, 0 @ save secure copy of VBAR > >>+ mcr p15, 0, r1, c1, c1, 0 @ write SCR, switch to non-sec > > > >Not quite a "swith to non-sec"; you're still in monitor mode. > > Right, _non-secure_ monitor mode. If I got this thing correctly, > then secure/non-secure is a state, not a mode. Switching from secure > to non-secure can only be done in monitor mode. And the state > totally depends on the NS bit in SCR, so by setting this we enter > non-secure world immediately. I think the ARM ARM is pretty clear on the fact that the NS bit in the SCR determine the secure state, *except* from monitor mode, which is always in secure state: "Software executing in Monitor mode executes in the Secure state regardless of the value of the SCR.NS bit." ARM ARM, DDI 0406C.b, B1-1157 > > >>+ isb > >>+ mcr p15, 0, r0, c12, c0, 0 @ write non-secure copy of VBAR > > > >I don't actually think that you are, I think you're writing the secure > >copy here. > > With the mcr above I set the NS bit, so I am non-secure from that > point on (hence the isb). Writing the VBAR with the NS bit set > should set the non-secure copy. Not doing this was a problem I > chased down for some days, so I believe this is correct. Hmmm, that seems to contradict the quote from above. Did you verify this emprically? If so, maybe there's a special caveat for writing banked co-processor registers (I doubt it though). > > >In that case, I'm also wondering if the isb is superflous, because we > >perform an exception return below, but we of course want to make damn > >sure that the write of the NS bit is set before the exception return, > >maybe some ARM guys have the right expertise here. thinking about it, it really shouldn't be necessary, because the movs implies all that's necessary. > > > >>+ > >>+ movs pc, lr > > > >This movs is pretty drastic, because it changes from secure to > >non-secure world, and yes, you can tell by looking at the orr > >instruction above, but I would prefer a (potentially big fat) comment > >here as well. > > OK. > > Regards, > Andre. > > > > >> > >> .align 5 > >> prefetch_abort: > >>-- > >>1.7.12.1 > >> >
Hi Christoffer, On Fri, 31 May 2013 16:50:09 -0700, Christoffer Dall <cdall@cs.columbia.edu> wrote: > On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote: > > On 05/31/2013 03:02 AM, Christoffer Dall wrote: > > This is added later in 5/6. For reviewing purposes I split the > > patches up to do the non-secure switch only first. Later I add the > > bits to actually go to HYP mode. > > The splitting of patches is fine, but it would be helpful to explain the > scope a little more in the commit text perhaps, maybe I'm just being > silly. That's the point where a cover letter will be very useful in V2. Also, speaking of V2 (and, presumably, later versions), that's where patman will be useful (see tools/patman/README) as it allows one's local branch to contain not only things like cover letter, but also patch history, which will be a must IMO for this series. Amicalement,
On Sat, 1 Jun 2013 12:06:24 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Christoffer, > > On Fri, 31 May 2013 16:50:09 -0700, Christoffer Dall > <cdall@cs.columbia.edu> wrote: > > > On Fri, May 31, 2013 at 11:23:16AM +0200, Andre Przywara wrote: > > > On 05/31/2013 03:02 AM, Christoffer Dall wrote: > > > > This is added later in 5/6. For reviewing purposes I split the > > > patches up to do the non-secure switch only first. Later I add the > > > bits to actually go to HYP mode. > > > > The splitting of patches is fine, but it would be helpful to explain the > > scope a little more in the commit text perhaps, maybe I'm just being > > silly. > > That's the point where a cover letter will be very useful in V2. Correction: V1 has a cover letter; what I meant and should have said is, that's where an overall description in the cover letter of how commits are organized will be very useful in V2. > Also, speaking of V2 (and, presumably, later versions), that's where > patman will be useful (see tools/patman/README) as it allows one's > local branch to contain not only things like cover letter, but also > patch history, which will be a must IMO for this series. Correction: maybe/probably Andre used patman already; in this case, scratch the paragraph above. Apologies for any misunderstanding,
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index e9e57e6..da48b36 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -155,6 +155,13 @@ reset: /* Set vector address in CP15 VBAR register */ ldr r0, =_start mcr p15, 0, r0, c12, c0, 0 @Set VBAR + +#ifdef CONFIG_ARMV7_VIRT + mrc p15, 0, r1, c0, c1, 1 @ check for security extension + ands r1, r1, #0x30 + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR +#endif + #endif /* the mask ROM code should have PLL and others stable */ @@ -257,6 +264,12 @@ ENTRY(c_runtime_cpu_setup) ldr r0, =_start mcr p15, 0, r0, c12, c0, 0 @Set VBAR +#ifdef CONFIG_ARMV7_VIRT + mrc p15, 0, r1, c0, c1, 1 @ check for security extension + ands r1, r1, #0x30 + mcrne p15, 0, r0, c12, c0, 1 @ Set secure monitor MVBAR +#endif + bx lr ENDPROC(c_runtime_cpu_setup) @@ -490,11 +503,23 @@ undefined_instruction: bad_save_user_regs bl do_undefined_instruction +/* + * software interrupt aka. secure monitor handler + * This is executed on a "smc" instruction, we use a "smc #0" to switch + * to non-secure state + */ .align 5 software_interrupt: - get_bad_stack_swi - bad_save_user_regs - bl do_software_interrupt + mrc p15, 0, r1, c1, c1, 0 @ read SCR + bic r1, r1, #0x07f + orr r1, r1, #0x31 @ enable NS, AW, FW + + mrc p15, 0, r0, c12, c0, 0 @ save secure copy of VBAR + mcr p15, 0, r1, c1, c1, 0 @ write SCR, switch to non-sec + isb + mcr p15, 0, r0, c12, c0, 0 @ write non-secure copy of VBAR + + movs pc, lr .align 5 prefetch_abort:
A prerequisite for using virtualization is to be in HYP mode, which requires the CPU to be in non-secure state. Introduce a monitor handler routine which switches the CPU to non-secure state by setting the NS and associated bits. According to the ARM ARM this should not be done in SVC mode, so we have to setup a SMC handler for this. We reuse the current vector table for this and make sure that we only access the MVBAR register if the CPU supports the security extension and only if we configured the board to use it, since boards entering u-boot already in non-secure mode would crash on accessing MVBAR otherwise. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- arch/arm/cpu/armv7/start.S | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)