| Message ID | 20200824084158.1769-1-jiangyifei@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: raise exception to HS-mode at get_physical_address | expand |
On Mon, Aug 24, 2020 at 1:43 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> Thanks for the patch! Sorry for the delay here. > --- > target/riscv/cpu.h | 1 + > target/riscv/cpu_helper.c | 12 ++++++++++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index a804a5d0ba..8b3b368d6a 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -85,6 +85,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 fd1d373b6f..1635b09c41 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -440,7 +440,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; Can we set guest_phys_fault_addr in riscv_cpu_tlb_fill() instead? > + return TRANSLATE_G_STAGE_FAIL; > } > > pte_addr = vbase + idx * ptesize; > @@ -728,12 +731,17 @@ 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); > > + 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) { Otherwise this patch looks correct. Alistair > /* Second stage lookup */ > im_address = pa; > > -- > 2.19.1 > > >
> -----Original Message----- > From: Alistair Francis [mailto:alistair23@gmail.com] > Sent: Saturday, September 26, 2020 6:24 AM > To: Jiangyifei <jiangyifei@huawei.com> > Cc: qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>; open > list:RISC-V <qemu-riscv@nongnu.org>; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; Sagar Karandikar > <sagark@eecs.berkeley.edu>; Bastian Koppelmann > <kbastian@mail.uni-paderborn.de>; Zhangxiaofeng (F) > <victor.zhangxiaofeng@huawei.com>; Alistair Francis > <Alistair.Francis@wdc.com>; yinyipeng <yinyipeng1@huawei.com>; Palmer > Dabbelt <palmer@dabbelt.com>; Wubin (H) <wu.wubin@huawei.com>; > dengkai (A) <dengkai1@huawei.com> > Subject: Re: [PATCH] target/riscv: raise exception to HS-mode at > get_physical_address > > On Mon, Aug 24, 2020 at 1:43 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> > > Thanks for the patch! > > Sorry for the delay here. > > > --- > > target/riscv/cpu.h | 1 + > > target/riscv/cpu_helper.c | 12 ++++++++++-- > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index > > a804a5d0ba..8b3b368d6a 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -85,6 +85,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 fd1d373b6f..1635b09c41 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -440,7 +440,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; > > Can we set guest_phys_fault_addr in riscv_cpu_tlb_fill() instead? Hi Alistair, It's not easy to do that. The key is that the wrong address(the `base` variable) is not visible to riscv_cpu_tlb_fill(). Because the wrong address may be from any level of PTE which can't be easily obtained by riscv_cpu_tlb_fill(). The alternative is to add an out parameter in get_physical_address(). But it is not either elegant. What is your advice? Best Regards, Yifei > > > + return TRANSLATE_G_STAGE_FAIL; > > } > > > > pte_addr = vbase + idx * ptesize; @@ -728,12 +731,17 @@ > > 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); > > > > + 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) { > > Otherwise this patch looks correct. > > Alistair > > > /* Second stage lookup */ > > im_address = pa; > > > > -- > > 2.19.1 > > > > > >
On Sun, Sep 27, 2020 at 12:54 AM Jiangyifei <jiangyifei@huawei.com> wrote: > > > > > -----Original Message----- > > From: Alistair Francis [mailto:alistair23@gmail.com] > > Sent: Saturday, September 26, 2020 6:24 AM > > To: Jiangyifei <jiangyifei@huawei.com> > > Cc: qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>; open > > list:RISC-V <qemu-riscv@nongnu.org>; Zhanghailiang > > <zhang.zhanghailiang@huawei.com>; Sagar Karandikar > > <sagark@eecs.berkeley.edu>; Bastian Koppelmann > > <kbastian@mail.uni-paderborn.de>; Zhangxiaofeng (F) > > <victor.zhangxiaofeng@huawei.com>; Alistair Francis > > <Alistair.Francis@wdc.com>; yinyipeng <yinyipeng1@huawei.com>; Palmer > > Dabbelt <palmer@dabbelt.com>; Wubin (H) <wu.wubin@huawei.com>; > > dengkai (A) <dengkai1@huawei.com> > > Subject: Re: [PATCH] target/riscv: raise exception to HS-mode at > > get_physical_address > > > > On Mon, Aug 24, 2020 at 1:43 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> > > > > Thanks for the patch! > > > > Sorry for the delay here. > > > > > --- > > > target/riscv/cpu.h | 1 + > > > target/riscv/cpu_helper.c | 12 ++++++++++-- > > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index > > > a804a5d0ba..8b3b368d6a 100644 > > > --- a/target/riscv/cpu.h > > > +++ b/target/riscv/cpu.h > > > @@ -85,6 +85,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 fd1d373b6f..1635b09c41 100644 > > > --- a/target/riscv/cpu_helper.c > > > +++ b/target/riscv/cpu_helper.c > > > @@ -440,7 +440,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; > > > > Can we set guest_phys_fault_addr in riscv_cpu_tlb_fill() instead? > > Hi Alistair, > > It's not easy to do that. The key is that the wrong address(the `base` variable) is not visible to riscv_cpu_tlb_fill(). > Because the wrong address may be from any level of PTE which can't be easily obtained by riscv_cpu_tlb_fill(). > The alternative is to add an out parameter in get_physical_address(). But it is not either elegant. > What is your advice? You are right. Good call. In which case this looks like the right way to do this. Can you add a small comment in riscv_cpu_tlb_fill() to indicate that guest_phys_fault_addr has already been set when handling the error? The TLB filling code is pretty complex so the more documentation the better. Alistair > > Best Regards, > Yifei > > > > > > + return TRANSLATE_G_STAGE_FAIL; > > > } > > > > > > pte_addr = vbase + idx * ptesize; @@ -728,12 +731,17 @@ > > > 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); > > > > > > + 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) { > > > > Otherwise this patch looks correct. > > > > Alistair > > > > > /* Second stage lookup */ > > > im_address = pa; > > > > > > -- > > > 2.19.1 > > > > > > > > >
> -----Original Message----- > From: Alistair Francis [mailto:alistair23@gmail.com] > Sent: Thursday, October 1, 2020 8:00 AM > To: Jiangyifei <jiangyifei@huawei.com> > Cc: qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>; open > list:RISC-V <qemu-riscv@nongnu.org>; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; Sagar Karandikar > <sagark@eecs.berkeley.edu>; Bastian Koppelmann > <kbastian@mail.uni-paderborn.de>; Zhangxiaofeng (F) > <victor.zhangxiaofeng@huawei.com>; Alistair Francis > <Alistair.Francis@wdc.com>; yinyipeng <yinyipeng1@huawei.com>; Palmer > Dabbelt <palmer@dabbelt.com>; Wubin (H) <wu.wubin@huawei.com>; > dengkai (A) <dengkai1@huawei.com> > Subject: Re: [PATCH] target/riscv: raise exception to HS-mode at > get_physical_address > > On Sun, Sep 27, 2020 at 12:54 AM Jiangyifei <jiangyifei@huawei.com> wrote: > > > > > > > > > -----Original Message----- > > > From: Alistair Francis [mailto:alistair23@gmail.com] > > > Sent: Saturday, September 26, 2020 6:24 AM > > > To: Jiangyifei <jiangyifei@huawei.com> > > > Cc: qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>; open > > > list:RISC-V <qemu-riscv@nongnu.org>; Zhanghailiang > > > <zhang.zhanghailiang@huawei.com>; Sagar Karandikar > > > <sagark@eecs.berkeley.edu>; Bastian Koppelmann > > > <kbastian@mail.uni-paderborn.de>; Zhangxiaofeng (F) > > > <victor.zhangxiaofeng@huawei.com>; Alistair Francis > > > <Alistair.Francis@wdc.com>; yinyipeng <yinyipeng1@huawei.com>; > > > Palmer Dabbelt <palmer@dabbelt.com>; Wubin (H) > > > <wu.wubin@huawei.com>; dengkai (A) <dengkai1@huawei.com> > > > Subject: Re: [PATCH] target/riscv: raise exception to HS-mode at > > > get_physical_address > > > > > > On Mon, Aug 24, 2020 at 1:43 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> > > > > > > Thanks for the patch! > > > > > > Sorry for the delay here. > > > > > > > --- > > > > target/riscv/cpu.h | 1 + > > > > target/riscv/cpu_helper.c | 12 ++++++++++-- > > > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index > > > > a804a5d0ba..8b3b368d6a 100644 > > > > --- a/target/riscv/cpu.h > > > > +++ b/target/riscv/cpu.h > > > > @@ -85,6 +85,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 fd1d373b6f..1635b09c41 100644 > > > > --- a/target/riscv/cpu_helper.c > > > > +++ b/target/riscv/cpu_helper.c > > > > @@ -440,7 +440,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; > > > > > > Can we set guest_phys_fault_addr in riscv_cpu_tlb_fill() instead? > > > > Hi Alistair, > > > > It's not easy to do that. The key is that the wrong address(the `base` variable) > is not visible to riscv_cpu_tlb_fill(). > > Because the wrong address may be from any level of PTE which can't be > easily obtained by riscv_cpu_tlb_fill(). > > The alternative is to add an out parameter in get_physical_address(). But it is > not either elegant. > > What is your advice? > > You are right. Good call. > > In which case this looks like the right way to do this. Can you add a small > comment in riscv_cpu_tlb_fill() to indicate that guest_phys_fault_addr has > already been set when handling the error? > The TLB filling code is pretty complex so the more documentation the better. > > Alistair > I have already sent a new patch to add necessary comment. Please review the new patch. Thanks, Yifei > > > > Best Regards, > > Yifei > > > > > > > > > + return TRANSLATE_G_STAGE_FAIL; > > > > } > > > > > > > > pte_addr = vbase + idx * ptesize; @@ -728,12 +731,17 > > > > @@ 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); > > > > > > > > + 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) { > > > > > > Otherwise this patch looks correct. > > > > > > Alistair > > > > > > > /* Second stage lookup */ > > > > im_address = pa; > > > > > > > > -- > > > > 2.19.1 > > > > > > > > > > > >
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index a804a5d0ba..8b3b368d6a 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -85,6 +85,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 fd1d373b6f..1635b09c41 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -440,7 +440,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; @@ -728,12 +731,17 @@ 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); + 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;