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

Palmer Dabbelt posted 1 patch 1 week ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191107222500.8018-1-palmer@sifive.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
hw/riscv/virt.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

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

Posted by Palmer Dabbelt 1 week ago
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.  I copied the odd idiom for adding a
two-string compatible field from the ARM virt board.

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

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 23f340df19..74f2dce81c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -359,7 +359,10 @@ 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");
+    {
+        const char compat[] = "sifive,test1\0sifive,test0";
+        qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
+    }
     qemu_fdt_setprop_cells(fdt, nodename, "reg",
         0x0, memmap[VIRT_TEST].base,
         0x0, memmap[VIRT_TEST].size);
-- 
2.21.0


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

Posted by Alistair Francis 6 days ago
On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> 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.  I copied the odd idiom for adding a
> two-string compatible field from the ARM virt board.
>
> Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality")
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  hw/riscv/virt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 23f340df19..74f2dce81c 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -359,7 +359,10 @@ 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");
> +    {
> +        const char compat[] = "sifive,test1\0sifive,test0";

Does this really work? Why not use qemu_fdt_setprop_cells()?

Alistair

> +        qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
> +    }
>      qemu_fdt_setprop_cells(fdt, nodename, "reg",
>          0x0, memmap[VIRT_TEST].base,
>          0x0, memmap[VIRT_TEST].size);
> --
> 2.21.0
>
>

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

Posted by Peter Maydell 6 days ago
On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > 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.  I copied the odd idiom for adding a
> > two-string compatible field from the ARM virt board.
> >
> > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality")
> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > ---
> >  hw/riscv/virt.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 23f340df19..74f2dce81c 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -359,7 +359,10 @@ 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");
> > +    {
> > +        const char compat[] = "sifive,test1\0sifive,test0";
>
> Does this really work? Why not use qemu_fdt_setprop_cells()?
>
> Alistair
>
> > +        qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
> > +    }

qemu_fdt_setprop_cells() is for "set this property to
contain this list of 32-bit integers" (and it does a byteswap
of each 32-bit value from host to BE). That's not what
you want for a string (or a string list, which is what
we have here).

Cc'ing David Gibson who's our device tree expert to see if there's
a nicer way to write this. Oddly, given that it's used in the
ubiquitous 'compatible' prop, the dtc Documentation/manual.txt
doesn't say anything about properties being able to be
'string lists', only 'strings', '32 bit numbers', 'lists of
32-bit numbers' and 'byte sequences'. You have to dig through
the header file comments to deduce that a string list is
represented by a string with embedded NULs separating
each list item.

thanks
-- PMM

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

Posted by David Gibson 4 days ago
On Fri, Nov 08, 2019 at 06:04:47PM +0000, Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> > >
> > > 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.  I copied the odd idiom for adding a
> > > two-string compatible field from the ARM virt board.
> > >
> > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality")
> > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > ---
> > >  hw/riscv/virt.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > index 23f340df19..74f2dce81c 100644
> > > --- a/hw/riscv/virt.c
> > > +++ b/hw/riscv/virt.c
> > > @@ -359,7 +359,10 @@ 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");
> > > +    {
> > > +        const char compat[] = "sifive,test1\0sifive,test0";
> >
> > Does this really work? Why not use qemu_fdt_setprop_cells()?
> >
> > Alistair
> >
> > > +        qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
> > > +    }
> 
> qemu_fdt_setprop_cells() is for "set this property to
> contain this list of 32-bit integers" (and it does a byteswap
> of each 32-bit value from host to BE). That's not what
> you want for a string (or a string list, which is what
> we have here).

Yes.

> Cc'ing David Gibson who's our device tree expert to see if there's
> a nicer way to write this.

Not amongst the qemu wrappers AFAIK.  With raw libfdt you could build
it up in stages using fdt_setprop_string() followed by
fdt_appendprop_string(), although that would require additional
memmove()s to complete.

A stringlist setprop function using varargs might be a useful addition
to libfdt.  The tricky bit is that libfdt can be used in environments
with no allocator, which makes implementation tricky.

> Oddly, given that it's used in the
> ubiquitous 'compatible' prop, the dtc Documentation/manual.txt
> doesn't say anything about properties being able to be
> 'string lists', only 'strings', '32 bit numbers', 'lists of
> 32-bit numbers' and 'byte sequences'.

Oof, yeah that manual hasn't really been updated for ages.  That
really only describes the pieces from which a property can be mode.
You can then concatenate those together using ,.  So in .dts you can
make a string list as:
	compatible = "foo", "bar";

The , is effectively a bytestring append operator.

> You have to dig through
> the header file comments to deduce that a string list is
> represented by a string with embedded NULs separating
> each list item.

Well, you can do that as well, but using commas is more common in
modern dts files.  The two are equivalent by construction (in .dtb
properties are simply bytestrings).

-- 
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: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher

Posted by Palmer Dabbelt 6 days ago
On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>> >
>> > 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.  I copied the odd idiom for adding a
>> > two-string compatible field from the ARM virt board.
>> >
>> > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality")
>> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> > ---
>> >  hw/riscv/virt.c | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> > index 23f340df19..74f2dce81c 100644
>> > --- a/hw/riscv/virt.c
>> > +++ b/hw/riscv/virt.c
>> > @@ -359,7 +359,10 @@ 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");
>> > +    {
>> > +        const char compat[] = "sifive,test1\0sifive,test0";
>>
>> Does this really work? Why not use qemu_fdt_setprop_cells()?
>>
>> Alistair
>>
>> > +        qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
>> > +    }
>
> qemu_fdt_setprop_cells() is for "set this property to
> contain this list of 32-bit integers" (and it does a byteswap
> of each 32-bit value from host to BE). That's not what
> you want for a string (or a string list, which is what
> we have here).
>
> Cc'ing David Gibson who's our device tree expert to see if there's
> a nicer way to write this. Oddly, given that it's used in the
> ubiquitous 'compatible' prop, the dtc Documentation/manual.txt
> doesn't say anything about properties being able to be
> 'string lists', only 'strings', '32 bit numbers', 'lists of
> 32-bit numbers' and 'byte sequences'. You have to dig through
> the header file comments to deduce that a string list is
> represented by a string with embedded NULs separating
> each list item.

I copied this from hw/arm/virt.c, but messed up.  There they use

        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
        qemu_fdt_setprop(vms->fdt, "/timer", "compatible",
                         compat, sizeof(compat));

I'll send a v2, but I'd be happy to add some sort of setprop_stringlist 
function.  Maybe we just indicate the length with two '\0's?  IIRC that's how 
other similar-looking data structures are encoded.

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

Posted by David Gibson 4 days ago
On Fri, Nov 08, 2019 at 10:13:16AM -0800, Palmer Dabbelt wrote:
> On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote:
> > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote:
> > > 
> > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> > > >
> > > > 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.  I copied the odd idiom for adding a
> > > > two-string compatible field from the ARM virt board.
> > > >
> > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality")
> > > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > > ---
> > > >  hw/riscv/virt.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > > index 23f340df19..74f2dce81c 100644
> > > > --- a/hw/riscv/virt.c
> > > > +++ b/hw/riscv/virt.c
> > > > @@ -359,7 +359,10 @@ 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");
> > > > +    {
> > > > +        const char compat[] = "sifive,test1\0sifive,test0";
> > > 
> > > Does this really work? Why not use qemu_fdt_setprop_cells()?
> > > 
> > > Alistair
> > > 
> > > > +        qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
> > > > +    }
> > 
> > qemu_fdt_setprop_cells() is for "set this property to
> > contain this list of 32-bit integers" (and it does a byteswap
> > of each 32-bit value from host to BE). That's not what
> > you want for a string (or a string list, which is what
> > we have here).
> > 
> > Cc'ing David Gibson who's our device tree expert to see if there's
> > a nicer way to write this. Oddly, given that it's used in the
> > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt
> > doesn't say anything about properties being able to be
> > 'string lists', only 'strings', '32 bit numbers', 'lists of
> > 32-bit numbers' and 'byte sequences'. You have to dig through
> > the header file comments to deduce that a string list is
> > represented by a string with embedded NULs separating
> > each list item.
> 
> I copied this from hw/arm/virt.c, but messed up.  There they use
> 
>        const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
>        qemu_fdt_setprop(vms->fdt, "/timer", "compatible",
>                         compat, sizeof(compat));

I'm not sure what you're saying is messed up.  AFAICT, this matches
the code you have above, and both should be correct.

> I'll send a v2, but I'd be happy to add some sort of setprop_stringlist
> function.  Maybe we just indicate the length with two '\0's?  IIRC that's
> how other similar-looking data structures are encoded.
> 

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