Re: [PATCH v3 21/30] target/ppc: Introduce PowerPCCPUClass::has_work()

[David Gibson]

On Thu, Sep 02, 2021 at 06:15:34PM +0200, Philippe Mathieu-Daudé wrote:
> Each POWER cpu has its own has_work() implementation. Instead of
> overloading CPUClass on each PowerPCCPUClass init, register the
> generic ppc_cpu_has_work() handler, and have it call the POWER
> specific has_work().

I don't quite see the rationale for introducing a second layer of
indirection here.  What's wrong with switching the base has_work for
each cpu variant?

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/ppc/cpu-qom.h  |  3 +++
>  target/ppc/cpu_init.c | 26 ++++++++++++++++++--------
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 5800fa324e8..ff2bafcde6f 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -189,6 +189,9 @@ struct PowerPCCPUClass {
>      int bfd_mach;
>      uint32_t l1_dcache_size, l1_icache_size;
>  #ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_TCG
> +    bool (*has_work)(CPUState *cpu);
> +#endif /* CONFIG_TCG */
>      unsigned int gdb_num_sprs;
>      const char *gdb_spr_xml;
>  #endif
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index e2e721c2b81..bbad16cc1ec 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7583,6 +7583,7 @@ static bool ppc_pvr_match_power7(PowerPCCPUClass *pcc, 
> uint32_t pvr)
>      return false;
>  }
>  
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>  static bool cpu_has_work_POWER7(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -7616,12 +7617,12 @@ static bool cpu_has_work_POWER7(CPUState *cs)
>          return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
>      }
>  }
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>  
>  POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -    CPUClass *cc = CPU_CLASS(oc);
>  
>      dc->fw_name = "PowerPC,POWER7";
>      dc->desc = "POWER7";
> @@ -7630,7 +7631,6 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>      pcc->pcr_supported = PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
>      pcc->init_proc = init_proc_POWER7;
>      pcc->check_pow = check_pow_nocheck;
> -    cc->has_work = cpu_has_work_POWER7;
>      pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>                         PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
>                         PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -7673,6 +7673,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>      pcc->lpcr_pm = LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2;
>      pcc->mmu_model = POWERPC_MMU_2_06;
>  #if defined(CONFIG_SOFTMMU)
> +    pcc->has_work = cpu_has_work_POWER7;
>      pcc->hash64_opts = &ppc_hash64_opts_POWER7;
>      pcc->lrg_decr_bits = 32;
>  #endif
> @@ -7743,6 +7744,7 @@ static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, 
> uint32_t pvr)
>      return false;
>  }
>  
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>  static bool cpu_has_work_POWER8(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -7784,12 +7786,12 @@ static bool cpu_has_work_POWER8(CPUState *cs)
>          return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
>      }
>  }
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>  
>  POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -    CPUClass *cc = CPU_CLASS(oc);
>  
>      dc->fw_name = "PowerPC,POWER8";
>      dc->desc = "POWER8";
> @@ -7798,7 +7800,6 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>      pcc->pcr_supported = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
>      pcc->init_proc = init_proc_POWER8;
>      pcc->check_pow = check_pow_nocheck;
> -    cc->has_work = cpu_has_work_POWER8;
>      pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>                         PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
>                         PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -7848,6 +7849,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>                     LPCR_P8_PECE3 | LPCR_P8_PECE4;
>      pcc->mmu_model = POWERPC_MMU_2_07;
>  #if defined(CONFIG_SOFTMMU)
> +    pcc->has_work = cpu_has_work_POWER8;
>      pcc->hash64_opts = &ppc_hash64_opts_POWER7;
>      pcc->lrg_decr_bits = 32;
>      pcc->n_host_threads = 8;
> @@ -7941,6 +7943,7 @@ static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, 
> uint32_t pvr)
>      return false;
>  }
>  
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>  static bool cpu_has_work_POWER9(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -7998,12 +8001,12 @@ static bool cpu_has_work_POWER9(CPUState *cs)
>          return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
>      }
>  }
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>  
>  POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -    CPUClass *cc = CPU_CLASS(oc);
>  
>      dc->fw_name = "PowerPC,POWER9";
>      dc->desc = "POWER9";
> @@ -8013,7 +8016,6 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>                           PCR_COMPAT_2_05;
>      pcc->init_proc = init_proc_POWER9;
>      pcc->check_pow = check_pow_nocheck;
> -    cc->has_work = cpu_has_work_POWER9;
>      pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>                         PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
>                         PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -8062,6 +8064,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>      pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
>      pcc->mmu_model = POWERPC_MMU_3_00;
>  #if defined(CONFIG_SOFTMMU)
> +    pcc->has_work = cpu_has_work_POWER9;
>      /* segment page size remain the same */
>      pcc->hash64_opts = &ppc_hash64_opts_POWER7;
>      pcc->radix_page_info = &POWER9_radix_page_info;
> @@ -8150,6 +8153,7 @@ static bool ppc_pvr_match_power10(PowerPCCPUClass *pcc, 
> uint32_t pvr)
>      return false;
>  }
>  
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>  static bool cpu_has_work_POWER10(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -8207,12 +8211,12 @@ static bool cpu_has_work_POWER10(CPUState *cs)
>          return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
>      }
>  }
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>  
>  POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -    CPUClass *cc = CPU_CLASS(oc);
>  
>      dc->fw_name = "PowerPC,POWER10";
>      dc->desc = "POWER10";
> @@ -8223,7 +8227,6 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>                           PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
>      pcc->init_proc = init_proc_POWER10;
>      pcc->check_pow = check_pow_nocheck;
> -    cc->has_work = cpu_has_work_POWER10;
>      pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>                         PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
>                         PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -8275,6 +8278,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>      pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
>      pcc->mmu_model = POWERPC_MMU_3_00;
>  #if defined(CONFIG_SOFTMMU)
> +    pcc->has_work = cpu_has_work_POWER10;
>      /* segment page size remain the same */
>      pcc->hash64_opts = &ppc_hash64_opts_POWER7;
>      pcc->radix_page_info = &POWER10_radix_page_info;
> @@ -8796,6 +8800,12 @@ static bool ppc_cpu_has_work(CPUState *cs)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
>  
> +    if (cs->halted) {
> +        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +        return pcc->has_work(cs);
> +    }
> +
>      return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
>  }
>  #endif /* CONFIG_TCG && !CONFIG_USER_ONLY */

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