Re: [Qemu-devel] [PATCH qom-next 57/59] cpu: Introduce mandatory tlb_flush callback

[Paul Brook]

> >> +void cpu_tlb_flush(CPUState *cpu, bool flush_global)
> >> +{
> >> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> >> +
> >> +    g_assert(cc->tlb_flush != NULL);
> >> +
> >> +    cc->tlb_flush(cpu, flush_global);
> >> +}
> > 
> > This needs to be able to call tlb_flush() itself
> > rather than having to have every single subclass of CPUState
> > implement an identical tlb_flush method. You could do this
> > if there was a CPU_GET_ENV()...
> 
> Which is exactly the point: CPUState does not know about the
> target-specific "env". And CPU_GET_ENV() is just plain wrong
> conceptually because it adds yet another cpu.h dependency.

Maybe so, but having every single taget implement its own copy of the exact 
same target independent wrapper seems even more wrong.

> There's a separation between old code using env and new, clean code:
> Just like Anthony doesn't want old concepts rewritten with the new type
> (cf. object_realize() discussion) I don't want the old cpu.h #define
> mess leaking into code that I'm redesigning specifically to get rid of
> that target-*/cpu.h dependency in favor of a single qemu/cpu.h.
> qom/cpu.c is by definition not compiled per target so it cannot contain
> any target-specific code.

At minimum it should be clearly documented[1] that this is a transitional 
hack, and how it should be removed.  There have already been two posts in this 
thread suggesting this is a feature, implying that this operation is somehow 
target specific.  I think the opposite is true:  This is a target agnostic 
detail of the TCG implementation, and implementing architecturally defined 
MMU/TLB behavior here is activley wrong.

Paul

[1] In the code, not the commit message.  Commit logs are not documentation.  
Commit logs are transient information valid only when the patch is applied.  
After that point they become archeological evidence, and you should not expect 
subsequent developers to be aware of them.