`riscv`: register: fix target architecture conditional compilation

[rmsyn]

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

[rmsyn]

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]

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]

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]

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]

How are these custom compilation flags added currently?

These are set by the build script: https://github.com/rust-embedded/riscv/blob/a9d3e33ff9d321bb0b19e260c66e1d5fa7a4221c/riscv/build.rs#L4-L12

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]

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]

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?

[rmsyn]

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]

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 https://github.com/rust-embedded/riscv/pull/203. Exported macros need to reference cfg(target_arch = "..").

[rmsyn]

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.

[rmsyn]

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. :thinking:

[romancardenas]

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