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

[Claudio Fontana]

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


> 
>>
>> and this concludes our experiment here?
>>
>> Thanks,
>>
>> Claudio
>>
>