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

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

* 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

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer

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

From:	Bruno Piazera Larsen
Subject:	RE: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
Date:	Tue, 13 Apr 2021 17:43:02 +0000