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
