Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline assembly Unable to dynamically access RISC-V CSRs #73220

Closed
alistair23 opened this issue Jun 10, 2020 · 6 comments
Closed

Inline assembly Unable to dynamically access RISC-V CSRs #73220

alistair23 opened this issue Jun 10, 2020 · 6 comments

Comments

@alistair23
Copy link
Contributor

With the new inline assembly it would be nice to be able to read/write from a RISC-V CSR, where the CSR value is passed in via a function argument (or some other means).

Currently there is no way to access the CSR without hard coding the address in the assembly.

const

I tried this code:

let value: usize = 0x3A0;
let r: u32;
unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = const value) }
r

Which results in this error:

error: argument 2 is required to be a constant
  --> libraries/riscv-csr/src/csr.rs:39:18
   |
39 |         unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = const value) }
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

in(reg)

I have tried this code:

let value: usize = 0x3A0;
let r: u32;
unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = in(reg) value) }
r

Which results in this error:

error: operand must be a valid system register name or an integer in the range [0, 4095]
  |
note: instantiated into assembly here
 --> <inline asm>:1:11
  |
1 |     csrr a4, s6

in(csr)

and I have tried this code:

let value: usize = 0x3A0;
let r: u32;
unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = in(csr) value) }
r

Which results in this error:

error: invalid register class `csr`: unknown register class
  --> libraries/riscv-csr/src/csr.rs:39:60
   |
39 |         unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = in(csr) value) }
   |                      

Obviously all of the above are incorrect as defined in the RFC, so that is fine.

It would be great if there is either a in(csr) option or the const option is expanded to allow non-consts (maybe a num option or something).

This seems like a common usecase to have a read_csr(csr_num: usize) -> u32 function that can read a CSR value from a number passed in.

Meta

rustc --version --verbose:

rustc 1.45.0-nightly (fe10f1a49 2020-06-02)
binary: rustc
commit-hash: fe10f1a49f5ca46e57261b95f46f519523f418fe
commit-date: 2020-06-02
host: x86_64-unknown-linux-gnu
release: 1.45.0-nightly
LLVM version: 10.0
Backtrace

<backtrace>

@alistair23 alistair23 added the C-bug Category: This is a bug. label Jun 10, 2020
@jonas-schievink jonas-schievink removed the C-bug Category: This is a bug. label Jun 10, 2020
@jonas-schievink
Copy link
Contributor

The CSR is encoded as an immediate, so it must be a value statically known at compile-time. Thus, closing as expected behavior.

@alistair23
Copy link
Contributor Author

So the plan is to never allow accessing them without hard coding the assembly?

@jonas-schievink
Copy link
Contributor

Well, "hard coding the assembly" is what a compiler does. Otherwise this would require generating or modifying machine code at runtime, which is something much more complex than inline assembly (IIRC GCC does it for a GNU C extension, breaking non-executable stacks in the process).

@alistair23
Copy link
Contributor Author

I was hoping we could have something like:

    fn read_csr(csr_num: usize) -> u32 {
        let r: u32;
        unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = csr_num) }
        r
    }

Then the calls to the function would produce assembly like this:

read_csr(0x10) which generates: csrr a0, 0x10
read_csr(0x20) which generates: csrr a0, 0x20

I see what you mean though that it would then be possible to have: read_csr(user_input * 10) which would require run-time generating of the instruction. Is it possible to force the input to be known at compile time?

Otherwise we would have to handwrite something like this:

    fn read_csr(csr_num: usize) -> u32 {
        let r: u32;
        if csr_num == 0x10 {
            unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = 0x10) }
        } else if csr_num == 0x20 {
            unsafe { asm!("csrr {rd}, {csr}", rd = out(reg) r, csr = 0x20) }
        }
        r
    }

@jonas-schievink
Copy link
Contributor

I'm not sure, but I can suggest asking questions on https://users.rust-lang.org/. Maybe using a trait with an associated const works?

@lenary
Copy link
Contributor

lenary commented Jun 17, 2020

@alistair23 I've got a proposal somewhere for LLVM and Clang builtins which would support this (as long as you can get LLVM to compile down the csr identifier to a constant). The path to upstreaming them probably involves a proper proposal to sw-dev though, and agreement on the intrinsic names.

bors bot added a commit to tock/tock that referenced this issue Jun 17, 2020
1926: Prepare for 64 PMP config registers r=hudson-ayers a=alistair23

### Pull Request Overview

This PR was supposed to add support for 64 PMP config register on RISC-V now that it is supported by the spec.

Instead this patch has turned more info converting the riscv-csr library to use the new inline assembly macro and to allow dynamic array access to the RISC-V registers.

This PR does a few things:
 - Converts the PMP CSRs to an array (to allow simplifying the PMP code)
 - Uses the new inline assembly. Unfortunately Rust won't let use access CSR dynamically like we used to (see rust-lang/rust#73220) so we now have a giant if statement to see which one to access. I couldn't see any other way to make the value const.
 - We also implement the register functions on specific types (u32) instead of generics. This is because with generics we can't dynamically access arrays of CSRs with the inline assembly.
 - Finally we clean up some of the PMP code.

### Testing Strategy

None yet.

### TODO or Help Wanted

Comments please, this is uglier then I would like it to be.

### Documentation Updated

- [X] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [X] Ran `make prepush`.


Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants