| Message ID | 20201014101728.848-1-jiangyifei@huawei.com |
|---|---|
| State | Accepted |
| Commit | 33a9a57d2c31ec9ed68858911dc490b5de15f342 |
| Headers | show |
| Series | [V3] target/riscv: raise exception to HS-mode at get_physical_address | expand |
On Wed, Oct 14, 2020 at 3:18 AM Yifei Jiang <jiangyifei@huawei.com> wrote: > > VS-stage translation at get_physical_address needs to translate pte > address by G-stage translation. But the G-stage translation error > can not be distinguished from VS-stage translation error in > riscv_cpu_tlb_fill. On migration, destination needs to rebuild pte, > and this G-stage translation error must be handled by HS-mode. So > introduce TRANSLATE_STAGE2_FAIL so that riscv_cpu_tlb_fill could > distinguish and raise it to HS-mode. > > Signed-off-by: Yifei Jiang <jiangyifei@huawei.com> > Signed-off-by: Yipeng Yin <yinyipeng1@huawei.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.h | 10 +++++++--- > target/riscv/cpu_helper.c | 35 ++++++++++++++++++++++++++--------- > 2 files changed, 33 insertions(+), 12 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index de275782e6..de4705bb57 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -82,9 +82,13 @@ enum { > > #define VEXT_VERSION_0_07_1 0x00000701 > > -#define TRANSLATE_PMP_FAIL 2 > -#define TRANSLATE_FAIL 1 > -#define TRANSLATE_SUCCESS 0 > +enum { > + TRANSLATE_SUCCESS, > + TRANSLATE_FAIL, > + TRANSLATE_PMP_FAIL, > + TRANSLATE_G_STAGE_FAIL > +}; > + > #define MMU_USER_IDX 3 > > #define MAX_RISCV_PMPS (16) > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 904899054d..ae66722d32 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -316,6 +316,8 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv) > * @physical: This will be set to the calculated physical address > * @prot: The returned protection attributes > * @addr: The virtual address to be translated > + * @fault_pte_addr: If not NULL, this will be set to fault pte address > + * when a error occurs on pte address translation. > * @access_type: The type of MMU access > * @mmu_idx: Indicates current privilege level > * @first_stage: Are we in first stage translation? > @@ -324,6 +326,7 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv) > */ > static int get_physical_address(CPURISCVState *env, hwaddr *physical, > int *prot, target_ulong addr, > + target_ulong *fault_pte_addr, > int access_type, int mmu_idx, > bool first_stage, bool two_stage) > { > @@ -447,11 +450,14 @@ restart: > > /* Do the second stage translation on the base PTE address. */ > int vbase_ret = get_physical_address(env, &vbase, &vbase_prot, > - base, MMU_DATA_LOAD, > + base, NULL, MMU_DATA_LOAD, > mmu_idx, false, true); > > if (vbase_ret != TRANSLATE_SUCCESS) { > - return vbase_ret; > + if (fault_pte_addr) { > + *fault_pte_addr = (base + idx * ptesize) >> 2; > + } > + return TRANSLATE_G_STAGE_FAIL; > } > > pte_addr = vbase + idx * ptesize; > @@ -632,13 +638,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) > int prot; > int mmu_idx = cpu_mmu_index(&cpu->env, false); > > - if (get_physical_address(env, &phys_addr, &prot, addr, 0, mmu_idx, > + if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx, > true, riscv_cpu_virt_enabled(env))) { > return -1; > } > > if (riscv_cpu_virt_enabled(env)) { > - if (get_physical_address(env, &phys_addr, &prot, phys_addr, > + if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL, > 0, mmu_idx, false, true)) { > return -1; > } > @@ -727,19 +733,30 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > if (riscv_cpu_virt_enabled(env) || > (riscv_cpu_two_stage_lookup(env) && access_type != MMU_INST_FETCH)) { > /* Two stage lookup */ > - ret = get_physical_address(env, &pa, &prot, address, access_type, > + ret = get_physical_address(env, &pa, &prot, address, > + &env->guest_phys_fault_addr, access_type, > mmu_idx, true, true); > > + /* > + * A G-stage exception may be triggered during two state lookup. > + * 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_SUCCESS) { > /* Second stage lookup */ > im_address = pa; > > - ret = get_physical_address(env, &pa, &prot2, im_address, > + ret = get_physical_address(env, &pa, &prot2, im_address, NULL, > access_type, mmu_idx, false, true); > > qemu_log_mask(CPU_LOG_MMU, > @@ -768,8 +785,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > } > } else { > /* Single stage lookup */ > - ret = get_physical_address(env, &pa, &prot, address, access_type, > - mmu_idx, true, false); > + ret = get_physical_address(env, &pa, &prot, address, NULL, > + access_type, mmu_idx, true, false); > > qemu_log_mask(CPU_LOG_MMU, > "%s address=%" VADDR_PRIx " ret %d physical " > -- > 2.19.1 > >
On 10/14/20 3:17 AM, Yifei Jiang wrote: > + if (fault_pte_addr) { > + *fault_pte_addr = (base + idx * ptesize) >> 2; The shift is wrong. It should be exactly like... > + } > + return TRANSLATE_G_STAGE_FAIL; > } > > pte_addr = vbase + idx * ptesize; ... this. r~
> -----Original Message----- > From: Richard Henderson [mailto:richard.henderson@linaro.org] > Sent: Thursday, October 15, 2020 4:22 AM > To: Jiangyifei <jiangyifei@huawei.com>; qemu-devel@nongnu.org; > qemu-riscv@nongnu.org > Cc: palmer@dabbelt.com; Alistair.Francis@wdc.com; > sagark@eecs.berkeley.edu; kbastian@mail.uni-paderborn.de; Zhangxiaofeng > (F) <victor.zhangxiaofeng@huawei.com>; Wubin (H) <wu.wubin@huawei.com>; > Zhanghailiang <zhang.zhanghailiang@huawei.com>; dengkai (A) > <dengkai1@huawei.com>; yinyipeng <yinyipeng1@huawei.com> > Subject: Re: [PATCH V3] target/riscv: raise exception to HS-mode at > get_physical_address > > On 10/14/20 3:17 AM, Yifei Jiang wrote: > > + if (fault_pte_addr) { > > + *fault_pte_addr = (base + idx * ptesize) >> 2; > > The shift is wrong. It should be exactly like... > We have tested in the VM migration. fault_pte_addr will eventually be assigned to htval register. Description of htval register according to the specification: When a guest-page-fault trap is taken into HS-mode, htval is written with either zero or the guest physical address that faulted, shifted right by 2 bits. In addition, fault_pte_addr is named after env->guest_phys_fault_addr, which makes sense in a sense. Yifei > > + } > > + return TRANSLATE_G_STAGE_FAIL; > > } > > > > pte_addr = vbase + idx * ptesize; > > ... this. > > > r~
On Wed, Oct 14, 2020 at 6:59 PM Jiangyifei <jiangyifei@huawei.com> wrote: > > > > -----Original Message----- > > From: Richard Henderson [mailto:richard.henderson@linaro.org] > > Sent: Thursday, October 15, 2020 4:22 AM > > To: Jiangyifei <jiangyifei@huawei.com>; qemu-devel@nongnu.org; > > qemu-riscv@nongnu.org > > Cc: palmer@dabbelt.com; Alistair.Francis@wdc.com; > > sagark@eecs.berkeley.edu; kbastian@mail.uni-paderborn.de; Zhangxiaofeng > > (F) <victor.zhangxiaofeng@huawei.com>; Wubin (H) <wu.wubin@huawei.com>; > > Zhanghailiang <zhang.zhanghailiang@huawei.com>; dengkai (A) > > <dengkai1@huawei.com>; yinyipeng <yinyipeng1@huawei.com> > > Subject: Re: [PATCH V3] target/riscv: raise exception to HS-mode at > > get_physical_address > > > > On 10/14/20 3:17 AM, Yifei Jiang wrote: > > > + if (fault_pte_addr) { > > > + *fault_pte_addr = (base + idx * ptesize) >> 2; > > > > The shift is wrong. It should be exactly like... > > > > We have tested in the VM migration. > > fault_pte_addr will eventually be assigned to htval register. > > Description of htval register according to the specification: > When a guest-page-fault trap is taken into HS-mode, htval is written with either zero > or the guest physical address that faulted, shifted right by 2 bits. Yep, I agree that the shift is correct, it's what we do when we set guest_phys_fault_addr in other places. It is a little confusing that we shift it in get_physical_address(), instead of when guest_phys_fault_addr is set. In this case you are setting guest_phys_fault_addr directly when calling get_physical_address(... &env->guest_phys_fault_addr ...). I have added this comment to make sure it's clear and applied it, I hope that's ok. diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 4ea9510c07..4652082df1 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -318,6 +318,7 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv) * @addr: The virtual address to be translated * @fault_pte_addr: If not NULL, this will be set to fault pte address * when a error occurs on pte address translation. + * This will already be shifted to match htval. * @access_type: The type of MMU access * @mmu_idx: Indicates current privilege level * @first_stage: Are we in first stage translation? Alistair > > In addition, fault_pte_addr is named after env->guest_phys_fault_addr, which makes > sense in a sense. > > Yifei > > > > + } > > > + return TRANSLATE_G_STAGE_FAIL; > > > } > > > > > > pte_addr = vbase + idx * ptesize; > > > > ... this. > > > > > > r~
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index de275782e6..de4705bb57 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -82,9 +82,13 @@ enum { #define VEXT_VERSION_0_07_1 0x00000701 -#define TRANSLATE_PMP_FAIL 2 -#define TRANSLATE_FAIL 1 -#define TRANSLATE_SUCCESS 0 +enum { + TRANSLATE_SUCCESS, + TRANSLATE_FAIL, + TRANSLATE_PMP_FAIL, + TRANSLATE_G_STAGE_FAIL +}; + #define MMU_USER_IDX 3 #define MAX_RISCV_PMPS (16) diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 904899054d..ae66722d32 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -316,6 +316,8 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv) * @physical: This will be set to the calculated physical address * @prot: The returned protection attributes * @addr: The virtual address to be translated + * @fault_pte_addr: If not NULL, this will be set to fault pte address + * when a error occurs on pte address translation. * @access_type: The type of MMU access * @mmu_idx: Indicates current privilege level * @first_stage: Are we in first stage translation? @@ -324,6 +326,7 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv) */ static int get_physical_address(CPURISCVState *env, hwaddr *physical, int *prot, target_ulong addr, + target_ulong *fault_pte_addr, int access_type, int mmu_idx, bool first_stage, bool two_stage) { @@ -447,11 +450,14 @@ restart: /* Do the second stage translation on the base PTE address. */ int vbase_ret = get_physical_address(env, &vbase, &vbase_prot, - base, MMU_DATA_LOAD, + base, NULL, MMU_DATA_LOAD, mmu_idx, false, true); if (vbase_ret != TRANSLATE_SUCCESS) { - return vbase_ret; + if (fault_pte_addr) { + *fault_pte_addr = (base + idx * ptesize) >> 2; + } + return TRANSLATE_G_STAGE_FAIL; } pte_addr = vbase + idx * ptesize; @@ -632,13 +638,13 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) int prot; int mmu_idx = cpu_mmu_index(&cpu->env, false); - if (get_physical_address(env, &phys_addr, &prot, addr, 0, mmu_idx, + if (get_physical_address(env, &phys_addr, &prot, addr, NULL, 0, mmu_idx, true, riscv_cpu_virt_enabled(env))) { return -1; } if (riscv_cpu_virt_enabled(env)) { - if (get_physical_address(env, &phys_addr, &prot, phys_addr, + if (get_physical_address(env, &phys_addr, &prot, phys_addr, NULL, 0, mmu_idx, false, true)) { return -1; } @@ -727,19 +733,30 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, if (riscv_cpu_virt_enabled(env) || (riscv_cpu_two_stage_lookup(env) && access_type != MMU_INST_FETCH)) { /* Two stage lookup */ - ret = get_physical_address(env, &pa, &prot, address, access_type, + ret = get_physical_address(env, &pa, &prot, address, + &env->guest_phys_fault_addr, access_type, mmu_idx, true, true); + /* + * A G-stage exception may be triggered during two state lookup. + * 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_SUCCESS) { /* Second stage lookup */ im_address = pa; - ret = get_physical_address(env, &pa, &prot2, im_address, + ret = get_physical_address(env, &pa, &prot2, im_address, NULL, access_type, mmu_idx, false, true); qemu_log_mask(CPU_LOG_MMU, @@ -768,8 +785,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, } } else { /* Single stage lookup */ - ret = get_physical_address(env, &pa, &prot, address, access_type, - mmu_idx, true, false); + ret = get_physical_address(env, &pa, &prot, address, NULL, + access_type, mmu_idx, true, false); qemu_log_mask(CPU_LOG_MMU, "%s address=%" VADDR_PRIx " ret %d physical "