search

riscvarchive / riscv-binutils-gdb

RISC-V backports for binutils-gdb. Development is done upstream at the FSF.
GNU General Public License v2.0
148 stars 232 forks source link

CSR 0x320 disassembles as `mucounteren`, rather than `mcountinhibit` #202

Closed rswarbrick closed 4 years ago

rswarbrick commented 4 years ago

At the moment, disassembling a CSR instruction that mentions CSR 0x320 gives you the name mucounteren. This name has presumably been kept for backwards compatibility: the mucounteren register was renamed to scounteren and moved to 0x106 back in 2017. Unfortunately, since then 0x320 has been repurposed and is now used for a new CSR (mcountinhibit); I think this new CSR appears in version 1.11 of the ISA. I can see an argument for continuing to assemble mucounteren by name (although maybe it should actually assemble to 0x106), but think that disassembly should probably never give that name.

jim-wilson commented 4 years ago

There is no 1.11 ISA yet. It is merely a draft, and is subject to change at any time. I've been told that parts of it, like the hypervisor stuff, definitely will change. It would be wrong to add support for a draft ISA to FSF binutils sources, because if the ISA changes after an FSF release, we are then stuck permanently supporting a register name/number that never officially existed. If you need to program to the draft, then you can use register numbers instead of names.

We currently don't have support to specify the privilege spec version when assembling. If you use mucounteren, then the assembler assumes you have 1.9.1 and it is 0x320. Likewise, if the disassembler sees 0x320 it assumes 1.9.1 and mucounteren. There is no conflict with 1.10, as 0x320 is not used in 1.10. And there is no 1.11 yet.

You are right that there is a conflict between 1.9.1 and 1.11. We will probably have to drop some support for 1.9.1 when we add 1.11 support. Or force people to start specifying privilege spec versions when they compile. I think this should be raised as an ISA issue. They should not be reusing register numbers when this is avoidable. That breaks backward compatibility with older systems built using older versions of the privilege spec.

jim-wilson commented 4 years ago

Sorry, I got the numbers wrong. 1.11 is official, 1.12 is a draft. The 0x320 change is already in 1.11, so we are already screwed. But the assembler you are using only supports 1.9.1 and 1.10 which is why it is still using mucounteren. When 1.11 support is added, this will be fixed. 1.11 support is already an item on the to do list and is waiting for a volunteer that has time to do the work.

aswaterman commented 4 years ago

The commitment since v1.10 has been to avoid egregious backwards incompatibilities with v1.10. (Recycling CSR numbers would be an example of an egregious backwards incompatibility.)

The horse is out of the barn on v1.9.1. If continued support for v1.9.1 and v1.1x is feasible, great. If not, I think removing support v1.9.1 is a reasonable course of action, even though it will ruffle some feathers.

rswarbrick commented 4 years ago

Right, this all makes sense. I don't know whether it's helpful to keep this issue open (since it's just part of "support v1.11"). Please feel free to close it if not.

Nelson1225 commented 4 years ago

The horse is out of the barn on v1.9.1. If continued support for v1.9.1 and v1.1x is feasible, great. If not, I think removing support v1.9.1 is a reasonable course of action, even though it will ruffle some feathers.

Umm, I think we shouldn't support the obsolete versions before we have a good solution that can check the versions of spec. The version checking is needed both for instructions and CSR, and this is also related to the -march parsing and elf build attributes. The version checking for these is one of my todo, and I should have time recently to deal with this.

Currently, the CSR are belonged to one of the enum classes which are used to check if the CSR is valid according to the ISA. So I believe we need similar classes for different versions. But it is hard to define the version classes if the CSR has the same name, but has different values for different spec versions. And for now we have Zicsr extension, so should we need to define the mapping between the ISA spec version and privileged version?

jim-wilson commented 4 years ago

I think supporting 1.11 which is already released is more important than continuing to support 1.9.1. People can always use older binutils releases if they need 1.9.1 support, but there is no binutils release with 1.11 support.

Longer term we will need to find a way to deal with versioning problems. We already had a break between 1.9 and 1.9.1 because misa changed its address and we can't handle that, so we dropped 1.9 support. Now we have a break between 1.9.1 and 1.11, so the short term solution is to drop 1.9.1 support, though if it is just one register we can drop the 1.9.1 support for that one reg and keep the rest of the 1.9.1 support, adding a comment pointing out that support for the 1.9.1 version of the reg is missing due to a conflict. We already have a comment like that for the 1.9 misa support. If we only drop support for one csr, most people probably won't notice, and they can always substitute the csr number for the csr name, which isn't too bad if it is only one csr.

Nelson1225 commented 4 years ago

I think supporting 1.11 which is already released is more important than continuing to support 1.9.1. People can always use older binutils releases if they need 1.9.1 support, but there is no binutils release with 1.11 support.

Longer term we will need to find a way to deal with versioning problems. We already had a break between 1.9 and 1.9.1 because misa changed its address and we can't handle that, so we dropped 1.9 support. Now we have a break between 1.9.1 and 1.11, so the short term solution is to drop 1.9.1 support, though if it is just one register we can drop the 1.9.1 support for that one reg and keep the rest of the 1.9.1 support, adding a comment pointing out that support for the 1.9.1 version of the reg is missing due to a conflict. We already have a comment like that for the 1.9 misa support. If we only drop support for one csr, most people probably won't notice, and they can always substitute the csr number for the csr name, which isn't too bad if it is only one csr.

Thanks for the information, it is helpful. The short term solution is good to me, we need to update the CSR to 1.11 which is already released, and we probably need to update the binutils, gdb, openOCD and qemu just like you mentioned before.. I will take care of this recently.

Nelson1225 commented 4 years ago

Hi @rswarbrick,

I think the current upstream binutils can support the priv 1.11 spec for CSR.

Thanks Nelson

rswarbrick commented 4 years ago

@Nelson1225 Thanks for the heads-up.

Nelson1225 commented 4 years ago

We already have supported this on FSF binutils. And now we have the new assembler and dis-assembler options -mpriv-sepc=[1.9.1 | 1.10 | 1.11] and -Mpriv-spec=[1.9.1 | 1.10 | 1.11] to choose which privileged spec you want. Besides, you can also set the assembler option -mcsr-check, and then assembler will warn you that which CSR are not valid for the chosen priv spec.

You can reopen the PR or create a new one if needed. Thanks.