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

[Vikram Garhwal <vikram.garhwal@amd.com>]

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@amd.com>
>>>> ---
>>>>   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,
>