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

[Michael Clark]

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
>