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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Apr 13, 2021 at 06:38:57PM -0300, Fabiano Rosas wrote:
> Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:
> 
> >> I'm actually not sure if we'll want translate_init.c for !tcg builds.
> >> It's *primarily* for TCG, but we still need at least some of the cpu
> >> state structure for KVM, and some of that is initialized in
> >> translate_init.
> >>
> >> I think it will probably make more sense to leave it in for a first
> >> cut.  Later refinement might end up removing it.
> >>
> >> The whole #include translate_init.c.inc thing might make for some
> >> awkward fiddling in this, of course.
> >
> > 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);
> 
> Below I'm assuming we have one place for TCG stuff and other for KVM
> stuff, whatever this particular discussion ends up producing.
> 
> > * 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.
> 
> 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);
> }

I really dislike the idea above - it'd be way too easy for KVM and TCG
to get out of sync.  Instead make spr_register() itself a macro if
necessary, so it just ignores the access functions in the !TCG case.

> //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);
>         ...
> }
> 
> 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.
> 
> > * 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
> 
> Makes sense. This and the other part below about callback tables would
> be mostly about moving code so it's a candidate for coming soon.
> 
> > * 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.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]