Re: [Qemu-devel] [PULL 22/27] target/tricore: Convert to CPUClass::tlb_fill

[Philippe Mathieu-Daudé]

Hi Richard,

On 5/10/19 5:19 PM, Richard Henderson wrote:
> Acked-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/tricore/cpu.h       |  6 +++---
>  target/tricore/cpu.c       |  1 +
>  target/tricore/helper.c    | 27 +++++++++++++++++++--------
>  target/tricore/op_helper.c | 26 --------------------------
>  4 files changed, 23 insertions(+), 37 deletions(-)
...

>  #define DEFINE_TRICORE_CPU_TYPE(cpu_model, initfn) \
> diff --git a/target/tricore/helper.c b/target/tricore/helper.c
> index 78ee87c9ea..ed184fee3a 100644
> --- a/target/tricore/helper.c
> +++ b/target/tricore/helper.c
> @@ -50,8 +50,9 @@ static void raise_mmu_exception(CPUTriCoreState *env, 
> target_ulong address,
>  {
>  }
>  
> -int cpu_tricore_handle_mmu_fault(CPUState *cs, target_ulong address,
> -                                 int rw, int mmu_idx)
> +bool tricore_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                          MMUAccessType rw, int mmu_idx,
> +                          bool probe, uintptr_t retaddr)
>  {
>      TriCoreCPU *cpu = TRICORE_CPU(cs);
>      CPUTriCoreState *env = &cpu->env;

Completing the patch:

       hwaddr physical;
       int prot;
       int access_type;
       int ret = 0;

       rw &= 1;

       ^^^^^^^

This became a bit fragile... Having:

typedef enum MMUAccessType {
    MMU_DATA_LOAD  = 0,
    MMU_DATA_STORE = 1,
    MMU_INST_FETCH = 2
} MMUAccessType;

I see this enum has fixed value, but still...

Maybe we could have an helper to explicit the 'rw &= 1' magic?

>      access_type = ACCESS_INT;
>      ret = get_physical_address(env, &physical, &prot,
>                                 address, rw, access_type);
> -    qemu_log_mask(CPU_LOG_MMU, "%s address=" TARGET_FMT_lx " ret %d physical 
> " TARGET_FMT_plx
> -                  " prot %d\n", __func__, address, ret, physical, prot);
> +
> +    qemu_log_mask(CPU_LOG_MMU, "%s address=" TARGET_FMT_lx " ret %d physical 
> "
> +                  TARGET_FMT_plx " prot %d\n",
> +                  __func__, (target_ulong)address, ret, physical, prot);
>  
>      if (ret == TLBRET_MATCH) {
>          tlb_set_page(cs, address & TARGET_PAGE_MASK,
>                       physical & TARGET_PAGE_MASK, prot | PAGE_EXEC,
>                       mmu_idx, TARGET_PAGE_SIZE);
> -        ret = 0;
> -    } else if (ret < 0) {
> +        return true;
> +    } else {
> +        assert(ret < 0);
> +        if (probe) {
> +            return false;
> +        }
>          raise_mmu_exception(env, address, rw, ret);
> -        ret = 1;
> +        cpu_loop_exit_restore(cs, retaddr);
>      }
> +}