Traps support #29

fesqvw · 2024-03-11T12:17:21Z

No description provided.

CharlyCst

I wrote a few comments, I think the main problem so far is that you are mixing physical and virtual CSRs, in particular you are saving physical CSRs into the virtual CSRs when trying to save the context, but you don't need to do that because we already virtualize all writes to virtual registers, so they contain the last value written. For the CSR who are updated by the hardware, you do need to write some emulation code yourself though, and you already started doing that so that part looks all good :)

CharlyCst · 2024-03-11T12:27:08Z

src/main.rs

+        cause @ _ => {
+            // Save csr registers
+            ctx.csr.mcause = Arch::read_mcause_raw();
+            ctx.csr.mepc = Arch::read_mepc();


You don't need to save mepc, it is already saved in ctx.pc when the payload exits. What you probably want to do here is to save the PC at which the trap occured into mepc, so basically ctx.csr.mepc = ctx.pc.

Think about what would happen here if a trap occurred in Mirage itself since the trap from the payload, for instance a timer tick once we activate them. Then the physical mepc would point somewhere in Mirage, and you would configure the payload to execute some Mirage code!

CharlyCst · 2024-03-11T12:28:12Z

src/arch/metal.rs

+        )
+    }
+
+    unsafe fn write_mepc(mepc: usize) {


You won't need to write mepc manually, it is written when entering the firmware (the assembly trampoline loads the value from ctx.pc into mepc.

CharlyCst · 2024-03-11T12:38:35Z

src/main.rs

+            ctx.csr.mepc = Arch::read_mepc();
+            ctx.csr.mtval = Arch::read_mtval();
+            ctx.csr.mstatus = Arch::read_mstatus();
+            ctx.csr.mtinst = Arch::read_mtinst();


Be careful for those three registers (mtval, mstatus, and mtinst), you are saving Mirage's registers into the payload context, that is probably not what you intended to do ^^

When you do Arch::read_xxx you are reading physical registers, but the payload operates on virtual registers. We already saved the virtual value of mtval when the payload configured it for instance, so there is nothing more to do for mtval.
It is true that you might need to update some of the virtual registers, like mstatusI suppose, but you will only need to update a few bits I assume. To perform a proper emulation you should try to figure out which bits should be updated (Neelu might be able to help :) ) and perform those updates manually. It's possible some updates will require reading stuff from the physical registers, but only some bits.

CharlyCst · 2024-03-11T12:44:36Z

src/main.rs

@@ -95,7 +95,16 @@ fn handle_trap(ctx: &mut VirtContext, max_exit: Option<usize>) {
            // Skip to next instruction
            ctx.pc += 4;
        }
-        _ => (), // Continue
+        cause @ _ => {


Here you will catch all mcause, and a lot of them we don't know how to handle right now.
For this kind of projects we need to have a philosophy of correctness over completeness, which means that should strive to have a correct implementation at all time, event if it is not complete. So we should narrow down the scope, for instance let's handle only breakpoint traps, and be sure that we do so correctly.

Ed will tell you the story at some point, but the original VMWare hypervisor was definitely not complete, there were multiple hundreds of TODOs throughout the code, event at like version 5. But, everything that was implemented was correct, and that's why it worked so well.

The real problem here is that bugs will become really hard to debug if we start allowing incorrect emulation, and we might loose track of what is implemented and what is not. Bottom line is that we should panic on everything we did not intentionally handle properly :)

In that specific case, match a single trap and we will focus on making that one work before allowing more.

src/main.rs

CharlyCst

Thanks for the good work!
Now that you spent some time exploring and that I had time to think a bit about it, I think we should think about doing a bit of refactoring to better handle those traps. In particular we should probably save the info about the exit just after the exit happens, and then using those we can properly emulate the trapping behavior on virtual registers.

We can discuss that a bit if you want :)

CharlyCst · 2024-03-15T09:37:15Z

src/virt.rs

+            Csr::Dscratch0 => todo!(),
+            Csr::Dscratch1 => todo!(),
+            Csr::Mepc => {
+                if value > usize::MAX {


usize::MAX doesn't return the maximum valid address, just the maximum number we can fit in a usize. If you need to check the maximum supported address, we should add it in the Platform, because that will depend on the specific board.

CharlyCst · 2024-03-15T09:47:15Z

src/main.rs

        _ => todo!("Instruction not yet implemented: {:?}", instr),
    }
 }

+fn payload_trap_handler(ctx: &mut VirtContext) {


I propose that we pass the MCause as argument here, that way we won't have to read it from the hardware registers (because the hardware registers might contain an other fault at that point).

After thinking about it, I am thinking that we should have more info that just the. MCause, we would also need the mtval and mstatus at the time of exit.
So maybe we should create a new type like TrapInfo that contains all the values we need, and return it from the enter_firmware function. That way you don't need to read hardware registers anymore.

Yes, that would be a solution to the problem. Once we have the values, emulation should be much easier.

I'll take a look on how to do that as soon as I can.

CharlyCst · 2024-03-15T10:26:53Z

src/main.rs

+
+    // TODO : this should be done in the context switch assembly : it's ok for now because those registers are not modified by any code before
+    //mcause
+    ctx.csr.mcause = Arch::read_mcause_raw();


Here the fault we should inject might not be the cause in the physical register.
If we take the cause as argument we could use that here.

CharlyCst · 2024-03-15T10:27:35Z

src/main.rs

+    //mcause
+    ctx.csr.mcause = Arch::read_mcause_raw();
+    //mstatus
+    ctx.csr.mstatus = Arch::read_mstatus();


What do you want to update from mstatus? The mstatus of Mirage will probably contain different values than what we expect in the payload's mstatus.

CharlyCst · 2024-03-15T10:28:43Z

src/main.rs

+    //mtval
+    ctx.csr.mtval = Arch::read_mtval();
+    //mip
+    ctx.csr.mip = Arch::read_mip();


We will need to think about how we emulate interrupts. For now maybe let's assume we have no interrupts at all (so no interrupt pending) and write all 0.

CharlyCst · 2024-03-15T10:45:44Z

src/main.rs

+    //mstatus
+    ctx.csr.mstatus = Arch::read_mstatus();
+    //mtval
+    ctx.csr.mtval = Arch::read_mtval();


The mtval depends on the exception, right? We will probably also need to configure that manually by matching on the trap cause.

Yes, but like mcause and other csrs it is written by the HW (at least Neelu said so).
It should go into TrapInfo given by the enter_firmware function

payloads/breakpoint/src/main.rs

This field is not used for now, but can be useful to diferentiate between contexts in the future

CharlyCst · 2024-03-21T07:57:40Z

src/arch/metal.rs

@@ -332,8 +376,18 @@ _raw_trap_handler:
    sd x30,(8+8*30)(x31)
    csrr x30, mscratch    // Restore x31 into x30 from mscratch
    sd x30,(8+8*31)(x31)  // Save x31 (whose value is stored in x30)
-    csrr x30, mepc        // Read payload PC
-    sd x30, (8+8*32)(x31) // Save the PC


You removed the assembly to save the PC, but we still restore the context from the PC when jumping back into the firmware. Isn't that a bug, or maybe I am missing something?

The pc is set later on when we are sure the trap is coming from the payload and not mirage. If the trap was from mirage we would not want to put that mepc into the virtual context.

CharlyCst · 2024-03-21T08:33:43Z

src/virt.rs

+
+impl TrapInfo {
+    /// Whether the trap comes from M mode
+    pub fn from_mmode(self) -> bool {


Here you want to use &self instead of self.

If you use self the struct TrapInfo is passed by value to the function, that means that either the struct must be moved, or it must be copied (and you need the Copy trait indeed).

If you use &self the value is passed by reference to the method, so from_mmode will simply receive a pointer instead of a copy of the whole struct. This is what we want here, we simply inspect a few fields.

In general, you can use self for small sturcts, for instance when the structs are simply one or two integers, on in the special case when you want to move the struct (that can be useful in some case, such as with the builder pattern)
For all other cases, use either &self or &mut self depending if you need to modify the struct or not.

Same comment for the other method :)

CharlyCst · 2024-03-22T22:03:03Z

Rebased and merged as part of #32.

fesqvw added 3 commits March 10, 2024 21:32

Started work on traps

fbe4627

[WIP] save trap info and return from traps

153a860

formatting

f259914

CharlyCst reviewed Mar 11, 2024

View reviewed changes

fesqvw added 5 commits March 11, 2024 14:46

Implemented feedback

aef58f6

Fixed typo in BreakPoint

a5d85e6

added TODOs

04f1e7d

typos and mpp values

838ef01

Merge branch 'main' into traps

23bea8a

CharlyCst reviewed Mar 12, 2024

View reviewed changes

src/main.rs Show resolved Hide resolved

fesqvw added 13 commits March 12, 2024 12:29

quick mret test

7fb9352

Added unimplemented M-mode csrs

e8a40f1

virtualizing uninplemented registers

fc87f7c

Mret only to m mode

0f58778

Jumping to payload trap handler on unkown csr

b44da8c

Added remaining csr registers

fc08b40

formatting

f70fa39

Reading mcause, mstatus and mtval after trapping

f372ac9

Started trapping on illegal isntructions

1d1e043

Mret checks virtual registers correctly

6f5eb50

formatting

d3c4e84

Mepc WARL behaviour

faeb7bf

updated comment on csr values

549cb88

CharlyCst reviewed Mar 15, 2024

View reviewed changes

fesqvw added 6 commits March 15, 2024 19:39

Put the TODO in the trap test again

d64efa6

[WIP] TrapInfo and trap separation

7ee06ad

formatting

b0102ab

[WIP] setting up the trap handler structure

b4da608

Added id field to the virtual context

1d00fdc

This field is not used for now, but can be useful to diferentiate between contexts in the future

Removed virtual context id field

0a2b4b0

fesqvw added 18 commits March 17, 2024 15:00

Propagate trap information

641683f

formatting

edc4742

Change name of function for clarity

06dc186

Logging added for later

69a17f8

Added TODOs for the remaining specification csrs

7848c84

More TODOs for Csrs

5bd4688

formatting

a1681ce

commented scrap implementation of mepc

5c68fcb

Split trap handling between payload and mirage

6e2a472

Information structure to use in context switch

9216556

formatting

eb87a30

TrapInfo implemented

10c99f0

Added some registers for trapping

ec3d025

Testing and preparing mstatus emulation

b53c58f

mstatus emulation

b98be95

formatting

c256257

Trace message

c23e172

Merge branch 'main' into traps

6aac995

CharlyCst reviewed Mar 21, 2024

View reviewed changes

fesqvw added 2 commits March 21, 2024 12:43

Cleanup of panic message

90f46a8

TrapInfo functions use references

fa35da3

CharlyCst mentioned this pull request Mar 21, 2024

Merging trap handling #32

Merged

CharlyCst closed this Mar 22, 2024

CharlyCst deleted the traps branch April 1, 2024 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Traps support #29

Traps support #29

fesqvw commented Mar 11, 2024

CharlyCst left a comment

CharlyCst Mar 11, 2024

CharlyCst Mar 11, 2024

CharlyCst Mar 11, 2024

CharlyCst Mar 11, 2024

CharlyCst left a comment

CharlyCst Mar 15, 2024

CharlyCst Mar 15, 2024

CharlyCst Mar 15, 2024

fesqvw Mar 15, 2024

CharlyCst Mar 15, 2024

CharlyCst Mar 15, 2024

CharlyCst Mar 15, 2024

CharlyCst Mar 15, 2024

fesqvw Mar 15, 2024

CharlyCst Mar 21, 2024 •

edited

Loading

fesqvw Mar 21, 2024

CharlyCst Mar 21, 2024

CharlyCst commented Mar 22, 2024

Traps support #29

Traps support #29

Conversation

fesqvw commented Mar 11, 2024

CharlyCst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlyCst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlyCst Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlyCst commented Mar 22, 2024

CharlyCst Mar 21, 2024 •

edited

Loading