Claudio Fontana <cfont...@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 <cfont...@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.ben...@linaro.org>
--
Alex Bennée
