On 06/28/2013 05:18 AM, Masahiro Yamada wrote:
> Hi Andre.
>
>
>> --- a/arch/arm/lib/Makefile
>> +++ b/arch/arm/lib/Makefile
>> @@ -60,6 +60,8 @@ COBJS-y += reset.o
>> COBJS-y += cache.o
>> COBJS-y += cache-cp15.o
>>
>> +COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
>> +
>
> Judging from the file name virt-v7.c,
> you are thinkig this file is specific to ARMv7, aren't you?
>
> If so, why don't you move this file
> to arch/arm/cpu/armv7/ ?
>
>
>> +static void set_generic_timer_frequency(void)
>> +{
>> +#ifdef CONFIG_SYS_CLK_FREQ
>> + unsigned int reg;
>> +
>> + reg = read_id_pfr1();
>> + if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
>> + asm("mcr p15, 0, %0, c14, c0, 0\n"
>> + : : "r"(CONFIG_SYS_CLK_FREQ));
>> +#endif
>
> CPUID_ARM_TIMER_MASK
> CPUID_ARM_TIMER_SHIFT
>
> I think these macro names are vague.
> There are Generic Timer, Global Timer, Private Timer etc.
Good point.
> Unlike other parts, the care for Cortex-A9 is missing here.
> To be more generic, I'd like to suggest to allow Non-secure access
> to Global/Private timers before switching to non-secure state.
>
>
> How about like this for armv7_switch_nonsec function?
>
>
> /* check whether the CPU supports the security extensions */
> reg = read_id_pfr1();
> if ((reg & 0xF0) == 0)
> return HYP_ERR_NO_SEC_EXT;
>
> if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
> set_generic_timer_frequency();
> else
> /* Allow Non-secure access to Global/Private timers */
> writel(0xfff, periph_base + SCU_SNSAC);
Interesting, however I don't have access to an A9 board currently to
properly test this. So I will do the renaming and let the access to the
other timers up to a follow-up patch.
Thanks,
Andre.
>
> For more info about SCU Non-secure Access Control (SNSAC) Register,
> please refer Cortex A9 mpcore TRM. page 2-11
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407g/DDI0407G_cortex_a9_mpcore_r3p0_trm.pdf
>
>
>
>> + /* enable the GIC distributor */
>> + writel(readl(&gicdptr[GICD_CTLR]) | 0x03, &gicdptr[GICD_CTLR]);
>
> I am not sure this code is really necessary.
>
> Because your are setting all available interrupts to Group1 just below,
> I think we don't need to do this in secure state.
>
>
> Best Regards
> Masahiro Yamada
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
