Re: [U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state
On 05/31/2013 05:04 AM, Christoffer Dall wrote: On Mon, May 06, 2013 at 03:17:46PM +0200, Andre Przywara wrote: While actually switching to non-secure state is one thing, the more important part of this process is to make sure that we still have full access to the interrupt controller (GIC). The GIC is fully aware of secure vs. non-secure state, some registers are banked, others may be configured to be accessible from secure state only. To be as generic as possible, we get the GIC memory mapped address based on the PERIPHBASE register. We check explicitly for ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those cores we know the offsets for the GIC CPU interface from the PERIPHBASE content. Other cores could be added as needed. With the GIC accessible, we: a) allow private interrupts to be delivered to the core (GICD_IGROUPR0 = 0x) b) enable the CPU interface (GICC_CTLR[0] = 1) c) set the priority filter to allow non-secure interrupts (GICC_PMR = 0x80) After having switched to non-secure state, we also enable the non-secure GIC CPU interface, since this register is banked. Also we allow access to all coprocessor interfaces from non-secure state by writing the appropriate bits in the NSACR register. For reasons obvious later we only use caller saved registers r0-r3. You probably want to put that in a comment in the code, and it would also be super helpful to explain the obvious part here, because most readers don't look forward in time to understand this patch... Agreed. Signed-off-by: Andre Przywara andre.przyw...@linaro.org --- arch/arm/cpu/armv7/start.S | 47 ++ 1 file changed, 47 insertions(+) diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index da48b36..e63e892 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -572,3 +572,50 @@ fiq: #endif /* CONFIG_USE_IRQ */ #endif /* CONFIG_SPL_BUILD */ + +#ifdef CONFIG_ARMV7_VIRT +/* Routine to initialize GIC CPU interface and switch to nonsecure state. + */ +.globl _nonsec_gic_switch +_nonsec_gic_switch: + mrc p15, 4, r2, c15, c0, 0 @ r2 = PERIPHBASE You should probably check if bits [7:0] == 0x00 here, otherwise you may end up doing some very strange stuff - if any of those bits are set you can just error out at this point, but it should be gracefully handled. Also since it's core specific, you'd probably want to check that before using it. As you found out later, I am doing this before calling this routine. But I will add a comment here to avoid confusion. + add r3, r2, #0x1000 @ GIC dist i/f offset Since this is core specific, could the offset please go in an appropriate header file? It will also eliminate the need for the comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...) + mvn r1, #0 + str r1, [r3, #0x80] @ allow private interrupts Aren't you makeing an assumption about the number of available interrupts here? You should read the ITLinesNumber field from the GICD_TYPER if I'm not mistaking. This is the per core private interrupts. All bits should be implemented. From the GIC spec, chapter 4.3.4: In a multiprocessor implementation, GICD_IGROUPR0 is banked for each connected processor. This register holds the group status bits for interrupts 0-31. I also think it would be much cleaner if you again used a define for the 0x80 offset. Also, don't you need to set some enable fields on the GICD_CTLR here to enable group 1 interrupts? Since this a non-banked per-system register, I do this later in the C part. + + mrc p15, 0, r0, c0, c0, 0 @ MIDR + bfc r0, #16, #8 @ mask out variant, arch + bfc r0, #0, #4 @ and revision + movwr1, #0xc070 + movtr1, #0x4100 + cmp r0, r1 @ check for Cortex-A7 + orr r1, #0xf0 wow, nice bit fiddling. It may be quite a bit easier to read if you simply had defines for the bitmasks and real values and just did a load and placed a literal section accordingly. The sequence is necessary since we are short on registers. I agree it is a bit obfuscated, will make it more readable. + cmpne r0, r1 @ check for Cortex-A15 + movne r1, #0x100 @ GIC CPU offset for A9 So I read the ARMV7_VIRT config flag as something you can only enable on a board that actually supports the virtulization extensions, which the A9 doesn't so this should probably error out instead (or do an endless loop, or ignore the case in the code, ...). Yeah, originally I had the idea to support a non-sec switch only on non-virt capable CPUs. My first version had a separate non-sec switch command. This here is kind of a leftover of that early version. But until patch 5/6 is applied this actually
Re: [U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state
On Fri, May 31, 2013 at 11:26:06AM +0200, Andre Przywara wrote: On 05/31/2013 05:04 AM, Christoffer Dall wrote: On Mon, May 06, 2013 at 03:17:46PM +0200, Andre Przywara wrote: While actually switching to non-secure state is one thing, the more important part of this process is to make sure that we still have full access to the interrupt controller (GIC). The GIC is fully aware of secure vs. non-secure state, some registers are banked, others may be configured to be accessible from secure state only. To be as generic as possible, we get the GIC memory mapped address based on the PERIPHBASE register. We check explicitly for ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those cores we know the offsets for the GIC CPU interface from the PERIPHBASE content. Other cores could be added as needed. With the GIC accessible, we: a) allow private interrupts to be delivered to the core (GICD_IGROUPR0 = 0x) b) enable the CPU interface (GICC_CTLR[0] = 1) c) set the priority filter to allow non-secure interrupts (GICC_PMR = 0x80) After having switched to non-secure state, we also enable the non-secure GIC CPU interface, since this register is banked. Also we allow access to all coprocessor interfaces from non-secure state by writing the appropriate bits in the NSACR register. For reasons obvious later we only use caller saved registers r0-r3. You probably want to put that in a comment in the code, and it would also be super helpful to explain the obvious part here, because most readers don't look forward in time to understand this patch... Agreed. Signed-off-by: Andre Przywara andre.przyw...@linaro.org --- arch/arm/cpu/armv7/start.S | 47 ++ 1 file changed, 47 insertions(+) diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index da48b36..e63e892 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -572,3 +572,50 @@ fiq: #endif /* CONFIG_USE_IRQ */ #endif /* CONFIG_SPL_BUILD */ + +#ifdef CONFIG_ARMV7_VIRT +/* Routine to initialize GIC CPU interface and switch to nonsecure state. + */ +.globl _nonsec_gic_switch +_nonsec_gic_switch: + mrc p15, 4, r2, c15, c0, 0 @ r2 = PERIPHBASE You should probably check if bits [7:0] == 0x00 here, otherwise you may end up doing some very strange stuff - if any of those bits are set you can just error out at this point, but it should be gracefully handled. Also since it's core specific, you'd probably want to check that before using it. As you found out later, I am doing this before calling this routine. But I will add a comment here to avoid confusion. yeah, or just expect that this address is in r0 upon calling the routine, then you're in the clear. + add r3, r2, #0x1000 @ GIC dist i/f offset Since this is core specific, could the offset please go in an appropriate header file? It will also eliminate the need for the comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...) + mvn r1, #0 + str r1, [r3, #0x80] @ allow private interrupts Aren't you makeing an assumption about the number of available interrupts here? You should read the ITLinesNumber field from the GICD_TYPER if I'm not mistaking. This is the per core private interrupts. All bits should be implemented. From the GIC spec, chapter 4.3.4: In a multiprocessor implementation, GICD_IGROUPR0 is banked for each connected processor. This register holds the group status bits for interrupts 0-31. I understand it, but the comments or naming of the routine never suggested that this was the code that was called per-core. I really think that is the core objective of this function: The NS-init that each core must do, it's not really GIC specific, so I suggest you rename it. I also think it would be much cleaner if you again used a define for the 0x80 offset. Also, don't you need to set some enable fields on the GICD_CTLR here to enable group 1 interrupts? Since this a non-banked per-system register, I do this later in the C part. later in the patch series, before in the flow of the code, right? :) + + mrc p15, 0, r0, c0, c0, 0 @ MIDR + bfc r0, #16, #8 @ mask out variant, arch + bfc r0, #0, #4 @ and revision + movwr1, #0xc070 + movtr1, #0x4100 + cmp r0, r1 @ check for Cortex-A7 + orr r1, #0xf0 wow, nice bit fiddling. It may be quite a bit easier to read if you simply had defines for the bitmasks and real values and just did a load and placed a literal section accordingly. The sequence is necessary since we are short on registers. I agree it is a bit obfuscated, will make it more readable. yeah but: #define ARM_CORTEX_A15_ID 0x4100c070 .ltorg [...] ldr r0,
Re: [U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state
On Mon, May 06, 2013 at 03:17:46PM +0200, Andre Przywara wrote: While actually switching to non-secure state is one thing, the more important part of this process is to make sure that we still have full access to the interrupt controller (GIC). The GIC is fully aware of secure vs. non-secure state, some registers are banked, others may be configured to be accessible from secure state only. To be as generic as possible, we get the GIC memory mapped address based on the PERIPHBASE register. We check explicitly for ARM Cortex-A7 and A15 cores, assuming an A9 otherwise, as for those cores we know the offsets for the GIC CPU interface from the PERIPHBASE content. Other cores could be added as needed. With the GIC accessible, we: a) allow private interrupts to be delivered to the core (GICD_IGROUPR0 = 0x) b) enable the CPU interface (GICC_CTLR[0] = 1) c) set the priority filter to allow non-secure interrupts (GICC_PMR = 0x80) After having switched to non-secure state, we also enable the non-secure GIC CPU interface, since this register is banked. Also we allow access to all coprocessor interfaces from non-secure state by writing the appropriate bits in the NSACR register. For reasons obvious later we only use caller saved registers r0-r3. You probably want to put that in a comment in the code, and it would also be super helpful to explain the obvious part here, because most readers don't look forward in time to understand this patch... Signed-off-by: Andre Przywara andre.przyw...@linaro.org --- arch/arm/cpu/armv7/start.S | 47 ++ 1 file changed, 47 insertions(+) diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index da48b36..e63e892 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -572,3 +572,50 @@ fiq: #endif /* CONFIG_USE_IRQ */ #endif /* CONFIG_SPL_BUILD */ + +#ifdef CONFIG_ARMV7_VIRT +/* Routine to initialize GIC CPU interface and switch to nonsecure state. + */ +.globl _nonsec_gic_switch +_nonsec_gic_switch: + mrc p15, 4, r2, c15, c0, 0 @ r2 = PERIPHBASE You should probably check if bits [7:0] == 0x00 here, otherwise you may end up doing some very strange stuff - if any of those bits are set you can just error out at this point, but it should be gracefully handled. Also since it's core specific, you'd probably want to check that before using it. + add r3, r2, #0x1000 @ GIC dist i/f offset Since this is core specific, could the offset please go in an appropriate header file? It will also eliminate the need for the comment if you just have a proper define (i.e. GIC_DIST_OFFSET ...) + mvn r1, #0 + str r1, [r3, #0x80] @ allow private interrupts Aren't you makeing an assumption about the number of available interrupts here? You should read the ITLinesNumber field from the GICD_TYPER if I'm not mistaking. I also think it would be much cleaner if you again used a define for the 0x80 offset. Also, don't you need to set some enable fields on the GICD_CTLR here to enable group 1 interrupts? + + mrc p15, 0, r0, c0, c0, 0 @ MIDR + bfc r0, #16, #8 @ mask out variant, arch + bfc r0, #0, #4 @ and revision + movwr1, #0xc070 + movtr1, #0x4100 + cmp r0, r1 @ check for Cortex-A7 + orr r1, #0xf0 wow, nice bit fiddling. It may be quite a bit easier to read if you simply had defines for the bitmasks and real values and just did a load and placed a literal section accordingly. + cmpne r0, r1 @ check for Cortex-A15 + movne r1, #0x100 @ GIC CPU offset for A9 So I read the ARMV7_VIRT config flag as something you can only enable on a board that actually supports the virtulization extensions, which the A9 doesn't so this should probably error out instead (or do an endless loop, or ignore the case in the code, ...). + moveq r1, #0x2000 @ GIC CPU offset for A15/A7 Again, I think defines for these are appropriate. + add r3, r2, r1 @ r3 = GIC CPU i/f addr + + mov r1, #1 + str r1, [r3, #0]@ set GICC_CTLR[enable] + mov r1, #0x80 Why are you not setting this to #0xff ? + str r1, [r3, #4]@ set GICC_PIMR[7] + here it seems we're moving into non-gic related stuff in a function called _nonsec_gic_switch + movwr1, #0x3fff + movtr1, #0x0006 + mcr p15, 0, r1, c1, c1, 2 @ NSACR = all copros to non-sec + + ldr r1, =_start + mcr p15, 0, r1, c12, c0, 0 @ set VBAR + mcr p15, 0, r1, c12, c0, 1 @ set MVBAR I thought you already took care of the MVBAR during init? + + isb +
