riscv-rt: Add mtvec-align features for the vector table alignment

[ia0]

The RISC-V Privileged Specification (version 1.11) for mtvec CSR says:

The value in the BASE field must always be aligned on a 4-byte boundary, and the MODE setting may
impose additional alignment constraints on the value in the BASE field.
[...]
An implementation may have different alignment constraints for different modes. In particular,
MODE=Vectored may have stricter alignment constraints than MODE=Direct.

The Ibex Reference Guide for mtvec CSR says¹:

The trap-vector base address, always aligned to 256 bytes

Other implementations have yet other alignment constraints (see #259 (comment)).

This PR introduces the mtvec-align-{16,64,256} cargo features to let users choose the alignment constraint appropriate to their implementation.

Footnotes

1. https://ibex-core.readthedocs.io/en/latest/03_reference/cs_registers.html#machine-trap-vector-base-address-mtvec ↩

[hegza]

Truly, each interrupt controller has its own alignment requirements. For instance, the riscv-fast-interrupt specification (CLIC) specifies 4-byte alignment in CLINT mode and 64-byte alignment in CLIC mode.

This has been an issue for us at https://github.com/soc-hub-fi/atalanta working on an Ibex-based CPU design as well, as not being able to parameterize this forces us to flag out & write the trap vector ourselves. I'd be in favor of a flag such as mtvec-align-256, though I don't know if that scales nicely.

[romancardenas]

I think the best approach is using a linker symbol, such as _heap_size, for instance. By default, the symbol should be 4. PACs can overwrite it according to their particular needs.

The linker script should check that the alignment is a power of 2, I guess (and greater than 4)

[rmsyn]

I'm not sure how we can parametrize mtvec alignment without cargo features, so this PR simply uses the stricter alignment.

One possibility is to use const generics to parameterize the Mtvec::set_address function directly, or offer another Mtvec::set_address_aligned<const N: usize>.

Something like:

impl Mtvec {
    pub fn set_address_aligned<const N: usize>(&mut self, address: usize) {
        // check alignment does not violate spec requirements
        const _: () = assert!(N >= 4);
        const _: () = assert_eq!(N % 4, 0);
        const MASK: usize = usize::MAX & !((1usize << N) - 1);

        self.set_address(address & MASK);
    }
}

The above could also obviously be made into a fallible try_set_address_aligned, just used this one for brevity.

---

For the actual alignment of the trap vector function, I think @romancardenas approach is the right way to go.

[ia0]

I'd be in favor of a flag such as mtvec-align-256, though I don't know if that scales nicely.

It kind of scales thanks to the cfg_global_asm! macro (which I had to move to crate-level). However, this has an impact on compile time since this expands the macro multiple times. But that's similar to what's done in riscv-rt/src/asm.rs anyway.

I used the values 4 (default), 16, 64, and 256, since 4, 64, and 256 are used, and 16 is the missing power of 2 with an even exponent.

I think the best approach is using a linker symbol, such as _heap_size, for instance.

I don't think this works? .balign takes an absolute expression. Feel free to demonstrate what you had in mind with a PR if I misunderstood it.

One possibility is to use const generics to parameterize the Mtvec::set_address function directly

This seems unrelated. This PR is about letting users configure the alignment of the vector table in riscv-rt. What you mention seems to be a functionality of riscv and already too late, since the problem is compile-time, not runtime.

[romancardenas]

Perhaps another option is using a procedural macro with environment variables (e.g., RISCV_RT_MTVEC_ALIGN). Something like this:

riscv/riscv-rt/build.rs

Line 44 in b391a48

"cargo:rustc-env=RISCV_RT_LLVM_ARCH_PATCH={}",

PACs would set the mtvec alignment as part of their build.rs script. Then, the riscv-rt-macros would generate the assembly code for the table according to that table. If the environment variable is not defined, the default alignment is 4.
The macro can check that the alignment is a power of 2, or any check that we consider necessary.

(I'm not against the approach of this PR, but I'd rather have something more configurable that would not potentially lead to more features etc.)

[romancardenas]

THe riscv-macros crate already provides a similar macro:

riscv/riscv/macros/src/lib.rs

Line 277 in b391a48

.balign 0x4 // TODO check if this is the correct alignment

We can make a new version of this macro that also allows to define the alignment of the vector table and use it in riscv-rt,

[romancardenas]

The good thing about the riscv-macros macro is that it serves for custom core interrupt enumeration, not only the enumeration provided by the RISC-V standard

[ia0]

Perhaps another option is using a procedural macro with environment variables (e.g., RISCV_RT_MTVEC_ALIGN).

This looks much better! I've updated the PR.

[ia0]

THe riscv-macros crate already provides a similar macro:

Ah, I've seen this too late. I'll update the PR to reuse it.

[ia0]

THe riscv-macros crate already provides a similar macro:

Ah, I've seen this too late. I'll update the PR to reuse it.

Actually, I don't think we can reuse it without creating a separate library for proc-macro helpers (or possibly making riscv-rt depend on riscv-macros). What do you think?

[romancardenas]

Good (and delicate) question. Creating a new riscv-util-macro is an option, but not my favorite.

I personally don't want riscv to depend on riscv-rt-macros. Maybe someone wants to use riscv with a different runtime, and keeping them as much isolated as possible is desirable.. However, I don't see any issue in riscv-rt-macros depending on riscv-macros. After all, users already had already chosen to use riscv-rt.

Any thoughts/suggestions/concerns?

[ia0]

However, I don't see any issue in riscv-rt-macros depending on riscv-macros.

A proc-macro cannot depend on another proc-macro as a library. So it would only be riscv-rt depending on riscv-macros.

I updated the PR.

[romancardenas]

__CORE_INTERRUPTS and _dispatch_core_interrupt are also generated by the pac_enum macro:

riscv/riscv/macros/src/lib.rs

Line 380 in cfb7128

pub static #vector_table: [Option<unsafe extern "C" fn(#(#array_signature),*)>; #max_discriminant + 1] = [

riscv/riscv/macros/src/lib.rs

Line 386 in cfb7128

unsafe extern "C" fn #dispatch_fn_name(#(#dispatch_fn_args),*) {

They are only necessary when the v-trap feature is disabled, though the pac_enum macro does not include that feature gate currently.

I think the way to go is:

• Modifying the pac_enum macro so __CORE_INTERRUPTS and _dispatch_core_interrupt are only active when v-trap is disabled.
• Removing __CORE_INTERRUPTS and _dispatch_core_interrupt from riscv_rt::interrupts.rs and leave the macro implementation.
• Do not feature-gate the pac_enum macro invocation in riscv_rt::interrupts.rs

[romancardenas]

Actually, the first action point is not that immediate. v-trap feature gates only apply for CoreInterruptNumber. For other ExceptionHandler and ExternalInterruptHandler, there should not be feature gates.

In other words ,the macro should only add feature gates when this condition is met:

riscv/riscv/macros/src/lib.rs

Line 399 in cfb7128

if let PacTrait::Interrupt(InterruptType::Core) = attr {

[rmsyn]

Minor nit, removes the need for a mutable align variable.

[ia0]

I think the way to go is:

I updated the PR accordingly (also gating the extern declarations since they were in src/interrupts.rs).

[romancardenas]

LGTM! I added a minor documentation suggestion.

I think we should add a directive toriscv-rt's build script to re-compile the crate if RISCV_MTVEC_ALIGN changes its value, right after this:

riscv/riscv-rt/build.rs

Line 49 in b391a48

println!("cargo:rerun-if-env-changed=RISCV_RT_LLVM_ARCH_PATCH");

We can opt-out this check if the v-trap vector is disabled (no vector table at all) or if the no-interrupts feature is enabled (i.e., a PAC is implementing its custom core interrupt vector).

PACs with custom core interrupt vectors (i.e., those that use riscv::pac_enum(CoreInterruptNumber) must also include a similar directive to their build.rs. We can also include this in the docs.

[romancardenas]

Could you add a few lines of documentation for vectored mode about the RISCV_MTVEC_ALIGN environment variable and why it is necessary? You should also mention that, in most of the cases, this environment variable must be handled by the PAC crate, not users directly

[ia0]

I'm not good at documentation, so I attempted something. Feel free to suggest an alternative.