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

[Fabiano Rosas <farosas@linux.ibm.com>]

Bruno Piazera Larsen <bruno.larsen@eldorado.org.br> writes:

>> > > +/*****************************************************************************/
>> > > +/* 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.
>
> if we added this and removed TCG only files from meson.build it might
> compile already (not sure, I think there were a couple of things that
> used mmu functions), 

I'm just pointing out that the set of changes needed to compile
translate_init.c.inc with disable-tcg is actually pretty small. Sure, we
want to move some stuff around and have new files and whatnot to make
the code easier to read/more maintainable, etc. But none of that is
strictly required.

> but wouldn't there be way too many ifdefs in that case?
> The R/W callbacks are spread all around the file, and we'd have to ifdef around
> some .h files that are included in common files.

Right, but it is easier to review only the macro change plus some
ifdefs. Once we are sure the change is correct and the ifdefs select the
right code, any follow-up patch that only moves things to another file
will be trivial. And if the first patch gets too silly we can always
squash them before merging, no worries.

>
> Bruno Piazera Larsen
>
> Instituto de Pesquisas ELDORADO<http://clickemailmkt.eldorado.org.br/ls/click?upn=UPoxpeIcHnAcbUZyo7TTaswyiVb1TXP3jEbQqiiJKKGsxOn8hBEs5ZsMLQfXkKuKXZ7MVDg0ij9eG8HV4TXI75dBzDiNGLxQ8Xx5PzCVNt6TpGrzBbU-2Biu0o69X5ce-2FW-2FOk1uUipuK0fZnWXJEgbRw-3D-3DJY4T_wWk-2BG6VvNBoa1YzxYjhCdFS9IfANIaBzDSklR1NyyrKOI1wj0P-2BdBFcuO4FnHcsA1MyHu0ly1Yt3oDMp7KKdJPM68iKuI2jiRH5v4B0d8wf3chU3qy5n5iXWnW1QjSaNFHOgELzxaP-2FnesTeBgJ5dFkjH4f279sVQpOtyjw5xAqj34M6pgNRAxVvuXif4IWDcVzXg1FzfYlEfkKzr9vvpA3Hg8kitwMtlU3zwbQUBCgL30fQoJPcRPMGKyOY8RmoAlXNqTJYDYIvqmfnI7KLUvw6vKB5R-2B5q1FJRAzX7H-2BmF0NnDET6jMLuIqtCcVIch>
>
> Departamento Computação Embarcada
>
> Analista de Software Trainee
>
> Aviso Legal - Disclaimer<https://www.eldorado.org.br/disclaimer.html>