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

[Bharata B Rao]

On Fri, Jan 30, 2015 at 01:19:39PM +0530, Bharata B Rao wrote:
> 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 <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.
> 
> Ok, will re-work on this patch.

There is only PowerPCCPU and no CPU specific class for sPAPR. So such things
as above which should ideally be done in realizefn path but are sPAPR specific
should sit where ? Under TARGET_PPC64 ?

Regards,
Bharata.