Re: [PATCH v2 3/7] target/ppc: Isolated SPR read/write callbacks

[David Gibson]

On Fri, Apr 30, 2021 at 10:02:16AM -0300, Bruno Piazera Larsen wrote:
> On 30/04/2021 01:21, David Gibson wrote:
> > On Thu, Apr 29, 2021 at 01:21:26PM -0300, Bruno Larsen (billionai) wrote:
> > > Moved all SPR read/write callback, and some related functions, to a
> > > new file specific for it. These callbacks are TCG only, so separating
> > > them is required to support the flag disable-tcg.
> > > 
> > > Making the spr_noaccess function not static, and moving the define to
> > > internal.h is required, as spr_tcg.c.inc has to be included after
> > > SPR_NOACCESS has been used.
> > > 
> > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > > ---
> > >   target/ppc/internal.h           |    3 +
> > >   target/ppc/spr_tcg.c.inc        | 1033 +++++++++++++++++++++++++++++++
> > >   target/ppc/translate.c          |   49 +-
> > >   target/ppc/translate_init.c.inc |  981 -----------------------------
> > >   4 files changed, 1039 insertions(+), 1027 deletions(-)
> > >   create mode 100644 target/ppc/spr_tcg.c.inc
> > > 
> > > diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> > > index 184ba6d6b3..1de15acfbd 100644
> > > --- a/target/ppc/internal.h
> > > +++ b/target/ppc/internal.h
> > > @@ -228,4 +228,7 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
> > >   void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
> > >   gchar *ppc_gdb_arch_name(CPUState *cs);
> > > +void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> > > +#define SPR_NOACCESS (&spr_noaccess)
> > > +
> > >   #endif /* PPC_INTERNAL_H */
> > > diff --git a/target/ppc/spr_tcg.c.inc b/target/ppc/spr_tcg.c.inc
> > > new file mode 100644
> > > index 0000000000..48274dd52b
> > > --- /dev/null
> > > +++ b/target/ppc/spr_tcg.c.inc
> > I'm confused.  You create the new file as a .inc.c, but I don't see
> > anything #including the new file.
> <snip>
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index b319d409c6..a6e677fa6d 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -36,6 +36,7 @@
> > >   #include "exec/translator.h"
> > >   #include "exec/log.h"
> > >   #include "qemu/atomic128.h"
> > > +#include "internal.h"
> > >   #define CPU_SINGLE_STEP 0x1
> > > @@ -4175,43 +4176,6 @@ static void gen_tdi(DisasContext *ctx)
> > >   /***                          Processor control                         
> > >    ***/
> > > -static void gen_read_xer(DisasContext *ctx, TCGv dst)
> > > -{
> > > -    TCGv t0 = tcg_temp_new();
> > > -    TCGv t1 = tcg_temp_new();
> > > -    TCGv t2 = tcg_temp_new();
> > > -    tcg_gen_mov_tl(dst, cpu_xer);
> > > -    tcg_gen_shli_tl(t0, cpu_so, XER_SO);
> > > -    tcg_gen_shli_tl(t1, cpu_ov, XER_OV);
> > > -    tcg_gen_shli_tl(t2, cpu_ca, XER_CA);
> > > -    tcg_gen_or_tl(t0, t0, t1);
> > > -    tcg_gen_or_tl(dst, dst, t2);
> > > -    tcg_gen_or_tl(dst, dst, t0);
> > > -    if (is_isa300(ctx)) {
> > > -        tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
> > > -        tcg_gen_or_tl(dst, dst, t0);
> > > -        tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
> > > -        tcg_gen_or_tl(dst, dst, t0);
> > > -    }
> > > -    tcg_temp_free(t0);
> > > -    tcg_temp_free(t1);
> > > -    tcg_temp_free(t2);
> > > -}
> > > -
> > > -static void gen_write_xer(TCGv src)
> > > -{
> > > -    /* Write all flags, while reading back check for isa300 */
> > > -    tcg_gen_andi_tl(cpu_xer, src,
> > > -                    ~((1u << XER_SO) |
> > > -                      (1u << XER_OV) | (1u << XER_OV32) |
> > > -                      (1u << XER_CA) | (1u << XER_CA32)));
> > > -    tcg_gen_extract_tl(cpu_ov32, src, XER_OV32, 1);
> > > -    tcg_gen_extract_tl(cpu_ca32, src, XER_CA32, 1);
> > > -    tcg_gen_extract_tl(cpu_so, src, XER_SO, 1);
> > > -    tcg_gen_extract_tl(cpu_ov, src, XER_OV, 1);
> > > -    tcg_gen_extract_tl(cpu_ca, src, XER_CA, 1);
> > > -}
> > > -
> > >   /* mcrxr */
> > >   static void gen_mcrxr(DisasContext *ctx)
> > >   {
> > > @@ -4299,15 +4263,6 @@ static void gen_mfmsr(DisasContext *ctx)
> > >       tcg_gen_mov_tl(cpu_gpr[rD(ctx->opcode)], cpu_msr);
> > >   }
> > > -static void spr_noaccess(DisasContext *ctx, int gprn, int sprn)
> > > -{
> > > -#if 0
> > > -    sprn = ((sprn >> 5) & 0x1F) | ((sprn & 0x1F) << 5);
> > > -    printf("ERROR: try to access SPR %d !\n", sprn);
> > > -#endif
> > > -}
> > > -#define SPR_NOACCESS (&spr_noaccess)
> > > -
> > >   /* mfspr */
> > >   static inline void gen_op_mfspr(DisasContext *ctx)
> > >   {
> > > @@ -7639,6 +7594,7 @@ GEN_HANDLER2_E(trechkpt, "trechkpt", 0x1F, 0x0E, 
> > > 0x1F, 0x03FFF800, \
> > >   };
> > >   #include "helper_regs.h"
> > > +#include "spr_tcg.c.inc"
> > >   #include "translate_init.c.inc"
> 
> You probably just missed it here. Buried a bit in all the code motion,
> unfortunately...

Oops, sorry.  Thought I'd grepped for it.


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