* Massive stm32mp1 breakage with v2021.10-rc2
@ 2021-08-24 20:30 Alex G.
2021-08-26 21:47 ` [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP Alexandru Gagniuc
0 siblings, 1 reply; 8+ messages in thread
From: Alex G. @ 2021-08-24 20:30 UTC (permalink / raw)
To: Patrick DELAUNAY, u-boot; +Cc: Marek Vasut
Hi Patrick,
I'm having issues with some of the recent changes centered around FIP
support and CONFIG_STM32MP15x_STM32IMAGE. and commit f91783edf224 ("arm:
stm32mp: handle the OP-TEE nodes in DT with FIP support")
## Problem description
> +#ifdef CONFIG_STM32MP15x_STM32IMAGE
> + /* only needed for boot with TF-A, witout FIP support */
> firmware {
> optee {
> compatible = "linaro,optee-tz";
> @@ -33,6 +35,7 @@
> no-map;
> };
> };
> +#endif
This removes the "optee" and "reserved-memory" nodes. These nodes are
required by SPL for setting up TZC memory regions, as introduced in
commit 8533263c8512 ("stm32mp1: spl: Configure TrustZone controller for
OP-TEE").
## Further details
We now have several boot flows:
1) BL1 -> SPL -> u-boot
2) BL1 -> SPL -> OP-TEE (this path is now broken)
3) BL1 -> TF-A -> u-boot (use case for STM32IMAGE)
4) BL1 -> TF-A -> OP-TEE (use case for STM32IMAGE)
5) BL1 -> TF-A -> FIP container
So it seems that STM32IMAGE only makes sense for (3) and (4), but
shouldn't affect the others. The fact that OP-TEE is mixed into this is
the first red flag.
> INPUTS-$(CONFIG_STM32MP15x_STM32IMAGE) += u-boot.stm32
> MKIMAGEFLAGS_u-boot.stm32 = -T stm32image ...
This tells me we use _STM32IMAGE just to enable another output image
format. The build could give me both u-boot.img, and u-boot.stm32, and I
would use the correct file. SO in this case, I would expect _STM32IMAGE
to not change the code behavior. This is the second red flag.
> $ git grep -c 'ifdef CONFIG_STM32MP15x_STM32IMAGE'
> arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c:1
> arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c:2
> arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h:1
> arch/arm/mach-stm32mp/include/mach/stm32prog.h:1
We're actually trying to move away from ifdefs, so this intense reliance
on _STM32IMAGE raises the third red flag.
> board/st/common/stm32mp_mtdparts.c:9
I'm not sure I fully understand why there are so many ifdefs in
mtdparts.c. MTD layout seems to me like something that is intended to be
devicetree-driven. This is the fourth red flag.
Let's take a deeper look at one of those:
> stm32mp_mtdparts.c:#ifdef CONFIG_STM32MP15x_STM32IMAGE
> stm32mp_mtdparts.c: tee = >
stm32prog_get_tee_partitions();
> stm32mp_mtdparts.c-#endif
This makes no sense to me. What does OP-TEE presence have to do with the
image format of u-boot? If TF-A requires a different layout in FIP vs
non-FIP mode, then it's the responsibility of TF-A to supply a corrected
devicetree to u-boot. It's not u-boot's role.
## Proposal
I propose we remove CONFIG_STM32MP15x_STM32IMAGE. Always build
u-boot.stm32, and don't mix it with code behavior.
Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-08-24 20:30 Massive stm32mp1 breakage with v2021.10-rc2 Alex G.
@ 2021-08-26 21:47 ` Alexandru Gagniuc
2021-08-31 14:54 ` Patrick DELAUNAY
0 siblings, 1 reply; 8+ messages in thread
From: Alexandru Gagniuc @ 2021-08-26 21:47 UTC (permalink / raw)
To: patrick.delaunay, u-boot; +Cc: marex, Alexandru Gagniuc
Hi Patrick,
I proposing a better fix fir the issues I outlined earlier, I made a
classification of the currently supported boot modes.
1) BL1 -> SPL -> u-boot
2) BL1 -> SPL -> OP-TEE
---------------------------------------------------------------------
| 3) BL1 -> TF-A -> u-boot |
| 4) BL1 -> TF-A -> OP-TEE |
| _________________________________________________________________ |
|| 5) BL1 -> TF-A -> FIP container ||
|| CONFIG_TFABOOT_FIP ||
||_________________________________________________________________||
| |
| CONFIG_TFABOOT |
---------------------------------------------------------------------
Here, I'm looking at FIP as a new boot mode. In order to avoid
breakage, any changes to support FIP it should naturally be done only
to this new path.
This proposal contains several changes, but I've squashed them into
one for ease of discussion.
This better matches the boot mode classification above.
This config mixes boot path (2) with paths (3) and (4), and thus is
contrary to our goal of making changes only to the new FIP path.
Because it mixes and matches SPL assumptions with TF-A assumptions,
I've had a hard time figuting it out. I suspect it would be just as
confusing for others in the future.
I've had issues with tee_find_device() in the past when using SPL as
the FSBL. As u-boot was running in secure mode and did not have a
handler, it would result in a CPU exception and crash.
The second argument against this is that stm32mp1 is the only platform
to call tee_find_device() with the intent of detecting the presence of
OP-TEE.
Have there been issues with not callinf this in the past, or was this
more of a "seems nice to have" ?
"stm32mp15_defconfig" implies that would be the correct configuraion
for STM32MP1. New contributor chooses this config, tries to run SPL
+ u-boot, which is what u-boot user expects is the default. Things
likely fail miserably. A lot of u-boot users don't know what FIP is.
It's an extra concept that is not strictly necessary in u-boot.
So I think this name is vague, as it doesn't really describe what is
going on. If we change it to "stm32mp15_tfaboot_fip_defconfig", then
it very accurately describes the boot scenario, and avoids the
confusion above.
We're setting CONFIG_MTDPARTS_xxx based on TFABOOT_FIP_CONTAINER now,
so I don't think we need any ifdefs here. This part needs the most
scrutiny, as I don't have a way to test if I've broken anything.
---
arch/arm/Kconfig | 14 +++++++
arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi | 7 ++--
arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi | 9 ++---
arch/arm/mach-stm32mp/Kconfig | 7 ----
.../cmd_stm32prog/cmd_stm32prog.c | 2 -
.../mach-stm32mp/cmd_stm32prog/stm32prog.c | 4 --
.../mach-stm32mp/cmd_stm32prog/stm32prog.h | 2 -
arch/arm/mach-stm32mp/config.mk | 2 +-
arch/arm/mach-stm32mp/fdt.c | 37 -------------------
.../arm/mach-stm32mp/include/mach/stm32prog.h | 2 -
board/st/common/Kconfig | 20 +++++-----
board/st/common/stm32mp_mtdparts.c | 18 ---------
board/st/stm32mp1/stm32mp1.c | 6 +--
...config => stm32mp15_tfaboot_fip_defconfig} | 1 +
configs/stm32mp15_trusted_defconfig | 1 -
15 files changed, 35 insertions(+), 97 deletions(-)
rename configs/{stm32mp15_defconfig => stm32mp15_tfaboot_fip_defconfig} (99%)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d692139199..4c6f7ab3de 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1905,6 +1905,20 @@ config TFABOOT
Enabling this option will make a U-Boot binary that is relying
on other firmware layers to provide secure functionality.
+config TFABOOT_FIP_CONTAINER
+ bool "Support for booting from TF-A inside a FIP container"
+ depends on TFABOOT
+ default n
+ help
+ TF-A has its own container format, named FIP (not to be confused with
+ FIT). When u-boot is started from a FIP, it sometimes needs to make
+ different assumptions than it would with a non-FIP boot. Although
+ those could be resolved with dynamic devicetree patching, TF-A is
+ either can't patch devicetrees, or is unwilling to do so.
+ Enabling this option will tell u-boot platform code that it is okay
+ to assume U-Boot will be started from a FIP container, even if such
+ assumptions would break things in a more normal setting.
+
config TI_SECURE_DEVICE
bool "HS Device Type Support"
depends on ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
diff --git a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
index 0101962ea5..54424e398f 100644
--- a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
+++ b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
@@ -15,18 +15,18 @@
config {
u-boot,boot-led = "heartbeat";
u-boot,error-led = "error";
- u-boot,mmc-env-partition = "fip";
+ u-boot,mmc-env-partition = "fip1";
st,adc_usb_pd = <&adc1 18>, <&adc1 19>;
st,fastboot-gpios = <&gpioa 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
st,stm32prog-gpios = <&gpioa 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
};
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
+#ifdef CONFIG_TFABOOT_FIP_CONTAINER
config {
u-boot,mmc-env-partition = "ssbl";
};
+#endif
- /* only needed for boot with TF-A, witout FIP support */
firmware {
optee {
compatible = "linaro,optee-tz";
@@ -43,7 +43,6 @@
u-boot,dm-spl;
};
};
-#endif
led {
red {
diff --git a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
index 32777384c6..184774df58 100644
--- a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
+++ b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
@@ -15,17 +15,17 @@
config {
u-boot,boot-led = "heartbeat";
u-boot,error-led = "error";
- u-boot,mmc-env-partition = "fip";
+ u-boot,mmc-env-partition = "ssbl";
st,fastboot-gpios = <&gpioa 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
st,stm32prog-gpios = <&gpioa 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
};
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
+#ifdef CONFIG_TFABOOT_FIP_CONTAINER
config {
- u-boot,mmc-env-partition = "ssbl";
+ u-boot,mmc-env-partition = "fip";
};
+#endif
- /* only needed for boot with TF-A, witout FIP support */
firmware {
optee {
compatible = "linaro,optee-tz";
@@ -39,7 +39,6 @@
no-map;
};
};
-#endif
led {
red {
diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig
index 5d7eca649a..4c1eeef165 100644
--- a/arch/arm/mach-stm32mp/Kconfig
+++ b/arch/arm/mach-stm32mp/Kconfig
@@ -56,13 +56,6 @@ config STM32MP15x
dual core A7 for STM32MP157/3, monocore for STM32MP151
target all the STMicroelectronics board with SOC STM32MP1 family
-config STM32MP15x_STM32IMAGE
- bool "Support STM32 image for generated U-Boot image"
- depends on STM32MP15x && TFABOOT
- help
- Support of STM32 image generation for SOC STM32MP15x
- for TF-A boot when FIP container is not used
-
choice
prompt "STM32MP15x board select"
optional
diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
index 41452b5a29..be53a52977 100644
--- a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
+++ b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
@@ -185,7 +185,6 @@ U_BOOT_CMD(stm32prog, 5, 0, do_stm32prog,
" <size> = size of flashlayout (optional for image with STM32 header)\n"
);
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
bool stm32prog_get_tee_partitions(void)
{
if (stm32prog_data)
@@ -193,7 +192,6 @@ bool stm32prog_get_tee_partitions(void)
return false;
}
-#endif
bool stm32prog_get_fsbl_nor(void)
{
diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
index 3b6ca4e773..26fe8b654a 100644
--- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
+++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
@@ -824,9 +824,7 @@ static int treat_partition_list(struct stm32prog_data *data)
INIT_LIST_HEAD(&data->dev[j].part_list);
}
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
data->tee_detected = false;
-#endif
data->fsbl_nor_detected = false;
for (i = 0; i < data->part_nb; i++) {
part = &data->part_array[i];
@@ -880,12 +878,10 @@ static int treat_partition_list(struct stm32prog_data *data)
/* fallthrough */
case STM32PROG_NAND:
case STM32PROG_SPI_NAND:
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
if (!data->tee_detected &&
!strncmp(part->name, "tee", 3))
data->tee_detected = true;
break;
-#endif
default:
break;
}
diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
index 240c5c44bc..9d58cf0e2d 100644
--- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
+++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
@@ -122,9 +122,7 @@ struct stm32prog_data {
struct stm32prog_dev_t dev[STM32PROG_MAX_DEV]; /* array of device */
int part_nb; /* nb of partition */
struct stm32prog_part_t *part_array; /* array of partition */
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
bool tee_detected;
-#endif
bool fsbl_nor_detected;
/* command internal information */
diff --git a/arch/arm/mach-stm32mp/config.mk b/arch/arm/mach-stm32mp/config.mk
index f7f5b77c41..c30bf482f7 100644
--- a/arch/arm/mach-stm32mp/config.mk
+++ b/arch/arm/mach-stm32mp/config.mk
@@ -4,7 +4,7 @@
#
ifndef CONFIG_SPL
-INPUTS-$(CONFIG_STM32MP15x_STM32IMAGE) += u-boot.stm32
+INPUTS-y += u-boot.stm32
else
ifdef CONFIG_SPL_BUILD
INPUTS-y += u-boot-spl.stm32
diff --git a/arch/arm/mach-stm32mp/fdt.c b/arch/arm/mach-stm32mp/fdt.c
index a19e954cf7..8c729e1f32 100644
--- a/arch/arm/mach-stm32mp/fdt.c
+++ b/arch/arm/mach-stm32mp/fdt.c
@@ -224,30 +224,6 @@ static void stm32_fdt_disable(void *fdt, int offset, u32 addr,
string, addr, name);
}
-static void stm32_fdt_disable_optee(void *blob)
-{
- int off, node;
-
- /* Delete "optee" firmware node */
- off = fdt_node_offset_by_compatible(blob, -1, "linaro,optee-tz");
- if (off >= 0 && fdtdec_get_is_enabled(blob, off))
- fdt_del_node(blob, off);
-
- /* Delete "optee@..." reserved-memory node */
- off = fdt_path_offset(blob, "/reserved-memory/");
- if (off < 0)
- return;
- for (node = fdt_first_subnode(blob, off);
- node >= 0;
- node = fdt_next_subnode(blob, node)) {
- if (strncmp(fdt_get_name(blob, node, NULL), "optee@", 6))
- continue;
-
- if (fdt_del_node(blob, node))
- printf("Failed to remove optee reserved-memory node\n");
- }
-}
-
/*
* This function is called right before the kernel is booted. "blob" is the
* device tree that will be passed to the kernel.
@@ -332,18 +308,5 @@ int ft_system_setup(void *blob, struct bd_info *bd)
"st,package", pkg, false);
}
- /*
- * TEMP: remove OP-TEE nodes in kernel device tree
- * copied from U-Boot device tree by optee_copy_fdt_nodes
- * when OP-TEE is not detected (probe failed)
- * these OP-TEE nodes are present in <board>-u-boot.dtsi
- * under CONFIG_STM32MP15x_STM32IMAGE only for compatibility
- * when FIP is not used by TF-A
- */
- if (CONFIG_IS_ENABLED(STM32MP15x_STM32IMAGE) &&
- CONFIG_IS_ENABLED(OPTEE) &&
- !tee_find_device(NULL, NULL, NULL, NULL))
- stm32_fdt_disable_optee(blob);
-
return ret;
}
diff --git a/arch/arm/mach-stm32mp/include/mach/stm32prog.h b/arch/arm/mach-stm32mp/include/mach/stm32prog.h
index 99be4e1d65..c080b9cc42 100644
--- a/arch/arm/mach-stm32mp/include/mach/stm32prog.h
+++ b/arch/arm/mach-stm32mp/include/mach/stm32prog.h
@@ -11,8 +11,6 @@ int stm32prog_read_medium_virt(struct dfu_entity *dfu, u64 offset,
void *buf, long *len);
int stm32prog_get_medium_size_virt(struct dfu_entity *dfu, u64 *size);
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
bool stm32prog_get_tee_partitions(void);
-#endif
bool stm32prog_get_fsbl_nor(void);
diff --git a/board/st/common/Kconfig b/board/st/common/Kconfig
index 2f57118bb2..0bb8e808aa 100644
--- a/board/st/common/Kconfig
+++ b/board/st/common/Kconfig
@@ -8,9 +8,8 @@ config CMD_STBOARD
config MTDPARTS_NAND0_BOOT
string "mtd boot partitions for nand0"
- default "2m(fsbl),2m(ssbl1),2m(ssbl2)" if STM32MP15x_STM32IMAGE || \
- !TFABOOT
- default "2m(fsbl),4m(fip1),4m(fip2)"
+ default "2m(fsbl),2m(ssbl1),2m(ssbl2)"
+ default "2m(fsbl),4m(fip1),4m(fip2)" if TFABOOT_FIP_CONTAINER
depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
help
This define the partitions of nand0 used to build mtparts dynamically
@@ -23,7 +22,7 @@ config MTDPARTS_NAND0_BOOT
config MTDPARTS_NAND0_TEE
string "mtd tee partitions for nand0"
default "512k(teeh),512k(teed),512k(teex)"
- depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP && STM32MP15x_STM32IMAGE
+ depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
help
This define the tee partitions added in mtparts dynamically
when tee is supported with boot from nand0.
@@ -32,9 +31,8 @@ config MTDPARTS_NAND0_TEE
config MTDPARTS_NOR0_BOOT
string "mtd boot partitions for nor0"
- default "256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env)" if STM32MP15x_STM32IMAGE || \
- !TFABOOT
- default "256k(fsbl1),256k(fsbl2),4m(fip),512k(u-boot-env)"
+ default "256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env)"
+ default "256k(fsbl1),256k(fsbl2),4m(fip),512k(u-boot-env)" if TFABOOT_FIP_CONTAINER
depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
help
This define the partitions of nand0 used to build mtparts dynamically
@@ -46,15 +44,15 @@ config MTDPARTS_NOR0_BOOT
config MTDPARTS_NOR0_TEE
string "mtd tee partitions for nor0"
default "256k(teeh),512k(teed),256k(teex)"
- depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP && STM32MP15x_STM32IMAGE
+ depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
help
This define the tee partitions added in mtparts dynamically
when tee is supported with boot from nor0.
config MTDPARTS_SPINAND0_BOOT
string "mtd boot partitions for spi-nand0"
- default "2m(fsbl),2m(ssbl1),2m(ssbl2)" if STM32MP15x_STM32IMAGE || !TFABOOT
- default "2m(fsbl),4m(fip1),4m(fip2)"
+ default "2m(fsbl),2m(ssbl1),2m(ssbl2)"
+ default "2m(fsbl),4m(fip1),4m(fip2)" if TFABOOT_FIP_CONTAINER
depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
help
This define the partitions of nand0 used to build mtparts dynamically
@@ -66,7 +64,7 @@ config MTDPARTS_SPINAND0_BOOT
config MTDPARTS_SPINAND0_TEE
string "mtd tee partitions for spi-nand0"
default "512k(teeh),512k(teed),512k(teex)"
- depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP && STM32MP15x_STM32IMAGE
+ depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
help
This define the tee partitions added in mtparts dynamically
when tee is supported with boot from spi-nand0,
diff --git a/board/st/common/stm32mp_mtdparts.c b/board/st/common/stm32mp_mtdparts.c
index 8b636d62fa..dea5d506a1 100644
--- a/board/st/common/stm32mp_mtdparts.c
+++ b/board/st/common/stm32mp_mtdparts.c
@@ -11,9 +11,7 @@
#include <log.h>
#include <mtd.h>
#include <mtd_node.h>
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
#include <tee.h>
-#endif
#include <asm/arch/stm32prog.h>
#include <asm/arch/sys_proto.h>
#include <asm/global_data.h>
@@ -33,9 +31,7 @@ static void board_set_mtdparts(const char *dev,
char *mtdids,
char *mtdparts,
const char *boot,
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
const char *tee,
-#endif
const char *user)
{
/* mtdids: "<dev>=<dev>, ...." */
@@ -59,12 +55,10 @@ static void board_set_mtdparts(const char *dev,
strncat(mtdparts, ",", MTDPARTS_LEN);
}
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
if (tee) {
strncat(mtdparts, tee, MTDPARTS_LEN);
strncat(mtdparts, ",", MTDPARTS_LEN);
}
-#endif
strncat(mtdparts, user, MTDPARTS_LEN);
}
@@ -77,9 +71,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
static char ids[MTDIDS_LEN + 1];
static bool mtd_initialized;
bool nor, nand, spinand, serial;
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
bool tee = false;
-#endif
if (mtd_initialized) {
*mtdids = ids;
@@ -97,9 +89,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
case BOOT_SERIAL_USB:
serial = true;
if (CONFIG_IS_ENABLED(CMD_STM32PROG)) {
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
tee = stm32prog_get_tee_partitions();
-#endif
nor = stm32prog_get_fsbl_nor();
}
nand = true;
@@ -118,11 +108,9 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
break;
}
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
if (!serial && CONFIG_IS_ENABLED(OPTEE) &&
tee_find_device(NULL, NULL, NULL, NULL))
tee = true;
-#endif
memset(parts, 0, sizeof(parts));
memset(ids, 0, sizeof(ids));
@@ -139,9 +127,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
if (!IS_ERR_OR_NULL(mtd)) {
board_set_mtdparts("nand0", ids, parts,
CONFIG_MTDPARTS_NAND0_BOOT,
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
!nor && tee ? CONFIG_MTDPARTS_NAND0_TEE : NULL,
-#endif
"-(UBI)");
put_mtd_device(mtd);
}
@@ -152,9 +138,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
if (!IS_ERR_OR_NULL(mtd)) {
board_set_mtdparts("spi-nand0", ids, parts,
CONFIG_MTDPARTS_SPINAND0_BOOT,
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
!nor && tee ? CONFIG_MTDPARTS_SPINAND0_TEE : NULL,
-#endif
"-(UBI)");
put_mtd_device(mtd);
}
@@ -164,9 +148,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
if (!uclass_get_device(UCLASS_SPI_FLASH, 0, &dev)) {
board_set_mtdparts("nor0", ids, parts,
CONFIG_MTDPARTS_NOR0_BOOT,
-#ifdef CONFIG_STM32MP15x_STM32IMAGE
tee ? CONFIG_MTDPARTS_NOR0_TEE : NULL,
-#endif
"-(nor_user)");
}
}
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index 032f08d795..583f923d29 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -106,10 +106,10 @@ int checkboard(void)
int fdt_compat_len;
if (IS_ENABLED(CONFIG_TFABOOT)) {
- if (IS_ENABLED(CONFIG_STM32MP15x_STM32IMAGE))
- mode = "trusted - stm32image";
- else
+ if (IS_ENABLED(CONFIG_TFABOOT_FIP_CONTAINER))
mode = "trusted";
+ else
+ mode = "trusted - stm32image";
} else {
mode = "basic";
}
diff --git a/configs/stm32mp15_defconfig b/configs/stm32mp15_tfaboot_fip_defconfig
similarity index 99%
rename from configs/stm32mp15_defconfig
rename to configs/stm32mp15_tfaboot_fip_defconfig
index e725b916b9..f7ef511e9c 100644
--- a/configs/stm32mp15_defconfig
+++ b/configs/stm32mp15_tfaboot_fip_defconfig
@@ -1,6 +1,7 @@
CONFIG_ARM=y
CONFIG_ARCH_STM32MP=y
CONFIG_TFABOOT=y
+CONFIG_TFABOOT_FIP_CONTAINER=y
CONFIG_SYS_MALLOC_F_LEN=0x3000
CONFIG_SYS_MEMTEST_START=0xc0000000
CONFIG_SYS_MEMTEST_END=0xc4000000
diff --git a/configs/stm32mp15_trusted_defconfig b/configs/stm32mp15_trusted_defconfig
index 2e2f0c76ca..1671cb24f5 100644
--- a/configs/stm32mp15_trusted_defconfig
+++ b/configs/stm32mp15_trusted_defconfig
@@ -7,7 +7,6 @@ CONFIG_SYS_MEMTEST_END=0xc4000000
CONFIG_ENV_OFFSET=0x280000
CONFIG_ENV_SECT_SIZE=0x40000
CONFIG_DEFAULT_DEVICE_TREE="stm32mp157c-ev1"
-CONFIG_STM32MP15x_STM32IMAGE=y
CONFIG_TARGET_ST_STM32MP15x=y
CONFIG_CMD_STM32KEY=y
CONFIG_CMD_STM32PROG=y
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-08-26 21:47 ` [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP Alexandru Gagniuc
@ 2021-08-31 14:54 ` Patrick DELAUNAY
2021-08-31 16:42 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Patrick DELAUNAY @ 2021-08-31 14:54 UTC (permalink / raw)
To: Alexandru Gagniuc, u-boot; +Cc: marex, Etienne Carriere, Patrice CHOTARD
Hi Alexandru,
On 8/26/21 11:47 PM, Alexandru Gagniuc wrote:
> Hi Patrick,
>
> I proposing a better fix fir the issues I outlined earlier, I made a
> classification of the currently supported boot modes.
>
> 1) BL1 -> SPL -> U-Boot
> 2) BL1 -> SPL -> OP-TEE
> ---------------------------------------------------------------------
> | 3) BL1 -> TF-A -> U-Boot |
> | 4) BL1 -> TF-A -> OP-TEE |
> | _________________________________________________________________ |
> || 5) BL1 -> TF-A -> FIP container ||
> || CONFIG_TFABOOT_FIP ||
> ||_________________________________________________________________||
> | |
> | CONFIG_TFABOOT |
> ---------------------------------------------------------------------
>
> Here, I'm looking at FIP as a new boot mode. In order to avoid
> breakage, any changes to support FIP it should naturally be done only
> to this new path.
>
> This proposal contains several changes, but I've squashed them into
> one for ease of discussion.
>
> This better matches the boot mode classification above.
1) is supported but with many constraint for security part and low power
management
it is not recommended for real product / it will be not supported
by STMicroelectronics
2) is not supported by STMicroelectronics, introduced by your patchset
3 and 4) are common for U-Boot point of view: running in normal world,
STM32IMAGE container
BL1 -> TF-A -> SecureMonitor -> U-Boot (STM32IMAGE or FIP container)
With Secure Monitor
= SPMIN (included in a the STM32IMAGE container of TF-A)
or OP-TEE (loaded by TF-A BL2 in "tf-a.stm32", using the STM32IMAGE
container)
The STM32IMAGE container are correct for the BL1 load with first root of
trust but have limitations
for new security features (Public key infrastructure, secure software
update, M4 firmware load by TF-A)
or for next products (coming soon) with limited size of embedded RAM.
So we decide to use for future development/product the default TF-A
container = the FIP.
=> For the next products only we only support FIP container.
And the FIP containers is now the recommended TF-A container for
STM32MP15 platform
and we could decide one day to deprecate/remove the STM32IMAGE support
for STM32MP15 platform....
It is why I introduced the CONFIG_STM32MP15x_STM32IMAGE,
to keep the compatibility with the previous "trusted" boot chain
and the default container is/becomes the FIP for all STM32MP product
=> this STM32IMAGE support (under compilation flag) could be removed in
future
by choice for the maintenance I prefer to mark the code
to remove under a specific compilation flags only supported for
STM32MP15x
Today I have the classification:
1) BL1 -> SPL -> u-boot ("basic" boot)
---------------------------------------------------------------------
| 2) BL1 -> TF-A -> u-boot ("trusted" boot) |
| |
| 2.1) TF-A BL2 use STM32IMAGE container |
| (CONFIG_STM32MP15x_STM32IMAGE => deprecated) |
| |
| 2.2) TF-A BL2 use FIP container (new default) |
| |
| CONFIG_TFABOOT |
---------------------------------------------------------------------
The STM32IMAGE container support is only variant for STM32MP15x trusted boot.
FIP is a evolution of the STMicroelectronics proposal
and is now the default recommended container for trusted boot.
OP-TEE presence is detected dynamically in U-Boot (it should be
mandatory for next STM32MP product).
The root cause the of the issue is the introduction of
the "strange" OP-TEE after SPL boot mode (*)
1) BL1 -> SPL (secure) -> U-Boot (secure)
*) BL1 -> SPL (secure) -> OP-TEE (secure) -> kernel (normal world) for falcon mode
-> U-Boot (normal world)
it is strange for me, because SPL and U-Boot are not running at the same exception level,
and OP-TEE firmware is only present/running when U-Boot proper is running.
So all my assumption in STM32MP platform are false
(SPL support => U-Boot is running in secure world when TFABOOT is not present)
and not only the FIP support.
This mode is not recommended: it will not supported by STMicroelectronics
and not supported by next products.
But you are free to manage it.
This new boot scheme could be managed by a new defconfig....
to avoid to break the existing configuration (basic / trusted)
>
> This config mixes boot path (2) with paths (3) and (4), and thus is
> contrary to our goal of making changes only to the new FIP path.
> Because it mixes and matches SPL assumptions with TF-A assumptions,
> I've had a hard time figuting it out. I suspect it would be just as
> confusing for others in the future.
Yes I make the assumption that SPL is for "basic" boot
SPL loads the next stage in secure world because it the general use case
for SPL / U-Boot:
the switch to normal world is done AFTER U-Boot when the
secure monitor for PSCI suport is installed by U-Boot.
But in you use-case you mix the 2 boot schemes ("basic" for SPL and
"trusted" for U-boot)
For my point of view you can use
- SPL (u-boot-spl.stm32) from stm32mp15_basic_defconfig:
=> simple loader / running in secure world
- U-Boot (u-boot-nodtb.bin + u-boot.dtb = included in FIT) from
stm32mp15_trusted_defconfig:
=> U-Boot running in normal world / using OP-TEE
If you want manage the feature with a single defconfig it should be
difficult
to avoid to break the existing boot schemes.
>
> I've had issues with tee_find_device() in the past when using SPL as
> the FSBL. As u-boot was running in secure mode and did not have a
> handler, it would result in a CPU exception and crash.
yes it is normal for me: OP-TEE driver support should be only activated when
U-Boot is running in normal world, in "trusted" defconfig
tee_find_device() should not be called when OP-TEE driver is not
supported, in SPL or for "basic" boot.
PS: in the code the API call are protected by CONFIG_IS_ENABLED:
if (CONFIG_IS_ENABLED(OPTEE) && !tee_find_device(NULL, NULL, NULL,
NULL))
I thought that was enought because
the config CONFIG_OPTEE is used to activate the OP-TEE driver support in
U-Boot
So the CONFIG_SPL_OPTEE is used to activate the OP-TEE driver support in SPL
=> for me CONFIG_SPL_OPTEE should be never activated
but after check and I am confused with the CONFIG_OPTEE: it is defined 2
times with 2 meanings
and I think U-Boot have a issue here....
./lib/optee/Kconfig:
config OPTEE
bool "Support OPTEE images"
help
U-Boot can be configured to boot OPTEE images.
Selecting this option will enable shared OPTEE library code and
enable an OPTEE specific bootm command that will perform
additional
OPTEE specific checks before booting an OPTEE image created with
mkimage.
./drivers/tee/optee/Kconfig:
config OPTEE
bool "OP-TEE"
depends on ARM_SMCCC
help
This implements the OP-TEE Trusted Execution Environment (TEE)
driver. OP-TEE is a Trusted OS designed primarily to rely on the
ARM TrustZone(R) technology as the underlying hardware isolation
mechanism. This driver can request services from OP-TEE, but also
handle Remote Procedure Calls (RPC) from OP-TEE needed to
execute a service. For more information see: https://www.op-tee.org
/lib/Makefile:19:obj-$(CONFIG_OPTEE) += optee/
./drivers/tee/Makefile:7:obj-$(CONFIG_OPTEE) += optee/
I think one existing CONFIG_OPTEE should be renamed to a better name
"CONFIG_OPTEE_IMAGE" for "BOOT of OPTEE firmware"
./lib/optee/Kconfig:
- config OPTEE
+ config OPTEE_IMAGE
and the associated becomes
- config SPL_OPTEE
+ config SPL_OPTEE_IMAGE
bool "Support boot of OP-TEE Trusted OS"
depends on ARM
help
OP-TEE is an open source Trusted OS which is loaded by SPL.
More detail at: https://github.com/OP-TEE/optee_os
.....
- #if CONFIG_IS_ENABLED(OPTEE)
- #if CONFIG_IS_ENABLED(OPTEE_IMAGE)
case IH_OS_TEE:
debug("Jumping to U-Boot via OP-TEE\n");
spl_board_prepare_for_optee(spl_image.fdt_addr);
spl_optee_entry(NULL, NULL, spl_image.fdt_addr,
(void *)spl_image.entry_point);
break;
#endif
+ other part in the code
I think I will propose this patch to solve this issue
> The second argument against this is that stm32mp1 is the only platform
> to call tee_find_device() with the intent of detecting the presence of
> OP-TEE.
I introduce this check to avoid to duplicate the trusted defconfig
for OP-TEE /SPMIN support and I think the dynamic detection is more
flexible:
the same U-Boot can be used with SPMIN or OP-TEE as secure monitor
I don't see the problem here: it CONFIG_OPTEE is activated,
the OP-TEE driver is present and the function can be called
(at least in U-Boot, but see previous remark).
but perhaps some inline function in include/tee.h could be more elegant
when the CONFIG issue is solved....
#if CONFIG_IS_ENABLED(OPTEE)
struct udevice *tee_find_device(struct udevice *start,
int (*match)(struct tee_version_data *vers,
const void *data),
const void *data,
struct tee_version_data *vers);
#else
static inline struct udevice *tee_find_device(struct udevice *start,
int (*match)(struct tee_version_data *vers,
const void *data),
const void *data,
struct tee_version_data *vers) {
return NULL;
}
#endif
Your problem here is that you activate OPTEE in SPL= CONFIG_SPL_OPTEE
and this config is strange, the OP-TEE driver is not activated in SPL
but SPL boot OPTEE image.
> Have there been issues with not callinf this in the past, or was this
> more of a "seems nice to have" ?
>
> "stm32mp15_defconfig" implies that would be the correct configuraion
> for STM32MP1. New contributor chooses this config, tries to run SPL
> + u-boot, which is what u-boot user expects is the default. Things
> likely fail miserably. A lot of u-boot users don't know what FIP is.
> It's an extra concept that is not strictly necessary in u-boot.
For me, SPL is not the only configuration expected by U-Boot user
(sometime you have SPL , TPL, TF-A or proprietary first stage bootloader).
And yes "stm32mp15_defconfig" it the new STMicroelectronics configuration to use
for STMicroelectronics STM32MP15x boards and the recommended starting point
for new customer board support. And it doesn't use SPL.
TF-A and FIP are perhaps not necessary today for STM32MP15x,
but they will be mandatory in the next products (SPL boot will be no
more supported).
I try to document each defconfig in platform documentation (in
doc/board/st/)
https://u-boot.readthedocs.io/en/latest/board/st/stm32mp1.html#boot-sequences
And it is also explained in our WIKI:
https://wiki.st.com/stm32mpu/wiki/Boot_chain_overview
https://wiki.st.com/stm32mpu/wiki/TF-A_overview
but if the user don't read the SW documentation....
yes he can be confused.
>
> So I think this name is vague, as it doesn't really describe what is
> going on. If we change it to "stm32mp15_tfaboot_fip_defconfig", then
> it very accurately describes the boot scenario, and avoids the
> confusion above.
for next product STM32MP1X, and for ST boards, as only the TF-A with FIP
is supported I will use the simple name:
- stm32mpXX_defconfig
with for STM32MP15x:
- stm32mp15_defconfig => boot with TF-A with FIP (default)
only for stm32mp15 I keeps the previous defconfig only for compatibility
- stm32mp15_basic_defconfig => boot with SPL
- stm32mp15_trusted_defconfig => boot with TF-A without FIP support (deprecated)
PS: our Marketing team don't like "FIP" / "tfaboot" in config name...
my first proposal was stm32mp15_spl_defconfig /
stm32mp15_tfa_defconfig
instead of stm32mp15_basic_defconfig / stm32mp15_trusted_defconfig
but if you want support a other configuration / a other customization /
a other board, you can manage
your stm32mp15 defconfig with your boot configuration, for example
stm32mp15_spl_optee_defconfig
> We're setting CONFIG_MTDPARTS_xxx based on TFABOOT_FIP_CONTAINER now,
> so I don't think we need any ifdefs here. This part needs the most
> scrutiny, as I don't have a way to test if I've broken anything.
> ---
> arch/arm/Kconfig | 14 +++++++
> arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi | 7 ++--
> arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi | 9 ++---
> arch/arm/mach-stm32mp/Kconfig | 7 ----
> .../cmd_stm32prog/cmd_stm32prog.c | 2 -
> .../mach-stm32mp/cmd_stm32prog/stm32prog.c | 4 --
> .../mach-stm32mp/cmd_stm32prog/stm32prog.h | 2 -
> arch/arm/mach-stm32mp/config.mk | 2 +-
> arch/arm/mach-stm32mp/fdt.c | 37 -------------------
> .../arm/mach-stm32mp/include/mach/stm32prog.h | 2 -
> board/st/common/Kconfig | 20 +++++-----
> board/st/common/stm32mp_mtdparts.c | 18 ---------
> board/st/stm32mp1/stm32mp1.c | 6 +--
> ...config => stm32mp15_tfaboot_fip_defconfig} | 1 +
> configs/stm32mp15_trusted_defconfig | 1 -
> 15 files changed, 35 insertions(+), 97 deletions(-)
> rename configs/{stm32mp15_defconfig => stm32mp15_tfaboot_fip_defconfig} (99%)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d692139199..4c6f7ab3de 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1905,6 +1905,20 @@ config TFABOOT
> Enabling this option will make a U-Boot binary that is relying
> on other firmware layers to provide secure functionality.
>
> +config TFABOOT_FIP_CONTAINER
> + bool "Support for booting from TF-A inside a FIP container"
> + depends on TFABOOT
> + default n
default y ?
> + help
> + TF-A has its own container format, named FIP (not to be confused with
> + FIT). When u-boot is started from a FIP, it sometimes needs to make
> + different assumptions than it would with a non-FIP boot. Although
> + those could be resolved with dynamic devicetree patching, TF-A is
> + either can't patch devicetrees, or is unwilling to do so.
> + Enabling this option will tell u-boot platform code that it is okay
> + to assume U-Boot will be started from a FIP container, even if such
> + assumptions would break things in a more normal setting.
> +
As FIP container is the default container for TF-A.... no need to have
configuration for it.
I prefer to have a compilation flag for the ST specific container
= STM32IMAGE (it will be marked deprecated soon)
> config TI_SECURE_DEVICE
> bool "HS Device Type Support"
> depends on ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> diff --git a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> index 0101962ea5..54424e398f 100644
> --- a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> @@ -15,18 +15,18 @@
> config {
> u-boot,boot-led = "heartbeat";
> u-boot,error-led = "error";
> - u-boot,mmc-env-partition = "fip";
> + u-boot,mmc-env-partition = "fip1";
why it is "fip1" for dk1 and "fip" for ed1, in eMMC the GPT parttion is
named fip
fip1 is for NAND
> st,adc_usb_pd = <&adc1 18>, <&adc1 19>;
> st,fastboot-gpios = <&gpioa 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> st,stm32prog-gpios = <&gpioa 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> };
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> +#ifdef CONFIG_TFABOOT_FIP_CONTAINER
> config {
> u-boot,mmc-env-partition = "ssbl";
> };
> +#endif
I prefer the initial code => the default devicetree if for FIP support
on ST board because it is the ST Microelectronics recommandation
And I assure the backward compatibility with compilation flag.
=> it is more easy to compare the STM32MP15 device tree with next products.
NB: a other solution is to remove the "basic" / SPL support ....
as ST Microelectronics don't support it.
but it is not acceptable as it is already used by customers
>
> - /* only needed for boot with TF-A, witout FIP support */
> firmware {
> optee {
> compatible = "linaro,optee-tz";
> @@ -43,7 +43,6 @@
> u-boot,dm-spl;
> };
> };
> -#endif
>
> led {
> red {
> diff --git a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
> index 32777384c6..184774df58 100644
> --- a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
> @@ -15,17 +15,17 @@
> config {
> u-boot,boot-led = "heartbeat";
> u-boot,error-led = "error";
> - u-boot,mmc-env-partition = "fip";
> + u-boot,mmc-env-partition = "ssbl";
> st,fastboot-gpios = <&gpioa 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> st,stm32prog-gpios = <&gpioa 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> };
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> +#ifdef CONFIG_TFABOOT_FIP_CONTAINER
> config {
> - u-boot,mmc-env-partition = "ssbl";
> + u-boot,mmc-env-partition = "fip";
> };
> +#endif
>
> - /* only needed for boot with TF-A, witout FIP support */
> firmware {
> optee {
> compatible = "linaro,optee-tz";
> @@ -39,7 +39,6 @@
> no-map;
> };
> };
> -#endif
>
> led {
> red {
> diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig
> index 5d7eca649a..4c1eeef165 100644
> --- a/arch/arm/mach-stm32mp/Kconfig
> +++ b/arch/arm/mach-stm32mp/Kconfig
> @@ -56,13 +56,6 @@ config STM32MP15x
> dual core A7 for STM32MP157/3, monocore for STM32MP151
> target all the STMicroelectronics board with SOC STM32MP1 family
>
> -config STM32MP15x_STM32IMAGE
> - bool "Support STM32 image for generated U-Boot image"
> - depends on STM32MP15x && TFABOOT
> - help
> - Support of STM32 image generation for SOC STM32MP15x
> - for TF-A boot when FIP container is not used
> -
> choice
> prompt "STM32MP15x board select"
> optional
> diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
> index 41452b5a29..be53a52977 100644
> --- a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
> +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
> @@ -185,7 +185,6 @@ U_BOOT_CMD(stm32prog, 5, 0, do_stm32prog,
> " <size> = size of flashlayout (optional for image with STM32 header)\n"
> );
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> bool stm32prog_get_tee_partitions(void)
> {
> if (stm32prog_data)
> @@ -193,7 +192,6 @@ bool stm32prog_get_tee_partitions(void)
>
> return false;
> }
> -#endif
I prefer to have the code under compilation flag to prepare code removal
and STM32IMAGE will be no more supported
and also easy customer board creation (with FIP support) to detect part
of obsolete code.
>
> bool stm32prog_get_fsbl_nor(void)
> {
> diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
> index 3b6ca4e773..26fe8b654a 100644
> --- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
> +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c
> @@ -824,9 +824,7 @@ static int treat_partition_list(struct stm32prog_data *data)
> INIT_LIST_HEAD(&data->dev[j].part_list);
> }
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
#ifndef CONFIG_TFABOOT_FIP_CONTAINER ?
> data->tee_detected = false;
> -#endif
> data->fsbl_nor_detected = false;
> for (i = 0; i < data->part_nb; i++) {
> part = &data->part_array[i];
> @@ -880,12 +878,10 @@ static int treat_partition_list(struct stm32prog_data *data)
> /* fallthrough */
> case STM32PROG_NAND:
> case STM32PROG_SPI_NAND:
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
#ifndef CONFIG_TFABOOT_FIP_CONTAINER ?
> if (!data->tee_detected &&
> !strncmp(part->name, "tee", 3))
> data->tee_detected = true;
> break;
> -#endif
> default:
> break;
> }
> diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
> index 240c5c44bc..9d58cf0e2d 100644
> --- a/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
> +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h
> @@ -122,9 +122,7 @@ struct stm32prog_data {
> struct stm32prog_dev_t dev[STM32PROG_MAX_DEV]; /* array of device */
> int part_nb; /* nb of partition */
> struct stm32prog_part_t *part_array; /* array of partition */
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> bool tee_detected;
> -#endif
> bool fsbl_nor_detected;
>
> /* command internal information */
> diff --git a/arch/arm/mach-stm32mp/config.mk b/arch/arm/mach-stm32mp/config.mk
> index f7f5b77c41..c30bf482f7 100644
> --- a/arch/arm/mach-stm32mp/config.mk
> +++ b/arch/arm/mach-stm32mp/config.mk
> @@ -4,7 +4,7 @@
> #
>
> ifndef CONFIG_SPL
> -INPUTS-$(CONFIG_STM32MP15x_STM32IMAGE) += u-boot.stm32
> +INPUTS-y += u-boot.stm32
normally "u-boot.stm32" is not generated when FIP is expected to avoid
user confusion !
it is no more the case with your proposal
> else
> ifdef CONFIG_SPL_BUILD
> INPUTS-y += u-boot-spl.stm32
> diff --git a/arch/arm/mach-stm32mp/fdt.c b/arch/arm/mach-stm32mp/fdt.c
> index a19e954cf7..8c729e1f32 100644
> --- a/arch/arm/mach-stm32mp/fdt.c
> +++ b/arch/arm/mach-stm32mp/fdt.c
> @@ -224,30 +224,6 @@ static void stm32_fdt_disable(void *fdt, int offset, u32 addr,
> string, addr, name);
> }
>
> -static void stm32_fdt_disable_optee(void *blob)
> -{
> - int off, node;
> -
> - /* Delete "optee" firmware node */
> - off = fdt_node_offset_by_compatible(blob, -1, "linaro,optee-tz");
> - if (off >= 0 && fdtdec_get_is_enabled(blob, off))
> - fdt_del_node(blob, off);
> -
> - /* Delete "optee@..." reserved-memory node */
> - off = fdt_path_offset(blob, "/reserved-memory/");
> - if (off < 0)
> - return;
> - for (node = fdt_first_subnode(blob, off);
> - node >= 0;
> - node = fdt_next_subnode(blob, node)) {
> - if (strncmp(fdt_get_name(blob, node, NULL), "optee@", 6))
> - continue;
> -
> - if (fdt_del_node(blob, node))
> - printf("Failed to remove optee reserved-memory node\n");
> - }
> -}
> -
> /*
> * This function is called right before the kernel is booted. "blob" is the
> * device tree that will be passed to the kernel.
> @@ -332,18 +308,5 @@ int ft_system_setup(void *blob, struct bd_info *bd)
> "st,package", pkg, false);
> }
>
> - /*
> - * TEMP: remove OP-TEE nodes in kernel device tree
> - * copied from U-Boot device tree by optee_copy_fdt_nodes
> - * when OP-TEE is not detected (probe failed)
> - * these OP-TEE nodes are present in <board>-u-boot.dtsi
> - * under CONFIG_STM32MP15x_STM32IMAGE only for compatibility
> - * when FIP is not used by TF-A
> - */
> - if (CONFIG_IS_ENABLED(STM32MP15x_STM32IMAGE) &&
> - CONFIG_IS_ENABLED(OPTEE) &&
> - !tee_find_device(NULL, NULL, NULL, NULL))
> - stm32_fdt_disable_optee(blob);
> -
without this code you break the TF-A boot with SPMIN (the existing
upstreamed support)
> return ret;
> }
> diff --git a/arch/arm/mach-stm32mp/include/mach/stm32prog.h b/arch/arm/mach-stm32mp/include/mach/stm32prog.h
> index 99be4e1d65..c080b9cc42 100644
> --- a/arch/arm/mach-stm32mp/include/mach/stm32prog.h
> +++ b/arch/arm/mach-stm32mp/include/mach/stm32prog.h
> @@ -11,8 +11,6 @@ int stm32prog_read_medium_virt(struct dfu_entity *dfu, u64 offset,
> void *buf, long *len);
> int stm32prog_get_medium_size_virt(struct dfu_entity *dfu, u64 *size);
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> bool stm32prog_get_tee_partitions(void);
> -#endif
>
> bool stm32prog_get_fsbl_nor(void);
> diff --git a/board/st/common/Kconfig b/board/st/common/Kconfig
> index 2f57118bb2..0bb8e808aa 100644
> --- a/board/st/common/Kconfig
> +++ b/board/st/common/Kconfig
> @@ -8,9 +8,8 @@ config CMD_STBOARD
>
> config MTDPARTS_NAND0_BOOT
> string "mtd boot partitions for nand0"
> - default "2m(fsbl),2m(ssbl1),2m(ssbl2)" if STM32MP15x_STM32IMAGE || \
> - !TFABOOT
> - default "2m(fsbl),4m(fip1),4m(fip2)"
> + default "2m(fsbl),2m(ssbl1),2m(ssbl2)"
> + default "2m(fsbl),4m(fip1),4m(fip2)" if TFABOOT_FIP_CONTAINER
> depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
> help
> This define the partitions of nand0 used to build mtparts dynamically
> @@ -23,7 +22,7 @@ config MTDPARTS_NAND0_BOOT
> config MTDPARTS_NAND0_TEE
> string "mtd tee partitions for nand0"
> default "512k(teeh),512k(teed),512k(teex)"
> - depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP && STM32MP15x_STM32IMAGE
> + depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
NAK => the tee partitions are not present when FIP is used by TF-A /
this define should be not defined
> help
> This define the tee partitions added in mtparts dynamically
> when tee is supported with boot from nand0.
> @@ -32,9 +31,8 @@ config MTDPARTS_NAND0_TEE
>
> config MTDPARTS_NOR0_BOOT
> string "mtd boot partitions for nor0"
> - default "256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env)" if STM32MP15x_STM32IMAGE || \
> - !TFABOOT
> - default "256k(fsbl1),256k(fsbl2),4m(fip),512k(u-boot-env)"
> + default "256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env)"
> + default "256k(fsbl1),256k(fsbl2),4m(fip),512k(u-boot-env)" if TFABOOT_FIP_CONTAINER
> depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
> help
> This define the partitions of nand0 used to build mtparts dynamically
> @@ -46,15 +44,15 @@ config MTDPARTS_NOR0_BOOT
> config MTDPARTS_NOR0_TEE
> string "mtd tee partitions for nor0"
> default "256k(teeh),512k(teed),256k(teex)"
> - depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP && STM32MP15x_STM32IMAGE
> + depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
NAK => same
> help
> This define the tee partitions added in mtparts dynamically
> when tee is supported with boot from nor0.
>
> config MTDPARTS_SPINAND0_BOOT
> string "mtd boot partitions for spi-nand0"
> - default "2m(fsbl),2m(ssbl1),2m(ssbl2)" if STM32MP15x_STM32IMAGE || !TFABOOT
> - default "2m(fsbl),4m(fip1),4m(fip2)"
> + default "2m(fsbl),2m(ssbl1),2m(ssbl2)"
> + default "2m(fsbl),4m(fip1),4m(fip2)" if TFABOOT_FIP_CONTAINER
> depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
> help
> This define the partitions of nand0 used to build mtparts dynamically
> @@ -66,7 +64,7 @@ config MTDPARTS_SPINAND0_BOOT
> config MTDPARTS_SPINAND0_TEE
> string "mtd tee partitions for spi-nand0"
> default "512k(teeh),512k(teed),512k(teex)"
> - depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP && STM32MP15x_STM32IMAGE
> + depends on SYS_MTDPARTS_RUNTIME && ARCH_STM32MP
NAK => same
> help
> This define the tee partitions added in mtparts dynamically
> when tee is supported with boot from spi-nand0,
> diff --git a/board/st/common/stm32mp_mtdparts.c b/board/st/common/stm32mp_mtdparts.c
> index 8b636d62fa..dea5d506a1 100644
> --- a/board/st/common/stm32mp_mtdparts.c
> +++ b/board/st/common/stm32mp_mtdparts.c
> @@ -11,9 +11,7 @@
> #include <log.h>
> #include <mtd.h>
> #include <mtd_node.h>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> #include <tee.h>
> -#endif
> #include <asm/arch/stm32prog.h>
> #include <asm/arch/sys_proto.h>
> #include <asm/global_data.h>
> @@ -33,9 +31,7 @@ static void board_set_mtdparts(const char *dev,
> char *mtdids,
> char *mtdparts,
> const char *boot,
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> const char *tee,
> -#endif
> const char *user)
> {
> /* mtdids: "<dev>=<dev>, ...." */
> @@ -59,12 +55,10 @@ static void board_set_mtdparts(const char *dev,
> strncat(mtdparts, ",", MTDPARTS_LEN);
> }
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> if (tee) {
> strncat(mtdparts, tee, MTDPARTS_LEN);
> strncat(mtdparts, ",", MTDPARTS_LEN);
> }
> -#endif
>
> strncat(mtdparts, user, MTDPARTS_LEN);
> }
> @@ -77,9 +71,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
> static char ids[MTDIDS_LEN + 1];
> static bool mtd_initialized;
> bool nor, nand, spinand, serial;
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> bool tee = false;
> -#endif
>
> if (mtd_initialized) {
> *mtdids = ids;
> @@ -97,9 +89,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
> case BOOT_SERIAL_USB:
> serial = true;
> if (CONFIG_IS_ENABLED(CMD_STM32PROG)) {
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> tee = stm32prog_get_tee_partitions();
> -#endif
> nor = stm32prog_get_fsbl_nor();
> }
> nand = true;
> @@ -118,11 +108,9 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
> break;
> }
>
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> if (!serial && CONFIG_IS_ENABLED(OPTEE) &&
> tee_find_device(NULL, NULL, NULL, NULL))
> tee = true;
> -#endif
>
> memset(parts, 0, sizeof(parts));
> memset(ids, 0, sizeof(ids));
> @@ -139,9 +127,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
> if (!IS_ERR_OR_NULL(mtd)) {
> board_set_mtdparts("nand0", ids, parts,
> CONFIG_MTDPARTS_NAND0_BOOT,
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> !nor && tee ? CONFIG_MTDPARTS_NAND0_TEE : NULL,
> -#endif
> "-(UBI)");
> put_mtd_device(mtd);
> }
> @@ -152,9 +138,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
> if (!IS_ERR_OR_NULL(mtd)) {
> board_set_mtdparts("spi-nand0", ids, parts,
> CONFIG_MTDPARTS_SPINAND0_BOOT,
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> !nor && tee ? CONFIG_MTDPARTS_SPINAND0_TEE : NULL,
> -#endif
> "-(UBI)");
> put_mtd_device(mtd);
> }
> @@ -164,9 +148,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
> if (!uclass_get_device(UCLASS_SPI_FLASH, 0, &dev)) {
> board_set_mtdparts("nor0", ids, parts,
> CONFIG_MTDPARTS_NOR0_BOOT,
> -#ifdef CONFIG_STM32MP15x_STM32IMAGE
> tee ? CONFIG_MTDPARTS_NOR0_TEE : NULL,
> -#endif
> "-(nor_user)");
> }
> }
> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> index 032f08d795..583f923d29 100644
> --- a/board/st/stm32mp1/stm32mp1.c
> +++ b/board/st/stm32mp1/stm32mp1.c
> @@ -106,10 +106,10 @@ int checkboard(void)
> int fdt_compat_len;
>
> if (IS_ENABLED(CONFIG_TFABOOT)) {
> - if (IS_ENABLED(CONFIG_STM32MP15x_STM32IMAGE))
> - mode = "trusted - stm32image";
> - else
> + if (IS_ENABLED(CONFIG_TFABOOT_FIP_CONTAINER))
> mode = "trusted";
> + else
> + mode = "trusted - stm32image";
> } else {
> mode = "basic";
> }
> diff --git a/configs/stm32mp15_defconfig b/configs/stm32mp15_tfaboot_fip_defconfig
> similarity index 99%
> rename from configs/stm32mp15_defconfig
> rename to configs/stm32mp15_tfaboot_fip_defconfig
> index e725b916b9..f7ef511e9c 100644
> --- a/configs/stm32mp15_defconfig
> +++ b/configs/stm32mp15_tfaboot_fip_defconfig
> @@ -1,6 +1,7 @@
> CONFIG_ARM=y
> CONFIG_ARCH_STM32MP=y
> CONFIG_TFABOOT=y
> +CONFIG_TFABOOT_FIP_CONTAINER=y
> CONFIG_SYS_MALLOC_F_LEN=0x3000
> CONFIG_SYS_MEMTEST_START=0xc0000000
> CONFIG_SYS_MEMTEST_END=0xc4000000
> diff --git a/configs/stm32mp15_trusted_defconfig b/configs/stm32mp15_trusted_defconfig
> index 2e2f0c76ca..1671cb24f5 100644
> --- a/configs/stm32mp15_trusted_defconfig
> +++ b/configs/stm32mp15_trusted_defconfig
> @@ -7,7 +7,6 @@ CONFIG_SYS_MEMTEST_END=0xc4000000
> CONFIG_ENV_OFFSET=0x280000
> CONFIG_ENV_SECT_SIZE=0x40000
> CONFIG_DEFAULT_DEVICE_TREE="stm32mp157c-ev1"
> -CONFIG_STM32MP15x_STM32IMAGE=y
> CONFIG_TARGET_ST_STM32MP15x=y
> CONFIG_CMD_STM32KEY=y
> CONFIG_CMD_STM32PROG=y
For me you just inverse the logic for the config
CONFIG_STM32MP15x_STM32IMAGE => !CONFIG_TFABOOT_FIP_CONTAINER
I don't see the gain here, and this proposition will make more difficult
for me the introduction of the next product (in few months)
because it is based only on TF-A/FIP.
Sorry for the long answer, and sorry if my last update breaks
your OP-TEE load feature in SPL.
Patrick
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-08-31 14:54 ` Patrick DELAUNAY
@ 2021-08-31 16:42 ` Marek Vasut
2021-09-01 9:07 ` Patrick DELAUNAY
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2021-08-31 16:42 UTC (permalink / raw)
To: Patrick DELAUNAY, Alexandru Gagniuc, u-boot
Cc: Etienne Carriere, Patrice CHOTARD, Tom Rini
On 8/31/21 4:54 PM, Patrick DELAUNAY wrote:
> Hi Alexandru,
Hi,
> On 8/26/21 11:47 PM, Alexandru Gagniuc wrote:
>> Hi Patrick,
>>
>> I proposing a better fix fir the issues I outlined earlier, I made a
>> classification of the currently supported boot modes.
>>
>> 1) BL1 -> SPL -> U-Boot
>> 2) BL1 -> SPL -> OP-TEE
>> ---------------------------------------------------------------------
>> | 3) BL1 -> TF-A -> U-Boot |
>> | 4) BL1 -> TF-A -> OP-TEE |
>> | _________________________________________________________________ |
>> || 5) BL1 -> TF-A -> FIP container ||
>> || CONFIG_TFABOOT_FIP ||
>> ||_________________________________________________________________||
>> | |
>> | CONFIG_TFABOOT |
>> ---------------------------------------------------------------------
>>
>> Here, I'm looking at FIP as a new boot mode. In order to avoid
>> breakage, any changes to support FIP it should naturally be done only
>> to this new path.
>>
>> This proposal contains several changes, but I've squashed them into
>> one for ease of discussion.
>>
>> This better matches the boot mode classification above.
>
> 1) is supported but with many constraint for security part and low power
> management
>
> it is not recommended for real product / it will be not supported
> by STMicroelectronics
Does this mean ST will be cutting off their own customers who use this
boot mode because they do not need/want additional complex
problematically licensed components in their boot chain and/or ST will
be forcing those customers into adding such unneeded/unwanted components
unconditionally ?
I am strongly opposed to that.
I would argue that the U-Boot crypto code went through multiple
independent security reviews, personally I trust that more than code
fully controlled and maintained by any one single company, so I am not
buying the security constraint argument here.
Regarding power management and low power modes, there is literally
nothing preventing Linux from implementing those low power modes, so
there is no reason to hide all that code in firmware, so I am not buying
the low power argument either.
Finally, the argument that the component that is being forced upon
everyone is "open source" is really turning any design with such a SoC
into a huge risk.
There have been SoCs where the vendor took "open source" bootloader
code, compiled a blob, released a blob and never gave out the sources,
because it is "open source" and not "free software", the BSD license
permits such practice, GPL does not. Whoever wanted to design a board or
SoM with such a SoC, had to adjust their design to match that one blob.
Of course, that also implies that any security problems were not fixable
in that blob.
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-08-31 16:42 ` Marek Vasut
@ 2021-09-01 9:07 ` Patrick DELAUNAY
2021-09-03 15:32 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Patrick DELAUNAY @ 2021-09-01 9:07 UTC (permalink / raw)
To: Marek Vasut, Alexandru Gagniuc, u-boot, bernard.puel
Cc: Etienne Carriere, Patrice CHOTARD, Tom Rini, Lionel DEBIEVE
Hi Marek,
On 8/31/21 6:42 PM, Marek Vasut wrote:
> On 8/31/21 4:54 PM, Patrick DELAUNAY wrote:
>> Hi Alexandru,
>
> Hi,
>
>> On 8/26/21 11:47 PM, Alexandru Gagniuc wrote:
>>> Hi Patrick,
>>>
>>> I proposing a better fix fir the issues I outlined earlier, I made a
>>> classification of the currently supported boot modes.
>>>
>>> 1) BL1 -> SPL -> U-Boot
>>> 2) BL1 -> SPL -> OP-TEE
>>> ---------------------------------------------------------------------
>>> | 3) BL1 -> TF-A -> U-Boot |
>>> | 4) BL1 -> TF-A -> OP-TEE |
>>> | _________________________________________________________________ |
>>> || 5) BL1 -> TF-A -> FIP container ||
>>> || CONFIG_TFABOOT_FIP ||
>>> ||_________________________________________________________________||
>>> | |
>>> | CONFIG_TFABOOT |
>>> ---------------------------------------------------------------------
>>>
>>> Here, I'm looking at FIP as a new boot mode. In order to avoid
>>> breakage, any changes to support FIP it should naturally be done only
>>> to this new path.
>>>
>>> This proposal contains several changes, but I've squashed them into
>>> one for ease of discussion.
>>>
>>> This better matches the boot mode classification above.
>>
>> 1) is supported but with many constraint for security part and low
>> power management
>>
>> it is not recommended for real product / it will be not
>> supported by STMicroelectronics
>
> Does this mean ST will be cutting off their own customers who use this
> boot mode because they do not need/want additional complex
> problematically licensed components in their boot chain and/or ST will
> be forcing those customers into adding such unneeded/unwanted
> components unconditionally ?
>
> I am strongly opposed to that.
>
Our concerns is not a licensing issue (BSD vs GPL) but firmware
complexity to manage Cortex A (Linux) + Cortex M (RTOS) and shared
ressources.
To guarantee the Cortex M security, the shared resource (as root clock
or regulator for example) need to be managed by a centralized element.
It is the same for low power mode = to manage each possible step for
Cortex M + Cortex A and PMIC security, the low power sequence are
executed in secure side.
=> it is done in our ecosystem for STM32MP15x platform in secure monitor
(OP-TEE - the preferred one - or SPMin) based on SCMI and PSCI api
called by Linux kernel generic driver.
These API are NOT supported by SPL / U-Boot, it is the reason of the
restrictions = I have only implement a limited PSCI support to start the
kernel
You can continue to use SPL on STM32MP15 (at the current upstream level)
but with limited features.
but only the boot with TF-A is supported / promoted by
STMicroelectronics on the STM32MP family.
These restriction are already announced to STMicroelectronics customers
since OpenSTLinux v1.2 I think.
For example in
https://wiki.st.com/stm32mpu-ecosystem-v2/wiki/STM32MP15_ecosystem_release_note_-_v2.1.0
Basic boot has been removed since STM32MP15-ecosystem-v2.0.0,
if you were using basic boot with U-BOOT-SPL to load U-BOOT and
the Kernel,
then you need to use now the ST reference boot scheme in replacing
U-BOOT-SPL by TF-A
as FSBL as explained in Boot chain overview.
> I would argue that the U-Boot crypto code went through multiple
> independent security reviews, personally I trust that more than code
> fully controlled and maintained by any one single company, so I am not
> buying the security constraint argument here.
When I talk about security, it is not crypto code, but some security
features as isolation between cortex A and Cortex M, key infractructure,
trusted environment execution, secure update.
> Regarding power management and low power modes, there is literally
> nothing preventing Linux from implementing those low power modes, so
> there is no reason to hide all that code in firmware, so I am not
> buying the low power argument either.
Some part is already done in Linux with call SCMI / PSCI api.
Some part is done in secured firmware: DDR self refresh entry sequence /
root clock deactivation / PMIC access on secured I2C.
>
> Finally, the argument that the component that is being forced upon
> everyone is "open source" is really turning any design with such a SoC
> into a huge risk.
>
> There have been SoCs where the vendor took "open source" bootloader
> code, compiled a blob, released a blob and never gave out the sources,
> because it is "open source" and not "free software", the BSD license
> permits such practice, GPL does not. Whoever wanted to design a board
> or SoM with such a SoC, had to adjust their design to match that one
> blob. Of course, that also implies that any security problems were not
> fixable in that blob.
>
TF-A in OpenSTLinux is delivered as source, you can recompile it => no
blob delivery for STMicroelectronics
https://github.com/STMicroelectronics/arm-trusted-firmware
And we upstream all the TF-A developments (work in progress).
For us, OpenSTLinux is not only "open source" but "free software"....
and customers (SoM maker or other) manage their distribution.
> [...]
Regards.
Patrick
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-09-01 9:07 ` Patrick DELAUNAY
@ 2021-09-03 15:32 ` Marek Vasut
2021-09-03 17:47 ` Alex G.
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2021-09-03 15:32 UTC (permalink / raw)
To: Patrick DELAUNAY, Alexandru Gagniuc, u-boot, bernard.puel
Cc: Etienne Carriere, Patrice CHOTARD, Tom Rini, Lionel DEBIEVE
On 9/1/21 11:07 AM, Patrick DELAUNAY wrote:
> Hi Marek,
Hi,
> On 8/31/21 6:42 PM, Marek Vasut wrote:
>> On 8/31/21 4:54 PM, Patrick DELAUNAY wrote:
>>> Hi Alexandru,
>>
>> Hi,
>>
>>> On 8/26/21 11:47 PM, Alexandru Gagniuc wrote:
>>>> Hi Patrick,
>>>>
>>>> I proposing a better fix fir the issues I outlined earlier, I made a
>>>> classification of the currently supported boot modes.
>>>>
>>>> 1) BL1 -> SPL -> U-Boot
>>>> 2) BL1 -> SPL -> OP-TEE
>>>> ---------------------------------------------------------------------
>>>> | 3) BL1 -> TF-A -> U-Boot |
>>>> | 4) BL1 -> TF-A -> OP-TEE |
>>>> | _________________________________________________________________ |
>>>> || 5) BL1 -> TF-A -> FIP container ||
>>>> || CONFIG_TFABOOT_FIP ||
>>>> ||_________________________________________________________________||
>>>> | |
>>>> | CONFIG_TFABOOT |
>>>> ---------------------------------------------------------------------
>>>>
>>>> Here, I'm looking at FIP as a new boot mode. In order to avoid
>>>> breakage, any changes to support FIP it should naturally be done only
>>>> to this new path.
>>>>
>>>> This proposal contains several changes, but I've squashed them into
>>>> one for ease of discussion.
>>>>
>>>> This better matches the boot mode classification above.
>>>
>>> 1) is supported but with many constraint for security part and low
>>> power management
>>>
>>> it is not recommended for real product / it will be not
>>> supported by STMicroelectronics
>>
>> Does this mean ST will be cutting off their own customers who use this
>> boot mode because they do not need/want additional complex
>> problematically licensed components in their boot chain and/or ST will
>> be forcing those customers into adding such unneeded/unwanted
>> components unconditionally ?
>>
>> I am strongly opposed to that.
>>
>
> Our concerns is not a licensing issue (BSD vs GPL) but firmware
> complexity to manage Cortex A (Linux) + Cortex M (RTOS) and shared
> ressources.
By adding adding another component into the boot chain, the complexity
increases.
> To guarantee the Cortex M security, the shared resource (as root clock
> or regulator for example) need to be managed by a centralized element.
How come Linux cannot manage those resources, like on other vendors'
SoCs, where Linux can do that just fine (namely e.g. iMX6SX, which is
very similar to the MP1) ?
> It is the same for low power mode = to manage each possible step for
> Cortex M + Cortex A and PMIC security, the low power sequence are
> executed in secure side.
Linux is capable of managing both the PMIC and the CM4, what exactly is
the problem ?
> => it is done in our ecosystem for STM32MP15x platform in secure monitor
> (OP-TEE - the preferred one - or SPMin) based on SCMI and PSCI api
> called by Linux kernel generic driver.
Neither of these APIs are mandatory on ARMv7a , so why force them onto
users if Linux kernel can do the same without this extra complexity and
extra code ?
> These API are NOT supported by SPL / U-Boot, it is the reason of the
> restrictions = I have only implement a limited PSCI support to start the
> kernel
U-Boot does have PSCI support, so why not just extend that code which
you already have as part of the boot chain if you have to have PSCI
support (which is optional on armv7a and definitely not needed to boot
the kernel on armv7a) ?
If you were to do the above, you would have one bootloader code base
under one license, not two disparate code bases with separate set of
bugs and disparate licenses.
> You can continue to use SPL on STM32MP15 (at the current upstream level)
> but with limited features.
> but only the boot with TF-A is supported / promoted by
What exactly would I be missing that cannot be done with U-Boot / SPL ?
This statement above is extremely vague and makes it look as if adding
extra complexity was the only way, however so far I do not see anything
that I would be missing on the MP1 with SPL.
> STMicroelectronics on the STM32MP family.
>
>
> These restriction are already announced to STMicroelectronics customers
> since OpenSTLinux v1.2 I think.
>
>
> For example in
> https://wiki.st.com/stm32mpu-ecosystem-v2/wiki/STM32MP15_ecosystem_release_note_-_v2.1.0
>
>
> Basic boot has been removed since STM32MP15-ecosystem-v2.0.0,
>
> if you were using basic boot with U-BOOT-SPL to load U-BOOT and
> the Kernel,
>
> then you need to use now the ST reference boot scheme in replacing
> U-BOOT-SPL by TF-A
>
> as FSBL as explained in Boot chain overview.
This is not relevant here, this is downstream vendor BSP which is easy
to ignore, including whatever policy it tries to enforce onto its consumers.
However, if this is a policy that ST is trying to enforce outside of the
BSP and even on upstream users, then there has to be a justification for
it, with clear pros and cons. So far, I only see the cons -- complexity,
maintenance burden, duplication of effort, increased boot time, need to
rework boot chain on existing systems, and I can go on -- I do not see
any benefit this change would bring me, sorry.
>> I would argue that the U-Boot crypto code went through multiple >> independent security reviews, personally I trust that more than code
>> fully controlled and maintained by any one single company, so I am not
>> buying the security constraint argument here.
>
>
> When I talk about security, it is not crypto code, but some security
> features as isolation between cortex A and Cortex M, key infractructure,
> trusted environment execution, secure update.
I'll offload this answer to Alexandru, however as far as I can tell, SPL
is perfectly capable of starting the TEE and setting up the correct
execution levels etc. too.
>> Regarding power management and low power modes, there is literally
>> nothing preventing Linux from implementing those low power modes, so
>> there is no reason to hide all that code in firmware, so I am not
>> buying the low power argument either.
>
>
> Some part is already done in Linux with call SCMI / PSCI api.
>
> Some part is done in secured firmware: DDR self refresh entry sequence /
> root clock deactivation / PMIC access on secured I2C.
All of this can be done perfectly well in Linux as well, so why hide
this into difficult to inspect firmware ?
>> Finally, the argument that the component that is being forced upon
>> everyone is "open source" is really turning any design with such a SoC
>> into a huge risk.
>>
>> There have been SoCs where the vendor took "open source" bootloader
>> code, compiled a blob, released a blob and never gave out the sources,
>> because it is "open source" and not "free software", the BSD license
>> permits such practice, GPL does not. Whoever wanted to design a board
>> or SoM with such a SoC, had to adjust their design to match that one
>> blob. Of course, that also implies that any security problems were not
>> fixable in that blob.
>>
>
> TF-A in OpenSTLinux is delivered as source, you can recompile it => no
> blob delivery for STMicroelectronics
Yes, it is open source software that is _currently_ released also in
source form. However, that also means that if someone gives me a
pre-compiled blob of this software at any time, I have no legal way of
getting the matching sources, they can just tell me off, because that is
exactly what the permissive license allows them to do. And it also means
that if sometimes in the future this will no longer be released in
source form, I have a maintenance problem with my existing systems.
There is zero guarantee that neither of these scenarios will happen, and
I cannot just hope it won't happen.
With U-Boot, which is GPLv2, I do have that guarantee -- if I have a
blob, I have legal right to matching sources.
> https://github.com/STMicroelectronics/arm-trusted-firmware
>
> And we upstream all the TF-A developments (work in progress).
>
>
> For us, OpenSTLinux is not only "open source" but "free software"....
These two terms mean two very different things, please do not conflate them.
> and customers (SoM maker or other) manage their distribution.
I hope we can keep it that way, esp. in terms of not making optional
software components look mandatory.
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-09-03 15:32 ` Marek Vasut
@ 2021-09-03 17:47 ` Alex G.
2021-09-06 6:16 ` Etienne Carriere
0 siblings, 1 reply; 8+ messages in thread
From: Alex G. @ 2021-09-03 17:47 UTC (permalink / raw)
To: Marek Vasut, Patrick DELAUNAY, u-boot, bernard.puel
Cc: Etienne Carriere, Patrice CHOTARD, Tom Rini, Lionel DEBIEVE
On 9/3/21 10:32 AM, Marek Vasut wrote:
> On 9/1/21 11:07 AM, Patrick DELAUNAY wrote:
>> On 8/31/21 6:42 PM, Marek Vasut wrote:
>>> I would argue that the U-Boot crypto code went through multiple >>
>>> independent security reviews, personally I trust that more than code
>>> fully controlled and maintained by any one single company, so I am
>>> not buying the security constraint argument here.
>>
>>
>> When I talk about security, it is not crypto code, but some security
>> features as isolation between cortex A and Cortex M, key
>> infractructure, trusted environment execution, secure update.
>
> I'll offload this answer to Alexandru, however as far as I can tell, SPL
> is perfectly capable of starting the TEE and setting up the correct
> execution levels etc. too.
There is not a unisex size for security. It's all driven by
requirements. My client wants certain pieces of code to be
triple-encrypted with a full-body latex suit (and kevlar reinforced). I
don't care about A and M separation, clock control and other fancies. In
fact it's better that those are handled by linux because that helps with
boot time, considerably.
I can achieve that by ECDSA-signing the FSBL, and putting that critical
code in TEE trusted applications. I don't need to TiVO-ize the device,
lock the kernel, etc. Because of that, can incorporate GPL-v3 programs
and libraries, and generally have more flexibility in the software
architecture.
I'm more than happy to deal with the TZC and ETZPC in SPL, because SPL
is fast. Again, this is really driven by the totality of requirements.
TF-A is a pretty poor solution in this case for a trio of reasons. When
I was trying TF-A, I found a small bug in assembly code. I tried to
submit a fix, which was immediately rejected. I was required to sign a
CLA granting patent rights. I can't do that legally on behalf of my
client. This puts TF-A on a take it or leave it basis, such that I can't
fix problems with it.
TF-A also requires me to use a new file format (FIP). It's a binary
format which is already a huge red flag for me. It seems very similar to
a concatenation of EFI HOBs and FFS, and it makes no sense why TF-A
would not have just gone with those more established formats. Is it
secure? I don't know. It's pretty new to start with, and much more
inflexible than FIT. Why would I recommend to my client an unproven
format, when FIT has already gone through several CVEs and is more
widely supported? Definitely not.
Then TF-A is one extra codebase in the secure path of the system. Do I
want to have to deal with an extra program bringing its own possible
flaws on holes? Not really.
The official STM wiki seems to document the "how" of security: "Use TF-A
and TEE, all others unsupported". It doesn't explain the "why". The
"how" is what my client complained it takes two minutes to boot. And
those two minutes are the reason I'm working on this.
It would help to have better explained options with their corresponding
security implications. Even absent that, SPL is perfectly capable of
starting a secure system. I think it's also more flexible.
Let's get serious. Without SPL, I wouldn't have been able to boot linux
in one second, with a secure OS.
Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP
2021-09-03 17:47 ` Alex G.
@ 2021-09-06 6:16 ` Etienne Carriere
0 siblings, 0 replies; 8+ messages in thread
From: Etienne Carriere @ 2021-09-06 6:16 UTC (permalink / raw)
To: Alex G.
Cc: Marek Vasut, Patrick DELAUNAY, U-Boot Mailing List, bernard.puel,
Patrice CHOTARD, Tom Rini, Lionel DEBIEVE
Hello Alex,
Thanks for sharing your thoughts.
On Fri, 3 Sept 2021 at 19:47, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> On 9/3/21 10:32 AM, Marek Vasut wrote:
> > On 9/1/21 11:07 AM, Patrick DELAUNAY wrote:
> >> On 8/31/21 6:42 PM, Marek Vasut wrote:
>
> >>> I would argue that the U-Boot crypto code went through multiple >>
> >>> independent security reviews, personally I trust that more than code
> >>> fully controlled and maintained by any one single company, so I am
> >>> not buying the security constraint argument here.
> >>
> >>
> >> When I talk about security, it is not crypto code, but some security
> >> features as isolation between cortex A and Cortex M, key
> >> infractructure, trusted environment execution, secure update.
> >
> > I'll offload this answer to Alexandru, however as far as I can tell, SPL
> > is perfectly capable of starting the TEE and setting up the correct
> > execution levels etc. too.
>
> There is not a unisex size for security. It's all driven by
> requirements.
I fully agree.
> My client wants certain pieces of code to be
> triple-encrypted with a full-body latex suit (and kevlar reinforced). I
> don't care about A and M separation, clock control and other fancies. In
> fact it's better that those are handled by linux because that helps with
> boot time, considerably.
>
> I can achieve that by ECDSA-signing the FSBL, and putting that critical
> code in TEE trusted applications. I don't need to TiVO-ize the device,
> lock the kernel, etc. Because of that, can incorporate GPL-v3 programs
> and libraries, and generally have more flexibility in the software
> architecture.
TF-A sources are available on the web. You can fetch them, and even
incorporate GPL source, provided you comply with the terms.
I must admit that upstream changes incorporating GPL code will likely
be quite discussed.
>
> I'm more than happy to deal with the TZC and ETZPC in SPL, because SPL
> is fast. Again, this is really driven by the totality of requirements.
>
> TF-A is a pretty poor solution in this case for a trio of reasons. When
> I was trying TF-A, I found a small bug in assembly code. I tried to
> submit a fix, which was immediately rejected. I was required to sign a
> CLA granting patent rights. I can't do that legally on behalf of my
> client. This puts TF-A on a take it or leave it basis, such that I can't
True, at the time it was not handy to contribute. TF-A now relies on
Developer's Certificate of Origin 1.1.
So anyone can post a change proposal. It's true that the review
process is less flexible than the well established U-Boot ML.
> fix problems with it.
>
> TF-A also requires me to use a new file format (FIP). It's a binary
> format which is already a huge red flag for me. It seems very similar to
> a concatenation of EFI HOBs and FFS, and it makes no sense why TF-A
> would not have just gone with those more established formats. Is it
> secure? I don't know. It's pretty new to start with, and much more
> inflexible than FIT. Why would I recommend to my client an unproven
> format, when FIT has already gone through several CVEs and is more
> widely supported? Definitely not.
>
> Then TF-A is one extra codebase in the secure path of the system. Do I
> want to have to deal with an extra program bringing its own possible
> flaws on holes? Not really.
On 64bit, the system needs TF-A as secure monitor (BL31) so TF-A
package is already part of the system build for these.
TF-A also boots platforms that do not use U-Boot in the boot sequence.
For them, adding SPL would be adding a extra codebase.
>
> The official STM wiki seems to document the "how" of security: "Use TF-A
> and TEE, all others unsupported". It doesn't explain the "why". The
> "how" is what my client complained it takes two minutes to boot. And
> those two minutes are the reason I'm working on this.
>
> It would help to have better explained options with their corresponding
> security implications. Even absent that, SPL is perfectly capable of
> starting a secure system. I think it's also more flexible.
I agree SPL could be used in place of TF-A BL2, if one prefers this
one over that one.
SPL would need to pass a few boot information to secure world firmware
image (TF-A BL31), as those TF-A BL2 passes to it's BL31 stage.
as those SPL passes the U-Boot proper :)
regards,
etienne
>
> Let's get serious. Without SPL, I wouldn't have been able to boot linux
> in one second, with a secure OS.
>
> Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-06 6:16 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 20:30 Massive stm32mp1 breakage with v2021.10-rc2 Alex G.
2021-08-26 21:47 ` [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP Alexandru Gagniuc
2021-08-31 14:54 ` Patrick DELAUNAY
2021-08-31 16:42 ` Marek Vasut
2021-09-01 9:07 ` Patrick DELAUNAY
2021-09-03 15:32 ` Marek Vasut
2021-09-03 17:47 ` Alex G.
2021-09-06 6:16 ` Etienne Carriere
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.