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

[Bruno Piazera Larsen]

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

> 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

Departamento Computação Embarcada

Analista de Software Trainee

Aviso Legal - Disclaimer