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

[David Gibson]

On Sun, May 06, 2018 at 09:59:50AM +1200, Michael Clark 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!

Yes.

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

See my other reply for some of the history of that.

> 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

Ah.. we should probably get rid of that. libfdt now has
fdt_create_empty_tree() built into it, which does basically the same
thing.

> 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

Note that fdt_resize() works on trees in "sequential write" mode -
using it on completed trees (which is nearly all of qemu's fdt
manipulation) won't work (or if it does, only by accident).

> new buffer was large enough however it excludes dt_off_dt_struct(fdt) and
> space for the uin32_t terminator. See here:

Uh.. what uint32_t terminator?

> int fdt_resize(void *fdt, void *buf, int bufsize)
> {
> ...
> 
> headsize = fdt_off_dt_struct(fdt);

.. and it's including off_dt_struct right here.

> 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

signature.asc
Description: PGP signature