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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 07/12/2023 13:54, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 07/12/2023 12:39, Michal Orzel wrote:
>> 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.
> 
> I think we need to understand a bit more why because on x86
> consoel_init_preirq() is called very early. So we ought to be able to do
> the same on Arm.
But this won't cover early fdt parsing. I don't think we can get away without 
adding earlycon
and earlycon helpers in all the serial drivers. arm_uart_init depends on 
unflattening fdt which
depends on relocating fdt which is done after parsing FDT. We want to be able 
to print messages
after early_fdt_map. Once FDT is mapped, the only thing we need is to retrieve 
the bootargs to parse
for earlycon= , add the region to fixmap and register the handlers.

~Michal