From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMpuz-0002GL-I1 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 01:07:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMpuu-00021a-GP for qemu-devel@nongnu.org; Tue, 12 Jul 2016 01:07:36 -0400 Received: from ozlabs.org ([103.22.144.67]:49069) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMput-00021H-QO for qemu-devel@nongnu.org; Tue, 12 Jul 2016 01:07:32 -0400 Date: Tue, 12 Jul 2016 15:07:09 +1000 From: David Gibson Message-ID: <20160712050709.GT16355@voom.fritz.box> References: <20160711033507.GN25522@in.ibm.com> <1468244550-33910-1-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OelmujB7rQAg+yu0" Content-Disposition: inline In-Reply-To: <1468244550-33910-1-git-send-email-imammedo@redhat.com> Subject: Re: [Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, bharata@linux.vnet.ibm.com, ehabkost@redhat.com, marcel@redhat.com, pbonzini@redhat.com --OelmujB7rQAg+yu0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote: > this approach i I preffer as it uses less per machine migration glue > and follows typical compat pattern for devices >=20 > Signed-off-by: Igor Mammedov > Signed-off-by: Bharata B Rao After our IRC discussion last night, I prefer this version as well. We can definitely work with this on Power as well. Reviewed-by: David Gibson One query regarding the x86 specific implementation below, though. > --- > exec.c | 7 +++++-- > hw/i386/pc.c | 10 +++++++--- > include/hw/compat.h | 6 +++++- > include/qom/cpu.h | 3 +++ > qom/cpu.c | 13 +++++++++++++ > 5 files changed, 33 insertions(+), 6 deletions(-) >=20 > diff --git a/exec.c b/exec.c > index 0122ef7..79e1dcb 100644 > --- a/exec.c > +++ b/exec.c > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > { > CPUClass *cc =3D CPU_GET_CLASS(cpu); > Error *local_err =3D NULL; > + int migration_id; > =20 > cpu->as =3D NULL; > cpu->num_ases =3D 0; > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > (void) cc; > cpu_list_unlock(); > #else > + migration_id =3D cc->use_migration_id > 0 ? > + cpu->migration_id : cpu->cpu_index; > if (qdev_get_vmsd(DEVICE(cpu)) =3D=3D NULL) { > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); > + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); > } > if (cc->vmsd !=3D NULL) { > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > + vmstate_register(NULL, migration_id, cc->vmsd, cpu); > } > #endif > } > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index cd1745e..f041279 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, = int level) > } > =20 > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > - Error **errp) > + Error **errp, int migration_id) > { > X86CPU *cpu =3D NULL; > + CPUState *cs =3D CPU(cpu); > Error *local_err =3D NULL; > Any reason not to re-use the apic_id as the migration_id? It's 64-bit here which is no good, but apic_id_t is defined as uint32_t elsewhere, so is that just a bug with the function signature? > cpu =3D cpu_x86_create(cpu_model, &local_err); > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, in= t64_t apic_id, > goto out; > } > =20 > + cs->migration_id =3D migration_id; > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > =20 > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > return; > } > =20 > - cpu =3D pc_new_cpu(machine->cpu_model, apic_id, &local_err); > + cpu =3D pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) > pcms->possible_cpus->len++; > if (i < smp_cpus) { > cpu =3D pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_= index(i), > - &error_fatal); > + &error_fatal, i); > pcms->possible_cpus->cpus[i].cpu =3D CPU(cpu); > object_unref(OBJECT(cpu)); > } > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, = void *data) > PCMachineClass *pcmc =3D PC_MACHINE_CLASS(oc); > HotplugHandlerClass *hc =3D HOTPLUG_HANDLER_CLASS(oc); > NMIClass *nc =3D NMI_CLASS(oc); > + CPUClass *cc =3D CPU_CLASS(object_class_by_name(TYPE_CPU)); > =20 > + cc->use_migration_id =3D true; > pcmc->get_hotplug_handler =3D mc->get_hotplug_handler; > pcmc->pci_enabled =3D true; > pcmc->has_acpi_build =3D true; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 636befe..a66e80d 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,11 @@ > #define HW_COMPAT_H > =20 > #define HW_COMPAT_2_6 \ > - /* empty */ > + { \ > + .driver =3D TYPE_CPU, \ > + .property =3D "use-migration-id", \ > + .value =3D "off", \ > + }, > =20 > #define HW_COMPAT_2_5 \ > {\ > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 32f3af3..a29a2c9 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -177,6 +177,8 @@ typedef struct CPUClass { > void *opaque); > =20 > const struct VMStateDescription *vmsd; > + int use_migration_id; > + > int gdb_num_core_regs; > const char *gdb_core_xml_file; > gchar * (*gdb_arch_name)(CPUState *cpu); > @@ -360,6 +362,7 @@ struct CPUState { > (absolute value) offset as small as possible. This reduces code > size, especially for hosts without large memory offsets. */ > uint32_t tcg_exit_req; > + int migration_id; Not really in scope for this patch, but I do wonder if vmstate_register() should actually take a uint32_t instead of an int, since that's what it is on the wire, IIUC. > }; > =20 > QTAILQ_HEAD(CPUTailQ, CPUState); > diff --git a/qom/cpu.c b/qom/cpu.c > index 751e992..3eb48b9 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > return cpu->cpu_index; > } > =20 > +static void cpu_common_set_use_migration_id(Object *obj, bool value, > + Error **err) > +{ > + CPUClass *cc =3D CPU_GET_CLASS(obj); > + cc->use_migration_id =3D value ? 1 : 0; > +} > + > static void cpu_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc =3D DEVICE_CLASS(klass); > CPUClass *k =3D CPU_CLASS(klass); > =20 > + k->use_migration_id =3D -1; > + object_class_property_add_bool(klass, "use-migration-id", > + NULL, > + cpu_common_set_use_migration_id, > + &error_abort); > + > k->class_by_name =3D cpu_common_class_by_name; > k->parse_features =3D cpu_common_parse_features; > k->reset =3D cpu_common_reset; --=20 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 --OelmujB7rQAg+yu0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXhHr9AAoJEGw4ysog2bOSIgIQAMiXQwc2YtQ0DYqA7mxdJonp igATkPhQXHiLPYV+THXHDjbZES3UQ7Sno+fJM4J60OFF3Di6lu2+7hzwY4HiK/bO JkcPu5SjdEtOq+KsG8sld8LGdBXnTfOxYIhmWrTU8x/T4dDmHLREiqt8R2WcYWdx GRV17Em8ZZw1HXZNEeowT5dzoBeINIeAaquz3JPY/9CDTZoBqDOancxfA8RUilO+ L/H9XMWpCO7crfvEb5onBeDnp5e+u4QnVs1Zwfs+5NKGH+dp4rUETBo3ckeURzxs 7KGmxc49jOX3Ag44qY3hNYdMx6hW/YezZQ5paJGFRTOrhowurzV0nd0p/RWMDozp DGWrloTiy1DFGM3/qAO1vCQWBi0TP839qVBLwrRu86NimODzI6EZ3z8W6U8JHdfn lFTv9VNXqr+2cQr8d8jDP/dKwb2tGHNVkwIWAxxmHeP+fWkYhMn6mChNVwcgPZLR YjEyy1E0ACAEZJmkhi+wGJL55doNaar60669WbRqYf6+v/vwv1Lylh/HL0qUfJVY oHjWrxOk/ayBw/L9CJ/day1Kv9sWdoDVwmGDTqs1EoVPa4WuV1bi3Tpa24kawcZD kMfnNffNFWYQn62IT/mOil8/Z/QF/myl11dQ/A+/gXTqCIGYpxASENWVYyIHKF9W XaZGN96+RbbDEUhAzGa6 =G/dV -----END PGP SIGNATURE----- --OelmujB7rQAg+yu0--