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

[Julien Grall <julien@xxxxxxx>]

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.
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.
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?
Cheers,

--
Julien Grall