Re: [RFC PATCH v2 2/2] hw/ppc: Moved TCG code to spapr_hcall_tcg

[David Gibson]

On Fri, Apr 30, 2021 at 08:38:05PM -0300, Fabiano Rosas wrote:
> "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br> writes:
> 
> > Also spapr_hcall_tcg.c only has 2 duplicated functions (valid_ptex and
> > is_ram_address), what is the advised way to deal with these
> > duplications?
> 
> valid_ptex is only needed by the TCG hcalls isn't it?
> 
> is_ram_address could in theory stay where it is but be exposed via
> hw/ppc/spapr.h since spapr_hcall.c will always be present.
> 
> > Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
> > ---
> >  hw/ppc/meson.build       |   3 +
> >  hw/ppc/spapr_hcall.c     | 300 ++--------------------------------
> >  hw/ppc/spapr_hcall_tcg.c | 343 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 363 insertions(+), 283 deletions(-)
> >  create mode 100644 hw/ppc/spapr_hcall_tcg.c
> 
> <snip>
> 
> > @@ -2021,14 +1752,17 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
> > target_ulong opcode,
> >  
> >  static void hypercall_register_types(void)
> >  {
> > +
> > +#ifndef CONFIG_TCG
> >      /* hcall-pft */
> > -    spapr_register_hypercall(H_ENTER, h_enter);
> > -    spapr_register_hypercall(H_REMOVE, h_remove);
> > -    spapr_register_hypercall(H_PROTECT, h_protect);
> > -    spapr_register_hypercall(H_READ, h_read);
> > +    spapr_register_hypercall(H_ENTER, h_tcg_only);
> > +    spapr_register_hypercall(H_REMOVE, h_tcg_only);
> > +    spapr_register_hypercall(H_PROTECT, h_tcg_only);
> > +    spapr_register_hypercall(H_READ, h_tcg_only);
> >  
> >      /* hcall-bulk */
> > -    spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);
> > +    spapr_register_hypercall(H_BULK_REMOVE, h_tcg_only);
> > +#endif /* !CONFIG_TCG */
> 
> My suggestion for this was:
> 
> #ifndef CONFIG_TCG
> static target_ulong h_tcg_only(PowerPCCPU *cpu, SpaprMachineState *spapr,
>                                target_ulong opcode, target_ulong *args)
> {
>     g_assert_not_reached();
> }
> 
> static void hypercall_register_tcg(void)
> {
>     spapr_register_hypercall(H_ENTER, h_tcg_only);
>     spapr_register_hypercall(H_REMOVE, h_tcg_only);
>     spapr_register_hypercall(H_PROTECT, h_tcg_only);
>     spapr_register_hypercall(H_READ, h_tcg_only);
>     (...)
> }
> #endif
> 
> static void hypercall_register_types(void)
> {
>     hypercall_register_tcg();
> 
>     <register KVM hcalls>
> }
> type_init(hypercall_register_types);

Eh, swings and roundabouts.  Either of these approaches is fine.

> > +static void hypercall_register_types(void)
> > +{
> > +    /* hcall-pft */
> > +    spapr_register_hypercall(H_ENTER, h_enter);
> > +    spapr_register_hypercall(H_REMOVE, h_remove);
> > +    spapr_register_hypercall(H_PROTECT, h_protect);
> > +    spapr_register_hypercall(H_READ, h_read);
> > +
> > +    /* hcall-bulk */
> > +    spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);
> > +}
> > +
> > +type_init(hypercall_register_types)
> 
> And here:
> 
> void hypercall_register_tcg(void)
> {
>     /* hcall-pft */
>     spapr_register_hypercall(H_ENTER, h_enter);
>     spapr_register_hypercall(H_REMOVE, h_remove);
>     spapr_register_hypercall(H_PROTECT, h_protect);
>     spapr_register_hypercall(H_READ, h_read);
>     (...)
> }
> 
> Because the TCG and KVM builds are not mutually exlusive, so you would
> end up calling type_init twice (which I don't know much about but I
> assume is not allowed).
> 

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