On 2/3/21 2:23 PM, Alex Bennée wrote:
>
> 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.
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.ben...@linaro.org>
>
Thanks Alex,
Claudio
