Re: [PATCH v9 03/17] target/riscv: save and restore elp state on priv transitions

[Deepak Gupta <debug@rivosinc.com>]

On Tue, Aug 27, 2024 at 10:33:04AM +1000, Richard Henderson wrote:
>On 8/27/24 01:29, Deepak Gupta wrote:
>>diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>index 8e1f05e5b1..083d405516 100644
>>--- a/target/riscv/cpu.c
>>+++ b/target/riscv/cpu.c
>>@@ -1022,6 +1022,11 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>>      env->load_res = -1;
>>      set_default_nan_mode(1, &env->fp_status);
>>+#ifdef CONFIG_USER_ONLY
>>+    /* qemu-user for riscv, fcfi is off by default */
>>+    env->ufcfien = false;
>>+#endif
>...
>>@@ -226,6 +226,7 @@ struct CPUArchState {
>>      bool      elp;
>>  #ifdef CONFIG_USER_ONLY
>>      uint32_t elf_flags;
>>+    bool ufcfien;
>>  #endif
>
>Thinking about this more, I think adding separate controls for 
>user-only is a bad precedent to set.  You said you are adding these 
>because senvcfg/menvcfg are ifdefed: well, that should be the thing 
>that we fix.

If a binary is compiled with zicbo, it'll have those instructions in user
binary. I am not sure how those binaries will run on qemu-user (or if it was
tested with qemu-user)

In case of zicfilp + zicfiss we are runnning binaries compiled with zicfilp
and zicfiss with qemu-user and qemu-system both. Use of qemu-user to generate
traces for feeding into CPU modeling is quite useful. Thus mechanism to track
if landing pad and shadow stack is enabled for current user task (in case of
qemu-user) is very useful.

senvcfg and menvcfg belong to S and M state and don't actually mean anything
for qemu-user. However if that's how it is for arm as well (i.e. exposing system
state for qemu-user), then probably there is precedent.
But it looks like a much larger exercise to me.

>
>The only real user of *envcfg that I see so far is check_zicbo_envcfg, 
>which does not use the same switch statement as this:
>
>>+    switch (env->priv) {
>>+    case PRV_U:
>>+        if (riscv_has_ext(env, RVS)) {
>>+            return env->senvcfg & MENVCFG_LPE;
>>+        }
>>+        return env->menvcfg & MENVCFG_LPE;
>>+    case PRV_S:
>>+        if (env->virt_enabled) {
>>+            return env->henvcfg & HENVCFG_LPE;
>>+        }
>>+        return env->menvcfg & MENVCFG_LPE;
>>+    case PRV_M:
>>+        return env->mseccfg & MSECCFG_MLPE;
>>+    default:
>>+        g_assert_not_reached();
>>+    }
>
>I think your function should look more like check_zicbo_envcfg: (1) 
>PRV_U may be either U or VU, and different tests apply; (2) M-mode 
>disable means that no lower level may be enabled.

In case of landing pad, (2) doesn't hold true. Each mode can independently
enable landing pad for next lower mode even if it wasn't enabled for current
mode. Reason being that you can have a firmware which is still not landing pad
enabled and you would want to avoid landing pad related faults in M mode but
still want to make sure S mode and U mode can enable landing pad for themselves.
Same goes with respect to S mode, S mode can have its landing pad disabled while
it can enable landing pad for U mode.

Although logic for shadow stack enable is same as zicbo. Shadow stack enable requires
target software to opt-in by having instructions compiled in and doesn't impact 
behavior of existing instructions.

>
>It would be nice if you could generalize check_zicbo_envcfg to apply 
>to your new use as well.  Perhaps some tri-state function to say 
>"enabled", "disabled", "virtual disabled".

So while shadow stack and zicbo are similar in terms of enabling. Landing pad
enable differs. 

You still want me to generalize *envcfg flow?
>
>
>r~