arch: rv32i: fix comments #4059

bradjc · 2024-07-03T02:39:13Z

Pull Request Overview

This allows the docs to build.

Testing Strategy

Ran cargo doc

TODO or Help Wanted

n/a

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

lschuermann · 2024-07-03T02:43:42Z

arch/rv32i/src/lib.rs

 /// as suggested by the RISC-V developers:
-/// https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/XKkYacERM04/m/CdpOcqtRAgAJ
+/// <https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/XKkYacERM04/m/CdpOcqtRAgAJ>
 #[cfg(all(target_arch = "riscv32", target_os = "none"))]


Ideally we'd do #[cfg(any(doc, all(target_arch = "riscv32", target_os = "none")))] here, and then !doc for the other branch. This ensures that we generate the correct docs regardless of --target passed to Rustdoc.

Should probably be a repo-wide, distinct PR though.

Please no more extra nonsense to fix a nonsensical problem. There is absolutely no purpose to compile a crate called "rv32i" on any architecture other than rv32i. That cargo doesn't understand that is frustrating but unless something is broken I want to avoid adding more workarounds.

Please no more extra nonsense to fix a nonsensical problem.

I understand the frustration, but this is a real (albeit arguably nonsensical) problem. #4054 will fix this, but the way that we call out to Rustdoc right now breaks exactly this -- https://docs.tockos.org/rv32i/fn._start_trap, for example, completely lacks docs that are present in source:

tock/arch/rv32i/src/lib.rs

Lines 185 to 271 in 9d1f488

/// This is the trap handler function. This code is called on all traps,

/// including interrupts, exceptions, and system calls from applications.

///

/// Tock uses only the single trap handler, and does not use any vectored

/// interrupts or other exception handling. The trap handler has to

/// determine why the trap handler was called, and respond

/// accordingly. Generally, there are two reasons the trap handler gets

/// called: an interrupt occurred or an application called a syscall.

///

/// In the case of an interrupt while the kernel was executing we only need

/// to save the kernel registers and then run whatever interrupt handling

/// code we need to. If the trap happens while an application was executing,

/// we have to save the application state and then resume the `switch_to()`

/// function to correctly return back to the kernel.

///

/// We implement this distinction through a branch on the value of the

/// `mscratch` CSR. If, at the time the trap was taken, it contains `0`, we

/// assume that the hart is currently executing kernel code.

///

/// If it contains any other value, we interpret it to be a memory address

/// pointing to a particular data structure:

///

/// ```

/// mscratch 0 1 2 3

/// \->|--------------------------------------------------------------|

/// | scratch word, overwritten with s1 register contents |

/// |--------------------------------------------------------------|

/// | trap handler address, continue trap handler execution here |

/// |--------------------------------------------------------------|

/// ```

///

/// Thus, process implementations can define their own strategy for how

/// traps should be handled when they occur during process execution. This

/// global trap handler behavior is well defined. It will:

///

/// 1. atomically swap s0 and the mscratch CSR,

///

/// 2. execute the default kernel trap handler if s0 now contains `0`

/// (meaning that the mscratch CSR contained `0` before entering this trap

/// handler),

///

/// 3. otherwise, save s1 to `0*4(s0)`, and finally

///

/// 4. load the address at `1*4(s0)` into s1, and jump to it.

///

/// No registers other than s0, s1 and the mscratch CSR are to be clobbered

/// before continuing execution at the address loaded into the mscratch CSR

/// or the _start_kernel_trap kernel trap handler. Execution with these

/// second-stage trap handlers must continue in the same trap handler

/// context as originally invoked by the trap (e.g., the global trap handler

/// will not execute an mret instruction). It will not modify CSRs that

/// contain information on the trap source or the system state prior to

/// entering the trap handler.

///

/// We deliberately clobber callee-saved instead of caller-saved registers,

/// as this makes it easier to call other functions as part of the trap

/// handler (for example to to disable interrupts from within Rust

/// code). This global trap handler saves the previous values of these

/// clobbered registers ensuring that they can be restored later. It places

/// new values into these clobbered registers (such as the previous `s0`

/// register contents) that are required to be retained for correctly

/// returning from the trap handler, and as such need to be saved across

/// C-ABI function calls. Loading them into saved registers avoids the need

/// to manually save them across such calls.

///

/// When a custom trap handler stack is registered in `mscratch`, the custom

/// handler is responsible for restoring the kernel trap handler (by setting

/// mscratch=0) before returning to kernel execution from the trap handler

/// context.

///

/// If a board or chip must, for whichever reason, use a different global

/// trap handler, it should abide to the above contract and emulate its

/// behavior for all traps and interrupts that are required to be handled by

/// the respective kernel or other trap handler as registered in mscratch.

///

/// For instance, a chip that does not support non-vectored trap handlers

/// can register a vectored trap handler that routes each trap source to

/// this global trap handler.

///

/// Alternatively, a board can be allowed to ignore certain traps or

/// interrupts, some or all of the time, provided they are not vital to

/// Tock's execution. These boards may choose to register an alternative

/// handler for some or all trap sources. When this alternative handler is

/// invoked, it may, for instance, choose to ignore a certain trap, access

/// global state (subject to synchronization), etc. It must still abide to

/// the contract as stated above.

pub fn _start_trap();

#[cfg(doc)] is the intended way of doing this(TM): https://doc.rust-lang.org/rustdoc/advanced-features.html#cfgdoc-documenting-platform-specific-or-feature-specific-information

I don't like this, and neither do I want to make this repo-wide change. I'm merely suggesting what "solution" upstream Rust has to offer.

There is absolutely no purpose to compile a crate called "rv32i" on any architecture other than rv32i.

For what its worth, at this point why don't we just get rid of all conditional compilation and break compilations for all other targets deliberately?

#[cfg(not(all(target_arch = "riscv32", target_os = "none")))] compile_error!("Crate rv32i can only be built on a rv32i*-unknown-none-elf toolchain!");

Wait. We can do that now? Talk about buying the lede! Hmm...suddenly hidden files don't seem so bad. (All the other problems are still there, however.)

But let's just delete the code we never would have written if make check just did the right thing.

Yeah, this doesn't seem to work :(

Yeah, it does not force cargo to build for a specific target. As discussed on the call today, we still want to support doc builds for other targets, as that's how our doc builds work (for better or for worse).

From what we talked about today, it seemed like #[cfg(doc)] was a somewhat reasonable option and could allow us to get rid of the make alldocs bodge, which seems brittle anyways. However let's table that until we have #4054 in at least.

arch: rv32i: fix comments

3200c2d

github-actions bot added the risc-v RISC-V architecture label Jul 3, 2024

bradjc mentioned this pull request Jul 3, 2024

Compile without Make / migrate flags to .cargo/config.toml #4054

Closed

2 tasks

lschuermann approved these changes Jul 3, 2024

View reviewed changes

lschuermann added the P-Upkeep This a relatively minor change, or one that is limited in scope, and requires less scrutiny. label Jul 3, 2024

lschuermann added this pull request to the merge queue Jul 5, 2024

Merged via the queue into master with commit 91af070 Jul 5, 2024
18 checks passed

lschuermann deleted the arch-rv32i-docs branch July 5, 2024 16:26

lschuermann mentioned this pull request Jul 22, 2024

arch: fix clippy warnings #4117

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

arch: rv32i: fix comments #4059

arch: rv32i: fix comments #4059

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	/// This is the trap handler function. This code is called on all traps,
	/// including interrupts, exceptions, and system calls from applications.
	///
	/// Tock uses only the single trap handler, and does not use any vectored
	/// interrupts or other exception handling. The trap handler has to
	/// determine why the trap handler was called, and respond
	/// accordingly. Generally, there are two reasons the trap handler gets
	/// called: an interrupt occurred or an application called a syscall.
	///
	/// In the case of an interrupt while the kernel was executing we only need
	/// to save the kernel registers and then run whatever interrupt handling
	/// code we need to. If the trap happens while an application was executing,
	/// we have to save the application state and then resume the `switch_to()`
	/// function to correctly return back to the kernel.
	///
	/// We implement this distinction through a branch on the value of the
	/// `mscratch` CSR. If, at the time the trap was taken, it contains `0`, we
	/// assume that the hart is currently executing kernel code.
	///
	/// If it contains any other value, we interpret it to be a memory address
	/// pointing to a particular data structure:
	///
	/// ```
	/// mscratch 0 1 2 3
	/// \->\|--------------------------------------------------------------\|
	/// \| scratch word, overwritten with s1 register contents \|
	/// \|--------------------------------------------------------------\|
	/// \| trap handler address, continue trap handler execution here \|
	/// \|--------------------------------------------------------------\|
	/// ```
	///
	/// Thus, process implementations can define their own strategy for how
	/// traps should be handled when they occur during process execution. This
	/// global trap handler behavior is well defined. It will:
	///
	/// 1. atomically swap s0 and the mscratch CSR,
	///
	/// 2. execute the default kernel trap handler if s0 now contains `0`
	/// (meaning that the mscratch CSR contained `0` before entering this trap
	/// handler),
	///
	/// 3. otherwise, save s1 to `0*4(s0)`, and finally
	///
	/// 4. load the address at `1*4(s0)` into s1, and jump to it.
	///
	/// No registers other than s0, s1 and the mscratch CSR are to be clobbered
	/// before continuing execution at the address loaded into the mscratch CSR
	/// or the _start_kernel_trap kernel trap handler. Execution with these
	/// second-stage trap handlers must continue in the same trap handler
	/// context as originally invoked by the trap (e.g., the global trap handler
	/// will not execute an mret instruction). It will not modify CSRs that
	/// contain information on the trap source or the system state prior to
	/// entering the trap handler.
	///
	/// We deliberately clobber callee-saved instead of caller-saved registers,
	/// as this makes it easier to call other functions as part of the trap
	/// handler (for example to to disable interrupts from within Rust
	/// code). This global trap handler saves the previous values of these
	/// clobbered registers ensuring that they can be restored later. It places
	/// new values into these clobbered registers (such as the previous `s0`
	/// register contents) that are required to be retained for correctly
	/// returning from the trap handler, and as such need to be saved across
	/// C-ABI function calls. Loading them into saved registers avoids the need
	/// to manually save them across such calls.
	///
	/// When a custom trap handler stack is registered in `mscratch`, the custom
	/// handler is responsible for restoring the kernel trap handler (by setting
	/// mscratch=0) before returning to kernel execution from the trap handler
	/// context.
	///
	/// If a board or chip must, for whichever reason, use a different global
	/// trap handler, it should abide to the above contract and emulate its
	/// behavior for all traps and interrupts that are required to be handled by
	/// the respective kernel or other trap handler as registered in mscratch.
	///
	/// For instance, a chip that does not support non-vectored trap handlers
	/// can register a vectored trap handler that routes each trap source to
	/// this global trap handler.
	///
	/// Alternatively, a board can be allowed to ignore certain traps or
	/// interrupts, some or all of the time, provided they are not vital to
	/// Tock's execution. These boards may choose to register an alternative
	/// handler for some or all trap sources. When this alternative handler is
	/// invoked, it may, for instance, choose to ignore a certain trap, access
	/// global state (subject to synchronization), etc. It must still abide to
	/// the contract as stated above.
	pub fn _start_trap();

arch: rv32i: fix comments #4059

arch: rv32i: fix comments #4059

Uh oh!

Conversation

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!