Patchwork [28/58] device tree: give dt more size

login
register
mail settings
Submitter Alexander Graf
Date Sept. 14, 2011, 8:42 a.m.
Message ID <1315989802-18753-29-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/114655/
State New
Headers show

Comments

Alexander Graf - Sept. 14, 2011, 8:42 a.m.
We currently load a device tree blob and then just take its size x2 to
account for modifications we do inside. While this is nice and great,
it fails when we have a small device tree as blob and lots of nodes added
in machine init code.

So for now, just make it 20k bigger than it was before. We maybe want to
be more clever about this later.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 device_tree.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
David Gibson - Sept. 15, 2011, 3:19 a.m.
On Wed, Sep 14, 2011 at 10:42:52AM +0200, Alexander Graf wrote:
> We currently load a device tree blob and then just take its size x2 to
> account for modifications we do inside. While this is nice and great,
> it fails when we have a small device tree as blob and lots of nodes added
> in machine init code.
> 
> So for now, just make it 20k bigger than it was before. We maybe want to
> be more clever about this later.

In fact, one of the few things I can think of that might justify
qemu's "abstraction" of the libfdt interface, is that the wrappers
could be modified to detect -FDT_ERR_NOSPACE and realloc()
appropriately.

Otherwise the wrappers, which are limited and not notably simpler to
use than the raw libfdt functions seem pretty pointless to me.

Not that I'm biased as the author of libfdt or anything :).
Alexander Graf - Sept. 15, 2011, 7:37 a.m.
On 15.09.2011, at 05:19, David Gibson wrote:

> On Wed, Sep 14, 2011 at 10:42:52AM +0200, Alexander Graf wrote:
>> We currently load a device tree blob and then just take its size x2 to
>> account for modifications we do inside. While this is nice and great,
>> it fails when we have a small device tree as blob and lots of nodes added
>> in machine init code.
>> 
>> So for now, just make it 20k bigger than it was before. We maybe want to
>> be more clever about this later.
> 
> In fact, one of the few things I can think of that might justify
> qemu's "abstraction" of the libfdt interface, is that the wrappers
> could be modified to detect -FDT_ERR_NOSPACE and realloc()
> appropriately.

Oh, yeah, that sounds like a very good idea!

> Otherwise the wrappers, which are limited and not notably simpler to
> use than the raw libfdt functions seem pretty pointless to me.
> 
> Not that I'm biased as the author of libfdt or anything :).

I agree that the wrappers are not all that overly useful atm. I was actually very close to just ripping them out completely instead of extending them for new functionality. I did have the feeling that wrapping libfdt would give us a few benefits, maybe even the chance of getting rid of #ifdefs in target code.

Could you please put this on your todo list? We should probably force every target code in QEMU to only use the wrappers and dynamically realloc() in them.


Alex
David Gibson - Sept. 15, 2011, 11:03 a.m.
On Thu, Sep 15, 2011 at 09:37:48AM +0200, Alexander Graf wrote:
> 
> On 15.09.2011, at 05:19, David Gibson wrote:
> 
> > On Wed, Sep 14, 2011 at 10:42:52AM +0200, Alexander Graf wrote:
> >> We currently load a device tree blob and then just take its size x2 to
> >> account for modifications we do inside. While this is nice and great,
> >> it fails when we have a small device tree as blob and lots of nodes added
> >> in machine init code.
> >> 
> >> So for now, just make it 20k bigger than it was before. We maybe want to
> >> be more clever about this later.
> > 
> > In fact, one of the few things I can think of that might justify
> > qemu's "abstraction" of the libfdt interface, is that the wrappers
> > could be modified to detect -FDT_ERR_NOSPACE and realloc()
> > appropriately.
> 
> Oh, yeah, that sounds like a very good idea!
> 
> > Otherwise the wrappers, which are limited and not notably simpler to
> > use than the raw libfdt functions seem pretty pointless to me.
> > 
> > Not that I'm biased as the author of libfdt or anything :).
> 
> I agree that the wrappers are not all that overly useful atm. I was
> actually very close to just ripping them out completely instead of
> extending them for new functionality. I did have the feeling that
> wrapping libfdt would give us a few benefits, maybe even the chance
> of getting rid of #ifdefs in target code.

Hrm, maybe.  Can't really see it.  Of course, my preference would be
to get rid of those #ifdefs by embedding libfdt in qemu so it's always
there.

> Could you please put this on your todo list? We should probably
> force every target code in QEMU to only use the wrappers and
> dynamically realloc() in them.

Uh, sure, but it's a long list and it won't be near the top.

The wrappers would need to be a lot more extensive to do this.  I use
libfdt directly in the spapr code for a reason.
Alexander Graf - Sept. 15, 2011, 3 p.m.
Am 15.09.2011 um 13:03 schrieb David Gibson <david@gibson.dropbear.id.au>:

> On Thu, Sep 15, 2011 at 09:37:48AM +0200, Alexander Graf wrote:
>> 
>> On 15.09.2011, at 05:19, David Gibson wrote:
>> 
>>> On Wed, Sep 14, 2011 at 10:42:52AM +0200, Alexander Graf wrote:
>>>> We currently load a device tree blob and then just take its size x2 to
>>>> account for modifications we do inside. While this is nice and great,
>>>> it fails when we have a small device tree as blob and lots of nodes added
>>>> in machine init code.
>>>> 
>>>> So for now, just make it 20k bigger than it was before. We maybe want to
>>>> be more clever about this later.
>>> 
>>> In fact, one of the few things I can think of that might justify
>>> qemu's "abstraction" of the libfdt interface, is that the wrappers
>>> could be modified to detect -FDT_ERR_NOSPACE and realloc()
>>> appropriately.
>> 
>> Oh, yeah, that sounds like a very good idea!
>> 
>>> Otherwise the wrappers, which are limited and not notably simpler to
>>> use than the raw libfdt functions seem pretty pointless to me.
>>> 
>>> Not that I'm biased as the author of libfdt or anything :).
>> 
>> I agree that the wrappers are not all that overly useful atm. I was
>> actually very close to just ripping them out completely instead of
>> extending them for new functionality. I did have the feeling that
>> wrapping libfdt would give us a few benefits, maybe even the chance
>> of getting rid of #ifdefs in target code.
> 
> Hrm, maybe.  Can't really see it.  Of course, my preference would be
> to get rid of those #ifdefs by embedding libfdt in qemu so it's always
> there.

It's a library and should be treated that way. But yeah, I'm inclined to fail configure for ppc when libfdt can't be found too.

> 
>> Could you please put this on your todo list? We should probably
>> force every target code in QEMU to only use the wrappers and
>> dynamically realloc() in them.
> 
> Uh, sure, but it's a long list and it won't be near the top.
> 
> The wrappers would need to be a lot more extensive to do this.  I use
> libfdt directly in the spapr code for a reason.

Expensiveness is not too bad here, no? It should still be fast.

Also, I'm perfectly fine with this being low on the list.

Alex

>
David Gibson - Sept. 16, 2011, 1:49 a.m.
On Thu, Sep 15, 2011 at 05:00:41PM +0200, Alexander Graf wrote:
> 
> Am 15.09.2011 um 13:03 schrieb David Gibson <david@gibson.dropbear.id.au>:
> 
> > On Thu, Sep 15, 2011 at 09:37:48AM +0200, Alexander Graf wrote:
> >> 
> >> On 15.09.2011, at 05:19, David Gibson wrote:
> >> 
> >>> On Wed, Sep 14, 2011 at 10:42:52AM +0200, Alexander Graf wrote:
> >>>> We currently load a device tree blob and then just take its size x2 to
> >>>> account for modifications we do inside. While this is nice and great,
> >>>> it fails when we have a small device tree as blob and lots of nodes added
> >>>> in machine init code.
> >>>> 
> >>>> So for now, just make it 20k bigger than it was before. We maybe want to
> >>>> be more clever about this later.
> >>> 
> >>> In fact, one of the few things I can think of that might justify
> >>> qemu's "abstraction" of the libfdt interface, is that the wrappers
> >>> could be modified to detect -FDT_ERR_NOSPACE and realloc()
> >>> appropriately.
> >> 
> >> Oh, yeah, that sounds like a very good idea!
> >> 
> >>> Otherwise the wrappers, which are limited and not notably simpler to
> >>> use than the raw libfdt functions seem pretty pointless to me.
> >>> 
> >>> Not that I'm biased as the author of libfdt or anything :).
> >> 
> >> I agree that the wrappers are not all that overly useful atm. I was
> >> actually very close to just ripping them out completely instead of
> >> extending them for new functionality. I did have the feeling that
> >> wrapping libfdt would give us a few benefits, maybe even the chance
> >> of getting rid of #ifdefs in target code.
> > 
> > Hrm, maybe.  Can't really see it.  Of course, my preference would be
> > to get rid of those #ifdefs by embedding libfdt in qemu so it's always
> > there.
> 
> It's a library and should be treated that way.

What the hell does that mean.  It's a library, and things are easier
when you can count on it being there.  Embedding means we can do that
without adding an external dependency.  And we already use git
submodules, so that provides an easy way of embedding.

> But yeah, I'm
> inclined to fail configure for ppc when libfdt can't be found too.
> 
> >> Could you please put this on your todo list? We should probably
> >> force every target code in QEMU to only use the wrappers and
> >> dynamically realloc() in them.
> > 
> > Uh, sure, but it's a long list and it won't be near the top.
> > 
> > The wrappers would need to be a lot more extensive to do this.  I use
> > libfdt directly in the spapr code for a reason.
> 
> Expensiveness is not too bad here, no? It should still be fast.

Uh, how did expensiveness get into this?  I'm talking about the sheer
number of potentially useful interfaces that would have to be wrapped
with boilerplate code.

> Also, I'm perfectly fine with this being low on the list.

Also note that handling -FDT_ERR_NOSPACE only applies to write
functions.  I still see little point in wrapping the read only
functions.

Patch

diff --git a/device_tree.c b/device_tree.c
index 751538e..dc69232 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -41,6 +41,7 @@  void *load_device_tree(const char *filename_path, int *sizep)
     }
 
     /* Expand to 2x size to give enough room for manipulation.  */
+    dt_size += 10000;
     dt_size *= 2;
     /* First allocate space in qemu for device tree */
     fdt = g_malloc0(dt_size);