On Tue, Apr 13, 2021 at 05:43:02PM +0000, Bruno Piazera Larsen wrote: > > I'm actually not sure if we'll want translate_init.c for !tcg builds. > > It's *primarily* for TCG, but we still need at least some of the cpu > > state structure for KVM, and some of that is initialized in > > translate_init. > > > > I think it will probably make more sense to leave it in for a first > > cut. Later refinement might end up removing it. > > > > The whole #include translate_init.c.inc thing might make for some > > awkward fiddling in this, of course. > > I just checked, there is going to be some shuffling of functions > around, as there are some static variables defined on translate.c, > and used in translate_init.c.inc, some functions needed for KVM > on translate.c and some TCG only functions in the > translate_init.c.inc. > > The trivial path is to: > * rename translate_init.c.inc to cpu_init.c (since it has to do with > initial definitions for CPUs, and it's not related to translating > anymore); > * move gen_write_xer and gen_read_xer into cpu_init.c, as they're > used for some sprs, and whatever needs to be moved with it
Hmm.. that doesn't seem right. gen_*() functions are explicitly for generating code, so it really seems like they belong in the translation file. > * move is_indirect_opcode and ind_table to translate.c, since they > are used to translate ppc instructions, and the things defined for > these functions > * Figure out what needs to be added to the includes for both > files to compile > * move opcodes and invalid_handler into cpu_init.c, because they > are only used by stuff in this file. > > I'm just not sure about this last point. The stuff that use opcodes > create the callback tables for TCG, AFAICT. The better plan would > be to move all of that to tanslate.c, but might be a lot. > > Can I follow the trivial plan for the first cut and leave a TODO in > the code for a better solution in the future? Or is there a nuance > about one of those functions that I have not understood? > > > Bruno Piazera Larsen > > Instituto de Pesquisas > ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch> > > Departamento Computação Embarcada > > Analista de Software Trainee > > Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html> > > ________________________________ > De: David Gibson > Enviadas: Terça-feira, 13 de Abril de 2021 03:40 > Para: Bruno Piazera Larsen > Cc: Fabiano Rosas; Thomas Huth; qemu-devel@nongnu.org; Luis Fernando Fujita > Pires; Andre Fernando da Silva; Lucas Mateus Martins Araujo e Castro; > Fernando Eckhardt Valle; qemu-...@nongnu.org; lagar...@br.ibm.com; Matheus > Kowalczuk Ferst > Assunto: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling > tcg > > On Mon, Apr 12, 2021 at 12:05:31PM +0000, Bruno Piazera Larsen wrote: > > > 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. > > I think those are all sound reasons; I think not compiling translate.c > for !tcg builds is the right choice. We might, however, need to > "rescue" certain functions from there by moving them to another file > so they can be used by KVM code as well. > > > > 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. > > > > > IIRC you don't only have to exclude translate.c from the build, you also > > > have to separate translate_init.c.inc from it, i.e. turn > > > translate_init.c.inc into a proper .c file and get rid of the #include > > > "translate_init.c.inc" statement in translate.c, since many functions in > > > the > > > translate_init.c.inc file are still needed for the KVM-only builds, too. > > > So > > > maybe that's a good place to start as a first mini series. > > > > 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. > > > > > > Bruno Piazera Larsen > > > > Instituto de Pesquisas > > ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch> > > > > Departamento Computação Embarcada > > > > Analista de Software Trainee > > > > Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html> > -- 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
signature.asc
Description: PGP signature
