[U-Boot] [PATCH] ARM: HYP/non-sec: Add MIDR check to detect unsupported CPUs

[U-Boot] [PATCH] ARM: HYP/non-sec: Add MIDR check to detect unsupported CPUs

[Siarhei Siamashka]

Unlike 9d195a546179bc732aba9eacccf0a9a3db591288, which had removed
the MIDR check against the "white list" of supported CPUs earlier,
now we introduce the "black list" of unsupported CPUs.

The current PSCI code is not compatible with the Cortex-A8 CPU used
in Allwinner A10/A13 SoCs because of making GIC and virtualization
support assumptions. Allwinner A10 and Allwinner A20 (with Cortex-A7
CPU, supported by PSCI) are pin compatible, they can be used as
drop-in replacements for each other and share the same PCB design:
   https://www.olimex.com/Products/OLinuXino/A10/A10-OLinuXino-LIME
   https://www.olimex.com/Products/OLinuXino/A20/A20-OLinuXino-LIME
The same u-boot binary binary can boot on Allwinner A10 and
Allwinner A20 with just minor tweaks applied.

This patch implements one of such necessary tweaks to allow the
PSCI code to be compiled in, but skip it if Cortex-A8 is detected
at runtime. The function 'armv7_init_nonsec()' now returns an error
in the case if Cortex-A8 is detected. Also an extra error check
is added for the 'armv7_init_nonsec()' call. If the board config
header file provides CONFIG_ARMV7_ALLOW_SECURE_MODE_FALLBACK define,
then the kernel is loaded in secure mode as a fallback. In the
case is this define is not provided, the failure to switch to
non-secure mode is treated as a fatal error and an appropriate
message is displayed.

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
---
 arch/arm/cpu/armv7/virt-dt.c |  3 +++
 arch/arm/cpu/armv7/virt-v7.c | 20 ++++++++++++++++++++
 arch/arm/include/asm/armv7.h |  5 +++++
 arch/arm/lib/bootm.c         | 16 +++++++++++++---
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
index 0b0d6a7..ab1fae9 100644
--- a/arch/arm/cpu/armv7/virt-dt.c
+++ b/arch/arm/cpu/armv7/virt-dt.c
@@ -31,6 +31,9 @@ static int fdt_psci(void *fdt)
 	int nodeoff;
 	int tmp;
 
+	if (armv7_is_cpu_blacklisted_for_nonsec())
+		return 0;
+
 	nodeoff = fdt_path_offset(fdt, "/cpus");
 	if (nodeoff < 0) {
 		printf("couldn't find /cpus\n");
diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
index 651ca40..4b3bc54 100644
--- a/arch/arm/cpu/armv7/virt-v7.c
+++ b/arch/arm/cpu/armv7/virt-v7.c
@@ -71,11 +71,31 @@ void __weak smp_kick_all_cpus(void)
 	kick_secondary_cpus_gic(gic_dist_addr);
 }
 
+/*
+ * Check the "black list" of CPUs, which are not supported by this code
+ */
+int armv7_is_cpu_blacklisted_for_nonsec(void)
+{
+	unsigned midr;
+	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(midr));
+
+	/* Cortex-A8 is not supported yet */
+	if ((midr & MIDR_PRIMARY_PART_MASK) == MIDR_CORTEX_A8_PRIMARY_PART)
+		return 1;
+
+	return 0;
+}
+
 int armv7_init_nonsec(void)
 {
 	unsigned int reg;
 	unsigned itlinesnr, i;
 
+	if (armv7_is_cpu_blacklisted_for_nonsec()) {
+		printf("nonsec: This CPU is not supported.\n");
+		return -1;
+	}
+
 	/* check whether the CPU supports the security extensions */
 	reg = read_id_pfr1();
 	if ((reg & 0xF0) == 0) {
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 323f282..e22c9c1 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -8,6 +8,10 @@
 #ifndef ARMV7_H
 #define ARMV7_H
 
+/* Cortex-A8 revisions */
+#define MIDR_CORTEX_A8_PRIMARY_PART	0x410FC080
+#define MIDR_CORTEX_A8_R3P2		0x413FC082
+
 /* Cortex-A9 revisions */
 #define MIDR_CORTEX_A9_R0P1	0x410FC091
 #define MIDR_CORTEX_A9_R1P2	0x411FC092
@@ -80,6 +84,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
 
 int armv7_init_nonsec(void);
 int armv7_update_dt(void *fdt);
+int armv7_is_cpu_blacklisted_for_nonsec(void);
 
 /* defined in assembly file */
 unsigned int _nonsec_init(void);
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 178e8fb..888a0ba 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -281,9 +281,19 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
 
 	if (!fake) {
 #if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
-		armv7_init_nonsec();
-		secure_ram_addr(_do_nonsec_entry)(kernel_entry,
-						  0, machid, r2);
+		if (armv7_init_nonsec()) {
+			printf("Switch to non-secure mode has failed - ");
+#ifdef CONFIG_ARMV7_ALLOW_SECURE_MODE_FALLBACK
+			printf("booting the kernel in secure mode ...\n\n");
+			kernel_entry(0, machid, r2);
+#else
+			printf("hanging ...\n");
+			hang();
+#endif
+		} else {
+			secure_ram_addr(_do_nonsec_entry)(kernel_entry,
+							  0, machid, r2);
+		}
 #else
 		kernel_entry(0, machid, r2);
 #endif
-- 
1.8.3.2

[U-Boot] [PATCH] ARM: HYP/non-sec: Add MIDR check to detect unsupported CPUs

[Marc Zyngier]

Hi Siarhei,

On 03/08/14 03:36, Siarhei Siamashka wrote:
> Unlike 9d195a546179bc732aba9eacccf0a9a3db591288, which had removed
> the MIDR check against the "white list" of supported CPUs earlier,
> now we introduce the "black list" of unsupported CPUs.
> 
> The current PSCI code is not compatible with the Cortex-A8 CPU used
> in Allwinner A10/A13 SoCs because of making GIC and virtualization
> support assumptions. Allwinner A10 and Allwinner A20 (with Cortex-A7
> CPU, supported by PSCI) are pin compatible, they can be used as
> drop-in replacements for each other and share the same PCB design:
>    https://www.olimex.com/Products/OLinuXino/A10/A10-OLinuXino-LIME
>    https://www.olimex.com/Products/OLinuXino/A20/A20-OLinuXino-LIME
> The same u-boot binary binary can boot on Allwinner A10 and
> Allwinner A20 with just minor tweaks applied.
> 
> This patch implements one of such necessary tweaks to allow the
> PSCI code to be compiled in, but skip it if Cortex-A8 is detected
> at runtime. The function 'armv7_init_nonsec()' now returns an error
> in the case if Cortex-A8 is detected. Also an extra error check
> is added for the 'armv7_init_nonsec()' call. If the board config
> header file provides CONFIG_ARMV7_ALLOW_SECURE_MODE_FALLBACK define,
> then the kernel is loaded in secure mode as a fallback. In the
> case is this define is not provided, the failure to switch to
> non-secure mode is treated as a fatal error and an appropriate
> message is displayed.
> 
> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

My personal feeling is that booting in secure mode is always the wrong
thing to do. But worse than anything else, you're here blacklisting CPUs
that are perfectly happy to run in non-secure mode, which do have a GIC,
and so on (I have a Cortex-A8 platform on my desk that has these features).

If you want to go down the road of a single bootloader that is able to
run on several SOCs, then do it the proper way: parse the device tree
and have separate constraints for your SoC. But please don't blacklist
random cores just because it fits your environment.

Thanks,

	M.

> ---
>  arch/arm/cpu/armv7/virt-dt.c |  3 +++
>  arch/arm/cpu/armv7/virt-v7.c | 20 ++++++++++++++++++++
>  arch/arm/include/asm/armv7.h |  5 +++++
>  arch/arm/lib/bootm.c         | 16 +++++++++++++---
>  4 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
> index 0b0d6a7..ab1fae9 100644
> --- a/arch/arm/cpu/armv7/virt-dt.c
> +++ b/arch/arm/cpu/armv7/virt-dt.c
> @@ -31,6 +31,9 @@ static int fdt_psci(void *fdt)
>  	int nodeoff;
>  	int tmp;
>  
> +	if (armv7_is_cpu_blacklisted_for_nonsec())
> +		return 0;
> +
>  	nodeoff = fdt_path_offset(fdt, "/cpus");
>  	if (nodeoff < 0) {
>  		printf("couldn't find /cpus\n");
> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
> index 651ca40..4b3bc54 100644
> --- a/arch/arm/cpu/armv7/virt-v7.c
> +++ b/arch/arm/cpu/armv7/virt-v7.c
> @@ -71,11 +71,31 @@ void __weak smp_kick_all_cpus(void)
>  	kick_secondary_cpus_gic(gic_dist_addr);
>  }
>  
> +/*
> + * Check the "black list" of CPUs, which are not supported by this code
> + */
> +int armv7_is_cpu_blacklisted_for_nonsec(void)
> +{
> +	unsigned midr;
> +	asm("mrc p15, 0, %0, c0, c0, 0\n" : "=r"(midr));
> +
> +	/* Cortex-A8 is not supported yet */
> +	if ((midr & MIDR_PRIMARY_PART_MASK) == MIDR_CORTEX_A8_PRIMARY_PART)
> +		return 1;
> +
> +	return 0;
> +}
> +
>  int armv7_init_nonsec(void)
>  {
>  	unsigned int reg;
>  	unsigned itlinesnr, i;
>  
> +	if (armv7_is_cpu_blacklisted_for_nonsec()) {
> +		printf("nonsec: This CPU is not supported.\n");
> +		return -1;
> +	}
> +
>  	/* check whether the CPU supports the security extensions */
>  	reg = read_id_pfr1();
>  	if ((reg & 0xF0) == 0) {
> diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
> index 323f282..e22c9c1 100644
> --- a/arch/arm/include/asm/armv7.h
> +++ b/arch/arm/include/asm/armv7.h
> @@ -8,6 +8,10 @@
>  #ifndef ARMV7_H
>  #define ARMV7_H
>  
> +/* Cortex-A8 revisions */
> +#define MIDR_CORTEX_A8_PRIMARY_PART	0x410FC080
> +#define MIDR_CORTEX_A8_R3P2		0x413FC082
> +
>  /* Cortex-A9 revisions */
>  #define MIDR_CORTEX_A9_R0P1	0x410FC091
>  #define MIDR_CORTEX_A9_R1P2	0x411FC092
> @@ -80,6 +84,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
>  
>  int armv7_init_nonsec(void);
>  int armv7_update_dt(void *fdt);
> +int armv7_is_cpu_blacklisted_for_nonsec(void);
>  
>  /* defined in assembly file */
>  unsigned int _nonsec_init(void);
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 178e8fb..888a0ba 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -281,9 +281,19 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
>  
>  	if (!fake) {
>  #if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
> -		armv7_init_nonsec();
> -		secure_ram_addr(_do_nonsec_entry)(kernel_entry,
> -						  0, machid, r2);
> +		if (armv7_init_nonsec()) {
> +			printf("Switch to non-secure mode has failed - ");
> +#ifdef CONFIG_ARMV7_ALLOW_SECURE_MODE_FALLBACK
> +			printf("booting the kernel in secure mode ...\n\n");
> +			kernel_entry(0, machid, r2);
> +#else
> +			printf("hanging ...\n");
> +			hang();
> +#endif
> +		} else {
> +			secure_ram_addr(_do_nonsec_entry)(kernel_entry,
> +							  0, machid, r2);
> +		}
>  #else
>  		kernel_entry(0, machid, r2);
>  #endif
> 


-- 
Jazz is not dead. It just smells funny...

[U-Boot] [PATCH] ARM: HYP/non-sec: Add MIDR check to detect unsupported CPUs

[Ian Campbell]

On Mon, 2014-08-04 at 16:14 +0100, Marc Zyngier wrote:

> My personal feeling is that booting in secure mode is always the wrong
> thing to do.

FWIW I agree.

> If you want to go down the road of a single bootloader that is able to
> run on several SOCs, then do it the proper way: parse the device tree
> and have separate constraints for your SoC. But please don't blacklist
> random cores just because it fits your environment.

I think there is a CPU feature register which indicates whether support
for HYP mode is present, isn't there? In which case a tolerable fix for
now (going all the way DT is a big yakk to shave...) would be to use
that to decide between booting in NS.HYP vs NS.SVC (nb: not NS.HYP vs
S.SVC).

I don't recall if the GIC has a feature bit for the security extensions,
but if not then inferring it from the CPUs support wouldn't be the worst
thing in the world under the circumstances.

Ian.

[U-Boot] [PATCH] ARM: HYP/non-sec: Add MIDR check to detect unsupported CPUs

[Mark Rutland]

On Wed, Aug 06, 2014 at 08:38:13AM +0100, Ian Campbell wrote:
> On Mon, 2014-08-04 at 16:14 +0100, Marc Zyngier wrote:
> 
> > My personal feeling is that booting in secure mode is always the wrong
> > thing to do.
> 
> FWIW I agree.
> 
> > If you want to go down the road of a single bootloader that is able to
> > run on several SOCs, then do it the proper way: parse the device tree
> > and have separate constraints for your SoC. But please don't blacklist
> > random cores just because it fits your environment.
> 
> I think there is a CPU feature register which indicates whether support
> for HYP mode is present, isn't there?

ID_PFR1[15:12] should tell you if the CPU has the virtualization
extensions.

> In which case a tolerable fix for now (going all the way DT is a big
> yakk to shave...) would be to use that to decide between booting in
> NS.HYP vs NS.SVC (nb: not NS.HYP vs S.SVC).

That sounds ideal.

> I don't recall if the GIC has a feature bit for the security extensions,
> but if not then inferring it from the CPUs support wouldn't be the worst
> thing in the world under the circumstances.

GICD_TYPER[10] (SecurityExtn) should tell you if the GIC has the
security extensions. I don't know whether you'll encounter a platform
where the CPU and GIC are mismatched w.r.t. security extensions.

Mark.

[U-Boot] [PATCH] ARM: HYP/non-sec: Add MIDR check to detect unsupported CPUs

[Marc Zyngier]

On 06/08/14 10:49, Mark Rutland wrote:
> On Wed, Aug 06, 2014 at 08:38:13AM +0100, Ian Campbell wrote:
>> On Mon, 2014-08-04 at 16:14 +0100, Marc Zyngier wrote:
>>
>>> My personal feeling is that booting in secure mode is always the wrong
>>> thing to do.
>>
>> FWIW I agree.
>>
>>> If you want to go down the road of a single bootloader that is able to
>>> run on several SOCs, then do it the proper way: parse the device tree
>>> and have separate constraints for your SoC. But please don't blacklist
>>> random cores just because it fits your environment.
>>
>> I think there is a CPU feature register which indicates whether support
>> for HYP mode is present, isn't there?
> 
> ID_PFR1[15:12] should tell you if the CPU has the virtualization
> extensions.
> 
>> In which case a tolerable fix for now (going all the way DT is a big
>> yakk to shave...) would be to use that to decide between booting in
>> NS.HYP vs NS.SVC (nb: not NS.HYP vs S.SVC).
> 
> That sounds ideal.
> 
>> I don't recall if the GIC has a feature bit for the security extensions,
>> but if not then inferring it from the CPUs support wouldn't be the worst
>> thing in the world under the circumstances.
> 
> GICD_TYPER[10] (SecurityExtn) should tell you if the GIC has the
> security extensions. I don't know whether you'll encounter a platform
> where the CPU and GIC are mismatched w.r.t. security extensions.

I know at least of one for which this is the case... which makes
switching the interrupts from Group0 to Group1 a fun adventure.

	M.
-- 
Jazz is not dead. It just smells funny...