[Qemu-devel] [PATCH 0/8] Fix migration issues with arbitrary cpu-hot(un)plug

[Qemu-devel] [PATCH 0/8] Fix migration issues with arbitrary cpu-hot(un)plug

[Igor Mammedov]

Series fixes migration issues caused by unstable cpu_index which depended
on order cpus were created/destroyed. It follows David's idea to make
cpu_index assignable by selected boards if board supports cpu-hotplug
with device_add and needs stable cpu_index/'migration id' but leaves
behaviour of the same as before for users that don't care about
cpu-hot(un)plug making changes low-risk.

tested with:
  SRC -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
       -device qemu64-x86_64-cpu,id=cpudel,apic-id=1 \
       -device qemu64-x86_64-cpu,apic-id=2 
  (qemu) device_del cpudel
  (qemu) stop
  (qemu) migrate "exec:gzip -c > STATEFILE.gz"
  
  DST -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
      -device qemu64-x86_64-cpu,apic-id=2 \
      -incoming "exec: gzip -c -d STATEFILE.gz"

git tree to test with:
     https://github.com/imammedo/qemu cpu-index-stable
 to view
     https://github.com/imammedo/qemu/commits/cpu-index-stable

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Peter Crosthwaite <crosthwaite.peter@gmail.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Alexander Graf <agraf@suse.de>
CC: Riku Voipio <riku.voipio@iki.fi>
CC: Bharata B Rao <bharata@linux.vnet.ibm.com>
CC: qemu-ppc@nongnu.org

David Gibson (1):
  Revert "spapr: Ensure CPU cores are added contiguously and removed in
    LIFO order"

Igor Mammedov (7):
  exec: reduce CONFIG_USER_ONLY ifdeffenery
  exec: don't use cpu_index to detect if cpu_exec_init()'s been called
    for cpu
  exec: set cpu_index only if it's been explictly set
  qdev: fix object reference leak in case device.realize() fails
  pc: init CPUState->cpu_index with index in possible_cpus[]
  spapr: init CPUState->cpu_index with index relative to core-id
  Revert "pc: Enforce adding CPUs contiguously and removing them in
    opposite order"

 bsd-user/qemu.h         |  2 --
 include/exec/exec-all.h | 12 +++++++++
 include/qemu/queue.h    |  2 ++
 include/qom/cpu.h       |  2 ++
 linux-user/qemu.h       |  2 --
 exec.c                  | 65 +++++++++----------------------------------------
 hw/core/qdev.c          |  8 +++++-
 hw/i386/pc.c            | 38 +++--------------------------
 hw/ppc/spapr_cpu_core.c | 25 ++++---------------
 qom/cpu.c               |  2 +-
 10 files changed, 44 insertions(+), 114 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/8] exec: reduce CONFIG_USER_ONLY ifdeffenery

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 bsd-user/qemu.h         |  2 --
 include/exec/exec-all.h | 12 ++++++++++++
 linux-user/qemu.h       |  2 --
 exec.c                  | 17 +++--------------
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 6ccc544..2b2b918 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -209,8 +209,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
                        abi_ulong new_addr);
 int target_msync(abi_ulong start, abi_ulong len, int flags);
 extern unsigned long last_brk;
-void cpu_list_lock(void);
-void cpu_list_unlock(void);
 #if defined(CONFIG_USE_NPTL)
 void mmap_fork_start(void);
 void mmap_fork_end(int child);
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index acda7b6..d008296 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -56,6 +56,18 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
                               uint32_t flags,
                               int cflags);
+#if defined(CONFIG_USER_ONLY)
+void cpu_list_lock(void);
+void cpu_list_unlock(void);
+#else
+static inline void cpu_list_unlock(void)
+{
+}
+static inline void cpu_list_lock(void)
+{
+}
+#endif
+
 void cpu_exec_init(CPUState *cpu, Error **errp);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index cdf23a7..bef465d 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -419,8 +419,6 @@ int target_msync(abi_ulong start, abi_ulong len, int flags);
 extern unsigned long last_brk;
 extern abi_ulong mmap_next_start;
 abi_ulong mmap_find_vma(abi_ulong, abi_ulong);
-void cpu_list_lock(void);
-void cpu_list_unlock(void);
 void mmap_fork_start(void);
 void mmap_fork_end(int child);
 
diff --git a/exec.c b/exec.c
index 60cf46a..2f57c62 100644
--- a/exec.c
+++ b/exec.c
@@ -642,23 +642,17 @@ void cpu_exec_exit(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
-#if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
-#endif
     if (cpu->cpu_index == -1) {
         /* cpu_index was never allocated by this @cpu or was already freed. */
-#if defined(CONFIG_USER_ONLY)
         cpu_list_unlock();
-#endif
         return;
     }
 
     QTAILQ_REMOVE(&cpus, cpu, node);
     cpu_release_index(cpu);
     cpu->cpu_index = -1;
-#if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
-#endif
 
     if (cc->vmsd != NULL) {
         vmstate_unregister(NULL, cc->vmsd, cpu);
@@ -670,7 +664,7 @@ void cpu_exec_exit(CPUState *cpu)
 
 void cpu_exec_init(CPUState *cpu, Error **errp)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
+    CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
 
     cpu->as = NULL;
@@ -694,22 +688,17 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     object_ref(OBJECT(cpu->memory));
 #endif
 
-#if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
-#endif
     cpu->cpu_index = cpu_get_free_index(&local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-#if defined(CONFIG_USER_ONLY)
         cpu_list_unlock();
-#endif
         return;
     }
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
-#if defined(CONFIG_USER_ONLY)
-    (void) cc;
     cpu_list_unlock();
-#else
+
+#ifndef CONFIG_USER_ONLY
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/8] exec: reduce CONFIG_USER_ONLY ifdeffenery

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:32PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

I think this is long overdue.

> ---
>  bsd-user/qemu.h         |  2 --
>  include/exec/exec-all.h | 12 ++++++++++++
>  linux-user/qemu.h       |  2 --
>  exec.c                  | 17 +++--------------
>  4 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index 6ccc544..2b2b918 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -209,8 +209,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
>                         abi_ulong new_addr);
>  int target_msync(abi_ulong start, abi_ulong len, int flags);
>  extern unsigned long last_brk;
> -void cpu_list_lock(void);
> -void cpu_list_unlock(void);
>  #if defined(CONFIG_USE_NPTL)
>  void mmap_fork_start(void);
>  void mmap_fork_end(int child);
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index acda7b6..d008296 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -56,6 +56,18 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>                                target_ulong pc, target_ulong cs_base,
>                                uint32_t flags,
>                                int cflags);
> +#if defined(CONFIG_USER_ONLY)
> +void cpu_list_lock(void);
> +void cpu_list_unlock(void);
> +#else
> +static inline void cpu_list_unlock(void)
> +{
> +}
> +static inline void cpu_list_lock(void)
> +{
> +}
> +#endif
> +
>  void cpu_exec_init(CPUState *cpu, Error **errp);
>  void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index cdf23a7..bef465d 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -419,8 +419,6 @@ int target_msync(abi_ulong start, abi_ulong len, int flags);
>  extern unsigned long last_brk;
>  extern abi_ulong mmap_next_start;
>  abi_ulong mmap_find_vma(abi_ulong, abi_ulong);
> -void cpu_list_lock(void);
> -void cpu_list_unlock(void);
>  void mmap_fork_start(void);
>  void mmap_fork_end(int child);
>  
> diff --git a/exec.c b/exec.c
> index 60cf46a..2f57c62 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -642,23 +642,17 @@ void cpu_exec_exit(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> -#if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
> -#endif
>      if (cpu->cpu_index == -1) {
>          /* cpu_index was never allocated by this @cpu or was already freed. */
> -#if defined(CONFIG_USER_ONLY)
>          cpu_list_unlock();
> -#endif
>          return;
>      }
>  
>      QTAILQ_REMOVE(&cpus, cpu, node);
>      cpu_release_index(cpu);
>      cpu->cpu_index = -1;
> -#if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#endif
>  
>      if (cc->vmsd != NULL) {
>          vmstate_unregister(NULL, cc->vmsd, cpu);
> @@ -670,7 +664,7 @@ void cpu_exec_exit(CPUState *cpu)
>  
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
>  
>      cpu->as = NULL;
> @@ -694,22 +688,17 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      object_ref(OBJECT(cpu->memory));
>  #endif
>  
> -#if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
> -#endif
>      cpu->cpu_index = cpu_get_free_index(&local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -#if defined(CONFIG_USER_ONLY)
>          cpu_list_unlock();
> -#endif
>          return;
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
> -#if defined(CONFIG_USER_ONLY)
> -    (void) cc;
>      cpu_list_unlock();
> -#else
> +
> +#ifndef CONFIG_USER_ONLY
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>          vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
>      }

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

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

[Qemu-devel] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu

[Igor Mammedov]

Instead use QTAIL's tqe_prev field to detect if cpu's been
placed in list by cpu_exec_init() which is always set if
QTAIL element is in list.

Fixes SIGSEGV on failure path in case cpu_index is assigned
by board and cpu.relalize() fails before cpu_exec_init() is called.

In follow up patches, cpu_index will be assigned by boards that
support cpu hot(un)plug and need stable cpu_index that doesn't
depend on order cpus are created/removed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: David Gibson <david@gibson.dropbear.id.au>
---
 include/qemu/queue.h | 2 ++
 exec.c               | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index c2b6c81..2c2c74b 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -407,6 +407,7 @@ struct {                                                                \
         else                                                            \
                 (head)->tqh_last = (elm)->field.tqe_prev;               \
         *(elm)->field.tqe_prev = (elm)->field.tqe_next;                 \
+        (elm)->field.tqe_prev = NULL;                                   \
 } while (/*CONSTCOND*/0)
 
 #define QTAILQ_FOREACH(var, head, field)                                \
@@ -430,6 +431,7 @@ struct {                                                                \
 #define QTAILQ_EMPTY(head)               ((head)->tqh_first == NULL)
 #define QTAILQ_FIRST(head)               ((head)->tqh_first)
 #define QTAILQ_NEXT(elm, field)          ((elm)->field.tqe_next)
+#define QTAILQ_IN_USE(elm, field)        ((elm)->field.tqe_prev)
 
 #define QTAILQ_LAST(head, headname) \
         (*(((struct headname *)((head)->tqh_last))->tqh_last))
diff --git a/exec.c b/exec.c
index 2f57c62..8c5da32 100644
--- a/exec.c
+++ b/exec.c
@@ -643,8 +643,8 @@ void cpu_exec_exit(CPUState *cpu)
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
     cpu_list_lock();
-    if (cpu->cpu_index == -1) {
-        /* cpu_index was never allocated by this @cpu or was already freed. */
+    if (!QTAILQ_IN_USE(cpu, node)) {
+        /* there is nothing to undo since cpu_exec_init() hasn't been called */
         cpu_list_unlock();
         return;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:33PM +0200, Igor Mammedov wrote:
> Instead use QTAIL's tqe_prev field to detect if cpu's been
> placed in list by cpu_exec_init() which is always set if
> QTAIL element is in list.
> 
> Fixes SIGSEGV on failure path in case cpu_index is assigned
> by board and cpu.relalize() fails before cpu_exec_init() is called.
> 
> In follow up patches, cpu_index will be assigned by boards that
> support cpu hot(un)plug and need stable cpu_index that doesn't
> depend on order cpus are created/removed.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reported-by: David Gibson <david@gibson.dropbear.id.au>

Looks correct, although I wonder a bit about changing QTAILQ_REMOVE()
for everyone for the sake of this one use case.

> ---
>  include/qemu/queue.h | 2 ++
>  exec.c               | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> index c2b6c81..2c2c74b 100644
> --- a/include/qemu/queue.h
> +++ b/include/qemu/queue.h
> @@ -407,6 +407,7 @@ struct {                                                                \
>          else                                                            \
>                  (head)->tqh_last = (elm)->field.tqe_prev;               \
>          *(elm)->field.tqe_prev = (elm)->field.tqe_next;                 \
> +        (elm)->field.tqe_prev = NULL;                                   \
>  } while (/*CONSTCOND*/0)
>  
>  #define QTAILQ_FOREACH(var, head, field)                                \
> @@ -430,6 +431,7 @@ struct {                                                                \
>  #define QTAILQ_EMPTY(head)               ((head)->tqh_first == NULL)
>  #define QTAILQ_FIRST(head)               ((head)->tqh_first)
>  #define QTAILQ_NEXT(elm, field)          ((elm)->field.tqe_next)
> +#define QTAILQ_IN_USE(elm, field)        ((elm)->field.tqe_prev)
>  
>  #define QTAILQ_LAST(head, headname) \
>          (*(((struct headname *)((head)->tqh_last))->tqh_last))
> diff --git a/exec.c b/exec.c
> index 2f57c62..8c5da32 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -643,8 +643,8 @@ void cpu_exec_exit(CPUState *cpu)
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
>      cpu_list_lock();
> -    if (cpu->cpu_index == -1) {
> -        /* cpu_index was never allocated by this @cpu or was already freed. */
> +    if (!QTAILQ_IN_USE(cpu, node)) {
> +        /* there is nothing to undo since cpu_exec_init() hasn't been called */
>          cpu_list_unlock();
>          return;
>      }

-- 
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] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu

[Eduardo Habkost]

On Fri, Jul 22, 2016 at 11:32:48AM +1000, David Gibson wrote:
> On Thu, Jul 21, 2016 at 05:54:33PM +0200, Igor Mammedov wrote:
> > Instead use QTAIL's tqe_prev field to detect if cpu's been
> > placed in list by cpu_exec_init() which is always set if
> > QTAIL element is in list.
> > 
> > Fixes SIGSEGV on failure path in case cpu_index is assigned
> > by board and cpu.relalize() fails before cpu_exec_init() is called.
> > 
> > In follow up patches, cpu_index will be assigned by boards that
> > support cpu hot(un)plug and need stable cpu_index that doesn't
> > depend on order cpus are created/removed.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reported-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Looks correct, although I wonder a bit about changing QTAILQ_REMOVE()
> for everyone for the sake of this one use case.

I don't mind changing it, but it's better to do it as a separate
patch so it can get the attention from other people. There are a
few cases where code checks tqe_prev directly and I don't know
how it could affect them.

Considering we don't want to take too long to include those
changes, I think we should add the tqe_prev=NULL assignment to
cpu_exec_exit() first (it's not the first time we do this in QEMU
code, see commit f8aa905a), and propose moving it to
QTAILQ_REMOVE later (and give it enough time for people to review
it).

> 
> > ---
> >  include/qemu/queue.h | 2 ++
> >  exec.c               | 4 ++--
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> > index c2b6c81..2c2c74b 100644
> > --- a/include/qemu/queue.h
> > +++ b/include/qemu/queue.h
> > @@ -407,6 +407,7 @@ struct {                                                                \
> >          else                                                            \
> >                  (head)->tqh_last = (elm)->field.tqe_prev;               \
> >          *(elm)->field.tqe_prev = (elm)->field.tqe_next;                 \
> > +        (elm)->field.tqe_prev = NULL;                                   \
> >  } while (/*CONSTCOND*/0)
> >  
> >  #define QTAILQ_FOREACH(var, head, field)                                \
> > @@ -430,6 +431,7 @@ struct {                                                                \
> >  #define QTAILQ_EMPTY(head)               ((head)->tqh_first == NULL)
> >  #define QTAILQ_FIRST(head)               ((head)->tqh_first)
> >  #define QTAILQ_NEXT(elm, field)          ((elm)->field.tqe_next)
> > +#define QTAILQ_IN_USE(elm, field)        ((elm)->field.tqe_prev)
> >  
> >  #define QTAILQ_LAST(head, headname) \
> >          (*(((struct headname *)((head)->tqh_last))->tqh_last))
> > diff --git a/exec.c b/exec.c
> > index 2f57c62..8c5da32 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -643,8 +643,8 @@ void cpu_exec_exit(CPUState *cpu)
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> >  
> >      cpu_list_lock();
> > -    if (cpu->cpu_index == -1) {
> > -        /* cpu_index was never allocated by this @cpu or was already freed. */
> > +    if (!QTAILQ_IN_USE(cpu, node)) {
> > +        /* there is nothing to undo since cpu_exec_init() hasn't been called */
> >          cpu_list_unlock();
> >          return;
> >      }
> 
> -- 
> 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



-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/8] exec: don't use cpu_index to detect if cpu_exec_init()'s been called for cpu

[Igor Mammedov]

On Fri, 22 Jul 2016 17:46:40 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jul 22, 2016 at 11:32:48AM +1000, David Gibson wrote:
> > On Thu, Jul 21, 2016 at 05:54:33PM +0200, Igor Mammedov wrote:  
> > > Instead use QTAIL's tqe_prev field to detect if cpu's been
> > > placed in list by cpu_exec_init() which is always set if
> > > QTAIL element is in list.
> > > 
> > > Fixes SIGSEGV on failure path in case cpu_index is assigned
> > > by board and cpu.relalize() fails before cpu_exec_init() is called.
> > > 
> > > In follow up patches, cpu_index will be assigned by boards that
> > > support cpu hot(un)plug and need stable cpu_index that doesn't
> > > depend on order cpus are created/removed.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reported-by: David Gibson <david@gibson.dropbear.id.au>  
> > 
> > Looks correct, although I wonder a bit about changing QTAILQ_REMOVE()
> > for everyone for the sake of this one use case.  
> 
> I don't mind changing it, but it's better to do it as a separate
> patch so it can get the attention from other people. There are a
> few cases where code checks tqe_prev directly and I don't know
> how it could affect them.
I've looked at non qtail users of the field
 blockdev.c
 net/filter.c

and they also use NULL in tqe_prev as a sign that element
is not in list and net/filter.c removes element from list after
check (i.e the similar pattern as here.)

> 
> Considering we don't want to take too long to include those
> changes, I think we should add the tqe_prev=NULL assignment to
> cpu_exec_exit() first (it's not the first time we do this in QEMU
> code, see commit f8aa905a), and propose moving it to
> QTAILQ_REMOVE later (and give it enough time for people to review
> it).
I'll do v2 with local fixup as reply to this thread
and look later at QTAIL_REMOVE (if I won't forget).

> 
> >   
> > > ---
> > >  include/qemu/queue.h | 2 ++
> > >  exec.c               | 4 ++--
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> > > index c2b6c81..2c2c74b 100644
> > > --- a/include/qemu/queue.h
> > > +++ b/include/qemu/queue.h
> > > @@ -407,6 +407,7 @@ struct {                                                                \
> > >          else                                                            \
> > >                  (head)->tqh_last = (elm)->field.tqe_prev;               \
> > >          *(elm)->field.tqe_prev = (elm)->field.tqe_next;                 \
> > > +        (elm)->field.tqe_prev = NULL;                                   \
> > >  } while (/*CONSTCOND*/0)
> > >  
> > >  #define QTAILQ_FOREACH(var, head, field)                                \
> > > @@ -430,6 +431,7 @@ struct {                                                                \
> > >  #define QTAILQ_EMPTY(head)               ((head)->tqh_first == NULL)
> > >  #define QTAILQ_FIRST(head)               ((head)->tqh_first)
> > >  #define QTAILQ_NEXT(elm, field)          ((elm)->field.tqe_next)
> > > +#define QTAILQ_IN_USE(elm, field)        ((elm)->field.tqe_prev)
> > >  
> > >  #define QTAILQ_LAST(head, headname) \
> > >          (*(((struct headname *)((head)->tqh_last))->tqh_last))
> > > diff --git a/exec.c b/exec.c
> > > index 2f57c62..8c5da32 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -643,8 +643,8 @@ void cpu_exec_exit(CPUState *cpu)
> > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > >  
> > >      cpu_list_lock();
> > > -    if (cpu->cpu_index == -1) {
> > > -        /* cpu_index was never allocated by this @cpu or was already freed. */
> > > +    if (!QTAILQ_IN_USE(cpu, node)) {
> > > +        /* there is nothing to undo since cpu_exec_init() hasn't been called */
> > >          cpu_list_unlock();
> > >          return;
> > >      }  
> > 
> > -- 
> > 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  
> 
> 
> 

[Qemu-devel] [PATCH 3/8] exec: set cpu_index only if it's been explictly set

[Igor Mammedov]

it keeps the legacy behavior for all users that doesn't care
about stable cpu_index value, but would allow boards that
would support device_add/device_del to set stable cpu_index
that won't depend on order in which cpus are created/destroyed.

While at that simplify cpu_get_free_index() as cpu_index
generated by USER_ONLY and softmmu variants is the same
since none of the users support cpu-remove so far except
of not yet released spapr/x86 device_add/del. which
will be altered in follow up patches to provide stable
cpu_index.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/qom/cpu.h |  2 ++
 exec.c            | 44 ++++++--------------------------------------
 qom/cpu.c         |  2 +-
 3 files changed, 9 insertions(+), 39 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index cbcd64c..ce0c406 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -883,4 +883,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
     .offset = 0,                                                            \
 }
 
+#define UNASSIGNED_CPU_INDEX -1
+
 #endif
diff --git a/exec.c b/exec.c
index 8c5da32..8e8416b 100644
--- a/exec.c
+++ b/exec.c
@@ -598,30 +598,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 }
 #endif
 
-#ifndef CONFIG_USER_ONLY
-static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
-
-static int cpu_get_free_index(Error **errp)
-{
-    int cpu = find_first_zero_bit(cpu_index_map, MAX_CPUMASK_BITS);
-
-    if (cpu >= MAX_CPUMASK_BITS) {
-        error_setg(errp, "Trying to use more CPUs than max of %d",
-                   MAX_CPUMASK_BITS);
-        return -1;
-    }
-
-    bitmap_set(cpu_index_map, cpu, 1);
-    return cpu;
-}
-
-static void cpu_release_index(CPUState *cpu)
-{
-    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
-}
-#else
-
-static int cpu_get_free_index(Error **errp)
+static int cpu_get_free_index(void)
 {
     CPUState *some_cpu;
     int cpu_index = 0;
@@ -632,12 +609,6 @@ static int cpu_get_free_index(Error **errp)
     return cpu_index;
 }
 
-static void cpu_release_index(CPUState *cpu)
-{
-    return;
-}
-#endif
-
 void cpu_exec_exit(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
@@ -650,8 +621,7 @@ void cpu_exec_exit(CPUState *cpu)
     }
 
     QTAILQ_REMOVE(&cpus, cpu, node);
-    cpu_release_index(cpu);
-    cpu->cpu_index = -1;
+    cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     cpu_list_unlock();
 
     if (cc->vmsd != NULL) {
@@ -665,7 +635,7 @@ void cpu_exec_exit(CPUState *cpu)
 void cpu_exec_init(CPUState *cpu, Error **errp)
 {
     CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
-    Error *local_err = NULL;
+    Error *local_err ATTRIBUTE_UNUSED = NULL;
 
     cpu->as = NULL;
     cpu->num_ases = 0;
@@ -689,11 +659,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
 #endif
 
     cpu_list_lock();
-    cpu->cpu_index = cpu_get_free_index(&local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        cpu_list_unlock();
-        return;
+    if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
+        cpu->cpu_index = cpu_get_free_index();
+        assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
     }
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
     cpu_list_unlock();
diff --git a/qom/cpu.c b/qom/cpu.c
index 42b5631..2553247 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -340,7 +340,7 @@ static void cpu_common_initfn(Object *obj)
     CPUState *cpu = CPU(obj);
     CPUClass *cc = CPU_GET_CLASS(obj);
 
-    cpu->cpu_index = -1;
+    cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
     qemu_mutex_init(&cpu->work_mutex);
     QTAILQ_INIT(&cpu->breakpoints);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/8] exec: set cpu_index only if it's been explictly set

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:34PM +0200, Igor Mammedov wrote:
> it keeps the legacy behavior for all users that doesn't care
> about stable cpu_index value, but would allow boards that
> would support device_add/device_del to set stable cpu_index
> that won't depend on order in which cpus are created/destroyed.
> 
> While at that simplify cpu_get_free_index() as cpu_index
> generated by USER_ONLY and softmmu variants is the same
> since none of the users support cpu-remove so far except
> of not yet released spapr/x86 device_add/del. which
> will be altered in follow up patches to provide stable
> cpu_index.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

It looks like the 1-line description needs a "not" in it somewhere,
but otherwise:

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

> ---
>  include/qom/cpu.h |  2 ++
>  exec.c            | 44 ++++++--------------------------------------
>  qom/cpu.c         |  2 +-
>  3 files changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index cbcd64c..ce0c406 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -883,4 +883,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
>      .offset = 0,                                                            \
>  }
>  
> +#define UNASSIGNED_CPU_INDEX -1
> +
>  #endif
> diff --git a/exec.c b/exec.c
> index 8c5da32..8e8416b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -598,30 +598,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>  }
>  #endif
>  
> -#ifndef CONFIG_USER_ONLY
> -static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> -
> -static int cpu_get_free_index(Error **errp)
> -{
> -    int cpu = find_first_zero_bit(cpu_index_map, MAX_CPUMASK_BITS);
> -
> -    if (cpu >= MAX_CPUMASK_BITS) {
> -        error_setg(errp, "Trying to use more CPUs than max of %d",
> -                   MAX_CPUMASK_BITS);
> -        return -1;
> -    }
> -
> -    bitmap_set(cpu_index_map, cpu, 1);
> -    return cpu;
> -}
> -
> -static void cpu_release_index(CPUState *cpu)
> -{
> -    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> -}
> -#else
> -
> -static int cpu_get_free_index(Error **errp)
> +static int cpu_get_free_index(void)
>  {
>      CPUState *some_cpu;
>      int cpu_index = 0;
> @@ -632,12 +609,6 @@ static int cpu_get_free_index(Error **errp)
>      return cpu_index;
>  }
>  
> -static void cpu_release_index(CPUState *cpu)
> -{
> -    return;
> -}
> -#endif
> -
>  void cpu_exec_exit(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> @@ -650,8 +621,7 @@ void cpu_exec_exit(CPUState *cpu)
>      }
>  
>      QTAILQ_REMOVE(&cpus, cpu, node);
> -    cpu_release_index(cpu);
> -    cpu->cpu_index = -1;
> +    cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>      cpu_list_unlock();
>  
>      if (cc->vmsd != NULL) {
> @@ -665,7 +635,7 @@ void cpu_exec_exit(CPUState *cpu)
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
>      CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu);
> -    Error *local_err = NULL;
> +    Error *local_err ATTRIBUTE_UNUSED = NULL;
>  
>      cpu->as = NULL;
>      cpu->num_ases = 0;
> @@ -689,11 +659,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>  #endif
>  
>      cpu_list_lock();
> -    cpu->cpu_index = cpu_get_free_index(&local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        cpu_list_unlock();
> -        return;
> +    if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
> +        cpu->cpu_index = cpu_get_free_index();
> +        assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>      cpu_list_unlock();
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 42b5631..2553247 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -340,7 +340,7 @@ static void cpu_common_initfn(Object *obj)
>      CPUState *cpu = CPU(obj);
>      CPUClass *cc = CPU_GET_CLASS(obj);
>  
> -    cpu->cpu_index = -1;
> +    cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>      qemu_mutex_init(&cpu->work_mutex);
>      QTAILQ_INIT(&cpu->breakpoints);

-- 
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] [PATCH 4/8] qdev: fix object reference leak in case device.realize() fails

[Igor Mammedov]

If device doesn't have parent assined before its realize
is called, device_set_realized() will implicitly set parent
to '/machine/unattached'.

However device_set_realized() may fail after that point at
several other points leaving not realized object dangling
in '/machine/unattached' and as result caller of

  obj = object_new()
    obj->ref == 1
  object_property_set_bool(obj,..., true, "realized",...)
    obj->ref == 2
  if (fail)
      object_unref(obj);
      obj->ref == 1

will get object leak instead of expected object destruction.

Fix it by making device_set_realized() to cleanup after itself
in case of failure.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6680089..ee4a083 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -885,6 +885,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     HotplugHandler *hotplug_ctrl;
     BusState *bus;
     Error *local_err = NULL;
+    bool unattached_parent = false;
+    static int unattached_count;
 
     if (dev->hotplugged && !dc->hotpluggable) {
         error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
@@ -893,12 +895,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 
     if (value && !dev->realized) {
         if (!obj->parent) {
-            static int unattached_count;
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
             object_property_add_child(container_get(qdev_get_machine(),
                                                     "/unattached"),
                                       name, obj, &error_abort);
+            unattached_parent = true;
             g_free(name);
         }
 
@@ -987,6 +989,10 @@ post_realize_fail:
 
 fail:
     error_propagate(errp, local_err);
+    if (unattached_parent) {
+        object_unparent(OBJECT(dev));
+        unattached_count--;
+    }
 }
 
 static bool device_get_hotpluggable(Object *obj, Error **errp)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/8] qdev: fix object reference leak in case device.realize() fails

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:35PM +0200, Igor Mammedov wrote:
> If device doesn't have parent assined before its realize
> is called, device_set_realized() will implicitly set parent
> to '/machine/unattached'.
> 
> However device_set_realized() may fail after that point at
> several other points leaving not realized object dangling
> in '/machine/unattached' and as result caller of
> 
>   obj = object_new()
>     obj->ref == 1
>   object_property_set_bool(obj,..., true, "realized",...)
>     obj->ref == 2
>   if (fail)
>       object_unref(obj);
>       obj->ref == 1
> 
> will get object leak instead of expected object destruction.
> 
> Fix it by making device_set_realized() to cleanup after itself
> in case of failure.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  hw/core/qdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6680089..ee4a083 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -885,6 +885,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      HotplugHandler *hotplug_ctrl;
>      BusState *bus;
>      Error *local_err = NULL;
> +    bool unattached_parent = false;
> +    static int unattached_count;
>  
>      if (dev->hotplugged && !dc->hotpluggable) {
>          error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> @@ -893,12 +895,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>      if (value && !dev->realized) {
>          if (!obj->parent) {
> -            static int unattached_count;
>              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>  
>              object_property_add_child(container_get(qdev_get_machine(),
>                                                      "/unattached"),
>                                        name, obj, &error_abort);
> +            unattached_parent = true;
>              g_free(name);
>          }
>  
> @@ -987,6 +989,10 @@ post_realize_fail:
>  
>  fail:
>      error_propagate(errp, local_err);
> +    if (unattached_parent) {
> +        object_unparent(OBJECT(dev));
> +        unattached_count--;
> +    }
>  }
>  
>  static bool device_get_hotpluggable(Object *obj, Error **errp)

-- 
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] [PATCH 5/8] pc: init CPUState->cpu_index with index in possible_cpus[]

[Igor Mammedov]

It will enshure that cpu_index for a given cpu stays the same
regardless of the order cpus has been created/deleted.

No compat code is needed as for initial cpus index in
possible_cpus[] matches cpu_index that's been auto-allocated
in cpu_exec_init().

Tha same applies for hotplug with cpu-add command if cpus are
added sequentially in increasing order as 'id' matches cpu_index.

If cpu-add had been used for creating out-of-order cpus,
that created unmigratable instance since it were not possible
to start target with the same cpu_index using old way
of migrating instance with hotplugged cpus:

* source QEMU with CLI (-smp 1,maxcpus=3 and cpu-add id=2)
  following set of cpu_index is allocated [0, 1] with
  apics set [0, 2] respectivelly
* target QEMU is started with CLI -smp 2,maxcpus=3
  resulting in set of cpu_index [0, 1] but with
  set of apics [0, 1] wich doesn't match source.

So we don't need compat code in this case as it's never worked
and newelly added device_add support would use stable cpu_index
set by machine to begin with, so it won't have above limitation
and source QEMU could be migrated to destination regardless
of the order cpus were created.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ac7a4d5..316fb43 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1872,6 +1872,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
+    CPUState *cs;
     CPUArchId *cpu_slot;
     X86CPUTopoInfo topo;
     X86CPU *cpu = X86_CPU(dev);
@@ -1972,6 +1973,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
     cpu->thread_id = topo.smt_id;
+
+    cs = CPU(cpu);
+    cs->cpu_index = idx;
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/8] pc: init CPUState->cpu_index with index in possible_cpus[]

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:36PM +0200, Igor Mammedov wrote:
> It will enshure that cpu_index for a given cpu stays the same
> regardless of the order cpus has been created/deleted.
> 
> No compat code is needed as for initial cpus index in
> possible_cpus[] matches cpu_index that's been auto-allocated
> in cpu_exec_init().
> 
> Tha same applies for hotplug with cpu-add command if cpus are
> added sequentially in increasing order as 'id' matches cpu_index.
> 
> If cpu-add had been used for creating out-of-order cpus,
> that created unmigratable instance since it were not possible
> to start target with the same cpu_index using old way
> of migrating instance with hotplugged cpus:
> 
> * source QEMU with CLI (-smp 1,maxcpus=3 and cpu-add id=2)
>   following set of cpu_index is allocated [0, 1] with
>   apics set [0, 2] respectivelly
> * target QEMU is started with CLI -smp 2,maxcpus=3
>   resulting in set of cpu_index [0, 1] but with
>   set of apics [0, 1] wich doesn't match source.
> 
> So we don't need compat code in this case as it's never worked
> and newelly added device_add support would use stable cpu_index
> set by machine to begin with, so it won't have above limitation
> and source QEMU could be migrated to destination regardless
> of the order cpus were created.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

> ---
>  hw/i386/pc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ac7a4d5..316fb43 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1872,6 +1872,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> +    CPUState *cs;
>      CPUArchId *cpu_slot;
>      X86CPUTopoInfo topo;
>      X86CPU *cpu = X86_CPU(dev);
> @@ -1972,6 +1973,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>          return;
>      }
>      cpu->thread_id = topo.smt_id;
> +
> +    cs = CPU(cpu);
> +    cs->cpu_index = idx;
>  }
>  
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,

-- 
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] [PATCH 6/8] spapr: init CPUState->cpu_index with index relative to core-id

[Igor Mammedov]

It will enshure that cpu_index for a given cpu stays the same
regardless of the order cpus has been created/deleted and so
it would be possible to migrate QEMU instance with out of order
created CPU.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/ppc/spapr_cpu_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 4bfc96b..f68e88d 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -309,9 +309,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     sc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         char id[32];
+        CPUState *cs;
+
         obj = sc->threads + i * size;
 
         object_initialize(obj, size, typename);
+        cs = CPU(obj);
+        cs->cpu_index = cc->core_id + i;
         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] [PATCH 6/8] spapr: init CPUState->cpu_index with index relative to core-id

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:37PM +0200, Igor Mammedov wrote:
> It will enshure that cpu_index for a given cpu stays the same
> regardless of the order cpus has been created/deleted and so
> it would be possible to migrate QEMU instance with out of order
> created CPU.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

So, this isn't quite right (it wasn't right in my version either).

The problem occurs when smp_threads < kvmppc_smt_threads().  That is,
when the requested threads-per-core is less than the hardware's
maximum number of threads-per-core.

The core-id values are assigned essentially as i *
kvmppc_smt_threads(), meaning the patch below will leave gaps in the
cpu_index values and the last ones will exceed max_cpus, causing other
problems.

What I'm not sure about is whether the right way to fix this is to
change the core-id values, or to calculate the cpu_index from the
existing core-id values.

> ---
>  hw/ppc/spapr_cpu_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4bfc96b..f68e88d 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -309,9 +309,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
> +        CPUState *cs;
> +
>          obj = sc->threads + i * size;
>  
>          object_initialize(obj, size, typename);
> +        cs = CPU(obj);
> +        cs->cpu_index = cc->core_id + i;
>          snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {

-- 
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] [PATCH 6/8] spapr: init CPUState->cpu_index with index relative to core-id

[Bharata B Rao]

On Fri, Jul 22, 2016 at 01:23:01PM +1000, David Gibson wrote:
> On Thu, Jul 21, 2016 at 05:54:37PM +0200, Igor Mammedov wrote:
> > It will enshure that cpu_index for a given cpu stays the same
> > regardless of the order cpus has been created/deleted and so
> > it would be possible to migrate QEMU instance with out of order
> > created CPU.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> So, this isn't quite right (it wasn't right in my version either).
> 
> The problem occurs when smp_threads < kvmppc_smt_threads().  That is,
> when the requested threads-per-core is less than the hardware's
> maximum number of threads-per-core.
> 
> The core-id values are assigned essentially as i *
> kvmppc_smt_threads(), meaning the patch below will leave gaps in the
> cpu_index values and the last ones will exceed max_cpus, causing other
> problems.

This would lead to hotplug failures as cpu_dt_id is still being
derived from non-contiguous cpu_index resulting in wrong enumeration
of CPU nodes in DT.

For -smp 8,threads=4 we see the following CPU nodes in DT

PowerPC,POWER8@0 PowerPC,POWER8@10

which otherwise should have been

PowerPC,POWER8@0 PowerPC,POWER8@8

The problem manifests as drmgr failure.

Greg's patchset that moved cpu_dt_id setting to machine code and that
derived cpu_dt_id from core-id for sPAPR would be needed to fix this I guess.

Regards,
Bharata.

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

[David Gibson]

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

On Fri, Jul 22, 2016 at 11:40:03AM +0530, Bharata B Rao wrote:
> On Fri, Jul 22, 2016 at 01:23:01PM +1000, David Gibson wrote:
> > On Thu, Jul 21, 2016 at 05:54:37PM +0200, Igor Mammedov wrote:
> > > It will enshure that cpu_index for a given cpu stays the same
> > > regardless of the order cpus has been created/deleted and so
> > > it would be possible to migrate QEMU instance with out of order
> > > created CPU.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > So, this isn't quite right (it wasn't right in my version either).
> > 
> > The problem occurs when smp_threads < kvmppc_smt_threads().  That is,
> > when the requested threads-per-core is less than the hardware's
> > maximum number of threads-per-core.
> > 
> > The core-id values are assigned essentially as i *
> > kvmppc_smt_threads(), meaning the patch below will leave gaps in the
> > cpu_index values and the last ones will exceed max_cpus, causing other
> > problems.
> 
> This would lead to hotplug failures as cpu_dt_id is still being
> derived from non-contiguous cpu_index resulting in wrong enumeration
> of CPU nodes in DT.

Which "This" are you referring to?

> 
> For -smp 8,threads=4 we see the following CPU nodes in DT
> 
> PowerPC,POWER8@0 PowerPC,POWER8@10
> 
> which otherwise should have been
> 
> PowerPC,POWER8@0 PowerPC,POWER8@8
> 
> The problem manifests as drmgr failure.
> 
> Greg's patchset that moved cpu_dt_id setting to machine code and that
> derived cpu_dt_id from core-id for sPAPR would be needed to fix this
> I guess.

No, it shouldn't be necessary to fix it.  But we certainly do want to
clean this stuff up.  I'm not terribly convinced by the current
approach in Greg's series though.  I'd actually prefer to remove
cpu_dt_id from the cpustate entirely and instead work it out from the
(now stable) cpu index when we go to construct the device tree.

-- 
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] [PATCH 6/8] spapr: init CPUState->cpu_index with index relative to core-id

[Bharata B Rao]

On Fri, Jul 22, 2016 at 05:14:33PM +1000, David Gibson wrote:
> On Fri, Jul 22, 2016 at 11:40:03AM +0530, Bharata B Rao wrote:
> > On Fri, Jul 22, 2016 at 01:23:01PM +1000, David Gibson wrote:
> > > On Thu, Jul 21, 2016 at 05:54:37PM +0200, Igor Mammedov wrote:
> > > > It will enshure that cpu_index for a given cpu stays the same
> > > > regardless of the order cpus has been created/deleted and so
> > > > it would be possible to migrate QEMU instance with out of order
> > > > created CPU.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > So, this isn't quite right (it wasn't right in my version either).
> > > 
> > > The problem occurs when smp_threads < kvmppc_smt_threads().  That is,
> > > when the requested threads-per-core is less than the hardware's
> > > maximum number of threads-per-core.
> > > 
> > > The core-id values are assigned essentially as i *
> > > kvmppc_smt_threads(), meaning the patch below will leave gaps in the
> > > cpu_index values and the last ones will exceed max_cpus, causing other
> > > problems.
> > 
> > This would lead to hotplug failures as cpu_dt_id is still being
> > derived from non-contiguous cpu_index resulting in wrong enumeration
> > of CPU nodes in DT.
> 
> Which "This" are you referring to?

:) Gaps in cpu_index values due to which cpu_dt_id gets calculated wrongly.

Regards,
Bharata.

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

[Greg Kurz]

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

On Fri, 22 Jul 2016 17:14:33 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

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

That was the initial plan for my series :)

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

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

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

index_of_core_in_spapr_cores * smp_threads + index_of_thread_in_core_threads

Cheers.

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[David Gibson]

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

On Thu, Jul 21, 2016 at 05:54:37PM +0200, Igor Mammedov wrote:
> It will enshure that cpu_index for a given cpu stays the same
> regardless of the order cpus has been created/deleted and so
> it would be possible to migrate QEMU instance with out of order
> created CPU.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Now that I've merged Greg's patch fix the core ids so this is no
longer a problem, I've merged this patch into ppc-for-2.7 on top of
your more-recent version of this series.

> ---
>  hw/ppc/spapr_cpu_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4bfc96b..f68e88d 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -309,9 +309,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
> +        CPUState *cs;
> +
>          obj = sc->threads + i * size;
>  
>          object_initialize(obj, size, typename);
> +        cs = CPU(obj);
> +        cs->cpu_index = cc->core_id + i;
>          snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {

-- 
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] [PATCH 7/8] Revert "pc: Enforce adding CPUs contiguously and removing them in opposite order"

[Igor Mammedov]

This reverts commit 4da7faaeb0c7dd3f7f233165d336c878f78fd1eb.

However since commit:
  pc: init CPUState->cpu_index with index in possible_cpus[]
cpu_index is stable regardless of the order cpus were created
and QEMU instance stays migratable always so limitation added
by 4da7faaeb could be safely removed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 316fb43..d0e392a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1815,23 +1815,6 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    if (idx < pcms->possible_cpus->len - 1 &&
-        pcms->possible_cpus->cpus[idx + 1].cpu != NULL) {
-        X86CPU *cpu;
-
-        for (idx = pcms->possible_cpus->len - 1;
-             pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) {
-            ;;
-        }
-
-        cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu);
-        error_setg(&local_err, "CPU [socket-id: %u, core-id: %u,"
-                   " thread-id: %u] should be removed first",
-                   cpu->socket_id, cpu->core_id, cpu->thread_id);
-        goto out;
-
-    }
-
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
@@ -1929,23 +1912,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
 
-    if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) {
-        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-
-        for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) {
-            ;;
-        }
-
-        x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id,
-                                 smp_cores, smp_threads, &topo);
-
-        if (!pcmc->legacy_cpu_hotplug) {
-            error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be"
-                       " added first", topo.pkg_id, topo.core_id, topo.smt_id);
-            return;
-        }
-    }
-
     /* if 'address' properties socket-id/core-id/thread-id are not set, set them
      * so that query_hotpluggable_cpus would show correct values
      */
-- 
2.7.4

[Qemu-devel] [PATCH 8/8] Revert "spapr: Ensure CPU cores are added contiguously and removed in LIFO order"

[Igor Mammedov]

From: David Gibson <david@gibson.dropbear.id.au>

This reverts commit 5cbc64de25973e9129c5a7897734a06ac64b9aff.

We've now removed the limitations which made out-of-order cpu hotplug
cause problems.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index f68e88d..dc5deb7 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -126,23 +126,12 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
 void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                        Error **errp)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     CPUCore *cc = CPU_CORE(dev);
     sPAPRDRConnector *drc =
         spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
     sPAPRDRConnectorClass *drck;
     Error *local_err = NULL;
-    int smt = kvmppc_smt_threads();
-    int index = cc->core_id / smt;
-    int spapr_max_cores = max_cpus / smp_threads;
-    int i;
 
-    for (i = spapr_max_cores - 1; i > index; i--) {
-        if (spapr->cores[i]) {
-            error_setg(errp, "core-id %d should be removed first", i * smt);
-            return;
-        }
-    }
     g_assert(drc);
 
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -225,7 +214,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev));
     sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     int spapr_max_cores = max_cpus / smp_threads;
-    int index, i;
+    int index;
     int smt = kvmppc_smt_threads();
     Error *local_err = NULL;
     CPUCore *cc = CPU_CORE(dev);
@@ -263,14 +252,6 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
-    for (i = 0; i < index; i++) {
-        if (!spapr->cores[i]) {
-            error_setg(&local_err, "core-id %d should be added first",
-                       i * smt);
-            goto out;
-        }
-    }
-
 out:
     g_free(base_core_type);
     error_propagate(errp, local_err);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/8] Fix migration issues with arbitrary cpu-hot(un)plug

[Michael S. Tsirkin]

On Thu, Jul 21, 2016 at 05:54:31PM +0200, Igor Mammedov wrote:
> Series fixes migration issues caused by unstable cpu_index which depended
> on order cpus were created/destroyed. It follows David's idea to make
> cpu_index assignable by selected boards if board supports cpu-hotplug
> with device_add and needs stable cpu_index/'migration id' but leaves
> behaviour of the same as before for users that don't care about
> cpu-hot(un)plug making changes low-risk.
> 
> tested with:
>   SRC -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
>        -device qemu64-x86_64-cpu,id=cpudel,apic-id=1 \
>        -device qemu64-x86_64-cpu,apic-id=2 
>   (qemu) device_del cpudel
>   (qemu) stop
>   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
>   
>   DST -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
>       -device qemu64-x86_64-cpu,apic-id=2 \
>       -incoming "exec: gzip -c -d STATEFILE.gz"
> 
> git tree to test with:
>      https://github.com/imammedo/qemu cpu-index-stable
>  to view
>      https://github.com/imammedo/qemu/commits/cpu-index-stable

For PC bits:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

This would be nice to have in 2.7.

Who's reviewing/merging the rest? Eduardo?


> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Alexander Graf <agraf@suse.de>
> CC: Riku Voipio <riku.voipio@iki.fi>
> CC: Bharata B Rao <bharata@linux.vnet.ibm.com>
> CC: qemu-ppc@nongnu.org
> 
> David Gibson (1):
>   Revert "spapr: Ensure CPU cores are added contiguously and removed in
>     LIFO order"
> 
> Igor Mammedov (7):
>   exec: reduce CONFIG_USER_ONLY ifdeffenery
>   exec: don't use cpu_index to detect if cpu_exec_init()'s been called
>     for cpu
>   exec: set cpu_index only if it's been explictly set
>   qdev: fix object reference leak in case device.realize() fails
>   pc: init CPUState->cpu_index with index in possible_cpus[]
>   spapr: init CPUState->cpu_index with index relative to core-id
>   Revert "pc: Enforce adding CPUs contiguously and removing them in
>     opposite order"
> 
>  bsd-user/qemu.h         |  2 --
>  include/exec/exec-all.h | 12 +++++++++
>  include/qemu/queue.h    |  2 ++
>  include/qom/cpu.h       |  2 ++
>  linux-user/qemu.h       |  2 --
>  exec.c                  | 65 +++++++++----------------------------------------
>  hw/core/qdev.c          |  8 +++++-
>  hw/i386/pc.c            | 38 +++--------------------------
>  hw/ppc/spapr_cpu_core.c | 25 ++++---------------
>  qom/cpu.c               |  2 +-
>  10 files changed, 44 insertions(+), 114 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 0/8] Fix migration issues with arbitrary cpu-hot(un)plug

[David Gibson]

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

On Fri, Jul 22, 2016 at 12:56:26AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 21, 2016 at 05:54:31PM +0200, Igor Mammedov wrote:
> > Series fixes migration issues caused by unstable cpu_index which depended
> > on order cpus were created/destroyed. It follows David's idea to make
> > cpu_index assignable by selected boards if board supports cpu-hotplug
> > with device_add and needs stable cpu_index/'migration id' but leaves
> > behaviour of the same as before for users that don't care about
> > cpu-hot(un)plug making changes low-risk.
> > 
> > tested with:
> >   SRC -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
> >        -device qemu64-x86_64-cpu,id=cpudel,apic-id=1 \
> >        -device qemu64-x86_64-cpu,apic-id=2 
> >   (qemu) device_del cpudel
> >   (qemu) stop
> >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> >   
> >   DST -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
> >       -device qemu64-x86_64-cpu,apic-id=2 \
> >       -incoming "exec: gzip -c -d STATEFILE.gz"
> > 
> > git tree to test with:
> >      https://github.com/imammedo/qemu cpu-index-stable
> >  to view
> >      https://github.com/imammedo/qemu/commits/cpu-index-stable
> 
> For PC bits:
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> This would be nice to have in 2.7.

I agree.  Despite the lateness, I think this will avoid substantial
future pain.

> Who's reviewing/merging the rest? Eduardo?

I've reviewed.  I could merge through my tree if we don't have a
better option, but merging pc specific pieces through the ppc tree
would seem odd.

-- 
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] [PATCH 0/8] Fix migration issues with arbitrary cpu-hot(un)plug

[Igor Mammedov]

On Fri, 22 Jul 2016 14:35:05 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 22, 2016 at 12:56:26AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 21, 2016 at 05:54:31PM +0200, Igor Mammedov wrote:  
> > > Series fixes migration issues caused by unstable cpu_index which depended
> > > on order cpus were created/destroyed. It follows David's idea to make
> > > cpu_index assignable by selected boards if board supports cpu-hotplug
> > > with device_add and needs stable cpu_index/'migration id' but leaves
> > > behaviour of the same as before for users that don't care about
> > > cpu-hot(un)plug making changes low-risk.
> > > 
> > > tested with:
> > >   SRC -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
> > >        -device qemu64-x86_64-cpu,id=cpudel,apic-id=1 \
> > >        -device qemu64-x86_64-cpu,apic-id=2 
> > >   (qemu) device_del cpudel
> > >   (qemu) stop
> > >   (qemu) migrate "exec:gzip -c > STATEFILE.gz"
> > >   
> > >   DST -snapshot -enable-kvm -smp 1,maxcpus=3 -m 256M guest.img -monitor stdio \
> > >       -device qemu64-x86_64-cpu,apic-id=2 \
> > >       -incoming "exec: gzip -c -d STATEFILE.gz"
> > > 
> > > git tree to test with:
> > >      https://github.com/imammedo/qemu cpu-index-stable
> > >  to view
> > >      https://github.com/imammedo/qemu/commits/cpu-index-stable  
> > 
> > For PC bits:
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > This would be nice to have in 2.7.  
> 
> I agree.  Despite the lateness, I think this will avoid substantial
> future pain.
> 
> > Who's reviewing/merging the rest? Eduardo?  
> 
> I've reviewed.  I could merge through my tree if we don't have a
> better option, but merging pc specific pieces through the ppc tree
> would seem odd.
Eduardo,
if you take it through your tree could you drop spapr patches for now,
it looks like PPC side are going to redefine core-id semantics
so they'll post patches on top of this series.