Re: [PATCH v5 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()

[dramforever]

Hi Anup Patel,

I think there are some misunderstandings of the privileged spec with regards to
[m|h]tinst handling. Here are some possible issues I've found:

> +            case OPC_RISC_C_FUNC_FLD_LQ:
> +                if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
> +                    xinsn = OPC_RISC_FLD;
> +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> +                    xinsn_has_addr_offset = true;
> +                }
> +                break;

The privileged spec requires that 'for basic loads and stores, the
transformations replace the instruction’s immediate offset fields with zero',
so this SET_I_IMM() line isn't necessary. Similarly for all the compressed
instruction cases, the SET_I_IMM() and SET_S_IMM() calls are all unnecessary.

> +    } else {
> +        /* No need to transform 32bit (or wider) instructions */
> +        xinsn = insn;

For AMO, lr, sc, and hypervisor load/store instructions, this is fine. But as
above, 32-bit integer load/store instructions and floating point load/store
instructions need have their immediate fields cleared to zero.

In addition, the various V-extension vector load/store instructions do not have
defined transformations, so they should show up in [m|h]tinst as all zeros.

> +    if (xinsn_has_addr_offset) {
> +        /*
> +         * The "Addr. Offset" field in transformed instruction is non-zero
> +         * only for misaligned load/store traps which are very unlikely on
> +         * QEMU so for now always set "Addr. Offset" to zero.
> +         */
> +        xinsn = SET_RS1(xinsn, 0);
> +    }

There seems to be two misconceptions here:

Firstly, QEMU *does* raise address misaligned exceptions for misaligned atomic
accesses.

However, if I understood correctly, the address misaligned exceptions are
irrelevant here because 'Addr. Offset' is only non-zero for a misaligned
accesses that faults but *not* due to an address misaligned exception.

For example, if an ld instruction reads 8 bytes starting from 0xa00ffe, and the
page 0xa00000 to 0xa00fff is mapped, but 0xa01000 to 0xa01fff is not, a load
page fault is raised with [m|s]tval = 0xa01000, while the original virtual
address of the access is 0xa00ffe. The 'Addr. Offset' in this case is 2, i.e.
the difference calculated with (0xa01000 - 0xa00ffe). This means that we *do*
have to set 'Addr. Offset' *because* we handle some misaligned load/store
instructions.
 
> @@ -1355,18 +1558,31 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      if (!async) {
>          /* set tval to badaddr for traps with address information */
>          switch (cause) {
> -        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
>          case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
>          case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
> -        case RISCV_EXCP_INST_ADDR_MIS:
> -        case RISCV_EXCP_INST_ACCESS_FAULT:
>          case RISCV_EXCP_LOAD_ADDR_MIS:
>          case RISCV_EXCP_STORE_AMO_ADDR_MIS:
>          case RISCV_EXCP_LOAD_ACCESS_FAULT:
>          case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
> -        case RISCV_EXCP_INST_PAGE_FAULT:
>          case RISCV_EXCP_LOAD_PAGE_FAULT:
>          case RISCV_EXCP_STORE_PAGE_FAULT:
> +            write_gva = env->two_stage_lookup;
> +            tval = env->badaddr;
> +            if (env->two_stage_indirect_lookup) {
> +                /*
> +                 * special pseudoinstruction for G-stage fault taken while
> +                 * doing VS-stage page table walk.
> +                 */
> +                tinst = (riscv_cpu_xlen(env) == 32) ? 0x00002000 : 
> 0x00003000;
> +            } else {
> +                /* transformed instruction for all other load/store faults */
> +                tinst = riscv_transformed_insn(env, env->bins);
> +            }
> +            break;
> +        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> +        case RISCV_EXCP_INST_ADDR_MIS:
> +        case RISCV_EXCP_INST_ACCESS_FAULT:
> +        case RISCV_EXCP_INST_PAGE_FAULT:
>              write_gva = env->two_stage_lookup;
>              tval = env->badaddr;
>              break;

Instruction guest-page faults should set [m|h]tinst to one of the
pseudoinstructions if env->two_stage_lookup is true. Otherwise it should set
[m|h]tinst to zero.

In any case, as this seems to be one of the first implementations of
[m|h]tinst, it might be worthwhile to confirm with the spec authors and clarify
any unclear bits before this gets released.

dramforever