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: Tue, 13 Apr 2021 16:40:27 +1000	[thread overview]
Message-ID: <YHU82/SArAWirXIm@yekko.fritz.box> (raw)
In-Reply-To: <CP2PR80MB44994CEF7BA3C917016749B0C7709@CP2PR80MB4499.lamprd80.prod.outlook.com>

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

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).

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.

> > > 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 --]