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

[Eduardo Habkost]

On Sat, Dec 12, 2020 at 11:00:03AM +0100, 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.

Personally, I don't see the point of creating a new header if
it's not a proper header.

> 
> 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?

Making tcg_ops a pointer may make a lot of sense, but (as
mentioned in my reply to v12) I'm worried by the scope of this
series growing too much.

I suggest doing this refactor in smaller steps, to let us focus
in a single issue at a time.  Instead of splitting the struct and
creating a new header file in a single patch, you can first
create the new struct in the same header, and worry about moving
it to a separate header later (in the same series, or in another
series).

-- 
Eduardo