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,
>