Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Michael Clark]

On Sat, Mar 10, 2018 at 9:11 AM, Michael Clark  wrote:

>
>
> On Sat, Mar 10, 2018 at 5:49 AM, Peter Maydell 
> wrote:
>
>> On 9 March 2018 at 14:28, Peter Maydell  wrote:
>> > NB: there was a test failure on OpenBSD host:
>> >
>> > TEST: tests/qom-test... (pid=64016)
>> >   /riscv32/qom/spike_v1.9.1:
>>  **
>> > ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion
>> > failed: (qdict_haskey(response, "return"))
>> > FAIL
>> >
>> > but this seems to have been intermittent -- it was only on that one
>> > host, and I reran the test suite there and it passed fine the second
>> > time. So it may be nothing to do with your code; we'll see if it
>> > comes up again.
>>
>> On a later test run I got this different one; openbsd again:
>>
>> TEST: tests/test-hmp... (pid=45236)
>>   /riscv32/hmp/spike_v1.9.1:   **
>> ERROR:/home/qemu/qom/object.c:488:object_new_with_type: assertion
>> failed: (type != NULL)
>> Broken pipe
>> FAIL
>>
>> My current best theory is that OpenBSD libc's memory allocator
>> happens to be more sensitive to a memory corruption bug in the risc
>> code, resulting in intermittent failures if the allocations happen
>> to come out the wrong way. You do have at least one invalid-write
>> off the end of a block according to valgrind:
>>
>> ==17441== Invalid write of size 1
>> ==17441==at 0x26517F: riscv_isa_string (cpu.c:399)
>> ==17441==by 0x25C14D: create_fdt (spike.c:125)
>> ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199)
>> ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807)
>> ==17441==by 0x1BFF28: main (vl.c:4597)
>> ==17441==  Address 0x3055c425 is 0 bytes after a block of size 5 alloc'd
>> ==17441==at 0x4C2FB55: calloc (in
>> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==17441==by 0x70C8770: g_malloc0 (in
>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
>> ==17441==by 0x26511E: riscv_isa_string (cpu.c:395)
>> ==17441==by 0x25C14D: create_fdt (spike.c:125)
>> ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199)
>> ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807)
>> ==17441==by 0x1BFF28: main (vl.c:4597)
>>
>> If you can prioritise a patch that fixes the bug in riscv_isa_string()
>> I'll apply that and hopefully these intermittent failures will go away.
>>
>
> I'm looking at this right now.
>

It's a glaringly obvious logic bug. The use of the wrong bit manipulation
instrinsic. I just sent a patch.

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Michael Clark]

On Sat, Mar 10, 2018 at 5:49 AM, Peter Maydell 
wrote:

> On 9 March 2018 at 14:28, Peter Maydell  wrote:
> > NB: there was a test failure on OpenBSD host:
> >
> > TEST: tests/qom-test... (pid=64016)
> >   /riscv32/qom/spike_v1.9.1:   **
> > ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion
> > failed: (qdict_haskey(response, "return"))
> > FAIL
> >
> > but this seems to have been intermittent -- it was only on that one
> > host, and I reran the test suite there and it passed fine the second
> > time. So it may be nothing to do with your code; we'll see if it
> > comes up again.
>
> On a later test run I got this different one; openbsd again:
>
> TEST: tests/test-hmp... (pid=45236)
>   /riscv32/hmp/spike_v1.9.1:   **
> ERROR:/home/qemu/qom/object.c:488:object_new_with_type: assertion
> failed: (type != NULL)
> Broken pipe
> FAIL
>
> My current best theory is that OpenBSD libc's memory allocator
> happens to be more sensitive to a memory corruption bug in the risc
> code, resulting in intermittent failures if the allocations happen
> to come out the wrong way. You do have at least one invalid-write
> off the end of a block according to valgrind:
>
> ==17441== Invalid write of size 1
> ==17441==at 0x26517F: riscv_isa_string (cpu.c:399)
> ==17441==by 0x25C14D: create_fdt (spike.c:125)
> ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199)
> ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807)
> ==17441==by 0x1BFF28: main (vl.c:4597)
> ==17441==  Address 0x3055c425 is 0 bytes after a block of size 5 alloc'd
> ==17441==at 0x4C2FB55: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==17441==by 0x70C8770: g_malloc0 (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
> ==17441==by 0x26511E: riscv_isa_string (cpu.c:395)
> ==17441==by 0x25C14D: create_fdt (spike.c:125)
> ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199)
> ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807)
> ==17441==by 0x1BFF28: main (vl.c:4597)
>
> If you can prioritise a patch that fixes the bug in riscv_isa_string()
> I'll apply that and hopefully these intermittent failures will go away.
>

I'm looking at this right now.

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Peter Maydell]

On 9 March 2018 at 14:28, Peter Maydell  wrote:
> NB: there was a test failure on OpenBSD host:
>
> TEST: tests/qom-test... (pid=64016)
>   /riscv32/qom/spike_v1.9.1:   **
> ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion
> failed: (qdict_haskey(response, "return"))
> FAIL
>
> but this seems to have been intermittent -- it was only on that one
> host, and I reran the test suite there and it passed fine the second
> time. So it may be nothing to do with your code; we'll see if it
> comes up again.

On a later test run I got this different one; openbsd again:

TEST: tests/test-hmp... (pid=45236)
  /riscv32/hmp/spike_v1.9.1:   **
ERROR:/home/qemu/qom/object.c:488:object_new_with_type: assertion
failed: (type != NULL)
Broken pipe
FAIL

My current best theory is that OpenBSD libc's memory allocator
happens to be more sensitive to a memory corruption bug in the risc
code, resulting in intermittent failures if the allocations happen
to come out the wrong way. You do have at least one invalid-write
off the end of a block according to valgrind:

==17441== Invalid write of size 1
==17441==at 0x26517F: riscv_isa_string (cpu.c:399)
==17441==by 0x25C14D: create_fdt (spike.c:125)
==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199)
==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807)
==17441==by 0x1BFF28: main (vl.c:4597)
==17441==  Address 0x3055c425 is 0 bytes after a block of size 5 alloc'd
==17441==at 0x4C2FB55: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17441==by 0x70C8770: g_malloc0 (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==17441==by 0x26511E: riscv_isa_string (cpu.c:395)
==17441==by 0x25C14D: create_fdt (spike.c:125)
==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199)
==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807)
==17441==by 0x1BFF28: main (vl.c:4597)

If you can prioritise a patch that fixes the bug in riscv_isa_string()
I'll apply that and hopefully these intermittent failures will go away.

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Peter Maydell]

On 9 March 2018 at 15:15, Alex Bennée  wrote:
>
> Michael Clark  writes:
>
> 
>>
>> BTW - I've integrated the following 3 branches into the riscv tree:
>>
>> - https://github.com/riscv/riscv-qemu/tree/softfloat-snan-abort-fix
>> - https://github.com/riscv/riscv-qemu/tree/riscv-qemu-upstream-v8.2
>> - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel
>>
>> into our `riscv-all` integration branch, and we're now passing all FPU
>> tests, interestingly, including the NaN-boxing of single precision values
>> into doubles. We'll need to check that the riscv-tests testsuite is
>> exhastive enough... Suprised! I think Richard might have thought about
>> our NaN-boxing
>> issue or some other sort of magic is going on :-)
>>
>> - https://github.com/riscv/riscv-qemu/tree/riscv-all
>
> Is the SNaN patch going to get re-posted now it has had a review?

I'm just in the process of applying that to master now...

-- PMM

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Alex Bennée]

Michael Clark  writes:


>
> BTW - I've integrated the following 3 branches into the riscv tree:
>
> - https://github.com/riscv/riscv-qemu/tree/softfloat-snan-abort-fix
> - https://github.com/riscv/riscv-qemu/tree/riscv-qemu-upstream-v8.2
> - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel
>
> into our `riscv-all` integration branch, and we're now passing all FPU
> tests, interestingly, including the NaN-boxing of single precision values
> into doubles. We'll need to check that the riscv-tests testsuite is
> exhastive enough... Suprised! I think Richard might have thought about
> our NaN-boxing
> issue or some other sort of magic is going on :-)
>
> - https://github.com/riscv/riscv-qemu/tree/riscv-all

Is the SNaN patch going to get re-posted now it has had a review?

--
Alex Bennée

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Michael Clark]

On Sat, Mar 10, 2018 at 3:28 AM, Peter Maydell 
wrote:

> On 8 March 2018 at 19:53, Michael Clark  wrote:
> > I re-iterate Palmer's apology.
> >
> > I shouldn't be polling git.qemu.org/qemu.git and answering emails near
> to
> > 3am in the morning after 4 months of working on trying to get the RISC-V
> > port in shape to go upstream.
> >
> > It appears it is completely my mistake and I had tagged early deltas on
> top
> > of v8.2 instead of the tip of v8.2.
> >
> > I've force pushed the 'riscv-qemu-upstream-v8.2' so only the mailing list
> > will hold the history of my mistake.
>
> Thank you for the apology. On my side, I regret not starting this
> email thread by just asking if you'd pushed the wrong tag by mistake,
> since in retrospect that was certainly the most likely situation.
>

No worries. It was very late at night. I was tired and a little anxious.
Thanks very much for bearing with me after my comments.


> I've now merged and tested the revised tag, and pushed it upstream.
>

That's great news.


> NB: there was a test failure on OpenBSD host:
>
> TEST: tests/qom-test... (pid=64016)
>   /riscv32/qom/spike_v1.9.1:   **
> ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion
> failed: (qdict_haskey(response, "return"))
> FAIL
>
> but this seems to have been intermittent -- it was only on that one
> host, and I reran the test suite there and it passed fine the second
> time. So it may be nothing to do with your code; we'll see if it
> comes up again.
>
> I also had a look at running the port under valgrind, which shows
> what looks like a bug in riscv_isa_string():
>
> $ valgrind ./build/all/riscv32-softmmu/qemu-system-riscv32
> [...]
> ==24805== Invalid read of size 1
> ==24805==at 0x4C30F74: strlen (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==24805==by 0x26518E: riscv_isa_string (cpu.c:399)
> ==24805==by 0x25C15D: create_fdt (spike.c:125)
> ==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199)
> ==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807)
> ==24805==by 0x1BFF28: main (vl.c:4597)
> ==24805==  Address 0x3055be55 is 0 bytes after a block of size 5 alloc'd
> ==24805==at 0x4C2FB55: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==24805==by 0x70C8770: g_malloc0 (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
> ==24805==by 0x26512E: riscv_isa_string (cpu.c:395)
> ==24805==by 0x25C15D: create_fdt (spike.c:125)
> ==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199)
> ==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807)
> ==24805==by 0x1BFF28: main (vl.c:4597)
>
> I haven't looked too hard at the code, but I suspect you're
> miscalculating the length of the string and/or not writing the
> trailing NUL to the string. I recommend you have a look at that,
> and perhaps try running some other tests under valgrind.
>

Thanks very much!

I'll valgrind locally when I get time. It's very late at night here. 3.45am.

We may very well have a memory issue but it's likely restricted to the
RISC-V port and shouldn't cause any issues for other ports.

BTW - I've integrated the following 3 branches into the riscv tree:

- https://github.com/riscv/riscv-qemu/tree/softfloat-snan-abort-fix
- https://github.com/riscv/riscv-qemu/tree/riscv-qemu-upstream-v8.2
- https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel

into our `riscv-all` integration branch, and we're now passing all FPU
tests, interestingly, including the NaN-boxing of single precision values
into doubles. We'll need to check that the riscv-tests testsuite is
exhastive enough... Suprised! I think Richard might have thought about
our NaN-boxing
issue or some other sort of magic is going on :-)

- https://github.com/riscv/riscv-qemu/tree/riscv-all

Michael.
--

$ sh qemu-images/run-riscv-tests.sh
rv64ua-v-amoadd_d
rv64ua-v-amoadd_w
rv64ua-v-amoand_d
rv64ua-v-amoand_w
rv64ua-v-amomax_d
rv64ua-v-amomax_w
rv64ua-v-amomaxu_d
rv64ua-v-amomaxu_w
rv64ua-v-amomin_d
rv64ua-v-amomin_w
rv64ua-v-amominu_d
rv64ua-v-amominu_w
rv64ua-v-amoor_d
rv64ua-v-amoor_w
rv64ua-v-amoswap_d
rv64ua-v-amoswap_w
rv64ua-v-amoxor_d
rv64ua-v-amoxor_w
rv64ua-v-lrsc
rv64uc-v-rvc
rv64ud-v-fadd
rv64ud-v-fclass
rv64ud-v-fcmp
rv64ud-v-fcvt
rv64ud-v-fcvt_w
rv64ud-v-fdiv
rv64ud-v-fmadd
rv64ud-v-fmin
rv64ud-v-ldst
rv64ud-v-move
rv64ud-v-recoding
rv64ud-v-structural
rv64uf-v-fadd
rv64uf-v-fclass
rv64uf-v-fcmp
rv64uf-v-fcvt
rv64uf-v-fcvt_w
rv64uf-v-fdiv
rv64uf-v-fmadd
rv64uf-v-fmin
rv64uf-v-ldst
rv64uf-v-move
rv64uf-v-recoding
rv64ui-v-add
rv64ui-v-addi
rv64ui-v-addiw
rv64ui-v-addw
rv64ui-v-and
rv64ui-v-andi
rv64ui-v-auipc
rv64ui-v-beq
rv64ui-v-bge
rv64ui-v-bgeu
rv64ui-v-blt
rv64ui-v-bltu
rv64ui-v-bne
rv64ui-v-fence_i
rv64ui-v-jal
rv64ui-v-jalr
rv64ui-v-lb
rv64ui-v-lbu
rv64ui-v-ld
rv64ui-v-lh
rv64ui-v-lhu
rv64ui-v-lui
rv64ui-v-lw
rv64ui-v-lwu
rv64ui-v-or
rv64ui-v-ori

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Peter Maydell]

On 8 March 2018 at 19:53, Michael Clark  wrote:
> I re-iterate Palmer's apology.
>
> I shouldn't be polling git.qemu.org/qemu.git and answering emails near to
> 3am in the morning after 4 months of working on trying to get the RISC-V
> port in shape to go upstream.
>
> It appears it is completely my mistake and I had tagged early deltas on top
> of v8.2 instead of the tip of v8.2.
>
> I've force pushed the 'riscv-qemu-upstream-v8.2' so only the mailing list
> will hold the history of my mistake.

Thank you for the apology. On my side, I regret not starting this
email thread by just asking if you'd pushed the wrong tag by mistake,
since in retrospect that was certainly the most likely situation.

I've now merged and tested the revised tag, and pushed it upstream.

NB: there was a test failure on OpenBSD host:

TEST: tests/qom-test... (pid=64016)
  /riscv32/qom/spike_v1.9.1:   **
ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion
failed: (qdict_haskey(response, "return"))
FAIL

but this seems to have been intermittent -- it was only on that one
host, and I reran the test suite there and it passed fine the second
time. So it may be nothing to do with your code; we'll see if it
comes up again.

I also had a look at running the port under valgrind, which shows
what looks like a bug in riscv_isa_string():

$ valgrind ./build/all/riscv32-softmmu/qemu-system-riscv32
[...]
==24805== Invalid read of size 1
==24805==at 0x4C30F74: strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24805==by 0x26518E: riscv_isa_string (cpu.c:399)
==24805==by 0x25C15D: create_fdt (spike.c:125)
==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199)
==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807)
==24805==by 0x1BFF28: main (vl.c:4597)
==24805==  Address 0x3055be55 is 0 bytes after a block of size 5 alloc'd
==24805==at 0x4C2FB55: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24805==by 0x70C8770: g_malloc0 (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==24805==by 0x26512E: riscv_isa_string (cpu.c:395)
==24805==by 0x25C15D: create_fdt (spike.c:125)
==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199)
==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807)
==24805==by 0x1BFF28: main (vl.c:4597)

I haven't looked too hard at the code, but I suspect you're
miscalculating the length of the string and/or not writing the
trailing NUL to the string. I recommend you have a look at that,
and perhaps try running some other tests under valgrind.

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Michael Clark]

On Fri, Mar 9, 2018 at 8:29 AM, Palmer Dabbelt  wrote:

> On Thu, 08 Mar 2018 03:41:33 PST (-0800), Michael Clark wrote:
>
>> On Fri, 9 Mar 2018 at 12:18 AM, Michael Clark  wrote:
>>
>>> On Fri, 9 Mar 2018 at 12:10 AM, Michael Clark  wrote:
>>>
 On Thu, 8 Mar 2018 at 11:02 PM, Peter Maydell 
 wrote:

> On 6 March 2018 at 19:46, Michael Clark  wrote:
>
 You are making this very hard. Do you work for Arm perchance? I really
>>> wouldn’t be surprised if our port is being sandbagged by Arm. Apologies
>>> for
>>> being so direct about this, but things like this happen...
>>>
>>> I have complied with practically every review request and the sign-offs
>>> are there. It’s a bit ridiculous.
>>>
>>> It would be nice to find someone neutral, unrelated to Arm, to merge our
>>> PR
>>>
>>>
>> Some history on the origins of RISC to put things in perspective:
>>
>> https://en.m.wikipedia.org/wiki/Berkeley_RISC
>>
>> David Patterson worked with Andrew Waterman and Krste Asanovic on the
>> design of RISC-V. Sagar did most of the work on the QEMU port and he
>> agreeded to sign off on all patches. The SiFive patches only have
>> sign-offs
>> from SiFive because SiFive was the sole contributor for its hardware
>> model,
>> beside the SiFiveUART which has Stefan’s sign-off.
>>
>> In any case it seems there is not enough review bandwidth in the QEMU
>> project as a whole and the policy to accept contributions is too strict to
>> be reasonable, given earnest attempts to comply with *all* review
>> feedback.
>> Not impressed.
>>
>
> On behalf of the rest of the RISC-V QEMU team I'd like to apologize for
> Michael's comments.  That's a pretty insulting thing to say, and the whole
> thing comes off as a bit entitled: we've asked the QEMU community to do a
> lot of work for us in reviewing our port, and seeing as how none of us are
> QEMU contributors we certainly don't have any grounds to ask someone to
> stop reviewing it -- that's pretty absurd.
>
> While I haven't been following the upstreaming process as closely as I
> should have been, as far as I can tell there's no grounds to accuse Peter,
> or anyone else, of trying to shoot down our port for any reason.  Peter, I
> can understand if you're upset, as I certainly would be.  If you don't want
> to help out with our port any more then I can understand, but I'd just like
> to assure you that we value the time you've spent on our port and hope you
> continue to help out!
>
> Hopefully this doesn't derail our chances of moving forward with
> submitting the RISC-V port upstream.
>
> Sorry!
>

I re-iterate Palmer's apology.

I shouldn't be polling git.qemu.org/qemu.git and answering emails near
to 3am in the morning after 4 months of working on trying to get the RISC-V
port in shape to go upstream.

It appears it is completely my mistake and I had tagged early deltas on top
of v8.2 instead of the tip of v8.2.

I've force pushed the 'riscv-qemu-upstream-v8.2' so only the mailing list
will hold the history of my mistake.

$ git push -f --tags
Counting objects: 1, done.
Writing objects: 100% (1/1), 6.31 KiB | 0 bytes/s, done.
Total 1 (delta 0), reused 0 (delta 0)
To g...@github.com:riscv/riscv-qemu.git
 + feb8f5e...2c7f042 riscv-qemu-upstream-v8.2 -> riscv-qemu-upstream-v8.2
(forced update)

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2

[Palmer Dabbelt]

On Thu, 08 Mar 2018 03:41:33 PST (-0800), Michael Clark wrote:

On Fri, 9 Mar 2018 at 12:18 AM, Michael Clark  wrote:

On Fri, 9 Mar 2018 at 12:10 AM, Michael Clark  wrote:

On Thu, 8 Mar 2018 at 11:02 PM, Peter Maydell 
wrote:

On 6 March 2018 at 19:46, Michael Clark  wrote:

You are making this very hard. Do you work for Arm perchance? I really
wouldn’t be surprised if our port is being sandbagged by Arm. Apologies for
being so direct about this, but things like this happen...

I have complied with practically every review request and the sign-offs
are there. It’s a bit ridiculous.

It would be nice to find someone neutral, unrelated to Arm, to merge our PR



Some history on the origins of RISC to put things in perspective:

https://en.m.wikipedia.org/wiki/Berkeley_RISC

David Patterson worked with Andrew Waterman and Krste Asanovic on the
design of RISC-V. Sagar did most of the work on the QEMU port and he
agreeded to sign off on all patches. The SiFive patches only have sign-offs
from SiFive because SiFive was the sole contributor for its hardware model,
beside the SiFiveUART which has Stefan’s sign-off.

In any case it seems there is not enough review bandwidth in the QEMU
project as a whole and the policy to accept contributions is too strict to
be reasonable, given earnest attempts to comply with *all* review feedback.
Not impressed.


On behalf of the rest of the RISC-V QEMU team I'd like to apologize for 
Michael's comments.  That's a pretty insulting thing to say, and the whole 
thing comes off as a bit entitled: we've asked the QEMU community to do a lot 
of work for us in reviewing our port, and seeing as how none of us are QEMU 
contributors we certainly don't have any grounds to ask someone to stop 
reviewing it -- that's pretty absurd.


While I haven't been following the upstreaming process as closely as I should 
have been, as far as I can tell there's no grounds to accuse Peter, or anyone 
else, of trying to shoot down our port for any reason.  Peter, I can understand 
if you're upset, as I certainly would be.  If you don't want to help out with 
our port any more then I can understand, but I'd just like to assure you that 
we value the time you've spent on our port and hope you continue to help out!


Hopefully this doesn't derail our chances of moving forward with submitting the 
RISC-V port upstream.


Sorry!

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Tue, Mar 6, 2018 at 8:00 AM, Emilio G. Cota  wrote:

> On Sat, Mar 03, 2018 at 02:26:12 +1300, Michael Clark wrote:
> > It was qemu-2.7.50 (late 2016). The benchmarks were generated mid last
> year.
> >
> > I can run the benchmarks again... Has it doubled in speed?
>
> It depends on the benchmarks. Small-ish benchmarks such as rv8-bench
> show about a 1.5x speedup since QEMU v2.6.0 for Aarch64:
>
> Aarch64 rv8-bench performance under QEMU user-mode
>   Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
>
>   4.5 +-++--+--+--+--+--+--+--+---
> ---++-+
>   | ++
>   |
> 4 +-+..v2.8.0.v2.9.0v2.10.0.%%.
> v2.11.0+-+
>   3.5 +-+...%%@...
> +-+
>   | %%@
>  |
> 3 +-+...%%@...
> +-+
>   2.5 +-+...%%@...
> +-+
>   | ++$$$%@
>  |
> 2 +-+.$$$%@..##.$%@...
> +-+
>   |  ##+$%@ ##$$%@   ## $%@
>  |
>   1.5 +-+..+++%%@...**#.$%@.##.$%@%%@##.$%@...
> .##$$%@.+-+
> 1 +-+.**#$$%@+##$$%@**#.$%@**#.$%@**#$$%@**#$$%@**#.$%@**#$$%@
> **#.$%@.+-+
>   |   **# $%@**# $%@**# $%@**# $%@**# $%@**#+$%@**# $%@**# $%@**# $%@
>  |
>   0.5 +-+-**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@
> **#$$%@-+-+
>   aes bigidhrystone  miniz   norx primes  qsort sha512geomean
>   png: https://imgur.com/Agr5CJd
>
> SPEC06int shows a larger improvement, up to ~2x avg speedup for the train
> set:
>   Aarch64 SPEC06int (train set) performance under QEMU user-mode
>   Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
>
> 4 +-+--+++++++++++
> ++--+-+
>   |%%   ++
>   |
>   3.5 +-+..%%@.v2.8.0.v2.9.0v2.10.0.%%+
> v2.11.0+-+
>   |%%@  %%@   ++
>   |
> 3 +-+..%%@...++.%%@...
> %%+.+-+
>   |   +$%@|+%%@   %%@
>  |
>   2.5 +-+.##%@...%%@...+$%@...
> %%@.+-+
> 2 +-+.##%@..+%%@...%%@.%%@.##%@...
> %%@..++.+-+
>   |   ##%@  %%@ ##%@  +$%@   %%@   %%@ ##%@   $%@
> %%@  |
>   1.5 +-+**#%@.##%@.##%@..##%@...$%@...$%@.##%@..%%+.#
> #%@.##%@+-+
>   |  **#%@**#%@**#%@  +++**#%@  ##%@  ++  ##%@**#%@ ##%@ ##%@
> ##%@  |
> 1 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@+##%@**#%@+##%@**
> #%@**#%@+-+
>   |  **#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@
> |
>   0.5 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**
> #%@**#%@+-+
>401.bzi403.g429445.g456.h462.libq464.h471.omn4483.xalancbgeomean
>   png: https://imgur.com/JknVT5H
>
> Note that the test set is less sensitive to the changes:
>   https://imgur.com/W7CT0eO
>
> Running small benchmarks (such as SPEC "test" or rv8-bench) is
> very useful to get quick feedback on optimizations. However, some
> of these runs are still dominated by parts of the code that aren't
> that relevant -- for instance, some of them take so little time to
> run that the major contributor to execution time is memory allocation.
> Therefore, when publishing results it's best to stick with larger
> benchmarks that run for longer (e.g. SPEC "train" set), which are more
> sensitive to DBT performance.
>
> I tried running some other benchmarks, such as nbench[1], under rv-jit.
> I quickly get a "bus error" though -- don't know if I'm doing anything
> wrong, or maybe compiling with the glibc cross-compiler I used
> to build riscv linux isn't supported.
> I managed though to run rv8-bench on both rv-jit and qemu (v8 patchset);
> rv-jit is 1.30x faster on average for those, although note I dropped
> qsort because it wasn't working properly on rv-jit:
>

That's interesting. I know from some analysis that the current slow-down in
rv8 is mostly from accessing statically spilled registers (which in many
cases we embed in x86 memory operands to keep up code density, and make use
of the instruction cracker and uop cache in Intel's front-end). The
slowdown is mostly L1 cache latency vs register access latency given we are
emulating 31 registers on a 16 register host with a static register
allocation (based on the compiler register allocation order which optimizes
for the RVC accessible registers). With the addition of a register
allocator, I am sure I can make rv8 substantially faster. perhaps 1.7x

The user-mode emulation in rv8 is very limited, and 

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Emilio G. Cota]

On Sat, Mar 03, 2018 at 02:26:12 +1300, Michael Clark wrote:
> It was qemu-2.7.50 (late 2016). The benchmarks were generated mid last year.
> 
> I can run the benchmarks again... Has it doubled in speed?

It depends on the benchmarks. Small-ish benchmarks such as rv8-bench
show about a 1.5x speedup since QEMU v2.6.0 for Aarch64:

Aarch64 rv8-bench performance under QEMU user-mode  
  Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz

  4.5 +-++--+--+--+--+--+--+--+--++-+   
  | ++  |   
4 +-+..v2.8.0.v2.9.0v2.10.0.%%.v2.11.0+-+   
  3.5 +-+...%%@...+-+   
  | %%@ |   
3 +-+...%%@...+-+   
  2.5 +-+...%%@...+-+   
  | ++$$$%@ |   
2 +-+.$$$%@..##.$%@...+-+   
  |  ##+$%@ ##$$%@   ## $%@ |   
  1.5 +-+..+++%%@...**#.$%@.##.$%@%%@##.$%@##$$%@.+-+   
1 +-+.**#$$%@+##$$%@**#.$%@**#.$%@**#$$%@**#$$%@**#.$%@**#$$%@**#.$%@.+-+   
  |   **# $%@**# $%@**# $%@**# $%@**# $%@**#+$%@**# $%@**# $%@**# $%@   |   
  0.5 +-+-**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@-+-+   
  aes bigidhrystone  miniz   norx primes  qsort sha512geomean   
  png: https://imgur.com/Agr5CJd

SPEC06int shows a larger improvement, up to ~2x avg speedup for the train
set:
  Aarch64 SPEC06int (train set) performance under QEMU user-mode
  Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz

4 +-+--+++++++++++++--+-+   
  |%%   ++  |   
  3.5 +-+..%%@.v2.8.0.v2.9.0v2.10.0.%%+v2.11.0+-+   
  |%%@  %%@   ++|   
3 +-+..%%@...++.%%@...%%+.+-+   
  |   +$%@|+%%@   %%@   |   
  2.5 +-+.##%@...%%@...+$%@...%%@.+-+   
2 +-+.##%@..+%%@...%%@.%%@.##%@...%%@..++.+-+   
  |   ##%@  %%@ ##%@  +$%@   %%@   %%@ ##%@   $%@  %%@  |   
  1.5 +-+**#%@.##%@.##%@..##%@...$%@...$%@.##%@..%%+.##%@.##%@+-+   
  |  **#%@**#%@**#%@  +++**#%@  ##%@  ++  ##%@**#%@ ##%@ ##%@ ##%@  |   
1 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@+##%@**#%@+##%@**#%@**#%@+-+   
  |  **#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@  |   
  0.5 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@+-+   
   401.bzi403.g429445.g456.h462.libq464.h471.omn4483.xalancbgeomean 
  png: https://imgur.com/JknVT5H

Note that the test set is less sensitive to the changes:
  https://imgur.com/W7CT0eO

Running small benchmarks (such as SPEC "test" or rv8-bench) is
very useful to get quick feedback on optimizations. However, some
of these runs are still dominated by parts of the code that aren't
that relevant -- for instance, some of them take so little time to
run that the major contributor to execution time is memory allocation.
Therefore, when publishing results it's best to stick with larger
benchmarks that run for longer (e.g. SPEC "train" set), which are more
sensitive to DBT performance.

I tried running some other benchmarks, such as nbench[1], under rv-jit.
I quickly get a "bus error" though -- don't know if I'm doing anything
wrong, or maybe compiling with the glibc cross-compiler I used
to build riscv linux isn't supported.
I managed though to run rv8-bench on both rv-jit and qemu (v8 patchset);
rv-jit is 1.30x faster on average for those, although note I dropped
qsort because it wasn't working properly on rv-jit:

   rv8-bench performance under rv-jit and QEMU user-mode
  Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
 [qsort does not finish cleanly for rv8, so I dropped it.]  

3 +-+-+---+--+---+---+---+--+---+-+-+   
  2.5 +-+..*..+-+   
  |*-+-* b1bae23b7c2|   
2 

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Thu, Mar 1, 2018 at 9:40 AM, Michael Clark  wrote:

>
>
> On Thu, Mar 1, 2018 at 12:53 AM, Peter Maydell 
> wrote:
>
>> On 28 February 2018 at 00:09, Michael Clark  wrote:
>> > I've just talked to SiFive about this. They have agreed that we can
>> remove
>> > the sifive_e300 and sifive_u500 boards from the patch series that we are
>> > going to submit upstream again later this week or early next week. These
>> > machines and their devices are pretty easy for us to maintain in the
>> riscv
>> > or a sifive repo. This trims the number of machines from 5 to 3 and
>> lets us
>> > remove the SiFiveUART and SiFivePRCI from the next patch series we are
>> > going to submit. e.g. v8
>>
>> Models of boards which people actively want and are using are fine
>> (though indeed you can save them for a later round of patches if you
>> like). And it sounds like the 1.9.1 stuff is genuinely wanted, so
>> thanks for the clarification there. What I want to avoid is boards
>> going into QEMU just because you happened to have them already. Once
>> a board model goes into QEMU it's a commitment to supporting it for
>> the long term, and getting rid of it again is hard.
>
>
> Most folk in the community at large are interested in the 'spike' and
> 'virt' machines. Well, the linux distributors are currently targetting
> 'virt' as it has working network and block storage.
>
> It's mostly SiFive customers that are interested in the SiFive machines.
>
> SiFive do intend to submit their machines upstream however we've decided
> that we want to review the naming of the machine/board/SOC in light of
> several new models, as there are infact more MCU/SOC models than are
> currently represented in the RISC-V QEMU port (E21, E31, E51, U54, U54MC).
> Those are the SOCs and then there are boards like the HiFive1, the LoFive,
> the HiFive Unleashed and several others (the 7th RISC-V Foundation Workshop
> had an electronic badge with the FE310 designed by antmicro). SiFive might
> like to review the naming and re-jig the SOC / board relationship.
> Presently we created two generic boards to represent the MMU-less E-series
> (sifive_e300) and the U-series (sifive_u500) however that doesn't cover the
> E21 and E51. As well as silicon, there are soft-cores, including soft-cores
> from other vendors (who have not yet submitted anything to the QEMU port).
> After reflecting on this we don't think the naming of the two sifive
> machines is quite right and they are not yet complete emulations. The 
> sifive_u500
> is supposed to model the HiFive Unleashed however we don't have device
> emulation for all of the hardware widgets yet, and some of the drivers are
> not yet in upstream Linux. Linus essentially accepted the generic core,
> which is what we are now suggesting for QEMU. We are totally happy to
> submit them if folk don't mind us potentially renaming them later. This
> is 2 patches for 2 machines and 2 or 3 devices. It would reduce the patch
> series from 23 patches to 18 patches and we'd maintain the 5 files and
> associated headers out-of tree until things firm up. The core of the port
> is pretty solid as Fedora have build stage 4 images using SMP on the 'virt'
> machine.
>

We had internal discussions about the SiFive machines and SiFive decided to
rename SiFive E300 (sifive_e300.c) and SiFive U500 (sifive_u500.c) to
SiFive E Series (sifive_e.c) and SiFive U Series (sifive_u.c). This was the
result of synchronising with the product marketing folk.

- sifive_e will instantiate a e31 nommu core when run in qemu-system-riscv32
- sifive_e will instantiate a e51 nommu core when run in qemu-system-riscv64
- sifive_u will instantiate a u34 mmu core when run in qemu-system-riscv32
- sifive_u will instantiate a u54 mmu core when run in qemu-system-riscv64

The riscv port is interesting because the machines are designed to be
instantiated with either 32-bit or 64-bit cores. SiFive decided they wanted
to get this right before we contributed the SiFive boards. We think we now
have a model we are satisfied with submittting.

We did some last minute changes to strengthen the riscv-qemu port
specificaiton compliance. The nommu machines worked previously but the lack
of an mmu was not enforced. We've now disabled mmu on cores that lack mmu
(SiFive E series). We've also made some changes to strengthen specification
compliance. It is possible for a core to implement S mode and U mode
(Supervisor and User privilege mode) either with or without an mmu. We've
now matches the hardware behaviour and discard mstatus.mpp writes for modes
that are not supported by the core, making it not possible to enter S mode
on the E series cores. We are pretty confident that these are low risk
changes, and we have tested that Linux still works fine in the 'virt'
machine. The 'virt' machine is likely to be the most popular machine for
use by the Linux community.

Given SiFive has SOCs and Soft-core IP, we would 

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Thu, Mar 1, 2018 at 11:26 AM, Emilio G. Cota  wrote:

> On Wed, Feb 28, 2018 at 13:09:11 +1300, Michael Clark wrote:
> > BTW somewhat coincidentally, the binary translator I wrote; RV8, which is
> > practicaly twice as fast as QEMU only supports privileged ISA v1.9.1 and
> I
> > personally want to keep binary compatiblity with it.
> (snip)
> > - https://rv8.io/
> > - https://rv8.io/bench
> > - https://anarch128.org/~mclark/rv8-carrv.pdf
> > - https://anarch128.org/~mclark/rv8-slides.pdf
>
> What QEMU versions did you use for those comparisons? I wonder if
> the recent indirect branch handling improvements were included in those
> (this work was merged in 2.10 for aarch64). Also, 2.7 is quite a bit
> faster than previous versions for user-mode due to the use of QHT,
> although you probably used a later version.
>

Yes I noticed indirect branch handling was very slow in the QEMU verison I
tested. I have highly optimised assembly stubs that implement a direct
mapped translation cache and translation cache miss fallback to C code that
does a fast hash map lookup.


> BTW after the merge you might want to look into optimizing indirect
> branches (and cross-page direct jumps in softmmu) for riscv in qemu.
> See examples with
> $ git log -Stcg_gen_lookup_and_goto_ptr
>

It was qemu-2.7.50 (late 2016). The benchmarks were generated mid last year.

I can run the benchmarks again... Has it doubled in speed?

Note: I don't even have a register allocator. I've assigned RISC-V RVC
register to hard registers (compiler optimises to choose compressable
registers first) and the remainder are in spill slots that in many cases
can be embedded as memory operands in the x86_64 translation. i.e. no
explicit reload, we let the micro-architecture crack these into micro-ops,
as they help to keep up code density.

I think I can get close to double again with tiered optimization and a good
register allocator (lift RISC-V asm to SSA form). It's also a hotspot
interpreter, which is definately faster than compiling all code, as I
benchmarked it. It profiles and only translates hot paths, so code that
only runs a few iterations is not translated. When I did eager transaltion
I got a slow-down.

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Emilio G. Cota]

On Wed, Feb 28, 2018 at 13:09:11 +1300, Michael Clark wrote:
> BTW somewhat coincidentally, the binary translator I wrote; RV8, which is
> practicaly twice as fast as QEMU only supports privileged ISA v1.9.1 and I
> personally want to keep binary compatiblity with it.
(snip)
> - https://rv8.io/
> - https://rv8.io/bench
> - https://anarch128.org/~mclark/rv8-carrv.pdf
> - https://anarch128.org/~mclark/rv8-slides.pdf

What QEMU versions did you use for those comparisons? I wonder if
the recent indirect branch handling improvements were included in those
(this work was merged in 2.10 for aarch64). Also, 2.7 is quite a bit
faster than previous versions for user-mode due to the use of QHT,
although you probably used a later version.

BTW after the merge you might want to look into optimizing indirect
branches (and cross-page direct jumps in softmmu) for riscv in qemu.
See examples with
$ git log -Stcg_gen_lookup_and_goto_ptr

Cheers,

Emilio

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Thu, Mar 1, 2018 at 12:53 AM, Peter Maydell 
wrote:

> On 28 February 2018 at 00:09, Michael Clark  wrote:
> > I've just talked to SiFive about this. They have agreed that we can
> remove
> > the sifive_e300 and sifive_u500 boards from the patch series that we are
> > going to submit upstream again later this week or early next week. These
> > machines and their devices are pretty easy for us to maintain in the
> riscv
> > or a sifive repo. This trims the number of machines from 5 to 3 and lets
> us
> > remove the SiFiveUART and SiFivePRCI from the next patch series we are
> > going to submit. e.g. v8
>
> Models of boards which people actively want and are using are fine
> (though indeed you can save them for a later round of patches if you
> like). And it sounds like the 1.9.1 stuff is genuinely wanted, so
> thanks for the clarification there. What I want to avoid is boards
> going into QEMU just because you happened to have them already. Once
> a board model goes into QEMU it's a commitment to supporting it for
> the long term, and getting rid of it again is hard.


Most folk in the community at large are interested in the 'spike' and
'virt' machines. Well, the linux distributors are currently targetting
'virt' as it has working network and block storage.

It's mostly SiFive customers that are interested in the SiFive machines.

SiFive do intend to submit their machines upstream however we've decided
that we want to review the naming of the machine/board/SOC in light of
several new models, as there are infact more MCU/SOC models than are
currently represented in the RISC-V QEMU port (E21, E31, E51, U54, U54MC).
Those are the SOCs and then there are boards like the HiFive1, the LoFive,
the HiFive Unleashed and several others (the 7th RISC-V Foundation Workshop
had an electronic badge with the FE310 designed by antmicro). SiFive might
like to review the naming and re-jig the SOC / board relationship.
Presently we created two generic boards to represent the MMU-less E-series
(sifive_e300) and the U-series (sifive_u500) however that doesn't cover the
E21 and E51. As well as silicon, there are soft-cores, including soft-cores
from other vendors (who have not yet submitted anything to the QEMU port).
After reflecting on this we don't think the naming of the two sifive
machines is quite right and they are not yet complete emulations. The
sifive_u500
is supposed to model the HiFive Unleashed however we don't have device
emulation for all of the hardware widgets yet, and some of the drivers are
not yet in upstream Linux. Linus essentially accepted the generic core,
which is what we are now suggesting for QEMU. We are totally happy to
submit them if folk don't mind us potentially renaming them later. This is
2 patches for 2 machines and 2 or 3 devices. It would reduce the patch
series from 23 patches to 18 patches and we'd maintain the 5 files and
associated headers out-of tree until things firm up. The core of the port
is pretty solid as Fedora have build stage 4 images using SMP on the 'virt'
machine.


> > However I'm inclined to leave it as it is, at this point. It is not
> > something that we can't change in the future once the code is in-tree.
>
> With my 'upstream dev' hat on, I tend to be suspicious of this
> line of argument, because in a lot of cases what tends to happen
> is that the code for some new target or device goes in-tree, and
> then the people who worked on submitting it disappear, or never
> actually do get round to refactoring[*]. You get more leeway for
> making this argument the longer you've been around and participating
> in QEMU development, because then you have a track record of
> following up on things.
>
> [*] in fact we're currently discussing deleting support for
> a couple of target architectures that were basically "once the
> code went into mainline nothing further was ever done to it except
> global-refactorings and other tree wide maintenance by other
> QEMU developers".


I might explore coalescing the two spike boards before the v8 spin then.

I was thinking about using a global to enable the backwards compatibility
mode. It seems -global is for driver properties? Is it appropriate to use a
global property to change the behaviour of a machine?

Okay, I see raspi.c contains raspi, raspi 2, raspi 3, and it just defines
multiple machines in the same file and passes a version number to routines
that are shared. We could go with that approach.

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Peter Maydell]

On 28 February 2018 at 11:53, Peter Maydell  wrote:
> With my 'upstream dev' hat on, I tend to be suspicious of this
> line of argument, because in a lot of cases what tends to happen
> is that the code for some new target or device goes in-tree, and
> then the people who worked on submitting it disappear, or never
> actually do get round to refactoring[*]. You get more leeway for
> making this argument the longer you've been around and participating
> in QEMU development, because then you have a track record of
> following up on things.

That said, I can probably live with it in this particular instance,
given when the 2.12 codefreeze deadline is.

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Peter Maydell]

On 28 February 2018 at 00:09, Michael Clark  wrote:
> I've just talked to SiFive about this. They have agreed that we can remove
> the sifive_e300 and sifive_u500 boards from the patch series that we are
> going to submit upstream again later this week or early next week. These
> machines and their devices are pretty easy for us to maintain in the riscv
> or a sifive repo. This trims the number of machines from 5 to 3 and lets us
> remove the SiFiveUART and SiFivePRCI from the next patch series we are
> going to submit. e.g. v8

Models of boards which people actively want and are using are fine
(though indeed you can save them for a later round of patches if you
like). And it sounds like the 1.9.1 stuff is genuinely wanted, so
thanks for the clarification there. What I want to avoid is boards
going into QEMU just because you happened to have them already. Once
a board model goes into QEMU it's a commitment to supporting it for
the long term, and getting rid of it again is hard.

> However I'm inclined to leave it as it is, at this point. It is not
> something that we can't change in the future once the code is in-tree.

With my 'upstream dev' hat on, I tend to be suspicious of this
line of argument, because in a lot of cases what tends to happen
is that the code for some new target or device goes in-tree, and
then the people who worked on submitting it disappear, or never
actually do get round to refactoring[*]. You get more leeway for
making this argument the longer you've been around and participating
in QEMU development, because then you have a track record of
following up on things.

[*] in fact we're currently discussing deleting support for
a couple of target architectures that were basically "once the
code went into mainline nothing further was ever done to it except
global-refactorings and other tree wide maintenance by other
QEMU developers".

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Igor Mammedov]

On Wed, 28 Feb 2018 13:41:25 +1300
Michael Clark  wrote:

> On Wed, Feb 28, 2018 at 5:00 AM, Igor Mammedov  wrote:
> 
> > On Tue, 27 Feb 2018 14:01:05 +
> > Peter Maydell  wrote:
> >  
> > > On 27 February 2018 at 00:15, Michael Clark  wrote:  
> > > > -BEGIN PGP SIGNED MESSAGE-
> > > > Hash: SHA1
> > > >
> > > > The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a  
> > f8c5c2a6d0:  
> > > >
> > > >   maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07  
> > +)  
> > > >
> > > > are available in the git repository at:
> > > >
> > > >   https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7
> > > >
> > > > for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc  
> > bdff0fd7a8:  
> > > >
> > > >   RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300)
> > > >
> > > > - 
> > > > QEMU RISC-V Emulation Support (RV64GC, RV32GC)  
> > >
> > > Hi; thanks for this pull request. Unfortunately it seems to
> > > be missing Signed-off-by: tags. Every commit needs to have
> > > the Signed-off-by: tags from the people who contributed code to
> > > it, indicating that they're OK with the code going into QEMU.
> > > (If the work was done by and copyright a company then you don't
> > > need to provide signoffs from every person at the company who
> > > worked on the code if you don't want to.)
> > >  
> > > > The spike_v1.9
> > > > machine has been renamed to spike_v1.9.1 to match the privileged ISA
> > > > version and spike_v1.10 has been made the default machine.  
> > >
> > > I'm confused about this. Generally QEMU boards should model
> > > hardware, and the board shouldn't care about the ISA versions.
> > > Versioned board names in QEMU generally follow _QEMU_'s versioning,
> > > and indicate that a board is identical to whatever we modelled
> > > in that earlier QEMU version, for VM migration compatibility.
> > > Board renames for minor ISA version bumps sounds like there's going
> > > to be a lot of churn and breakage -- is this stuff really ready?
> > > (Also, should we really have two different board source files
> > > for two different ISA versions? I would have expected these to
> > > share a source file to share code.)
> > >
> > > I did a test build and there are some compile errors:
> > >
> > > /home/pm215/qemu/linux-user/main.c:38:24: fatal error: target_elf.h:
> > > No such file or directory
> > >  #include "target_elf.h"
> > > ^
> > > compilation terminated.
> > >
> > > This is because your patchset has a clash with commit 542ca4349878a2e
> > > which has just merged to master, and refactors out an ifdef ladder,
> > > so now all targets supporting linux-user need to provide a
> > > linux-user/$ARCH/target_elf.h file. Could you fix that up and rebase,
> > > please?  
> > also '[PATCH v7 03/23] RISC-V CPU Core Definition' still hasn't addressed
> > comment http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06412.html
> > which isn't fixed since it was first pointed out (v4).
> >
> > I'd prefer that being fixed before merge so another people
> > won't have to clean it up later after original authors,
> > When they try to generalize cpu_type -> cpu_model conversion.
> >  
> 
> I re-read the email and it doesn't seem clear what you want us to do.
> I changed the CPU suffix to a prefix as you requested. The rest of the CPU
> initialisation is "modelled" on arm not sh4.
Not addressed comment is not related to CPU suffix, it's a separate issue.

I haven't had time to clean up that part of ARM cpu initialization yet,
so it still uses old approach, but you've been pointed to right
approach rather early (v4) but ignored it.

There is no point to use legacy approach with new patches
that could block other people and would force them to cleanup
before doing what they intended to do.

I've pointed to sh4 as example that you should use for new CPU
types internalization. Beside making RISC-V consistent with
direction that part of code moves to, it will also simplify patch.

Please ask if something still unclear.

> If you want to make a pull request, please use this branch:
> 
> - https://github.com/riscv/riscv-qemu/tree/qemu-upstream-v7

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Wed, Feb 28, 2018 at 5:00 AM, Igor Mammedov  wrote:

> On Tue, 27 Feb 2018 14:01:05 +
> Peter Maydell  wrote:
>
> > On 27 February 2018 at 00:15, Michael Clark  wrote:
> > > -BEGIN PGP SIGNED MESSAGE-
> > > Hash: SHA1
> > >
> > > The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a
> f8c5c2a6d0:
> > >
> > >   maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07
> +)
> > >
> > > are available in the git repository at:
> > >
> > >   https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7
> > >
> > > for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc
> bdff0fd7a8:
> > >
> > >   RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300)
> > >
> > > - 
> > > QEMU RISC-V Emulation Support (RV64GC, RV32GC)
> >
> > Hi; thanks for this pull request. Unfortunately it seems to
> > be missing Signed-off-by: tags. Every commit needs to have
> > the Signed-off-by: tags from the people who contributed code to
> > it, indicating that they're OK with the code going into QEMU.
> > (If the work was done by and copyright a company then you don't
> > need to provide signoffs from every person at the company who
> > worked on the code if you don't want to.)
> >
> > > The spike_v1.9
> > > machine has been renamed to spike_v1.9.1 to match the privileged ISA
> > > version and spike_v1.10 has been made the default machine.
> >
> > I'm confused about this. Generally QEMU boards should model
> > hardware, and the board shouldn't care about the ISA versions.
> > Versioned board names in QEMU generally follow _QEMU_'s versioning,
> > and indicate that a board is identical to whatever we modelled
> > in that earlier QEMU version, for VM migration compatibility.
> > Board renames for minor ISA version bumps sounds like there's going
> > to be a lot of churn and breakage -- is this stuff really ready?
> > (Also, should we really have two different board source files
> > for two different ISA versions? I would have expected these to
> > share a source file to share code.)
> >
> > I did a test build and there are some compile errors:
> >
> > /home/pm215/qemu/linux-user/main.c:38:24: fatal error: target_elf.h:
> > No such file or directory
> >  #include "target_elf.h"
> > ^
> > compilation terminated.
> >
> > This is because your patchset has a clash with commit 542ca4349878a2e
> > which has just merged to master, and refactors out an ifdef ladder,
> > so now all targets supporting linux-user need to provide a
> > linux-user/$ARCH/target_elf.h file. Could you fix that up and rebase,
> > please?
> also '[PATCH v7 03/23] RISC-V CPU Core Definition' still hasn't addressed
> comment http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06412.html
> which isn't fixed since it was first pointed out (v4).
>
> I'd prefer that being fixed before merge so another people
> won't have to clean it up later after original authors,
> When they try to generalize cpu_type -> cpu_model conversion.
>

I re-read the email and it doesn't seem clear what you want us to do. I
changed the CPU suffix to a prefix as you requested. The rest of the CPU
initialisation is "modelled" on arm not sh4.

If you want to make a pull request, please use this branch:

- https://github.com/riscv/riscv-qemu/tree/qemu-upstream-v7

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Wed, Feb 28, 2018 at 6:50 AM, Peter Maydell 
wrote:

> On 27 February 2018 at 15:50, Stef O'Rear  wrote:
> > On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell 
> wrote:
> >> On 27 February 2018 at 00:15, Michael Clark  wrote:
> >>> The spike_v1.9
> >>> machine has been renamed to spike_v1.9.1 to match the privileged ISA
> >>> version and spike_v1.10 has been made the default machine.
> >>
> >> I'm confused about this. Generally QEMU boards should model
> >> hardware, and the board shouldn't care about the ISA versions.
> >
> > The spike boards model the Berkeley architectural simulator "spike"
> > (https://github.com/riscv/riscv-isa-sim), which does not have a formal
> > release process or version numbers so we are using the ISA version as
> > a proxy for spike's version.
> >
> > When physical boards are released with full documentation I presume we
> > will be adding board definitions for them, and they will imply
> > specific ISA versions.
> >
> >> Versioned board names in QEMU generally follow _QEMU_'s versioning,
> >> and indicate that a board is identical to whatever we modelled
> >> in that earlier QEMU version, for VM migration compatibility.
> >
> > In this case we're handling two logically distinct boards.  We could
> > combine them and implement a parameter; I was having trouble finding a
> > suitable example to follow earlier but it looks like gic-version in
> > hw/arm/virt.c is one.  This seems like a bad thing to change this late
> > in the review though?
>
> You don't need to make them one board with a command line option
> if that doesn't suit -- for instance hw/arm/vexpress.c defines
> multiple board models that are variants on each other and
> share a lot of code. That said, see below...
>
> >> Board renames for minor ISA version bumps sounds like there's going
> >> to be a lot of churn and breakage -- is this stuff really ready?
> >> (Also, should we really have two different board source files
> >> for two different ISA versions? I would have expected these to
> >> share a source file to share code.)
> >
> > 1.10 is the version we have committed to long term support for; it
> > matches all public hardware the upstream Linux port, so it seems
> > appropriate to use for QEMU.
> >
> > 1.9.1 was the version supported by riscv-qemu at the time Michael
> > Clark took over maintainership; we have not removed support for it
> > because we cannot prove that there is nobody depending on it, although
> > I do not use it myself and do not know anyone else who does, so I
> > would not personably object to removing it if that were required.
>
> I would rather not have stray legacy old versions in QEMU just
> because we think maybe somebody might be using them. If 1.10
> is the long-term-support committed version, then I think we
> should just have a model of that. Anybody who for some reason is
> still stuck on an older unsupported version gets to find out
> what "unsupported" means; they can always keep using whatever
> old QEMU code base they've been using up til now, presumably.
>

SiFive are happy to support privileged ISA v1.9.1.

I don't think the branch we maintain will easily merge with a branch that
has privileged ISA v1.9.1 torn out (the only version that actually worked 4
months ago).

If we can't submit our port with privileged ISA v1.9.1 suport then thats
going to put a big spanner in the works.

We've made a pretty strong choice to not break backwards compatibility
going forward and privileged ISA v1.9.1 is the line in the sand so to
speak. i.e. the QEMU port is still compatible with binaries from the v1.9.1
ISA spec published in November 2016 which has been implemented by many
folk. We have to have a much more reasonable deprecation period. Software
such as GDB and OpenOCD continue to support privileged ISA v1.9.1 and have
specific fallback code paths, as well as the OpenOCD port having support
for two versions of the debug spec.

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Michael Clark]

On Wed, Feb 28, 2018 at 4:50 AM, Stef O'Rear  wrote:

> On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell 
> wrote:
> > On 27 February 2018 at 00:15, Michael Clark  wrote:
> >> -BEGIN PGP SIGNED MESSAGE-
> >> Hash: SHA1
> >>
> >> The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a
> f8c5c2a6d0:
> >>
> >>   maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07
> +)
> >>
> >> are available in the git repository at:
> >>
> >>   https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7
> >>
> >> for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc
> bdff0fd7a8:
> >>
> >>   RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300)
> >>
> >> - 
> >> QEMU RISC-V Emulation Support (RV64GC, RV32GC)
> >
> > Hi; thanks for this pull request. Unfortunately it seems to
> > be missing Signed-off-by: tags. Every commit needs to have
> > the Signed-off-by: tags from the people who contributed code to
> > it, indicating that they're OK with the code going into QEMU.
> > (If the work was done by and copyright a company then you don't
> > need to provide signoffs from every person at the company who
> > worked on the code if you don't want to.)
>
> I'll add mine.
>
> >> The spike_v1.9
> >> machine has been renamed to spike_v1.9.1 to match the privileged ISA
> >> version and spike_v1.10 has been made the default machine.
> >
> > I'm confused about this. Generally QEMU boards should model
> > hardware, and the board shouldn't care about the ISA versions.
>
> The spike boards model the Berkeley architectural simulator "spike"
> (https://github.com/riscv/riscv-isa-sim), which does not have a formal
> release process or version numbers so we are using the ISA version as
> a proxy for spike's version.
>
> When physical boards are released with full documentation I presume we
> will be adding board definitions for them, and they will imply
> specific ISA versions.
>
> > Versioned board names in QEMU generally follow _QEMU_'s versioning,
> > and indicate that a board is identical to whatever we modelled
> > in that earlier QEMU version, for VM migration compatibility.
>
> In this case we're handling two logically distinct boards.  We could
> combine them and implement a parameter; I was having trouble finding a
> suitable example to follow earlier but it looks like gic-version in
> hw/arm/virt.c is one.  This seems like a bad thing to change this late
> in the review though?
>
> > Board renames for minor ISA version bumps sounds like there's going
> > to be a lot of churn and breakage -- is this stuff really ready?
> > (Also, should we really have two different board source files
> > for two different ISA versions? I would have expected these to
> > share a source file to share code.)
>
> 1.10 is the version we have committed to long term support for; it
> matches all public hardware the upstream Linux port, so it seems
> appropriate to use for QEMU.
>
> 1.9.1 was the version supported by riscv-qemu at the time Michael
> Clark took over maintainership; we have not removed support for it
> because we cannot prove that there is nobody depending on it, although
> I do not use it myself and do not know anyone else who does, so I
> would not personably object to removing it if that were required.
>
> Combining spike_v1.10 and spike_v1.9.1 would also be an option amenable to
> us.
>

I've just talked to SiFive about this. They have agreed that we can remove
the sifive_e300 and sifive_u500 boards from the patch series that we are
going to submit upstream again later this week or early next week. These
machines and their devices are pretty easy for us to maintain in the riscv
or a sifive repo. This trims the number of machines from 5 to 3 and lets us
remove the SiFiveUART and SiFivePRCI from the next patch series we are
going to submit. e.g. v8

SiFive have indicated that they would like to keep privileged ISA v1.9.1
support. It's likely the RISC-V foundation would also like us to start
supporting backwards compatibility from this point. Removing support for a
specification version only 4 months after the latest specification has been
implemented is too severe of a deprecation period. They have said they
would like QEMU to support at least 2 specification versions so we won't
consider removing privileged ISA v1.9.1 support until privileged ISA v1.11
has been released and implemented. There are still several OS ports and
private tape-outs and test chips that target privileged ISA v1.9.1. In
fact, someone may very well add privileged ISA v1.9 and privileged ISA v1.7
support, perhaps as a computing history project. The published
specifications are all available however the chips implementing these
versions of the spec are mostly test chips. Nevertheless, they are part of
RISC-V history.

With respect to combining them, we could investigate triggering the config

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Peter Maydell]

On 27 February 2018 at 15:50, Stef O'Rear  wrote:
> On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell  
> wrote:
>> On 27 February 2018 at 00:15, Michael Clark  wrote:
>>> The spike_v1.9
>>> machine has been renamed to spike_v1.9.1 to match the privileged ISA
>>> version and spike_v1.10 has been made the default machine.
>>
>> I'm confused about this. Generally QEMU boards should model
>> hardware, and the board shouldn't care about the ISA versions.
>
> The spike boards model the Berkeley architectural simulator "spike"
> (https://github.com/riscv/riscv-isa-sim), which does not have a formal
> release process or version numbers so we are using the ISA version as
> a proxy for spike's version.
>
> When physical boards are released with full documentation I presume we
> will be adding board definitions for them, and they will imply
> specific ISA versions.
>
>> Versioned board names in QEMU generally follow _QEMU_'s versioning,
>> and indicate that a board is identical to whatever we modelled
>> in that earlier QEMU version, for VM migration compatibility.
>
> In this case we're handling two logically distinct boards.  We could
> combine them and implement a parameter; I was having trouble finding a
> suitable example to follow earlier but it looks like gic-version in
> hw/arm/virt.c is one.  This seems like a bad thing to change this late
> in the review though?

You don't need to make them one board with a command line option
if that doesn't suit -- for instance hw/arm/vexpress.c defines
multiple board models that are variants on each other and
share a lot of code. That said, see below...

>> Board renames for minor ISA version bumps sounds like there's going
>> to be a lot of churn and breakage -- is this stuff really ready?
>> (Also, should we really have two different board source files
>> for two different ISA versions? I would have expected these to
>> share a source file to share code.)
>
> 1.10 is the version we have committed to long term support for; it
> matches all public hardware the upstream Linux port, so it seems
> appropriate to use for QEMU.
>
> 1.9.1 was the version supported by riscv-qemu at the time Michael
> Clark took over maintainership; we have not removed support for it
> because we cannot prove that there is nobody depending on it, although
> I do not use it myself and do not know anyone else who does, so I
> would not personably object to removing it if that were required.

I would rather not have stray legacy old versions in QEMU just
because we think maybe somebody might be using them. If 1.10
is the long-term-support committed version, then I think we
should just have a model of that. Anybody who for some reason is
still stuck on an older unsupported version gets to find out
what "unsupported" means; they can always keep using whatever
old QEMU code base they've been using up til now, presumably.

thanks
-- PMM

Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission

[Stef O'Rear]

On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell  wrote:
> On 27 February 2018 at 00:15, Michael Clark  wrote:
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5af8c5c2a6d0:
>>
>>   maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 +)
>>
>> are available in the git repository at:
>>
>>   https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7
>>
>> for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dcbdff0fd7a8:
>>
>>   RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300)
>>
>> - 
>> QEMU RISC-V Emulation Support (RV64GC, RV32GC)
>
> Hi; thanks for this pull request. Unfortunately it seems to
> be missing Signed-off-by: tags. Every commit needs to have
> the Signed-off-by: tags from the people who contributed code to
> it, indicating that they're OK with the code going into QEMU.
> (If the work was done by and copyright a company then you don't
> need to provide signoffs from every person at the company who
> worked on the code if you don't want to.)

I'll add mine.

>> The spike_v1.9
>> machine has been renamed to spike_v1.9.1 to match the privileged ISA
>> version and spike_v1.10 has been made the default machine.
>
> I'm confused about this. Generally QEMU boards should model
> hardware, and the board shouldn't care about the ISA versions.

The spike boards model the Berkeley architectural simulator "spike"
(https://github.com/riscv/riscv-isa-sim), which does not have a formal
release process or version numbers so we are using the ISA version as
a proxy for spike's version.

When physical boards are released with full documentation I presume we
will be adding board definitions for them, and they will imply
specific ISA versions.

> Versioned board names in QEMU generally follow _QEMU_'s versioning,
> and indicate that a board is identical to whatever we modelled
> in that earlier QEMU version, for VM migration compatibility.

In this case we're handling two logically distinct boards.  We could
combine them and implement a parameter; I was having trouble finding a
suitable example to follow earlier but it looks like gic-version in
hw/arm/virt.c is one.  This seems like a bad thing to change this late
in the review though?

> Board renames for minor ISA version bumps sounds like there's going
> to be a lot of churn and breakage -- is this stuff really ready?
> (Also, should we really have two different board source files
> for two different ISA versions? I would have expected these to
> share a source file to share code.)

1.10 is the version we have committed to long term support for; it
matches all public hardware the upstream Linux port, so it seems
appropriate to use for QEMU.

1.9.1 was the version supported by riscv-qemu at the time Michael
Clark took over maintainership; we have not removed support for it
because we cannot prove that there is nobody depending on it, although
I do not use it myself and do not know anyone else who does, so I
would not personably object to removing it if that were required.

Combining spike_v1.10 and spike_v1.9.1 would also be an option amenable to us.

-s