-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic V_COMMON
exception handler
#22
base: master
Are you sure you want to change the base?
Conversation
I am unfortunately away from keyboard for like another week, so I probably won't be able to review this until then. If feasible, could you ping me in a week? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the ghost of provenance will haunt us for some time...
/// Represents a coredump of the CoP0 registers. | ||
#[derive(Debug)] | ||
pub struct CoP0Dump { | ||
/// [BadVAddr] value loaded from the BadVAddr register. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something of a philosophy question: how much do we assume someone has read the user manuals? these comments make sense if someone keeps them open, but otherwise they're very self-referential.
I think something like "virtual address of exception" here would be a helpful memory aid, as an example.
impl CoP0Dump { | ||
/// Dumps all register contents and constructs an instance of [CoP0Dump]. | ||
pub fn load() -> Self { | ||
let bad_v_addr = BadVAddr::load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just directly loading things in the return structure is more succinct; we don't gain any readability by using an intermediate variable.
@@ -98,8 +184,15 @@ impl EPC { | |||
pub fn store(self) { | |||
unsafe { _write_epc(self.0) } | |||
} | |||
|
|||
/// Convert to a raw pointer. Remember to check the `Cause.BD` register field to determine if an offset is needed. | |||
pub fn as_raw_ptr(&self) -> *const u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I look at this API, the more I'm convinced this is not sound as-is with respect to provenance.
The question is: can it ever be made sound? Playing by the "definitely safe" Strict Provenance rules, we need a pointer that has access to whatever this points to. There's absolutely no way for us to do that here. That means we would have to use Exposed Provenance, and need everything to expose its provenance for it to be sound for us to read through.
So I'm tempted to say that not only should this method be deleted, but the docs updated to tell people not to convert this into a pointer.
} | ||
|
||
#[no_mangle] | ||
pub(super) extern "C" fn unimplemented_v_common_handler(tcb_ptr: *mut ThreadControlBlock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> !
, perhaps? Rust can detect that the infinite loop is diverging, I think.
|
||
use crate::{cop0::CoP0Dump, thread::ThreadControlBlock}; | ||
|
||
/// Purposefully trigger an overflow exception. For testing purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(thankfully, these went out of style)
zero_bss(); | ||
|
||
println_ee!("rt - BSS zero-ed out."); | ||
|
||
initialise_exception_vectors(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be smarter to initialise the vectors before zeroing bss in case the elf is somehow corrupt and bss points to invalid address space.
|
||
use crate::cop0::CoP0Dump; | ||
|
||
/// Main entrpoint for the [panic_handler](https://doc.rust-lang.org/nomicon/panic-handler.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
@@ -1,6 +1,7 @@ | |||
# The assembler tries to reorder our instructions around delay slots. | |||
# Since we want to preserve this order, disable the reordering feature. | |||
.set noreorder | |||
.set noat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I smash $at
in my code, do I?
@@ -1,6 +1,7 @@ | |||
# The assembler tries to reorder our instructions around delay slots. | |||
# Since we want to preserve this order, disable the reordering feature. | |||
.set noreorder | |||
.set noat | |||
|
|||
# Bootstrap routine for PruSSia. LLVM only needs a stack pointer, and the rest we can do in Rust. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eeek. While you're here can you fix the capitalisation of Prussia? :p
/// jumping to an exception handler. | ||
#[repr(C)] | ||
pub(crate) struct ThreadControlBlock { | ||
pub at: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember, is this from my code? If it was, it's technically incorrect because the GPRs are actually 128-bit wide...
V_COMMON
exceptions.COP0
register values and halts.sleep_thread
syscall.r0
crate.Partially addresses #20.