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@xxxxxxxxxx> >> --- >> 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 >> _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
