Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

[David Gibson]

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-ppc@nongnu.org; lagarcia@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