On 07/30/2013 12:02 AM, Christoffer Dall wrote:
> On Wed, Jul 10, 2013 at 01:54:17AM +0200, Andre Przywara wrote:
>> Currently the non-secure switch is only done for the boot processor.
>> To enable full SMP support, we have to switch all secondary cores
>> into non-secure state also.
>>
>> So we add an entry point for secondary CPUs coming out of low-power
>> state and make sure we put them into WFI again after having switched
>> to non-secure state.
>> For this we acknowledge and EOI the wake-up IPI, then go into WFI.
>> Once being kicked out of it later, we sanity check that the start
>> address has actually been changed (since another attempt to switch
>> to non-secure would block the core) and jump to the new address.
>>
>> The actual CPU kick is done by sending an inter-processor interrupt
>> via the GIC to all CPU interfaces except the requesting processor.
>> The secondary cores will then setup their respective GIC CPU
>> interface.
>>
>> The address secondary cores jump to is board specific, we provide
>> the value here for the Versatile Express board.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>> arch/arm/cpu/armv7/nonsec_virt.S | 27 +++++++++++++++++++++++++++
>> arch/arm/cpu/armv7/virt-v7.c | 19 ++++++++++++++++++-
>> arch/arm/include/asm/armv7.h | 1 +
>> arch/arm/include/asm/gic.h | 2 ++
>> include/configs/vexpress_ca15_tc2.h | 3 +++
>> 5 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/cpu/armv7/nonsec_virt.S b/arch/arm/cpu/armv7/nonsec_virt.S
>> index e9ee831..f9b6b39 100644
>> --- a/arch/arm/cpu/armv7/nonsec_virt.S
>> +++ b/arch/arm/cpu/armv7/nonsec_virt.S
>> @@ -58,6 +58,33 @@ _secure_monitor:
>> movs pc, lr @ return to non-secure SVC
>>
>> /*
>> + * Secondary CPUs start here and call the code for the core specific parts
>> + * of the non-secure and HYP mode transition. The GIC distributor specific
>> + * code has already been executed by a C function before.
>> + * Then they go back to wfi and wait to be woken up by the kernel again.
>> + */
>> +ENTRY(_smp_pen)
>> + mrs r0, cpsr
>> + orr r0, r0, #0xc0
>> + msr cpsr, r0 @ disable interrupts
>> + ldr r1, =_start
>> + mcr p15, 0, r1, c12, c0, 0 @ set VBAR
>> +
>> + bl _nonsec_init
>> +
>> + ldr r1, [r0, #GICC_IAR] @ acknowledge IPI
>> + str r1, [r0, #GICC_EOIR] @ signal end of interrupt
>> + adr r1, _smp_pen
>> +waitloop:
>> + wfi
>> + ldr r0, =CONFIG_SYSFLAGS_ADDR @ load start address
>
> You seem to have ignored my comment about using the sysflags name?
>
> As I understand, the sysflags name is a versatile express specific
> register name that just happens to be used for the SMP boot address as
> well...
>
> Therefore, this should really be CONFIG_SMP_BOOT_ADDR or something like
> that, at the very least.
>
>> + ldr r0, [r0]
>> + cmp r0, r1 @ make sure we dont execute this code
>> + beq waitloop @ again (due to a spurious wakeup)
>> + mov pc, r0
>> +ENDPROC(_smp_pen)
>> +
>> +/*
>> * Switch a core to non-secure state.
>> *
>> * 1. initialize the GIC per-core interface
>> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
>> index 54f9746..a0d0b34 100644
>> --- a/arch/arm/cpu/armv7/virt-v7.c
>> +++ b/arch/arm/cpu/armv7/virt-v7.c
>> @@ -77,6 +77,21 @@ static int get_gicd_base_address(unsigned int *gicdaddr)
>> #endif
>> }
>>
>> +static void kick_secondary_cpus(unsigned int gicdaddr)
>> +{
>> + unsigned int *secondary_boot_addr;
>> +
>> + secondary_boot_addr = (void *)CONFIG_SYSFLAGS_ADDR;
>> +#ifdef CONFIG_SYSFLAGS_NEED_CLEAR_BITS
>> + secondary_boot_addr[1] = (unsigned)-1;
>> +#endif
>
> again, if you disagreed with my previous comment, please comment on it
> and rationalize your choice.
I am sorry, I thought I reasoned that already in an earlier mail:
I don't want to introduce abstraction prematurely, so I wanted to
refactor the code as soon as a second board gets supported. Which will
probably be the Arndale, and also probably done by me ...
> I still feel you're wrapping board specific logic into generic code, and
> that you should call out to a more generic function. Imagine an SOC
> that uses an implementation defined control register for this instead of
> a memory address...
Well, I can imagine quite some ways to do this, but in fact I'd like to
focus on things that really exist ;-)
My fear is that any abstraction I introduce now will not suffice for a
future board and needs to be refactored then anyway.
Regards,
Andre.
>
> perhaps what you need is:
>
> void set_board_smp_boot_addr(unsigned long addr);
> unsigned long get_board_smp_boot_addr(void);
>
> and call these instead of your direct use of sysflags addr here...?
>
>> + *secondary_boot_addr = (uintptr_t)_smp_pen;
>> + dmb();
>> +
>> + /* now kick all CPUs (except this one) by writing to GICD_SGIR */
>> + writel(1U << 24, gicdaddr + GICD_SGIR);
>> +}
>> +
>> enum nonsec_virt_errors armv7_switch_nonsec(void)
>> {
>> unsigned int reg, ret;
>> @@ -110,7 +125,9 @@ enum nonsec_virt_errors armv7_switch_nonsec(void)
>> for (i = 0; i <= itlinesnr; i++)
>> writel((unsigned)-1, gicdaddr + GICD_IGROUPRn + 4 * i);
>>
>> - /* call the non-sec switching code on this CPU */
>> + kick_secondary_cpus(gicdaddr);
>> +
>> + /* call the non-sec switching code on this CPU also */
>> _nonsec_init();
>>
>> return NONSEC_VIRT_SUCCESS;
>> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
>> index e5c0279..f6582a1 100644
>> --- a/arch/arm/include/asm/armv7.h
>> +++ b/arch/arm/include/asm/armv7.h
>> @@ -102,6 +102,7 @@ enum nonsec_virt_errors armv7_switch_nonsec(void);
>>
>> /* defined in assembly file */
>> unsigned int _nonsec_init(void);
>> +void _smp_pen(void);
>> #endif /* CONFIG_ARMV7_NONSEC */
>>
>> #endif /* ! __ASSEMBLY__ */
>> diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
>> index c2b1e28..a0891cc 100644
>> --- a/arch/arm/include/asm/gic.h
>> +++ b/arch/arm/include/asm/gic.h
>> @@ -13,5 +13,7 @@
>> #define GIC_CPU_OFFSET_A15 0x2000
>> #define GICC_CTLR 0x0000
>> #define GICC_PMR 0x0004
>> +#define GICC_IAR 0x000C
>> +#define GICC_EOIR 0x0010
>>
>> #endif
>> diff --git a/include/configs/vexpress_ca15_tc2.h b/include/configs/vexpress_ca15_tc2.h
>> index 4f425ac..ade9e5b 100644
>> --- a/include/configs/vexpress_ca15_tc2.h
>> +++ b/include/configs/vexpress_ca15_tc2.h
>> @@ -31,4 +31,7 @@
>> #include "vexpress_common.h"
>> #define CONFIG_BOOTP_VCI_STRING "U-boot.armv7.vexpress_ca15x2_tc2"
>>
>> +#define CONFIG_SYSFLAGS_ADDR 0x1c010030
>> +#define CONFIG_SYSFLAGS_NEED_CLEAR_BITS
>> +
>> #endif
>> --
>> 1.7.12.1
>>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
