Re: [Qemu-devel] [Qemu-ppc] [PATCH 6/8] spapr: init CPUState->cpu_index with index relative to core-id

[Greg Kurz]

On Fri, 22 Jul 2016 17:14:33 +1000
David Gibson <address@hidden> wrote:

> On Fri, Jul 22, 2016 at 11:40:03AM +0530, Bharata B Rao wrote:
> > On Fri, Jul 22, 2016 at 01:23:01PM +1000, David Gibson wrote:  
> > > On Thu, Jul 21, 2016 at 05:54:37PM +0200, Igor Mammedov wrote:  
> > > > It will enshure that cpu_index for a given cpu stays the same
> > > > regardless of the order cpus has been created/deleted and so
> > > > it would be possible to migrate QEMU instance with out of order
> > > > created CPU.
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>  
> > > 
> > > So, this isn't quite right (it wasn't right in my version either).
> > > 
> > > The problem occurs when smp_threads < kvmppc_smt_threads().  That is,
> > > when the requested threads-per-core is less than the hardware's
> > > maximum number of threads-per-core.
> > > 
> > > The core-id values are assigned essentially as i *
> > > kvmppc_smt_threads(), meaning the patch below will leave gaps in the
> > > cpu_index values and the last ones will exceed max_cpus, causing other
> > > problems.  
> > 
> > This would lead to hotplug failures as cpu_dt_id is still being
> > derived from non-contiguous cpu_index resulting in wrong enumeration
> > of CPU nodes in DT.  
> 
> Which "This" are you referring to?
> 
> > 
> > For -smp 8,threads=4 we see the following CPU nodes in DT
> > 
> > PowerPC,address@hidden PowerPC,address@hidden
> > 
> > which otherwise should have been
> > 
> > PowerPC,address@hidden PowerPC,address@hidden
> > 
> > The problem manifests as drmgr failure.
> > 
> > Greg's patchset that moved cpu_dt_id setting to machine code and that
> > derived cpu_dt_id from core-id for sPAPR would be needed to fix this
> > I guess.  
> 
> No, it shouldn't be necessary to fix it.  But we certainly do want to
> clean this stuff up.  I'm not terribly convinced by the current
> approach in Greg's series though.  I'd actually prefer to remove
> cpu_dt_id from the cpustate entirely and instead work it out from the

That was the initial plan for my series :)

> (now stable) cpu index when we go to construct the device tree.
> 

Patch 5/8 provides a stable cpu_index for the pc machine type out of the
index in possible_cpus[].

Since we also store cores and threads in fixed size arrays, we could easily
follow the same logic: 

index_of_core_in_spapr_cores * smp_threads + index_of_thread_in_core_threads

Cheers.

--
Greg

pgpiwcZA4K2gM.pgp
Description: OpenPGP digital signature