Re: [Qemu-devel] [PATCH 2/3] target/mips: Switch to do_transaction_failed() hook

[Philippe Mathieu-Daudé]

Cc'ing James Hogan.

On 8/2/19 6:04 PM, Peter Maydell wrote:
> Switch the MIPS target from the old unassigned_access hook to the new
> do_transaction_failed hook.
> 
> Unlike the old hook, do_transaction_failed is only ever called from
> the TCG memory access paths, so there is no need for the "ignore this
> if we're using KVM" hack that we were previously using to work around
> the way unassigned_access was called for all kinds of memory accesses
> to unassigned physical addresses.
> 
> The MIPS target does not ever do direct memory reads by physical
> address (via either ldl_phys etc or address_space_ldl etc), so the
> only memory accesses this affects are the 'normal' guest loads and
> stores, which will be handled by the new hook; their behaviour is
> unchanged.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target/mips/internal.h  |  8 +++++---
>  target/mips/cpu.c       |  2 +-
>  target/mips/op_helper.c | 24 ++++++++----------------
>  3 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/target/mips/internal.h b/target/mips/internal.h
> index b2b41a51ab4..26533bb937d 100644
> --- a/target/mips/internal.h
> +++ b/target/mips/internal.h
> @@ -138,9 +138,11 @@ void r4k_helper_tlbinv(CPUMIPSState *env);
>  void r4k_helper_tlbinvf(CPUMIPSState *env);
>  void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra);
>  
> -void mips_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
> -                                bool is_write, bool is_exec, int unused,
> -                                unsigned size);
> +void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                    vaddr addr, unsigned size,
> +                                    MMUAccessType access_type,
> +                                    int mmu_idx, MemTxAttrs attrs,
> +                                    MemTxResult response, uintptr_t retaddr);
>  hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address,
>                                    int rw);
>  #endif
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 39eafafc5cd..a79badcb1a6 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -197,7 +197,7 @@ static void mips_cpu_class_init(ObjectClass *c, void 
> *data)
>      cc->gdb_read_register = mips_cpu_gdb_read_register;
>      cc->gdb_write_register = mips_cpu_gdb_write_register;
>  #ifndef CONFIG_USER_ONLY
> -    cc->do_unassigned_access = mips_cpu_unassigned_access;
> +    cc->do_transaction_failed = mips_cpu_do_transaction_failed;
>      cc->do_unaligned_access = mips_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = mips_cpu_get_phys_page_debug;
>      cc->vmsd = &vmstate_mips_cpu;
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 9e2e02f8586..65ff9f5b935 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -2666,27 +2666,19 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr 
> addr,
>      do_raise_exception_err(env, excp, error_code, retaddr);
>  }
>  
> -void mips_cpu_unassigned_access(CPUState *cs, hwaddr addr,
> -                                bool is_write, bool is_exec, int unused,
> -                                unsigned size)
> +void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                    vaddr addr, unsigned size,
> +                                    MMUAccessType access_type,
> +                                    int mmu_idx, MemTxAttrs attrs,
> +                                    MemTxResult response, uintptr_t retaddr)
>  {
>      MIPSCPU *cpu = MIPS_CPU(cs);
>      CPUMIPSState *env = &cpu->env;
>  
> -    /*
> -     * Raising an exception with KVM enabled will crash because it won't be 
> from
> -     * the main execution loop so the longjmp won't have a matching setjmp.
> -     * Until we can trigger a bus error exception through KVM lets just 
> ignore
> -     * the access.
> -     */
> -    if (kvm_enabled()) {
> -        return;

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> -    }
> -
> -    if (is_exec) {
> -        raise_exception(env, EXCP_IBE);
> +    if (access_type == MMU_INST_FETCH) {
> +        do_raise_exception(env, EXCP_IBE, retaddr);
>      } else {
> -        raise_exception(env, EXCP_DBE);
> +        do_raise_exception(env, EXCP_DBE, retaddr);
>      }
>  }
>  #endif /* !CONFIG_USER_ONLY */
>