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

[Alistair Francis]

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~