Re: [Qemu-devel] [RFC PATCH v0 10/15] ppc: Factor out CPU initialization code to a new routine

[Igor Mammedov]

On Mon, 29 Sep 2014 08:30:15 +0530
Bharata B Rao <address@hidden> wrote:

> On Fri, Sep 26, 2014 at 05:29:02PM +0200, Igor Mammedov wrote:
> > On Thu,  4 Sep 2014 11:36:20 +0530
> > Bharata B Rao <address@hidden> wrote:
> > 
> > > Separate out CPU initialization code into a new routine
> > > ppc_new_cpu() so that it can be used from CPU hotplug path too.
> > > 
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > > ---
> > >  hw/ppc/spapr.c | 73
> > > +++++++++++++++++++++++++++++++++------------------------- 1 file
> > > changed, 42 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index b2ca527..41207ae 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1603,6 +1603,45 @@ static SaveVMHandlers savevm_htab_handlers
> > > = { .load_state = htab_load,
> > >  };
> > >  
> > looking at following code from POV of using CPU with device_add cmd.
> > 
> > > +static const char *current_cpu_model;
> > do PPC CPUs use full 'model-name,feature1,-feature2' format or only
> > model-name from cpu_model string?
> 
> I think it is just the model-name.
Then without lagacy baggage that x86 has, it should be possible
to convert PPC to device_add/del supported object. You wont't need a
global to keep model name, it will come as type name device_add command.

> 
> > 
> > > +static PowerPCCPU *ppc_new_cpu(const char *cpu_model)
> > > +{
> > > +    PowerPCCPU *cpu;
> > > +    CPUPPCState *env;
> > > +
> > > +    cpu = cpu_ppc_init(cpu_model);
for fully QOMified CPU you don't need cpu_ppc_init()
object_new() should be sufficient.

> > > +    if (cpu == NULL) {
> > > +        fprintf(stderr, "Unable to find PowerPC CPU
> > > definition\n");
> > > +        exit(1);
> > > +    }
> > 
> > -- cut --
> > > +    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);
> > > +        }
> > > +    }
> > -- cut --
> > selected block looks like setting CPU internals, which could be done
> > inside of CPU's realizefn.
> > 
> > > +
> > > +    xics_cpu_setup(spapr->icp, cpu);
xics_cpu_setup() looks like a wiring of CPU with external component.
I'd move it into plug handler and handle it in PPCMAchine object.

It should be possible to split/move the rest of this function into
initfn()/realize() of CPU.

> > 
> > 
> > > +    qemu_register_reset(spapr_cpu_reset, cpu);
> > also could be put inside of CPU's realizefn, like it's done in
> > for x86 CPU.
> 
> Right, I just converted my CPU hotplug patchset for PowerPC to use the
> hotplug handler APIs and now working on to see if I can switch over to
> device_add and support device_del for CPU hotplug on PowerPC.
> 
> Regards,
> Bharata.
>