Re: [XEN][PATCH v5 02/17] common/device_tree: change __unflatten_device_tree()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/04/2023 18:51, Vikram Garhwal wrote:
> On 4/13/23 3:03 AM, Julien Grall wrote:
> > Hi,
> >
> > On 11/04/2023 20:16, Vikram Garhwal wrote:
> > > Following changes are done to __unflatten_device_tree():
> > >      1. __unflatten_device_tree() is renamed to unflatten_device_tree().
> > >      2. Remove static function type.
> > >      3. Add handling of memory allocation. This will be useful in 
> > > dynamic node
> > >          programming when we unflatten the dt during runtime memory 
> > > allocation
> > >          can fail.
> > >
> > > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> > > ---
> > >   xen/common/device_tree.c      | 10 ++++++----
> > >   xen/include/xen/device_tree.h |  5 +++++
> > >   2 files changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > index aed38ff63c..bf847b2584 100644
> > > --- a/xen/common/device_tree.c
> > > +++ b/xen/common/device_tree.c
> > > @@ -2047,7 +2047,7 @@ static unsigned long unflatten_dt_node(const 
> > > void *fdt,
> > >   }
> > >     /**
> > > - * __unflatten_device_tree - create tree of device_nodes from flat blob
> > > + * unflatten_device_tree - create tree of device_nodes from flat blob
> > >    *
> > >    * unflattens a device-tree, creating the
> > >    * tree of struct device_node. It also fills the "name" and "type"
> > > @@ -2056,8 +2056,7 @@ static unsigned long unflatten_dt_node(const 
> > > void *fdt,
> > >    * @fdt: The fdt to expand
> > >    * @mynodes: The device_node tree created by the call
> > >    */
> > > -static void __unflatten_device_tree(const void *fdt,
> > > -                                    struct dt_device_node **mynodes)
> > > +void unflatten_device_tree(const void *fdt, struct dt_device_node 
> > > **mynodes)
> > >   {
> > >       unsigned long start, mem, size;
> > >       struct dt_device_node **allnextp = mynodes;
> > > @@ -2079,6 +2078,9 @@ static void __unflatten_device_tree(const void 
> > > *fdt,
> > >       /* Allocate memory for the expanded device tree */
> > >       mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct 
> > > dt_device_node));
> > >   +    if ( !mem )
> > > +        panic("Cannot allocate memory for unflatten device tree\n");
> > After your series, unflatten_device_tree() will be called after boot, 
> > so we should not unconditionally called panic(). Instead, 
> > unflatten_device_tree() should return an error and let the caller 
> > decide what to do.
> Looks like i misunderstood v4 comments. Will change it to propagate an 
> error in case of failure here to the handle_add_overlay_nodes() caller 
> and that will further forward to error to toolstack.
> > I suggest to read misc/xen-error-handling.txt to understand when to 
> > use panic()/BUG() & co. For...
> >
> > > +
> > >       ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef);
> > >         dt_dprintk("  unflattening %lx...\n", mem);
> > > @@ -2179,7 +2181,7 @@ dt_find_interrupt_controller(const struct 
> > > dt_device_match *matches)
> > >     void __init dt_unflatten_host_device_tree(void)
> > >   {
> > > -    __unflatten_device_tree(device_tree_flattened, &dt_host);
> > > +    unflatten_device_tree(device_tree_flattened, &dt_host);
> > ... this caller this should be a panic() (this is OK here because it 
> > is boot code).
> > But for your new caller, you should properly report the error back the 
> > toolstack.
> Understood, will change it in next version.
> > Also, unflatten_dt_node() (called by __unflatten_device_tree()) seems 
> > to have some failure cases. Can you explain why they are not properly 
> > propagated in your case? Are you trusting the device-tree to always be 
> > valid?
> for dynamic_programming, while adding node(check patch: [XEN][PATCH v5 
> 14/17] xen/arm: Implement device tree node addition functionalities), 
> fdt_overlay_apply() is called before unflatten_device_tree() is called. 
> fdt_overlay_apply() will catch the invalid device-tree overlay nodes.
I agree that in theory fdt_overlay_apply() will catch invalid 
device-tree. However, neither of the functions are exempts of bugs and 
there is no code shared between the two (they are not even coming from 
the same project).
So we could end up in a situation where fdt_overlay_apply() works but 
not unflatten_device_tree(). Therefore, I would rather prefer if the 
latter function properly handle any errors.
Note that unflatten_dt_node() already have check the validity of the DT 
and will return. We just need to make sure they are treated as error 
rather than been ignored.
Cheers,

--
Julien Grall