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

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

[Michael Clark]

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 <crosthwaite.peter@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 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
-- 
2.7.0

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

[Peter Maydell]

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

thanks
-- PMM

On 4 May 2018 at 02:19, Michael Clark <mjc@sifive.com> 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 <crosthwaite.peter@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  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
> --
> 2.7.0

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]

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

> 
> thanks
> -- PMM
> 
> On 4 May 2018 at 02:19, Michael Clark <mjc@sifive.com> 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 <crosthwaite.peter@gmail.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Michael Clark]

On Sat, May 5, 2018 at 11:48 PM, David Gibson <david@gibson.dropbear.id.au>
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 <mjc@sifive.com> 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 <crosthwaite.peter@gmail.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >  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
>

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 <mjc@sifive.com> wrote:

>
>
> On Sat, May 5, 2018 at 11:48 PM, David Gibson <david@gibson.dropbear.id.au
> > 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 <mjc@sifive.com> 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 <crosthwaite.peter@gmail.com>
>> > > Cc: Alexander Graf <agraf@suse.de>
>> > > Cc: Alistair Francis <Alistair.Francis@wdc.com>
>> > > Cc: Peter Maydell <peter.maydell@linaro.org>
>> > > ---
>> > >  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
>>
>
>

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

[Peter Maydell]

On 5 May 2018 at 22:59, Michael Clark <mjc@sifive.com> wrote:
> 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.

Yeah, I find this combination of libfdt direct access and a
QEMU wrapper layer a bit confusing. I think it's grown more
than it's been actively designed. Your approach of doing
direct calls to pack/totalsize is fine. Maybe the wrapper
layer could be improved some day too.

thanks
-- PMM

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On Sun, May 06, 2018 at 01:23:30PM +0100, Peter Maydell wrote:
> On 5 May 2018 at 22:59, Michael Clark <mjc@sifive.com> wrote:
> > 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.
> 
> Yeah, I find this combination of libfdt direct access and a
> QEMU wrapper layer a bit confusing. I think it's grown more
> than it's been actively designed.

Absolutely.  I think the history is that when fdt was first used, the
layer was created as an (arguably) simpler wrapper.  But, it was very
limited.  Once you start doing more complex things, there's no great
advantage to using that limited wrapper over the raw library, hence
the direct usage.  At least, that's what I think though I'm obviously
a) already familiar with the libfdt interface and b) biased.

> Your approach of doing
> direct calls to pack/totalsize is fine. Maybe the wrapper
> layer could be improved some day too.

Well, I'm biased of course, but I think we'd be better off just
ditching the wrapper.  In its present form it is so limited as to not
really add any value.  If it was rewritten to do something useful
(e.g. handling reallocations), I think it would be even better if
done as an extension to libfdt itself so it can benefit everyone, not
just qemu.

Although, that said, I'll re-iterate that I think qemu's fdt
manipulation is now sufficiently complex that it would be better off
using a "live" (dynamically allocated & pointer based) tree
representation that we just flatten immediately before loading it into
the guest.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7621 bytes --]

On Sun, May 06, 2018 at 09:59:50AM +1200, Michael Clark wrote:
> On Sat, May 5, 2018 at 11:48 PM, David Gibson <david@gibson.dropbear.id.au>
> 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 <mjc@sifive.com> 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 <crosthwaite.peter@gmail.com>
> > > > Cc: Alexander Graf <agraf@suse.de>
> > > > Cc: Alistair Francis <Alistair.Francis@wdc.com>
> > > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > > ---
> > > >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Maydell]

On 6 May 2018 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> Well, I'm biased of course, but I think we'd be better off just
> ditching the wrapper.  In its present form it is so limited as to not
> really add any value.  If it was rewritten to do something useful
> (e.g. handling reallocations), I think it would be even better if
> done as an extension to libfdt itself so it can benefit everyone, not
> just qemu.

Well, some of it is working around infelicities in libfdt's
API (like all the getprop/setprop functions taking an offset
value rather than a node name), but yes, it would be better
to fix the libfdt API if possible.

> Although, that said, I'll re-iterate that I think qemu's fdt
> manipulation is now sufficiently complex that it would be better off
> using a "live" (dynamically allocated & pointer based) tree
> representation that we just flatten immediately before loading it into
> the guest.

This sounds to me like something that should be handled
by libfdt. I don't particularly care what libfdt's
internal representation of the data structure is, I just
want to be able to (a) hand it an fdt read in from a file
(b) call various functions to modify the data structure
and then (c) write the resulting thing out to an fdt in
guest memory. Whether libfdt prefers to do that by
modifying the flat representation or by converting into
a dynamically allocated unflattened tree and back again
is something I'd rather leave to it as an implementation
detail.

thanks
-- PMM

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2832 bytes --]

On Sun, May 06, 2018 at 04:04:02PM +0100, Peter Maydell wrote:
> On 6 May 2018 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> > Well, I'm biased of course, but I think we'd be better off just
> > ditching the wrapper.  In its present form it is so limited as to not
> > really add any value.  If it was rewritten to do something useful
> > (e.g. handling reallocations), I think it would be even better if
> > done as an extension to libfdt itself so it can benefit everyone, not
> > just qemu.
> 
> Well, some of it is working around infelicities in libfdt's
> API (like all the getprop/setprop functions taking an offset
> value rather than a node name), but yes, it would be better
> to fix the libfdt API if possible.
> 
> > Although, that said, I'll re-iterate that I think qemu's fdt
> > manipulation is now sufficiently complex that it would be better off
> > using a "live" (dynamically allocated & pointer based) tree
> > representation that we just flatten immediately before loading it into
> > the guest.
> 
> This sounds to me like something that should be handled
> by libfdt.

No, it's really not.  libfdt is specifically for reading and writing
flattened trees in place, in flattened form.  That's the whole basis
of the design and it's directly responsible for some of the
infelicities you mention.

I was never intended as a general runtime t manipulation library.  A
libdt that does that (using an allocator and internal pointer
representation) would be a nice thing to have, but libfdt isn't it.
Obviously being able to use libfdt to flatten and unflatten trees
would be a very good feature for such a library.

> internal representation of the data structure is, I just
> want to be able to (a) hand it an fdt read in from a file
> (b) call various functions to modify the data structure
> and then (c) write the resulting thing out to an fdt in
> guest memory. Whether libfdt prefers to do that by
> modifying the flat representation or by converting into
> a dynamically allocated unflattened tree and back again
> is something I'd rather leave to it as an implementation
> detail.

Sort of.  But there design constraints in libfdt which means that's
not really feasible.  Such as:
  * not requiring an allocator
  * not requiring a "read / unflatten" pass before reading values from
    an fdt
  * not requiring allocation or creation of a "context" state for
    manipulating a tree

Those aren't aren't really relevant to qemu and perhaps aren't part of
what you'd usually think of as the API.  But they do matter to real
libfdt use cases.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Maydell]

On 9 May 2018 at 06:32, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sun, May 06, 2018 at 04:04:02PM +0100, Peter Maydell wrote:
>> On 6 May 2018 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
>> > Although, that said, I'll re-iterate that I think qemu's fdt
>> > manipulation is now sufficiently complex that it would be better off
>> > using a "live" (dynamically allocated & pointer based) tree
>> > representation that we just flatten immediately before loading it into
>> > the guest.
>>
>> This sounds to me like something that should be handled
>> by libfdt.
>
> No, it's really not.  libfdt is specifically for reading and writing
> flattened trees in place, in flattened form.  That's the whole basis
> of the design and it's directly responsible for some of the
> infelicities you mention.
>
> I was never intended as a general runtime t manipulation library.  A
> libdt that does that (using an allocator and internal pointer
> representation) would be a nice thing to have, but libfdt isn't it.
> Obviously being able to use libfdt to flatten and unflatten trees
> would be a very good feature for such a library.

The difficulty is that libfdt is the closest we have to a
dt manipulation library. The current wrapper layer is sort
of trying to fix up its deficiencies for this use case.
I just think that libfdt (as project, not necessarily as
a single source file or library) is better placed to design
and implement a more featured dt manipulation library layer
than QEMU is.

(Possibly we have here a slight terminology confusion: when
I think about "libfdt" what I mean is "the project that has
code for manipulating device trees and provides a library
and some source files you can compile directly into a kernel
or bootloader and a standalone dtc compiler binary and so on",
not "specifically a single library". So it seems to me more
natural for generic library code for manipulating DTs with
an unflatten-manipulate-flatten approach would also be part
of that project.)

>> internal representation of the data structure is, I just
>> want to be able to (a) hand it an fdt read in from a file
>> (b) call various functions to modify the data structure
>> and then (c) write the resulting thing out to an fdt in
>> guest memory. Whether libfdt prefers to do that by
>> modifying the flat representation or by converting into
>> a dynamically allocated unflattened tree and back again
>> is something I'd rather leave to it as an implementation
>> detail.
>
> Sort of.  But there design constraints in libfdt which means that's
> not really feasible.  Such as:
>   * not requiring an allocator
>   * not requiring a "read / unflatten" pass before reading values from
>     an fdt
>   * not requiring allocation or creation of a "context" state for
>     manipulating a tree
>
> Those aren't aren't really relevant to qemu and perhaps aren't part of
> what you'd usually think of as the API.  But they do matter to real
> libfdt use cases.

Yes, you'd certainly want to keep the compile time option
to just have the "no allocation" parts of the library so
you could build it into kernels and boot loaders.

thanks
-- PMM

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4211 bytes --]

On Wed, May 09, 2018 at 12:23:49PM +0100, Peter Maydell wrote:
> On 9 May 2018 at 06:32, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Sun, May 06, 2018 at 04:04:02PM +0100, Peter Maydell wrote:
> >> On 6 May 2018 at 14:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > Although, that said, I'll re-iterate that I think qemu's fdt
> >> > manipulation is now sufficiently complex that it would be better off
> >> > using a "live" (dynamically allocated & pointer based) tree
> >> > representation that we just flatten immediately before loading it into
> >> > the guest.
> >>
> >> This sounds to me like something that should be handled
> >> by libfdt.
> >
> > No, it's really not.  libfdt is specifically for reading and writing
> > flattened trees in place, in flattened form.  That's the whole basis
> > of the design and it's directly responsible for some of the
> > infelicities you mention.
> >
> > I was never intended as a general runtime t manipulation library.  A
> > libdt that does that (using an allocator and internal pointer
> > representation) would be a nice thing to have, but libfdt isn't it.
> > Obviously being able to use libfdt to flatten and unflatten trees
> > would be a very good feature for such a library.
> 
> The difficulty is that libfdt is the closest we have to a
> dt manipulation library. The current wrapper layer is sort
> of trying to fix up its deficiencies for this use case.
> I just think that libfdt (as project, not necessarily as
> a single source file or library) is better placed to design
> and implement a more featured dt manipulation library layer
> than QEMU is.

Well, libfdt is already a subcomponent of the dtc project.  And yes,
an unflattened dt manipulation library would also make sense as a
component of that project.  Someone just has to write it...

> (Possibly we have here a slight terminology confusion: when
> I think about "libfdt" what I mean is "the project that has
> code for manipulating device trees and provides a library
> and some source files you can compile directly into a kernel
> or bootloader and a standalone dtc compiler binary and so on",
> not "specifically a single library". So it seems to me more
> natural for generic library code for manipulating DTs with
> an unflatten-manipulate-flatten approach would also be part
> of that project.)

Ah, yes, I think  you're right as to the source of confusion.  I think
of libfdt as specifically that one library, whose design is very much
based around *flattened* dt manipulation (hence the name).  It's
interface design is very much based around that, and an unflattened
manipulation library would really have to be separate, and wouldn't
actually share much code at all.

> >> internal representation of the data structure is, I just
> >> want to be able to (a) hand it an fdt read in from a file
> >> (b) call various functions to modify the data structure
> >> and then (c) write the resulting thing out to an fdt in
> >> guest memory. Whether libfdt prefers to do that by
> >> modifying the flat representation or by converting into
> >> a dynamically allocated unflattened tree and back again
> >> is something I'd rather leave to it as an implementation
> >> detail.
> >
> > Sort of.  But there design constraints in libfdt which means that's
> > not really feasible.  Such as:
> >   * not requiring an allocator
> >   * not requiring a "read / unflatten" pass before reading values from
> >     an fdt
> >   * not requiring allocation or creation of a "context" state for
> >     manipulating a tree
> >
> > Those aren't aren't really relevant to qemu and perhaps aren't part of
> > what you'd usually think of as the API.  But they do matter to real
> > libfdt use cases.
> 
> Yes, you'd certainly want to keep the compile time option
> to just have the "no allocation" parts of the library so
> you could build it into kernels and boot loaders.
> 
> thanks
> -- PMM
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]