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

[Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>]

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


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 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 5786 bytes --]