qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 07/13] softmmu: make do_unaligned_access a metho


From: Andreas Färber
Subject: Re: [Qemu-devel] [PATCH 07/13] softmmu: make do_unaligned_access a method of CPU
Date: Wed, 28 May 2014 19:20:21 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

Am 23.05.2014 17:20, schrieb Paolo Bonzini:
> We will reference it from more files in the next patch.  To avoid
> ruining the small steps we're making towards multi-target, make
> it a method of CPU rather than just a global.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  include/exec/softmmu_template.h |   30 ++++++++++++++++++------------
>  include/qom/cpu.h               |   15 +++++++++++++--
>  target-alpha/cpu-qom.h          |    2 ++
>  target-alpha/cpu.c              |    1 +
>  target-alpha/mem_helper.c       |    8 ++++----
>  target-mips/cpu-qom.h           |    2 ++
>  target-mips/cpu.c               |    1 +
>  target-mips/op_helper.c         |   11 +++++------
>  target-sparc/cpu-qom.h          |    3 +++
>  target-sparc/cpu.c              |    1 +
>  target-sparc/ldst_helper.c      |   13 ++++++-------
>  target-xtensa/cpu-qom.h         |    2 ++
>  target-xtensa/cpu.c             |    1 +
>  target-xtensa/op_helper.c       |   10 ++++------
>  14 files changed, 63 insertions(+), 37 deletions(-)
> 
> diff --git a/include/exec/softmmu_template.h b/include/exec/softmmu_template.h
> index 73ed7cf..12ead5a 100644
> --- a/include/exec/softmmu_template.h
> +++ b/include/exec/softmmu_template.h
> @@ -155,7 +155,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, 
> target_ulong addr, int mmu_idx,
>           != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
>  #ifdef ALIGNED_ONLY
>          if ((addr & (DATA_SIZE - 1)) != 0) {
> -            do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, 
> retaddr);
> +            cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
> +                                 mmu_idx, retaddr);
>          }
>  #endif
>          tlb_fill(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
> @@ -186,7 +187,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, 
> target_ulong addr, int mmu_idx,
>          unsigned shift;
>      do_unaligned_access:
>  #ifdef ALIGNED_ONLY
> -        do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
> +                             mmu_idx, retaddr);
>  #endif
>          addr1 = addr & ~(DATA_SIZE - 1);
>          addr2 = addr1 + DATA_SIZE;
> @@ -204,7 +206,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, 
> target_ulong addr, int mmu_idx,
>      /* Handle aligned access or unaligned access in the same page.  */
>  #ifdef ALIGNED_ONLY
>      if ((addr & (DATA_SIZE - 1)) != 0) {
> -        do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
> +                             mmu_idx, retaddr);
>      }
>  #endif
>  
> @@ -237,7 +240,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, 
> target_ulong addr, int mmu_idx,
>           != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
>  #ifdef ALIGNED_ONLY
>          if ((addr & (DATA_SIZE - 1)) != 0) {
> -            do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, 
> retaddr);
> +            cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
> +                                 mmu_idx, retaddr);
>          }
>  #endif
>          tlb_fill(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
> @@ -268,7 +272,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, 
> target_ulong addr, int mmu_idx,
>          unsigned shift;
>      do_unaligned_access:
>  #ifdef ALIGNED_ONLY
> -        do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
> +                             mmu_idx, retaddr);
>  #endif
>          addr1 = addr & ~(DATA_SIZE - 1);
>          addr2 = addr1 + DATA_SIZE;
> @@ -286,7 +291,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, 
> target_ulong addr, int mmu_idx,
>      /* Handle aligned access or unaligned access in the same page.  */
>  #ifdef ALIGNED_ONLY
>      if ((addr & (DATA_SIZE - 1)) != 0) {
> -        do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
> +                             mmu_idx, retaddr);
>      }
>  #endif
>  
> @@ -357,7 +363,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
> addr, DATA_TYPE val,
>          != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
>  #ifdef ALIGNED_ONLY
>          if ((addr & (DATA_SIZE - 1)) != 0) {
> -            do_unaligned_access(env, addr, 1, mmu_idx, retaddr);
> +            cpu_unaligned_access(ENV_GET_CPU(env), addr, 1, mmu_idx, 
> retaddr);
>          }
>  #endif
>          tlb_fill(ENV_GET_CPU(env), addr, 1, mmu_idx, retaddr);
> @@ -386,7 +392,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
> addr, DATA_TYPE val,
>          int i;
>      do_unaligned_access:
>  #ifdef ALIGNED_ONLY
> -        do_unaligned_access(env, addr, 1, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, 1, mmu_idx, retaddr);
>  #endif
>          /* XXX: not efficient, but simple */
>          /* Note: relies on the fact that tlb_fill() does not remove the
> @@ -405,7 +411,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
> addr, DATA_TYPE val,
>      /* Handle aligned access or unaligned access in the same page.  */
>  #ifdef ALIGNED_ONLY
>      if ((addr & (DATA_SIZE - 1)) != 0) {
> -        do_unaligned_access(env, addr, 1, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, 1, mmu_idx, retaddr);
>      }
>  #endif
>  
> @@ -433,7 +439,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
> addr, DATA_TYPE val,
>          != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
>  #ifdef ALIGNED_ONLY
>          if ((addr & (DATA_SIZE - 1)) != 0) {
> -            do_unaligned_access(env, addr, 1, mmu_idx, retaddr);
> +            cpu_unaligned_access(ENV_GET_CPU(env), addr, 1, mmu_idx, 
> retaddr);
>          }
>  #endif
>          tlb_fill(ENV_GET_CPU(env), addr, 1, mmu_idx, retaddr);
> @@ -462,7 +468,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
> addr, DATA_TYPE val,
>          int i;
>      do_unaligned_access:
>  #ifdef ALIGNED_ONLY
> -        do_unaligned_access(env, addr, 1, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, 1, mmu_idx, retaddr);
>  #endif
>          /* XXX: not efficient, but simple */
>          /* Note: relies on the fact that tlb_fill() does not remove the
> @@ -481,7 +487,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
> addr, DATA_TYPE val,
>      /* Handle aligned access or unaligned access in the same page.  */
>  #ifdef ALIGNED_ONLY
>      if ((addr & (DATA_SIZE - 1)) != 0) {
> -        do_unaligned_access(env, addr, 1, mmu_idx, retaddr);
> +        cpu_unaligned_access(ENV_GET_CPU(env), addr, 1, mmu_idx, retaddr);
>      }
>  #endif
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index df977c8..ee1fca8 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -80,6 +80,8 @@ struct TranslationBlock;
>   * @has_work: Callback for checking if there is work to do.
>   * @do_interrupt: Callback for interrupt handling.
>   * @do_unassigned_access: Callback for unassigned access handling.
> + * @do_unaligned_access: Callback for unaligned access handling, if
> + * the target defines #ALIGNED_ONLY.
>   * @memory_rw_debug: Callback for GDB memory access.
>   * @dump_state: Callback for dumping state.
>   * @dump_statistics: Callback for dumping statistics.
> @@ -112,6 +114,8 @@ typedef struct CPUClass {
>      bool (*has_work)(CPUState *cpu);
>      void (*do_interrupt)(CPUState *cpu);
>      CPUUnassignedAccess do_unassigned_access;
> +    void (*do_unaligned_access)(CPUState *cs, vaddr addr,

Minor nit: I'd prefer *cpu for consistency.

> +                                int is_write, int is_user, uintptr_t 
> retaddr);
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
> @@ -544,8 +548,7 @@ void cpu_interrupt(CPUState *cpu, int mask);
>  
>  #endif /* USER_ONLY */
>  
> -#ifndef CONFIG_USER_ONLY
> -
> +#ifdef CONFIG_SOFTMMU
>  static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
>                                           bool is_write, bool is_exec,
>                                           int opaque, unsigned size)
> @@ -557,6 +560,14 @@ static inline void cpu_unassigned_access(CPUState *cpu, 
> hwaddr addr,
>      }
>  }
>  
> +static inline void cpu_unaligned_access(CPUState *cs, vaddr addr,
> +                                        int is_write, int is_user,
> +                                        uintptr_t retaddr)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +
> +    return cc->do_unaligned_access(cs, addr, is_write, is_user, retaddr);
> +}
>  #endif
>  
>  /**
> diff --git a/target-alpha/cpu-qom.h b/target-alpha/cpu-qom.h
> index 198f1b1..fd97a32 100644
> --- a/target-alpha/cpu-qom.h
> +++ b/target-alpha/cpu-qom.h
> @@ -84,5 +84,7 @@ void alpha_cpu_dump_state(CPUState *cs, FILE *f, 
> fprintf_function cpu_fprintf,
>  hwaddr alpha_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int alpha_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int alpha_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> +void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,

Dito here. Only really needed in the implementation where both types are
used.

> +                                   int is_write, int is_user, uintptr_t 
> retaddr);
>  
>  #endif
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index 7ec46b9..2491f0a 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -292,6 +292,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->handle_mmu_fault = alpha_cpu_handle_mmu_fault;
>  #else
>      cc->do_unassigned_access = alpha_cpu_unassigned_access;
> +    cc->do_unaligned_access = alpha_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = alpha_cpu_get_phys_page_debug;
>      dc->vmsd = &vmstate_alpha_cpu;
>  #endif
> diff --git a/target-alpha/mem_helper.c b/target-alpha/mem_helper.c
> index 5964bdc..de03c9f 100644
> --- a/target-alpha/mem_helper.c
> +++ b/target-alpha/mem_helper.c
> @@ -96,11 +96,11 @@ uint64_t helper_stq_c_phys(CPUAlphaState *env, uint64_t 
> p, uint64_t v)
>      return ret;
>  }
>  
> -static void do_unaligned_access(CPUAlphaState *env, target_ulong addr,
> -                                int is_write, int is_user, uintptr_t retaddr)
> +void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> +                                   int is_write, int is_user, uintptr_t 
> retaddr)
>  {
> -    AlphaCPU *cpu = alpha_env_get_cpu(env);
> -    CPUState *cs = CPU(cpu);
> +    AlphaCPU *cpu = ALPHA_CPU(cs);
> +    CPUAlphaState *env = &cpu->env;
>      uint64_t pc;
>      uint32_t insn;
>  
> diff --git a/target-mips/cpu-qom.h b/target-mips/cpu-qom.h
> index 8877f81..858b6d4 100644
> --- a/target-mips/cpu-qom.h
> +++ b/target-mips/cpu-qom.h
> @@ -80,5 +80,7 @@ void mips_cpu_dump_state(CPUState *cpu, FILE *f, 
> fprintf_function cpu_fprintf,
>  hwaddr mips_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int mips_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int mips_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> +void mips_cpu_do_unaligned_access(CPUState *env, vaddr addr,

Copy&paste ;) *cpu, not *env

Also should this have QEMU_NORETURN? For sparc you kept it around.

> +                                  int is_write, int is_user, uintptr_t 
> retaddr);
>  
>  #endif
> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
> index ae37ae2..dd954fc 100644
> --- a/target-mips/cpu.c
> +++ b/target-mips/cpu.c
> @@ -137,6 +137,7 @@ static void mips_cpu_class_init(ObjectClass *c, void 
> *data)
>      cc->handle_mmu_fault = mips_cpu_handle_mmu_fault;
>  #else
>      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;
>  #endif
>  
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 4edec6c..8226461 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -2128,10 +2128,6 @@ void helper_wait(CPUMIPSState *env)
>  
>  #if !defined(CONFIG_USER_ONLY)
>  
> -static void QEMU_NORETURN do_unaligned_access(CPUMIPSState *env,
> -                                              target_ulong addr, int 
> is_write,
> -                                              int is_user, uintptr_t 
> retaddr);
> -
>  #define MMUSUFFIX _mmu
>  #define ALIGNED_ONLY
>  
> @@ -2147,9 +2143,12 @@ static void QEMU_NORETURN 
> do_unaligned_access(CPUMIPSState *env,
>  #define SHIFT 3
>  #include "exec/softmmu_template.h"
>  
> -static void do_unaligned_access(CPUMIPSState *env, target_ulong addr,
> -                                int is_write, int is_user, uintptr_t retaddr)
> +void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> +                                  int is_write, int is_user, uintptr_t 
> retaddr)
>  {
> +    MIPSCPU *cpu = MIPS_CPU(cs);
> +    CPUMIPSState *env = &cpu->env;
> +
>      env->CP0_BadVAddr = addr;
>      do_raise_exception(env, (is_write == 1) ? EXCP_AdES : EXCP_AdEL, 
> retaddr);
>  }
> diff --git a/target-sparc/cpu-qom.h b/target-sparc/cpu-qom.h
> index 8e3e0de..6a76e4e 100644
> --- a/target-sparc/cpu-qom.h
> +++ b/target-sparc/cpu-qom.h
> @@ -81,5 +81,8 @@ void sparc_cpu_dump_state(CPUState *cpu, FILE *f,
>  hwaddr sparc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int sparc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int sparc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> +void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cs,

*cpu?

> +                                                 vaddr addr, int is_write,
> +                                                 int is_user, uintptr_t 
> retaddr);
>  
>  #endif
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index d9f37e9..3a0ee50 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -825,6 +825,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->handle_mmu_fault = sparc_cpu_handle_mmu_fault;
>  #else
>      cc->do_unassigned_access = sparc_cpu_unassigned_access;
> +    cc->do_unaligned_access = sparc_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = sparc_cpu_get_phys_page_debug;
>  #endif
>  
> diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
> index ec14802..687845f 100644
> --- a/target-sparc/ldst_helper.c
> +++ b/target-sparc/ldst_helper.c
> @@ -65,9 +65,6 @@
>  #define QT1 (env->qt1)
>  
>  #if !defined(CONFIG_USER_ONLY)
> -static void QEMU_NORETURN do_unaligned_access(CPUSPARCState *env,
> -                                              target_ulong addr, int 
> is_write,
> -                                              int is_user, uintptr_t 
> retaddr);
>  #include "exec/softmmu_exec.h"
>  #define MMUSUFFIX _mmu
>  #define ALIGNED_ONLY
> @@ -2425,11 +2422,13 @@ void sparc_cpu_unassigned_access(CPUState *cs, hwaddr 
> addr,
>  #endif
>  
>  #if !defined(CONFIG_USER_ONLY)
> -static void QEMU_NORETURN do_unaligned_access(CPUSPARCState *env,
> -                                              target_ulong addr, int 
> is_write,
> -                                              int is_user, uintptr_t retaddr)
> +void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cs,
> +                                                 vaddr addr, int is_write,
> +                                                 int is_user, uintptr_t 
> retaddr)
>  {
> -    SPARCCPU *cpu = sparc_env_get_cpu(env);
> +    SPARCCPU *cpu = SPARC_CPU(cs);
> +    CPUSPARCState *env = &cpu->env;
> +
>  #ifdef DEBUG_UNALIGNED
>      printf("Unaligned access to 0x" TARGET_FMT_lx " from 0x" TARGET_FMT_lx
>             "\n", addr, env->pc);
> diff --git a/target-xtensa/cpu-qom.h b/target-xtensa/cpu-qom.h
> index c6cc2d9..6a7d52e 100644
> --- a/target-xtensa/cpu-qom.h
> +++ b/target-xtensa/cpu-qom.h
> @@ -89,5 +89,7 @@ void xtensa_cpu_dump_state(CPUState *cpu, FILE *f,
>  hwaddr xtensa_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int xtensa_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int xtensa_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> +void xtensa_cpu_do_unaligned_access(CPUState *cs,
> +       vaddr addr, int is_write, int is_user, uintptr_t retaddr);

I note this is differently line-broken than the others.

>  
>  #endif
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 6251f1c..9d8801b 100644
> --- a/target-xtensa/cpu.c
> +++ b/target-xtensa/cpu.c
> @@ -148,6 +148,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->gdb_read_register = xtensa_cpu_gdb_read_register;
>      cc->gdb_write_register = xtensa_cpu_gdb_write_register;
>  #ifndef CONFIG_USER_ONLY
> +    cc->do_unaligned_access = xtensa_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = xtensa_cpu_get_phys_page_debug;
>  #endif
>      dc->vmsd = &vmstate_xtensa_cpu;
> diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
> index b531019..29109e0 100644
> --- a/target-xtensa/op_helper.c
> +++ b/target-xtensa/op_helper.c
> @@ -31,9 +31,6 @@
>  #include "exec/softmmu_exec.h"
>  #include "exec/address-spaces.h"
>  
> -static void do_unaligned_access(CPUXtensaState *env,
> -        target_ulong addr, int is_write, int is_user, uintptr_t retaddr);
> -
>  #define ALIGNED_ONLY
>  #define MMUSUFFIX _mmu
>  
> @@ -49,10 +46,11 @@ static void do_unaligned_access(CPUXtensaState *env,
>  #define SHIFT 3
>  #include "exec/softmmu_template.h"
>  
> -static void do_unaligned_access(CPUXtensaState *env,
> -        target_ulong addr, int is_write, int is_user, uintptr_t retaddr)
> +void xtensa_cpu_do_unaligned_access(CPUState *cs,
> +        vaddr addr, int is_write, int is_user, uintptr_t retaddr)
>  {
> -    XtensaCPU *cpu = xtensa_env_get_cpu(env);
> +    XtensaCPU *cpu = XTENSA_CPU(cs);
> +    CPUXtensaState *env = &cpu->env;
>  
>      if (xtensa_option_enabled(env->config, 
> XTENSA_OPTION_UNALIGNED_EXCEPTION) &&
>              !xtensa_option_enabled(env->config, XTENSA_OPTION_HW_ALIGNMENT)) 
> {

That said, can silently be fixed up without big respins,

Reviewed-by: Andreas Färber <address@hidden>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg



reply via email to

[Prev in Thread] Current Thread [Next in Thread]