Skip to content
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

Clean up MTDC #21

Open
rmn30 opened this issue Dec 13, 2023 · 20 comments
Open

Clean up MTDC #21

rmn30 opened this issue Dec 13, 2023 · 20 comments

Comments

@rmn30
Copy link
Collaborator

rmn30 commented Dec 13, 2023

We inherited the MTDC (trap default capability) special capability register from upstream but it isn't needed as we don't support hybrid mode. It would make more sense to get rid of it and keep only MScratchC, MEPCC and MTCC. This would need a few changes in ISA and RTOS:

  1. MTDC currently contains the RW root on reset. We'd need to put it somewhere else, perhaps in the c1/x1 GPR?
  2. The RTOS would need to switch to using MScratchC as a scratch register instead of MTDC! This should be trivial.
@vmurali
Copy link
Contributor

vmurali commented Dec 20, 2023

  1. MTDC currently contains the RW root on reset. We'd need to put it somewhere else, perhaps in the c1/x1 GPR?

Since you have 3 registers, you can initialize the three of them with the 3 different roots. (And revert the following commit e0c5663).

@davidchisnall
Copy link
Collaborator

I think the switcher already uses m scratch.

@rmn30
Copy link
Collaborator Author

rmn30 commented Dec 21, 2023

Since you have 3 registers, you can initialize the three of them with the 3 different roots. (And revert the following commit e0c5663).

We could, but putting something non-executable in MEPCC or MTCC is a bit sketchy. I can't remember what the current semantics are for that but one option would be to clear the tag on such 'invalid' values and avoiding having to do the permit_execute check on instruction fetch. It probably doesn't matter than much but I think I'd rather avoid it unless there is a good reason. Anyone got any thoughts?

@rmn30
Copy link
Collaborator Author

rmn30 commented Dec 21, 2023

.

I think the switcher already uses m scratch.

Don't think so. Where?

@rmn30
Copy link
Collaborator Author

rmn30 commented Dec 21, 2023

If we do decide to use either MEPCC or MTCC for the RW root on reset I'd rather it was MEPCC since at least that gives you a chance to debug very early faults before MTCC has been configured (assuming you can put some code at 0).

@davidchisnall
Copy link
Collaborator

It’s convenient for the early loader to be able to keep all of the roots in SCRs so that they can be preserved across calls without needing the stack to be usable.

The other option, which might be cleaner, would be to add three new read-only SCRs for the roots, plus a write-only CSR that makes them inaccessible.

@davidchisnall
Copy link
Collaborator

.

I think the switcher already uses m scratch.

Don't think so. Where?

No, still MTDC. I must be thinking of an old version. Easy change to make though.

@vmurali
Copy link
Contributor

vmurali commented Dec 26, 2023

We could, but putting something non-executable in MEPCC or MTCC is a bit sketchy. I can't remember what the current semantics are for that but one option would be to clear the tag on such 'invalid' values and avoiding having to do the permit_execute check on instruction fetch.

The semantics of MTCC is to clear the tag bit on a bad write, and the semantics of MEPCC is to clear the tag bit on bad read (including MRet) - bad read/write is when you need to zero out one or two LSB bits and it becomes unrepresentable.

The idea is to run the early loader first which copies these into some specific memory. We already have MScratch holding the seal root, which has to be copied somewhere before execution starts.

Adding 3 new CSRs could be a solution because now we don't have to copy any root capabilities into memory, at the cost of a total 6 SCR slots out of 32 slots.

I agree that if restricted to putting arbitrary roots in arbitrary SCRs, putting the data root in MEPCC is better than putting that in MTCC.

@davidchisnall
Copy link
Collaborator

For a two-stage loader, it is convenient to keep them in SCRs while the first-stage loader runs. I’m not sure what the implications would be on software of not being able to do this (though possibly we still could, the loaders currently run with interrupts disabled and should reboot on traps).

We may not need to permanently consume SCR slots with three new ones. If we later introduce three that are not needed during early boot then the monotonic toggle can move them from holding the roots to their alternative use.

@vmurali
Copy link
Contributor

vmurali commented Jan 2, 2024

We may not need to permanently consume SCR slots with three new ones. If we later introduce three that are not needed during early boot then the monotonic toggle can move them from holding the roots to their alternative use.

Isn't that exactly the same as using the MTCC, MEPCC and MScratch for holding the 3 roots and later switching them for alternative use?

@kliuMsft
Copy link
Contributor

If possible, could we avoid putting the rw root in GPR? It might be just a matter of opinions but I am a little uneasy about a) making a special case in GPR that in some cases could make implementations more complex b) an reset (due to glitch attacks, etc) may suddenly place a root capability back into a GPR..

@davidchisnall
Copy link
Collaborator

b) an reset (due to glitch attacks, etc) may suddenly place a root capability back into a GPR

That's an interesting thought. If it's in an SCR, then you'd need to trick the switcher into misusing it (since it's the only thing that runs with ASR permission).

@rmn30 rmn30 mentioned this issue Oct 3, 2024
@rmn30
Copy link
Collaborator Author

rmn30 commented Oct 3, 2024

I think my preferred option for this is to rename MTDC to MScratch2. I don't want to put a non-executable root capability in MTCC or MEPCC. How does that sound?

@davidchisnall
Copy link
Collaborator

I think my preferred option for this is to rename MTDC to MScratch2.

I would prefer to remove it, since we will probably lose it in v2, but I see that it's hard.

I don't want to put a non-executable root capability in MTCC or MEPCC. How does that sound?

It's annoying to have an SCR that exists solely to hold a root at boot time, but I guess it's unavoidable.

@rmn30
Copy link
Collaborator Author

rmn30 commented Oct 3, 2024

Having an extra scratch register may allow some optimisation in the switcher? Doesn't seem too bad.

@davidchisnall
Copy link
Collaborator

In v2, we'll have to add it as an explicit delta to Zcheripurecap. I don't really like having a thing that we're explicitly adding and then not using for anything.

@nwf
Copy link
Member

nwf commented Oct 3, 2024

I don't want to put a non-executable root capability in MTCC or MEPCC.

Oh, that's an interesting aspect I'd not considered. Can you say more about why you want that invariant?

@nwf
Copy link
Member

nwf commented Oct 3, 2024

See also discussion at riscv/riscv-cheri#391

@rmn30
Copy link
Collaborator Author

rmn30 commented Oct 3, 2024

I don't want to put a non-executable root capability in MTCC or MEPCC.

Oh, that's an interesting aspect I'd not considered. Can you say more about why you want that invariant?

We now validate capabilities when they are written to those CSRs (checking that they are executable and not sealed) with violations resulting in clearing the tag. This somewhat simplifies exception handling. Having a non-executable capability in them on reset would violate that invariant.

@nwf
Copy link
Member

nwf commented Oct 3, 2024

If we adopt Zstid as part of v2, we would have another CSR we could initialize and that we'd actually use (for cgp), so we could initialize...

  • pcc with the X root (naturally enough)
  • mtidc with the RW root (maybe this one wants to require RW-root-derived caps like MTCC and MEPCC have required X-root-derived caps?)
  • mscratch with the sealing root (by elimination and because I don't think we impose constraints on this one?)
  • the other CSRs to NULL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Rejected
Development

No branches or pull requests

5 participants