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

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>
---
 target/riscv/cpu.h        | 10 +++++++---
 target/riscv/cpu_helper.c | 35 ++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 12 deletions(-)

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~