| Message ID | 20230411191636.26926-3-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
Hi Vikram, On 11/04/2023 21: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. I think there was no need to touch this function in patch 1 if you are modifying it here additionally in a separate patch. > 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. Didn't we say that checking if the memory allocation failed or not should be done as a separate patch (i.e. a prerequisite to your series) as part of hardening? In any case (depending on the maintainers vote), the change itself looks ok, so: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi, On 13/04/2023 10:45, Michal Orzel wrote: > On 11/04/2023 21: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. > I think there was no need to touch this function in patch 1 if you are modifying it here > additionally in a separate patch. > >> 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. > Didn't we say that checking if the memory allocation failed or not should be done > as a separate patch (i.e. a prerequisite to your series) as part of hardening? > > In any case (depending on the maintainers vote), the change itself looks ok, so: Yes it should be separate because this is something we may want to consider to backport. Cheers,
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. 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,
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@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. Regards, Vikram > > Cheers, >
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. Cheers,
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, >
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"); + ((__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); dt_alias_scan(); } diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 19a74909ce..58ac12abe3 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, int node, */ void dt_unflatten_host_device_tree(void); +/** + * unflatten any device tree. + */ +void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes); + /** * IRQ translation callback * TODO: For the moment we assume that we only have ONE
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(-)