More RISC-V CHERI PTE bits #117

nwf · 2020-07-02T19:32:52Z

As per the ongoing Sail work at CTSRD-CHERI/sail-cheri-riscv#18.

arichardson

Looks good to me

target/riscv/cpu_helper.c

target/riscv/cpu_bits.h

target/riscv/cpu_helper.c

bsdjhb · 2020-07-07T16:54:03Z

target/riscv/cpu_helper.c

            return TRANSLATE_CHERI_FAIL;
 #endif
        } else {
            /* if necessary, set accessed and dirty bits. */
            target_ulong updated_pte = pte | PTE_A |
                (access_type == MMU_DATA_STORE ? PTE_D : 0);

+#if defined(TARGET_CHERI) && !defined(TARGET_RISCV32)
+            if (access_type == MMU_DATA_CAP_STORE) {
+                /* (SCM and not SC) transitions to (SC and not SCM) */


So I don't see a check for both bits being set. If that is reserved, we should be checking that above and returning some other fault instead? (What fault is defined for invalid settings of PTE bits?)

Similarly for invalid combinations of the load cap bits.

Invalid setting of PTE bits is normally the page fault corresponding to the instruction type. I don't know if that means we'd want a page fault or a capability page fault if the CHERI-specific bits were the issue.

Hmmm. x86 has a special code for reserved bits being set (but it also uses a single PF# vector). I suspect we want to raise the relevant CHERI fault which would happen here by just returning TRANSLATION_FAIL_CHERI.

I'm torn between raising a CHERI-specific fault or following along with the upstream Sail that maps reserved bit combinations to the PTW_Invalid_PTE internal result (and then on to the generic page fault vector). See

https://github.com/rems-project/sail-riscv/blob/3026c56e7c2ec5e33a4e9f7e54323b9f8ad6f3aa/model/riscv_pte.sail#L37

https://github.com/rems-project/sail-riscv/blob/3026c56e7c2ec5e33a4e9f7e54323b9f8ad6f3aa/model/riscv_vmem_sv48.sail#L29-31

https://github.com/rems-project/sail-riscv/blob/3026c56e7c2ec5e33a4e9f7e54323b9f8ad6f3aa/model/riscv_ptw.sail#L35

I added checks that raise the existing TRANSLATE_FAIL code; if anyone feels strongly that this should be TRANSLATE_CHERI_FAIL, I'll flip to that.

target/mips/cp0_helper.c

target/riscv/cpu_bits.h

target/riscv/cpu_helper.c

target/riscv/csr.c

target/riscv/monitor.c

nwf · 2020-12-03T13:38:22Z

Rebasing. Assuming Jenkins doesn't find any problems and nobody screams, I'll merge.

arichardson

Sorry about the conflict.

arichardson · 2020-12-03T13:48:20Z

target/riscv/csr.c

+    }
+
+    /* Take the GCLG bits from the store and update state bits */
+    env->mccsr = set_field(env->mccsr, gclgmask, get_field(val, gclgmask));


Do we need another bit somewhere determining whether S-mode is allowed to write?

No, they're like [ms]status.SIE. But they should really have S prefixes on all of them to make that clear.

arichardson · 2020-12-03T13:50:48Z

Test failure is my fault. Will fix shortly.

nwf · 2020-12-03T14:19:09Z

Renamed the fields as @jrtc27 suggested.

arichardson

I haven't been following the discussion regarding these bits closely. QEMU code looks good.
Will defer functional review to @jrtc27 / @bsdjhb

target/riscv/cpu_helper.c

target/riscv/csr.c

More obviously parallels the existing W/R bits.

target/riscv/csr.c

nwf · 2021-07-12T12:29:41Z

@jrtc27: Are there still changes you want me to make?

target/riscv/cpu_helper.c

jrtc27 · 2021-07-12T12:48:50Z

target/riscv/csr.c

@@ -1333,12 +1333,8 @@ static int write_pmpaddr(CPURISCVState *env, int csrno, target_ulong val)
 #endif

 #ifdef TARGET_CHERI
-
 // See Capability Control and Status Registers (CCSRs) in CHERI ISA spec


Hm, either this comment should be grouped with the one below (which should also not have a blank line after it) or it should move to where XCCSR_* are defined

I've moved it to the XCCSR_* definitions and pruned the second comment.

target/riscv/csr.c

While here, guard the use of all the upper metadata bits with `!defined(TARGET_RISCV32)`

Don't yet let the hypervisor play the load-side trap game, but it's fairly apparent how we might go about it.

This makes [ms]ccsr writable, but only the GCLG bits are updated. Co-Authored-By: Jessica Clarke <[email protected]>

When raising a MMU exception, we were zeroing out the CLG bits from CP0 EntryHi, which was, understandably, making software sad. Fixes 7c74ddc and f0feea1

These are `#if 0`-ed out, but has proven useful in testing, as our FPGA cores trap rather than atomically update the PTE.

@jrtc27

Thanks to @jrtc27 for pointing out its omission.

arichardson

@jrtc27 can we merge this?

nwf requested review from jrtc27, arichardson and bsdjhb July 2, 2020 19:32

arichardson reviewed Jul 3, 2020

View reviewed changes

target/riscv/cpu_helper.c Outdated Show resolved Hide resolved

target/riscv/cpu_bits.h Outdated Show resolved Hide resolved

target/riscv/cpu_helper.c Outdated Show resolved Hide resolved

nwf force-pushed the riscv-cheri-pte-more branch from 17aef50 to 354ea58 Compare July 3, 2020 21:59

bsdjhb reviewed Jul 7, 2020

View reviewed changes

nwf force-pushed the riscv-cheri-pte-more branch from 354ea58 to 10f7fe3 Compare July 7, 2020 23:36

arichardson approved these changes Jul 14, 2020

View reviewed changes

nwf force-pushed the riscv-cheri-pte-more branch from 10f7fe3 to c1c5474 Compare August 10, 2020 20:43

nwf force-pushed the riscv-cheri-pte-more branch from c1c5474 to 6587776 Compare October 20, 2020 13:15

nwf mentioned this pull request Oct 20, 2020

Per-page capability-dirty tracking, take N+1 CTSRD-CHERI/cheribsd#405

Closed