qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: dramforever
Subject: Re: [PATCH v5 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
Date: Fri, 10 Jun 2022 19:50:30 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0

>
>> 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.
Just a clarification/suggestion: It might be easier to only write non-zero for
instructions with currently defined transformation. Writing zero is always
legal, but writing an undefined transformed instruction is not.
>>> @@ -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.

If an instruction guest-page fault occurs due to a G-stage fault while doing
VS-stage page table walk, i.e. env->two_stage_indirect_lookup is true (I had
mistakenly wrote env->two_stage_lookup earlier), and the faulting guest
physical address (env->guest_phys_fault_addr) is written to mtval2/htval,
[m|h]tinst must be a pseudoinstruction and not zero. This case is not handled
in the v5 patch.

dramforever



reply via email to

[Prev in Thread] Current Thread [Next in Thread]