Re: [PATCH] ARM: fdt: copy TF-A reserved memory into fdt passed to Linux

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 6149 bytes --]

Hi,

Le Sat 13 Jun 26, 22:41, Alexander Sverdlin a écrit :
> Currently some ARM-based platforms reserve TF-A memory in their own ways:
> - Mediatek gets BL31 region via SMC call in ft_system_setup()
> - K3 uses CONFIG_K3_ATF_LOAD_ADDR, effectively in ft_system_setup()
> 
> And others like Allwinner simply forget to do it, which results in Linux
> overwriting TF-A and crashing.

To be fair we've been adding the reserved memory regions statically in
the Linux device-trees to mitigate the issue.

But another thing we do overwrite current is the cpu idle states, which
are added by fdt_add_cpu_idle_states in tf-a. These are only set when the
SCP firmware is available (which is checked at run-time) and they are
never propagated to the final device-tree. Including the definitions
statically would result in cpu idle calls done even without the SCP
firmware, which would probably fail (although maybe some states can
still be supported).

Also note that the usual way to deal with this is to not load any
device-tree when booting the kernel, which will implicitly let U-Boot
use its current device-tree for Linux (with the modifications brought by
tf-a).

But of course I agree that it is very desirable to "forward" the
device-tree modifications to the kernel device-tree so we are not stuck
with whatever device-tree U-Boot was built with.

> Unfortunately seems that the things are not much better on TF-A side and
> there is no universal way to get the reserved memory region across
> platforms. But there is at least a most common way in TF-A, namely
> reserving  memory range in the FDT, in particular:
> - Allwinner	("tf-a@40000000" node)
> - ARM FPGA	("tf-a@80000000" node)
> - Xilinx	("tf-a" node)

RaspberryPi seems to be using "atf@0". Generally speaking the property
is a free-form argument to fdt_add_reserved_memory in tf-a and I don't
think we can have a common way to match them.

Introducing a Kconfig property for the prefix would be a satisfying
solution in my opinion.

> While this patch aims to improve the situation for Allwinner platforms,
> it's deliberately adding more generic code to pave the potential way of
> unification for other platforms.
>
> Note that fdtdec_add_reserved_memory() has a check for an already existing
> carveout with exactly matching boundaries and will not create a duplicate
> even if the name doesn't match. It would not however detect an already
> existing bigger carveout fully containing the one requested.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> ---
> The patch has been developed to faciliate Allwinner A133 SoC support, where
> most of the work currently happens on TF-A [1] and Linux [2] sides, but
> I wanted to send this patch upfront to get the first feedback and because
> already supported H616 SoC would already benefit from the patch.

Thanks for looking at this!

Like I said, I guess the same needs to be done for the cpuidle psci
nodes.

> [1] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/49754
> [2] https://lore.kernel.org/all/20260605070923.3045073-1-alexander.sverdlin@gmail.com/
> [3] https://lore.kernel.org/all/b428d57ba5464f1226daf099877f4c25fa4fc191.camel@gmail.com/
> 
>  arch/arm/lib/bootm-fdt.c | 55 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
> index 2671f9a0ebf..19d917943c1 100644
> --- a/arch/arm/lib/bootm-fdt.c
> +++ b/arch/arm/lib/bootm-fdt.c
> @@ -14,7 +14,9 @@
>   * Copyright (C) 2001  Erik Mouw (J.A.K.Mouw@its.tudelft.nl)
>   */
>  
> +#include <dm/ofnode.h>
>  #include <fdt_support.h>
> +#include <linux/ioport.h>
>  #ifdef CONFIG_ARMV7_NONSEC
>  #include <asm/armv7.h>
>  #endif
> @@ -24,6 +26,52 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +static int tfa_copy_reserved_memory(void *new_blob)

I would keep a fdt-related prefix to the name, so something like:
fdt_update_tfa_reserved_memory or
fdt_update_bl31_reserved_memory

> +{
> +#ifdef CONFIG_OF_CONTROL
> +	ofnode node, subnode;
> +
> +	/*
> +	 * TF-A for several platforms inserts its memory region as
> +	 * reserved-memory node

This will always be the case, I don't think we need a comment about it.

> +	 */
> +	node = ofnode_path("/reserved-memory");
> +	if (!ofnode_valid(node))
> +		return 0;
> +
> +	ofnode_for_each_subnode(subnode, node) {
> +		struct fdt_memory carveout;
> +		struct resource res;
> +		const char *name;
> +		int ret;
> +
> +		name = ofnode_get_name(subnode);
> +		if (!name)
> +			return -FDT_ERR_BADSTRUCTURE;
> +
> +		/* only handle TF-A reservations */

Please capitalize the first letter and end with a dot (same for all
comments).

> +		if (strncmp(name, "tf-a", 4))
> +			continue;
> +
> +		/* check if this subnode has a reg property */
> +		ret = ofnode_read_resource(subnode, 0, &res);
> +		if (ret)
> +			continue;
> +
> +		carveout.start = res.start,
> +		carveout.end = res.end,
> +
> +		ret = fdtdec_add_reserved_memory(new_blob, "tf-a", &carveout,
> +						 NULL, 0, NULL,
> +						 FDTDEC_RESERVED_MEMORY_NO_MAP);
> +		if (ret < 0)
> +			return ret;

Looks good otherwise.

> +	}
> +#endif
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_FMAN_ENET
>  __weak int fdt_update_ethernet_dt(void *blob)
>  {
> @@ -56,6 +104,13 @@ int arch_fixup_fdt(void *blob)
>  		return ret;
>  #endif
>  
> +	ret = tfa_copy_reserved_memory(blob);
> +	if (ret) {
> +		printf("ERROR: transfer of TF-A nodes to new fdt failed: %s\n",
> +		       fdt_strerror(ret));

This is very unlikely, not sure it is worth an error message.
Other calls in the same function are silent.

> +		return ret;
> +	}
> +
>  #ifdef CONFIG_ARMV8_SPIN_TABLE
>  	ret = spin_table_update_dt(blob);
>  	if (ret)
> -- 
> 2.54.0
> 

All the best,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]