Re: [U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

2013-07-04 Thread Andre Przywara

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



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

2013-06-27 Thread Masahiro Yamada
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.


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);



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

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

2013-06-19 Thread Christoffer Dall
On Thu, Jun 13, 2013 at 01:01:10PM +0200, Andre Przywara wrote:
> To actually trigger the non-secure switch we just implemented, call
> the switching routine from within the bootm command implementation.
> This way we automatically enable this feature without further user
> intervention.
> 
> The core specific part of the work is done in the assembly routine
> in nonsec_virt.S, introduced with the previous patch, but for the full
> glory we need to setup the GIC distributor interface once for the
> whole system, which is done in C here.
> The routine is placed in arch/arm/lib to allow easy access from
> different boards or CPUs.
> 
> We check the availability of the security extensions first.
> 
> The generic timer base frequency register is only accessible from
> secure state, so we have to program it now. Actually this should be
> done from primary firmware before, but some boards seems to omit
> this, so if needed we do this here with a board specific value.
> The Versatile Express board does not need this, so we remove the
> frequency from the configuration file here.
> 
> Since we need a safe way to access the GIC, we use the PERIPHBASE
> registers on Cortex-A15 and A7 CPUs and do some sanity checks.
> Board not implementing the CBAR can override this value via a
> configuration file variable.
> 
> Then we actually do the GIC enablement:
> a) enable the GIC distributor, both for non-secure and secure state
>(GICD_CTLR[1:0] = 11b)
> b) allow all interrupts to be handled from non-secure state
>(GICD_IGROUPRn = 0x)
> 
> The core specific GIC setup is then done in the assembly routine.
> 
> The actual bootm trigger is pretty small: calling the routine and
> doing some error reporting.
> 
> Signed-off-by: Andre Przywara 
> ---
>  arch/arm/include/asm/armv7.h|   7 ++
>  arch/arm/lib/Makefile   |   2 +
>  arch/arm/lib/bootm.c|  20 ++
>  arch/arm/lib/virt-v7.c  | 137 
> 
>  include/configs/vexpress_ca15_tc2.h |   2 -
>  5 files changed, 166 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/lib/virt-v7.c
> 
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 989bb72..56d0dd0 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -88,6 +88,13 @@ void v7_outer_cache_flush_range(u32 start, u32 end);
>  void v7_outer_cache_inval_range(u32 start, u32 end);
>  
>  #ifdef CONFIG_ARMV7_VIRT
> +
> +#define HYP_ERR_NO_SEC_EXT   2
> +#define HYP_ERR_NO_GIC_ADDRESS   3
> +#define HYP_ERR_GIC_ADDRESS_ABOVE_4GB4

enum?

> +
> +int armv7_switch_nonsec(void);
> +
>  /* defined in cpu/armv7/nonsec_virt.S */
>  void _nonsec_init(void);
>  #endif /* CONFIG_ARMV7_VIRT */
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 8ad9f66..1570ad5 100644
> --- 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
> +
>  SRCS := $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
>  $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
>  OBJS := $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 1b6e0ac..8251a89 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -34,6 +34,10 @@
>  #include 
>  #include 
>  
> +#ifdef CONFIG_ARMV7_VIRT
> +#include 
> +#endif
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  static struct tag *params;
> @@ -222,6 +226,22 @@ static void boot_prep_linux(bootm_headers_t *images)
>   printf("FDT and ATAGS support not compiled in - hanging\n");
>   hang();
>   }
> +#ifdef CONFIG_ARMV7_VIRT
> + switch (armv7_switch_nonsec()) {
> + case 0:
> + debug("entered non-secure state\n");
> + break;

this is weird, why not have a define for the success case?

I still think the debug printing should be done inside
armv7_switch_nonsec instead, and you can just have it be a void();

> + case HYP_ERR_NO_SEC_EXT:
> + printf("HYP mode: Security extensions not implemented.\n");
> + break;
> + case HYP_ERR_NO_GIC_ADDRESS:
> + printf("HYP mode: could not determine GIC address.\n");
> + break;
> + case HYP_ERR_GIC_ADDRESS_ABOVE_4GB:
> + printf("HYP mode: PERIPHBASE is above 4 GB, cannot access 
> this.\n");
> + break;
> + }
> +#endif
>  }
>  
>  /* Subcommand: GO */
> diff --git a/arch/arm/lib/virt-v7.c b/arch/arm/lib/virt-v7.c
> new file mode 100644
> index 000..7876a77
> --- /dev/null
> +++ b/arch/arm/lib/virt-v7.c
> @@ -0,0 +1,137 @@
> +/*
> + * (C) Copyright 2013
> + * Andre Przywara, Linaro
> + *
> + * Routines to transition ARMv7 processors from secure into non-secure state
> + * needed to enable ARMv7 virtualization for current hypervisors
> + *
> + * See file CREDITS for lis