[v2] vexpress: disable cci ace slave ports when booting in non-sec/hyp mode

Message ID 1474648719-2910-1-git-send-email-sudeep.holla@arm.com
State Superseded
Headers show

Commit Message

Sudeep Holla Sept. 23, 2016, 4:38 p.m.
Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting
to nonsec booting") added support to check if the firmware on TC2  is
configured appropriately before booting in nonsec/hyp mode.

However when booting in non-secure/hyp mode, CCI control must be done in
secure firmware and can't  be done in non-secure/hyp mode. In order to
ensure that, this patch disables the cci slave port inteface so that it
is not accessed at all.

Cc: Jon Medhurst <tixy@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 board/armltd/vexpress/vexpress_tc2.c | 52 ++++++++++++++++++++++++++++++++++++
 configs/vexpress_ca15_tc2_defconfig  |  1 +
 2 files changed, 53 insertions(+)

Hi,

This change is needed to avoid the kernel panic while attempting to access
CCI ports when booting in non-sec/HYP mode. The kernel patches to fix
this are available @[1]

Regards,
Sudeep

v1->v2:
	- Fix compilation with !CONFIG_ARMV7_NONSEC(Thanks to Tixy)

[1] http://www.spinics.net/lists/arm-kernel/msg533715.html

--
2.7.4

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

Comments

Jon Medhurst (Tixy) Sept. 26, 2016, 11:30 a.m. | #1
On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:
> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting

> to nonsec booting") added support to check if the firmware on TC2  is

> configured appropriately before booting in nonsec/hyp mode.

> 

> However when booting in non-secure/hyp mode, CCI control must be done in

> secure firmware and can't  be done in non-secure/hyp mode. In order to

> ensure that, this patch disables the cci slave port inteface so that it

> is not accessed at all.

> 

> Cc: Jon Medhurst <tixy@linaro.org>

> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---


This works for me (unsurprisingly) when booting in secure mode. What
kernel and firmware config do I need to use for non-sec mode? I tried
vexpress_defconfig, with bits 12 and 13 cleared in SCC: 0x700
but I get nothing out the console after "Starting kernel ..."

-- 
Tixy

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Sudeep Holla Sept. 26, 2016, 11:35 a.m. | #2
On 26/09/16 12:30, Jon Medhurst (Tixy) wrote:
> On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:

>> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting

>> to nonsec booting") added support to check if the firmware on TC2  is

>> configured appropriately before booting in nonsec/hyp mode.

>>

>> However when booting in non-secure/hyp mode, CCI control must be done in

>> secure firmware and can't  be done in non-secure/hyp mode. In order to

>> ensure that, this patch disables the cci slave port inteface so that it

>> is not accessed at all.

>>

>> Cc: Jon Medhurst <tixy@linaro.org>

>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>> ---

>

> This works for me (unsurprisingly) when booting in secure mode. What

> kernel and firmware config do I need to use for non-sec mode? I tried

> vexpress_defconfig, with bits 12 and 13 cleared in SCC: 0x700

> but I get nothing out the console after "Starting kernel ..."

>


I just flipped bit 12 and 13 in SCC: 0x700 to switch between MCPM/secure
and HYP/non-secure mode. All other images/settings remain the same.

So IIUC, with vexpress_defconfig + above SCC change you are seeing a
hand, I will check that. I am using multi_v7_defconfig.

-- 
Regards,
Sudeep
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Sudeep Holla Sept. 26, 2016, 11:37 a.m. | #3
On 26/09/16 12:35, Sudeep Holla wrote:
>

>

> On 26/09/16 12:30, Jon Medhurst (Tixy) wrote:

>> On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:

>>> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before

>>> defaulting

>>> to nonsec booting") added support to check if the firmware on TC2  is

>>> configured appropriately before booting in nonsec/hyp mode.

>>>

>>> However when booting in non-secure/hyp mode, CCI control must be done in

>>> secure firmware and can't  be done in non-secure/hyp mode. In order to

>>> ensure that, this patch disables the cci slave port inteface so that it

>>> is not accessed at all.

>>>

>>> Cc: Jon Medhurst <tixy@linaro.org>

>>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>> ---

>>

>> This works for me (unsurprisingly) when booting in secure mode. What

>> kernel and firmware config do I need to use for non-sec mode? I tried

>> vexpress_defconfig, with bits 12 and 13 cleared in SCC: 0x700

>> but I get nothing out the console after "Starting kernel ..."

>>

>

> I just flipped bit 12 and 13 in SCC: 0x700 to switch between MCPM/secure

> and HYP/non-secure mode. All other images/settings remain the same.

>

> So IIUC, with vexpress_defconfig + above SCC change you are seeing a

> hand, I will check that. I am using multi_v7_defconfig.

>


+ the patches from Lorenzo fixing MCPM code in the kernel [1] as
mentioned first email carrying this patch.

-- 
Regards,
Sudeep

[1] http://www.spinics.net/lists/arm-kernel/msg533715.html
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Jon Medhurst (Tixy) Sept. 26, 2016, 12:30 p.m. | #4
On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:
> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting

> to nonsec booting") added support to check if the firmware on TC2  is

> configured appropriately before booting in nonsec/hyp mode.

> 

> However when booting in non-secure/hyp mode, CCI control must be done in

> secure firmware and can't  be done in non-secure/hyp mode. In order to

> ensure that, this patch disables the cci slave port inteface so that it

> is not accessed at all.

> 

> Cc: Jon Medhurst <tixy@linaro.org>

> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---


Acked-by: Jon Medhurst <tixy@linaro.org>

Tested-by: Jon Medhurst <tixy@linaro.org>


>  board/armltd/vexpress/vexpress_tc2.c | 52 ++++++++++++++++++++++++++++++++++++

>  configs/vexpress_ca15_tc2_defconfig  |  1 +

>  2 files changed, 53 insertions(+)

> 

> Hi,

> 

> This change is needed to avoid the kernel panic while attempting to access

> CCI ports when booting in non-sec/HYP mode. The kernel patches to fix

> this are available @[1]

> 

> Regards,

> Sudeep

> 

> v1->v2:

> 	- Fix compilation with !CONFIG_ARMV7_NONSEC(Thanks to Tixy)

> 

> [1] http://www.spinics.net/lists/arm-kernel/msg533715.html

> 

> diff --git a/board/armltd/vexpress/vexpress_tc2.c b/board/armltd/vexpress/vexpress_tc2.c

> index ebb41a8833ab..c7adf950f579 100644

> --- a/board/armltd/vexpress/vexpress_tc2.c

> +++ b/board/armltd/vexpress/vexpress_tc2.c

> @@ -7,7 +7,11 @@

>   * SPDX-License-Identifier:	GPL-2.0+

>   */

> 

> +#include <asm/armv7.h>

>  #include <asm/io.h>

> +#include <asm/u-boot.h>

> +#include <common.h>

> +#include <libfdt.h>

> 

>  #define SCC_BASE	0x7fff0000

> 

> @@ -31,3 +35,51 @@ bool armv7_boot_nonsec_default(void)

>  	return (readl((u32 *)(SCC_BASE + 0x700)) & ((1 << 12) | (1 << 13))) == 0;

>  #endif

>  }

> +

> +#ifdef CONFIG_OF_BOARD_SETUP

> +int ft_board_setup(void *fdt, bd_t *bd)

> +{

> +	int offset, tmp, len;

> +	const struct fdt_property *prop;

> +	const char *cci_compatible = "arm,cci-400-ctrl-if";

> +

> +#ifdef CONFIG_ARMV7_NONSEC

> +	if (!armv7_boot_nonsec())

> +		return 0;

> +#else

> +	return 0;

> +#endif

> +	/* Booting in nonsec mode, disable CCI access */

> +	offset = fdt_path_offset(fdt, "/cpus");

> +	if (offset < 0) {

> +		printf("couldn't find /cpus\n");

> +		return offset;

> +	}

> +

> +	/* delete cci-control-port in each cpu node */

> +	for (tmp = fdt_first_subnode(fdt, offset); tmp >= 0;

> +	     tmp = fdt_next_subnode(fdt, tmp))

> +		fdt_delprop(fdt, tmp, "cci-control-port");

> +

> +	/* disable all ace cci slave ports */

> +	offset = fdt_node_offset_by_prop_value(fdt, offset, "compatible",

> +					       cci_compatible, 20);

> +	while (offset > 0) {

> +		prop = fdt_get_property(fdt, offset, "interface-type",

> +					&len);

> +		if (!prop)

> +			continue;

> +		if (len < 4)

> +			continue;

> +		if (strcmp(prop->data, "ace"))

> +			continue;

> +

> +		fdt_setprop_string(fdt, offset, "status", "disabled");

> +

> +		offset = fdt_node_offset_by_prop_value(fdt, offset, "compatible",

> +						       cci_compatible, 20);

> +	}

> +

> +	return 0;

> +}

> +#endif /* CONFIG_OF_BOARD_SETUP */

> diff --git a/configs/vexpress_ca15_tc2_defconfig b/configs/vexpress_ca15_tc2_defconfig

> index 2f141dda06c6..5154803b7c65 100644

> --- a/configs/vexpress_ca15_tc2_defconfig

> +++ b/configs/vexpress_ca15_tc2_defconfig

> @@ -1,5 +1,6 @@

>  CONFIG_ARM=y

>  CONFIG_TARGET_VEXPRESS_CA15_TC2=y

> +CONFIG_OF_BOARD_SETUP=y

>  CONFIG_HUSH_PARSER=y

>  # CONFIG_CMD_CONSOLE is not set

>  # CONFIG_CMD_BOOTD is not set

> --

> 2.7.4

> 



_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Sudeep Holla Sept. 26, 2016, 12:38 p.m. | #5
On 26/09/16 13:30, Jon Medhurst (Tixy) wrote:
> On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:

>> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting

>> to nonsec booting") added support to check if the firmware on TC2  is

>> configured appropriately before booting in nonsec/hyp mode.

>>

>> However when booting in non-secure/hyp mode, CCI control must be done in

>> secure firmware and can't  be done in non-secure/hyp mode. In order to

>> ensure that, this patch disables the cci slave port inteface so that it

>> is not accessed at all.

>>

>> Cc: Jon Medhurst <tixy@linaro.org>

>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>> ---

>

> Acked-by: Jon Medhurst <tixy@linaro.org>

> Tested-by: Jon Medhurst <tixy@linaro.org>


So, can I assume the missing kernel patches to be reason for boot hang ?
Just wanted to know if I need to investigate that any further ?

-- 
Regards,
Sudeep
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Jon Medhurst (Tixy) Sept. 26, 2016, 1:19 p.m. | #6
On Mon, 2016-09-26 at 13:38 +0100, Sudeep Holla wrote:
> 

> On 26/09/16 13:30, Jon Medhurst (Tixy) wrote:

> > On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:

> >> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting

> >> to nonsec booting") added support to check if the firmware on TC2  is

> >> configured appropriately before booting in nonsec/hyp mode.

> >>

> >> However when booting in non-secure/hyp mode, CCI control must be done in

> >> secure firmware and can't  be done in non-secure/hyp mode. In order to

> >> ensure that, this patch disables the cci slave port inteface so that it

> >> is not accessed at all.

> >>

> >> Cc: Jon Medhurst <tixy@linaro.org>

> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> >> ---

> >

> > Acked-by: Jon Medhurst <tixy@linaro.org>

> > Tested-by: Jon Medhurst <tixy@linaro.org>

> 

> So, can I assume the missing kernel patches to be reason for boot hang ?

> Just wanted to know if I need to investigate that any further ?


Sorry, yes they were the reason and no further investigation needed. I
remembered getting nonsec mode working some month's ago without such
patches, but I remember now that was by disabling MCPM in the kernel.

This morning I tried these U-Boot patches successfully with:
- Upstream vexpress_defconfig kernel booting with 'sec' mode
- That kernel with Lorenzo's patches in both 'sec' and 'nonsec'
- As above with CONFIG_BL_SWITCHER enabled

When booting in nonsec I also verified the device-tree modifications
made by this patch by seeing the following files existed and contained
the word 'disabled'...

/proc/device-tree/cci@2c090000/slave-if@4000/status
/proc/device-tree/cci@2c090000/slave-if@5000/status

-- 
Tixy

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Sudeep Holla Sept. 26, 2016, 1:24 p.m. | #7
On 26/09/16 14:19, Jon Medhurst (Tixy) wrote:
> On Mon, 2016-09-26 at 13:38 +0100, Sudeep Holla wrote:

>>

>> On 26/09/16 13:30, Jon Medhurst (Tixy) wrote:

>>> On Fri, 2016-09-23 at 17:38 +0100, Sudeep Holla wrote:

>>>> Commit f225d39d3093 ("vexpress: Check TC2 firmware support before defaulting

>>>> to nonsec booting") added support to check if the firmware on TC2  is

>>>> configured appropriately before booting in nonsec/hyp mode.

>>>>

>>>> However when booting in non-secure/hyp mode, CCI control must be done in

>>>> secure firmware and can't  be done in non-secure/hyp mode. In order to

>>>> ensure that, this patch disables the cci slave port inteface so that it

>>>> is not accessed at all.

>>>>

>>>> Cc: Jon Medhurst <tixy@linaro.org>

>>>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>>> ---

>>>

>>> Acked-by: Jon Medhurst <tixy@linaro.org>

>>> Tested-by: Jon Medhurst <tixy@linaro.org>

>>

>> So, can I assume the missing kernel patches to be reason for boot hang ?

>> Just wanted to know if I need to investigate that any further ?

>

> Sorry, yes they were the reason and no further investigation needed. I

> remembered getting nonsec mode working some month's ago without such

> patches, but I remember now that was by disabling MCPM in the kernel.

>

> This morning I tried these U-Boot patches successfully with:

> - Upstream vexpress_defconfig kernel booting with 'sec' mode

> - That kernel with Lorenzo's patches in both 'sec' and 'nonsec'

> - As above with CONFIG_BL_SWITCHER enabled

>

> When booting in nonsec I also verified the device-tree modifications

> made by this patch by seeing the following files existed and contained

> the word 'disabled'...

>

> /proc/device-tree/cci@2c090000/slave-if@4000/status

> /proc/device-tree/cci@2c090000/slave-if@5000/status

>


That was very detailed :), thanks for testing and confirming. I too did
similar examination and didn't find anything odd so far.

-- 
Regards,
Sudeep
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Patch

diff --git a/board/armltd/vexpress/vexpress_tc2.c b/board/armltd/vexpress/vexpress_tc2.c
index ebb41a8833ab..c7adf950f579 100644
--- a/board/armltd/vexpress/vexpress_tc2.c
+++ b/board/armltd/vexpress/vexpress_tc2.c
@@ -7,7 +7,11 @@ 
  * SPDX-License-Identifier:	GPL-2.0+
  */

+#include <asm/armv7.h>
 #include <asm/io.h>
+#include <asm/u-boot.h>
+#include <common.h>
+#include <libfdt.h>

 #define SCC_BASE	0x7fff0000

@@ -31,3 +35,51 @@  bool armv7_boot_nonsec_default(void)
 	return (readl((u32 *)(SCC_BASE + 0x700)) & ((1 << 12) | (1 << 13))) == 0;
 #endif
 }
+
+#ifdef CONFIG_OF_BOARD_SETUP
+int ft_board_setup(void *fdt, bd_t *bd)
+{
+	int offset, tmp, len;
+	const struct fdt_property *prop;
+	const char *cci_compatible = "arm,cci-400-ctrl-if";
+
+#ifdef CONFIG_ARMV7_NONSEC
+	if (!armv7_boot_nonsec())
+		return 0;
+#else
+	return 0;
+#endif
+	/* Booting in nonsec mode, disable CCI access */
+	offset = fdt_path_offset(fdt, "/cpus");
+	if (offset < 0) {
+		printf("couldn't find /cpus\n");
+		return offset;
+	}
+
+	/* delete cci-control-port in each cpu node */
+	for (tmp = fdt_first_subnode(fdt, offset); tmp >= 0;
+	     tmp = fdt_next_subnode(fdt, tmp))
+		fdt_delprop(fdt, tmp, "cci-control-port");
+
+	/* disable all ace cci slave ports */
+	offset = fdt_node_offset_by_prop_value(fdt, offset, "compatible",
+					       cci_compatible, 20);
+	while (offset > 0) {
+		prop = fdt_get_property(fdt, offset, "interface-type",
+					&len);
+		if (!prop)
+			continue;
+		if (len < 4)
+			continue;
+		if (strcmp(prop->data, "ace"))
+			continue;
+
+		fdt_setprop_string(fdt, offset, "status", "disabled");
+
+		offset = fdt_node_offset_by_prop_value(fdt, offset, "compatible",
+						       cci_compatible, 20);
+	}
+
+	return 0;
+}
+#endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/configs/vexpress_ca15_tc2_defconfig b/configs/vexpress_ca15_tc2_defconfig
index 2f141dda06c6..5154803b7c65 100644
--- a/configs/vexpress_ca15_tc2_defconfig
+++ b/configs/vexpress_ca15_tc2_defconfig
@@ -1,5 +1,6 @@ 
 CONFIG_ARM=y
 CONFIG_TARGET_VEXPRESS_CA15_TC2=y
+CONFIG_OF_BOARD_SETUP=y
 CONFIG_HUSH_PARSER=y
 # CONFIG_CMD_CONSOLE is not set
 # CONFIG_CMD_BOOTD is not set