implementing Simple-V in spike: potential bug in spike CSR implementation wrt spec; potential issue for RVV?

[Luke Kenneth Casson Leighton]

i'm just looking at implementing simple-v in spike, which needs a
careful review of spike to implement correctly, and i believe i may
have encountered a bug (incorrect implementation of CSR
specification).

the implementation of riscv/insns/csrrw.h (and csrrwi is similar) is as follows:

int csr = validate_csr(insn.csr(), true);
reg_t old = p->get_csr(csr);
p->set_csr(csr, RS1);
WRITE_RD(sext_xlen(old));
serialize();

the privileged spec (V20180801-draft) page 48 says:

For both CSRRS and CSRRC, if rs1=x0, then the instruction will not
write to the CSR at all, and
so shall not cause any of the side effects that might otherwise occur
on a CSR write, such as raising

and, further down (and also at the top of page 48 and crossing over
from the bottom of page 47, specifying the same thing for CSRRW):

For CSRRWI, if rd=x0, then the instruction shall not read the CSR and
shall not cause any
of the side-effects that might occur on a CSR read.

so the implementation of csrrwi does not appear to conform to the
privileged specification, as the read of the CSR is carried out
*unconditionally* (i.e. there is no test for RD==x0).

checking csrrs.h, there *is* in fact a check "bool write = insn.rs1()
!= 0;", however that is for the source: the specification specifically
says that if *rd* == x0 the CSR shall not be *read*.

when looking in processor_t::get_csr() there does not appear to be a
place where the instruction is decoded (a second time) such that it
may be determined if rd == x0

is this an oversight?

thoughts and insights appreciated (happy to send patches if needed,
once clarification is received).


secondly, there may be a potential conflict in how SETVL is to be
implemented using CSRs, which would require, i believe, a potential
violation (or at least, a-bit-of-creative-interpretation) of the
priv-spec to implement (and, annoyingly, SV would have the exact same
issue).

"SETVL t0, a0" in RVV means:

* take the contents of a0
* take the minimum of that and the current CSR
* store the resultant value in VL *and* return it in t0 (RD)

however, the CSRRW spec *specifically* says the following:

* take the old value in the CSR and store it for subsequent return
* set the CSR to the new value
* RETURN THE OLD PREVIOUS VALUE (in RD).

now, it could be reasonably claimed that the implementation of SETVL
"reading the CSR" could have "side-effects" which involve actually
setting VL: it's a bit of a stretch, and, honestly, in SV i feel i may
end up having to have a special-case version of csrrwi.h, which would
be a bit of a nuisance, but doable.

again, thoughts, clarification and insights appreciated: is the
planned RVV implementation of SETVL a violation of the priv-spec or is
it just "a bit of creative interpretation of the same"?

l.

[Andrew Waterman]

It is an oversight that there's not a comment explaining why this is

OK: no standard CSRs, and no CSRs that Spike implements, have side

effects on reads.   Can cross that bridge if a proposed standard adds

CSRs with side effects on reads (which I think many people would like

never to happen).

thoughts and insights appreciated (happy to send patches if needed,
once clarification is received).


secondly, there may be a potential conflict in how SETVL is to be
implemented using CSRs, which would require, i believe, a potential
violation (or at least, a-bit-of-creative-interpretation) of the
priv-spec to implement (and, annoyingly, SV would have the exact same
issue).

"SETVL t0, a0" in RVV means:

* take the contents of a0
* take the minimum of that and the current CSR
* store the resultant value in VL *and* return it in t0 (RD)

however, the CSRRW spec *specifically* says the following:

* take the old value in the CSR and store it for subsequent return
* set the CSR to the new value
* RETURN THE OLD PREVIOUS VALUE (in RD).

now, it could be reasonably claimed that the implementation of SETVL
"reading the CSR" could have "side-effects" which involve actually
setting VL: it's a bit of a stretch, and, honestly, in SV i feel i may
end up having to have a special-case version of csrrwi.h, which would
be a bit of a nuisance, but doable.

It is indeed a stretch, which is why, as of a July 2017 draft, RVV

doesn't use CSRRW to implement SETVL.  It is its own instruction,

which just happens to manipulate CSRs.  

again, thoughts, clarification and insights appreciated: is the
planned RVV implementation of SETVL a violation of the priv-spec or is
it just "a bit of creative interpretation of the same"?

l.

--
You received this message because you are subscribed to the Google Groups "RISC-V ISA Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to isa-dev+u...@groups.riscv.org.
To post to this group, send email to isa...@groups.riscv.org.
Visit this group at https://groups.google.com/a/groups.riscv.org/group/isa-dev/.
To view this discussion on the web visit https://groups.google.com/a/groups.riscv.org/d/msgid/isa-dev/CAPweEDzFkRS6cUZ_iSt3kube%3DeqTeYtp5yz1DtOkEQgyN%3DDkYw%40mail.gmail.com.

[Michael Clark]

Having the false dependency between the source and destination registers become a true dependency to support CSRs with “special” semantics is eminently preferable to adding special branch instructions that branch on inherently racy processor internal state. The CSR address space is IMHO the right place to put these types of (standard/non-standard) extensions

A true register dependency could be used to expand the “read-only” CSR address space from 0xFF to XLEN, allowing a READID instruction that takes one unused CSR slot and thus could be emulated in firmware (placing it anywhere else does not convey that advantage).

That said, spooky or magic CSRs are less that ideal, however compared to alternatives like interrupt controller specific branch instructions, they are vastly preferable.

BTW The code in spike, by decomposing CSRs into discrete read and write operations doesn’t allow allow implementation of a truly atomic CSR. This is not so bad in spike, but for a multithreaded simulator, it is less than ideal.

To view this discussion on the web visit https://groups.google.com/a/groups.riscv.org/d/msgid/isa-dev/CA%2B%2B6G0D89U%2B7Qe6z22NAMzjJQGPXRkjVgjtBbYzZ4KgTGy4jcw%40mail.gmail.com.

[Andrew Waterman]

Totally agreed.

BTW The code in spike, by decomposing CSRs into discrete read and write operations doesn’t allow allow implementation of a truly atomic CSR. This is not so bad in spike, but for a multithreaded simulator, it is less than ideal.

True, but it's only problematic if a given hart's CSRs are writable by multiple host threads.  There are other ways to skin this cat.

[Luke Kenneth Casson Leighton]

On Thu, Sep 27, 2018 at 7:34 PM, Andrew Waterman
<wate...@eecs.berkeley.edu> wrote:
>
>
>
> On Thu, Sep 27, 2018 at 7:52 AM Luke Kenneth Casson Leighton <lk...@lkcl.net>
> wrote:

>> when looking in processor_t::get_csr() there does not appear to be a
>> place where the instruction is decoded (a second time) such that it
>> may be determined if rd == x0
>>
>> is this an oversight?
>
>
>
> It is an oversight that there's not a comment explaining why this is
> OK: no standard CSRs, and no CSRs that Spike implements, have side
> effects on reads.

ok so the main priority: it's safe (spec-compliant). whew.

secondary: yes, with c++ being what it is (hidden overloading) a code
review of *just* get_csr would not necessarily pick up that accessing
the processor_t::state variable has (potential) side-effects, a
comment at the top of get_csr would be good [can create a patch if you
like?]

third, it's not necessarily optimal, i would be surprised if g++
notices that the old value isn't used and cuts out the entire call...
wait... csrrwi.h doesn't have the check to stop a write to the zero
reg...

... ah, it's in regfile_t:write (decode.h) - *that's* where the test
to see if writing to x0 results in the value to be written being
ignored.

ok so this is more about optimisation than anything, and if CSR
setting/getting is not a major activity it could be argued that
developer time is better spent elsewhere (80/20 rule)

> Can cross that bridge if a proposed standard adds
> CSRs with side effects on reads (which I think many people would like
> never to happen).

agreed. however... it'll be something that i could see catching out
people writing custom CSRs and/or people with less experience with
RISC-V. particularly when their first port of call would be to go "hm
done the spec, what's the next bit, ah yes: write a proof-of-concept
by implementing the custom extension in spike".

they would then find that the code's specifically not designed to
allow them to do that. or, worse, they *don't* find it, and spend
considerable time debugging [and eventually come here and spend list
members' cpu cycles on enquiries finding out why]

>> secondly, there may be a potential conflict in how SETVL is to be
>> implemented using CSRs,

>> [...]

>> now, it could be reasonably claimed that the implementation of SETVL
>> "reading the CSR" could have "side-effects" which involve actually
>> setting VL: it's a bit of a stretch, and, honestly, in SV i feel i may
>> end up having to have a special-case version of csrrwi.h, which would
>> be a bit of a nuisance, but doable.
>
>
> It is indeed a stretch, which is why, as of a July 2017 draft, RVV
> doesn't use CSRRW to implement SETVL. It is its own instruction,
> which just happens to manipulate CSRs.

okaay. and i've just noticed there's a cross-reference now to
https://github.com/riscv/riscv-v-spec been added to
V20180801-draft - excellent!

https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#code-vl-code-and-code-vsetvl-code-instruction

ok yes, got it: that's clear, "vsetvl not encoded as regular
CSRRW.... regular CSR can still read/write it"

damn. that's a pain. if implementing [SV]SETVL as a CSRRWI
overload, SV requires absolutely no new instructions whatsoever. may
just have to bite the bullet on that one and use an I-type encoding
(SV's SETVL is of the form SETVL t0, a0, #imm)

thanks for clarifying, andrew.

l.

[Michael Clark]

This is the case in QEMU for IO, in particular ‘mip’. I was able to significantly decrease interrupt latency and idle CPU by removing mutex overhead, that resulted in significant increase in performance, tested under load (RISC-V GCC bootstrap on 4 cores inside RISC-V Linux on QEMU).

[Andrew Waterman]

On Thu, Sep 27, 2018 at 1:44 PM Luke Kenneth Casson Leighton <lk...@lkcl.net> wrote:

On Thu, Sep 27, 2018 at 7:34 PM, Andrew Waterman
<wate...@eecs.berkeley.edu> wrote:
>
>
>
> On Thu, Sep 27, 2018 at 7:52 AM Luke Kenneth Casson Leighton <lk...@lkcl.net>
> wrote:

>> when looking in processor_t::get_csr() there does not appear to be a
>> place where the instruction is decoded (a second time) such that it
>> may be determined if rd == x0
>>
>> is this an oversight?
>
>
>
> It is an oversight that there's not a comment explaining why this is
> OK: no standard CSRs, and no CSRs that Spike implements, have side
> effects on reads.

 ok so the main priority: it's safe (spec-compliant). whew.

 secondary: yes, with c++ being what it is (hidden overloading) a code
review of *just* get_csr would not necessarily pick up that accessing
the processor_t::state variable has (potential) side-effects, a
comment at the top of get_csr would be good [can create a patch if you
like?]

Already done.

 

 third, it's not necessarily optimal, i would be surprised if g++
notices that the old value isn't used and cuts out the entire call...
wait... csrrwi.h doesn't have the check to stop a write to the zero
reg...

 ... ah, it's in regfile_t:write (decode.h) - *that's* where the test
to see if writing to x0 results in the value to be written being
ignored.

 ok so this is more about optimisation than anything, and if CSR
setting/getting is not a major activity it could be argued that
developer time is better spent elsewhere (80/20 rule)

Yeah, the underlying assumption is that these code paths do not contribute much to total runtime in most use cases.

>   Can cross that bridge if a proposed standard adds
> CSRs with side effects on reads (which I think many people would like
> never to happen).

 agreed.  however... it'll be something that i could see catching out
people writing custom CSRs and/or people with less experience with
RISC-V.  particularly when their first port of call would be to go "hm
done the spec, what's the next bit, ah yes: write a proof-of-concept
by implementing the custom extension in spike".

 they would then find that the code's specifically not designed to
allow them to do that.  or, worse, they *don't* find it, and spend
considerable time debugging [and eventually come here and spend list
members' cpu cycles on enquiries finding out why]

Agreed; solved by adding comment.

 

>> secondly, there may be a potential conflict in how SETVL is to be
>> implemented using CSRs,
>> [...]

>> now, it could be reasonably claimed that the implementation of SETVL
>> "reading the CSR" could have "side-effects" which involve actually
>> setting VL: it's a bit of a stretch, and, honestly, in SV i feel i may
>> end up having to have a special-case version of csrrwi.h, which would
>> be a bit of a nuisance, but doable.
>
>
> It is indeed a stretch, which is why, as of a July 2017 draft, RVV
> doesn't use CSRRW to implement SETVL.  It is its own instruction,
> which just happens to manipulate CSRs.

 okaay.  and i've just noticed there's a cross-reference now to
 https://github.com/riscv/riscv-v-spec been added to
 V20180801-draft - excellent!

 https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#code-vl-code-and-code-vsetvl-code-instruction

 ok yes, got it: that's clear, "vsetvl not encoded as regular
CSRRW.... regular CSR can still read/write it"

 damn.  that's a pain.  if implementing [SV]SETVL as a CSRRWI
overload, SV requires absolutely no new instructions whatsoever.  may
just have to bite the bullet on that one and use an I-type encoding
(SV's SETVL is of the form SETVL t0, a0, #imm)

 thanks for clarifying, andrew.

You're welcome!

 

l.