Re: [Qemu-devel] [PATCH] device_tree: Add qemu_fdt_totalsize function

[Michael Clark]

FYI - I've dropped this patch to qemu/include/sysemu/device_tree.h
and qemu/device_tree.c in favor of calling fdt_pack() and fdt_totalsize().

On Sun, May 6, 2018 at 9:59 AM, Michael Clark <address@hidden> wrote:

>
>
> On Sat, May 5, 2018 at 11:48 PM, David Gibson <address@hidden
> > wrote:
>
>> On Sat, May 05, 2018 at 11:44:25AM +0100, Peter Maydell wrote:
>> > cc'ing David -- is this the best way to do this?
>> >
>> > (It would be nicer if libfdt would just dynamically
>> > reallocate the buffer as needed,
>>
>> That's a deliberate design tradeoff.  Because it's designed to be
>> embeddable in really limited environments, libfdt tries to keep to
>> really minimal dependencies - in particular it doesn't depend on an
>> allocator.
>>
>> It'd be nice to have another (optional) layer on top that does
>> reallocate automatically.allocator.  In principle it's pretty simple -
>> you just call down to the existing functions and if they return
>> FDT_ERR_NOSPACE, rellocate and retry.  I've never had the time to
>> write it though - especially since in most simple cases just
>> allocating a far-to-big buffer initially actually works pretty well,
>> crude though it may be.
>>
>> For complex fdt manipulations.. well.. there comes a point where fdt
>> is the wrong tool for the job and you'd be better off using a "live"
>> representation of the tree and just flattening it when you're
>> finished.  IMO qemu is already past that point.  I send some early
>> draft patches adding live tree infrastructure a while back but got too
>> sidetracked by higher priorities to follow it through.
>>
>> > and then tell you
>> > the final size, rather than having to specify a
>> > maximum buffer size up front, but given the API we
>> > have a "tell me how big this thing actually is"
>> > function seems reasonable. I'm just surprised we
>> > need to know so much about the DT internals to do it.)
>>
>> I'm not familiar with the rest of this thread, so I'm not sure at what
>> point you're wanting to know this.  If it's only right at the end
>> before loading the final blob you can fdt_pack() and then check
>> fdt_totalsize().
>
>
> Okay, so an alternative is to call fdt_pack() and then fdt_totalsize().
> Thanks!
>
> QEMU has its own wrapper around libfdt and neither the fdt_pack() nor
> fdt_totalsize() functions are exposed.
>
> Some architecture use only the wrapper functions provided by
> qemu/include/include/sysemu/device_tree.h
>
> It seems that target/ppc has #include <libfdt.h> and calls fdt_pack() and
> fdt_totalsize() directly.
>
> Some architectures appear to copy the unpacked fdt into their ROMS where
> the allocated size in QEMU defaults to 1MiB.
>
> QEMU has a wrapper function called create_device_tree that
> calls fdt_create(), fdt_finish_reservemap(), fdt_
> begin_node(), fdt_end_node(), fdt_finish(), fdt_open_into() with a 1MiB
> initial buffer size. The resulting device tree is modified in memory using
> various QEMU wrapper APIs that call fdt_setprop_string(),
> fdt_setprop_cell(), etc
>
> For RISC-V we were able to get an accurate size using this function:
>
> size_t qemu_fdt_totalsize(void *fdt)
> {
>     return fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt) +
>            fdt_size_dt_strings(fdt) + sizeof(uint32_t) /* terminator */;
> }
>
> Now that we're aware of fdt_pack() and then fdt_totalsize() then we can
> avoid changing target neutral code in qemu/include/include/sysemu/
> device_tree.h by following ppc's lead and add #include <libfdt.h> and
> call fdt_pack() and fdt_totalsize() directly.
>
> The thing I spotted in fdt_resize() was the calculation to check that the
> new buffer was large enough however it excludes dt_off_dt_struct(fdt) and
> space for the uin32_t terminator. See here:
>
> int fdt_resize(void *fdt, void *buf, int bufsize)
> {
> ...
>
> headsize = fdt_off_dt_struct(fdt);
> tailsize = fdt_size_dt_strings(fdt);
>
> if ((headsize + tailsize) > bufsize)
> return -FDT_ERR_NOSPACE;
>
> ...
>
> return 0;
> }
>
> >
>> > thanks
>> > -- PMM
>> >
>> > On 4 May 2018 at 02:19, Michael Clark <address@hidden> wrote:
>> > > Currently the device-tree create_device_tree function
>> > > returns the size of the allocated device tree buffer
>> > > however there is no way to get the actual amount of
>> > > buffer space used by the device-tree.
>> > >
>> > > 14ec3cbd7c1e31dca4d23f028100c8f43e156573 increases
>> > > the FDT_MAX_SIZE to 1 MiB. This creates an issue
>> > > for boards that have less than 1 MiB in their ROM
>> > > for device tree. While cpu_physical_memory_write
>> > > will not write past the end of a buffer there is
>> > > and a board is aware of its ROM buffer size, so
>> > > can use min(fdt_size,rom_size); this provides no
>> > > indication as to whether the device-tree may be
>> > > truncated. qemu_fdt_totalsize allows a board to
>> > > check that a dynamically created device tree will
>> > > fit within its alloted ROM space.
>> > >
>> > > Add qemu_fdt_totalsize which uses the logic and
>> > > public APIs from libfdt to calculate the device
>> > > size: struct_offset + struct_size + strings_size
>> > > + terminator.
>> > >
>> > > Cc: Peter Crosthwaite <address@hidden>
>> > > Cc: Alexander Graf <address@hidden>
>> > > Cc: Alistair Francis <address@hidden>
>> > > Cc: Peter Maydell <address@hidden>
>> > > ---
>> > >  device_tree.c                | 6 ++++++
>> > >  include/sysemu/device_tree.h | 6 ++++++
>> > >  2 files changed, 12 insertions(+)
>> > >
>> > > diff --git a/device_tree.c b/device_tree.c
>> > > index 52c3358a5583..3a2166d61f37 100644
>> > > --- a/device_tree.c
>> > > +++ b/device_tree.c
>> > > @@ -215,6 +215,12 @@ void *load_device_tree_from_sysfs(void)
>> > >
>> > >  #endif /* CONFIG_LINUX */
>> > >
>> > > +size_t qemu_fdt_totalsize(void *fdt)
>> > > +{
>> > > +    return fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt) +
>> > > +           fdt_size_dt_strings(fdt) + sizeof(uint32_t) /* terminator
>> */;
>> > > +}
>> > > +
>> > >  static int findnode_nofail(void *fdt, const char *node_path)
>> > >  {
>> > >      int offset;
>> > > diff --git a/include/sysemu/device_tree.h
>> b/include/sysemu/device_tree.h
>> > > index e22e5bec9c3f..4af232dfdc65 100644
>> > > --- a/include/sysemu/device_tree.h
>> > > +++ b/include/sysemu/device_tree.h
>> > > @@ -26,6 +26,12 @@ void *load_device_tree_from_sysfs(void);
>> > >  #endif
>> > >
>> > >  /**
>> > > + * qemu_fdt_total_size: returns the size required to store the
>> current
>> > > + * device tree versus the buffer size returned by create_device_tree
>> > > + */
>> > > +size_t qemu_fdt_totalsize(void *fdt);
>> > > +
>> > > +/**
>> > >   * qemu_fdt_node_path: return the paths of nodes matching a given
>> > >   * name and compat string
>> > >   * @fdt: pointer to the dt blob
>> >
>>
>> --
>> David Gibson                    | I'll have my music baroque, and my code
>> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_
>> _other_
>>                                 | _way_ _around_!
>> http://www.ozlabs.org/~dgibson
>>
>
>