Re: [PATCH] xen/arm: bootfdt: Check return code of device_tree_for_each_node()

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 07/12/2023 10:14, Michal Orzel wrote:
> As a result of not checking the return code of device_tree_for_each_node()
> in boot_fdt_info(), any error occured during early FDT parsing does not
> stop Xen from booting. This can result in an unwanted behavior in later
> boot stages. Fix it by checking the return code and panicing on an error.
>
> Fixes: 9cf4a9a46717 ("device tree: add device_tree_for_each_node()")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
With one remark below:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> I've lost count how many times I had to fix missing rc check. However, I have
> not yet found any checker for this (-Wunused-result is pretty useless).
> ---
>   xen/arch/arm/bootfdt.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index b1f03eb2fcdd..f496a8cf9494 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -541,7 +541,9 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>   
>       add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>   
> -    device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> +    ret = device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> +    if ( ret )
> +        panic("Early FDT parsing failed (%d)\n", ret);
AFAIU, the parsing is done before the console is setup. This means a 
user would not be able to get some logs unless they are re-compiling Xen 
with earlyprintk.
I understand this is not a new issue, but I am getting concerned of the 
amount of check we add before the console is up and running.
What is the impact if we don't check the return here? Is it missing regions?

I wonder whether we can either enable the console earlier, or make 
earlyprintk more dynamic (similar to what Linux is doing with 
earlyprintk=...).
Cheers,

--
Julien Grall