| Message ID | 20201009075740.688-1-jiangyifei@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] target/riscv: raise exception to HS-mode at get_physical_address | expand |
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, }; > +++ 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). Also, isn't the offset more naturally written idx * ptesize, as seen just a few lines below? > + if (ret != TRANSLATE_FAIL && ret != TRANSLATE_G_STAGE_FAIL) { Should this not be ret == TRANSLATE_SUCCESS? This looks buggy with TRANSLATE_PMP_FAIL... r~
> -----Original Message----- > From: Richard Henderson [mailto:richard.henderson@linaro.org] > Sent: Friday, October 9, 2020 10:34 PM > To: Jiangyifei <jiangyifei@huawei.com>; qemu-devel@nongnu.org; > qemu-riscv@nongnu.org > Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>; > sagark@eecs.berkeley.edu; kbastian@mail.uni-paderborn.de; Zhangxiaofeng > (F) <victor.zhangxiaofeng@huawei.com>; Alistair.Francis@wdc.com; yinyipeng > <yinyipeng1@huawei.com>; palmer@dabbelt.com; Wubin (H) > <wu.wubin@huawei.com>; dengkai (A) <dengkai1@huawei.com> > 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~
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index de275782e6..8b856d518e 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -86,6 +86,7 @@ enum { #define TRANSLATE_FAIL 1 #define TRANSLATE_SUCCESS 0 #define MMU_USER_IDX 3 +#define TRANSLATE_G_STAGE_FAIL 4 #define MAX_RISCV_PMPS (16) diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 904899054d..096006dc00 100644 --- a/target/riscv/cpu_helper.c +++ 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; } pte_addr = vbase + idx * ptesize; @@ -730,12 +733,22 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, ret = get_physical_address(env, &pa, &prot, address, access_type, mmu_idx, true, true); + /* + * A G-stage exception may be triggered during VS-stage translation. + * And the env->guest_phys_fault_addr has already been set in + * get_physical_address(). + */ + if (ret == TRANSLATE_G_STAGE_FAIL) { + first_stage_error = false; + access_type = MMU_DATA_LOAD; + } + qemu_log_mask(CPU_LOG_MMU, "%s 1st-stage address=%" VADDR_PRIx " ret %d physical " TARGET_FMT_plx " prot %d\n", __func__, address, ret, pa, prot); - if (ret != TRANSLATE_FAIL) { + if (ret != TRANSLATE_FAIL && ret != TRANSLATE_G_STAGE_FAIL) { /* Second stage lookup */ im_address = pa;