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

[Alexander Graf]

On 23.02.15 08:36, Bharata B Rao wrote:
> 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 ?

They should be in the board init function and in the board's cpu hotplug
functions.


Alex