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

[Fabiano Rosas]

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

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

Yeah, move them away from the common file into a tcg-only file.

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

KVM does not use the callbacks.

>> //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?

Yeah, so that's one of the downsides of this change I proposed.

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

If they were mutually exclusive we could solve most problems by having
the same signature for a function and compiling one or the other
depending on the config.

That would mean we would be able to move the whole gen_spr_* functions
to the accel-specific files. So:

//tcg.c
static void gen_spr_generic(CPUPPCState *env)
{
    spr_register(env, SPR_FOO, "FOO", 0xf00, &read_foo, &write_foo);
    spr_register(env, SPR_BAR, "BAR", 0xb4d, &read_bar, &write_bar);
}

//kvm.c
static void gen_spr_generic(CPUPPCState *env)
{
    spr_register(env, SPR_FOO, "FOO", 0xf00, KVM_REG_FOO);
    spr_register(env, SPR_BAR, "BAR", 0xb4d, KVM_REG_BAR);
}

//common.c
init_ppc_proc()
{
        ...
        gen_spr_generic(env);
        ...
}

But we can't do this because we want to have a QEMU binary that supports
both accel types in certain scenarios.

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

I think it is. I don't see the issue. Aside from the opcodes destructor
you'll just move a chunk of code over. We do want cpu_init.c to be the
common file, right? So it cannot have TCG-only code in it. Better do it
now while we're (mostly) just moving code around.

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

These are tricky because there's some logic for system emulation that is
used by kvm, spapr, etc. So we'll need to do more invasive
changes. Let's tackle the easy things first.

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