Re: [RFC PATCH 3/4] target/ppc: Move SPR generation to separate file

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

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

On Mon, Apr 26, 2021 at 06:08:53PM -0300, Fabiano Rosas wrote:
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
> 
> > This move is required to enable building without TCG.
> > All the logic related to registering SPRs specific to
> > some architectures or machines has been hidden in this
> > new file.
> 
> Hm... I thought we ended up deciding to keep the gen_spr_<machine>
> functions in translate_init.c.inc (cpu_init.c). I don't strongly favour
> one way or the other, but one alternative would be to just rename the
> gen_spr_<machine> functions to add_sprs_<machine> or even
> register_<machine>_sprs and leave them where they are.

Right.  I think renaming these away from gen_*() is a good idea, to
avoid confusion with the other gen_*() functions which, well, generate
TCG code.

I don't think there's a lot of value in moving them out from
translate_init.  Honestly the more useful way to break up
translate_init would be by CPU family, rather than by SPR vs. non-SPR
setup

> 
> > The idea of this final patch is to hide all SPR generation from
> > translate_init, but in an effort to simplify the RFC the 4
> > functions for not_implemented SPRs were created. They'll be
> > substituted by gen_spr_<machine>_misc in reusable ways for the
> > final patch.
> 
> I'd expect this patch to be just a big removal of gen_spr* from
> translate_init.c.inc and their addition into spr_common.c. So any other
> prep work should come in separate patches ealier in the
> series. Specifically, at least one patch for the macro work and another
> for the refactoring of open coded spr_register calls into gen_spr_*
> functions.

Seconded.

> 
> > another issue we ran into was vscr_init using static functions
> > means it has to be static, so we had to remove them from 
> > gen_spr_74xx and gen_spr_book3s_altivec, and have them in
> > the init_procs instead.
> 
> Looks like moving vscr_init out, along with a more detailed explanation
> of the issue could be in another preliminary change.
> 
> >
> > Finally, SPR_NOACCESS had to be defined in internal.h, as it
> > is used by spr_common, translate_init and translate. If there
> > is a better solution, I'll be happy to implement it.
> >
> > As for the redundant code complaint this patch will get, it has only
> > been moved, so I don't know if I can remove that code
> >
> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > ---
> >  target/ppc/internal.h           |  108 +
> >  target/ppc/meson.build          |    1 +
> >  target/ppc/spr_common.c         | 2943 ++++++++++++++++++++++
> >  target/ppc/translate_init.c.inc | 4031 ++-----------------------------
> >  4 files changed, 3314 insertions(+), 3769 deletions(-)
> >  create mode 100644 target/ppc/spr_common.c
> >
> > diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> > index de78c23717..25df546eae 100644
> > --- a/target/ppc/internal.h
> > +++ b/target/ppc/internal.h
> > @@ -226,4 +226,112 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
> >  void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
> >  gchar *ppc_gdb_arch_name(CPUState *cs);
> >  
> > +/* spr-common.c */
> > +#include "cpu.h"
> > +void gen_spr_generic(CPUPPCState *env);
> 
> The fact that these are called gen_* is confusing since they don't
> really generate anything. They mostly just add SPRs to the list and
> register the SPR rw callbacks for TCG. Maybe we could rename them at the
> end of the series to something more clear.
> 
> > +void gen_spr_ne_601(CPUPPCState *env);
> > +void gen_spr_sdr1(CPUPPCState *env);
> > +void gen_low_BATs(CPUPPCState *env);
> > +void gen_high_BATs(CPUPPCState *env);
> > +void gen_tbl(CPUPPCState *env);
> > +void gen_6xx_7xx_soft_tlb(CPUPPCState *env, int nb_tlbs, int nb_ways);
> > +void gen_spr_G2_755(CPUPPCState *env);
> > +void gen_spr_7xx(CPUPPCState *env);
> > +#ifdef TARGET_PPC64
> > +void gen_spr_amr(CPUPPCState *env);
> > +void gen_spr_iamr(CPUPPCState *env);
> > +#endif /* TARGET_PPC64 */
> > +void gen_spr_thrm(CPUPPCState *env);
> > +void gen_spr_604(CPUPPCState *env);
> > +void gen_spr_603(CPUPPCState *env);
> > +void gen_spr_G2(CPUPPCState *env);
> > +void gen_spr_602(CPUPPCState *env);
> > +void gen_spr_601(CPUPPCState *env);
> > +void gen_spr_74xx(CPUPPCState *env);
> > +void gen_l3_ctrl(CPUPPCState *env);
> > +void gen_74xx_soft_tlb(CPUPPCState *env, int nb_tlbs, int nb_ways);
> > +void gen_spr_not_implemented(CPUPPCState *env,
> > +                             int num, const char *name);
> > +void gen_spr_not_implemented_ureg(CPUPPCState *env,
> > +                                  int num, const char *name);
> > +void gen_spr_not_implemented_no_write(CPUPPCState *env,
> > +                                      int num, const char *name);
> > +void gen_spr_not_implemented_write_nop(CPUPPCState *env,
> > +                                       int num, const char *name);
> > +void gen_spr_PSSCR(CPUPPCState *env);
> > +void gen_spr_TIDR(CPUPPCState *env);
> > +void gen_spr_pvr(CPUPPCState *env, PowerPCCPUClass *pcc);
> > +void gen_spr_svr(CPUPPCState *env, PowerPCCPUClass *pcc);
> > +void gen_spr_pir(CPUPPCState *env);
> > +void gen_spr_spefscr(CPUPPCState *env);
> > +void gen_spr_l1fgc(CPUPPCState *env, int num, int initial_value);
> > +void gen_spr_hid0(CPUPPCState *env);
> > +void gen_spr_mas73(CPUPPCState *env);
> > +void gen_spr_mmucsr0(CPUPPCState *env);
> > +void gen_spr_l1csr0(CPUPPCState *env);
> > +void gen_spr_l1csr1(CPUPPCState *env);
> > +void gen_spr_l2csr0(CPUPPCState *env);
> > +void gen_spr_usprg3(CPUPPCState *env);
> > +void gen_spr_usprgh(CPUPPCState *env);
> > +void gen_spr_BookE(CPUPPCState *env, uint64_t ivor_mask);
> > +uint32_t gen_tlbncfg(uint32_t assoc, uint32_t minsize,
> > +                     uint32_t maxsize, uint32_t flags,
> > +                     uint32_t nentries);
> > +void gen_spr_BookE206(CPUPPCState *env, uint32_t mas_mask,
> > +                             uint32_t *tlbncfg, uint32_t mmucfg);
> > +void gen_spr_440(CPUPPCState *env);
> > +void gen_spr_440_misc(CPUPPCState *env);
> > +void gen_spr_40x(CPUPPCState *env);
> > +void gen_spr_405(CPUPPCState *env);
> > +void gen_spr_401_403(CPUPPCState *env);
> > +void gen_spr_401(CPUPPCState *env);
> > +void gen_spr_401x2(CPUPPCState *env);
> > +void gen_spr_403(CPUPPCState *env);
> > +void gen_spr_403_real(CPUPPCState *env);
> > +void gen_spr_403_mmu(CPUPPCState *env);
> > +void gen_spr_40x_bus_control(CPUPPCState *env);
> > +void gen_spr_compress(CPUPPCState *env);
> > +void gen_spr_5xx_8xx(CPUPPCState *env);
> > +void gen_spr_5xx(CPUPPCState *env);
> > +void gen_spr_8xx(CPUPPCState *env);
> > +void gen_spr_970_hid(CPUPPCState *env);
> > +void gen_spr_970_hior(CPUPPCState *env);
> > +void gen_spr_book3s_ctrl(CPUPPCState *env);
> > +void gen_spr_book3s_altivec(CPUPPCState *env);
> > +void gen_spr_book3s_dbg(CPUPPCState *env);
> > +void gen_spr_book3s_207_dbg(CPUPPCState *env);
> > +void gen_spr_970_dbg(CPUPPCState *env);
> > +void gen_spr_book3s_pmu_sup(CPUPPCState *env);
> > +void gen_spr_book3s_pmu_user(CPUPPCState *env);
> > +void gen_spr_970_pmu_sup(CPUPPCState *env);
> > +void gen_spr_970_pmu_user(CPUPPCState *env);
> > +void gen_spr_power8_pmu_sup(CPUPPCState *env);
> > +void gen_spr_power8_pmu_user(CPUPPCState *env);
> > +void gen_spr_power5p_ear(CPUPPCState *env);
> > +void gen_spr_power5p_tb(CPUPPCState *env);
> > +void gen_spr_970_lpar(CPUPPCState *env);
> > +void gen_spr_power5p_lpar(CPUPPCState *env);
> > +void gen_spr_book3s_ids(CPUPPCState *env);
> > +void gen_spr_rmor(CPUPPCState *env);
> > +void gen_spr_power8_ids(CPUPPCState *env);
> > +void gen_spr_book3s_purr(CPUPPCState *env);
> > +void gen_spr_power6_dbg(CPUPPCState *env);
> > +void gen_spr_power5p_common(CPUPPCState *env);
> > +void gen_spr_power6_common(CPUPPCState *env);
> > +void gen_spr_power8_tce_address_control(CPUPPCState *env);
> > +void gen_spr_power8_tm(CPUPPCState *env);
> > +void gen_spr_power8_ebb(CPUPPCState *env);
> > +void gen_spr_vtb(CPUPPCState *env);
> > +void gen_spr_power8_fscr(CPUPPCState *env);
> > +void gen_spr_power8_pspb(CPUPPCState *env);
> > +void gen_spr_power8_dpdes(CPUPPCState *env);
> > +void gen_spr_power8_ic(CPUPPCState *env);
> > +void gen_spr_power8_book4(CPUPPCState *env);
> > +void gen_spr_power7_book4(CPUPPCState *env);
> > +void gen_spr_power8_rpr(CPUPPCState *env);
> > +void gen_spr_power9_mmu(CPUPPCState *env);
> 
> Maybe it would be better to rename spr_tcg.h to spr.h and move all of
> this in there?
> 
> > +/* TODO: find better solution for gen_op_mfspr and gen_op_mtspr */
> 
> What is the issue with these? I only see them being used in translate.c.
> 
> > +void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> 
> This is a rw callback, it should not be here.
> 
> > +#define SPR_NOACCESS (&spr_noaccess)
> 
> If you're only adding this now, it means the previous patch is
> broken. As a general rule you should make sure every patch works. I know
> that for the RFC things might be a bit broken temporarily and that is ok
> but it is always a good idea to make sure every individual change is in
> the correct patch at least.
> 
> > +
> >  #endif /* PPC_INTERNAL_H */
> > diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> > index bbfef90e08..aaee5e7c0c 100644
> > --- a/target/ppc/meson.build
> > +++ b/target/ppc/meson.build
> > @@ -9,6 +9,7 @@ ppc_ss.add(files(
> >    'int_helper.c',
> >    'mem_helper.c',
> >    'misc_helper.c',
> > +  'spr_common.c',
> >    'timebase_helper.c',
> >    'translate.c',
> >  ))
> > diff --git a/target/ppc/spr_common.c b/target/ppc/spr_common.c
> > new file mode 100644
> > index 0000000000..e34f4fe9dc
> > --- /dev/null
> > +++ b/target/ppc/spr_common.c
> > @@ -0,0 +1,2943 @@
> > +
> > +#include "qemu/osdep.h"
> > +#include "disas/disas.h"
> > +#include "cpu.h"
> > +#include "fpu/softfloat.h"
> > +#include "cpu-models.h"
> > +#include "sysemu/hw_accel.h"
> > +#include "tcg/tcg-op.h"
> > +#include "tcg/tcg-op-gvec.h"
> > +#include "exec/translator.h"
> > +#include "internal.h"
> > +#include "exec/gen-icount.h"
> > +#include "spr_tcg.h"
> > +
> > +/*****************************************************************************/
> > +/* SPR definitions and registration */
> > +
> > +#ifdef CONFIG_TCG
> > +#ifdef CONFIG_USER_ONLY
> > +#define spr_register_kvm(env, num, name, uea_read, uea_write,                  \
> > +                         oea_read, oea_write, one_reg_id, initial_value)       \
> > +    _spr_register(env, num, name, uea_read, uea_write, initial_value)
> > +#define spr_register_kvm_hv(env, num, name, uea_read, uea_write,               \
> > +                            oea_read, oea_write, hea_read, hea_write,          \
> > +                            one_reg_id, initial_value)                         \
> > +    _spr_register(env, num, name, uea_read, uea_write, initial_value)
> > +#else /* CONFIG_USER_ONLY */
> > +#if !defined(CONFIG_KVM)
> > +#define spr_register_kvm(env, num, name, uea_read, uea_write,                  \
> > +                         oea_read, oea_write, one_reg_id, initial_value)       \
> > +    _spr_register(env, num, name, uea_read, uea_write,                         \
> > +                  oea_read, oea_write, oea_read, oea_write, initial_value)
> > +#define spr_register_kvm_hv(env, num, name, uea_read, uea_write,               \
> > +                            oea_read, oea_write, hea_read, hea_write,          \
> > +                            one_reg_id, initial_value)                         \
> > +    _spr_register(env, num, name, uea_read, uea_write,                         \
> > +                  oea_read, oea_write, hea_read, hea_write, initial_value)
> > +#else /* !CONFIG_KVM */
> > +#define spr_register_kvm(env, num, name, uea_read, uea_write,                  \
> > +                         oea_read, oea_write, one_reg_id, initial_value)       \
> > +    _spr_register(env, num, name, uea_read, uea_write,                         \
> > +                  oea_read, oea_write, oea_read, oea_write,                    \
> > +                  one_reg_id, initial_value)
> > +#define spr_register_kvm_hv(env, num, name, uea_read, uea_write,               \
> > +                            oea_read, oea_write, hea_read, hea_write,          \
> > +                            one_reg_id, initial_value)                         \
> > +    _spr_register(env, num, name, uea_read, uea_write,                         \
> > +                  oea_read, oea_write, hea_read, hea_write,                    \
> > +                  one_reg_id, initial_value)
> > +#endif /* !CONFIG_KVM */
> > +#endif /* CONFIG_USER_ONLY */
> > +#else /* CONFIG_TCG */
> > +#ifdef CONFIG_KVM /* sanity check */
> > +#define spr_register_kvm(env, num, name, uea_read, uea_write,                  \
> > +                         oea_read, oea_write, one_reg_id, initial_value)       \
> > +    _spr_register(env, num, name, one_reg_id, initial_value)
> > +#define spr_register_kvm_hv(env, num, name, uea_read, uea_write,               \
> > +                            oea_read, oea_write, hea_read, hea_write,          \
> > +                            one_reg_id, initial_value)                         \
> > +    _spr_register(env, num, name, one_reg_id, initial_value)
> > +#else /* CONFIG_KVM */
> > +#error "Either TCG or KVM should be configured"
> > +#endif /* CONFIG_KVM */
> > +#endif /*CONFIG_TCG */
> > +
> > +#define spr_register(env, num, name, uea_read, uea_write,                      \
> > +                     oea_read, oea_write, initial_value)                       \
> > +    spr_register_kvm(env, num, name, uea_read, uea_write,                      \
> > +                     oea_read, oea_write, 0, initial_value)
> > +
> > +#define spr_register_hv(env, num, name, uea_read, uea_write,                   \
> > +                        oea_read, oea_write, hea_read, hea_write,              \
> > +                        initial_value)                                         \
> > +    spr_register_kvm_hv(env, num, name, uea_read, uea_write,                   \
> > +                        oea_read, oea_write, hea_read, hea_write,              \
> > +                        0, initial_value)
> 
> This change is crucial for this to work, we don't want it buried along
> with all of the code movement. It should be clearly described and in
> it's own patch at the top of the series.
> 
> If you add this change, plus some #ifdef TCG around the spr callbacks,
> it should already be possible to compile this with disable-tcg. It would
> also make the series as a whole easier to understand.
> 
> > +
> > +static inline void _spr_register(CPUPPCState *env, int num,
> > +                                 const char *name,
> > +#ifdef CONFIG_TCG
> > +                                 void (*uea_read)(DisasContext *ctx,
> > +                                                  int gprn, int sprn),
> > +                                 void (*uea_write)(DisasContext *ctx,
> > +                                                   int sprn, int gprn),
> > +#if !defined(CONFIG_USER_ONLY)
> > +
> > +                                 void (*oea_read)(DisasContext *ctx,
> > +                                                  int gprn, int sprn),
> > +                                 void (*oea_write)(DisasContext *ctx,
> > +                                                   int sprn, int gprn),
> > +                                 void (*hea_read)(DisasContext *opaque,
> > +                                                  int gprn, int sprn),
> > +                                 void (*hea_write)(DisasContext *opaque,
> > +                                                   int sprn, int gprn),
> > +#endif
> > +#endif
> > +#if defined(CONFIG_KVM)
> > +                                 uint64_t one_reg_id,
> > +#endif
> > +                                 target_ulong initial_value)
> > +{
> > +    ppc_spr_t *spr;
> > +
> > +    spr = &env->spr_cb[num];
> > +    if (spr->name != NULL || env->spr[num] != 0x00000000
> > +#ifdef CONFIG_TCG
> > +#if !defined(CONFIG_USER_ONLY)
> > +        || spr->oea_read != NULL || spr->oea_write != NULL
> > +#endif
> > +        || spr->uea_read != NULL || spr->uea_write != NULL
> > +#endif
> > +        ) {
> > +        printf("Error: Trying to register SPR %d (%03x) twice !\n", num, num);
> > +        exit(1);
> > +    }
> > +#if defined(PPC_DEBUG_SPR)
> > +    printf("*** register spr %d (%03x) %s val " TARGET_FMT_lx "\n", num, num,
> > +           name, initial_value);
> > +#endif
> > +    spr->name = name;
> > +#ifdef CONFIG_TCG
> > +    spr->uea_read = uea_read;
> > +    spr->uea_write = uea_write;
> > +#if !defined(CONFIG_USER_ONLY)
> > +    spr->oea_read = oea_read;
> > +    spr->oea_write = oea_write;
> > +    spr->hea_read = hea_read;
> > +    spr->hea_write = hea_write;
> > +#endif
> > +#endif
> > +#if defined(CONFIG_KVM)
> > +    spr->one_reg_id = one_reg_id,
> > +#endif
> > +    env->spr[num] = spr->default_value = initial_value;
> > +}
> 

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