Re: [Qemu-devel] [PULL v2 12/27] target/mips: Convert to CPUClass::tlb_fill

[Aleksandar Markovic]

On May 10, 2019 8:57 PM, "Richard Henderson" <address@hidden>
wrote:
>

Please change the title to 'target/mips: Switch to using
mips_cpu_tlb_fill()', or something along that line.

Also, the reason for changing the field access_type to mips_access type
should be explained in the commit message.

This commit message is generally poor, as it explains relatively
unimportant logging issue, while not explaining the core of the change.

Thanks,
Aleksandar

> Note that env->active_tc.PC is removed from the qemu_log as that value
> is garbage.  The PC isn't recovered until cpu_restore_state, called from
> cpu_loop_exit_restore, called from do_raise_exception_err.
>
> Cc: Aleksandar Markovic <address@hidden>
> Cc: Aleksandar Rikalo <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/mips/internal.h  |  5 +++--
>  target/mips/cpu.c       |  5 ++---
>  target/mips/helper.c    | 45 ++++++++++++++++++++++-------------------
>  target/mips/op_helper.c | 15 --------------
>  4 files changed, 29 insertions(+), 41 deletions(-)
>
> diff --git a/target/mips/internal.h b/target/mips/internal.h
> index 286e3888ab..b2b41a51ab 100644
> --- a/target/mips/internal.h
> +++ b/target/mips/internal.h
> @@ -202,8 +202,9 @@ void cpu_mips_start_count(CPUMIPSState *env);
>  void cpu_mips_stop_count(CPUMIPSState *env);
>
>  /* helper.c */
> -int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size,
int rw,
> -                              int mmu_idx);
> +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                       MMUAccessType access_type, int mmu_idx,
> +                       bool probe, uintptr_t retaddr);
>
>  /* op_helper.c */
>  uint32_t float_class_s(uint32_t arg, float_status *fst);
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index e217fb3e36..a33058609a 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -197,9 +197,7 @@ static void mips_cpu_class_init(ObjectClass *c, void
*data)
>      cc->synchronize_from_tb = mips_cpu_synchronize_from_tb;
>      cc->gdb_read_register = mips_cpu_gdb_read_register;
>      cc->gdb_write_register = mips_cpu_gdb_write_register;
> -#ifdef CONFIG_USER_ONLY
> -    cc->handle_mmu_fault = mips_cpu_handle_mmu_fault;
> -#else
> +#ifndef CONFIG_USER_ONLY
>      cc->do_unassigned_access = mips_cpu_unassigned_access;
>      cc->do_unaligned_access = mips_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = mips_cpu_get_phys_page_debug;
> @@ -208,6 +206,7 @@ static void mips_cpu_class_init(ObjectClass *c, void
*data)
>      cc->disas_set_info = mips_cpu_disas_set_info;
>  #ifdef CONFIG_TCG
>      cc->tcg_initialize = mips_tcg_init;
> +    cc->tlb_fill = mips_cpu_tlb_fill;
>  #endif
>
>      cc->gdb_num_core_regs = 73;
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index 86e622efb8..3a4917ce7b 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -874,31 +874,25 @@ refill:
>  #endif
>  #endif
>
> -int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int
rw,
> -                              int mmu_idx)
> +bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                       MMUAccessType access_type, int mmu_idx,
> +                       bool probe, uintptr_t retaddr)
>  {
>      MIPSCPU *cpu = MIPS_CPU(cs);
>      CPUMIPSState *env = &cpu->env;
>  #if !defined(CONFIG_USER_ONLY)
>      hwaddr physical;
>      int prot;
> -    int access_type;
> +    int mips_access_type;
>  #endif
>      int ret = TLBRET_BADADDR;
>
> -#if 0
> -    log_cpu_state(cs, 0);
> -#endif
> -    qemu_log_mask(CPU_LOG_MMU,
> -              "%s pc " TARGET_FMT_lx " ad %" VADDR_PRIx " rw %d mmu_idx
%d\n",
> -              __func__, env->active_tc.PC, address, rw, mmu_idx);
> -
>      /* data access */
>  #if !defined(CONFIG_USER_ONLY)
>      /* XXX: put correct access by using cpu_restore_state() correctly */
> -    access_type = ACCESS_INT;
> -    ret = get_physical_address(env, &physical, &prot,
> -                               address, rw, access_type, mmu_idx);
> +    mips_access_type = ACCESS_INT;
> +    ret = get_physical_address(env, &physical, &prot, address,
> +                               access_type, mips_access_type, mmu_idx);
>      switch (ret) {
>      case TLBRET_MATCH:
>          qemu_log_mask(CPU_LOG_MMU,
> @@ -915,7 +909,7 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr
address, int size, int rw,
>          tlb_set_page(cs, address & TARGET_PAGE_MASK,
>                       physical & TARGET_PAGE_MASK, prot | PAGE_EXEC,
>                       mmu_idx, TARGET_PAGE_SIZE);
> -        return 0;
> +        return true;
>      }
>  #if !defined(TARGET_MIPS64)
>      if ((ret == TLBRET_NOMATCH) && (env->tlb->nb_tlb > 1)) {
> @@ -926,27 +920,36 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, vaddr
address, int size, int rw,
>          int mode = (env->hflags & MIPS_HFLAG_KSU);
>          bool ret_walker;
>          env->hflags &= ~MIPS_HFLAG_KSU;
> -        ret_walker = page_table_walk_refill(env, address, rw, mmu_idx);
> +        ret_walker = page_table_walk_refill(env, address, access_type,
mmu_idx);
>          env->hflags |= mode;
>          if (ret_walker) {
> -            ret = get_physical_address(env, &physical, &prot,
> -                                       address, rw, access_type,
mmu_idx);
> +            ret = get_physical_address(env, &physical, &prot, address,
> +                                       access_type, mips_access_type,
mmu_idx);¿
>              if (ret == TLBRET_MATCH) {
>                  tlb_set_page(cs, address & TARGET_PAGE_MASK,
>                               physical & TARGET_PAGE_MASK, prot |
PAGE_EXEC,
>                               mmu_idx, TARGET_PAGE_SIZE);
> -                return 0;
> +                return true;
>              }
>          }
>      }
>  #endif
> +    if (probe) {
> +        return false;
> +    }
>  #endif
>
> -    raise_mmu_exception(env, address, rw, ret);
> -    return 1;
> +    raise_mmu_exception(env, address, access_type, ret);
> +    do_raise_exception_err(env, cs->exception_index, env->error_code,
retaddr);
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +void tlb_fill(CPUState *cs, target_ulong addr, int size,
> +              MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
> +{
> +    mips_cpu_tlb_fill(cs, addr, size, access_type, mmu_idx, false,
retaddr);
>  }
>
> -#if !defined(CONFIG_USER_ONLY)
>  hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong
address, int rw)
>  {
>      hwaddr physical;
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 0f272a5b93..6d86912958 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -2669,21 +2669,6 @@ void mips_cpu_do_unaligned_access(CPUState *cs,
vaddr addr,
>      do_raise_exception_err(env, excp, error_code, retaddr);
>  }
>
> -void tlb_fill(CPUState *cs, target_ulong addr, int size,
> -              MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
> -{
> -    int ret;
> -
> -    ret = mips_cpu_handle_mmu_fault(cs, addr, size, access_type,
mmu_idx);
> -    if (ret) {
> -        MIPSCPU *cpu = MIPS_CPU(cs);
> -        CPUMIPSState *env = &cpu->env;
> -
> -        do_raise_exception_err(env, cs->exception_index,
> -                               env->error_code, retaddr);
> -    }
> -}
> -
>  void mips_cpu_unassigned_access(CPUState *cs, hwaddr addr,
>                                  bool is_write, bool is_exec, int unused,
>                                  unsigned size)
> --
> 2.17.1
>
>