[PATCH] imx8m: soc: cope with existing optee node

[PATCH] imx8m: soc: cope with existing optee node

[Vincent Stehlé]

On i.MX8M SoCs, the /firmware/optee Devicetree node is created just before
booting the OS when OP-TEE is found running. If the node already exists,
this results in an error, which prevents the OS to boot:

  Could not create optee node.
  ERROR: system-specific fdt fixup failed: FDT_ERR_EXISTS
   - must RESET the board to recover.

  failed to process device tree

On the i.MX8M systems where CONFIG_OF_SYSTEM_SETUP is defined, the
ft_add_optee_node() function is called before booting the OS. It will
create the OP-TEE Devicetree node and populate it with reserved memory
informations gathered at runtime.

On on most i.MX8M systems the Devicetree is built with an optee node if
CONFIG_OPTEE is defined. This node is indeed necessary for commands and
drivers communicating with OP-TEE, even before attempting OS boot.

The aforementioned issue can happen on the Compulab IOT-GATE-iMX8, which is
the only in-tree i.MX8M system where both CONFIG_OPTEE and
CONFIG_OF_SYSTEM_SETUP are defined (see the imx8mm-cl-iot-gate*
defconfigs).

Deal with an existing optee node gracefully at runtime to fix this issue.

Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/mach-imx/imx8m/soc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 85dc8b51a14..567e8e9e81a 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -1270,8 +1270,9 @@ static int ft_add_optee_node(void *fdt, struct bd_info *bd)
 		}
 	}
 
+	/* Locate the optee node if it exists or create it. */
 	subpath = "optee";
-	offs = fdt_add_subnode(fdt, offs, subpath);
+	offs = fdt_find_or_add_subnode(fdt, offs, subpath);
 	if (offs < 0) {
 		printf("Could not create %s node.\n", subpath);
 		return offs;
-- 
2.47.2

RE: [PATCH] imx8m: soc: cope with existing optee node

[Peng Fan]

> Subject: [PATCH] imx8m: soc: cope with existing optee node
> 
> On i.MX8M SoCs, the /firmware/optee Devicetree node is created just
> before booting the OS when OP-TEE is found running. If the node
> already exists, this results in an error, which prevents the OS to boot:
> 
>   Could not create optee node.
>   ERROR: system-specific fdt fixup failed: FDT_ERR_EXISTS
>    - must RESET the board to recover.
> 
>   failed to process device tree
> 
> On the i.MX8M systems where CONFIG_OF_SYSTEM_SETUP is defined,
> the
> ft_add_optee_node() function is called before booting the OS. It will
> create the OP-TEE Devicetree node and populate it with reserved
> memory informations gathered at runtime.
> 
> On on most i.MX8M systems the Devicetree is built with an optee node
> if CONFIG_OPTEE is defined. This node is indeed necessary for
> commands and drivers communicating with OP-TEE, even before
> attempting OS boot.
> 
> The aforementioned issue can happen on the Compulab IOT-GATE-
> iMX8, which is the only in-tree i.MX8M system where both
> CONFIG_OPTEE and CONFIG_OF_SYSTEM_SETUP are defined (see the
> imx8mm-cl-iot-gate* defconfigs).
> 
> Deal with an existing optee node gracefully at runtime to fix this issue.
> 
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Tim Harvey <tharvey@gateworks.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH] imx8m: soc: cope with existing optee node

[Fabio Estevam]

Hi Vincent,

On Mon, Mar 10, 2025 at 9:36 AM Vincent Stehlé <vincent.stehle@arm.com> wrote:

> --- a/arch/arm/mach-imx/imx8m/soc.c
> +++ b/arch/arm/mach-imx/imx8m/soc.c
> @@ -1270,8 +1270,9 @@ static int ft_add_optee_node(void *fdt, struct bd_info *bd)
>                 }
>         }
>
> +       /* Locate the optee node if it exists or create it. */
>         subpath = "optee";
> -       offs = fdt_add_subnode(fdt, offs, subpath);
> +       offs = fdt_find_or_add_subnode(fdt, offs, subpath);
>         if (offs < 0) {
>                 printf("Could not create %s node.\n", subpath);
>                 return offs;

This looks correct.

However, shouldn't we add the optee node only when CONFIG_OPTEE is selected?

--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -1235,6 +1235,9 @@ static int ft_add_optee_node(void *fdt, struct
bd_info *bd)
        int offs;
        int ret;

+       if (!IS_ENABLED(CONFIG_OPTEE))
+               return 0;
+
        /*
         * No TEE space allocated indicating no TEE running, so no
         * need to add optee node in dts

RE: [PATCH] imx8m: soc: cope with existing optee node

[Peng Fan]

> Subject: Re: [PATCH] imx8m: soc: cope with existing optee node
> 
> Hi Vincent,
> 
> On Mon, Mar 10, 2025 at 9:36 AM Vincent Stehlé
> <vincent.stehle@arm.com> wrote:
> 
> > --- a/arch/arm/mach-imx/imx8m/soc.c
> > +++ b/arch/arm/mach-imx/imx8m/soc.c
> > @@ -1270,8 +1270,9 @@ static int ft_add_optee_node(void *fdt,
> struct bd_info *bd)
> >                 }
> >         }
> >
> > +       /* Locate the optee node if it exists or create it. */
> >         subpath = "optee";
> > -       offs = fdt_add_subnode(fdt, offs, subpath);
> > +       offs = fdt_find_or_add_subnode(fdt, offs, subpath);
> >         if (offs < 0) {
> >                 printf("Could not create %s node.\n", subpath);
> >                 return offs;
> 
> This looks correct.
> 
> However, shouldn't we add the optee node only when CONFIG_OPTEE
> is selected?

Partially right.
But The issue here is to handle the case that the node
is created by optee os and uboot should bypass creating it again.

Regards,
Peng

> 
> --- a/arch/arm/mach-imx/imx8m/soc.c
> +++ b/arch/arm/mach-imx/imx8m/soc.c
> @@ -1235,6 +1235,9 @@ static int ft_add_optee_node(void *fdt,
> struct bd_info *bd)
>         int offs;
>         int ret;
> 
> +       if (!IS_ENABLED(CONFIG_OPTEE))
> +               return 0;
> +
>         /*
>          * No TEE space allocated indicating no TEE running, so no
>          * need to add optee node in dts

Re: [PATCH] imx8m: soc: cope with existing optee node

[Fabio Estevam]

On Wed, Mar 12, 2025 at 12:10 PM Peng Fan <peng.fan@nxp.com> wrote:

> Partially right.
> But The issue here is to handle the case that the node
> is created by optee os and uboot should bypass creating it again.

Yes, ths patch is correct.

My question is if we also need to pass

  if (!IS_ENABLED(CONFIG_OPTEE))
      return 0;

As a separate patch.

RE: [PATCH] imx8m: soc: cope with existing optee node

[Peng Fan]

> Subject: Re: [PATCH] imx8m: soc: cope with existing optee node
> 
> On Wed, Mar 12, 2025 at 12:10 PM Peng Fan <peng.fan@nxp.com>
> wrote:
> 
> > Partially right.
> > But The issue here is to handle the case that the node is created by
> > optee os and uboot should bypass creating it again.
> 
> Yes, ths patch is correct.
> 
> My question is if we also need to pass
> 
>   if (!IS_ENABLED(CONFIG_OPTEE))
>       return 0;
> 
> As a separate patch.

I think no, CONFIG_OPTEE is for U-Boot to use OP-TEE feature.
But here the node creation is for linux kernel to use OP-TEE.

Thanks,
Peng.

Re: [PATCH] imx8m: soc: cope with existing optee node

[Vincent Stehlé]

On Wed, Mar 12, 2025 at 11:12:35AM -0300, Fabio Estevam wrote:
> Hi Vincent,
> 
> On Mon, Mar 10, 2025 at 9:36 AM Vincent Stehlé <vincent.stehle@arm.com> wrote:
> 
> > --- a/arch/arm/mach-imx/imx8m/soc.c
> > +++ b/arch/arm/mach-imx/imx8m/soc.c
> > @@ -1270,8 +1270,9 @@ static int ft_add_optee_node(void *fdt, struct bd_info *bd)
> >                 }
> >         }
> >
> > +       /* Locate the optee node if it exists or create it. */
> >         subpath = "optee";
> > -       offs = fdt_add_subnode(fdt, offs, subpath);
> > +       offs = fdt_find_or_add_subnode(fdt, offs, subpath);
> >         if (offs < 0) {
> >                 printf("Could not create %s node.\n", subpath);
> >                 return offs;
> 
> This looks correct.

Hi Fabio,

Thank you for the review.

> 
> However, shouldn't we add the optee node only when CONFIG_OPTEE is selected?

Peng has answered this question already, but I would like to add some more
details:

Currently the ft_add_optee_node() function does add the /reserved-memory and
/firmware/optee nodes to the Devicetree passed to Linux when OP-TEE is detected
at runtime (the information is in the rom_pointer[] array).
If we skip this when CONFIG_OPTEE is not set, Linux will not even have the
reserved memory information anymore and the Linux optee driver will not probe.

Best regards,
Vincent.

> 
> --- a/arch/arm/mach-imx/imx8m/soc.c
> +++ b/arch/arm/mach-imx/imx8m/soc.c
> @@ -1235,6 +1235,9 @@ static int ft_add_optee_node(void *fdt, struct
> bd_info *bd)
>         int offs;
>         int ret;
> 
> +       if (!IS_ENABLED(CONFIG_OPTEE))
> +               return 0;
> +
>         /*
>          * No TEE space allocated indicating no TEE running, so no
>          * need to add optee node in dts

Re: [PATCH] imx8m: soc: cope with existing optee node

[Rasmus Villemoes]

On Mon, Mar 10 2025, Vincent Stehlé <vincent.stehle@arm.com> wrote:

> On i.MX8M SoCs, the /firmware/optee Devicetree node is created just before
> booting the OS when OP-TEE is found running. If the node already exists,
> this results in an error, which prevents the OS to boot:
>
>   Could not create optee node.
>   ERROR: system-specific fdt fixup failed: FDT_ERR_EXISTS
>    - must RESET the board to recover.
>
>   failed to process device tree
>
[...]
>
> Deal with an existing optee node gracefully at runtime to fix this issue.

We hit that exact problem on our imx8mp-evk trying to boot
v2025.04. However, since the .dtb in our kernel FIT image certainly
did/does not have a /firmware nor /firmware/optee node, I was wondering
how we could possibly hit that FDT_ERR_EXISTS, but we clearly did.

Digging, we found this, which is now commit 048fabc21bf. But I'm
wondering if this is really the right fix. Because digging further, it
seems that it is U-Boot stepping on its own toes:

We have the generic

  optee_copy_fdt_nodes()

called from image_setup_libfdt(), which copies various optee-related
nodes and properties from U-Boot's control dtb to the blob which is to
become the linux kernel's dtb. So that's where the /firmware/optee node
is really created in the first place. Later, image_setup_libfdt() calls
ft_system_setup() which in turn calls the helper

  ft_add_optee_node()

and then hits that "oh but the node already exists". 

It seems that the two highlighted functions really try to do the
same thing, though in slightly different ways. The former copies from
U-Boot's control dtb, while the latter hard-codes the compatible and
method strings and (re-)computes the memory areas. So using the
"find_or_add" variant does feel like somewhat of a band-aid. Shouldn't
we have just one place that updates the kernel dtb, instead of one piece
of code silently changing or redoing what another piece of code has
already done?

Rasmus

PS: On a somewhat related note, it seems that the whole CONFIG_OPTEE
stuff broke our setup in another way: We used to have
loadaddr=0x60000000, but trying to use that address now results in

** Reading file would overwrite reserved memory **
Failed to load 'kernel.itb'

because somehow U-Boot ends up with this memory view:

lmb_dump_all:
 memory.count = 0x2
 memory[0]      [0x40000000-0x55ffffff], 0x16000000 bytes, flags: none
 memory[1]      [0x58000000-0x1bfffffff], 0x168000000 bytes, flags: none
 reserved.count = 0x2
 reserved[0]    [0x52f020c0-0x55ffffff], 0x30fdf40 bytes, flags: no-map
 reserved[1]    [0x58000000-0x1bfffffff], 0x168000000 bytes, flags: no-map

That 0x56000000-0x58000000 is carved out of memory is fine (that's where
op-tee lives), but it's really odd that all memory from 0x58000000 till
the end ends up "reserved". The first reserved entry is, I guess, U-Boot
having relocated itself to the end of the first chunk of memory.