Extend access type to cache accesses

[KotorinMinami]

Added an implementation for #787, but I was wondering how to implement this TODO in the comment

// TODO: This is incorrect since CHERI only requires at least one byte
// to be in bounds here, whereas ext_data_get_addr() checks that all bytes
// are in bounds. We will need to add a new function, parameter or access type.

[github-actions]

Test Results

2 101 tests  ±0   2 101 ✅ ±0   15m 55s ⏱️ +3s
    1 suites ±0       0 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 3851ce1. ± Comparison against base commit 296f8cd.

♻️ This comment has been updated with latest results.

[Timmmm]

I'm getting pretty close to pushing our latest code (just trying to get it to build) so give me a couple of days and then I can point you at it.

[Timmmm]

Here's how we are doing it for CBOs:

https://github.com/CHERI-Alliance/sail-riscv/blob/de9279755a2fbce96314083bff884f21e6937992/model/riscv_insts_zicboz.sail#L34

https://github.com/CHERI-Alliance/sail-riscv/blob/de9279755a2fbce96314083bff884f21e6937992/model/riscv_insts_zicbom.sail#L48-L53

Basically you just have to make sure to pass in the correct access type, and for cbom it is based on the original access type rather than the modified one.

@tomaird that bit isn't super clear in the CHERI spec. Do you know where it came from?

[tomaird]

Not sure I understand which bit isn't clear so I'll just explain the full rationale behind all this code...

The CHERI spec describes the following checks for the CBO instructions (amongst others):

CBO   | Bounds    | Permissions
--------------------------------
CLEAN | Any byte  | W+R
FLUSH | Any byte  | W+R
INVAL | Any byte  | W+R+ASR
ZERO  | All bytes | W

So for the bounds check, CLEAN/FLUSH/INVAL all behave the same - it only needs a single byte to be in bounds.

But for the permissions checks it's a bit different, with CBO.INVAL requiring more permissions than the others.

The RISC-V spec describes that CBO.INVAL can also do a flush depending on xenvcfg.cbie, which in the model we treat as executing a CBO.FLUSH. But from the table above you can see that CBO.FLUSH has a looser permissions requirement than CBO.INVAL, so we need to make sure we do the permissions check based on what instruction actually executed, i.e. the CBO.INVAL, rather than what we're effectively executing in the model. Hence the need to pass in the "original" access type.

Does that answer your question?

[Timmmm]

Yeah I think my question is do you definitely still need ASR permission if INVAL executes as FLUSH. My understanding was that INVAL needs the extra permission because it's "unsafe" - you can resurrect capabilities that should have died. But if it's actually doing a FLUSH then it's safe.

Although now that I check the spec again it does actually explicitly mention this:

The CBIE bit in menvcfg and senvcfg indicates whether CBO.INVAL performs cache block flushes instead of invalidations for less privileged modes. The instruction checks shown in the table below remain unchanged regardless of the value of CBIE and the privilege mode.

[jrtc27]

Not sure I understand which bit isn't clear so I'll just explain the full rationale behind all this code...

The CHERI spec describes the following checks for the CBO instructions (amongst others):

CBO   | Bounds    | Permissions
--------------------------------
CLEAN | Any byte  | W+R
FLUSH | Any byte  | W+R
INVAL | Any byte  | W+R+ASR

This should be "All bytes":

ifdef::cbo_inval[]
| Permission violation | It does not grant <<w_perm>>, <<r_perm>> or <<asr_perm>>
| Length violation | At least one byte accessed is outside the bounds
endif::[]

[tomaird]

@jrtc27 where are you quoting that bit of spec from? On latest main of the CHERI spec has cbo.inval only requiring any byte to be in bounds:

| Bounds violation      | None of the bytes accessed are within the bounds, or the capability has <<section_cap_malformed,malformed>> bounds

https://github.com/riscv/riscv-cheri/blob/main/src/cheri/insns/cbo_exceptions.adoc?plain=1#L23

[jrtc27]

@jrtc27 where are you quoting that bit of spec from? On latest main of the CHERI spec has cbo.inval only requiring any byte to be in bounds:

| Bounds violation      | None of the bytes accessed are within the bounds, or the capability has <<section_cap_malformed,malformed>> bounds

https://github.com/riscv/riscv-cheri/blob/main/src/cheri/insns/cbo_exceptions.adoc?plain=1#L23

I guess I still had an old version open somehow. But this is very wrong. See riscv/riscv-cheri#221 (comment) :(

[Timmmm]

It would be good to get this in - do you mind if I just update it to remove ExecutableDataFetch? Or do you think we should keep it even though it isn't used yet?

[KotorinMinami]

Okay, I'll remove it.

[Timmmm]

Hey, I rebased this and changed it to use the AMO PMA level in the AMO access type instead of the full AMO op since a) that's all we really need to know, and b) we don't have access to that type in riscv_types.sail any more. I maybe could have moved the types around to fix that but this seemed better.

Also I think we probably want to go the full way and remove all the aq/rel/res/con bools before merging this, otherwise it's just weird having both things. I'll have a go at that at some point.

[KotorinMinami]

Thanks for the rebase @Timmmm . I'm also considering how to proceed. Perhaps we should first open an issue to document the removal of the aq/rel/res/con boolean parameters.

[Timmmm]

I don't think we need to document anything, we just need to remove those parameters and instead source them from the access type. I did start working on this, it doesn't look too bad. The only minor issue I ran into is that a couple of functions have the bool flags but not the access type, but we can just add that.

[jordancarlin]

@KotorinMinami @Timmmm This would be great to get in soon as it is blocking #328 (which was recently ratified and would be good to get merged). Are either of you able to rebase and finish this (after the holidays)?

[Timmmm]

Yeah maybe we could take a look at each of our approaches in the next meeting and see which one is best (I haven't actually looked yet).

[pmundkur]

Notes from comparing this to #1405. Sorry, I should have checked this before.

[pmundkur]

AMO width handling is not handled in #1405. In that PR, pmaCheck gets a more informative access, but doesn't do anything with it. I think that's best left to a subsequent PR, which would also handle the width of cache block accesses.

[pmundkur]

It's not clear whether to use StoreConditional(aq, rl,..) here or StoreConditional(aq & rl, rl, ...) as per the original code. This is why I did not fold in these flags in #1405. It's probably better to have litmus tests running first to ensure we don't break things in the refactor.

[pmundkur]

We've since added prefetch from Zicbop as well.

[pmundkur]

This needs to be updated: "During address translation, the [cache-block management] instruction also checks the accessed bit and may either raise an exception or set the bit as required." Similarly for cache-block zero.

[pmundkur]

This is incorrect: "During address translation, the [cache-block zero] instruction also checks the accessed and dirty bits and may either raise an exception or set the bits as required."

[pmundkur]

I left this as "RW" in #1405. Is "A" preferable?

[jordancarlin]

Until we have other trace/logging formats, I would avoid changing what is printed in the log. It is currently the only way of parsing the output of the model, so switching from RW to A would likely break many tools. Once we have new machine-readable formats available, we can intentionally break the human-readable log, and then I think switching to A/LR/SC is worth considering.

[pmundkur]

I left these as "R" and "W", perhaps "LR" and "SC" are more informative?

[pmundkur]

Should we close this, now that #1405 is merged?