Re: [Qemu-devel] [RFC PATCH v0 4/6] spapr: CPU hotplug support

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

On Mon, Feb 29, 2016 at 10:12:10AM +0530, Bharata B Rao wrote:
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index b7c5ebd..cc0369e 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -34,6 +34,7 @@
> > >  
> > >  #include "hw/ppc/spapr.h"
> > >  #include "hw/ppc/spapr_vio.h"
> > > +#include "hw/ppc/ppc.h"
> > >  #include "qapi-event.h"
> > >  #include "hw/boards.h"
> > >  
> > > @@ -161,6 +162,27 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
> > >      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > >  }
> > >  
> > > +/*
> > > + * Set the timebase offset of the CPU to that of first CPU.
> > > + * This helps hotplugged CPU to have the correct timebase offset.
> > > + */
> > > +static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> > > +{
> > > +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > > +
> > > +    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> > > +}
> > > +
> > > +static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> > > +{
> > > +    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> > > +
> > > +    if (!pcc->interrupts_big_endian(fcpu)) {
> > > +        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> > > +    }
> > > +}
> > > +
> > 
> > Any particular reason for doing these things at rtas_start_cpu() time,
> > but other initialization at plug time?  Could you consolidate it to
> > one place or the other?
> 
> Those board specific things that are needed to be done have been consolidated
> into spapr_cpu_init() which will be called from the plug handler. We have
> discussed this earlier at:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg04399.html
> 
> It has been a while but there was a good reason why setting endianness
> here rather than in plug handler is necessary. W/o this LE hotplug on guests
> wouldn't work, I will dig up and come back on what exactly necessiated
> this change.

If we set LPCR_ILE in cpu->env.spr[SPR_LPCR] at plug time
(from spapr_cpu_init()), there are at least two places later where it gets
over-written. One is spapr_cpu_reset() and the other one when
kvm_cpu_synchronize_state() is called from rtas_start_cpu(). We could
probably issue a kvm_arch_put_registers(), but I found rtas_start_cpu()
as a place where this change is guaranteed to get reflected.

Regards,
Bharata.