On Fri, Jun 10, 2022 at 3:00 PM dramforever <dramfore...@live.com> wrote: > > 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.
Sure, I will update. > > > + } 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. Okay, I will update. > > 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. Okay, I will update. > > > + 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. Well, I am aware of how "Addr. Offset" field is set but I was not aware that QEMU generates misaligned exception in a specific case (i.e. misaligned atomic). I will update this patch to > > > @@ -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. This is already handled because tinst is initialized to zero. Regards, Anup > > dramforever