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

[Anup Patel]

On Fri, Jun 10, 2022 at 3:00 PM dramforever <dramforever@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