Re: [PULL 38/46] cpu: move cc->transaction_failed to tcg_ops

[Philippe Mathieu-Daudé]

On 2/5/21 11:56 PM, Richard Henderson wrote:
> From: Claudio Fontana <cfontana@suse.de>
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> [claudio: wrap target code around CONFIG_TCG and !CONFIG_USER_ONLY]
> 
> avoiding its use in headers used by common_ss code (should be poisoned).
> 
> Note: need to be careful with the use of CONFIG_USER_ONLY,
> Message-Id: <20210204163931.7358-11-cfontana@suse.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/hw/core/cpu.h     | 28 +++++++++++++---------------
>  hw/mips/jazz.c            |  9 +++++++--
>  target/alpha/cpu.c        |  2 +-
>  target/arm/cpu.c          |  4 ++--
>  target/m68k/cpu.c         |  2 +-
>  target/microblaze/cpu.c   |  2 +-
>  target/mips/cpu.c         |  4 +++-
>  target/riscv/cpu.c        |  2 +-
>  target/riscv/cpu_helper.c |  2 +-
>  target/sparc/cpu.c        |  2 +-
>  target/xtensa/cpu.c       |  2 +-
>  target/xtensa/helper.c    |  4 ++--
>  12 files changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 60cf20bf05..41ce1daefc 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -122,6 +122,14 @@ typedef struct TcgCpuOperations {
>      /** @debug_excp_handler: Callback for handling debug exceptions */
>      void (*debug_excp_handler)(CPUState *cpu);
>  
> +    /**
> +     * @do_transaction_failed: Callback for handling failed memory 
> transactions
> +     * (ie bus faults or external aborts; not MMU faults)
> +     */
> +    void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
> +                                  unsigned size, MMUAccessType access_type,
> +                                  int mmu_idx, MemTxAttrs attrs,
> +                                  MemTxResult response, uintptr_t retaddr);
>  } TcgCpuOperations;
>  
>  /**
> @@ -133,8 +141,6 @@ typedef struct TcgCpuOperations {
>   * @has_work: Callback for checking if there is work to do.
>   * @do_unaligned_access: Callback for unaligned access handling, if
>   * the target defines #TARGET_ALIGNED_ONLY.
> - * @do_transaction_failed: Callback for handling failed memory transactions
> - * (ie bus faults or external aborts; not MMU faults)
>   * @virtio_is_big_endian: Callback to return %true if a CPU which supports
>   * runtime configurable endianness is currently big-endian. Non-configurable
>   * CPUs can use the default implementation of this method. This method should
> @@ -203,10 +209,6 @@ struct CPUClass {
>      void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                  MMUAccessType access_type,
>                                  int mmu_idx, uintptr_t retaddr);
> -    void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
> -                                  unsigned size, MMUAccessType access_type,
> -                                  int mmu_idx, MemTxAttrs attrs,
> -                                  MemTxResult response, uintptr_t retaddr);
>      bool (*virtio_is_big_endian)(CPUState *cpu);
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
> @@ -879,9 +881,6 @@ CPUState *cpu_by_arch_id(int64_t id);
>  
>  void cpu_interrupt(CPUState *cpu, int mask);
>  
> -#ifdef NEED_CPU_H
> -
> -#ifdef CONFIG_SOFTMMU
>  static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
>                                          MMUAccessType access_type,
>                                          int mmu_idx, uintptr_t retaddr)
> @@ -900,14 +899,13 @@ static inline void cpu_transaction_failed(CPUState 
> *cpu, hwaddr physaddr,
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> -    if (!cpu->ignore_memory_transaction_failures && 
> cc->do_transaction_failed) {
> -        cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
> -                                  mmu_idx, attrs, response, retaddr);
> +    if (!cpu->ignore_memory_transaction_failures &&
> +        cc->tcg_ops.do_transaction_failed) {
> +        cc->tcg_ops.do_transaction_failed(cpu, physaddr, addr, size,
> +                                          access_type, mmu_idx, attrs,
> +                                          response, retaddr);
>      }
>  }
> -#endif
> -
> -#endif /* NEED_CPU_H */
>  
>  /**
>   * cpu_set_pc:
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index f9442731dd..46c71a0ac8 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
> @@ -116,6 +116,8 @@ static const MemoryRegionOps dma_dummy_ops = {
>  #define MAGNUM_BIOS_SIZE_MAX 0x7e000
>  #define MAGNUM_BIOS_SIZE                                                     
>   \
>          (BIOS_SIZE < MAGNUM_BIOS_SIZE_MAX ? BIOS_SIZE : MAGNUM_BIOS_SIZE_MAX)
> +
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>  static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr,
>                                            vaddr addr, unsigned size,
>                                            MMUAccessType access_type,
> @@ -137,6 +139,7 @@ static void mips_jazz_do_transaction_failed(CPUState *cs, 
> hwaddr physaddr,
>      (*real_do_transaction_failed)(cs, physaddr, addr, size, access_type,
>                                    mmu_idx, attrs, response, retaddr);
>  }
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>  
>  static void mips_jazz_init(MachineState *machine,
>                             enum jazz_model_e jazz_model)
> @@ -205,8 +208,10 @@ static void mips_jazz_init(MachineState *machine,
>       * memory region that catches all memory accesses, as we do on Malta.
>       */
>      cc = CPU_GET_CLASS(cpu);
> -    real_do_transaction_failed = cc->do_transaction_failed;
> -    cc->do_transaction_failed = mips_jazz_do_transaction_failed;
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> +    real_do_transaction_failed = cc->tcg_ops.do_transaction_failed;
> +    cc->tcg_ops.do_transaction_failed = mips_jazz_do_transaction_failed;
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */

Why CONFIG_USER_ONLY isn't poisoned under hw/ ?