Re: [PATCH v11 18/25] cpu: Move synchronize_from_tb() to tcg_ops

[Richard Henderson]

On 12/11/20 11:10 AM, Claudio Fontana wrote:
> On 12/11/20 6:05 PM, Richard Henderson wrote:
>> On 12/11/20 2:31 AM, Claudio Fontana wrote:
>>> From: Eduardo Habkost <ehabkost@redhat.com>
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> [claudio: wrapped in CONFIG_TCG]
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  include/hw/core/cpu.h         |  8 --------
>>>  include/hw/core/tcg-cpu-ops.h | 12 ++++++++++++
>>>  accel/tcg/cpu-exec.c          |  4 ++--
>>>  target/arm/cpu.c              |  4 +++-
>>>  target/avr/cpu.c              |  2 +-
>>>  target/hppa/cpu.c             |  2 +-
>>>  target/i386/tcg/tcg-cpu.c     |  2 +-
>>>  target/microblaze/cpu.c       |  2 +-
>>>  target/mips/cpu.c             |  4 +++-
>>>  target/riscv/cpu.c            |  2 +-
>>>  target/rx/cpu.c               |  2 +-
>>>  target/sh4/cpu.c              |  2 +-
>>>  target/sparc/cpu.c            |  2 +-
>>>  target/tricore/cpu.c          |  2 +-
>>>  14 files changed, 29 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index ea648d52ad..83007d262c 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -110,13 +110,6 @@ struct TranslationBlock;
>>>   *       If the target behaviour here is anything other than "set
>>>   *       the PC register to the value passed in" then the target must
>>>   *       also implement the synchronize_from_tb hook.
>>> - * @synchronize_from_tb: Callback for synchronizing 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 always includes at least the program counter; some targets
>>> - *       will need to do more. If this hook is not implemented then the
>>> - *       default is to call @set_pc(tb->pc).
>>>   * @tlb_fill: Callback for handling 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
>>> @@ -193,7 +186,6 @@ struct CPUClass {
>>>      void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
>>>                                 Error **errp);
>>>      void (*set_pc)(CPUState *cpu, vaddr value);
>>> -    void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock 
>>> *tb);
>>>      bool (*tlb_fill)(CPUState *cpu, vaddr address, int size,
>>>                       MMUAccessType access_type, int mmu_idx,
>>>                       bool probe, uintptr_t retaddr);
>>> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
>>> index 4475ef0996..e1d50b3c8b 100644
>>> --- a/include/hw/core/tcg-cpu-ops.h
>>> +++ b/include/hw/core/tcg-cpu-ops.h
>>> @@ -10,6 +10,8 @@
>>>  #ifndef TCG_CPU_OPS_H
>>>  #define TCG_CPU_OPS_H
>>>  
>>> +#include "hw/core/cpu.h"
>>
>> This include is circular.
> 
> Yes, it's protected though, it was asked that way.

Well, in my strong opinion, someone asked incorrectly.  It's "harmless" because
of the protection ifdefs, but it's Wrong because it has the potential to hide 
bugs.

What is it that you thought you needed from core/cpu.h anyway?

>> Are you sure that splitting out hw/core/tcg-cpu-ops.h from hw/core/cpu.h in
>> patch 15 is even useful?
> 
> it avoids a huge #ifdef CONFIG_TCG

So?  The question should be: is it useful on its own, and I think the answer to
that is clearly not.  Thus it should not pretend to be a standalone header file.


r~