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

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi,

On 4/14/23 11:09 AM, Julien Grall wrote:
> 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.
Thanks for explanation. Will add error handling for unflatten_dt_node() 
and unflatten_device_tree() for failures.
> Cheers,