Re: [PATCH v2 10/15] target/ppc: Restrict ppc_tcg_hv_emu() to TCG

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 1/27/25 15:56, Philippe Mathieu-Daudé wrote:
> Make is_prefix_insn_excp() prototype but have it guarded by
> a tcg_enabled() check. Inline part of it in powerpc_excp_books().
> 
> Extract POWERPC_EXCP_HV_EMU handling code to ppc_tcg_hv_emu(),
> also exposing its prototype in "internal.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/internal.h        |   6 +++
>   target/ppc/excp_helper.c     | 101 +++++------------------------------
>   target/ppc/tcg-excp_helper.c |  75 ++++++++++++++++++++++++++
>   3 files changed, 93 insertions(+), 89 deletions(-)
> 
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 62186bc1e61..0e66b29ec68 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -291,6 +291,12 @@ bool ppc_cpu_debug_check_breakpoint(CPUState *cs);
>   bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
>   
>   G_NORETURN void powerpc_checkstop(CPUPPCState *env, const char *reason);
> +
> +#if defined(TARGET_PPC64)
> +bool is_prefix_insn_excp(CPUPPCState *env, int excp);
> +void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
> +                    int *srr0, int *srr1);
> +#endif /* TARGET_PPC64 */
>   #endif /* !CONFIG_USER_ONLY */
>   
>   FIELD(GER_MSK, XMSK, 0, 4)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 6a12402b23a..56a56148a40 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -19,6 +19,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/main-loop.h"
>   #include "qemu/log.h"
> +#include "system/kvm.h"
>   #include "system/tcg.h"
>   #include "system/system.h"
>   #include "system/runstate.h"
> @@ -1194,81 +1195,6 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
>       return false;
>   }
>   
> -#ifdef CONFIG_TCG
> -static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
> -{
> -    if (!(env->insns_flags2 & PPC2_ISA310)) {
> -        return false;
> -    }
> -    return ((insn & 0xfc000000) == 0x04000000);
> -}
> -
> -static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
> -{
> -    CPUPPCState *env = &cpu->env;
> -
> -    if (!(env->insns_flags2 & PPC2_ISA310)) {
> -        return false;
> -    }
> -
> -    if (!tcg_enabled()) {
> -        /*
> -         * This does not load instructions and set the prefix bit correctly
> -         * for injected interrupts with KVM. That may have to be discovered
> -         * and set by the KVM layer before injecting.
> -         */
> -        return false;
> -    }
> -
> -    switch (excp) {
> -    case POWERPC_EXCP_MCHECK:
> -        if (!(env->error_code & PPC_BIT(42))) {
> -            /*
> -             * Fetch attempt caused a machine check, so attempting to fetch
> -             * again would cause a recursive machine check.
> -             */
> -            return false;
> -        }
> -        break;
> -    case POWERPC_EXCP_HDSI:
> -        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
> -        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
> -            (env->error_code == MMU_INST_FETCH)) {
> -            /*
> -             * Fetch failed due to partition scope translation, so prefix
> -             * indication is not relevant (and attempting to load the
> -             * instruction at NIP would cause recursive faults with the same
> -             * translation).
> -             */
> -            return false;
> -        }
> -        break;
> -
> -    case POWERPC_EXCP_DSI:
> -    case POWERPC_EXCP_DSEG:
> -    case POWERPC_EXCP_ALIGN:
> -    case POWERPC_EXCP_PROGRAM:
> -    case POWERPC_EXCP_FPU:
> -    case POWERPC_EXCP_TRACE:
> -    case POWERPC_EXCP_HV_EMU:
> -    case POWERPC_EXCP_VPU:
> -    case POWERPC_EXCP_VSXU:
> -    case POWERPC_EXCP_FU:
> -    case POWERPC_EXCP_HV_FU:
> -        break;
> -    default:
> -        return false;
> -    }
> -
> -    return is_prefix_insn(env, ppc_ldl_code(env, env->nip));
> -}
> -#else
> -static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
> -{
> -    return false;
> -}
> -#endif
> -
>   static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>   {
>       CPUPPCState *env = &cpu->env;
> @@ -1310,7 +1236,15 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>       }
>       vector |= env->excp_prefix;
>   
> -    if (is_prefix_insn_excp(cpu, excp)) {
> +    if (env->insns_flags2 & PPC2_ISA310) {

I guess you meant checking for ! of above.
is_prefix_insn_excp() returns false for ! of above.

> +        /* nothing to do */
> +    } else if (kvm_enabled()) {
> +        /*
> +         * This does not load instructions and set the prefix bit correctly
> +         * for injected interrupts with KVM. That may have to be discovered
> +         * and set by the KVM layer before injecting.
> +         */
> +    } else if (tcg_enabled() && is_prefix_insn_excp(env, excp)) {
>           msr |= PPC_BIT(34);
>       }
>   
> @@ -1484,20 +1418,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>           new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>           break;
>   #ifdef CONFIG_TCG
> -    case POWERPC_EXCP_HV_EMU: {
> -        uint32_t insn = ppc_ldl_code(env, env->nip);
> -        env->spr[SPR_HEIR] = insn;
> -        if (is_prefix_insn(env, insn)) {
> -            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> -            env->spr[SPR_HEIR] <<= 32;
> -            env->spr[SPR_HEIR] |= insn2;
> -        }
> -        srr0 = SPR_HSRR0;
> -        srr1 = SPR_HSRR1;
> -        new_msr |= (target_ulong)MSR_HVB;
> -        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> +    case POWERPC_EXCP_HV_EMU:
> +        ppc_tcg_hv_emu(env, &new_msr, &srr0, &srr1);

Naming suggestion: ppc_excp_hv_emu may be more apt.

Thanks,
Harsh

>           break;
> -    }
>   #endif
>       case POWERPC_EXCP_VPU:       /* Vector unavailable exception             */
>       case POWERPC_EXCP_VSXU:       /* VSX unavailable exception               */
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index 268a1614597..dc5601a4577 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -283,4 +283,79 @@ uint32_t ppc_ldl_code(CPUArchState *env, target_ulong addr)
>       return insn;
>   }
>   
> +#if defined(TARGET_PPC64)
> +
> +static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
> +{
> +    if (!(env->insns_flags2 & PPC2_ISA310)) {
> +        return false;
> +    }
> +    return ((insn & 0xfc000000) == 0x04000000);
> +}
> +
> +bool is_prefix_insn_excp(CPUPPCState *env, int excp)
> +{
> +    switch (excp) {
> +    case POWERPC_EXCP_MCHECK:
> +        if (!(env->error_code & PPC_BIT(42))) {
> +            /*
> +             * Fetch attempt caused a machine check, so attempting to fetch
> +             * again would cause a recursive machine check.
> +             */
> +            return false;
> +        }
> +        break;
> +    case POWERPC_EXCP_HDSI:
> +        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
> +        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
> +            (env->error_code == MMU_INST_FETCH)) {
> +            /*
> +             * Fetch failed due to partition scope translation, so prefix
> +             * indication is not relevant (and attempting to load the
> +             * instruction at NIP would cause recursive faults with the same
> +             * translation).
> +             */
> +            return false;
> +        }
> +        break;
> +
> +    case POWERPC_EXCP_DSI:
> +    case POWERPC_EXCP_DSEG:
> +    case POWERPC_EXCP_ALIGN:
> +    case POWERPC_EXCP_PROGRAM:
> +    case POWERPC_EXCP_FPU:
> +    case POWERPC_EXCP_TRACE:
> +    case POWERPC_EXCP_HV_EMU:
> +    case POWERPC_EXCP_VPU:
> +    case POWERPC_EXCP_VSXU:
> +    case POWERPC_EXCP_FU:
> +    case POWERPC_EXCP_HV_FU:
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    return is_prefix_insn(env, ppc_ldl_code(env, env->nip));
> +}
> +
> +void ppc_tcg_hv_emu(CPUPPCState *env, target_ulong *new_msr,
> +                    int *srr0, int *srr1)
> +{
> +    uint32_t insn = ppc_ldl_code(env, env->nip);
> +
> +    env->spr[SPR_HEIR] = insn;
> +    if (is_prefix_insn(env, insn)) {
> +        uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> +
> +        env->spr[SPR_HEIR] <<= 32;
> +        env->spr[SPR_HEIR] |= insn2;
> +    }
> +    *srr0 = SPR_HSRR0;
> +    *srr1 = SPR_HSRR1;
> +    *new_msr |= (target_ulong)MSR_HVB;
> +    *new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> +}
> +
> +#endif /* TARGET_PPC64 */
> +
>   #endif /* !CONFIG_USER_ONLY */