Re: [Qemu-devel] [RFC PATCH v1 04/13] spapr: Factor out CPU initialization code into realizefn

[David Gibson]

On Thu, Jan 08, 2015 at 11:40:11AM +0530, Bharata B Rao wrote:
> Move some CPU initialization code from machine init function to
> CPU realizefn so that it can be used from CPU hotplug path too.
> 
> With the inclusion of ppc.h in translate_init.c, explicit *irq_init()
> function definitions aren't required, remove them.
> 
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
>  hw/ppc/spapr.c              | 29 +----------------------------
>  include/hw/ppc/spapr.h      |  3 +++
>  target-ppc/translate_init.c | 43 ++++++++++++++++++++++++++-----------------
>  3 files changed, 30 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 779d364..f49b0fa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -81,8 +81,6 @@
>  
>  #define MIN_RMA_SLOF            128UL
>  
> -#define TIMEBASE_FREQ           512000000ULL
> -
>  #define MAX_CPUS                255
>  
>  #define PHANDLE_XICP            0x00001111
> @@ -971,7 +969,7 @@ static void ppc_spapr_reset(void)
>  
>  }
>  
> -static void spapr_cpu_reset(void *opaque)
> +void spapr_cpu_reset(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
>      CPUState *cs = CPU(cpu);
> @@ -1387,7 +1385,6 @@ static void ppc_spapr_init(MachineState *machine)
>      const char *initrd_filename = machine->initrd_filename;
>      const char *boot_device = machine->boot_order;
>      PowerPCCPU *cpu;
> -    CPUPPCState *env;
>      PCIHostState *phb;
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
> @@ -1472,30 +1469,6 @@ static void ppc_spapr_init(MachineState *machine)
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
>          }
> -        env = &cpu->env;
> -
> -        /* Set time-base frequency to 512 MHz */
> -        cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> -
> -        /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> -         * MSR[IP] should never be set.
> -         */
> -        env->msr_mask &= ~(1 << 6);
> -
> -        /* Tell KVM that we're in PAPR mode */
> -        if (kvm_enabled()) {
> -            kvmppc_set_papr(cpu);
> -        }
> -
> -        if (cpu->max_compat) {
> -            if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
> -                exit(1);
> -            }
> -        }
> -
> -        xics_cpu_setup(spapr->icp, cpu);
> -
> -        qemu_register_reset(spapr_cpu_reset, cpu);
>      }
>  
>      /* allocate RAM */
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b1a0838..831db6b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -478,6 +478,8 @@ struct sPAPRTCETable {
>      QLIST_ENTRY(sPAPRTCETable) list;
>  };
>  
> +#define TIMEBASE_FREQ           512000000ULL
> +
>  void spapr_events_init(sPAPREnvironment *spapr);
>  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
>  int spapr_h_cas_compose_response(target_ulong addr, target_ulong size);
> @@ -494,5 +496,6 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char 
> *propname,
>                        sPAPRTCETable *tcet);
>  void spapr_hotplug_req_add_event(sPAPRDRConnector *drc);
>  void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
> +void spapr_cpu_reset(void *opaque);
>  
>  #endif /* !defined (__HW_SPAPR_H__) */
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 72cc9d0..9c642a5 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -30,29 +30,14 @@
>  #include "qemu/error-report.h"
>  #include "qapi/visitor.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/ppc.h"
>  
>  //#define PPC_DUMP_CPU
>  //#define PPC_DEBUG_SPR
>  //#define PPC_DUMP_SPR_ACCESSES
>  /* #define USE_APPLE_GDB */
>  
> -/* For user-mode emulation, we don't emulate any IRQ controller */
> -#if defined(CONFIG_USER_ONLY)
> -#define PPC_IRQ_INIT_FN(name)                                                
>  \
> -static inline void glue(glue(ppc, name),_irq_init) (CPUPPCState *env)        
>  \
> -{                                                                            
>  \
> -}
> -#else
> -#define PPC_IRQ_INIT_FN(name)                                                
>  \
> -void glue(glue(ppc, name),_irq_init) (CPUPPCState *env);
> -#endif
> -
> -PPC_IRQ_INIT_FN(40x);
> -PPC_IRQ_INIT_FN(6xx);
> -PPC_IRQ_INIT_FN(970);
> -PPC_IRQ_INIT_FN(POWER7);
> -PPC_IRQ_INIT_FN(e500);
> -
>  /* Generic callbacks:
>   * do nothing but store/retrieve spr value
>   */
> @@ -8905,6 +8890,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      CPUState *cs = CPU(dev);
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    CPUPPCState *env = &cpu->env;
>      Error *local_err = NULL;
>  #if !defined(CONFIG_USER_ONLY)
>      int max_smt = kvm_enabled() ? kvmppc_smt_threads() : 1;
> @@ -8965,6 +8951,29 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  
>      qemu_init_vcpu(cs);
>  
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> +
> +    /* PAPR always has exception vectors in RAM not ROM. To ensure this,
> +     * MSR[IP] should never be set.
> +     */
> +    env->msr_mask &= ~(1 << 6);
> +
> +    /* Tell KVM that we're in PAPR mode */
> +    if (kvm_enabled()) {
> +        kvmppc_set_papr(cpu);
> +    }
> +
> +    if (cpu->max_compat) {
> +        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
> +            exit(1);
> +        }
> +    }
> +
> +    xics_cpu_setup(spapr->icp, cpu);
> +
> +    qemu_register_reset(spapr_cpu_reset, cpu);
> +
>      pcc->parent_realize(dev, errp);

This doesn't look right.  Several of these are clearly PAPR specific
operations, but you're now doing them from code that isn't PAPR specific.

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

pgpcighHWW2eL.pgp
Description: PGP signature