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

Message ID	20210430184047.81653-3-lucas.araujo@eldorado.org.br (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=L1kx=J3=nongnu.org=qemu-devel-bounces+qemu-devel=archiver.kernel.org@kernel.org> DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6DCDA6146D From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br> To: qemu-devel@nongnu.org, qemu-ppc@nongnu.org Subject: [RFC PATCH v2 2/2] hw/ppc: Moved TCG code to spapr_hcall_tcg Date: Fri, 30 Apr 2021 15:40:47 -0300 Message-Id: <20210430184047.81653-3-lucas.araujo@eldorado.org.br> In-Reply-To: <20210430184047.81653-1-lucas.araujo@eldorado.org.br> References: <20210430184047.81653-1-lucas.araujo@eldorado.org.br> Received-SPF: pass client-ip=201.28.113.2; envelope-from=lucas.araujo@eldorado.org.br; helo=outlook.eldorado.org.br X-Spam_score_int: -10 X-Spam_score: -1.1 X-Spam_bar: - X-Spam_report: (-1.1 / 5.0 requ) BAYES_00=-1.9, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action Precedence: list Cc: bruno.larsen@eldorado.org.br, "Lucas Mateus Castro \(alqotel\)" <lucas.araujo@eldorado.org.br>, farosas@linux.ibm.com, david@gibson.dropbear.id.au Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org>
Series	hw/ppc: code motion to compile without TCG \| expand [RFC,v2,0/2] hw/ppc: code motion to compile without TCG [RFC,v2,1/2] target/ppc: Moved functions out of mmu-hash64 [RFC,v2,2/2] hw/ppc: Moved TCG code to spapr_hcall_tcg

Lucas Mateus Martins Araujo e Castro April 30, 2021, 6:40 p.m. UTC

Moved h_enter, remove_hpte, h_remove, h_bulk_remove,h_protect and
h_read to spapr_hcall_tcg.c, added h_tcg_only to be used in a !TCG
environment in spapr_hcall.c and changed build options to only build
spapr_hcall_tcg.c when CONFIG_TCG is enabled.

Added the function h_tcg_only to be used for hypercalls that should be
called only in TCG environment but have been called in a TCG-less one.

Right now, a #ifndef is used to know if there is a need of a h_tcg_only
function to be implemented and used as hypercalls, I initially thought
of always having that option turned on and having spapr_hcall_tcg.c
overwrite those hypercalls when TCG is enabled, but
spapr_register_hypercalls checks if a hypercall already exists for that
opcode so that wouldn't work, so if anyone has any suggestions I'm
interested.

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?

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

Fabiano Rosas April 30, 2021, 11:38 p.m. UTC | #1

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

David Gibson May 3, 2021, 4:34 a.m. UTC | #2

On Fri, Apr 30, 2021 at 03:40:47PM -0300, Lucas Mateus Castro (alqotel) wrote:
> Moved h_enter, remove_hpte, h_remove, h_bulk_remove,h_protect and
> h_read to spapr_hcall_tcg.c, added h_tcg_only to be used in a !TCG
> environment in spapr_hcall.c and changed build options to only build
> spapr_hcall_tcg.c when CONFIG_TCG is enabled.

This looks good.  I'd suggest the name 'spapr_softmmu.c' instead,
which more specifically describes what's special about these
functions.

h_resize_hpt_prepare(), h_resize_hpt_commit() and the functions they
depend on belong in the softmmu set as well.

> Added the function h_tcg_only to be used for hypercalls that should be
> called only in TCG environment but have been called in a TCG-less
> one.

Again, 'h_softmmu' would be a better name.

> 
> Right now, a #ifndef is used to know if there is a need of a h_tcg_only
> function to be implemented and used as hypercalls, I initially thought
> of always having that option turned on and having spapr_hcall_tcg.c
> overwrite those hypercalls when TCG is enabled, but
> spapr_register_hypercalls checks if a hypercall already exists for that
> opcode so that wouldn't work, so if anyone has any suggestions I'm
> interested.

The ifdef is fine.  We don't want to litter the code with them, but a
few is fine.  Especially in this context where it's pretty clearly
just excluding some things from a simple list of calls.

> 
> 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 used by softmmu functions that are moving, so it
should travel with them, no need for duplication.  is_ram_address() is
also used by h_page_init() which is also needed in !TCG code.  So,
leave it in spapr_hcall.c and just export it for use in the TCG only
code.

> 
> 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
> 
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 218631c883..3c7f2f08b7 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -29,6 +29,9 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
>    'spapr_numa.c',
>    'pef.c',
>  ))
> +ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
> +  'spapr_hcall_tcg.c'
> +))
>  ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c'))
>  ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files(
>    'spapr_pci_vfio.c',
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 4b0ba69841..b37fbdc32e 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -22,6 +22,15 @@
>  #include "mmu-book3s-v3.h"
>  #include "hw/mem/memory-device.h"
>  
> +#ifndef CONFIG_TCG
> +static target_ulong h_tcg_only(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                            target_ulong opcode, target_ulong *args)
> +{
> +    g_assert_not_reached();
> +    return 0;
> +}
> +#endif /* !CONFIG_TCG */
> +
>  static bool has_spr(PowerPCCPU *cpu, int spr)
>  {
>      /* We can test whether the SPR is defined by checking for a valid name */
> @@ -55,284 +64,6 @@ static bool is_ram_address(SpaprMachineState *spapr, hwaddr addr)
>      return false;
>  }
>  
> -static target_ulong h_enter(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                            target_ulong opcode, target_ulong *args)
> -{
> -    target_ulong flags = args[0];
> -    target_ulong ptex = args[1];
> -    target_ulong pteh = args[2];
> -    target_ulong ptel = args[3];
> -    unsigned apshift;
> -    target_ulong raddr;
> -    target_ulong slot;
> -    const ppc_hash_pte64_t *hptes;
> -
> -    apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
> -    if (!apshift) {
> -        /* Bad page size encoding */
> -        return H_PARAMETER;
> -    }
> -
> -    raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << apshift) - 1);
> -
> -    if (is_ram_address(spapr, raddr)) {
> -        /* Regular RAM - should have WIMG=0010 */
> -        if ((ptel & HPTE64_R_WIMG) != HPTE64_R_M) {
> -            return H_PARAMETER;
> -        }
> -    } else {
> -        target_ulong wimg_flags;
> -        /* Looks like an IO address */
> -        /* FIXME: What WIMG combinations could be sensible for IO?
> -         * For now we allow WIMG=010x, but are there others? */
> -        /* FIXME: Should we check against registered IO addresses? */
> -        wimg_flags = (ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M));
> -
> -        if (wimg_flags != HPTE64_R_I &&
> -            wimg_flags != (HPTE64_R_I | HPTE64_R_M)) {
> -            return H_PARAMETER;
> -        }
> -    }
> -
> -    pteh &= ~0x60ULL;
> -
> -    if (!valid_ptex(cpu, ptex)) {
> -        return H_PARAMETER;
> -    }
> -
> -    slot = ptex & 7ULL;
> -    ptex = ptex & ~7ULL;
> -
> -    if (likely((flags & H_EXACT) == 0)) {
> -        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> -        for (slot = 0; slot < 8; slot++) {
> -            if (!(ppc_hash64_hpte0(cpu, hptes, slot) & HPTE64_V_VALID)) {
> -                break;
> -            }
> -        }
> -        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
> -        if (slot == 8) {
> -            return H_PTEG_FULL;
> -        }
> -    } else {
> -        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
> -        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
> -            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
> -            return H_PTEG_FULL;
> -        }
> -        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> -    }
> -
> -    spapr_store_hpte(cpu, ptex + slot, pteh | HPTE64_V_HPTE_DIRTY, ptel);
> -
> -    args[0] = ptex + slot;
> -    return H_SUCCESS;
> -}
> -
> -typedef enum {
> -    REMOVE_SUCCESS = 0,
> -    REMOVE_NOT_FOUND = 1,
> -    REMOVE_PARM = 2,
> -    REMOVE_HW = 3,
> -} RemoveResult;
> -
> -static RemoveResult remove_hpte(PowerPCCPU *cpu
> -                                , target_ulong ptex,
> -                                target_ulong avpn,
> -                                target_ulong flags,
> -                                target_ulong *vp, target_ulong *rp)
> -{
> -    const ppc_hash_pte64_t *hptes;
> -    target_ulong v, r;
> -
> -    if (!valid_ptex(cpu, ptex)) {
> -        return REMOVE_PARM;
> -    }
> -
> -    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> -    v = ppc_hash64_hpte0(cpu, hptes, 0);
> -    r = ppc_hash64_hpte1(cpu, hptes, 0);
> -    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> -
> -    if ((v & HPTE64_V_VALID) == 0 ||
> -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> -        return REMOVE_NOT_FOUND;
> -    }
> -    *vp = v;
> -    *rp = r;
> -    spapr_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
> -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> -    return REMOVE_SUCCESS;
> -}
> -
> -static target_ulong h_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                             target_ulong opcode, target_ulong *args)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    target_ulong flags = args[0];
> -    target_ulong ptex = args[1];
> -    target_ulong avpn = args[2];
> -    RemoveResult ret;
> -
> -    ret = remove_hpte(cpu, ptex, avpn, flags,
> -                      &args[0], &args[1]);
> -
> -    switch (ret) {
> -    case REMOVE_SUCCESS:
> -        check_tlb_flush(env, true);
> -        return H_SUCCESS;
> -
> -    case REMOVE_NOT_FOUND:
> -        return H_NOT_FOUND;
> -
> -    case REMOVE_PARM:
> -        return H_PARAMETER;
> -
> -    case REMOVE_HW:
> -        return H_HARDWARE;
> -    }
> -
> -    g_assert_not_reached();
> -}
> -
> -#define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
> -#define   H_BULK_REMOVE_REQUEST        0x4000000000000000ULL
> -#define   H_BULK_REMOVE_RESPONSE       0x8000000000000000ULL
> -#define   H_BULK_REMOVE_END            0xc000000000000000ULL
> -#define H_BULK_REMOVE_CODE             0x3000000000000000ULL
> -#define   H_BULK_REMOVE_SUCCESS        0x0000000000000000ULL
> -#define   H_BULK_REMOVE_NOT_FOUND      0x1000000000000000ULL
> -#define   H_BULK_REMOVE_PARM           0x2000000000000000ULL
> -#define   H_BULK_REMOVE_HW             0x3000000000000000ULL
> -#define H_BULK_REMOVE_RC               0x0c00000000000000ULL
> -#define H_BULK_REMOVE_FLAGS            0x0300000000000000ULL
> -#define   H_BULK_REMOVE_ABSOLUTE       0x0000000000000000ULL
> -#define   H_BULK_REMOVE_ANDCOND        0x0100000000000000ULL
> -#define   H_BULK_REMOVE_AVPN           0x0200000000000000ULL
> -#define H_BULK_REMOVE_PTEX             0x00ffffffffffffffULL
> -
> -#define H_BULK_REMOVE_MAX_BATCH        4
> -
> -static target_ulong h_bulk_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                                  target_ulong opcode, target_ulong *args)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    int i;
> -    target_ulong rc = H_SUCCESS;
> -
> -    for (i = 0; i < H_BULK_REMOVE_MAX_BATCH; i++) {
> -        target_ulong *tsh = &args[i*2];
> -        target_ulong tsl = args[i*2 + 1];
> -        target_ulong v, r, ret;
> -
> -        if ((*tsh & H_BULK_REMOVE_TYPE) == H_BULK_REMOVE_END) {
> -            break;
> -        } else if ((*tsh & H_BULK_REMOVE_TYPE) != H_BULK_REMOVE_REQUEST) {
> -            return H_PARAMETER;
> -        }
> -
> -        *tsh &= H_BULK_REMOVE_PTEX | H_BULK_REMOVE_FLAGS;
> -        *tsh |= H_BULK_REMOVE_RESPONSE;
> -
> -        if ((*tsh & H_BULK_REMOVE_ANDCOND) && (*tsh & H_BULK_REMOVE_AVPN)) {
> -            *tsh |= H_BULK_REMOVE_PARM;
> -            return H_PARAMETER;
> -        }
> -
> -        ret = remove_hpte(cpu, *tsh & H_BULK_REMOVE_PTEX, tsl,
> -                          (*tsh & H_BULK_REMOVE_FLAGS) >> 26,
> -                          &v, &r);
> -
> -        *tsh |= ret << 60;
> -
> -        switch (ret) {
> -        case REMOVE_SUCCESS:
> -            *tsh |= (r & (HPTE64_R_C | HPTE64_R_R)) << 43;
> -            break;
> -
> -        case REMOVE_PARM:
> -            rc = H_PARAMETER;
> -            goto exit;
> -
> -        case REMOVE_HW:
> -            rc = H_HARDWARE;
> -            goto exit;
> -        }
> -    }
> - exit:
> -    check_tlb_flush(env, true);
> -
> -    return rc;
> -}
> -
> -static target_ulong h_protect(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                              target_ulong opcode, target_ulong *args)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    target_ulong flags = args[0];
> -    target_ulong ptex = args[1];
> -    target_ulong avpn = args[2];
> -    const ppc_hash_pte64_t *hptes;
> -    target_ulong v, r;
> -
> -    if (!valid_ptex(cpu, ptex)) {
> -        return H_PARAMETER;
> -    }
> -
> -    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> -    v = ppc_hash64_hpte0(cpu, hptes, 0);
> -    r = ppc_hash64_hpte1(cpu, hptes, 0);
> -    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> -
> -    if ((v & HPTE64_V_VALID) == 0 ||
> -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> -        return H_NOT_FOUND;
> -    }
> -
> -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> -    r |= (flags << 55) & HPTE64_R_PP0;
> -    r |= (flags << 48) & HPTE64_R_KEY_HI;
> -    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
> -    spapr_store_hpte(cpu, ptex,
> -                     (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
> -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> -    /* Flush the tlb */
> -    check_tlb_flush(env, true);
> -    /* Don't need a memory barrier, due to qemu's global lock */
> -    spapr_store_hpte(cpu, ptex, v | HPTE64_V_HPTE_DIRTY, r);
> -    return H_SUCCESS;
> -}
> -
> -static target_ulong h_read(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                           target_ulong opcode, target_ulong *args)
> -{
> -    target_ulong flags = args[0];
> -    target_ulong ptex = args[1];
> -    int i, ridx, n_entries = 1;
> -    const ppc_hash_pte64_t *hptes;
> -
> -    if (!valid_ptex(cpu, ptex)) {
> -        return H_PARAMETER;
> -    }
> -
> -    if (flags & H_READ_4) {
> -        /* Clear the two low order bits */
> -        ptex &= ~(3ULL);
> -        n_entries = 4;
> -    }
> -
> -    hptes = ppc_hash64_map_hptes(cpu, ptex, n_entries);
> -    for (i = 0, ridx = 0; i < n_entries; i++) {
> -        args[ridx++] = ppc_hash64_hpte0(cpu, hptes, i);
> -        args[ridx++] = ppc_hash64_hpte1(cpu, hptes, i);
> -    }
> -    ppc_hash64_unmap_hptes(cpu, hptes, ptex, n_entries);
> -
> -    return H_SUCCESS;
> -}
> -
>  struct SpaprPendingHpt {
>      /* These fields are read-only after initialization */
>      int shift;
> @@ -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 */
>  
>      /* hcall-hpt-resize */
>      spapr_register_hypercall(H_RESIZE_HPT_PREPARE, h_resize_hpt_prepare);
> diff --git a/hw/ppc/spapr_hcall_tcg.c b/hw/ppc/spapr_hcall_tcg.c
> new file mode 100644
> index 0000000000..92ff24c8dc
> --- /dev/null
> +++ b/hw/ppc/spapr_hcall_tcg.c
> @@ -0,0 +1,343 @@
> +#include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> +#include "qapi/error.h"
> +#include "sysemu/hw_accel.h"
> +#include "sysemu/runstate.h"
> +#include "qemu/log.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/module.h"
> +#include "qemu/error-report.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +#include "helper_regs.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> +#include "mmu-hash64.h"
> +#include "mmu-misc.h"
> +#include "cpu-models.h"
> +#include "trace.h"
> +#include "kvm_ppc.h"
> +#include "hw/ppc/fdt.h"
> +#include "hw/ppc/spapr_ovec.h"
> +#include "mmu-book3s-v3.h"
> +#include "hw/mem/memory-device.h"
> +
> +static inline bool valid_ptex(PowerPCCPU *cpu, target_ulong ptex)
> +{
> +    /*
> +     * hash value/pteg group index is normalized by HPT mask
> +     */
> +    if (((ptex & ~7ULL) / HPTES_PER_GROUP) & ~ppc_hash64_hpt_mask(cpu)) {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static bool is_ram_address(SpaprMachineState *spapr, hwaddr addr)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    DeviceMemoryState *dms = machine->device_memory;
> +
> +    if (addr < machine->ram_size) {
> +        return true;
> +    }
> +    if ((addr >= dms->base)
> +        && ((addr - dms->base) < memory_region_size(&dms->mr))) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static target_ulong h_enter(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                            target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong flags = args[0];
> +    target_ulong ptex = args[1];
> +    target_ulong pteh = args[2];
> +    target_ulong ptel = args[3];
> +    unsigned apshift;
> +    target_ulong raddr;
> +    target_ulong slot;
> +    const ppc_hash_pte64_t *hptes;
> +
> +    apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
> +    if (!apshift) {
> +        /* Bad page size encoding */
> +        return H_PARAMETER;
> +    }
> +
> +    raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << apshift) - 1);
> +
> +    if (is_ram_address(spapr, raddr)) {
> +        /* Regular RAM - should have WIMG=0010 */
> +        if ((ptel & HPTE64_R_WIMG) != HPTE64_R_M) {
> +            return H_PARAMETER;
> +        }
> +    } else {
> +        target_ulong wimg_flags;
> +        /* Looks like an IO address */
> +        /* FIXME: What WIMG combinations could be sensible for IO?
> +         * For now we allow WIMG=010x, but are there others? */
> +        /* FIXME: Should we check against registered IO addresses? */
> +        wimg_flags = (ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M));
> +
> +        if (wimg_flags != HPTE64_R_I &&
> +            wimg_flags != (HPTE64_R_I | HPTE64_R_M)) {
> +            return H_PARAMETER;
> +        }
> +    }
> +
> +    pteh &= ~0x60ULL;
> +
> +    if (!valid_ptex(cpu, ptex)) {
> +        return H_PARAMETER;
> +    }
> +
> +    slot = ptex & 7ULL;
> +    ptex = ptex & ~7ULL;
> +
> +    if (likely((flags & H_EXACT) == 0)) {
> +        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
> +        for (slot = 0; slot < 8; slot++) {
> +            if (!(ppc_hash64_hpte0(cpu, hptes, slot) & HPTE64_V_VALID)) {
> +                break;
> +            }
> +        }
> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
> +        if (slot == 8) {
> +            return H_PTEG_FULL;
> +        }
> +    } else {
> +        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
> +        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
> +            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
> +            return H_PTEG_FULL;
> +        }
> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> +    }
> +
> +    spapr_store_hpte(cpu, ptex + slot, pteh | HPTE64_V_HPTE_DIRTY, ptel);
> +
> +    args[0] = ptex + slot;
> +    return H_SUCCESS;
> +}
> +
> +typedef enum {
> +    REMOVE_SUCCESS = 0,
> +    REMOVE_NOT_FOUND = 1,
> +    REMOVE_PARM = 2,
> +    REMOVE_HW = 3,
> +} RemoveResult;
> +
> +static RemoveResult remove_hpte(PowerPCCPU *cpu
> +                                , target_ulong ptex,
> +                                target_ulong avpn,
> +                                target_ulong flags,
> +                                target_ulong *vp, target_ulong *rp)
> +{
> +    const ppc_hash_pte64_t *hptes;
> +    target_ulong v, r;
> +
> +    if (!valid_ptex(cpu, ptex)) {
> +        return REMOVE_PARM;
> +    }
> +
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> +
> +    if ((v & HPTE64_V_VALID) == 0 ||
> +        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
> +        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
> +        return REMOVE_NOT_FOUND;
> +    }
> +    *vp = v;
> +    *rp = r;
> +    spapr_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
> +    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> +    return REMOVE_SUCCESS;
> +}
> +
> +static target_ulong h_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                             target_ulong opcode, target_ulong *args)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong flags = args[0];
> +    target_ulong ptex = args[1];
> +    target_ulong avpn = args[2];
> +    RemoveResult ret;
> +
> +    ret = remove_hpte(cpu, ptex, avpn, flags,
> +                      &args[0], &args[1]);
> +
> +    switch (ret) {
> +    case REMOVE_SUCCESS:
> +        check_tlb_flush(env, true);
> +        return H_SUCCESS;
> +
> +    case REMOVE_NOT_FOUND:
> +        return H_NOT_FOUND;
> +
> +    case REMOVE_PARM:
> +        return H_PARAMETER;
> +
> +    case REMOVE_HW:
> +        return H_HARDWARE;
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
> +#define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
> +#define   H_BULK_REMOVE_REQUEST        0x4000000000000000ULL
> +#define   H_BULK_REMOVE_RESPONSE       0x8000000000000000ULL
> +#define   H_BULK_REMOVE_END            0xc000000000000000ULL
> +#define H_BULK_REMOVE_CODE             0x3000000000000000ULL
> +#define   H_BULK_REMOVE_SUCCESS        0x0000000000000000ULL
> +#define   H_BULK_REMOVE_NOT_FOUND      0x1000000000000000ULL
> +#define   H_BULK_REMOVE_PARM           0x2000000000000000ULL
> +#define   H_BULK_REMOVE_HW             0x3000000000000000ULL
> +#define H_BULK_REMOVE_RC               0x0c00000000000000ULL
> +#define H_BULK_REMOVE_FLAGS            0x0300000000000000ULL
> +#define   H_BULK_REMOVE_ABSOLUTE       0x0000000000000000ULL
> +#define   H_BULK_REMOVE_ANDCOND        0x0100000000000000ULL
> +#define   H_BULK_REMOVE_AVPN           0x0200000000000000ULL
> +#define H_BULK_REMOVE_PTEX             0x00ffffffffffffffULL
> +
> +#define H_BULK_REMOVE_MAX_BATCH        4
> +
> +static target_ulong h_bulk_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                                  target_ulong opcode, target_ulong *args)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    int i;
> +    target_ulong rc = H_SUCCESS;
> +
> +    for (i = 0; i < H_BULK_REMOVE_MAX_BATCH; i++) {
> +        target_ulong *tsh = &args[i*2];
> +        target_ulong tsl = args[i*2 + 1];
> +        target_ulong v, r, ret;
> +
> +        if ((*tsh & H_BULK_REMOVE_TYPE) == H_BULK_REMOVE_END) {
> +            break;
> +        } else if ((*tsh & H_BULK_REMOVE_TYPE) != H_BULK_REMOVE_REQUEST) {
> +            return H_PARAMETER;
> +        }
> +
> +        *tsh &= H_BULK_REMOVE_PTEX | H_BULK_REMOVE_FLAGS;
> +        *tsh |= H_BULK_REMOVE_RESPONSE;
> +
> +        if ((*tsh & H_BULK_REMOVE_ANDCOND) && (*tsh & H_BULK_REMOVE_AVPN)) {
> +            *tsh |= H_BULK_REMOVE_PARM;
> +            return H_PARAMETER;
> +        }
> +
> +        ret = remove_hpte(cpu, *tsh & H_BULK_REMOVE_PTEX, tsl,
> +                          (*tsh & H_BULK_REMOVE_FLAGS) >> 26,
> +                          &v, &r);
> +
> +        *tsh |= ret << 60;
> +
> +        switch (ret) {
> +        case REMOVE_SUCCESS:
> +            *tsh |= (r & (HPTE64_R_C | HPTE64_R_R)) << 43;
> +            break;
> +
> +        case REMOVE_PARM:
> +            rc = H_PARAMETER;
> +            goto exit;
> +
> +        case REMOVE_HW:
> +            rc = H_HARDWARE;
> +            goto exit;
> +        }
> +    }
> + exit:
> +    check_tlb_flush(env, true);
> +
> +    return rc;
> +}
> +
> +static target_ulong h_protect(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                              target_ulong opcode, target_ulong *args)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong flags = args[0];
> +    target_ulong ptex = args[1];
> +    target_ulong avpn = args[2];
> +    const ppc_hash_pte64_t *hptes;
> +    target_ulong v, r;
> +
> +    if (!valid_ptex(cpu, ptex)) {
> +        return H_PARAMETER;
> +    }
> +
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
> +
> +    if ((v & HPTE64_V_VALID) == 0 ||
> +        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
> +        return H_NOT_FOUND;
> +    }
> +
> +    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
> +           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
> +    r |= (flags << 55) & HPTE64_R_PP0;
> +    r |= (flags << 48) & HPTE64_R_KEY_HI;
> +    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
> +    spapr_store_hpte(cpu, ptex,
> +                     (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
> +    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
> +    /* Flush the tlb */
> +    check_tlb_flush(env, true);
> +    /* Don't need a memory barrier, due to qemu's global lock */
> +    spapr_store_hpte(cpu, ptex, v | HPTE64_V_HPTE_DIRTY, r);
> +    return H_SUCCESS;
> +}
> +
> +static target_ulong h_read(PowerPCCPU *cpu, SpaprMachineState *spapr,
> +                           target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong flags = args[0];
> +    target_ulong ptex = args[1];
> +    int i, ridx, n_entries = 1;
> +    const ppc_hash_pte64_t *hptes;
> +
> +    if (!valid_ptex(cpu, ptex)) {
> +        return H_PARAMETER;
> +    }
> +
> +    if (flags & H_READ_4) {
> +        /* Clear the two low order bits */
> +        ptex &= ~(3ULL);
> +        n_entries = 4;
> +    }
> +
> +    hptes = ppc_hash64_map_hptes(cpu, ptex, n_entries);
> +    for (i = 0, ridx = 0; i < n_entries; i++) {
> +        args[ridx++] = ppc_hash64_hpte0(cpu, hptes, i);
> +        args[ridx++] = ppc_hash64_hpte1(cpu, hptes, i);
> +    }
> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, n_entries);
> +
> +    return H_SUCCESS;
> +}
> +
> +
> +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)

David Gibson May 3, 2021, 4:35 a.m. UTC | #3

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

Lucas Mateus Martins Araujo e Castro May 4, 2021, 6:14 p.m. UTC | #4

On 03/05/2021 01:34, David Gibson wrote:
> On Fri, Apr 30, 2021 at 03:40:47PM -0300, Lucas Mateus Castro (alqotel) wrote:
>> Moved h_enter, remove_hpte, h_remove, h_bulk_remove,h_protect and
>> h_read to spapr_hcall_tcg.c, added h_tcg_only to be used in a !TCG
>> environment in spapr_hcall.c and changed build options to only build
>> spapr_hcall_tcg.c when CONFIG_TCG is enabled.
> This looks good.  I'd suggest the name 'spapr_softmmu.c' instead,
> which more specifically describes what's special about these
> functions.
>
> h_resize_hpt_prepare(), h_resize_hpt_commit() and the functions they
> depend on belong in the softmmu set as well.

Moved these hcalls to spapr_softmmu.c as well, along with the most
functions they depend on, but 1 function (push_sregs_to_kvm_pr) is
also used by hcalls in spapr_hcall.c, so for now I'm just leaving it in
spapr_hcall.c and exporting it to be used in spapr_softmmu.c.

On a related note, from what I've seen h_resize_hpt_prepare and
h_resize_hpt_commit are not implementede in KVM, so they're only
called when there's softmmu so that's why they can be moved to
spapr_softmmu.c?
>> Added the function h_tcg_only to be used for hypercalls that should be
>> called only in TCG environment but have been called in a TCG-less
>> one.
> Again, 'h_softmmu' would be a better name.
Ok, I will use that name.
>> Right now, a #ifndef is used to know if there is a need of a h_tcg_only
>> function to be implemented and used as hypercalls, I initially thought
>> of always having that option turned on and having spapr_hcall_tcg.c
>> overwrite those hypercalls when TCG is enabled, but
>> spapr_register_hypercalls checks if a hypercall already exists for that
>> opcode so that wouldn't work, so if anyone has any suggestions I'm
>> interested.
> The ifdef is fine.  We don't want to litter the code with them, but a
> few is fine.  Especially in this context where it's pretty clearly
> just excluding some things from a simple list of calls.
>
>> 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 used by softmmu functions that are moving, so it
> should travel with them, no need for duplication.  is_ram_address() is
> also used by h_page_init() which is also needed in !TCG code.  So,
> leave it in spapr_hcall.c and just export it for use in the TCG only
> code.
>
On both is_ram_address and push_sregs_to_kvm_pr I exported them

by adding their prototypes to spapr.h and made them non-static.

>> 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
>>
>> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
>> index 218631c883..3c7f2f08b7 100644
>> --- a/hw/ppc/meson.build
>> +++ b/hw/ppc/meson.build
>> @@ -29,6 +29,9 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
>>     'spapr_numa.c',
>>     'pef.c',
>>   ))
>> +ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_TCG'], if_true: files(
>> +  'spapr_hcall_tcg.c'
>> +))
>>   ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c'))
>>   ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files(
>>     'spapr_pci_vfio.c',
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 4b0ba69841..b37fbdc32e 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -22,6 +22,15 @@
>>   #include "mmu-book3s-v3.h"
>>   #include "hw/mem/memory-device.h"
>>   
>> +#ifndef CONFIG_TCG
>> +static target_ulong h_tcg_only(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                            target_ulong opcode, target_ulong *args)
>> +{
>> +    g_assert_not_reached();
>> +    return 0;
>> +}
>> +#endif /* !CONFIG_TCG */
>> +
>>   static bool has_spr(PowerPCCPU *cpu, int spr)
>>   {
>>       /* We can test whether the SPR is defined by checking for a valid name */
>> @@ -55,284 +64,6 @@ static bool is_ram_address(SpaprMachineState *spapr, hwaddr addr)
>>       return false;
>>   }
>>   
>> -static target_ulong h_enter(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> -                            target_ulong opcode, target_ulong *args)
>> -{
>> -    target_ulong flags = args[0];
>> -    target_ulong ptex = args[1];
>> -    target_ulong pteh = args[2];
>> -    target_ulong ptel = args[3];
>> -    unsigned apshift;
>> -    target_ulong raddr;
>> -    target_ulong slot;
>> -    const ppc_hash_pte64_t *hptes;
>> -
>> -    apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
>> -    if (!apshift) {
>> -        /* Bad page size encoding */
>> -        return H_PARAMETER;
>> -    }
>> -
>> -    raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << apshift) - 1);
>> -
>> -    if (is_ram_address(spapr, raddr)) {
>> -        /* Regular RAM - should have WIMG=0010 */
>> -        if ((ptel & HPTE64_R_WIMG) != HPTE64_R_M) {
>> -            return H_PARAMETER;
>> -        }
>> -    } else {
>> -        target_ulong wimg_flags;
>> -        /* Looks like an IO address */
>> -        /* FIXME: What WIMG combinations could be sensible for IO?
>> -         * For now we allow WIMG=010x, but are there others? */
>> -        /* FIXME: Should we check against registered IO addresses? */
>> -        wimg_flags = (ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M));
>> -
>> -        if (wimg_flags != HPTE64_R_I &&
>> -            wimg_flags != (HPTE64_R_I | HPTE64_R_M)) {
>> -            return H_PARAMETER;
>> -        }
>> -    }
>> -
>> -    pteh &= ~0x60ULL;
>> -
>> -    if (!valid_ptex(cpu, ptex)) {
>> -        return H_PARAMETER;
>> -    }
>> -
>> -    slot = ptex & 7ULL;
>> -    ptex = ptex & ~7ULL;
>> -
>> -    if (likely((flags & H_EXACT) == 0)) {
>> -        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
>> -        for (slot = 0; slot < 8; slot++) {
>> -            if (!(ppc_hash64_hpte0(cpu, hptes, slot) & HPTE64_V_VALID)) {
>> -                break;
>> -            }
>> -        }
>> -        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
>> -        if (slot == 8) {
>> -            return H_PTEG_FULL;
>> -        }
>> -    } else {
>> -        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
>> -        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
>> -            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
>> -            return H_PTEG_FULL;
>> -        }
>> -        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>> -    }
>> -
>> -    spapr_store_hpte(cpu, ptex + slot, pteh | HPTE64_V_HPTE_DIRTY, ptel);
>> -
>> -    args[0] = ptex + slot;
>> -    return H_SUCCESS;
>> -}
>> -
>> -typedef enum {
>> -    REMOVE_SUCCESS = 0,
>> -    REMOVE_NOT_FOUND = 1,
>> -    REMOVE_PARM = 2,
>> -    REMOVE_HW = 3,
>> -} RemoveResult;
>> -
>> -static RemoveResult remove_hpte(PowerPCCPU *cpu
>> -                                , target_ulong ptex,
>> -                                target_ulong avpn,
>> -                                target_ulong flags,
>> -                                target_ulong *vp, target_ulong *rp)
>> -{
>> -    const ppc_hash_pte64_t *hptes;
>> -    target_ulong v, r;
>> -
>> -    if (!valid_ptex(cpu, ptex)) {
>> -        return REMOVE_PARM;
>> -    }
>> -
>> -    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
>> -    v = ppc_hash64_hpte0(cpu, hptes, 0);
>> -    r = ppc_hash64_hpte1(cpu, hptes, 0);
>> -    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>> -
>> -    if ((v & HPTE64_V_VALID) == 0 ||
>> -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
>> -        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
>> -        return REMOVE_NOT_FOUND;
>> -    }
>> -    *vp = v;
>> -    *rp = r;
>> -    spapr_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
>> -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
>> -    return REMOVE_SUCCESS;
>> -}
>> -
>> -static target_ulong h_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> -                             target_ulong opcode, target_ulong *args)
>> -{
>> -    CPUPPCState *env = &cpu->env;
>> -    target_ulong flags = args[0];
>> -    target_ulong ptex = args[1];
>> -    target_ulong avpn = args[2];
>> -    RemoveResult ret;
>> -
>> -    ret = remove_hpte(cpu, ptex, avpn, flags,
>> -                      &args[0], &args[1]);
>> -
>> -    switch (ret) {
>> -    case REMOVE_SUCCESS:
>> -        check_tlb_flush(env, true);
>> -        return H_SUCCESS;
>> -
>> -    case REMOVE_NOT_FOUND:
>> -        return H_NOT_FOUND;
>> -
>> -    case REMOVE_PARM:
>> -        return H_PARAMETER;
>> -
>> -    case REMOVE_HW:
>> -        return H_HARDWARE;
>> -    }
>> -
>> -    g_assert_not_reached();
>> -}
>> -
>> -#define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
>> -#define   H_BULK_REMOVE_REQUEST        0x4000000000000000ULL
>> -#define   H_BULK_REMOVE_RESPONSE       0x8000000000000000ULL
>> -#define   H_BULK_REMOVE_END            0xc000000000000000ULL
>> -#define H_BULK_REMOVE_CODE             0x3000000000000000ULL
>> -#define   H_BULK_REMOVE_SUCCESS        0x0000000000000000ULL
>> -#define   H_BULK_REMOVE_NOT_FOUND      0x1000000000000000ULL
>> -#define   H_BULK_REMOVE_PARM           0x2000000000000000ULL
>> -#define   H_BULK_REMOVE_HW             0x3000000000000000ULL
>> -#define H_BULK_REMOVE_RC               0x0c00000000000000ULL
>> -#define H_BULK_REMOVE_FLAGS            0x0300000000000000ULL
>> -#define   H_BULK_REMOVE_ABSOLUTE       0x0000000000000000ULL
>> -#define   H_BULK_REMOVE_ANDCOND        0x0100000000000000ULL
>> -#define   H_BULK_REMOVE_AVPN           0x0200000000000000ULL
>> -#define H_BULK_REMOVE_PTEX             0x00ffffffffffffffULL
>> -
>> -#define H_BULK_REMOVE_MAX_BATCH        4
>> -
>> -static target_ulong h_bulk_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> -                                  target_ulong opcode, target_ulong *args)
>> -{
>> -    CPUPPCState *env = &cpu->env;
>> -    int i;
>> -    target_ulong rc = H_SUCCESS;
>> -
>> -    for (i = 0; i < H_BULK_REMOVE_MAX_BATCH; i++) {
>> -        target_ulong *tsh = &args[i*2];
>> -        target_ulong tsl = args[i*2 + 1];
>> -        target_ulong v, r, ret;
>> -
>> -        if ((*tsh & H_BULK_REMOVE_TYPE) == H_BULK_REMOVE_END) {
>> -            break;
>> -        } else if ((*tsh & H_BULK_REMOVE_TYPE) != H_BULK_REMOVE_REQUEST) {
>> -            return H_PARAMETER;
>> -        }
>> -
>> -        *tsh &= H_BULK_REMOVE_PTEX | H_BULK_REMOVE_FLAGS;
>> -        *tsh |= H_BULK_REMOVE_RESPONSE;
>> -
>> -        if ((*tsh & H_BULK_REMOVE_ANDCOND) && (*tsh & H_BULK_REMOVE_AVPN)) {
>> -            *tsh |= H_BULK_REMOVE_PARM;
>> -            return H_PARAMETER;
>> -        }
>> -
>> -        ret = remove_hpte(cpu, *tsh & H_BULK_REMOVE_PTEX, tsl,
>> -                          (*tsh & H_BULK_REMOVE_FLAGS) >> 26,
>> -                          &v, &r);
>> -
>> -        *tsh |= ret << 60;
>> -
>> -        switch (ret) {
>> -        case REMOVE_SUCCESS:
>> -            *tsh |= (r & (HPTE64_R_C | HPTE64_R_R)) << 43;
>> -            break;
>> -
>> -        case REMOVE_PARM:
>> -            rc = H_PARAMETER;
>> -            goto exit;
>> -
>> -        case REMOVE_HW:
>> -            rc = H_HARDWARE;
>> -            goto exit;
>> -        }
>> -    }
>> - exit:
>> -    check_tlb_flush(env, true);
>> -
>> -    return rc;
>> -}
>> -
>> -static target_ulong h_protect(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> -                              target_ulong opcode, target_ulong *args)
>> -{
>> -    CPUPPCState *env = &cpu->env;
>> -    target_ulong flags = args[0];
>> -    target_ulong ptex = args[1];
>> -    target_ulong avpn = args[2];
>> -    const ppc_hash_pte64_t *hptes;
>> -    target_ulong v, r;
>> -
>> -    if (!valid_ptex(cpu, ptex)) {
>> -        return H_PARAMETER;
>> -    }
>> -
>> -    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
>> -    v = ppc_hash64_hpte0(cpu, hptes, 0);
>> -    r = ppc_hash64_hpte1(cpu, hptes, 0);
>> -    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>> -
>> -    if ((v & HPTE64_V_VALID) == 0 ||
>> -        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
>> -        return H_NOT_FOUND;
>> -    }
>> -
>> -    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
>> -           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
>> -    r |= (flags << 55) & HPTE64_R_PP0;
>> -    r |= (flags << 48) & HPTE64_R_KEY_HI;
>> -    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
>> -    spapr_store_hpte(cpu, ptex,
>> -                     (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
>> -    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
>> -    /* Flush the tlb */
>> -    check_tlb_flush(env, true);
>> -    /* Don't need a memory barrier, due to qemu's global lock */
>> -    spapr_store_hpte(cpu, ptex, v | HPTE64_V_HPTE_DIRTY, r);
>> -    return H_SUCCESS;
>> -}
>> -
>> -static target_ulong h_read(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> -                           target_ulong opcode, target_ulong *args)
>> -{
>> -    target_ulong flags = args[0];
>> -    target_ulong ptex = args[1];
>> -    int i, ridx, n_entries = 1;
>> -    const ppc_hash_pte64_t *hptes;
>> -
>> -    if (!valid_ptex(cpu, ptex)) {
>> -        return H_PARAMETER;
>> -    }
>> -
>> -    if (flags & H_READ_4) {
>> -        /* Clear the two low order bits */
>> -        ptex &= ~(3ULL);
>> -        n_entries = 4;
>> -    }
>> -
>> -    hptes = ppc_hash64_map_hptes(cpu, ptex, n_entries);
>> -    for (i = 0, ridx = 0; i < n_entries; i++) {
>> -        args[ridx++] = ppc_hash64_hpte0(cpu, hptes, i);
>> -        args[ridx++] = ppc_hash64_hpte1(cpu, hptes, i);
>> -    }
>> -    ppc_hash64_unmap_hptes(cpu, hptes, ptex, n_entries);
>> -
>> -    return H_SUCCESS;
>> -}
>> -
>>   struct SpaprPendingHpt {
>>       /* These fields are read-only after initialization */
>>       int shift;
>> @@ -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 */
>>   
>>       /* hcall-hpt-resize */
>>       spapr_register_hypercall(H_RESIZE_HPT_PREPARE, h_resize_hpt_prepare);
>> diff --git a/hw/ppc/spapr_hcall_tcg.c b/hw/ppc/spapr_hcall_tcg.c
>> new file mode 100644
>> index 0000000000..92ff24c8dc
>> --- /dev/null
>> +++ b/hw/ppc/spapr_hcall_tcg.c
>> @@ -0,0 +1,343 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>> +#include "qapi/error.h"
>> +#include "sysemu/hw_accel.h"
>> +#include "sysemu/runstate.h"
>> +#include "qemu/log.h"
>> +#include "qemu/main-loop.h"
>> +#include "qemu/module.h"
>> +#include "qemu/error-report.h"
>> +#include "cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "helper_regs.h"
>> +#include "hw/ppc/spapr.h"
>> +#include "hw/ppc/spapr_cpu_core.h"
>> +#include "mmu-hash64.h"
>> +#include "mmu-misc.h"
>> +#include "cpu-models.h"
>> +#include "trace.h"
>> +#include "kvm_ppc.h"
>> +#include "hw/ppc/fdt.h"
>> +#include "hw/ppc/spapr_ovec.h"
>> +#include "mmu-book3s-v3.h"
>> +#include "hw/mem/memory-device.h"
>> +
>> +static inline bool valid_ptex(PowerPCCPU *cpu, target_ulong ptex)
>> +{
>> +    /*
>> +     * hash value/pteg group index is normalized by HPT mask
>> +     */
>> +    if (((ptex & ~7ULL) / HPTES_PER_GROUP) & ~ppc_hash64_hpt_mask(cpu)) {
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +static bool is_ram_address(SpaprMachineState *spapr, hwaddr addr)
>> +{
>> +    MachineState *machine = MACHINE(spapr);
>> +    DeviceMemoryState *dms = machine->device_memory;
>> +
>> +    if (addr < machine->ram_size) {
>> +        return true;
>> +    }
>> +    if ((addr >= dms->base)
>> +        && ((addr - dms->base) < memory_region_size(&dms->mr))) {
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static target_ulong h_enter(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                            target_ulong opcode, target_ulong *args)
>> +{
>> +    target_ulong flags = args[0];
>> +    target_ulong ptex = args[1];
>> +    target_ulong pteh = args[2];
>> +    target_ulong ptel = args[3];
>> +    unsigned apshift;
>> +    target_ulong raddr;
>> +    target_ulong slot;
>> +    const ppc_hash_pte64_t *hptes;
>> +
>> +    apshift = ppc_hash64_hpte_page_shift_noslb(cpu, pteh, ptel);
>> +    if (!apshift) {
>> +        /* Bad page size encoding */
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    raddr = (ptel & HPTE64_R_RPN) & ~((1ULL << apshift) - 1);
>> +
>> +    if (is_ram_address(spapr, raddr)) {
>> +        /* Regular RAM - should have WIMG=0010 */
>> +        if ((ptel & HPTE64_R_WIMG) != HPTE64_R_M) {
>> +            return H_PARAMETER;
>> +        }
>> +    } else {
>> +        target_ulong wimg_flags;
>> +        /* Looks like an IO address */
>> +        /* FIXME: What WIMG combinations could be sensible for IO?
>> +         * For now we allow WIMG=010x, but are there others? */
>> +        /* FIXME: Should we check against registered IO addresses? */
>> +        wimg_flags = (ptel & (HPTE64_R_W | HPTE64_R_I | HPTE64_R_M));
>> +
>> +        if (wimg_flags != HPTE64_R_I &&
>> +            wimg_flags != (HPTE64_R_I | HPTE64_R_M)) {
>> +            return H_PARAMETER;
>> +        }
>> +    }
>> +
>> +    pteh &= ~0x60ULL;
>> +
>> +    if (!valid_ptex(cpu, ptex)) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    slot = ptex & 7ULL;
>> +    ptex = ptex & ~7ULL;
>> +
>> +    if (likely((flags & H_EXACT) == 0)) {
>> +        hptes = ppc_hash64_map_hptes(cpu, ptex, HPTES_PER_GROUP);
>> +        for (slot = 0; slot < 8; slot++) {
>> +            if (!(ppc_hash64_hpte0(cpu, hptes, slot) & HPTE64_V_VALID)) {
>> +                break;
>> +            }
>> +        }
>> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, HPTES_PER_GROUP);
>> +        if (slot == 8) {
>> +            return H_PTEG_FULL;
>> +        }
>> +    } else {
>> +        hptes = ppc_hash64_map_hptes(cpu, ptex + slot, 1);
>> +        if (ppc_hash64_hpte0(cpu, hptes, 0) & HPTE64_V_VALID) {
>> +            ppc_hash64_unmap_hptes(cpu, hptes, ptex + slot, 1);
>> +            return H_PTEG_FULL;
>> +        }
>> +        ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>> +    }
>> +
>> +    spapr_store_hpte(cpu, ptex + slot, pteh | HPTE64_V_HPTE_DIRTY, ptel);
>> +
>> +    args[0] = ptex + slot;
>> +    return H_SUCCESS;
>> +}
>> +
>> +typedef enum {
>> +    REMOVE_SUCCESS = 0,
>> +    REMOVE_NOT_FOUND = 1,
>> +    REMOVE_PARM = 2,
>> +    REMOVE_HW = 3,
>> +} RemoveResult;
>> +
>> +static RemoveResult remove_hpte(PowerPCCPU *cpu
>> +                                , target_ulong ptex,
>> +                                target_ulong avpn,
>> +                                target_ulong flags,
>> +                                target_ulong *vp, target_ulong *rp)
>> +{
>> +    const ppc_hash_pte64_t *hptes;
>> +    target_ulong v, r;
>> +
>> +    if (!valid_ptex(cpu, ptex)) {
>> +        return REMOVE_PARM;
>> +    }
>> +
>> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
>> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
>> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
>> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>> +
>> +    if ((v & HPTE64_V_VALID) == 0 ||
>> +        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn) ||
>> +        ((flags & H_ANDCOND) && (v & avpn) != 0)) {
>> +        return REMOVE_NOT_FOUND;
>> +    }
>> +    *vp = v;
>> +    *rp = r;
>> +    spapr_store_hpte(cpu, ptex, HPTE64_V_HPTE_DIRTY, 0);
>> +    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
>> +    return REMOVE_SUCCESS;
>> +}
>> +
>> +static target_ulong h_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                             target_ulong opcode, target_ulong *args)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    target_ulong flags = args[0];
>> +    target_ulong ptex = args[1];
>> +    target_ulong avpn = args[2];
>> +    RemoveResult ret;
>> +
>> +    ret = remove_hpte(cpu, ptex, avpn, flags,
>> +                      &args[0], &args[1]);
>> +
>> +    switch (ret) {
>> +    case REMOVE_SUCCESS:
>> +        check_tlb_flush(env, true);
>> +        return H_SUCCESS;
>> +
>> +    case REMOVE_NOT_FOUND:
>> +        return H_NOT_FOUND;
>> +
>> +    case REMOVE_PARM:
>> +        return H_PARAMETER;
>> +
>> +    case REMOVE_HW:
>> +        return H_HARDWARE;
>> +    }
>> +
>> +    g_assert_not_reached();
>> +}
>> +
>> +#define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
>> +#define   H_BULK_REMOVE_REQUEST        0x4000000000000000ULL
>> +#define   H_BULK_REMOVE_RESPONSE       0x8000000000000000ULL
>> +#define   H_BULK_REMOVE_END            0xc000000000000000ULL
>> +#define H_BULK_REMOVE_CODE             0x3000000000000000ULL
>> +#define   H_BULK_REMOVE_SUCCESS        0x0000000000000000ULL
>> +#define   H_BULK_REMOVE_NOT_FOUND      0x1000000000000000ULL
>> +#define   H_BULK_REMOVE_PARM           0x2000000000000000ULL
>> +#define   H_BULK_REMOVE_HW             0x3000000000000000ULL
>> +#define H_BULK_REMOVE_RC               0x0c00000000000000ULL
>> +#define H_BULK_REMOVE_FLAGS            0x0300000000000000ULL
>> +#define   H_BULK_REMOVE_ABSOLUTE       0x0000000000000000ULL
>> +#define   H_BULK_REMOVE_ANDCOND        0x0100000000000000ULL
>> +#define   H_BULK_REMOVE_AVPN           0x0200000000000000ULL
>> +#define H_BULK_REMOVE_PTEX             0x00ffffffffffffffULL
>> +
>> +#define H_BULK_REMOVE_MAX_BATCH        4
>> +
>> +static target_ulong h_bulk_remove(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                                  target_ulong opcode, target_ulong *args)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    int i;
>> +    target_ulong rc = H_SUCCESS;
>> +
>> +    for (i = 0; i < H_BULK_REMOVE_MAX_BATCH; i++) {
>> +        target_ulong *tsh = &args[i*2];
>> +        target_ulong tsl = args[i*2 + 1];
>> +        target_ulong v, r, ret;
>> +
>> +        if ((*tsh & H_BULK_REMOVE_TYPE) == H_BULK_REMOVE_END) {
>> +            break;
>> +        } else if ((*tsh & H_BULK_REMOVE_TYPE) != H_BULK_REMOVE_REQUEST) {
>> +            return H_PARAMETER;
>> +        }
>> +
>> +        *tsh &= H_BULK_REMOVE_PTEX | H_BULK_REMOVE_FLAGS;
>> +        *tsh |= H_BULK_REMOVE_RESPONSE;
>> +
>> +        if ((*tsh & H_BULK_REMOVE_ANDCOND) && (*tsh & H_BULK_REMOVE_AVPN)) {
>> +            *tsh |= H_BULK_REMOVE_PARM;
>> +            return H_PARAMETER;
>> +        }
>> +
>> +        ret = remove_hpte(cpu, *tsh & H_BULK_REMOVE_PTEX, tsl,
>> +                          (*tsh & H_BULK_REMOVE_FLAGS) >> 26,
>> +                          &v, &r);
>> +
>> +        *tsh |= ret << 60;
>> +
>> +        switch (ret) {
>> +        case REMOVE_SUCCESS:
>> +            *tsh |= (r & (HPTE64_R_C | HPTE64_R_R)) << 43;
>> +            break;
>> +
>> +        case REMOVE_PARM:
>> +            rc = H_PARAMETER;
>> +            goto exit;
>> +
>> +        case REMOVE_HW:
>> +            rc = H_HARDWARE;
>> +            goto exit;
>> +        }
>> +    }
>> + exit:
>> +    check_tlb_flush(env, true);
>> +
>> +    return rc;
>> +}
>> +
>> +static target_ulong h_protect(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                              target_ulong opcode, target_ulong *args)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    target_ulong flags = args[0];
>> +    target_ulong ptex = args[1];
>> +    target_ulong avpn = args[2];
>> +    const ppc_hash_pte64_t *hptes;
>> +    target_ulong v, r;
>> +
>> +    if (!valid_ptex(cpu, ptex)) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    hptes = ppc_hash64_map_hptes(cpu, ptex, 1);
>> +    v = ppc_hash64_hpte0(cpu, hptes, 0);
>> +    r = ppc_hash64_hpte1(cpu, hptes, 0);
>> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, 1);
>> +
>> +    if ((v & HPTE64_V_VALID) == 0 ||
>> +        ((flags & H_AVPN) && (v & ~0x7fULL) != avpn)) {
>> +        return H_NOT_FOUND;
>> +    }
>> +
>> +    r &= ~(HPTE64_R_PP0 | HPTE64_R_PP | HPTE64_R_N |
>> +           HPTE64_R_KEY_HI | HPTE64_R_KEY_LO);
>> +    r |= (flags << 55) & HPTE64_R_PP0;
>> +    r |= (flags << 48) & HPTE64_R_KEY_HI;
>> +    r |= flags & (HPTE64_R_PP | HPTE64_R_N | HPTE64_R_KEY_LO);
>> +    spapr_store_hpte(cpu, ptex,
>> +                     (v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
>> +    ppc_hash64_tlb_flush_hpte(cpu, ptex, v, r);
>> +    /* Flush the tlb */
>> +    check_tlb_flush(env, true);
>> +    /* Don't need a memory barrier, due to qemu's global lock */
>> +    spapr_store_hpte(cpu, ptex, v | HPTE64_V_HPTE_DIRTY, r);
>> +    return H_SUCCESS;
>> +}
>> +
>> +static target_ulong h_read(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                           target_ulong opcode, target_ulong *args)
>> +{
>> +    target_ulong flags = args[0];
>> +    target_ulong ptex = args[1];
>> +    int i, ridx, n_entries = 1;
>> +    const ppc_hash_pte64_t *hptes;
>> +
>> +    if (!valid_ptex(cpu, ptex)) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    if (flags & H_READ_4) {
>> +        /* Clear the two low order bits */
>> +        ptex &= ~(3ULL);
>> +        n_entries = 4;
>> +    }
>> +
>> +    hptes = ppc_hash64_map_hptes(cpu, ptex, n_entries);
>> +    for (i = 0, ridx = 0; i < n_entries; i++) {
>> +        args[ridx++] = ppc_hash64_hpte0(cpu, hptes, i);
>> +        args[ridx++] = ppc_hash64_hpte1(cpu, hptes, i);
>> +    }
>> +    ppc_hash64_unmap_hptes(cpu, hptes, ptex, n_entries);
>> +
>> +    return H_SUCCESS;
>> +}
>> +
>> +
>> +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)

David Gibson May 5, 2021, 4:58 a.m. UTC | #5

On Tue, May 04, 2021 at 03:14:17PM -0300, Lucas Mateus Martins Araujo e Castro wrote:
> 
> On 03/05/2021 01:34, David Gibson wrote:
> > On Fri, Apr 30, 2021 at 03:40:47PM -0300, Lucas Mateus Castro (alqotel) wrote:
> > > Moved h_enter, remove_hpte, h_remove, h_bulk_remove,h_protect and
> > > h_read to spapr_hcall_tcg.c, added h_tcg_only to be used in a !TCG
> > > environment in spapr_hcall.c and changed build options to only build
> > > spapr_hcall_tcg.c when CONFIG_TCG is enabled.
> > This looks good.  I'd suggest the name 'spapr_softmmu.c' instead,
> > which more specifically describes what's special about these
> > functions.
> > 
> > h_resize_hpt_prepare(), h_resize_hpt_commit() and the functions they
> > depend on belong in the softmmu set as well.
> 
> Moved these hcalls to spapr_softmmu.c as well, along with the most
> functions they depend on, but 1 function (push_sregs_to_kvm_pr) is
> also used by hcalls in spapr_hcall.c, so for now I'm just leaving it in
> spapr_hcall.c and exporting it to be used in spapr_softmmu.c.

Right.  That one's an ugly workaround for some weird KVM PR behaviour,
and we will need it in the common code.

> On a related note, from what I've seen h_resize_hpt_prepare and
> h_resize_hpt_commit are not implementede in KVM, so they're only
> called when there's softmmu so that's why they can be moved to
> spapr_softmmu.c?

Ah, sorry, I forgot how these worked.  The bulk of the logic to
implement these *is* in KVM, but KVM doesn't directly intercept them
as hcalls.  Instead the hcall is caught by qemu, but it just forwards
them back to KVM by calling an ioctl().  See the calls to
kvmppc_resize_hpt_prepare() and kvmppc_resize_hpt_commit() in
spapr_hcall.c.

[Aside: the reason for this is that they're not latency critical, and
 it's useful for qemu to be aware of and possibly filter HPT resize
 requests, but because KVM manages the HPT, qemu can't implement these
 directly for the KVM case]

So what we'll need to do for those is keep the first part if
h_resize_hpt_{prepare,commit}() in common code - that's basically just
parameter validation - up to the call to
kvmppc_resize_hpt_{prepare,commit}().

if the kvmppc_() call fails with -ENOSYS then we need to do
	if (kvm_enabled()) {
		return H_HARDWARE;
	}
	softmmu_resize_hpt_{prepare,commit}()

Where that softmmu_..() function is the remainder of the logic that's
in the current implementation.  That bit can move to the softmmu file
and can be stubbed with an assert-not-reached for the !TCG case.

In practice we should never actually hit that H_HARDWARE - we should
have failed at machine setup if we tried to enable the resize HPT
feature on a KVM that doesn't implement it.

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

Commit Message

Comments

Patch