[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg
From: |
Fabiano Rosas |
Subject: |
RE: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg |
Date: |
Mon, 12 Apr 2021 10:56:23 -0300 |
Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:
>> > 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
Ok, so whatever you decide to do, make sure you state: "this is where
TCG functions go", "this file is built on KVM|TCG only", and so on. And
it's ok in principle to do a partial move for the RFC, but please
mention that somewhere so we're all in the same page.
>
> 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.
My point about ifdefs is that it would be easier to understand if you
either:
a) wrapped code in ifdefs, got the RCF going and then later moved all
ifdef'ed code into a new tcg-only file;
or
b) define which is the tcg-only file right away and just move code in
there.
When you moved code into cpu.c *along with* the ifdefs, you kind of did
both at the same time, which got confusing; to me at least.
About moving CPU related functions into cpu.c, that's fine. But it is
not strictly related to disable-tcg, so maybe send that in a separate
patch that does it explicitly at the start of the series.
>
>> 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).
Ok, so you left out some things on purpose to reduce complexity for the
first RFC. That's fine, we just need to state these kinds of thing more
clearly.
>
>> > 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?
You could git rebase -i back into your first patch and then split it in
two (git reset HEAD~1 and stage + commit each one), one for moving CPU
code into cpu.c and other for moving GDB code into gdbstub.c.
Or just checkout a new branch, apply the patch on top of it and commit
just the GDB change.
Feel free to ping me on IRC if you need help.