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

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi,
Julien & Michal, thanks for comments.

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.
Regards,
Vikram
> Cheers,