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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 07/12/2023 13:20, Julien Grall wrote:
> 
> 
> 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?
There are many things that can go wrong.
Quite recently, I faced an issue where I specified 2 dom0less domUs in 
configuration
and due to the error while parsing the last node of domU1, domU2 node was 
skipped and
Xen booted only domU1 without giving any errors.

Issues with shared memory led to either Xen continue to run with improper 
configuration,
silencing overlap conditions, errors at later boot stages that were impossible 
to deduct
from the logs.

All in all, early boot code parsing assume the error to result in a failure and 
the parsing
for domain creation makes this assumption as well (checks are more relaxed to 
avoid duplication).

For now, we can't do anything better than panicing early if we want to avoid 
unwanted behavior.

> 
> I wonder whether we can either enable the console earlier, or make
> earlyprintk more dynamic (similar to what Linux is doing with
> earlyprintk=...).
The most imporatant part is early fdt parsing. The main console init cannot be 
moved that early.
We need to add earlycon support just like in Linux. User could then select 
either earlyprintk or earlycon.
(earlyprintk would also print from assembly + from everything before 
boot_fdt_info). I think adding support
for it is doable. But as always, no one has time for that.

~Michal