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

["Alex Bennée" <alex.bennee@linaro.org>]

Aleksandar Markovic <aleksandar.m.mail@gmail.com> writes:

> On May 10, 2019 8:57 PM, "Richard Henderson" <richard.henderson@linaro.org>
> wrote:
>>
>
> Please change the title to 'target/mips: Switch to using
> mips_cpu_tlb_fill()', or something along that line.

It does seem a little redundant as "target/mips:" already marks it as a
mips specific change and viewing the log you can see a series of
architectures being converted to a new API.

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

ok

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

Surely the core of the change is explained in the main patches that
introduce the new API? I think it would be redundant to repeat that for
every individual architecture touched. It's a shame it's hard to
explicitly reference a patch in the same series as the commit hashes are
not yet permanent. At least when we fix things referring to the short
hash of the original commit is fairly easy.

Generally for an architecture conversion I want to know what might be
different from other architectures converted. If it is a broadly
mechanical change it doesn't need to be too detailed.

>
> 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 <amarkovic@wavecomp.com>
>> Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  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
>>
>>


--
Alex Bennée