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

[Julien Grall <julien@xxxxxxx>]

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.
I will add this in my list after I get the earlyprintk working while 
switching the MMU.
Cheers,

--
Julien Grall