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

[David Gibson]

On Wed, Apr 28, 2021 at 02:47:37PM +0000, Bruno Piazera Larsen wrote:
> > > > 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
> 
> Right, ok. I thought we agreed to separate, but I can't remember the reason.
> I guess less is more in this case, so I won't create the new file.
> As for separating by CPU family: sounds good for a later cleanup, but I don't
> think it's a priority right now.

That's fair.

> I'll work on that renaming, I agree its a good idea.

Great.  A head's up, there are also patches in preparation which add
64-bit instruction support.  Those are also replacing some of our
existing instruction decode logic with the new somewhat generic
decodetree system.  That seems to be replacing the gen_*() convention
for TCG op generating functions with trans_*() for the new style
decodetree op generating functions.

Nonetheless, it's worth renaming the SPR construction functions to
something that won't collide with either the old or new convention.
However, since you're working in adjacent areas, one or both of you
are likely to have to do some rebasing and conflict fixing along the
way.

> > > > 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.
> 
> Ok. Should it be 2 commits (refactoring, then macro) or 3 commits (renaming,
> vscr_init, then macro)? I guess also that since I won't move stuff, I don't
> need to fix the vscr_init, but it's no big deal at this point.

3 please.  In general, lean towards more, simpler commits.

> > > 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.
> 
> yeah... sorry about that. I'm correcting all these problems.
> 
> > > > +/*****************************************************************************/
> > > > +/* 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), 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.
> 
> 
> 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>

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