RE: [PATCH] target/riscv: raise exception to HS-mode at get_physical_address

[Jiangyifei]

> -----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
> > > >
> > > >
> > > >