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

From: David Gibson <david@gibson.dropbear.id.au>
To: Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>
Cc: Thomas Huth <thuth@redhat.com>,
	Fabiano Rosas <farosas@linux.ibm.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"lagarcia@br.ibm.com" <lagarcia@br.ibm.com>,
	Lucas Mateus Martins Araujo e Castro
	<lucas.araujo@eldorado.org.br>,
	Fernando Eckhardt Valle <fernando.valle@eldorado.org.br>,
	"qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>,
	Andre Fernando da Silva <andre.silva@eldorado.org.br>,
	Matheus Kowalczuk Ferst <matheus.ferst@eldorado.org.br>,
	Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
Subject: Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
Date: Mon, 19 Apr 2021 15:21:19 +1000	[thread overview]
Message-ID: <YH0TT/muW78nWUWV@yekko.fritz.box> (raw)
In-Reply-To: <CP2PR80MB449996D26DEA4C27397EEF14C74F9@CP2PR80MB4499.lamprd80.prod.outlook.com>

[-- Attachment #1: Type: text/plain, Size: 8723 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]