Re: [PATCH v4 0/5] target/ppc: Untangle CPU init from translation

[Bruno Piazera Larsen]

On 04/05/2021 17:38, Fabiano Rosas wrote:

> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
>
> > Based-on: ppc-for-6.1 tree
> >
> > This patch series aims to remove the logic of initializing CPU from
> > the file related to TCG translation. To achieve this, we have to make
> > it so registering SPRs isn't directly tied to TCG, and move code only
> > related to translation out of translate_init.c.inc and into translate.c.
> > This is in preparation to compile this target without TCG.
> >
> > Changes for v4:
> >  * reordered patches, to make partially applying simpler
> >  * removed patches that were already applied
> >  * undone creation of spt_tcg.c.inc, now waiting for further cleanup
> >  * moved SPR_NOACCESS motion to last patch, and to spr_tcg.h
> >
> > Changes for v3:
> >  * fixed the parameters of _spr_register
> >  * remove some redundant #include statements
> >  * removed some functions that were mentioned in v2 as unnecessary
> >  * added copyright header to relevant files
> >  * removed first patch, that was already applied
> >  * removed a changed that would add a regression
> >
> >  Changes for v2:
> >  * split and reordered patches, to make it easier to review
> >  * improved commit messages 
> >  * Undid creation of spr_common, as it was unnecessary
> >  * kept more functions as static
> >  * ensured that the project builds after every commit
> >
> > Bruno Larsen (billionai) (5):
> >   target/ppc: Fold gen_*_xer into their callers
> >   target/ppc: renamed SPR registration functions
> >   target/ppc: move SPR R/W callbacks to translate.c
> >   target/ppc: turned SPR R/W callbacks not static
> >   target/ppc: isolated cpu init from translation logic
> >
> >  .../ppc/{translate_init.c.inc => cpu_init.c}  | 1848 ++++-------------
> >  target/ppc/meson.build                        |    1 +
> >  target/ppc/spr_tcg.h                          |  136 ++
> >  target/ppc/translate.c                        | 1072 +++++++++-
> >  4 files changed, 1598 insertions(+), 1459 deletions(-)
> >  rename target/ppc/{translate_init.c.inc => cpu_init.c} (89%)
> >  create mode 100644 target/ppc/spr_tcg.h
> >       
> We're still missing some changes:
>
> - some files (hw/ppc/pnv.c, hw/ppc/spapr.c) use oea_read to check if an
> SPR exists. This needs to be changed to something that is present in
> both configs (I believe Bruno is working on this).
>
> - The commit 6113563982 ("target/ppc: Clean up _spr_register et al")
> from the ppc-for-6.1 branch missed some TCG-specific code in
> gen_spr_BookE206:
>

These patches are all in preparation for disabling TCG. I wouldn't expect the project to support that flag yet, so the errors make sense and are being worked on.

I guess I did things in a weird order, so let me explain my thought process for the work so far: In my first RFC (when I still thought it was an easy-ish fix) I tried to define what needed to be done so the project would build. Then I went back to the drawing board, and decided to implement good solutions, and only expect the --disable-tcg to work at the very end of these, otherwise the bodged solutions could be forgotten and committed into the final tree by accident. Now that I know the code motion was done in a satisfactory manner, I'll move on to these issues and what else shows up.

> $ ../configure --target-list=ppc64-softmmu --disable-tcg
> $ make
> (...)
> [193/264] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc_cpu_init.c.o
> FAILED: libqemu-ppc64-softmmu.fa.p/target_ppc_cpu_init.c.o 
> (...)
> ../target/ppc/cpu_init.c: In function ‘register_BookE206_sprs’:
> ../target/ppc/cpu_init.c:1207:16: error: variable ‘uea_write’ set but not used [-Werror=unused-but-set-variable]
>          void (*uea_write)(DisasContext *ctx, int sprn, int gprn) =
>                 ^~~~~~~~~
> cc1: all warnings being treated as errors
>
> We need something like:
>
> --- a/target/ppc/translate_init.c.inc   2021-05-04 16:24:53.549556292 -0400
> +++ b/target/ppc/translate_init.c.inc   2021-05-04 16:26:41.005280971 -0400
> @@ -2025,11 +2025,13 @@
>      /* TLB assist registers */
>      /* XXX : not implemented */
>      for (i = 0; i < 8; i++) {
> +#ifdef CONFIG_TCG
>          void (*uea_write)(DisasContext *ctx, int sprn, int gprn) =
>              &spr_write_generic32;
>          if (i == 2 && (mas_mask & (1 << i)) && (env->insns_flags & PPC_64B)) {
>              uea_write = &spr_write_generic;
>          }
> +#endif
>          if (mas_mask & (1 << i)) {
>              spr_register(env, mas_sprn[i], mas_names[i],
>                           SPR_NOACCESS, SPR_NOACCESS,
> ---
>

Also ran into this problem, I intend to fix it as well. If you find anything else, let me know :)

--

Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer