> -----Original Message-----
> From: Richard Henderson [mailto:[email protected]]
> Sent: Friday, October 9, 2020 10:34 PM
> To: Jiangyifei <[email protected]>; [email protected];
> [email protected]
> Cc: Zhanghailiang <[email protected]>;
> [email protected]; [email protected]; Zhangxiaofeng
> (F) <[email protected]>; [email protected]; yinyipeng
> <[email protected]>; [email protected]; Wubin (H)
> <[email protected]>; dengkai (A) <[email protected]>
> Subject: Re: [PATCH V2] target/riscv: raise exception to HS-mode at
> get_physical_address
>
> On 10/9/20 2:57 AM, Yifei Jiang wrote:
> > #define TRANSLATE_FAIL 1
> > #define TRANSLATE_SUCCESS 0
> > #define MMU_USER_IDX 3
> > +#define TRANSLATE_G_STAGE_FAIL 4
>
> Note that you're interleaving TRANSLATE_* around an unrelated define.
> Perhaps rearrange to
>
> enum {
> TRANSLATE_SUCCESS = 0,
> TRANSLATE_FAIL,
> TRANSLATE_PMP_FAIL,
> TRANSLATE_G_STAGE_FAIL,
> };
>
OK
>
> > +++ b/target/riscv/cpu_helper.c
> > @@ -451,7 +451,10 @@ restart:
> > mmu_idx,
> false,
> > true);
> >
> > if (vbase_ret != TRANSLATE_SUCCESS) {
> > - return vbase_ret;
> > + env->guest_phys_fault_addr = (base |
> > + (addr &
> > +
> (TARGET_PAGE_SIZE - 1))) >> 2;
> > + return TRANSLATE_G_STAGE_FAIL;
> > }
>
> I don't think you can make this change to cpu state, as this function is also
> used
> by gdb. I think you'll need to add a new (target_ulong *) parameter to
> get_physical_address to return this.
>
> The usage in riscv_cpu_tlb_fill could pass &env->guest_phys_fault_addr, and
> the usage in riscv_cpu_get_phys_page_debug could pass the address of a local
> variable (which it then ignores).
>
OK
> Also, isn't the offset more naturally written idx * ptesize, as seen just a
> few
> lines below?
OK
>
> > + if (ret != TRANSLATE_FAIL && ret != TRANSLATE_G_STAGE_FAIL) {
>
> Should this not be ret == TRANSLATE_SUCCESS?
> This looks buggy with TRANSLATE_PMP_FAIL...
On TRANSLATE_PMP_FAIL, it should not execute G-stage translation.
So I think it is ok for 'ret == TRANSLATE_SUCCESS'
I will send V3.
Yifei
>
>
> r~
