| Message ID | 1467903025-13383-3-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 7 Jul 2016 20:20:22 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > Add CPUState::stable_cpu_id and use that as instance_id in > vmstate_register() call. > > Introduce has-stable_cpu_id property that allows target machines to > optionally switch to using stable_cpu_id instead of cpu_index. If stable_cpu_id is in the [0..max_vcpus) range, then it does not really depend on the machine type or version. In this case, the property is not needed. > This will help allow successful migration in cases where holes are > introduced in cpu_index range after CPU hot removals. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > exec.c | 6 ++++-- > include/qom/cpu.h | 5 +++++ > qom/cpu.c | 6 ++++++ > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/exec.c b/exec.c > index fb73910..3b36fe5 100644 > --- a/exec.c > +++ b/exec.c > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > void cpu_vmstate_register(CPUState *cpu) > { > CPUClass *cc = CPU_GET_CLASS(cpu); > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > + 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/qom/cpu.h b/include/qom/cpu.h > index 331386f..527c021 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. > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > + * over cpu_index during vmstate registration. > * > * State of one CPU core or thread. > */ > @@ -360,6 +363,8 @@ 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; > + int stable_cpu_id; > + bool has_stable_cpu_id; > }; > > QTAILQ_HEAD(CPUTailQ, CPUState); > diff --git a/qom/cpu.c b/qom/cpu.c > index 1095ea1..bae1bf7 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > return cpu->cpu_index; > } > > +static Property cpu_common_properties[] = { > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > + DEFINE_PROP_END_OF_LIST() > +}; > + > static void cpu_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > * IRQs, adding reset handlers, halting non-first CPUs, ... > */ > dc->cannot_instantiate_with_device_add_yet = true; > + dc->props = cpu_common_properties; > } > > static const TypeInfo cpu_type_info = {
On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > Add CPUState::stable_cpu_id and use that as instance_id in > vmstate_register() call. > > Introduce has-stable_cpu_id property that allows target machines to > optionally switch to using stable_cpu_id instead of cpu_index. > This will help allow successful migration in cases where holes are > introduced in cpu_index range after CPU hot removals. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > exec.c | 6 ++++-- > include/qom/cpu.h | 5 +++++ > qom/cpu.c | 6 ++++++ > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/exec.c b/exec.c > index fb73910..3b36fe5 100644 > --- a/exec.c > +++ b/exec.c > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > void cpu_vmstate_register(CPUState *cpu) > { > CPUClass *cc = CPU_GET_CLASS(cpu); > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > + 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/qom/cpu.h b/include/qom/cpu.h > index 331386f..527c021 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. > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > + * over cpu_index during vmstate registration. > * > * State of one CPU core or thread. > */ > @@ -360,6 +363,8 @@ 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; > + int stable_cpu_id; > + bool has_stable_cpu_id; > }; > > QTAILQ_HEAD(CPUTailQ, CPUState); > diff --git a/qom/cpu.c b/qom/cpu.c > index 1095ea1..bae1bf7 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > return cpu->cpu_index; > } > > +static Property cpu_common_properties[] = { > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), It seems odd to me that stable_cpu_id itself isn't exposed as a property. Even if we don't need to set it externally for now, it really should be QOM introspectable. > + DEFINE_PROP_END_OF_LIST() > +}; > + > static void cpu_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > * IRQs, adding reset handlers, halting non-first CPUs, ... > */ > dc->cannot_instantiate_with_device_add_yet = true; > + dc->props = cpu_common_properties; > } > > static const TypeInfo cpu_type_info = {
On Thu, Jul 07, 2016 at 07:52:32PM +0200, Greg Kurz wrote: > On Thu, 7 Jul 2016 20:20:22 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > Add CPUState::stable_cpu_id and use that as instance_id in > > vmstate_register() call. > > > > Introduce has-stable_cpu_id property that allows target machines to > > optionally switch to using stable_cpu_id instead of cpu_index. > > If stable_cpu_id is in the [0..max_vcpus) range, then it does not really depend > on the machine type or version. No.. it really does. The ids need to exactly match, not just be in the same range in order to maintain migration compatibility with pre-stable_cpu_id versions of qemu. > In this case, the property is not needed. > > > This will help allow successful migration in cases where holes are > > introduced in cpu_index range after CPU hot removals. > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > exec.c | 6 ++++-- > > include/qom/cpu.h | 5 +++++ > > qom/cpu.c | 6 ++++++ > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index fb73910..3b36fe5 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > void cpu_vmstate_register(CPUState *cpu) > > { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > + 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/qom/cpu.h b/include/qom/cpu.h > > index 331386f..527c021 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. > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > + * over cpu_index during vmstate registration. > > * > > * State of one CPU core or thread. > > */ > > @@ -360,6 +363,8 @@ 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; > > + int stable_cpu_id; > > + bool has_stable_cpu_id; > > }; > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > diff --git a/qom/cpu.c b/qom/cpu.c > > index 1095ea1..bae1bf7 100644 > > --- a/qom/cpu.c > > +++ b/qom/cpu.c > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > return cpu->cpu_index; > > } > > > > +static Property cpu_common_properties[] = { > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > + DEFINE_PROP_END_OF_LIST() > > +}; > > + > > static void cpu_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > * IRQs, adding reset handlers, halting non-first CPUs, ... > > */ > > dc->cannot_instantiate_with_device_add_yet = true; > > + dc->props = cpu_common_properties; > > } > > > > static const TypeInfo cpu_type_info = { >
On Fri, 8 Jul 2016 15:19:58 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > > Add CPUState::stable_cpu_id and use that as instance_id in > > vmstate_register() call. > > > > Introduce has-stable_cpu_id property that allows target machines to > > optionally switch to using stable_cpu_id instead of cpu_index. > > This will help allow successful migration in cases where holes are > > introduced in cpu_index range after CPU hot removals. > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > exec.c | 6 ++++-- > > include/qom/cpu.h | 5 +++++ > > qom/cpu.c | 6 ++++++ > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index fb73910..3b36fe5 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > void cpu_vmstate_register(CPUState *cpu) > > { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > + 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/qom/cpu.h b/include/qom/cpu.h > > index 331386f..527c021 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. > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > + * over cpu_index during vmstate registration. > > * > > * State of one CPU core or thread. > > */ > > @@ -360,6 +363,8 @@ 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; > > + int stable_cpu_id; > > + bool has_stable_cpu_id; > > }; > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > diff --git a/qom/cpu.c b/qom/cpu.c > > index 1095ea1..bae1bf7 100644 > > --- a/qom/cpu.c > > +++ b/qom/cpu.c > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > return cpu->cpu_index; > > } > > > > +static Property cpu_common_properties[] = { > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > It seems odd to me that stable_cpu_id itself isn't exposed as a > property. Even if we don't need to set it externally for now, it > really should be QOM introspectable. Should it? Why? It's QEMU internal detail and outside world preferably shouldn't know anything about it. As example look at cpu_index which were/is used in HMP and -numa interfaces and now mgmt tries make some sense of it. Cleaner way should be teaching -numa to handle cpus specified by socket/core/thread IDs so that mgmt would actually know what CPUs it assigns to what nodes and not to look at/invent/generate some internal cpu_id. > > > + DEFINE_PROP_END_OF_LIST() > > +}; > > + > > static void cpu_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > * IRQs, adding reset handlers, halting non-first CPUs, ... > > */ > > dc->cannot_instantiate_with_device_add_yet = true; > > + dc->props = cpu_common_properties; > > } > > > > static const TypeInfo cpu_type_info = { >
On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote: > On Fri, 8 Jul 2016 15:19:58 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > > > Add CPUState::stable_cpu_id and use that as instance_id in > > > vmstate_register() call. > > > > > > Introduce has-stable_cpu_id property that allows target machines to > > > optionally switch to using stable_cpu_id instead of cpu_index. > > > This will help allow successful migration in cases where holes are > > > introduced in cpu_index range after CPU hot removals. > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > --- > > > exec.c | 6 ++++-- > > > include/qom/cpu.h | 5 +++++ > > > qom/cpu.c | 6 ++++++ > > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/exec.c b/exec.c > > > index fb73910..3b36fe5 100644 > > > --- a/exec.c > > > +++ b/exec.c > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > > void cpu_vmstate_register(CPUState *cpu) > > > { > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > > + 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/qom/cpu.h b/include/qom/cpu.h > > > index 331386f..527c021 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. > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > > + * over cpu_index during vmstate registration. > > > * > > > * State of one CPU core or thread. > > > */ > > > @@ -360,6 +363,8 @@ 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; > > > + int stable_cpu_id; > > > + bool has_stable_cpu_id; > > > }; > > > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > index 1095ea1..bae1bf7 100644 > > > --- a/qom/cpu.c > > > +++ b/qom/cpu.c > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > > return cpu->cpu_index; > > > } > > > > > > +static Property cpu_common_properties[] = { > > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > > > It seems odd to me that stable_cpu_id itself isn't exposed as a > > property. Even if we don't need to set it externally for now, it > > really should be QOM introspectable. > Should it? Why? Well, for one thing it's really strange to have the boolean flag exposed, but not the value itself. > It's QEMU internal detail and outside world preferably shouldn't > know anything about it. Hrm.. I guess kinda. But I think it's less an internal detail than the existing cpu_index is. > As example look at cpu_index which were/is used in HMP and -numa > interfaces and now mgmt tries make some sense of it. > > Cleaner way should be teaching -numa to handle cpus specified by > socket/core/thread IDs so that mgmt would actually know what CPUs > it assigns to what nodes and not to look at/invent/generate some > internal cpu_id. > > > > > > + DEFINE_PROP_END_OF_LIST() > > > +}; > > > + > > > static void cpu_class_init(ObjectClass *klass, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > > * IRQs, adding reset handlers, halting non-first CPUs, ... > > > */ > > > dc->cannot_instantiate_with_device_add_yet = true; > > > + dc->props = cpu_common_properties; > > > } > > > > > > static const TypeInfo cpu_type_info = { > > >
On Mon, Jul 11, 2016 at 01:22:37PM +1000, David Gibson wrote: > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote: > > On Fri, 8 Jul 2016 15:19:58 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > > > > Add CPUState::stable_cpu_id and use that as instance_id in > > > > vmstate_register() call. > > > > > > > > Introduce has-stable_cpu_id property that allows target machines to > > > > optionally switch to using stable_cpu_id instead of cpu_index. > > > > This will help allow successful migration in cases where holes are > > > > introduced in cpu_index range after CPU hot removals. > > > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > --- > > > > exec.c | 6 ++++-- > > > > include/qom/cpu.h | 5 +++++ > > > > qom/cpu.c | 6 ++++++ > > > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/exec.c b/exec.c > > > > index fb73910..3b36fe5 100644 > > > > --- a/exec.c > > > > +++ b/exec.c > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > > > void cpu_vmstate_register(CPUState *cpu) > > > > { > > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > > > + 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/qom/cpu.h b/include/qom/cpu.h > > > > index 331386f..527c021 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. > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > > > + * over cpu_index during vmstate registration. > > > > * > > > > * State of one CPU core or thread. > > > > */ > > > > @@ -360,6 +363,8 @@ 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; > > > > + int stable_cpu_id; > > > > + bool has_stable_cpu_id; > > > > }; > > > > > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > > index 1095ea1..bae1bf7 100644 > > > > --- a/qom/cpu.c > > > > +++ b/qom/cpu.c > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > > > return cpu->cpu_index; > > > > } > > > > > > > > +static Property cpu_common_properties[] = { > > > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > > > > > It seems odd to me that stable_cpu_id itself isn't exposed as a > > > property. Even if we don't need to set it externally for now, it > > > really should be QOM introspectable. > > Should it? Why? > > Well, for one thing it's really strange to have the boolean flag > exposed, but not the value itself. How about just the property which starts with a deafult value (-1 ?) based on which we can either use stable_cpu_id or cpu_index with vmstate_register() calls ? Machine types that want to use stable_cpu_id can explicitly set this property to a valid "non -1" value ? I remember you were suggesting something like this earlier. Regards, Bharata.
On Mon, 11 Jul 2016 09:05:07 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Mon, Jul 11, 2016 at 01:22:37PM +1000, David Gibson wrote: > > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote: > > > On Fri, 8 Jul 2016 15:19:58 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > > > > > Add CPUState::stable_cpu_id and use that as instance_id in > > > > > vmstate_register() call. > > > > > > > > > > Introduce has-stable_cpu_id property that allows target machines to > > > > > optionally switch to using stable_cpu_id instead of cpu_index. > > > > > This will help allow successful migration in cases where holes are > > > > > introduced in cpu_index range after CPU hot removals. > > > > > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > --- > > > > > exec.c | 6 ++++-- > > > > > include/qom/cpu.h | 5 +++++ > > > > > qom/cpu.c | 6 ++++++ > > > > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/exec.c b/exec.c > > > > > index fb73910..3b36fe5 100644 > > > > > --- a/exec.c > > > > > +++ b/exec.c > > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > > > > void cpu_vmstate_register(CPUState *cpu) > > > > > { > > > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > > > > + 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/qom/cpu.h b/include/qom/cpu.h > > > > > index 331386f..527c021 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. > > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > > > > + * over cpu_index during vmstate registration. > > > > > * > > > > > * State of one CPU core or thread. > > > > > */ > > > > > @@ -360,6 +363,8 @@ 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; > > > > > + int stable_cpu_id; > > > > > + bool has_stable_cpu_id; > > > > > }; > > > > > > > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > > > index 1095ea1..bae1bf7 100644 > > > > > --- a/qom/cpu.c > > > > > +++ b/qom/cpu.c > > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > > > > return cpu->cpu_index; > > > > > } > > > > > > > > > > +static Property cpu_common_properties[] = { > > > > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > > > > > > > It seems odd to me that stable_cpu_id itself isn't exposed as a > > > > property. Even if we don't need to set it externally for now, it > > > > really should be QOM introspectable. > > > Should it? Why? > > > > Well, for one thing it's really strange to have the boolean flag > > exposed, but not the value itself. > > How about just the property which starts with a deafult value (-1 ?) > based on which we can either use stable_cpu_id or cpu_index with > vmstate_register() calls ? Machine types that want to use stable_cpu_id > can explicitly set this property to a valid "non -1" value ? the main purpose of CPU::has-stable-cpu-id is to provide means to reuse current compat logic for devices and keep it localized in CPU device. It could be done the other way around i.e. - use -1 to detect not set stable-cpu-id - add has-stable-cpu-id field to machine or arch specific subclass (you see it doesn't go away, it just spreads compat logic to machine) - set has-stable-cpu-id in machine class init depending on version (on pc it would be pc_i440fx_X_X_machine_options) - do in machine code: if (has-stable-cpu-id) { assign stable-cpu-id } I think the original device compat way is a bit more cleaner than above (not to mention that it follows typical pattern) > I remember you were suggesting something like this earlier. > > Regards, > Bharata. > >
On Mon, 11 Jul 2016 13:22:37 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote: > > On Fri, 8 Jul 2016 15:19:58 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > > > > Add CPUState::stable_cpu_id and use that as instance_id in > > > > vmstate_register() call. > > > > > > > > Introduce has-stable_cpu_id property that allows target machines to > > > > optionally switch to using stable_cpu_id instead of cpu_index. > > > > This will help allow successful migration in cases where holes are > > > > introduced in cpu_index range after CPU hot removals. > > > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > --- > > > > exec.c | 6 ++++-- > > > > include/qom/cpu.h | 5 +++++ > > > > qom/cpu.c | 6 ++++++ > > > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/exec.c b/exec.c > > > > index fb73910..3b36fe5 100644 > > > > --- a/exec.c > > > > +++ b/exec.c > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > > > void cpu_vmstate_register(CPUState *cpu) > > > > { > > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > > > + 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/qom/cpu.h b/include/qom/cpu.h > > > > index 331386f..527c021 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. > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > > > + * over cpu_index during vmstate registration. > > > > * > > > > * State of one CPU core or thread. > > > > */ > > > > @@ -360,6 +363,8 @@ 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; > > > > + int stable_cpu_id; > > > > + bool has_stable_cpu_id; > > > > }; > > > > > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > > index 1095ea1..bae1bf7 100644 > > > > --- a/qom/cpu.c > > > > +++ b/qom/cpu.c > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > > > return cpu->cpu_index; > > > > } > > > > > > > > +static Property cpu_common_properties[] = { > > > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > > > > > It seems odd to me that stable_cpu_id itself isn't exposed as a > > > property. Even if we don't need to set it externally for now, it > > > really should be QOM introspectable. > > Should it? Why? > > Well, for one thing it's really strange to have the boolean flag > exposed, but not the value itself. property doesn't always means that it's intended as an external interface > > > It's QEMU internal detail and outside world preferably shouldn't > > know anything about it. > > Hrm.. I guess kinda. But I think it's less an internal detail than > the existing cpu_index is. so it' better not to start to advertise it as an external interface. Should be add some flag to generic property to mark it as internal? > > > As example look at cpu_index which were/is used in HMP and -numa > > interfaces and now mgmt tries make some sense of it. > > > > Cleaner way should be teaching -numa to handle cpus specified by > > socket/core/thread IDs so that mgmt would actually know what CPUs > > it assigns to what nodes and not to look at/invent/generate some > > internal cpu_id. > > > > > > > > > + DEFINE_PROP_END_OF_LIST() > > > > +}; > > > > + > > > > static void cpu_class_init(ObjectClass *klass, void *data) > > > > { > > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > > @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > > > * IRQs, adding reset handlers, halting non-first CPUs, ... > > > > */ > > > > dc->cannot_instantiate_with_device_add_yet = true; > > > > + dc->props = cpu_common_properties; > > > > } > > > > > > > > static const TypeInfo cpu_type_info = { > > > > > >
On Mon, Jul 11, 2016 at 09:58:21AM +0200, Igor Mammedov wrote: > On Mon, 11 Jul 2016 13:22:37 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote: > > > On Fri, 8 Jul 2016 15:19:58 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: > > > > > Add CPUState::stable_cpu_id and use that as instance_id in > > > > > vmstate_register() call. > > > > > > > > > > Introduce has-stable_cpu_id property that allows target machines to > > > > > optionally switch to using stable_cpu_id instead of cpu_index. > > > > > This will help allow successful migration in cases where holes are > > > > > introduced in cpu_index range after CPU hot removals. > > > > > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > --- > > > > > exec.c | 6 ++++-- > > > > > include/qom/cpu.h | 5 +++++ > > > > > qom/cpu.c | 6 ++++++ > > > > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/exec.c b/exec.c > > > > > index fb73910..3b36fe5 100644 > > > > > --- a/exec.c > > > > > +++ b/exec.c > > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > > > > void cpu_vmstate_register(CPUState *cpu) > > > > > { > > > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > > > + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : > > > > > + 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/qom/cpu.h b/include/qom/cpu.h > > > > > index 331386f..527c021 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. > > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls > > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > > > > + * over cpu_index during vmstate registration. > > > > > * > > > > > * State of one CPU core or thread. > > > > > */ > > > > > @@ -360,6 +363,8 @@ 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; > > > > > + int stable_cpu_id; > > > > > + bool has_stable_cpu_id; > > > > > }; > > > > > > > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > > > index 1095ea1..bae1bf7 100644 > > > > > --- a/qom/cpu.c > > > > > +++ b/qom/cpu.c > > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > > > > return cpu->cpu_index; > > > > > } > > > > > > > > > > +static Property cpu_common_properties[] = { > > > > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), > > > > > > > > It seems odd to me that stable_cpu_id itself isn't exposed as a > > > > property. Even if we don't need to set it externally for now, it > > > > really should be QOM introspectable. > > > Should it? Why? > > > > Well, for one thing it's really strange to have the boolean flag > > exposed, but not the value itself. > property doesn't always means that it's intended as an external interface > > > > > > It's QEMU internal detail and outside world preferably shouldn't > > > know anything about it. > > > > Hrm.. I guess kinda. But I think it's less an internal detail than > > the existing cpu_index is. > so it' better not to start to advertise it as an external interface. Right. My comments were based on the assumption that this was intended as some sort of generally useful stable CPU id, rather than something narrowly focussed on migration. Having understood things better from our IRC discussion, I withdraw this objection. Things also make more sense to me once this is made a class property instead of an instance property, which you've done in your proposed patches. > Should be add some flag to generic property to mark it as internal? If such a thing exists that would be good.
diff --git a/exec.c b/exec.c index fb73910..3b36fe5 100644 --- a/exec.c +++ b/exec.c @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) void cpu_vmstate_register(CPUState *cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); + int instance_id = cpu->has_stable_cpu_id ? cpu->stable_cpu_id : + 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/qom/cpu.h b/include/qom/cpu.h index 331386f..527c021 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. + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_register calls + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id + * over cpu_index during vmstate registration. * * State of one CPU core or thread. */ @@ -360,6 +363,8 @@ 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; + int stable_cpu_id; + bool has_stable_cpu_id; }; QTAILQ_HEAD(CPUTailQ, CPUState); diff --git a/qom/cpu.c b/qom/cpu.c index 1095ea1..bae1bf7 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) return cpu->cpu_index; } +static Property cpu_common_properties[] = { + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_cpu_id, false), + DEFINE_PROP_END_OF_LIST() +}; + static void cpu_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -394,6 +399,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) * IRQs, adding reset handlers, halting non-first CPUs, ... */ dc->cannot_instantiate_with_device_add_yet = true; + dc->props = cpu_common_properties; } static const TypeInfo cpu_type_info = {
Add CPUState::stable_cpu_id and use that as instance_id in vmstate_register() call. Introduce has-stable_cpu_id property that allows target machines to optionally switch to using stable_cpu_id instead of cpu_index. This will help allow successful migration in cases where holes are introduced in cpu_index range after CPU hot removals. Suggested-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- exec.c | 6 ++++-- include/qom/cpu.h | 5 +++++ qom/cpu.c | 6 ++++++ 3 files changed, 15 insertions(+), 2 deletions(-)