All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [RFC PATCH 3/4] target/ppc: Move SPR generation to separate file
@ 2021-04-28 14:47 Bruno Piazera Larsen
  2021-04-28 19:45 ` Fabiano Rosas
  2021-04-29  4:24 ` David Gibson
  0 siblings, 2 replies; 6+ messages in thread
From: Bruno Piazera Larsen @ 2021-04-28 14:47 UTC (permalink / raw)
  To: David Gibson, Fabiano Rosas
  Cc: Luis Fernando Fujita Pires, qemu-devel,
	Lucas Mateus Martins Araujo e Castro, Fernando Eckhardt Valle,
	qemu-ppc, Matheus Kowalczuk Ferst

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

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

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

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

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [RFC PATCH 0/4] target/ppc: code motion to compile translate_init
@ 2021-04-23 19:18 Bruno Larsen (billionai)
  2021-04-23 19:18 ` [RFC PATCH 3/4] target/ppc: Move SPR generation to separate file Bruno Larsen (billionai)
  0 siblings, 1 reply; 6+ messages in thread
From: Bruno Larsen (billionai) @ 2021-04-23 19:18 UTC (permalink / raw)
  To: qemu-devel
  Cc: farosas, luis.pires, lucas.araujo, fernando.valle, qemu-ppc,
	Bruno Larsen (billionai),
	matheus.ferst, david

The current patch series aims to isolate common code from translation-related
code. This isolation is required to disable TCG at build time, and the
final system still work.

This patch series is still WIP, so comments are welcome

Bruno Larsen (billionai) (4):
  target/ppc: move opcode table logic to translate.c
  target/ppc: isolated SPR read/write callbacks
  target/ppc: Move SPR generation to separate file
  target/ppc: isolated cpu init from translation logic

 .../ppc/{translate_init.c.inc => cpu_init.c}  | 5414 +----------------
 target/ppc/internal.h                         |  114 +
 target/ppc/meson.build                        |    2 +
 target/ppc/spr_common.c                       | 2943 +++++++++
 target/ppc/spr_tcg.c.inc                      | 1002 +++
 target/ppc/spr_tcg.h                          |  132 +
 target/ppc/translate.c                        |  446 +-
 7 files changed, 4863 insertions(+), 5190 deletions(-)
 rename target/ppc/{translate_init.c.inc => cpu_init.c} (50%)
 create mode 100644 target/ppc/spr_common.c
 create mode 100644 target/ppc/spr_tcg.c.inc
 create mode 100644 target/ppc/spr_tcg.h

-- 
2.17.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-04-29  4:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-28 14:47 [RFC PATCH 3/4] target/ppc: Move SPR generation to separate file Bruno Piazera Larsen
2021-04-28 19:45 ` Fabiano Rosas
2021-04-29  4:24 ` David Gibson
  -- strict thread matches above, loose matches on Subject: below --
2021-04-23 19:18 [RFC PATCH 0/4] target/ppc: code motion to compile translate_init Bruno Larsen (billionai)
2021-04-23 19:18 ` [RFC PATCH 3/4] target/ppc: Move SPR generation to separate file Bruno Larsen (billionai)
2021-04-26 21:08   ` Fabiano Rosas
2021-04-27  3:35     ` David Gibson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.