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

[Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>]

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

> > * 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
>
> I'd leave them where they are currently. Instead what I think we should
> do is to find a way to not need the uea/oea/hea|read/write callbacks
> with KVM.

so we'd also move all callbacks to translate.c, right? RN, gen_write_xer
is only used in spr_read_xer, which is defined in cpu_init.c

From a quick glance, this would be almost 3k lines, so bigger patches
are incoming (side note: I tried to use that git config to show that I only
changed file names and deal better with code motion, but it doesn't
appear to have worked, is the wiki correct about this?)

> Maybe extract a function from _spr_register that sets what is common for
> both tcg and kvm (num, name, initial_value, AFAICT). Then alter the
> gen_spr* functions to first create all registers and then call both
> configs to supplement:
>
> //tcg.c
> static void tcg_gen_spr_generic(CPUPPCState *env)
> {
>     // these only set the callbacks
>     spr_register(env, SPR_FOO,
>                  SPR_NOACCESS, SPR_NOACCESS,
>                  &spr_read_foo, &spr_write_foo);
>     spr_register(env, SPR_BAR,
>                  SPR_NOACCESS, SPR_NOACCESS,
>                  &spr_read_bar, &spr_write_bar);
> }
>
> //kvm.c
> static void kvm_gen_spr_generic(CPUPPCState *env)
> {
>     // these only set one_reg_id
>     spr_register_kvm(env, SPR_FOO, KVM_REG_PPC_FOO);
>     spr_register_kvm(env, SPR_BAR, KVM_REG_PPC_BAR);
> }

by default, KVM already doesn't use the callbacks? Or would we have to
also change where these registers are accessed? If the first one is right
this looks easy enough.

> //common.c
> static void gen_spr_generic(CPUPPCState *env)
> {
>     // these only set name, num, initial value
>     spr_register(env, SPR_FOO, "FOO", 0xf00);
>     spr_register(env, SPR_BAR, "BAR", 0xb4d);
>     ...
>
>     // have these stubbed if not chosen via config
>     tcg_gen_spr_generic(env);
>     kvm_gen_spr_generic(env);
> }
>
> init_ppc_proc()
> {
>         ...
>         gen_spr_generic(env);
>         ...
> }

I'm guessing we'd need to do this to all gen_spr_* functions, this is just
an example, right?

> Can anyone see a better way? That would be much easier if we could
> afford to say that TCG and KVM are mutually exclusive for a given build,
> but I don't think they are.

Instead of stubbing, we could also create macros that turn the function call
into a nop if the config was disabled, and add "if kvm_enabled()" and
"if tcg_enabled()" if needed. I don't see how TCG and KVM being mutually
exclusive makes this easier, unless it has to do with where they are
accessed (idk yet where that is).

Another option is the solution I prototyped in [PATCH 2/4] in arch_dump.c,
having ifdef encapsulating kvm and tcg calls, and if/else blocks. I'm also
open to suggestions on how to do it better (:

> > * 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.
>
> translate.c seems like a better place indeed.

ok. But is it worth doing for the first cut?

Also, looking now, I see definition for exception vectors and some
exception handling code in it, which I'm not 100% sure what to do with.
It's starting to seem like should actually make this translate_init.c.inc
into a mini series of its own, if we're going to make this the best way
from the start.


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>


[-- Attachment #2: Type: text/html, Size: 13164 bytes --]