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~