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

Revocation 3.0 PTE bits #18

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Revocation 3.0 PTE bits #18

wants to merge 5 commits into from

Conversation

nwf
Copy link
Member

@nwf nwf commented May 15, 2020

First stab at adjusting PTW for capability load generation bits.

src/cheri_pte.sail Outdated Show resolved Hide resolved
@nwf
Copy link
Member Author

nwf commented May 15, 2020

For FETT, I think we want to just have SC and LC, taking the other two (LC_Mod and LC_CLG) to be constant 0s.

Speaking of those bits, I'm not elated with the current names... I thought about LC_CLG and LC_LCLG but those aren't great either. Open to suggestions.

Copy link
Member

@pmundkur pmundkur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally lgtm, though i haven't checked against the cheri spec.

@nwf
Copy link
Member Author

nwf commented May 15, 2020

I have added a little more commentary and shuffled the different behaviours so that LC_Mod is more obviously a modification to the LC behaviour and LC_LCLG is now just the LCLG until we have other need for it.

At the moment the code throws if an invalid state is found, but perhaps there's a more correct in-model thing to do. What happens if the RWX bits are set to W or WX, for example?

@nwf
Copy link
Member Author

nwf commented May 27, 2020

This batch of commits moves us to LoadCapInhibit and StoreCapInhibit bits and shuffles things a bit more so that, again, all-zeros should be what we want. The Load side is more fully fleshed out than the Store side, which, right now, defines but does not use a "fast" capability-dirtying path.

@nwf nwf requested a review from pmundkur May 27, 2020 21:03
nwf referenced this pull request in CTSRD-CHERI/Flute Jun 3, 2020
@nwf nwf changed the title Introduce capability load generation traps Revocation 3.0 PTE bits Jun 5, 2020
@nwf
Copy link
Member Author

nwf commented Jun 5, 2020

This is atop #19 and #20 and should, I think, get both the load and store sides.

@nwf
Copy link
Member Author

nwf commented Jun 17, 2020

Orthogonality of commits is hard; where'd those pullbacks of patches go, anyway? This PR now also includes fixes to cheri_insts to pass virtual, not physical, address to handle_mem_exception as well.

@nwf nwf force-pushed the clg branch 2 times, most recently from dc50c4d to 76b1b91 Compare June 19, 2020 20:08
src/cheri_pte.sail Outdated Show resolved Hide resolved
@@ -94,7 +93,7 @@ function CapExCode(ex) : CapEx -> bits(5) =
CapEx_ReturnTrap => 0b00110,
CapEx_TSSUnderFlow => 0b00111,
CapEx_UserDefViolation => 0b01000,
CapEx_TLBNoStoreCap => 0b01001,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point we will want to annotate this CHERI exception code as MIPS-only in the ISA doc.

src/cheri_sys_regs.sail Outdated Show resolved Hide resolved
src/cheri_sys_regs.sail Outdated Show resolved Hide resolved
@nwf
Copy link
Member Author

nwf commented Apr 27, 2021

Rebased. These continue to pass https://gist.github.com/nwf/cd4d760247ee54e3c9e136ba53d9fdbf ("pte-bits.S")

@nwf
Copy link
Member Author

nwf commented Jun 17, 2021

As per CTSRD-CHERI/qemu#150 (comment), @jrtc27 makes an argument that the behaviour of CLoadTags should rather be that it doesn't trap but instead clears tags if the PTW says PTW_LC_CLEAR. Data dependence for the PTW_LC_TRAP case seems perhaps a bridge too far, but could be warranted as well.

@nwf
Copy link
Member Author

nwf commented Oct 26, 2021

Renamed CR_Mod to CR_Trap as per @jrtc27's suggestion.

Copy link
Contributor

@PeterRugg PeterRugg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (I must admit I'd thought this was merged already because I remember testing against it!)

src/cheri_insts.sail Show resolved Hide resolved
src/cheri_addr_checks.sail Outdated Show resolved Hide resolved
src/cheri_pte.sail Outdated Show resolved Hide resolved
src/cheri_pte.sail Outdated Show resolved Hide resolved
src/cheri_pte.sail Outdated Show resolved Hide resolved
src/cheri_pte.sail Outdated Show resolved Hide resolved
d : 1, /* dirty */
e : 0 /* enable */
}

register mccsr : ccsr
register sccsr : ccsr
register sccsr : ccsr /* mccsr aliases sccsr */
register uccsr : ccsr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit weird to still have uccsr... I kinda get why, but that's primarily because uccsr currently serves no purpose. Also, if mccsr and sccsr alias then normally you'd have the definition be mccsr and sccsr be an alias for (or view into) it, not the other way round.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it "serves no purpose" because the d and e bits are not meaningfully implemented in sail, but they're still defined...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though maybe that does argue against making sccsr and mccsr alias.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly happy with what I have just written in cheri_pte.sail -- "use sccsr if haveSupMode() and mccsr otherwise" -- but it might be viable. I'm quite open to suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrtc27 Opinions?

nwf-msr and others added 5 commits August 19, 2022 23:47
Pull up to
riscv/sail-riscv@2265a25

Some upstream changes apply to files in this repository as well:

- Following along with
riscv/sail-riscv@b1db5b9
add ext_check_phys_mem_read and ext_check_phys_mem_write implementations.

- Following along with
riscv/sail-riscv@9b38e33
always use riscv_flen_D.sail rather than riscv_flen_F.sail on RV32.

- Following along with
riscv/sail-riscv@ffea7a3
use -fcommon in C_FLAGS.
The cap-store instructions now look at the data being stored to decide
whether they issue Write(Cap) or Write(Data) requests on "the bus".
This allows the PTW logic (and update_PTE_Bits) in particular to not
fast-cap-dirty a page that's being targeted by the store of an untagged
capability.  This is not, however, viable for AMOCAS and so additional
changes may be required if we are to avoid considering AMOCAS as always
capability-dirtying.

Co-authored-by: Jessica Clarke <jrtc27@jrtc27.com>
Use Either-monadic style

Eliminate ext_ptw_sc / PTW_SC_* as we now simply test the error cases in
priority order, so there's no need to explicitly pass this information forward
in checkPTEPermission.  ext_ptw_lc / PTW_LC_* persist as the extended PTW
information is analysed by instructions.

Co-authored-by: Jessica Clarke <jrtc27@jrtc27.com>
Co-authored-by: Jessica Clarke <jrtc27@jrtc27.com>
* used if the machine has Supervisor mode, otherwise mccsr.
*
* Sv32: There are no extension bits available, so we hard-code the result to
* CW=1 CR=1 CD=1 CRT=0 CRG=0
*/
type extPte = bits(10)

bitfield Ext_PTE_Bits : extPte = {
CapWrite : 9, /* Permit capability stores */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre-existing, but bits 9 and 8 conflict with “Svpbmt” Standard Extension for Page-Based Memory Types. I wonder if we should shift everything down by two as part of this change series or start from zero (since upstream started from the top)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. I guess that's not the worst possible flag day, it'd just touch sail, the cores, and the CheriBSD kernel.

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

Successfully merging this pull request may close these issues.

7 participants