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

[Claudio Fontana <cfontana@suse.de>]

On 2/23/21 10:43 PM, Philippe Mathieu-Daudé wrote:
> 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/ ?
> 

As I can see, hw/ contains a wide variety of objects, which go into hw_arch, specific, softmmu or common.

There does not seem to be a common way to handle objects in hw/ .

In the case of hw/mips, it goes to hw_arch in meson.build, so it sees config target.

Other users of CONFIG_USER_ONLY in hw/ are arm semihosting:

semihosting/arm-compat-semi.c which goes to specific_ss

and hw/cpu.c, which uses CONFIG_USER_ONLY but does not see it, since it is a common_ss module.

So the uses of CONFIG_USER_ONLY in hw/cpu.c are wrong for sure, or at least until hw/cpu.c goes to common_ss.
It does not hurt because the tests are in the negative, and those sysemu-only / softmmu-only symbols are actually always visible, including for CONFIG_USER_ONLY.

Ciao,

Claudio