`riscv`: register: fix target architecture conditional compilation #204

rmsyn · 2024-05-03T00:24:43Z

Uses a conditional compilation selector set by modern compiler versions for code gated behind a target architecture.

rmsyn · 2024-05-03T01:30:23Z

If this is approved, I can open another PR to use these selectors in the rest of the library, or add more commits to do it here.

romancardenas · 2024-05-03T09:15:41Z

Regarding using target_arch instead of custom compilation flags, I don't know what to say. We would still need flags such as riscve for ISA extensions.

rmsyn · 2024-05-03T20:16:45Z

Regarding using target_arch instead of custom compilation flags, I don't know what to say.

How are these custom compilation flags added currently?

I ran into this issue because of a crate I'm working on panicing during boot.

Took a while to trace the cause to this conditional compilation.

We would still need flags such as riscve for ISA extensions.

I think this would work for what we would want for RISC-V extensions:

https://doc.rust-lang.org/reference/conditional-compilation.html#target_feature

To get a list of available features by target:

$ rustc --print target-features --target <riscv-target-triple>

rmsyn · 2024-05-03T20:58:47Z

Added some fixes for the rest of the riscv crate. Let me know if you would like me to split these changes into a separate PR instead.

taiki-e · 2024-05-04T09:06:54Z

How are these custom compilation flags added currently?

These are set by the build script:

riscv/riscv/build.rs

Lines 4 to 12 in a9d3e33

    
           let target = env::var("TARGET").unwrap(); 
        
           if target.starts_with("riscv32") { 
        
               println!("cargo:rustc-cfg=riscv"); 
        
               println!("cargo:rustc-cfg=riscv32"); 
        
           } else if target.starts_with("riscv64") { 
        
               println!("cargo:rustc-cfg=riscv"); 
        
               println!("cargo:rustc-cfg=riscv64"); 
        
           }

However, target.starts_with("riscv32")/target.starts_with("riscv64") is not very robust and will not work if the target is a custom target with a name is something like riscv-unknown-*. (Referring to CARGO_CFG_TARGET_ARCH instead of TARGET is a robust way here)

romancardenas · 2024-05-04T12:13:08Z

Added some fixes for the rest of the riscv crate. Let me know if you would like me to split these changes into a separate PR instead.

Please, do so in a separate PR. I am not sure whether it is worth it or not. Notice that feature gates become way more verbose. However, changing the build script as suggested by @taiki-e sounds more than reasonable.

rmsyn · 2024-05-04T20:56:31Z

However, changing the build script as suggested by @taiki-e sounds more than reasonable.

I was exploring this a bit, as well. I'll try the suggestion by @taiki-e. If it works, I'll apply that patch instead.

@taiki-e would you like to make the PR, or would you like me to?

Change which Cargo `CFG` environment variables are used to select the custom `cfg` variables. See rust-embedded#204 (comment) for discussion.

rmsyn · 2024-05-04T23:39:49Z

I am not sure whether it is worth it or not. Notice that feature gates become way more verbose.

So, I A/B tested the changes in this PR and #205. The changes here work as expected, calls into the various asm and register::macro calls call the expected RISC-V assembly instructions.

With #205, the code panics from the unimplemented functions. For some reason, the custom cfg arguments are not making it through to downstream users. Any thoughts?

I agree the verbosity is a bit of a problem, but a small price to pay for functioning code.

Here is assembly using changes in this PR, from the jh71xx-hal-examples crate:

;-- with riscv@fixup/riscv/macro-conditional-comp
;-- jh71xx_hal::register::feature_disable::_clear::h1696354862e236cf:
0x08002c68      4111           addi sp, sp, -16            ; jh71xx_hal::register::feature_disable::_clear::h1696354862e236cf
0x08002c6a      2ae4           sd a0, 8(sp)
0x08002c6c      7330157c       csrc 0x7c1, a0              ; jalr -24(ra) from `clear_all` jumps here
0x08002c70      4101           addi sp, sp, 16
0x08002c72      8280           ret

;-- jh71xx_hal::register::feature_disable::clear_all::h38eafbba3f744711:
0x08002c74      4111           addi sp, sp, -16            ; jh71xx_hal::register::feature_disable::clear_all::h38eafbba3f744711
0x08002c76      06e4           sd ra, 8(sp)
0x08002c78      37050300       lui a0, 0x30
0x08002c7c      1b05f520       addiw a0, a0, 527
0x08002c80      97000000       auipc ra, 0x0
0x08002c84      e78080fe       jalr -24(ra)                ; this is the main difference, offset is `-24`
0x08002c88      a260           ld ra, 8(sp)
0x08002c8a      4101           addi sp, sp, 16
0x08002c8c      8280           ret

Here is the assembly using the changes in #205:

;-- with riscv@build/robust-cfg
;-- jh71xx_hal::register::feature_disable::_clear::h1696354862e236cf:
;-- $x.0:
0x08002c68      4111           addi sp, sp, -16            ; jh71xx_hal::register::feature_disable::_clear::h1696354862e236cf
0x08002c6a      2ae4           sd a0, 8(sp)
;-- .Lpcrel_hi0:
0x08002c6c      17650100       auipc a0, 0x16              ; jalr -42(ra) from `clear_all` jumps here
0x08002c70      1305c5d1       addi a0, a0, -740
;-- .Lpcrel_hi1:
0x08002c74      97650100       auipc a1, 0x16
0x08002c78      1386c5d5       addi a2, a1, -676
0x08002c7c      bd45           li a1, 15
0x08002c7e      97300100       auipc ra, 0x13
0x08002c82      e78000ff       jalr -16(ra)
;-- jh71xx_hal::register::feature_disable::clear_all::h38eafbba3f744711:
0x08002c86      4111           addi sp, sp, -16            ; jh71xx_hal::register::feature_disable::clear_all::h38eafbba3f744711
0x08002c88      06e4           sd ra, 8(sp)
0x08002c8a      37050300       lui a0, 0x30
0x08002c8e      1b05f520       addiw a0, a0, 527
0x08002c92      97000000       auipc ra, 0x0
0x08002c96      e78060fd       jalr -42(ra)                ; this is the main difference, offset is `-42`
0x08002c9a      a260           ld ra, 8(sp)
0x08002c9c      4101           addi sp, sp, 16
0x08002c9e      8280           ret

And the code for jh71xx_hal::register::feature_disable:

//! Representation of the custom SiFive CSR for disabling U74 (MC) SoC features.
//!
//! Ss. 7.6 SiFive Feature Disable CSR, "U74MC Core Complex Manual 21 G1"
//!
//! <https://starfivetech.com/uploads/u74_core_complex_manual_21G1.pdf>

/// Bit-field mask that represents the settable fields in the [FeatureDisable] CSR.
pub const FIELD_MASK: usize = 0b11_0000_0010_0000_1111;

/// Represents the custom SiFive CSR for disabling U74 (MC) SoC features.
#[derive(Clone, Copy, Debug)]
pub struct FeatureDisable {
    bits: usize,
}

impl FeatureDisable {
    /// Returns the contents of the register as raw bits.
    #[inline]
    pub fn bits(&self) -> usize {
        self.bits
    }

    /// Disable data cache clock gating.
    #[inline]
    pub fn dccg(&self) -> bool {
        self.bits & (1 << 0) != 0
    }

    /// Disable instruction cache clock gating.
    #[inline]
    pub fn iccg(&self) -> bool {
        self.bits & (1 << 1) != 0
    }

    /// Disable pipeline clock gating.
    #[inline]
    pub fn pcg(&self) -> bool {
        self.bits & (1 << 2) != 0
    }

    /// Disable speculative instruction cache refill.
    #[inline]
    pub fn sicr(&self) -> bool {
        self.bits & (1 << 3) != 0
    }

    /// Suppress corrupt signal on GrantData messages.
    #[inline]
    pub fn csgdm(&self) -> bool {
        self.bits & (1 << 9) != 0
    }

    /// Disable short forward branch optimization.
    #[inline]
    pub fn sfbo(&self) -> bool {
        self.bits & (1 << 16) != 0
    }

    /// Disable instruction cache next-line prefetcher.
    #[inline]
    pub fn icnlp(&self) -> bool {
        self.bits & (1 << 17) != 0
    }
}

riscv::read_csr_as!(FeatureDisable, 0x7c1);
riscv::write_csr_as_usize!(0x7c1);
riscv::set!(0x7c1);
riscv::clear!(0x7c1);

riscv::set_clear_csr!(
    /// Disable data cache clock gating.
    , set_dccg, clear_dccg, 1 << 0);
riscv::set_clear_csr!(
    /// Disable instruction cache clock gating.
    , set_iccg, clear_iccg, 1 << 1);
riscv::set_clear_csr!(
    /// Disable pipeline clock gating.
    , set_pcg, clear_pcg, 1 << 2);
riscv::set_clear_csr!(
    /// Disable speculative instruction cache refill.
    , set_sicr, clear_sicr, 1 << 3);
riscv::set_clear_csr!(
    /// Suppress corrupt signal on GrantData messages.
    , set_csgdm, clear_csgdm, 1 << 9);
riscv::set_clear_csr!(
    /// Disable short forward branch optimization.
    , set_sfbo, clear_sfbo, 1 << 16);
riscv::set_clear_csr!(
    /// Disable instruction cache next-line prefetcher.
    , set_icnlp, clear_icnlp, 1 << 17);
riscv::set_clear_csr!(
    /// Disable all features.
    , set_all, clear_all, FIELD_MASK);

taiki-e · 2024-05-05T01:10:15Z

With #205, the code panics from the unimplemented functions. For some reason, the custom cfg arguments are not making it through to downstream users. Any thoughts?

The cfg set by the build script affects only the crate where the build script is located.

And I think that is what was missed in #203. Exported macros need to reference cfg(target_arch = "..").

taiki-e · 2024-05-05T01:17:31Z

riscv/src/register/macros.rs

@@ -10,14 +10,14 @@ macro_rules! read_csr {
        #[inline]
        unsafe fn _read() -> usize {
            match () {
-                #[cfg(riscv)]
+                #[cfg(target_arch = "riscv64")]


I think replacing riscv -> target_arch = "riscv64" in this file is not correct: #203 (comment)

This was my mistake, I'll fix it in the next set of fixups to this PR. Along with the comments about non-_rv32 macros.

rmsyn · 2024-05-05T03:31:46Z

Exported macros need to reference cfg(target_arch = "..").

So, maybe just the exported macros get cfg(target_arch = ".."), and the rest can use the existing cfg(riscv*)?

If so, I can drop the rest of the commits for code outside the riscv::register::macros module.

Change which Cargo `CFG` environment variables are used to select the custom `cfg` variables. See rust-embedded#204 (comment) for discussion.

taiki-e · 2024-05-05T05:10:39Z

riscv/CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed

 - Fixed `sip::set_ssoft` and `sip::clear_ssoft` using wrong address
+- Fixed conditional compilation selectors for `riscv::register::macros`


#203 has not been released yet (i.e., users of the released version have never been affected by the bug), so I don't think the changelog entry for this is needed.

Where should it go then, or are you saying it doesn't need a CHANGELOG.md entry at all?

Asking because the CI check fails without it an entry.

I added a tag to skip the changelog CI

I added a tag to skip the changelog CI

Sounds good, I'll remove it when I rebase the changes on #205.

rmsyn · 2024-05-05T05:24:35Z

I don't know why the clippy checks are suddenly failing now. I run the checks locally from CI runner:

cargo clippy --package riscv-rt --all --features=s-mode -- -D warnings

The checks pass. 🤔

romancardenas

Looks good to me! I am OK with using riscv, riscv32, etc. in the crate but using cfg(target_arch = _) in public macros.

romancardenas · 2024-05-05T15:09:33Z

riscv/CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed

 - Fixed `sip::set_ssoft` and `sip::clear_ssoft` using wrong address
+- Fixed conditional compilation selectors for `riscv::register::macros`


I added a tag to skip the changelog CI

romancardenas · 2024-05-05T15:26:47Z

Let's merge #205 before. It should fix the current clippy issues.

Uses `cfg(target_arch)` conditional compilation selectors for exported macros, since `cfg` flags generated from `build.rs` are not present for downstream users.

romancardenas

LGTM

rmsyn requested a review from a team as a code owner May 3, 2024 00:24

rmsyn force-pushed the fixup/riscv/macro-conditional-comp branch from 7d70619 to d7add6a Compare May 3, 2024 00:27

rmsyn changed the title ~~riscv: fix target architecture condition compilation~~ riscv: register: fix target architecture conditional compilation May 3, 2024

romancardenas previously approved these changes May 3, 2024

View reviewed changes

rmsyn dismissed romancardenas’s stale review via 9fc99d7 May 3, 2024 20:55

rmsyn added a commit to rmsyn/riscv that referenced this pull request May 4, 2024

riscv: build: make cfg variables more robust

60ff795

Change which Cargo `CFG` environment variables are used to select the custom `cfg` variables. See rust-embedded#204 (comment) for discussion.

rmsyn mentioned this pull request May 4, 2024

riscv: build: make cfg variables more robust #205

Merged

rmsyn added a commit to rmsyn/riscv that referenced this pull request May 4, 2024

riscv: build: make cfg variables more robust

75bca98

Change which Cargo `CFG` environment variables are used to select the custom `cfg` variables. See rust-embedded#204 (comment) for discussion.

taiki-e reviewed May 5, 2024

View reviewed changes

rmsyn added a commit to rmsyn/riscv that referenced this pull request May 5, 2024

riscv: build: make cfg variables more robust

7c280ee

Change which Cargo `CFG` environment variables are used to select the custom `cfg` variables. See rust-embedded#204 (comment) for discussion.

rmsyn force-pushed the fixup/riscv/macro-conditional-comp branch from 8a3c865 to da244da Compare May 5, 2024 04:48

rmsyn added a commit to rmsyn/riscv that referenced this pull request May 5, 2024

riscv: build: make cfg variables more robust

942a45e

Change which Cargo `CFG` environment variables are used to select the custom `cfg` variables. See rust-embedded#204 (comment) for discussion.

taiki-e reviewed May 5, 2024

View reviewed changes

romancardenas added the skip changelog label May 5, 2024

romancardenas previously approved these changes May 5, 2024

View reviewed changes

riscv: use cfg(target_arch) for exported macros

72f879c

Uses `cfg(target_arch)` conditional compilation selectors for exported macros, since `cfg` flags generated from `build.rs` are not present for downstream users.

rmsyn dismissed romancardenas’s stale review via 72f879c May 8, 2024 00:16

rmsyn force-pushed the fixup/riscv/macro-conditional-comp branch from da244da to 72f879c Compare May 8, 2024 00:16

romancardenas approved these changes May 8, 2024

View reviewed changes

romancardenas added this pull request to the merge queue May 8, 2024

Merged via the queue into rust-embedded:master with commit b77662f May 8, 2024
95 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`riscv`: register: fix target architecture conditional compilation #204

`riscv`: register: fix target architecture conditional compilation #204

rmsyn commented May 3, 2024

rmsyn commented May 3, 2024

romancardenas commented May 3, 2024

rmsyn commented May 3, 2024

rmsyn commented May 3, 2024

taiki-e commented May 4, 2024

romancardenas commented May 4, 2024

rmsyn commented May 4, 2024

rmsyn commented May 4, 2024 •

edited

Loading

taiki-e commented May 5, 2024 •

edited

Loading

taiki-e May 5, 2024

rmsyn May 5, 2024

rmsyn commented May 5, 2024

taiki-e May 5, 2024

rmsyn May 5, 2024

romancardenas May 5, 2024

rmsyn May 5, 2024

rmsyn commented May 5, 2024

romancardenas left a comment

romancardenas May 5, 2024

romancardenas commented May 5, 2024

romancardenas left a comment

riscv: register: fix target architecture conditional compilation #204

riscv: register: fix target architecture conditional compilation #204

Conversation

rmsyn commented May 3, 2024

rmsyn commented May 3, 2024

romancardenas commented May 3, 2024

rmsyn commented May 3, 2024

rmsyn commented May 3, 2024

taiki-e commented May 4, 2024

romancardenas commented May 4, 2024

rmsyn commented May 4, 2024

rmsyn commented May 4, 2024 • edited Loading

taiki-e commented May 5, 2024 • edited Loading

taiki-e May 5, 2024

Choose a reason for hiding this comment

rmsyn May 5, 2024

Choose a reason for hiding this comment

rmsyn commented May 5, 2024

taiki-e May 5, 2024

Choose a reason for hiding this comment

rmsyn May 5, 2024

Choose a reason for hiding this comment

romancardenas May 5, 2024

Choose a reason for hiding this comment

rmsyn May 5, 2024

Choose a reason for hiding this comment

rmsyn commented May 5, 2024

romancardenas left a comment

Choose a reason for hiding this comment

romancardenas May 5, 2024

Choose a reason for hiding this comment

romancardenas commented May 5, 2024

romancardenas left a comment

Choose a reason for hiding this comment

`riscv`: register: fix target architecture conditional compilation #204

`riscv`: register: fix target architecture conditional compilation #204

rmsyn commented May 4, 2024 •

edited

Loading

taiki-e commented May 5, 2024 •

edited

Loading