On Fri, Apr 09, 2021 at 04:48:41PM -0300, Fabiano Rosas wrote:
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
> 
> A general advice for this whole series is: make sure you add in some
> words explaining why you decided to make a particular change. It will be
> much easier to review if we know what were the logical steps leading to
> the change.
> 
> > This commit does the necessary code motion from translate_init.c.inc
> 
> For instance, I don't immediately see why these changes are necessary. I
> see that translate_init.c.inc already has some `#ifdef CONFIG_TCG`, so
> why do we need to move a bunch of code into cpu.c instead of just adding
> more code under ifdef CONFIG_TCG? (I'm not saying it's wrong, just trying to
> understand the reasoning).
> 
> Is translate_init.c.inc intended to be TCG only? But then I see you
> moved TCG-only functions out of it (ppc_fixup_cpu) and left not TCG-only
> functions (gen_spr_generic).
> 
> > This moves all functions that start with gdb_* into target/ppc/gdbstub.c
> > and creates a new function that calls those and is called by ppc_cpu_realize
> 
> This looks like it makes sense regardless of disable-tcg, could we have
> it in a standalone patch?
> 
> > All functions related to realizing the cpu have been moved to cpu.c, which
> > may call functions from gdbstub or translate_init
> 
> Again, I don't disagree with this, but at first sight it doesn't seem
> entirely related to disabling TCG.

Fabioano's points seconded.  This isn't necessarily a bad idea, but a
rationale would really help.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson