Closed rmsyn closed 3 months ago
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.
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.
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 panic
ing 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>
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.
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)
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.
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?
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 panic
s 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);
With #205, the code
panic
s from theunimplemented
functions. For some reason, the customcfg
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 = "..")
.
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.
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:
Let's merge #205 before. It should fix the current clippy issues.
Uses a conditional compilation selector set by modern compiler versions for code gated behind a target architecture.