[PATCH v2 0/3] device_tree: Allow for and use string arrays [Was: RISC-V: virt: This is a "sifive, test1" test finisher]

[PATCH v2 0/3] device_tree: Allow for and use string arrays [Was: RISC-V: virt: This is a "sifive, test1" test finisher]

[Palmer Dabbelt]

Device trees commonly contain arrays of strings for compatible nodes.
We recently extended the "sifive,test0" node in a backwards-compatible
way, but QEMU didn't contain an FDT function to set 'compatible =
"sifive,test1", "sifive,test0";'.  I've converted over the code from the
ARM virt board that was doing something similar to be a helper function,
which I could then use for RISC-V as well.

I haven't tested the ARM change, but I have tested the RISC-V one.  It
appears to parse correctly in Linux, and a DTC treats it the same way as
it treats the string arrays it compiles -- specifically:

    $ cat test.dts
    /dts-v1/;

    / {
        string = "stringa";
        strings = "string1", "string2";
    };
    $ dtc -I dts test.dts -O dtb -o test.dtb
    $ dtc -I dtb test.dtb -O dts -o out.dts
    $ cat out.dts
    /dts-v1/;

    / {
            string = "stringa";
            strings = "string1\0string2";
    };

and

    $ qemu-system-riscv64 -m virt,dumpdtb=out.dtb ...
    $ dtc -I dtb test.dtb -O dts -o out.dts
    $ cat out.dts
    ...
            test@100000 {
                reg = <0x00 0x100000 0x00 0x1000>;
                compatible = "sifive,test1\0sifive,test0";
        };
    ...

Changes since v1 <20191107222500.8018-1-palmer@sifive.com>:

* This is now a multiple patch series.
* The hepler function has been added and used by the RISC-V virt board.
* The ARM virt board has been converted to use the helper function

[PATCH v2 1/3] device_tree: Add a helper function for string arrays

[Palmer Dabbelt]

The device tree format allows for arrays of strings, which are encoded
with '\0's inside regular strings.  These are ugly to represent in C, so
the helper function represents them as strings with internal '\0's that
are terminated with a double '\0'.  In other words, the array
["string1", "string2"] is represeted as "string1\0string2\0".

The DTB generated by this function is accepted by DTC and produces an
array of strings, but I can't find any explicit line in the DT
specification that defines how these are encoded.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 device_tree.c                | 17 +++++++++++++++++
 include/sysemu/device_tree.h |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index f8b46b3c73..b4379f13a7 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -397,6 +397,23 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
     return r;
 }
 
+static size_t stringarr_length(const char *strings)
+{
+    size_t count = 1;
+    while (strings[count - 1] != '\0' || strings[count] != '\0') {
+        count++;
+    }
+    return count;
+}
+
+int qemu_fdt_setprop_strings(void *fdt, const char *node_path,
+                            const char *property, const char *strings)
+{
+    size_t length = stringarr_length(strings);
+    return qemu_fdt_setprop(fdt, node_path, property, strings, length);
+}
+
+
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
                              const char *property, int *lenp, Error **errp)
 {
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index c16fd69bc0..d43c07128e 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -70,6 +70,12 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
 int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
                              const char *property,
                              const char *target_node_path);
+/*
+ * This uses a particularly odd encoding: "strings" is a list of strings that
+ * must be terminated by two back-to-back '\0' characters.
+ */
+int qemu_fdt_setprop_strings(void *fdt, const char *node_path,
+                             const char *property, const char *strings);
 /**
  * qemu_fdt_getprop: retrieve the value of a given property
  * @fdt: pointer to the device tree blob
-- 
2.21.0

Re: [PATCH v2 1/3] device_tree: Add a helper function for string arrays

[Peter Maydell]

On Fri, 8 Nov 2019 at 19:48, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> The device tree format allows for arrays of strings, which are encoded
> with '\0's inside regular strings.  These are ugly to represent in C, so
> the helper function represents them as strings with internal '\0's that
> are terminated with a double '\0'.  In other words, the array
> ["string1", "string2"] is represeted as "string1\0string2\0".
>
> The DTB generated by this function is accepted by DTC and produces an
> array of strings, but I can't find any explicit line in the DT
> specification that defines how these are encoded.

> +/*
> + * This uses a particularly odd encoding: "strings" is a list of strings that
> + * must be terminated by two back-to-back '\0' characters.
> + */
> +int qemu_fdt_setprop_strings(void *fdt, const char *node_path,
> +                             const char *property, const char *strings);

The clean API for this would be to use varargs so you could write

qemu_fdt_setprop_stringlist(fdt, node, prop, "arm,armv8-timer",
                            "arm,armv7-timer");

and have it do the assembly into the encoding that fdt expects.
That would require us to do a bit of allocation-and-freeing
to assemble the string, of course, but then we only do fdt
creation at startup.

NB: I think that this is a good idea but not-for-4.2 material,
so if you wanted your sifive board change to go into 4.2 you
should probably start with the simple approach and leave the
refactoring for the next release cycle.

thanks
-- PMM

Re: [PATCH v2 1/3] device_tree: Add a helper function for string arrays

[David Gibson]

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

On Sat, Nov 09, 2019 at 03:59:24PM +0000, Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 19:48, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> > The device tree format allows for arrays of strings, which are encoded
> > with '\0's inside regular strings.  These are ugly to represent in C, so
> > the helper function represents them as strings with internal '\0's that
> > are terminated with a double '\0'.  In other words, the array
> > ["string1", "string2"] is represeted as "string1\0string2\0".
> >
> > The DTB generated by this function is accepted by DTC and produces an
> > array of strings, but I can't find any explicit line in the DT
> > specification that defines how these are encoded.
> 
> > +/*
> > + * This uses a particularly odd encoding: "strings" is a list of strings that
> > + * must be terminated by two back-to-back '\0' characters.
> > + */
> > +int qemu_fdt_setprop_strings(void *fdt, const char *node_path,
> > +                             const char *property, const char *strings);
> 
> The clean API for this would be to use varargs so you could write
> 
> qemu_fdt_setprop_stringlist(fdt, node, prop, "arm,armv8-timer",
>                             "arm,armv7-timer");
> 
> and have it do the assembly into the encoding that fdt expects.
> That would require us to do a bit of allocation-and-freeing
> to assemble the string, of course, but then we only do fdt
> creation at startup.

Right, I really don't see the value in this interface.  Using
"foo\0bar" is a little ugly, but not really any uglier than
"foo\0bar\0".  The existing interface would be a drag if you had
dynamically created entries in the list (because getting the size
can't be done with sizeof() then), but I don't think that's actually a
very likely usecase.

> NB: I think that this is a good idea but not-for-4.2 material,
> so if you wanted your sifive board change to go into 4.2 you
> should probably start with the simple approach and leave the
> refactoring for the next release cycle.

I concur.

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

[PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings()

[Palmer Dabbelt]

This new helper function encodes the idiom used by the ARM virt board to
set a string array.  I don't currently have a working ARM userspace, so I haven't tested
this, but I made the helper function because I wanted to use it for the
RISC-V virt board where I have tested it.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 hw/arm/virt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc2607..4dc00f54d5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -304,9 +304,8 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
 
     armcpu = ARM_CPU(qemu_get_cpu(0));
     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
-        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
-        qemu_fdt_setprop(vms->fdt, "/timer", "compatible",
-                         compat, sizeof(compat));
+        qemu_fdt_setprop_strings(vms->fdt, "/timer", "compatible",
+                                 "arm,armv8-timer\0arm,armv7-timer\0");
     } else {
         qemu_fdt_setprop_string(vms->fdt, "/timer", "compatible",
                                 "arm,armv7-timer");
-- 
2.21.0

Re: [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings()

[Peter Maydell]

On Fri, 8 Nov 2019 at 19:48, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> This new helper function encodes the idiom used by the ARM virt board to
> set a string array.  I don't currently have a working ARM userspace, so I haven't tested
> this, but I made the helper function because I wanted to use it for the
> RISC-V virt board where I have tested it.
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  hw/arm/virt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d4bedc2607..4dc00f54d5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -304,9 +304,8 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
>
>      armcpu = ARM_CPU(qemu_get_cpu(0));
>      if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> -        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
> -        qemu_fdt_setprop(vms->fdt, "/timer", "compatible",
> -                         compat, sizeof(compat));
> +        qemu_fdt_setprop_strings(vms->fdt, "/timer", "compatible",
> +                                 "arm,armv8-timer\0arm,armv7-timer\0");
>      } else {


This seems to be changing the property we put in -- in
the old code it is 'foo\0bar\0', but in the new code
there will end up being two \0 at the end: 'foo\0bar\0\0'.

thanks
-- PMM

Re: [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings()

[David Gibson]

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

On Sat, Nov 09, 2019 at 03:56:21PM +0000, Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 19:48, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> > This new helper function encodes the idiom used by the ARM virt board to
> > set a string array.  I don't currently have a working ARM userspace, so I haven't tested
> > this, but I made the helper function because I wanted to use it for the
> > RISC-V virt board where I have tested it.
> >
> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > ---
> >  hw/arm/virt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index d4bedc2607..4dc00f54d5 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -304,9 +304,8 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
> >
> >      armcpu = ARM_CPU(qemu_get_cpu(0));
> >      if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> > -        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
> > -        qemu_fdt_setprop(vms->fdt, "/timer", "compatible",
> > -                         compat, sizeof(compat));
> > +        qemu_fdt_setprop_strings(vms->fdt, "/timer", "compatible",
> > +                                 "arm,armv8-timer\0arm,armv7-timer\0");
> >      } else {
> 
> 
> This seems to be changing the property we put in -- in
> the old code it is 'foo\0bar\0', but in the new code
> there will end up being two \0 at the end: 'foo\0bar\0\0'.

In fact it's not because the setprop_strings() helper just uses the
\0\0 to detect the end, but truncates what it actually puts into the
dtb at the first \0.  But I agree this is confusing enough not to
really be an improvement over the original version.

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

[PATCH v2 3/3] RISC-V: virt: This is a "sifive,test1" test finisher

[Palmer Dabbelt]

The test finisher implements the reset command, which means it's a
"sifive,test1" device.  This is a backwards compatible change, so it's
also a "sifive,test0" device.

Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality")
Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 hw/riscv/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 23f340df19..65ad725920 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -359,7 +359,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     nodename = g_strdup_printf("/test@%lx",
         (long)memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(fdt, nodename);
-    qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0");
+    qemu_fdt_setprop_strings(fdt, nodename, "compatible",
+                             "sifive,test1\0sifive,test0\0");
     qemu_fdt_setprop_cells(fdt, nodename, "reg",
         0x0, memmap[VIRT_TEST].base,
         0x0, memmap[VIRT_TEST].size);
-- 
2.21.0