Re: [RFC PATCH 08/11] target/ppc: wrapped some TCG only logic with ifdefs

[David Gibson]

On Fri, May 14, 2021 at 01:22:48PM -0300, Bruno Piazera Larsen wrote:
> 
> On 14/05/2021 11:44, Richard Henderson wrote:
> > On 5/14/21 8:29 AM, Bruno Piazera Larsen wrote:
> > > 
> > > On 12/05/2021 15:57, Bruno Piazera Larsen wrote:
> > > > On 12/05/2021 15:33, Richard Henderson wrote:
> > > > > On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
> > > > > > diff --git a/include/exec/helper-proto.h
> > > > > > b/include/exec/helper-proto.h
> > > > > > index ba100793a7..ce287222ee 100644
> > > > > > --- a/include/exec/helper-proto.h
> > > > > > +++ b/include/exec/helper-proto.h
> > > > > > @@ -38,7 +38,9 @@ dh_ctype(ret) HELPER(name)
> > > > > > (dh_ctype(t1), dh_ctype(t2), dh_ctype(t3), \
> > > > > >   #define IN_HELPER_PROTO
> > > > > >     #include "helper.h"
> > > > > > +#ifdef CONFIG_TCG
> > > > > >   #include "trace/generated-helpers.h"
> > > > > > +#endif
> > > > > >   #include "accel/tcg/tcg-runtime.h"
> > > > > >   #include "accel/tcg/plugin-helpers.h"
> > > > > 
> > > > > Um.. this file is exclusively TCG already.
> > > > > Are you missing some use of helper_foo()?
> > > > A lot of files that we are compiling (mainly mmu-*, excp_helper
> > > > and gdbstub IIRC). We could comb through all of them and remove
> > > > all declarations of helpers and wrap the inclusion of
> > > > helper-proto itself in ifdefs, but it felt unnecessarily long.
> > > > If it is preferable, we can do it.
> > > > 
> > > So, I just looked and we'd need to change excp_helper.c and
> > > mmu-hash64.c, encasing 14 and 8 helper_foo() declarations. Is it
> > > better to work on those 2 files, or to change helper-proto?
> > 
> > Let's work on excp_helper.c and mmu-hash64.c.
> > 
> > For excp_helper.c, ideally everything in there would be tcg related. 
> > Either explicitly as helper_foo() or by being one of the TCGCPUOps
> > functions like ppc_cpu_exec_interrupt.
> 
> Removing excp_helper.c gives linker errors for the functions:
> 
> * ppc_cpu_do_system_reset, on hw/ppc/pnv.c and hw/ppc/spapr.c

Oof, that's a bit tricky.  We definitely do need this system reset
injection for KVM as well as TCG.  Unfortunately it calls into
powerpc_excp() which I think has a bunch of TCG specific stuff as
well.

Long term, I think the thing would be to remove the giant ugly
multiplexer in powerpc_excp() in favour of different entry points.
But that's a big job.

Short term, littering it with ifdefs might be the least worst we can
do.  Richard, any better ideas?


> * ppc_do_cpu_interrupt, on hw/ppc/spapr_events.c and target/ppc/kvm.c, and
> from what was discussed on the IRC this should still be compiled

Similar situation to above, but much easier because it doesn't go
through the giant multiplexer.
> 
> 
> I think ppc_do_cpu_interrupt could go to cpu.c, but no clue where the others
> should go.

Yes, moving ppc_cpu_do_interrupt() to common code is the right call.

> 
> > 
> > For mmu-hash64.c... I guess the easiest thing in the short term is to
> > put big ifdefs around helper_slbi{a,e,eg}.  Or they could be moved to
> > mmu_helper.c, with slb_lookup declared in mmu-hash64.h.
> > 
> The ifdefs are easy enough, and not compiling it gives way too many linker
> errors. Moving the helpers and static functions sounds the cleanest though,
> so I'll look into it while I wait for the excp_helper help
> > 
> > r~

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

signature.asc
Description: PGP signature