Re: [U-Boot] [PATCH v3 4/7] ARM: switch to non-secure state during bootm execution
On Tue, Jul 30, 2013 at 01:32:14PM +0200, Andre Przywara wrote:
> On 07/30/2013 12:02 AM, Christoffer Dall wrote:
> >On Wed, Jul 10, 2013 at 01:54:16AM +0200, Andre Przywara wrote:
> >
> >[...]
> >
> >>diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> >>index 1b6e0ac..7b0619e 100644
> >>--- a/arch/arm/lib/bootm.c
> >>+++ b/arch/arm/lib/bootm.c
> >>@@ -34,6 +34,10 @@
> >> #include
> >> #include
> >>
> >>+#ifdef CONFIG_ARMV7_NONSEC
> >>+#include
> >>+#endif
> >>+
> >> DECLARE_GLOBAL_DATA_PTR;
> >>
> >> static struct tag *params;
> >>@@ -186,6 +190,29 @@ static void setup_end_tag(bd_t *bd)
> >>
> >> __weak void setup_board_tags(struct tag **in_params) {}
> >>
> >>+static void do_nonsec_virt_switch(void)
> >>+{
> >>+#ifdef CONFIG_ARMV7_NONSEC
> >>+ int ret;
> >>+
> >>+ ret = armv7_switch_nonsec();
> >>+ switch (ret) {
> >>+ case NONSEC_VIRT_SUCCESS:
> >>+ debug("entered non-secure state\n");
> >>+ break;
> >>+ case NONSEC_ERR_NO_SEC_EXT:
> >>+ printf("nonsec: Security extensions not implemented.\n");
> >>+ break;
> >>+ case NONSEC_ERR_NO_GIC_ADDRESS:
> >>+ printf("nonsec: could not determine GIC address.\n");
> >>+ break;
> >>+ case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
> >>+ printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
> >>+ break;
> >>+ }
> >>+#endif
> >>+}
> >
> >I still don't get why you just don't make armv7_switch_nonsec a void and
> >print the error when they occur... ???
>
> My apologies for not elaborating on these comments I didn't incorporate:
>
> So, I don't like the idea of marrying a low-level routine with high
> level output. I don't want to constraint the usage of the routine by
> requiring an output channel. Also some parts may not be fatal for
> all users - someone could just try to switch and then behave
> differently if that failed - without bothering the user.
> May seem a bit over-engineered, but I like it better this way ;-)
>
> If that is a show-stopper for you, I can change it, of course.
>
I won't hold back my ack for the patch series based on this, but I do
think it's over-engineered. I think at least just returning -1 for
error and 0 for success (or even make it a bool) and just printing a
generic error message is cleaner - the level of details as to why the
switch to hyp/nonsec didn't work could then be debug statements that a
board developer could enable with a "#define DEBUG 1" in the
corresponding file.
But ok, we've had the conversation, if you still feel this is better and
necessary, then I'll let it be.
-Christoffer
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 4/7] ARM: switch to non-secure state during bootm execution
On 07/30/2013 12:02 AM, Christoffer Dall wrote:
On Wed, Jul 10, 2013 at 01:54:16AM +0200, Andre Przywara wrote:
[...]
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 1b6e0ac..7b0619e 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -34,6 +34,10 @@
#include
#include
+#ifdef CONFIG_ARMV7_NONSEC
+#include
+#endif
+
DECLARE_GLOBAL_DATA_PTR;
static struct tag *params;
@@ -186,6 +190,29 @@ static void setup_end_tag(bd_t *bd)
__weak void setup_board_tags(struct tag **in_params) {}
+static void do_nonsec_virt_switch(void)
+{
+#ifdef CONFIG_ARMV7_NONSEC
+ int ret;
+
+ ret = armv7_switch_nonsec();
+ switch (ret) {
+ case NONSEC_VIRT_SUCCESS:
+ debug("entered non-secure state\n");
+ break;
+ case NONSEC_ERR_NO_SEC_EXT:
+ printf("nonsec: Security extensions not implemented.\n");
+ break;
+ case NONSEC_ERR_NO_GIC_ADDRESS:
+ printf("nonsec: could not determine GIC address.\n");
+ break;
+ case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
+ printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
+ break;
+ }
+#endif
+}
I still don't get why you just don't make armv7_switch_nonsec a void and
print the error when they occur... ???
My apologies for not elaborating on these comments I didn't incorporate:
So, I don't like the idea of marrying a low-level routine with high
level output. I don't want to constraint the usage of the routine by
requiring an output channel. Also some parts may not be fatal for all
users - someone could just try to switch and then behave differently if
that failed - without bothering the user.
May seem a bit over-engineered, but I like it better this way ;-)
If that is a show-stopper for you, I can change it, of course.
Regards,
Andre.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 4/7] ARM: switch to non-secure state during bootm execution
On Wed, Jul 10, 2013 at 01:54:16AM +0200, Andre Przywara wrote:
[...]
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 1b6e0ac..7b0619e 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -34,6 +34,10 @@
> #include
> #include
>
> +#ifdef CONFIG_ARMV7_NONSEC
> +#include
> +#endif
> +
> DECLARE_GLOBAL_DATA_PTR;
>
> static struct tag *params;
> @@ -186,6 +190,29 @@ static void setup_end_tag(bd_t *bd)
>
> __weak void setup_board_tags(struct tag **in_params) {}
>
> +static void do_nonsec_virt_switch(void)
> +{
> +#ifdef CONFIG_ARMV7_NONSEC
> + int ret;
> +
> + ret = armv7_switch_nonsec();
> + switch (ret) {
> + case NONSEC_VIRT_SUCCESS:
> + debug("entered non-secure state\n");
> + break;
> + case NONSEC_ERR_NO_SEC_EXT:
> + printf("nonsec: Security extensions not implemented.\n");
> + break;
> + case NONSEC_ERR_NO_GIC_ADDRESS:
> + printf("nonsec: could not determine GIC address.\n");
> + break;
> + case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
> + printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
> + break;
> + }
> +#endif
> +}
I still don't get why you just don't make armv7_switch_nonsec a void and
print the error when they occur... ???
-Christoffer
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 4/7] ARM: switch to non-secure state during bootm execution
Hello Andre,
On Wed, Jul 10, 2013 at 2:54 AM, 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/cpu/armv7 to allow easy access from
> other ARMv7 boards.
>
> We check the availability of the security extensions first.
>
> 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/cpu/armv7/Makefile | 1 +
> arch/arm/cpu/armv7/virt-v7.c | 117
> +++
> arch/arm/include/asm/armv7.h | 10
> arch/arm/lib/bootm.c | 28 +++
> 4 files changed, 156 insertions(+)
> create mode 100644 arch/arm/cpu/armv7/virt-v7.c
>
> diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
> index 5d75077..b59f59e 100644
> --- a/arch/arm/cpu/armv7/Makefile
> +++ b/arch/arm/cpu/armv7/Makefile
> @@ -38,6 +38,7 @@ endif
>
> ifneq ($(CONFIG_ARMV7_NONSEC),)
> SOBJS += nonsec_virt.o
> +COBJS += virt-v7.o
> endif
>
> SRCS := $(START:.o=.S) $(COBJS:.o=.c)
> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
> new file mode 100644
> index 000..54f9746
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/virt-v7.c
> @@ -0,0 +1,117 @@
> +/*
> + * (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 list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +static unsigned int read_id_pfr1(void)
> +{
> + unsigned int reg;
> +
> + asm("mrc p15, 0, %0, c0, c1, 1\n" : "=r"(reg));
> + return reg;
> +}
> +
> +static int get_gicd_base_address(unsigned int *gicdaddr)
> +{
> +#ifdef CONFIG_ARM_GIC_BASE_ADDRESS
> + *gicdaddr = CONFIG_ARM_GIC_BASE_ADDRESS + GIC_DIST_OFFSET;
> + return 0;
> +#else
> + unsigned midr;
> + unsigned periphbase;
> +
> + /* check whether we are an Cortex-A15 or A7.
> +* The actual HYP switch should work with all CPUs supporting
> +* the virtualization extension, but we need the GIC address,
> +* which we know only for sure for those two CPUs.
> +*/
> + asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(midr));
> + switch (midr & MIDR_PRIMARY_PART_MASK) {
> + case MIDR_CORTEX_A9_R0P1:
> + case MIDR_CORTEX_A15_R0P0:
> + case MIDR_CORTEX_A7_R0P0:
> + break;
> + default:
> + return NONSEC_ERR_NO_GIC_ADDRESS;
> + }
> +
> + /* get the GIC base address from the CBAR register */
> + asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (periphbase));
> +
> + /* the PERIPHBASE can be mapped above 4 GB (lower 8 bits used to
> +* encode this). Bail out here since we cannot access this without
> +* enabling paging.
> +*/
> + if ((periphbase & 0xff) != 0)
> + return NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB;
> +
> + *gicdaddr = periphbase + GIC_DIST_OFFSET;
>
The same as in _nonsec_init
periphbase &= PERIPHBASE_MASK; // 0x8000
> +
> + return 0;
> +#endif
