Bruno Piazera Larsen <bruno.lar...@eldorado.org.br> writes: >> > 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
Ok, so whatever you decide to do, make sure you state: "this is where TCG functions go", "this file is built on KVM|TCG only", and so on. And it's ok in principle to do a partial move for the RFC, but please mention that somewhere so we're all in the same page. > > 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. My point about ifdefs is that it would be easier to understand if you either: a) wrapped code in ifdefs, got the RCF going and then later moved all ifdef'ed code into a new tcg-only file; or b) define which is the tcg-only file right away and just move code in there. When you moved code into cpu.c *along with* the ifdefs, you kind of did both at the same time, which got confusing; to me at least. About moving CPU related functions into cpu.c, that's fine. But it is not strictly related to disable-tcg, so maybe send that in a separate patch that does it explicitly at the start of the series. > >> 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). Ok, so you left out some things on purpose to reduce complexity for the first RFC. That's fine, we just need to state these kinds of thing more clearly. > >> > 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? You could git rebase -i back into your first patch and then split it in two (git reset HEAD~1 and stage + commit each one), one for moving CPU code into cpu.c and other for moving GDB code into gdbstub.c. Or just checkout a new branch, apply the patch on top of it and commit just the GDB change. Feel free to ping me on IRC if you need help.
