[U-Boot] [PATCH 2/6] ARM: add assembly routine to switch to non-secure state

[Andre Przywara]

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 = 0xFFFFFFFF)
>> 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.przywara at 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
>> +	movw	r1, #0xc070
>> +	movt	r1, #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 works (and we had a use-case 
internally here), so I decided to leave this in.

>> +	moveq	r1, #0x2000			@ GIC CPU offset for A15/A7
>
> Again, I think defines for these are appropriate.

Good point. Will fix this.

>> +	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 ?

Because a certain Christoffer Dall did this the same way in his Arndale 
specific patch ;-)
+       /* Set GIC priority mask bit [7] = 1 */
+       addr = EXYNOS5_GIC_CPU_BASE;
+       writel(0x80, addr + ARM_GICV2_ICCPMR);
(and I remember having read this recommendation somewhere)

>> +	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

Right, but this is per CPU and needs to be done in secure monitor mode, 
so it belongs here. Shall I rename the function to be called 
non_secure_init or the like?

>
>> +	movw	r1, #0x3fff
>> +	movt	r1, #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?

Right, as mentioned earlier I have fixed this already.

>> +
>> +	isb
>> +	smc	#0				@ call into MONITOR mode
>> +	isb					@ clobbers r0 and r1
>
> This isb shouldn't be necessary if you just did an exception return?

Seems to be a paranoid leftover of one debug session...

>> +
>> +	mov	r1, #1
>> +	str	r1, [r3, #0]			@ set GICC_CTLR[enable]
>
> you're doing more than setting the enable bit: you're setting the EOImodeNS,
> IRQBypDisGrp1, and FIQBypDisGrp1, as you should.

But 0x01 is the correct value? And the reset value is 0, right?
I will extend the comment.

>> +	add	r2, r2, #0x1000			@ GIC dist i/f offset
>> +	str	r1, [r2]			@ allow private interrupts
>
> It seems a bit brittle to rely on the smc handler to not clobber r2 and
> r3, and it may an annoying thing to track down.  You could just
> re-generate the the gic base address from the cp15 register.  Your
> choice.

So shall I put comments here and at the smc handler?

Thanks again for the comments!
Andre.

>> +
>> +	mov	pc, lr
>> +#endif /* CONFIG_ARMV7_VIRT */
>> --
>> 1.7.12.1
>>