Re: [PATCH v3 15/20] target/riscv: shadow stack mmu index for shadow stack instructions

[Deepak Gupta <debug@rivosinc.com>]

On Wed, Aug 07, 2024 at 12:43:31PM +1000, Richard Henderson wrote:
>On 8/7/24 10:06, Deepak Gupta wrote:
>>Shadow stack instructions shadow stack mmu index for load/stores.
>>`MMU_IDX_SS_ACCESS` at bit positon 3 is used as shadow stack index.
>>Shadow stack mmu index depend on privilege and SUM bit. If shadow stack
>>accesses happening in user mode, shadow stack mmu index = 0b1000. If
>>shaodw stack access happening in supervisor mode mmu index = 0b1001. If
>>shadow stack access happening in supervisor mode with SUM=1 then mmu
>>index = 0b1010
>>
>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>---
>>  target/riscv/cpu.h                            | 13 ++++++++++
>>  target/riscv/cpu_helper.c                     |  3 +++
>>  target/riscv/insn_trans/trans_rva.c.inc       |  8 ++++++
>>  target/riscv/insn_trans/trans_rvzicfiss.c.inc |  6 +++++
>>  target/riscv/internals.h                      |  1 +
>>  target/riscv/translate.c                      | 25 +++++++++++++++++++
>>  6 files changed, 56 insertions(+)
>>
>>diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>index 6da94c417c..3ad220a9fe 100644
>>--- a/target/riscv/cpu.h
>>+++ b/target/riscv/cpu.h
>>@@ -615,6 +615,19 @@ FIELD(TB_FLAGS, FCFI_ENABLED, 28, 1)
>>  FIELD(TB_FLAGS, FCFI_LP_EXPECTED, 29, 1)
>>  /* zicfiss needs a TB flag so that correct TB is located based on tb flags */
>>  FIELD(TB_FLAGS, BCFI_ENABLED, 30, 1)
>>+/*
>>+ * zicfiss shadow stack is special memory on which regular stores aren't
>>+ * allowed but shadow stack stores are allowed. Shadow stack stores can
>>+ * happen as `sspush` or `ssamoswap` instructions. `sspush` implicitly
>>+ * takes shadow stack address from CSR_SSP. But `ssamoswap` takes address
>>+ * from encoded input register and it will be used by supervisor software
>>+ * to access (read/write) user shadow stack for setting up rt_frame during
>>+ * signal delivery. Supervisor software will do so by setting SUM=1. Thus
>>+ * a TB flag is needed if SUM was 1 during TB generation to correctly
>>+ * reflect memory permissions to access shadow stack user memory from
>>+ * supervisor mode.
>>+ */
>>+FIELD(TB_FLAGS, SUM, 31, 1)
>
>This is already encoded into the mmu_idx as MMUIdx_S_SUM.

This is where I need some help / suggestion and clarifications.

`riscv_env_mmu_index` is the which does mode --> mmu index translation and that's
where `MMUIdx_S_SUM` is set.

Although above function assumes following things
    -- Only loads ands stores are supposed to do read and write.
    -- Translates env/priv --> mmu index

In case of shadow stack, we need to hold following true:
Shadow stack are not writeable via regular stores but are allowed to be readable.
Shadow stack are writeable only via shadow stack instruction.
Shadow stack instructions can't operate on non-shadow stack memory.

This let me to create a new mmu index (as you saw in patches). This mmu index is only
setup by shadow stack instruction and thus has to be known at translation time
(and that's why SUM TB flag)

There is no way of telling in `riscv_env_mmu_index` about whether mmu index is requested
for regular load/store or some other instruction (in this case shadow stack instruction).
If that is available then I think I can use `riscv_env_mmu_index`.

Question:
I see that `riscv_env_mmu_index` could be called from a bunch of places in (like
`accel/tcg/ldst_common.c.inc` as well. Does it exclude loads, stores which calculate mmu
indexes during translation (like shadow stack load, stores) ?

>
>
>r~