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

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

[Yifei Jiang]

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        |  1 +
 target/riscv/cpu_helper.c | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index de275782e6..8b856d518e 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -86,6 +86,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 904899054d..096006dc00 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -451,7 +451,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;
@@ -730,12 +733,22 @@ 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);
 
+        /*
+         * A G-stage exception may be triggered during VS-stage translation.
+         * 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_FAIL && ret != TRANSLATE_G_STAGE_FAIL) {
             /* Second stage lookup */
             im_address = pa;
 
-- 
2.19.1

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

[Richard Henderson]

On 10/9/20 2:57 AM, Yifei Jiang wrote:
>  #define TRANSLATE_FAIL 1
>  #define TRANSLATE_SUCCESS 0
>  #define MMU_USER_IDX 3
> +#define TRANSLATE_G_STAGE_FAIL 4

Note that you're interleaving TRANSLATE_* around an unrelated define.  Perhaps
rearrange to

enum {
    TRANSLATE_SUCCESS = 0,
    TRANSLATE_FAIL,
    TRANSLATE_PMP_FAIL,
    TRANSLATE_G_STAGE_FAIL,
};


> +++ b/target/riscv/cpu_helper.c
> @@ -451,7 +451,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;
>              }

I don't think you can make this change to cpu state, as this function is also
used by gdb.  I think you'll need to add a new (target_ulong *) parameter to
get_physical_address to return this.

The usage in riscv_cpu_tlb_fill could pass &env->guest_phys_fault_addr, and the
usage in riscv_cpu_get_phys_page_debug could pass the address of a local
variable (which it then ignores).

Also, isn't the offset more naturally written idx * ptesize, as seen just a few
lines below?

> +        if (ret != TRANSLATE_FAIL && ret != TRANSLATE_G_STAGE_FAIL) {

Should this not be ret == TRANSLATE_SUCCESS?
This looks buggy with TRANSLATE_PMP_FAIL...


r~

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

[Jiangyifei]

> -----Original Message-----
> From: Richard Henderson [mailto:richard.henderson@linaro.org]
> Sent: Friday, October 9, 2020 10:34 PM
> To: Jiangyifei <jiangyifei@huawei.com>; qemu-devel@nongnu.org;
> qemu-riscv@nongnu.org
> Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> sagark@eecs.berkeley.edu; kbastian@mail.uni-paderborn.de; Zhangxiaofeng
> (F) <victor.zhangxiaofeng@huawei.com>; Alistair.Francis@wdc.com; yinyipeng
> <yinyipeng1@huawei.com>; palmer@dabbelt.com; Wubin (H)
> <wu.wubin@huawei.com>; dengkai (A) <dengkai1@huawei.com>
> Subject: Re: [PATCH V2] target/riscv: raise exception to HS-mode at
> get_physical_address
> 
> On 10/9/20 2:57 AM, Yifei Jiang wrote:
> >  #define TRANSLATE_FAIL 1
> >  #define TRANSLATE_SUCCESS 0
> >  #define MMU_USER_IDX 3
> > +#define TRANSLATE_G_STAGE_FAIL 4
> 
> Note that you're interleaving TRANSLATE_* around an unrelated define.
> Perhaps rearrange to
> 
> enum {
>     TRANSLATE_SUCCESS = 0,
>     TRANSLATE_FAIL,
>     TRANSLATE_PMP_FAIL,
>     TRANSLATE_G_STAGE_FAIL,
> };
> 

OK

> 
> > +++ b/target/riscv/cpu_helper.c
> > @@ -451,7 +451,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;
> >              }
> 
> I don't think you can make this change to cpu state, as this function is also used
> by gdb.  I think you'll need to add a new (target_ulong *) parameter to
> get_physical_address to return this.
> 
> The usage in riscv_cpu_tlb_fill could pass &env->guest_phys_fault_addr, and
> the usage in riscv_cpu_get_phys_page_debug could pass the address of a local
> variable (which it then ignores).
> 

OK

> Also, isn't the offset more naturally written idx * ptesize, as seen just a few
> lines below?

OK

> 
> > +        if (ret != TRANSLATE_FAIL && ret != TRANSLATE_G_STAGE_FAIL) {
> 
> Should this not be ret == TRANSLATE_SUCCESS?
> This looks buggy with TRANSLATE_PMP_FAIL...

On TRANSLATE_PMP_FAIL, it should not execute G-stage translation.
So I think it is ok for 'ret == TRANSLATE_SUCCESS'

I will send V3.

Yifei

> 
> 
> r~