[Qemu-devel] [RFC PATCH v0 0/5] sPAPR: Fix migration when CPUs are removed in random order

[Qemu-devel] [RFC PATCH v0 0/5] sPAPR: Fix migration when CPUs are removed in random order

[Bharata B Rao]

device_add/del based CPU hotplug and unplug support is upstream for
sPAPR PowerPC and is under development for x86. Both of these will
support CPU device removal in random order (and not necessarily in LIFO
order). Random order removal will result in holes in cpu_index range
which causes migration to fail. This needs fixes in both generic code
as well as arch specific code.

- We need to use arch_id as the instance_id when registering CPU devices
  using vmstate_register. To support forward migration, as per Igor's
  suggestion, this needs to be done conditionally based on machine type
  version.
- From pseries-2.7 onwards, we start using arch_id for migration as well
  as in XICS code.

In fact, Igor even suggested to move the vmstate registration calls
into cpu_common_realizefn and yet-to-be-introduced cpu_common_unrealizefn.
But I haven't done that in this patchset as I am hitting an unrelated
issue with that movement.

This patchset depends on Greg Kurz's patchset where among other things,
he is deriving cpu_dt_it (arch_id) based on core-id.
(https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00210.html) 
  
Tested changes to XICS code with both kernel_irqchip=on/off.
This applies on ppc-for-2.7 branch of David's tree.

Bharata B Rao (5):
  cpu: Factor out cpu vmstate_[un]register into separate routines
  cpu: Optionally use arch_id instead of cpu_index in cpu
    vmstate_register()
  spapr: Implement CPUClass.get_arch_id() for PowerPC CPUs
  xics: Use arch_id instead of cpu_index in XICS code
  spapr: Prefer arch_id over cpu_index

 exec.c                      | 53 +++++++++++++++++++++++++++++----------------
 hw/intc/xics.c              | 14 ++++++++----
 hw/intc/xics_kvm.c          | 12 +++++-----
 hw/intc/xics_spapr.c        | 33 ++++++++++++++++++++++------
 hw/ppc/spapr.c              |  2 ++
 hw/ppc/spapr_cpu_core.c     |  5 +++++
 include/hw/boards.h         |  1 +
 include/qom/cpu.h           |  4 ++++
 target-ppc/translate_init.c |  8 +++++++
 9 files changed, 96 insertions(+), 36 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Bharata B Rao]

Consolidates cpu vmstate_[un]register calls into separate routines.
No functionality change except that vmstate_unregister calls are
now done under !CONFIG_USER_ONLY to match with vmstate_register calls.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/exec.c b/exec.c
index 0122ef7..8ce8e90 100644
--- a/exec.c
+++ b/exec.c
@@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
     /* Return the AddressSpace corresponding to the specified index */
     return cpu->cpu_ases[asidx].as;
 }
-#endif
 
-#ifndef CONFIG_USER_ONLY
 static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
 
 static int cpu_get_free_index(Error **errp)
@@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
 {
     bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
 }
+
+static void cpu_vmstate_register(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
+    }
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
+    }
+}
+
+static void cpu_vmstate_unregister(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->vmsd != NULL) {
+        vmstate_unregister(NULL, cc->vmsd, cpu);
+    }
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+    }
+}
+
 #else
 
 static int cpu_get_free_index(Error **errp)
@@ -638,8 +661,6 @@ static void cpu_release_index(CPUState *cpu)
 
 void cpu_exec_exit(CPUState *cpu)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
-
 #if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
 #endif
@@ -656,19 +677,13 @@ void cpu_exec_exit(CPUState *cpu)
     cpu->cpu_index = -1;
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
+#else
+    cpu_vmstate_unregister(cpu);
 #endif
-
-    if (cc->vmsd != NULL) {
-        vmstate_unregister(NULL, cc->vmsd, cpu);
-    }
-    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
-    }
 }
 
 void cpu_exec_init(CPUState *cpu, Error **errp)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
 
     cpu->as = NULL;
@@ -705,15 +720,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     }
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
 #if defined(CONFIG_USER_ONLY)
-    (void) cc;
     cpu_list_unlock();
 #else
-    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
-    }
-    if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
-    }
+    cpu_vmstate_register(cpu);
 #endif
 }
 
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[David Gibson]

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

On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> Consolidates cpu vmstate_[un]register calls into separate routines.
> No functionality change except that vmstate_unregister calls are
> now done under !CONFIG_USER_ONLY to match with vmstate_register calls.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  exec.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..8ce8e90 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>      /* Return the AddressSpace corresponding to the specified index */
>      return cpu->cpu_ases[asidx].as;
>  }
> -#endif
>  
> -#ifndef CONFIG_USER_ONLY
>  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>  }
> +
> +static void cpu_vmstate_register(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> +    }
> +    if (cc->vmsd != NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +    }
> +}
> +
> +static void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->vmsd != NULL) {
> +        vmstate_unregister(NULL, cc->vmsd, cpu);
> +    }
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> +    }
> +}
> +

Given you're factoring this out, would it make sense to defined no-op
versions for CONFIG_USER_ONLY, to reduce the amount of ifdefs at the
call site?

>  #else
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -638,8 +661,6 @@ static void cpu_release_index(CPUState *cpu)
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -656,19 +677,13 @@ void cpu_exec_exit(CPUState *cpu)
>      cpu->cpu_index = -1;
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> +#else
> +    cpu_vmstate_unregister(cpu);
>  #endif
> -
> -    if (cc->vmsd != NULL) {
> -        vmstate_unregister(NULL, cc->vmsd, cpu);
> -    }
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> -    }
>  }
>  
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
>  
>      cpu->as = NULL;
> @@ -705,15 +720,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
> -    (void) cc;
>      cpu_list_unlock();
>  #else
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> -    }
> -    if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> -    }
> +    cpu_vmstate_register(cpu);
>  #endif
>  }
>  

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

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Bharata B Rao]

On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> > Consolidates cpu vmstate_[un]register calls into separate routines.
> > No functionality change except that vmstate_unregister calls are
> > now done under !CONFIG_USER_ONLY to match with vmstate_register calls.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> >  1 file changed, 28 insertions(+), 19 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 0122ef7..8ce8e90 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
> >      /* Return the AddressSpace corresponding to the specified index */
> >      return cpu->cpu_ases[asidx].as;
> >  }
> > -#endif
> >  
> > -#ifndef CONFIG_USER_ONLY
> >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> >  
> >  static int cpu_get_free_index(Error **errp)
> > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> >  {
> >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> >  }
> > +
> > +static void cpu_vmstate_register(CPUState *cpu)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > +        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> > +    }
> > +    if (cc->vmsd != NULL) {
> > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > +    }
> > +}
> > +
> > +static void cpu_vmstate_unregister(CPUState *cpu)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (cc->vmsd != NULL) {
> > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > +    }
> > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > +    }
> > +}
> > +
> 
> Given you're factoring this out, would it make sense to defined no-op
> versions for CONFIG_USER_ONLY, to reduce the amount of ifdefs at the
> call site?

I did that in a subsequent patch that moved the calls to these routines
into cpu_common_[un]realize(), but ended up in some unrelated issue and
hence didn't include that patch yet.

But as you note, we could do that with the existing code itself.
Commit 741da0d3 limited the vmstate_register calls to !CONFIG_USER_ONLY.
I am still not sure why cpu vmstate registration isn't needed for
CONFIG_USER_ONLY

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Igor Mammedov]

On Tue, 5 Jul 2016 10:46:07 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> > > Consolidates cpu vmstate_[un]register calls into separate
> > > routines. No functionality change except that vmstate_unregister
> > > calls are now done under !CONFIG_USER_ONLY to match with
> > > vmstate_register calls.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > > ---
> > >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index 0122ef7..8ce8e90 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> > > *cpu, int asidx) /* Return the AddressSpace corresponding to the
> > > specified index */ return cpu->cpu_ases[asidx].as;
> > >  }
> > > -#endif
> > >  
> > > -#ifndef CONFIG_USER_ONLY
> > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > >  
> > >  static int cpu_get_free_index(Error **errp)
> > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> > >  {
> > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > >  }
> > > +
> > > +static void cpu_vmstate_register(CPUState *cpu)
> > > +{
> > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > +
> > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > +        vmstate_register(NULL, cpu->cpu_index,
> > > &vmstate_cpu_common, cpu);
> > > +    }
> > > +    if (cc->vmsd != NULL) {
> > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > +    }
> > > +}
> > > +
> > > +static void cpu_vmstate_unregister(CPUState *cpu)
> > > +{
> > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > +
> > > +    if (cc->vmsd != NULL) {
> > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > +    }
> > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > +    }
> > > +}
> > > +
> > 
> > Given you're factoring this out, would it make sense to defined
> > no-op versions for CONFIG_USER_ONLY, to reduce the amount of ifdefs
> > at the call site?
> 
> I did that in a subsequent patch that moved the calls to these
> routines into cpu_common_[un]realize()cpu_common_[un]realize(), but ended up in some
> unrelated issue and hence didn't include that patch yet.
I'd prefer to see it moved to cpu_common_[un]realize() directly
without tis intermediate transition as compat logic could be
implemented much cleaner if it's there.

> 
> But as you note, we could do that with the existing code itself.
> Commit 741da0d3 limited the vmstate_register calls
> to !CONFIG_USER_ONLY. I am still not sure why cpu vmstate
> registration isn't needed for CONFIG_USER_ONLY
there isn't migration for *-user targets but exec.c is common, hence
ifdefs
 
> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Bharata B Rao]

On Tue, Jul 05, 2016 at 07:49:38AM +0200, Igor Mammedov wrote:
> On Tue, 5 Jul 2016 10:46:07 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> > > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> > > > Consolidates cpu vmstate_[un]register calls into separate
> > > > routines. No functionality change except that vmstate_unregister
> > > > calls are now done under !CONFIG_USER_ONLY to match with
> > > > vmstate_register calls.
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > 
> > > > ---
> > > >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/exec.c b/exec.c
> > > > index 0122ef7..8ce8e90 100644
> > > > --- a/exec.c
> > > > +++ b/exec.c
> > > > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> > > > *cpu, int asidx) /* Return the AddressSpace corresponding to the
> > > > specified index */ return cpu->cpu_ases[asidx].as;
> > > >  }
> > > > -#endif
> > > >  
> > > > -#ifndef CONFIG_USER_ONLY
> > > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > > >  
> > > >  static int cpu_get_free_index(Error **errp)
> > > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> > > >  {
> > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > >  }
> > > > +
> > > > +static void cpu_vmstate_register(CPUState *cpu)
> > > > +{
> > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > +
> > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > +        vmstate_register(NULL, cpu->cpu_index,
> > > > &vmstate_cpu_common, cpu);
> > > > +    }
> > > > +    if (cc->vmsd != NULL) {
> > > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void cpu_vmstate_unregister(CPUState *cpu)
> > > > +{
> > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > +
> > > > +    if (cc->vmsd != NULL) {
> > > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > > +    }
> > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > > +    }
> > > > +}
> > > > +
> > > 
> > > Given you're factoring this out, would it make sense to defined
> > > no-op versions for CONFIG_USER_ONLY, to reduce the amount of ifdefs
> > > at the call site?
> > 
> > I did that in a subsequent patch that moved the calls to these
> > routines into cpu_common_[un]realize()cpu_common_[un]realize(), but ended up in some
> > unrelated issue and hence didn't include that patch yet.
> I'd prefer to see it moved to cpu_common_[un]realize() directly
> without tis intermediate transition as compat logic could be
> implemented much cleaner if it's there.

If I implement cpu_common_unrealize() and the associated logic similar
to the existing cpu_common_realize(), it would involve changes to all
archs. I have done the change only to ppc in the below experimental patch.
Is this kind of change preferred or is it possible to do this in
non-invasive way ?

diff --git a/exec.c b/exec.c
index 2e8ad14..5274cc8 100644
--- a/exec.c
+++ b/exec.c
@@ -617,7 +617,7 @@ static void cpu_release_index(CPUState *cpu)
 }
 
 /* TODO: cpu_index is int while .get_arch_id()is int64_t */
-static void cpu_vmstate_register(CPUState *cpu)
+void cpu_vmstate_register(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
     int instance_id = cpu->prefer_arch_id_over_cpu_index ?
@@ -631,7 +631,7 @@ static void cpu_vmstate_register(CPUState *cpu)
     }
 }
 
-static void cpu_vmstate_unregister(CPUState *cpu)
+void cpu_vmstate_unregister(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
@@ -660,6 +660,14 @@ static void cpu_release_index(CPUState *cpu)
 {
     return;
 }
+
+void cpu_vmstate_register(CPUState *cpu)
+{
+}
+
+void cpu_vmstate_unregister(CPUState *cpu)
+{
+}
 #endif
 
 void cpu_exec_exit(CPUState *cpu)
@@ -680,8 +688,6 @@ void cpu_exec_exit(CPUState *cpu)
     cpu->cpu_index = -1;
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
-#else
-    cpu_vmstate_unregister(cpu);
 #endif
 }
 
@@ -724,8 +730,6 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
-#else
-    cpu_vmstate_register(cpu);
 #endif
 }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 1f1706e..08eab39 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -874,4 +874,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
     .offset = 0,                                                            \
 }
 
+void cpu_vmstate_register(CPUState *cpu);
+void cpu_vmstate_unregister(CPUState *cpu);
 #endif
diff --git a/qom/cpu.c b/qom/cpu.c
index 751e992..65623e8 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -314,12 +314,20 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
 
+    cpu_vmstate_register(cpu);
     if (dev->hotplugged) {
         cpu_synchronize_post_init(cpu);
         cpu_resume(cpu);
     }
 }
 
+static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
+{
+    CPUState *cpu = CPU(dev);
+
+    cpu_vmstate_unregister(cpu);
+}
+
 static void cpu_common_initfn(Object *obj)
 {
     CPUState *cpu = CPU(obj);
@@ -367,6 +375,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
     dc->realize = cpu_common_realizefn;
+    dc->unrealize = cpu_common_unrealizefn;
     /*
      * Reason: CPUs still need special care by board code: wiring up
      * IRQs, adding reset handlers, halting non-first CPUs, ...
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 2864105..9b8fe36 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -173,6 +173,7 @@ typedef struct PowerPCCPUClass {
     /*< public >*/
 
     DeviceRealize parent_realize;
+    DeviceUnrealize parent_unrealize;
     void (*parent_reset)(CPUState *cpu);
 
     uint32_t pvr;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 09602be..f4986e4 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9743,10 +9743,12 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
 static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(dev);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
     opc_handler_t **table;
     int i, j;
 
+    pcc->parent_unrealize(dev, errp);
     cpu_exec_exit(CPU(dev));
 
     for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
@@ -10336,6 +10338,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     pcc->parent_realize = dc->realize;
+    pcc->parent_unrealize = dc->unrealize;
     pcc->pvr_match = ppc_pvr_match_default;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
     dc->realize = ppc_cpu_realizefn;

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Igor Mammedov]

On Tue, 5 Jul 2016 12:05:35 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Jul 05, 2016 at 07:49:38AM +0200, Igor Mammedov wrote:
> > On Tue, 5 Jul 2016 10:46:07 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> > > > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> > > > > Consolidates cpu vmstate_[un]register calls into separate
> > > > > routines. No functionality change except that
> > > > > vmstate_unregister calls are now done under !CONFIG_USER_ONLY
> > > > > to match with vmstate_register calls.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > 
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > 
> > > > > ---
> > > > >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index 0122ef7..8ce8e90 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -594,9 +594,7 @@ AddressSpace
> > > > > *cpu_get_address_space(CPUState *cpu, int asidx) /* Return
> > > > > the AddressSpace corresponding to the specified index */
> > > > > return cpu->cpu_ases[asidx].as; }
> > > > > -#endif
> > > > >  
> > > > > -#ifndef CONFIG_USER_ONLY
> > > > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > > > >  
> > > > >  static int cpu_get_free_index(Error **errp)
> > > > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState
> > > > > *cpu) {
> > > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > > >  }
> > > > > +
> > > > > +static void cpu_vmstate_register(CPUState *cpu)
> > > > > +{
> > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +
> > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > +        vmstate_register(NULL, cpu->cpu_index,
> > > > > &vmstate_cpu_common, cpu);
> > > > > +    }
> > > > > +    if (cc->vmsd != NULL) {
> > > > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd,
> > > > > cpu);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void cpu_vmstate_unregister(CPUState *cpu)
> > > > > +{
> > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +
> > > > > +    if (cc->vmsd != NULL) {
> > > > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > > > +    }
> > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > 
> > > > Given you're factoring this out, would it make sense to defined
> > > > no-op versions for CONFIG_USER_ONLY, to reduce the amount of
> > > > ifdefs at the call site?
> > > 
> > > I did that in a subsequent patch that moved the calls to these
> > > routines into cpu_common_[un]realize()cpu_common_[un]realize(),
> > > but ended up in some unrelated issue and hence didn't include
> > > that patch yet.
> > I'd prefer to see it moved to cpu_common_[un]realize() directly
> > without tis intermediate transition as compat logic could be
> > implemented much cleaner if it's there.
> 
> If I implement cpu_common_unrealize() and the associated logic similar
> to the existing cpu_common_realize(), it would involve changes to all
> archs.
maybe I'm missing something but why all arch will be involved?
The only ones that could be affected are the ones that have their own
cpu_xxx_unrealize() implemented without calling CPUClass::unrealize.

 
>I have done the change only to ppc in the below experimental
> patch. Is this kind of change preferred or is it possible to do this
> in non-invasive way ?
> 
> diff --git a/exec.c b/exec.c
> index 2e8ad14..5274cc8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -617,7 +617,7 @@ static void cpu_release_index(CPUState *cpu)
>  }
>  
>  /* TODO: cpu_index is int while .get_arch_id()is int64_t */
> -static void cpu_vmstate_register(CPUState *cpu)
> +void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      int instance_id = cpu->prefer_arch_id_over_cpu_index ?
> @@ -631,7 +631,7 @@ static void cpu_vmstate_register(CPUState *cpu)
>      }
>  }
>  
> -static void cpu_vmstate_unregister(CPUState *cpu)
> +void cpu_vmstate_unregister(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> @@ -660,6 +660,14 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      return;
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +}
>  #endif
>  
>  void cpu_exec_exit(CPUState *cpu)
> @@ -680,8 +688,6 @@ void cpu_exec_exit(CPUState *cpu)
>      cpu->cpu_index = -1;
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#else
> -    cpu_vmstate_unregister(cpu);
>  #endif
>  }
>  
> @@ -724,8 +730,6 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#else
> -    cpu_vmstate_register(cpu);
>  #endif
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 1f1706e..08eab39 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -874,4 +874,6 @@ extern const struct VMStateDescription
> vmstate_cpu_common; .offset =
> 0,                                                            \ }
>  
> +void cpu_vmstate_register(CPUState *cpu);
> +void cpu_vmstate_unregister(CPUState *cpu);
>  #endif
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..65623e8 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -314,12 +314,20 @@ static void cpu_common_realizefn(DeviceState
> *dev, Error **errp) {
>      CPUState *cpu = CPU(dev);
>  
> +    cpu_vmstate_register(cpu);
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);
>      }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    CPUState *cpu = CPU(dev);
> +
> +    cpu_vmstate_unregister(cpu);
> +}
> +
>  static void cpu_common_initfn(Object *obj)
>  {
>      CPUState *cpu = CPU(obj);
> @@ -367,6 +375,7 @@ static void cpu_class_init(ObjectClass *klass,
> void *data) k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
>      dc->realize = cpu_common_realizefn;
> +    dc->unrealize = cpu_common_unrealizefn;
>      /*
>       * Reason: CPUs still need special care by board code: wiring up
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 2864105..9b8fe36 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -173,6 +173,7 @@ typedef struct PowerPCCPUClass {
>      /*< public >*/
>  
>      DeviceRealize parent_realize;
> +    DeviceUnrealize parent_unrealize;
>      void (*parent_reset)(CPUState *cpu);
>  
>      uint32_t pvr;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 09602be..f4986e4 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9743,10 +9743,12 @@ static void ppc_cpu_realizefn(DeviceState
> *dev, Error **errp) static void ppc_cpu_unrealizefn(DeviceState *dev,
> Error **errp) {
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>      opc_handler_t **table;
>      int i, j;
>  
> +    pcc->parent_unrealize(dev, errp);
>      cpu_exec_exit(CPU(dev));
>  
>      for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
> @@ -10336,6 +10338,7 @@ static void ppc_cpu_class_init(ObjectClass
> *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      pcc->parent_realize = dc->realize;
> +    pcc->parent_unrealize = dc->unrealize;
>      pcc->pvr_match = ppc_pvr_match_default;
>      pcc->interrupts_big_endian =
> ppc_cpu_interrupts_big_endian_always; dc->realize = ppc_cpu_realizefn;
> 

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Bharata B Rao]

On Tue, Jul 05, 2016 at 09:22:30AM +0200, Igor Mammedov wrote:
> On Tue, 5 Jul 2016 12:05:35 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Jul 05, 2016 at 07:49:38AM +0200, Igor Mammedov wrote:
> > > On Tue, 5 Jul 2016 10:46:07 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> > > > > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> > > > > > Consolidates cpu vmstate_[un]register calls into separate
> > > > > > routines. No functionality change except that
> > > > > > vmstate_unregister calls are now done under !CONFIG_USER_ONLY
> > > > > > to match with vmstate_register calls.
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > 
> > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > 
> > > > > > ---
> > > > > >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> > > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/exec.c b/exec.c
> > > > > > index 0122ef7..8ce8e90 100644
> > > > > > --- a/exec.c
> > > > > > +++ b/exec.c
> > > > > > @@ -594,9 +594,7 @@ AddressSpace
> > > > > > *cpu_get_address_space(CPUState *cpu, int asidx) /* Return
> > > > > > the AddressSpace corresponding to the specified index */
> > > > > > return cpu->cpu_ases[asidx].as; }
> > > > > > -#endif
> > > > > >  
> > > > > > -#ifndef CONFIG_USER_ONLY
> > > > > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > > > > >  
> > > > > >  static int cpu_get_free_index(Error **errp)
> > > > > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState
> > > > > > *cpu) {
> > > > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > > > >  }
> > > > > > +
> > > > > > +static void cpu_vmstate_register(CPUState *cpu)
> > > > > > +{
> > > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > > +
> > > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > > +        vmstate_register(NULL, cpu->cpu_index,
> > > > > > &vmstate_cpu_common, cpu);
> > > > > > +    }
> > > > > > +    if (cc->vmsd != NULL) {
> > > > > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd,
> > > > > > cpu);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static void cpu_vmstate_unregister(CPUState *cpu)
> > > > > > +{
> > > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > > +
> > > > > > +    if (cc->vmsd != NULL) {
> > > > > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > > > > +    }
> > > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > Given you're factoring this out, would it make sense to defined
> > > > > no-op versions for CONFIG_USER_ONLY, to reduce the amount of
> > > > > ifdefs at the call site?
> > > > 
> > > > I did that in a subsequent patch that moved the calls to these
> > > > routines into cpu_common_[un]realize()cpu_common_[un]realize(),
> > > > but ended up in some unrelated issue and hence didn't include
> > > > that patch yet.
> > > I'd prefer to see it moved to cpu_common_[un]realize() directly
> > > without tis intermediate transition as compat logic could be
> > > implemented much cleaner if it's there.
> > 
> > If I implement cpu_common_unrealize() and the associated logic similar
> > to the existing cpu_common_realize(), it would involve changes to all
> > archs.
> maybe I'm missing something but why all arch will be involved?
> The only ones that could be affected are the ones that have their own
> cpu_xxx_unrealize() implemented without calling CPUClass::unrealize.

You are right, I didn't realize that most archs don't define their own
cpu_xxx_unrealize call. So this should be a simple change. Will include
a patch to move vmstate_[un]register() calls to cpu_common_[un]realize()
in the next version.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Igor Mammedov]

On Tue, 5 Jul 2016 13:08:28 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Jul 05, 2016 at 09:22:30AM +0200, Igor Mammedov wrote:
> > On Tue, 5 Jul 2016 12:05:35 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Tue, Jul 05, 2016 at 07:49:38AM +0200, Igor Mammedov wrote:
> > > > On Tue, 5 Jul 2016 10:46:07 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> > > > > > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao
> > > > > > wrote:
> > > > > > > Consolidates cpu vmstate_[un]register calls into separate
> > > > > > > routines. No functionality change except that
> > > > > > > vmstate_unregister calls are now done
> > > > > > > under !CONFIG_USER_ONLY to match with vmstate_register
> > > > > > > calls.
> > > > > > > 
> > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > 
> > > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > 
> > > > > > > ---
> > > > > > >  exec.c | 47
> > > > > > > ++++++++++++++++++++++++++++------------------- 1 file
> > > > > > > changed, 28 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/exec.c b/exec.c
> > > > > > > index 0122ef7..8ce8e90 100644
> > > > > > > --- a/exec.c
> > > > > > > +++ b/exec.c
> > > > > > > @@ -594,9 +594,7 @@ AddressSpace
> > > > > > > *cpu_get_address_space(CPUState *cpu, int asidx) /* Return
> > > > > > > the AddressSpace corresponding to the specified index */
> > > > > > > return cpu->cpu_ases[asidx].as; }
> > > > > > > -#endif
> > > > > > >  
> > > > > > > -#ifndef CONFIG_USER_ONLY
> > > > > > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > > > > > >  
> > > > > > >  static int cpu_get_free_index(Error **errp)
> > > > > > > @@ -617,6 +615,31 @@ static void
> > > > > > > cpu_release_index(CPUState *cpu) {
> > > > > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > > > > >  }
> > > > > > > +
> > > > > > > +static void cpu_vmstate_register(CPUState *cpu)
> > > > > > > +{
> > > > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > > > +
> > > > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > > > +        vmstate_register(NULL, cpu->cpu_index,
> > > > > > > &vmstate_cpu_common, cpu);
> > > > > > > +    }
> > > > > > > +    if (cc->vmsd != NULL) {
> > > > > > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd,
> > > > > > > cpu);
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void cpu_vmstate_unregister(CPUState *cpu)
> > > > > > > +{
> > > > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > > > +
> > > > > > > +    if (cc->vmsd != NULL) {
> > > > > > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > > > > > +    }
> > > > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > > > +        vmstate_unregister(NULL, &vmstate_cpu_common,
> > > > > > > cpu);
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > 
> > > > > > Given you're factoring this out, would it make sense to
> > > > > > defined no-op versions for CONFIG_USER_ONLY, to reduce the
> > > > > > amount of ifdefs at the call site?
> > > > > 
> > > > > I did that in a subsequent patch that moved the calls to these
> > > > > routines into
> > > > > cpu_common_[un]realize()cpu_common_[un]realize(), but ended
> > > > > up in some unrelated issue and hence didn't include that
> > > > > patch yet.
> > > > I'd prefer to see it moved to cpu_common_[un]realize() directly
> > > > without tis intermediate transition as compat logic could be
> > > > implemented much cleaner if it's there.
> > > 
> > > If I implement cpu_common_unrealize() and the associated logic
> > > similar to the existing cpu_common_realize(), it would involve
> > > changes to all archs.
> > maybe I'm missing something but why all arch will be involved?
> > The only ones that could be affected are the ones that have their
> > own cpu_xxx_unrealize() implemented without calling
> > CPUClass::unrealize.
> 
> You are right, I didn't realize that most archs don't define their own
> cpu_xxx_unrealize call. So this should be a simple change. Will
> include a patch to move vmstate_[un]register() calls to
> cpu_common_[un]realize() in the next version.
just replace this patch with cpu_common_[un]realize() variant

> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Greg Kurz]

On Tue, 5 Jul 2016 12:05:35 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Jul 05, 2016 at 07:49:38AM +0200, Igor Mammedov wrote:
> > On Tue, 5 Jul 2016 10:46:07 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:  
> > > > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:  
> > > > > Consolidates cpu vmstate_[un]register calls into separate
> > > > > routines. No functionality change except that vmstate_unregister
> > > > > calls are now done under !CONFIG_USER_ONLY to match with
> > > > > vmstate_register calls.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>  
> > > > 
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > >   
> > > > > ---
> > > > >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index 0122ef7..8ce8e90 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> > > > > *cpu, int asidx) /* Return the AddressSpace corresponding to the
> > > > > specified index */ return cpu->cpu_ases[asidx].as;
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > > -#ifndef CONFIG_USER_ONLY
> > > > >  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > > > >  
> > > > >  static int cpu_get_free_index(Error **errp)
> > > > > @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
> > > > >  {
> > > > >      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > > > >  }
> > > > > +
> > > > > +static void cpu_vmstate_register(CPUState *cpu)
> > > > > +{
> > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +
> > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > +        vmstate_register(NULL, cpu->cpu_index,
> > > > > &vmstate_cpu_common, cpu);
> > > > > +    }
> > > > > +    if (cc->vmsd != NULL) {
> > > > > +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void cpu_vmstate_unregister(CPUState *cpu)
> > > > > +{
> > > > > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > > > > +
> > > > > +    if (cc->vmsd != NULL) {
> > > > > +        vmstate_unregister(NULL, cc->vmsd, cpu);
> > > > > +    }
> > > > > +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> > > > > +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> > > > > +    }
> > > > > +}
> > > > > +  
> > > > 
> > > > Given you're factoring this out, would it make sense to defined
> > > > no-op versions for CONFIG_USER_ONLY, to reduce the amount of ifdefs
> > > > at the call site?  
> > > 
> > > I did that in a subsequent patch that moved the calls to these
> > > routines into cpu_common_[un]realize()cpu_common_[un]realize(), but ended up in some
> > > unrelated issue and hence didn't include that patch yet.  
> > I'd prefer to see it moved to cpu_common_[un]realize() directly
> > without tis intermediate transition as compat logic could be
> > implemented much cleaner if it's there.  
> 
> If I implement cpu_common_unrealize() and the associated logic similar
> to the existing cpu_common_realize(), it would involve changes to all
> archs. I have done the change only to ppc in the below experimental patch.
> Is this kind of change preferred or is it possible to do this in
> non-invasive way ?
> 

Even if affects all targets, the change is quite mechanical and should not
be a problem I guess.

> diff --git a/exec.c b/exec.c
> index 2e8ad14..5274cc8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -617,7 +617,7 @@ static void cpu_release_index(CPUState *cpu)
>  }
>  
>  /* TODO: cpu_index is int while .get_arch_id()is int64_t */
> -static void cpu_vmstate_register(CPUState *cpu)
> +void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      int instance_id = cpu->prefer_arch_id_over_cpu_index ?
> @@ -631,7 +631,7 @@ static void cpu_vmstate_register(CPUState *cpu)
>      }
>  }
>  
> -static void cpu_vmstate_unregister(CPUState *cpu)
> +void cpu_vmstate_unregister(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> @@ -660,6 +660,14 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      return;
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +}
>  #endif
>  
>  void cpu_exec_exit(CPUState *cpu)
> @@ -680,8 +688,6 @@ void cpu_exec_exit(CPUState *cpu)
>      cpu->cpu_index = -1;
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#else
> -    cpu_vmstate_unregister(cpu);
>  #endif
>  }
>  
> @@ -724,8 +730,6 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#else
> -    cpu_vmstate_register(cpu);
>  #endif
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 1f1706e..08eab39 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -874,4 +874,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
>      .offset = 0,                                                            \
>  }
>  
> +void cpu_vmstate_register(CPUState *cpu);
> +void cpu_vmstate_unregister(CPUState *cpu);
>  #endif
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..65623e8 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -314,12 +314,20 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
> +    cpu_vmstate_register(cpu);
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);
>      }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    CPUState *cpu = CPU(dev);
> +
> +    cpu_vmstate_unregister(cpu);
> +}
> +
>  static void cpu_common_initfn(Object *obj)
>  {
>      CPUState *cpu = CPU(obj);
> @@ -367,6 +375,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
>      dc->realize = cpu_common_realizefn;
> +    dc->unrealize = cpu_common_unrealizefn;
>      /*
>       * Reason: CPUs still need special care by board code: wiring up
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 2864105..9b8fe36 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -173,6 +173,7 @@ typedef struct PowerPCCPUClass {
>      /*< public >*/
>  
>      DeviceRealize parent_realize;
> +    DeviceUnrealize parent_unrealize;
>      void (*parent_reset)(CPUState *cpu);
>  
>      uint32_t pvr;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 09602be..f4986e4 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9743,10 +9743,12 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>  static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>      opc_handler_t **table;
>      int i, j;
>  
> +    pcc->parent_unrealize(dev, errp);
>      cpu_exec_exit(CPU(dev));
>  
>      for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
> @@ -10336,6 +10338,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      pcc->parent_realize = dc->realize;
> +    pcc->parent_unrealize = dc->unrealize;
>      pcc->pvr_match = ppc_pvr_match_default;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_always;
>      dc->realize = ppc_cpu_realizefn;
> 

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

[Bharata B Rao]

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

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

[David Gibson]

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

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

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

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

[Bharata B Rao]

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.

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

[Igor Mammedov]

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

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

[Igor Mammedov]

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.

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

[Bharata B Rao]

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.

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

[Igor Mammedov]

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

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

[Bharata B Rao]

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.

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

[David Gibson]

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

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

[Greg Kurz]

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

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

[Greg Kurz]

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

[Qemu-devel] [RFC PATCH v0 3/5] spapr: Implement CPUClass.get_arch_id() for PowerPC CPUs

[Bharata B Rao]

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 8f257fb..b810624 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10357,6 +10357,13 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
 #endif
 }
 
+static int64_t ppc_cpu_get_arch_id(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    return cpu->cpu_dt_id;
+}
+
 static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
@@ -10409,6 +10416,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #ifndef CONFIG_USER_ONLY
     cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
 #endif
+    cc->get_arch_id = ppc_cpu_get_arch_id;
 
     dc->fw_name = "PowerPC,UNKNOWN";
 }
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v0 3/5] spapr: Implement CPUClass.get_arch_id() for PowerPC CPUs

[David Gibson]

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

On Tue, Jul 05, 2016 at 10:12:50AM +0530, Bharata B Rao wrote:
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Longer term we should probably change the field name to arch_id.  In
theory we could have something like this on a platform that didn't do
device trees.

> ---
>  target-ppc/translate_init.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 8f257fb..b810624 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10357,6 +10357,13 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
>  #endif
>  }
>  
> +static int64_t ppc_cpu_get_arch_id(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +    return cpu->cpu_dt_id;
> +}
> +
>  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -10409,6 +10416,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>  #ifndef CONFIG_USER_ONLY
>      cc->virtio_is_big_endian = ppc_cpu_is_big_endian;
>  #endif
> +    cc->get_arch_id = ppc_cpu_get_arch_id;
>  
>      dc->fw_name = "PowerPC,UNKNOWN";
>  }

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

[Qemu-devel] [RFC PATCH v0 4/5] xics: Use arch_id instead of cpu_index in XICS code

[Bharata B Rao]

xics maintains an array of ICPState structures which is indexed
by cpu_index. Change this to index the ICPState array by arch_id
for pseries-2.7 onwards. This allows migration of guest to suceed
when there are holes in cpu_index range due to CPU hot removal.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/intc/xics.c       | 14 ++++++++++----
 hw/intc/xics_kvm.c   | 12 ++++++------
 hw/intc/xics_spapr.c | 33 ++++++++++++++++++++++++++-------
 3 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index cd48f42..6b79a8e 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -50,9 +50,12 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
 void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
+    ICPState *ss = &xics->ss[server];
 
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(server < xics->nr_servers);
     assert(cs == ss->cs);
 
     ss->output = NULL;
@@ -63,10 +66,13 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
-    ICPState *ss = &xics->ss[cs->cpu_index];
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
+    ICPState *ss = &xics->ss[server];
     XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
 
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(server < xics->nr_servers);
 
     ss->cs = cs;
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index edbd62f..2610458 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -326,14 +326,14 @@ static const TypeInfo ics_kvm_info = {
  */
 static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
 {
-    CPUState *cs;
-    ICPState *ss;
+    CPUState *cs = CPU(cpu);
     KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
+    ICPState *ss = ss = &xics->ss[server];
 
-    cs = CPU(cpu);
-    ss = &xics->ss[cs->cpu_index];
-
-    assert(cs->cpu_index < xics->nr_servers);
+    assert(server < xics->nr_servers);
     if (xicskvm->kernel_xics_fd == -1) {
         abort();
     }
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 618826d..fbc8205 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -43,16 +43,21 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
     target_ulong cppr = args[0];
 
-    icp_set_cppr(spapr->xics, cs->cpu_index, cppr);
+    icp_set_cppr(spapr->xics, server, cppr);
     return H_SUCCESS;
 }
 
 static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
+    CPUState *cs = CPU(cpu);
+    target_ulong server = cs->prefer_arch_id_over_cpu_index ? args[0] :
+                          xics_get_cpu_index_by_dt_id(args[0]);
     target_ulong mfrr = args[1];
 
     if (server >= spapr->xics->nr_servers) {
@@ -67,7 +72,10 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
-    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
+    uint32_t xirr = icp_accept(spapr->xics->ss + server);
 
     args[0] = xirr;
     return H_SUCCESS;
@@ -77,7 +85,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                              target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
+    ICPState *ss = &spapr->xics->ss[server];
     uint32_t xirr = icp_accept(ss);
 
     args[0] = xirr;
@@ -89,9 +100,12 @@ static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
     target_ulong xirr = args[0];
 
-    icp_eoi(spapr->xics, cs->cpu_index, xirr);
+    icp_eoi(spapr->xics, server, xirr);
     return H_SUCCESS;
 }
 
@@ -99,8 +113,11 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                             target_ulong opcode, target_ulong *args)
 {
     CPUState *cs = CPU(cpu);
+    CPUClass *cc = CPU_GET_CLASS(cs);
+    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
+                 cs->cpu_index;
     uint32_t mfrr;
-    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr);
+    uint32_t xirr = icp_ipoll(spapr->xics->ss + server, &mfrr);
 
     args[0] = xirr;
     args[1] = mfrr;
@@ -113,6 +130,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           uint32_t nargs, target_ulong args,
                           uint32_t nret, target_ulong rets)
 {
+    CPUState *cs = CPU(cpu);
     ICSState *ics = spapr->xics->ics;
     uint32_t nr, server, priority;
 
@@ -122,7 +140,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     nr = rtas_ld(args, 0);
-    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
+    server = cs->prefer_arch_id_over_cpu_index ? rtas_ld(args, 1) :
+             xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
     priority = rtas_ld(args, 2);
 
     if (!ics_valid_irq(ics, nr) || (server >= ics->xics->nr_servers)
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v0 4/5] xics: Use arch_id instead of cpu_index in XICS code

[David Gibson]

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

On Tue, Jul 05, 2016 at 10:12:51AM +0530, Bharata B Rao wrote:
> xics maintains an array of ICPState structures which is indexed
> by cpu_index. Change this to index the ICPState array by arch_id
> for pseries-2.7 onwards. This allows migration of guest to suceed
> when there are holes in cpu_index range due to CPU hot removal.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/intc/xics.c       | 14 ++++++++++----
>  hw/intc/xics_kvm.c   | 12 ++++++------
>  hw/intc/xics_spapr.c | 33 ++++++++++++++++++++++++++-------
>  3 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index cd48f42..6b79a8e 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -50,9 +50,12 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>  void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> -    ICPState *ss = &xics->ss[cs->cpu_index];
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;

Requiring this conditional in all the callsites is pretty ugly.  I'd
prefer to see a get_arch_id() helper that implements this fallback
internally.

> +    ICPState *ss = &xics->ss[server];
>  
> -    assert(cs->cpu_index < xics->nr_servers);
> +    assert(server < xics->nr_servers);
>      assert(cs == ss->cs);
>  
>      ss->output = NULL;
> @@ -63,10 +66,13 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> -    ICPState *ss = &xics->ss[cs->cpu_index];
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
> +    ICPState *ss = &xics->ss[server];
>      XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
>  
> -    assert(cs->cpu_index < xics->nr_servers);
> +    assert(server < xics->nr_servers);
>  
>      ss->cs = cs;
>  
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index edbd62f..2610458 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -326,14 +326,14 @@ static const TypeInfo ics_kvm_info = {
>   */
>  static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>  {
> -    CPUState *cs;
> -    ICPState *ss;
> +    CPUState *cs = CPU(cpu);
>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
> +    ICPState *ss = ss = &xics->ss[server];
>  
> -    cs = CPU(cpu);
> -    ss = &xics->ss[cs->cpu_index];
> -
> -    assert(cs->cpu_index < xics->nr_servers);
> +    assert(server < xics->nr_servers);
>      if (xicskvm->kernel_xics_fd == -1) {
>          abort();
>      }
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 618826d..fbc8205 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -43,16 +43,21 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
>      target_ulong cppr = args[0];
>  
> -    icp_set_cppr(spapr->xics, cs->cpu_index, cppr);
> +    icp_set_cppr(spapr->xics, server, cppr);
>      return H_SUCCESS;
>  }
>  
>  static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
> -    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
> +    CPUState *cs = CPU(cpu);
> +    target_ulong server = cs->prefer_arch_id_over_cpu_index ? args[0] :
> +                          xics_get_cpu_index_by_dt_id(args[0]);
>      target_ulong mfrr = args[1];
>  
>      if (server >= spapr->xics->nr_servers) {
> @@ -67,7 +72,10 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> -    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
> +    uint32_t xirr = icp_accept(spapr->xics->ss + server);
>  
>      args[0] = xirr;
>      return H_SUCCESS;
> @@ -77,7 +85,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                               target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> -    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
> +    ICPState *ss = &spapr->xics->ss[server];
>      uint32_t xirr = icp_accept(ss);
>  
>      args[0] = xirr;
> @@ -89,9 +100,12 @@ static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
>      target_ulong xirr = args[0];
>  
> -    icp_eoi(spapr->xics, cs->cpu_index, xirr);
> +    icp_eoi(spapr->xics, server, xirr);
>      return H_SUCCESS;
>  }
>  
> @@ -99,8 +113,11 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>  {
>      CPUState *cs = CPU(cpu);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    int server = cs->prefer_arch_id_over_cpu_index ? cc->get_arch_id(cs) :
> +                 cs->cpu_index;
>      uint32_t mfrr;
> -    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr);
> +    uint32_t xirr = icp_ipoll(spapr->xics->ss + server, &mfrr);
>  
>      args[0] = xirr;
>      args[1] = mfrr;
> @@ -113,6 +130,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            uint32_t nargs, target_ulong args,
>                            uint32_t nret, target_ulong rets)
>  {
> +    CPUState *cs = CPU(cpu);
>      ICSState *ics = spapr->xics->ics;
>      uint32_t nr, server, priority;
>  
> @@ -122,7 +140,8 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      nr = rtas_ld(args, 0);
> -    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
> +    server = cs->prefer_arch_id_over_cpu_index ? rtas_ld(args, 1) :
> +             xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
>      priority = rtas_ld(args, 2);
>  
>      if (!ics_valid_irq(ics, nr) || (server >= ics->xics->nr_servers)

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

Re: [Qemu-devel] [RFC PATCH v0 4/5] xics: Use arch_id instead of cpu_index in XICS code

[Igor Mammedov]

On Tue, 5 Jul 2016 14:59:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jul 05, 2016 at 10:12:51AM +0530, Bharata B Rao wrote:
> > xics maintains an array of ICPState structures which is indexed
> > by cpu_index. Change this to index the ICPState array by arch_id
> > for pseries-2.7 onwards. This allows migration of guest to suceed
> > when there are holes in cpu_index range due to CPU hot removal.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/intc/xics.c       | 14 ++++++++++----
> >  hw/intc/xics_kvm.c   | 12 ++++++------
> >  hw/intc/xics_spapr.c | 33 ++++++++++++++++++++++++++-------
> >  3 files changed, 42 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index cd48f42..6b79a8e 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -50,9 +50,12 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
> >  void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
> >  {
> >      CPUState *cs = CPU(cpu);
> > -    ICPState *ss = &xics->ss[cs->cpu_index];
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> 
> Requiring this conditional in all the callsites is pretty ugly.  I'd
> prefer to see a get_arch_id() helper that implements this fallback
> internally.
A cpu can look at prefer_arch_id_over_cpu_index and switch
behavior of get_arch_id() internally.

> 
> > +    ICPState *ss = &xics->ss[server];
> >  
> > -    assert(cs->cpu_index < xics->nr_servers);
> > +    assert(server < xics->nr_servers);
> >      assert(cs == ss->cs);
> >  
> >      ss->output = NULL;
> > @@ -63,10 +66,13 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU
> > *cpu) {
> >      CPUState *cs = CPU(cpu);
> >      CPUPPCState *env = &cpu->env;
> > -    ICPState *ss = &xics->ss[cs->cpu_index];
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> > +    ICPState *ss = &xics->ss[server];
> >      XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
> >  
> > -    assert(cs->cpu_index < xics->nr_servers);
> > +    assert(server < xics->nr_servers);
> >  
> >      ss->cs = cs;
> >  
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index edbd62f..2610458 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -326,14 +326,14 @@ static const TypeInfo ics_kvm_info = {
> >   */
> >  static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
> >  {
> > -    CPUState *cs;
> > -    ICPState *ss;
> > +    CPUState *cs = CPU(cpu);
> >      KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> > +    ICPState *ss = ss = &xics->ss[server];
> >  
> > -    cs = CPU(cpu);
> > -    ss = &xics->ss[cs->cpu_index];
> > -
> > -    assert(cs->cpu_index < xics->nr_servers);
> > +    assert(server < xics->nr_servers);
> >      if (xicskvm->kernel_xics_fd == -1) {
> >          abort();
> >      }
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 618826d..fbc8205 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -43,16 +43,21 @@ static target_ulong h_cppr(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, target_ulong opcode, target_ulong *args)
> >  {
> >      CPUState *cs = CPU(cpu);
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> >      target_ulong cppr = args[0];
> >  
> > -    icp_set_cppr(spapr->xics, cs->cpu_index, cppr);
> > +    icp_set_cppr(spapr->xics, server, cppr);
> >      return H_SUCCESS;
> >  }
> >  
> >  static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState
> > *spapr, target_ulong opcode, target_ulong *args)
> >  {
> > -    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
> > +    CPUState *cs = CPU(cpu);
> > +    target_ulong server = cs->prefer_arch_id_over_cpu_index ?
> > args[0] :
> > +                          xics_get_cpu_index_by_dt_id(args[0]);
> >      target_ulong mfrr = args[1];
> >  
> >      if (server >= spapr->xics->nr_servers) {
> > @@ -67,7 +72,10 @@ static target_ulong h_xirr(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, target_ulong opcode, target_ulong *args)
> >  {
> >      CPUState *cs = CPU(cpu);
> > -    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> > +    uint32_t xirr = icp_accept(spapr->xics->ss + server);
> >  
> >      args[0] = xirr;
> >      return H_SUCCESS;
> > @@ -77,7 +85,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, target_ulong opcode, target_ulong *args)
> >  {
> >      CPUState *cs = CPU(cpu);
> > -    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> > +    ICPState *ss = &spapr->xics->ss[server];
> >      uint32_t xirr = icp_accept(ss);
> >  
> >      args[0] = xirr;
> > @@ -89,9 +100,12 @@ static target_ulong h_eoi(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, target_ulong opcode, target_ulong *args)
> >  {
> >      CPUState *cs = CPU(cpu);
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> >      target_ulong xirr = args[0];
> >  
> > -    icp_eoi(spapr->xics, cs->cpu_index, xirr);
> > +    icp_eoi(spapr->xics, server, xirr);
> >      return H_SUCCESS;
> >  }
> >  
> > @@ -99,8 +113,11 @@ static target_ulong h_ipoll(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, target_ulong opcode, target_ulong *args)
> >  {
> >      CPUState *cs = CPU(cpu);
> > +    CPUClass *cc = CPU_GET_CLASS(cs);
> > +    int server = cs->prefer_arch_id_over_cpu_index ?
> > cc->get_arch_id(cs) :
> > +                 cs->cpu_index;
> >      uint32_t mfrr;
> > -    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index,
> > &mfrr);
> > +    uint32_t xirr = icp_ipoll(spapr->xics->ss + server, &mfrr);
> >  
> >      args[0] = xirr;
> >      args[1] = mfrr;
> > @@ -113,6 +130,7 @@ static void rtas_set_xive(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, uint32_t nargs, target_ulong args,
> >                            uint32_t nret, target_ulong rets)
> >  {
> > +    CPUState *cs = CPU(cpu);
> >      ICSState *ics = spapr->xics->ics;
> >      uint32_t nr, server, priority;
> >  
> > @@ -122,7 +140,8 @@ static void rtas_set_xive(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr, }
> >  
> >      nr = rtas_ld(args, 0);
> > -    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
> > +    server = cs->prefer_arch_id_over_cpu_index ? rtas_ld(args, 1) :
> > +             xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
> >      priority = rtas_ld(args, 2);
> >  
> >      if (!ics_valid_irq(ics, nr) || (server >=
> > ics->xics->nr_servers)
> 

[Qemu-devel] [RFC PATCH v0 5/5] spapr: Prefer arch_id over cpu_index

[Bharata B Rao]

Starting from pseries-2.7, prefer the use of arch_id (cpu_dt_id) over
cpu_index for cpu vmstate registration and in XICS code.

This allows migration to work when CPU cores are not necessarily
unplugged in LIFO order.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c          | 2 ++
 hw/ppc/spapr_cpu_core.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f33a1b..caa6254 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2426,6 +2426,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = spapr_machine_device_unplug;
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
     mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus;
+    mc->prefer_arch_id_over_cpu_index = true;
 
     smc->dr_lmb_enabled = true;
     smc->dr_cpu_enabled = true;
@@ -2512,6 +2513,7 @@ static void spapr_machine_2_6_class_options(MachineClass *mc)
     sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
 
     spapr_machine_2_7_class_options(mc);
+    mc->prefer_arch_id_over_cpu_index = false;
     smc->dr_cpu_enabled = false;
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_6);
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 70b6b0b..4bfd01e 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -281,6 +281,8 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
     const char *typename = object_class_get_name(sc->cpu_class);
@@ -295,6 +297,9 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         obj = sc->threads + i * size;
 
         object_initialize(obj, size, typename);
+        if (mc->prefer_arch_id_over_cpu_index) {
+            CPU(obj)->prefer_arch_id_over_cpu_index = true;
+        }
         snprintf(id, sizeof(id), "thread[%d]", i);
         object_property_add_child(OBJECT(sc), id, obj, &local_err);
         if (local_err) {
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v0 0/5] sPAPR: Fix migration when CPUs are removed in random order

[Greg Kurz]

On Tue,  5 Jul 2016 10:12:47 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> device_add/del based CPU hotplug and unplug support is upstream for
> sPAPR PowerPC and is under development for x86. Both of these will
> support CPU device removal in random order (and not necessarily in LIFO
> order). Random order removal will result in holes in cpu_index range
> which causes migration to fail. This needs fixes in both generic code
> as well as arch specific code.
> 
> - We need to use arch_id as the instance_id when registering CPU devices
>   using vmstate_register. To support forward migration, as per Igor's
>   suggestion, this needs to be done conditionally based on machine type
>   version.
> - From pseries-2.7 onwards, we start using arch_id for migration as well
>   as in XICS code.
> 
> In fact, Igor even suggested to move the vmstate registration calls
> into cpu_common_realizefn and yet-to-be-introduced cpu_common_unrealizefn.
> But I haven't done that in this patchset as I am hitting an unrelated
> issue with that movement.
> 
> This patchset depends on Greg Kurz's patchset where among other things,
> he is deriving cpu_dt_it (arch_id) based on core-id.
> (https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00210.html) 
>   

I'm respinning my patchset according to the latest comments I got. This
includes rebasing on top of Eduardo's tree to have the global parse_features
bits, as mentioned here:

https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00622.html

I'll rework this patch to apply on the current code:

https://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg07564.html

> Tested changes to XICS code with both kernel_irqchip=on/off.
> This applies on ppc-for-2.7 branch of David's tree.
> 
> Bharata B Rao (5):
>   cpu: Factor out cpu vmstate_[un]register into separate routines
>   cpu: Optionally use arch_id instead of cpu_index in cpu
>     vmstate_register()
>   spapr: Implement CPUClass.get_arch_id() for PowerPC CPUs
>   xics: Use arch_id instead of cpu_index in XICS code
>   spapr: Prefer arch_id over cpu_index
> 
>  exec.c                      | 53 +++++++++++++++++++++++++++++----------------
>  hw/intc/xics.c              | 14 ++++++++----
>  hw/intc/xics_kvm.c          | 12 +++++-----
>  hw/intc/xics_spapr.c        | 33 ++++++++++++++++++++++------
>  hw/ppc/spapr.c              |  2 ++
>  hw/ppc/spapr_cpu_core.c     |  5 +++++
>  include/hw/boards.h         |  1 +
>  include/qom/cpu.h           |  4 ++++
>  target-ppc/translate_init.c |  8 +++++++
>  9 files changed, 96 insertions(+), 36 deletions(-)
>