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

[Fabiano Rosas]

"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);

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