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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3022 bytes --]

On Tue, Mar 01, 2016 at 01:28:56PM +0530, Bharata B Rao wrote:
> 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.

Ok, makes sense.

In that case can we move all, or nearly all, of the PAPR specific
thread initialization to rtas_start_cpu()?  Obviously we'd need a
separate call to the same stuff for the boot cpu.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]