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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Thu, Jan 29, 2015 at 12:07:42PM +1100, David Gibson wrote:
> 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 <bharata@linux.vnet.ibm.com>
> > ---
> >  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.

Ok, will re-work on this patch.

Thanks for your review.

Regards,
Bharata.