On 18/10/2024 04:25, Alistair Francis wrote:
> On Thu, Oct 17, 2024 at 5:45 PM Clément Léger <cle...@rivosinc.com> wrote:
>>
>>
>>
>> On 17/10/2024 06:29, Alistair Francis wrote:
>>> On Mon, Oct 14, 2024 at 5:43 PM Clément Léger <cle...@rivosinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/10/2024 05:22, Alistair Francis wrote:
>>>>> On Wed, Sep 25, 2024 at 9:59 PM Clément Léger <cle...@rivosinc.com> wrote:
>>>>>>
>>>>>> When the Ssdbltrp ISA extension is enabled, if a trap happens in S-mode
>>>>>> while SSTATUS.SDT isn't cleared, generate a double trap exception to
>>>>>> M-mode.
>>>>>>
>>>>>> Signed-off-by: Clément Léger <cle...@rivosinc.com>
>>>>>> ---
>>>>>> target/riscv/cpu.c | 2 +-
>>>>>> target/riscv/cpu_bits.h | 1 +
>>>>>> target/riscv/cpu_helper.c | 47 ++++++++++++++++++++++++++++++++++-----
>>>>>> 3 files changed, 43 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>>>> index cf06cd741a..65347ccd5a 100644
>>>>>> --- a/target/riscv/cpu.c
>>>>>> +++ b/target/riscv/cpu.c
>>>>>> @@ -284,7 +284,7 @@ static const char * const riscv_excp_names[] = {
>>>>>> "load_page_fault",
>>>>>> "reserved",
>>>>>> "store_page_fault",
>>>>>> - "reserved",
>>>>>> + "double_trap",
>>>>>> "reserved",
>>>>>> "reserved",
>>>>>> "reserved",
>>>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>>>> index 3a5588d4df..5557a86348 100644
>>>>>> --- a/target/riscv/cpu_bits.h
>>>>>> +++ b/target/riscv/cpu_bits.h
>>>>>> @@ -699,6 +699,7 @@ typedef enum RISCVException {
>>>>>> RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
>>>>>> RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
>>>>>> RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
>>>>>> + RISCV_EXCP_DOUBLE_TRAP = 0x10,
>>>>>> RISCV_EXCP_SW_CHECK = 0x12, /* since: priv-1.13.0 */
>>>>>> RISCV_EXCP_HW_ERR = 0x13, /* since: priv-1.13.0 */
>>>>>> RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
>>>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>>>> index 395d8235ce..69da3c3384 100644
>>>>>> --- a/target/riscv/cpu_helper.c
>>>>>> +++ b/target/riscv/cpu_helper.c
>>>>>> @@ -575,7 +575,9 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState
>>>>>> *env)
>>>>>> mstatus_mask |= MSTATUS_FS;
>>>>>> }
>>>>>> bool current_virt = env->virt_enabled;
>>>>>> -
>>>>>> + if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
>>>>>> + mstatus_mask |= MSTATUS_SDT;
>>>>>> + }
>>>>>> g_assert(riscv_has_ext(env, RVH));
>>>>>>
>>>>>> if (current_virt) {
>>>>>> @@ -1707,6 +1709,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>>>>> CPURISCVState *env = &cpu->env;
>>>>>> bool virt = env->virt_enabled;
>>>>>> bool write_gva = false;
>>>>>> + bool vsmode_exc;
>>>>>> uint64_t s;
>>>>>> int mode;
>>>>>>
>>>>>> @@ -1721,6 +1724,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>>>>> !(env->mip & (1 << cause));
>>>>>> bool vs_injected = env->hvip & (1 << cause) & env->hvien &&
>>>>>> !(env->mip & (1 << cause));
>>>>>> + bool smode_double_trap = false;
>>>>>> + uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
>>>>>> target_ulong tval = 0;
>>>>>> target_ulong tinst = 0;
>>>>>> target_ulong htval = 0;
>>>>>> @@ -1837,13 +1842,35 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>>>>> !async &&
>>>>>> mode == PRV_M;
>>>>>>
>>>>>> + vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) ||
>>>>>> vs_injected);
>>>>>> + /*
>>>>>> + * Check double trap condition only if already in S-mode and
>>>>>> targeting
>>>>>> + * S-mode
>>>>>> + */
>>>>>> + if (cpu->cfg.ext_ssdbltrp && env->priv == PRV_S && mode == PRV_S) {
>>>>>> + bool dte = (env->menvcfg & MENVCFG_DTE) != 0;
>>>>>> + bool sdt = (env->mstatus & MSTATUS_SDT) != 0;
>>>>>> + /* In VS or HS */
>>>>>> + if (riscv_has_ext(env, RVH)) {
>>>>>> + if (vsmode_exc) {
>>>>>> + /* VS -> VS */
>>>>>> + /* Stay in VS mode, use henvcfg instead of menvcfg*/
>>>>>> + dte = (env->henvcfg & HENVCFG_DTE) != 0;
>>>>>> + } else if (env->virt_enabled) {
>>>>>> + /* VS -> HS */
>>>>>> + dte = false;
>>>>>
>>>>> I don't follow why this is false
>>>>
>>>> Hi Alistair,
>>>>
>>>> It's indeed probably lacking some comments here. The rationale is that
>>>> if you are trapping from VS to HS, then at some point, you returned to
>>>> VS using a sret/mret and thus cleared DTE, so rather than checking the
>>
>> s/DTE/SDT
>>
>>>
>>> Why not just clear it at sret/mret? Instead of having this assumption
>>
>> It has been cleared but since registers were swapped to enter virt mode,
>> hypervisor SDT value is stored in mstatus_hs rather than mstatus. So I
>> could have wrote it this way:
>>
>> + } else if (env->virt_enabled) {
>> + /* VS -> HS */
>> + sdt = (env->mstatus_hs & MSTATUS_SDT);
>>
>> Since this is always 0 better assume it is 0 (but should be sdt = 0
>> instead of dte = 0). But if you prefer using mstatus_hs for clarity, I
>> can use that of course.
>
> We should use the register directly. That way if it is accidently not
> cleared it's easier to catch and it makes the code easier to read
Yes indeed. I did that in the next version.
Thanks,
Clément
>
> Alistair
>
>>
>> Clément
>>
>>>
>>> Alistair
>>>
>>>> value of mstatus_hs, just assume it is false.
>>>>
>>>> Thanks,
>>>>
>>>> Clément
>>>>
>>>>>
>>>>> Alistair
>>>>
>>
