Re: [PATCH v15 15/23] cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass

[Claudio Fontana <cfontana@suse.de>]

On 2/3/21 2:23 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> we cannot in principle make the TCG Operations field definitions
>> conditional on CONFIG_TCG in code that is included by both common_ss
>> and specific_ss modules.
>>
>> Therefore, what we can do safely to restrict the TCG fields to TCG-only
>> builds, is to move all tcg cpu operations into a separate header file,
>> which is only included by TCG, target-specific code.
>>
>> This leaves just a NULL pointer in the cpu.h for the non-TCG builds.
>>
>> This also tidies up the code in all targets a bit, having all TCG cpu
>> operations neatly contained by a dedicated data struct.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
> <snip>
>>  
>> -/**
>> - * struct TcgCpuOperations: TCG operations specific to a CPU class
>> - */
>> -typedef struct TcgCpuOperations {
>> -    /**
>> -     * @initialize: Initalize TCG state
>> -     *
>> -     * Called when the first CPU is realized.
>> -     */
>> -    void (*initialize)(void);
>> -    /**
>> -     * @synchronize_from_tb: Synchronize state from a TCG #TranslationBlock
>> -     *
>> -     * This is called when we abandon execution of a TB before
>> -     * starting it, and must set all parts of the CPU state which
>> -     * the previous TB in the chain may not have updated. This
>> -     * will need to do more. If this hook is not implemented then
>> -     * the default is to call @set_pc(tb->pc).
>> -     */
>> -    void (*synchronize_from_tb)(CPUState *cpu,
>> -                                const struct TranslationBlock *tb);
>> -    /** @cpu_exec_enter: Callback for cpu_exec preparation */
>> -    void (*cpu_exec_enter)(CPUState *cpu);
>> -    /** @cpu_exec_exit: Callback for cpu_exec cleanup */
>> -    void (*cpu_exec_exit)(CPUState *cpu);
>> -    /** @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec */
>> -    bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
>> -    /** @do_interrupt: Callback for interrupt handling. */
>> -    void (*do_interrupt)(CPUState *cpu);
>> -    /**
>> -     * @tlb_fill: Handle a softmmu tlb miss or user-only address fault
>> -     *
>> -     * For system mode, if the access is valid, call tlb_set_page
>> -     * and return true; if the access is invalid, and probe is
>> -     * true, return false; otherwise raise an exception and do
>> -     * not return.  For user-only mode, always raise an exception
>> -     * and do not return.
>> -     */
>> -    bool (*tlb_fill)(CPUState *cpu, vaddr address, int size,
>> -                     MMUAccessType access_type, int mmu_idx,
>> -                     bool probe, uintptr_t retaddr);
>> -    /** @debug_excp_handler: Callback for handling debug exceptions */
>> -    void (*debug_excp_handler)(CPUState *cpu);
>> +/* see accel-cpu.h */
>> +struct AccelCPUClass;
> 
> This seems unrelated - wasn't AccelCPUClass already introduced. Or is
> this just catch up documentation.

Yep something to check, seems unnecessary.

> 
>>  
>> -    /**
>> -     * @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);
>> -    /**
>> -     * @do_unaligned_access: Callback for unaligned access handling
>> -     */
>> -    void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>> -                                MMUAccessType access_type,
>> -                                int mmu_idx, uintptr_t retaddr);
>> -    /**
>> -     * @adjust_watchpoint_address: hack for cpu_check_watchpoint used by ARM
>> -     */
>> -    vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
>> -
>> -    /**
>> -     * @debug_check_watchpoint: return true if the architectural
>> -     * watchpoint whose address has matched should really fire, used by ARM
>> -     */
>> -    bool (*debug_check_watchpoint)(CPUState *cpu, CPUWatchpoint *wp);
>> -
>> -} TcgCpuOperations;
>> +/* see tcg-cpu-ops.h */
>> +struct TCGCPUOps;
>>  
>>  /**
>>   * CPUClass:
>> @@ -256,7 +191,14 @@ struct CPUClass {
>>      int gdb_num_core_regs;
>>      bool gdb_stop_before_watchpoint;
>>  
>> -    TcgCpuOperations tcg_ops;
>> +    /*
>> +     * NB: this should be wrapped by CONFIG_TCG, but it is unsafe to do it here,
>> +     * as this header is included by both ss_specific and ss_common code,
>> +     * leading to potential differences in the data structure between modules.
>> +     * We could always keep it last, but it seems safer to just leave this
>> +     * pointer NULL for non-TCG.
>> +     */
>> +    struct TCGCPUOps *tcg_ops;
> 
> I suspect the editorial comment is better suited to the commit log
> rather than the comments. Maybe a simpler:
> 
>   As this header is included by both ss_specific and ss_common code we
>   cannot totally eliminate this field for non CONFIG_TCG builds although
>   the pointer will be NULL.
> 
> and move the justification to the commit comment.

Ok, I'd still keep also a comment with the code, as it's read more than commit logs, at the minimum a: "this is NULL when TCG code is not available".

> 
> <snip>
>>  
>> +#ifdef CONFIG_TCG
>> +/*
>> + * NB: cannot be const, as some elements are changed for specific
>> + * arm cpu classes.
>> + */
> 
> This comment seems wrong. I don't see arm_tcg_ops being changed after
> the fact. We have a separate arm_v7m_tcg_ops which we use instead.
> Indeed the following seemed to work:


You are right, the comment is obsolete.

This is a leftover comment from when I thought that there was no way to remove the last place where the tcg ops where changed in arm,
which was the part I removed here:

@@ -805,10 +808,6 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
 
-#ifdef CONFIG_TCG
-    cc->tcg_ops.cpu_exec_interrupt = arm_cpu_exec_interrupt;
-#endif /* CONFIG_TCG */
-


I then figured out that this code was completely unnecessary.


> 
> --8<---------------cut here---------------start------------->8---
> modified   include/hw/core/cpu.h
> @@ -199,7 +199,7 @@ struct CPUClass {
>       * We could always keep it last, but it seems safer to just leave this
>       * pointer NULL for non-TCG.
>       */
> -    struct TCGCPUOps *tcg_ops;
> +    const struct TCGCPUOps *tcg_ops;


Yes, I really wanted to do this.
Mips is the only blocker left that I can remember.


>  };
>  
>  /*
> modified   target/arm/cpu.c
> @@ -2248,7 +2248,7 @@ static gchar *arm_gdb_arch_name(CPUState *cs)
>   * NB: cannot be const, as some elements are changed for specific
>   * arm cpu classes.
>   */
> -static struct TCGCPUOps arm_tcg_ops = {
> +static const struct TCGCPUOps arm_tcg_ops = {
>      .initialize = arm_translate_init,
>      .synchronize_from_tb = arm_cpu_synchronize_from_tb,
>      .cpu_exec_interrupt = arm_cpu_exec_interrupt,
> --8<---------------cut here---------------end--------------->8---
> 
> This does later break MIPS jazz:
> 
> p/hw_mips_jazz.c.o -c ../../hw/mips/jazz.c
> ../../hw/mips/jazz.c: In function ‘mips_jazz_init’:
> ../../hw/mips/jazz.c:216:40: error: assignment of member ‘do_transaction_failed’ in read-only object
>      cc->tcg_ops->do_transaction_failed = mips_jazz_do_transaction_failed;
> 
> which...
> 
> <snip>
>>  
>> +#ifdef CONFIG_TCG
>> +#include "hw/core/tcg-cpu-ops.h"
>> +/*
>> + * NB: cannot be const, as some elements are changed for specific
>> + * mips hardware (see hw/mips/jazz.c).
>> + */
> 
> does have a valid comment. So guess keep it as static and just don't
> claim ARM hacks around with it or find a more elegant solution for the
> Jazz hack (I'm not sure there is one).

Yep, the ARM claim was true when I started looking at this, but now it's not anymore after the changes.

However, I haven't found a way to remove the mips jazz hack.

Maybe Philippe knows?


> 
> <snip>
> 
> These minor trivialities aside:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Thanks Alex,

Claudio