[RFC,v0,2/5] cpu: Optionally use arch_id instead of cpu_index in cpu vmstate_register()

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(-)

On Tue, Jul 05, 2016 at 10:12:49AM +0530, Bharata B Rao 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>

How is migration between a version with this flag and a version
without this flag handled?

> ---
>  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 ?
> + */
>  static void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cpu->prefer_arch_id_over_cpu_index ?
> +                      cc->get_arch_id(cpu) : cpu->cpu_index;
>  
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +        vmstate_register(NULL, instance_id, &vmstate_cpu_common, cpu);
>      }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
>      }
>  }
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ed6155..decabba 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -123,6 +123,7 @@ struct MachineClass {
>      ram_addr_t default_ram_size;
>      bool option_rom_has_mr;
>      bool rom_file_has_mr;
> +    bool prefer_arch_id_over_cpu_index;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..1f1706e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -273,6 +273,9 @@ struct qemu_work_item {
>   * @kvm_fd: vCPU file descriptor for KVM.
>   * @work_mutex: Lock to prevent multiple access to queued_work_*.
>   * @queued_work_first: First asynchronous work pending.
> + * @prefer_arch_id_over_cpu_index: Set to enforce the use of
> + *         CPUClass.get_arch_id() over cpu_index during vmstate registration
> + *         and any other uses by target machine where arch_id is preferred.
>   *
>   * State of one CPU core or thread.
>   */
> @@ -360,6 +363,7 @@ struct CPUState {
>         (absolute value) offset as small as possible.  This reduces code
>         size, especially for hosts without large memory offsets.  */
>      uint32_t tcg_exit_req;
> +    bool prefer_arch_id_over_cpu_index;
>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);

On Tue, Jul 05, 2016 at 02:56:53PM +1000, David Gibson wrote:
> On Tue, Jul 05, 2016 at 10:12:49AM +0530, Bharata B Rao 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>
> 
> How is migration between a version with this flag and a version
> without this flag handled?

There can't be such a case as this flag is enabled for a given machine version.

Regards,
Bharata.

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.
you could reuse compat mechanism like x86 instead of adding doing #1
see PC_COMPAT_2_6 and how it's used

> 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 ?
> + */
>  static void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cpu->prefer_arch_id_over_cpu_index ?
> +                      cc->get_arch_id(cpu) : cpu->cpu_index;
>  
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> cpu);
> +        vmstate_register(NULL, instance_id, &vmstate_cpu_common,
> cpu); }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
>      }
>  }
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ed6155..decabba 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -123,6 +123,7 @@ struct MachineClass {
>      ram_addr_t default_ram_size;
>      bool option_rom_has_mr;
>      bool rom_file_has_mr;
> +    bool prefer_arch_id_over_cpu_index;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..1f1706e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -273,6 +273,9 @@ struct qemu_work_item {
>   * @kvm_fd: vCPU file descriptor for KVM.
>   * @work_mutex: Lock to prevent multiple access to queued_work_*.
>   * @queued_work_first: First asynchronous work pending.
> + * @prefer_arch_id_over_cpu_index: Set to enforce the use of
> + *         CPUClass.get_arch_id() over cpu_index during vmstate
> registration
> + *         and any other uses by target machine where arch_id is
> preferred. *
>   * State of one CPU core or thread.
>   */
> @@ -360,6 +363,7 @@ struct CPUState {
>         (absolute value) offset as small as possible.  This reduces
> code size, especially for hosts without large memory offsets.  */
>      uint32_t tcg_exit_req;
> +    bool prefer_arch_id_over_cpu_index;
this property applies to all cpu, so shouldn't it be class property?

>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);

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.

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 ?
> + */

The current migration code assumes instance_id is a 32-bit word:

migration/savevm.c:        qemu_put_be32(f, se->instance_id);

>  static void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int instance_id = cpu->prefer_arch_id_over_cpu_index ?
> +                      cc->get_arch_id(cpu) : cpu->cpu_index;
>  
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +        vmstate_register(NULL, instance_id, &vmstate_cpu_common, cpu);
>      }
>      if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +        vmstate_register(NULL, instance_id, cc->vmsd, cpu);
>      }
>  }
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ed6155..decabba 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -123,6 +123,7 @@ struct MachineClass {
>      ram_addr_t default_ram_size;
>      bool option_rom_has_mr;
>      bool rom_file_has_mr;
> +    bool prefer_arch_id_over_cpu_index;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..1f1706e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -273,6 +273,9 @@ struct qemu_work_item {
>   * @kvm_fd: vCPU file descriptor for KVM.
>   * @work_mutex: Lock to prevent multiple access to queued_work_*.
>   * @queued_work_first: First asynchronous work pending.
> + * @prefer_arch_id_over_cpu_index: Set to enforce the use of
> + *         CPUClass.get_arch_id() over cpu_index during vmstate registration
> + *         and any other uses by target machine where arch_id is preferred.
>   *
>   * State of one CPU core or thread.
>   */
> @@ -360,6 +363,7 @@ struct CPUState {
>         (absolute value) offset as small as possible.  This reduces code
>         size, especially for hosts without large memory offsets.  */
>      uint32_t tcg_exit_req;
> +    bool prefer_arch_id_over_cpu_index;
>  };
>  
>  QTAILQ_HEAD(CPUTailQ, CPUState);

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 ?

Regards,
Bharata.

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.



> 
> Regards,
> Bharata.
>

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.

Regards,
Bharata.

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.

On Wed, 6 Jul 2016 07:25:44 +0200
Igor Mammedov <imammedo@redhat.com> 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.
> 

On pseries-2.7, CPUs are stored in a 2-dimensional array but it also
contains max_cpus elements actually.

Bharata sent this patch to add @stable_cpu_id to CPUState:

https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01702.html

Do we really need the associated property if we provide a stable index
in the [0..max_cpus) range ? This does not depend on the machine type or
version unless I've missed something.

> 
> 
> > 
> > Regards,
> > Bharata.
> >   
>