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

[Claudio Fontana]

On 12/12/20 11:00 AM, Claudio Fontana wrote:
> On 12/11/20 9:02 PM, Eduardo Habkost wrote:
>> On Fri, Dec 11, 2020 at 07:51:54PM +0100, Claudio Fontana wrote:
>>> On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote:
>>>> On 12/11/20 7:22 PM, Richard Henderson wrote:
>>>>> On 12/11/20 12:15 PM, Claudio Fontana wrote:
>>>>>> Should I return this file to the original state (without the extra 
>>>>>> #includes that pretend it to be a standalone header file,
>>>>>> and call it
>>>>>>
>>>>>> tcg-cpu-ops.h.inc
>>>>>>
>>>>>> ?
>>>>>
>>>>> If this header can work with qemu/typedefs.h, then no, because the 
>>>>> circularity
>>>>> has been resolved.  Otherwise, yes.
>>>>
>>>> My editor got confused with TranslationBlock, which is why I asked
>>>> to include its declaration.
>>>>
>>>> Easier to forward-declare TranslationBlock in qemu/typedefs.h?
>>>>
>>>> Regards,
>>>>
>>>> Phil.
>>>>
>>>
>>> Hello Philippe,
>>>
>>> ok you propose to move the existing fwd declaration of TranslationBlock 
>>> from cpu.h to qemu/typedefs.h .
>>
>> It seems simpler to just add a
>>
>>     typedef struct TranslationBlock TranslationBlock;
>>
>> line to tcg-cpu-ops.h.
>>
>> Or, an even simpler solution: just use `struct TranslationBlock`
>> instead of `TranslationBlock` in the declarations being moved to
>> tcg-cpu-ops.h.
>>
>> We don't need to move declarations to typedefs.h anymore, because
>> now the compilers we support don't warn about typedef
>> redefinitions:
>> https://lore.kernel.org/qemu-devel/20200914134636.GZ1618070@habkost.net/
>>
>>
>>>
>>> And what about #include "exec/memattrs.h"?
>>>
>>> I assume you propose to put struct MemTxAttrs there as a fwd declaration 
>>> too,
>>
>> This can't be done, because MemTxAttrs can't be an incomplete
>> type in the code you are moving (the methods get a MemTxAttrs
>> value, not a pointer).
> 
> 
> 
> I'm confused now on what we are trying to do: if we want the file to be a 
> "proper header" or just a TCG-ops-only convenience split of cpu.h.
> 
> I thought that we were only solving a highlighting issue in some editor 
> (Philippe),
> and I wonder if these changes in qemu/typedef.h help with that?
> 
> I tried adding both to qemu/typedef.h, and since cpu.h is the only user of 
> the file, and it already includes memattrs.h, everything is fine.
> 
> But here maybe you are proposing to make it a regular header, and include 
> this instead of just hw/core/cpu.h in the targets?
> 
> I am thinking whether it is the case to scrap this whole mess, make TCGCPUOps 
> a pointer in CPUClass, and in the targets say for example:
> 
> #include "tcg-cpu-ops.h"
> 
> ...
> 
> +static struct TCGCPUOps cris_tcg_ops = {
> +    .initialize = cris_initialize_tcg,
> +};
> +
>  static void cris_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -284,7 +292,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->gdb_stop_before_watchpoint = true;
>  
>      cc->disas_set_info = cris_disas_set_info;
> -    cc->tcg_ops.initialize = cris_initialize_tcg;
> +    cc->tcg_ops = &cris_tcg_ops;
>  }
> 
> 
> What do you all think of this?
> 
> Thanks,
> 
> Claudio

Not sure it solves all problems: the MMUAccessType is still a cpu.h enum, so we 
are back to the circular dependency.
Will try the .inc in the next spin, and I hope that the discussion can go on 
from there, with Eduardo, Philippe and Richard laying out more clearly what 
your requirements are.

Thanks,

Claudio