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

["Alex Bennée" <alex.bennee@linaro.org>]

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.

>  
> -    /**
> -     * @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.

<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:

--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;
 };
 
 /*
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).

<snip>

These minor trivialities aside:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée