> 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.
Fair point, I should've thought about that.
> > 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).
There are 3 main reasons for this decision. The first is kind of silly, but when I read translate.c my mind jumped to translating machine code to TCG, and the amount of TCGv variables
at the start reinforced this notion.
The second was that both s390x and i386 removed it (translate.c) from compilation, so I had no good reason to doubt it.
The last (and arguably most important) is that translate.c is many thousands of lines long (translate_init.c.inc alone was almost 11k). The whole point of disabling TCG is to speed up compilation and
reduce the final file size, so I think it makes sense to remove that big file.
And the final nail in the coffin is that at no point did it cross my mind to keep the init part of translation, but remove the rest
Also, I'm not a fan of big ifdefs, because it's kinda hard to follow them when viewing code in vim. Adding that to already having a cpu.c file, where it makes sense (to me, at least) to add all CPU
related functions, just sounded like a good idea.
> 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 is me misjudging what is TCG and what is not, mostly. I think that actually moving everything to cpu.c and adding ifdefs, or creating a cpu_tcg.c.inc or similar, would be the most sensible code
motion, but every function I removed from translate gave me 3 new errors, at some point I felt like I should leave something behind otherwise we're compiling everything from TCG again, just in different files, so I tried to guess what was TCG and what was
not (also, I really wanted the RFC out by the weekend, I _may_ have rushed a few choices).
> > 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?
Sure, I'll try and get it ready ASAP, and make sure I didn't miss one function before sending. Just a noob question... do I edit the patch manually to have it only contain the gdb code motion, or is
there a way to move back to before I actually made the commit without needing to re-do the changes?
Thomas, I'm adding you to this discussion since it sort of answers your message on the other one, about the functions used even in a KVM-only build.
Now that I know that translate doesn't mean "translating to TCG", this idea makes more sense. My question is, is it a better option than the code motion I'm suggesting? From my quick check on the bits that I haven't touched it might, but at this point it's
clear I'm very lost with what makes sense hahaha.