Re: [Qemu-devel] [RFC PATCH v0 2/5] cpu: Optionally use arch_id instead of cpu_index in cpu vmstate_register()

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

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

On Wed, Jul 06, 2016 at 01:55:25PM +0530, Bharata B Rao wrote:
> On Wed, Jul 06, 2016 at 07:25:44AM +0200, Igor Mammedov wrote:
> > On Tue, 5 Jul 2016 18:13:22 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Tue, Jul 05, 2016 at 09:15:51AM +0200, Igor Mammedov wrote:
> > > > On Tue,  5 Jul 2016 10:12:49 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > Introduce CPUState.prefer_arch_id_over_cpu_index and
> > > > > MachineClass.prefer_arch_id_over_cpu_index that allow target
> > > > > machines to optionally switch to using arch_id instead of
> > > > > cpu_index as instance_id in vmstate_register(). This will help
> > > > > allow successful migration in cases where holes are introduced in
> > > > > cpu_index range after CPU hot removals.
> > > > > 
> > > > > Whether to use arch_id or cpu_index is based on machine type
> > > > > version and hence added
> > > > > MachineClass.prefer_arch_id_over_cpu_index. However the
> > > > > enforcement is via and during CPU creation and hence added
> > > > > CPUState.prefer_arch_id_over_cpu_index. So it becomes a two step
> > > > > process for the target to enable the use of arch_id:
> > > > > 
> > > > > 1. Set MachineClass.prefer_arch_id_over_cpu_index.
> > > > > 2. Ensure CPUState.prefer_arch_id_over_cpu_index is set for all
> > > > > CPUs based on 1. above.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > >  exec.c              | 10 ++++++++--
> > > > >  include/hw/boards.h |  1 +
> > > > >  include/qom/cpu.h   |  4 ++++
> > > > >  3 files changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index 8ce8e90..7cc1d06 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -616,15 +616,21 @@ static void cpu_release_index(CPUState *cpu)
> > > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * TODO: cpu_index and instance_id are of type int
> > > > > while .get_arch_id()is
> > > > > + * of type int64_t. What is the consequence of changing
> > > > > instance_id to int64_t ?
> > > > > + */
> > > > ARM potentially could have get_arch_id() == MPIDR (return 64bit int)
> > > > that would get truncated in this case.
> > > > 
> > > > I wonder if we could add "int CPUState::migration_id" and let
> > > > machine code set it to what it uses for stable cpu id and then
> > > > cpu_realize could call vmstate_register with it.
> > > 
> > > Hmm how would that help ? instance_id argument to vmstate_register()
> > > is being treated as 32 bit integer as pointed by Greg earlier.
> > > 
> > > If we want to use 64 bit arch_id/migration_id as instance_id arg in
> > > vmstate_register, shouldn't we be changing instance_id to 64 bit ?
> > Trying to extend instance_id from 32bit to 64bit, looks hard to me as
> > it's on wire format so it's not possible to change without breaking
> > compatibility.
> > 
> > That's why dedicated migration_id should also be 32bit integer to keep
> > it compatible with vmstate_foo() magic. On x86 and ARM that id will be
> > in range [0..maxcpus), it's stable index in possible_cpus array.
> 
> Since we were planning to use arch_id (aka device tree id) as the stable
> index for CPU cores on PowerPC, we would have to use int64_t arch_id
> itself as int instance_id I suppose. The range could be
> [0...maxcpus*8] for us.

Huh?  I don't see how this comment relates to the one before.

-- 
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 --]