QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] device_tree: Allow for and use string arrays [Was: RISC-V: virt: This is a "sifive, test1" test finisher]
@ 2019-11-08 19:47 Palmer Dabbelt
  2019-11-08 19:47 ` [PATCH v2 1/3] device_tree: Add a helper function for string arrays Palmer Dabbelt
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2019-11-08 19:47 UTC (permalink / raw)
  To: Peter Maydell, Alistair Francis, qemu-devel, david; +Cc: qemu-riscv

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] device_tree: Add a helper function for string arrays
  2019-11-08 19:47 [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
@ 2019-11-08 19:47 ` Palmer Dabbelt
  2019-11-09 15:59   ` Peter Maydell
  2019-11-08 19:47 ` [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings() Palmer Dabbelt
  2019-11-08 19:47 ` [PATCH v2 3/3] RISC-V: virt: This is a "sifive,test1" test finisher Palmer Dabbelt
  2 siblings, 1 reply; 8+ messages in thread
From: Palmer Dabbelt @ 2019-11-08 19:47 UTC (permalink / raw)
  To: Peter Maydell, Alistair Francis, qemu-devel, david
  Cc: Palmer Dabbelt, qemu-riscv

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings()
  2019-11-08 19:47 [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
  2019-11-08 19:47 ` [PATCH v2 1/3] device_tree: Add a helper function for string arrays Palmer Dabbelt
@ 2019-11-08 19:47 ` Palmer Dabbelt
  2019-11-09 15:56   ` Peter Maydell
  2019-11-08 19:47 ` [PATCH v2 3/3] RISC-V: virt: This is a "sifive,test1" test finisher Palmer Dabbelt
  2 siblings, 1 reply; 8+ messages in thread
From: Palmer Dabbelt @ 2019-11-08 19:47 UTC (permalink / raw)
  To: Peter Maydell, Alistair Francis, qemu-devel, david
  Cc: Palmer Dabbelt, qemu-riscv

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] RISC-V: virt: This is a "sifive,test1" test finisher
  2019-11-08 19:47 [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
  2019-11-08 19:47 ` [PATCH v2 1/3] device_tree: Add a helper function for string arrays Palmer Dabbelt
  2019-11-08 19:47 ` [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings() Palmer Dabbelt
@ 2019-11-08 19:47 ` Palmer Dabbelt
  2 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2019-11-08 19:47 UTC (permalink / raw)
  To: Peter Maydell, Alistair Francis, qemu-devel, david
  Cc: Palmer Dabbelt, qemu-riscv

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings()
  2019-11-08 19:47 ` [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings() Palmer Dabbelt
@ 2019-11-09 15:56   ` Peter Maydell
  2019-11-10 19:47     ` David Gibson
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2019-11-09 15:56 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: open list:RISC-V, Alistair Francis, QEMU Developers, David Gibson

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/3] device_tree: Add a helper function for string arrays
  2019-11-08 19:47 ` [PATCH v2 1/3] device_tree: Add a helper function for string arrays Palmer Dabbelt
@ 2019-11-09 15:59   ` Peter Maydell
  2019-11-10 19:44     ` David Gibson
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2019-11-09 15:59 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: open list:RISC-V, Alistair Francis, QEMU Developers, David Gibson

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/3] device_tree: Add a helper function for string arrays
  2019-11-09 15:59   ` Peter Maydell
@ 2019-11-10 19:44     ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2019-11-10 19:44 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Alistair Francis, Palmer Dabbelt, QEMU Developers, open list:RISC-V

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings()
  2019-11-09 15:56   ` Peter Maydell
@ 2019-11-10 19:47     ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2019-11-10 19:47 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Alistair Francis, Palmer Dabbelt, QEMU Developers, open list:RISC-V

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 19:47 [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
2019-11-08 19:47 ` [PATCH v2 1/3] device_tree: Add a helper function for string arrays Palmer Dabbelt
2019-11-09 15:59   ` Peter Maydell
2019-11-10 19:44     ` David Gibson
2019-11-08 19:47 ` [PATCH v2 2/3] ARM/virt: Use fdt_setprop_strings() Palmer Dabbelt
2019-11-09 15:56   ` Peter Maydell
2019-11-10 19:47     ` David Gibson
2019-11-08 19:47 ` [PATCH v2 3/3] RISC-V: virt: This is a "sifive,test1" test finisher Palmer Dabbelt

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git