From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34709) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3z4a-0000L3-JW for qemu-devel@nongnu.org; Tue, 08 Nov 2016 00:35:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c3z4X-0002Gd-FB for qemu-devel@nongnu.org; Tue, 08 Nov 2016 00:35:52 -0500 Date: Tue, 8 Nov 2016 16:26:31 +1100 From: David Gibson Message-ID: <20161108052631.GU28688@umbus.fritz.box> References: <1477825928-10803-1-git-send-email-david@gibson.dropbear.id.au> <1477825928-10803-14-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4WCFFtl4AQpQKunj" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC 13/17] pseries: Move CPU compatibility property to machine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: nikunj@linux.vnet.ibm.com, mdroth@linux.vnet.ibm.com, thuth@redhat.com, lvivier@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --4WCFFtl4AQpQKunj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 04, 2016 at 06:43:52PM +1100, Alexey Kardashevskiy wrote: > On 30/10/16 22:12, David Gibson wrote: > > Server class POWER CPUs have a "compat" property, which is used to set = the > > backwards compatibility mode for the processor. However, this only mak= es > > sense for machine types which don't give the guest access to hypervisor > > privilege - otherwise the compatibility level is under the guest's cont= rol. > >=20 > > To reflect this, this removes the CPU 'compat' property and instead > > creates a 'max-cpu-compat' property on the pseries machine. Strictly > > speaking this breaks compatibility, but AFAIK the 'compat' option was > > never (directly) used with -device or device_add. > >=20 > > The option was used with -cpu. So, to maintain compatibility, this pat= ch > > adds a hack to the cpu option parsing to strip out any compat options > > supplied with -cpu and set them on the machine property instead of the = new > > removed cpu property. > >=20 > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 6 +++- > > hw/ppc/spapr_cpu_core.c | 47 +++++++++++++++++++++++++++-- > > hw/ppc/spapr_hcall.c | 2 +- > > include/hw/ppc/spapr.h | 10 +++++-- > > target-ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++++= +++ > > target-ppc/cpu.h | 6 ++-- > > target-ppc/translate_init.c | 73 -------------------------------------= -------- > > 7 files changed, 127 insertions(+), 82 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 6c78889..b983faa 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1849,7 +1849,7 @@ static void ppc_spapr_init(MachineState *machine) > > machine->cpu_model =3D kvm_enabled() ? "host" : smc->tcg_defau= lt_cpu; > > } > > =20 > > - ppc_cpu_parse_features(machine->cpu_model); > > + spapr_cpu_parse_features(spapr); > > =20 > > spapr_init_cpus(spapr); > > =20 > > @@ -2191,6 +2191,10 @@ static void spapr_machine_initfn(Object *obj) > > " place of standard EPOW events wh= en possible" > > " (required for memory hot-unplug = support)", > > NULL); > > + > > + object_property_add(obj, "max-cpu-compat", "str", > > + ppc_compat_prop_get, ppc_compat_prop_set, > > + NULL, &spapr->max_compat_pvr, &error_fatal); > > } > > =20 > > static void spapr_machine_finalizefn(Object *obj) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index ee5cd14..0319516 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -18,6 +18,49 @@ > > #include "target-ppc/mmu-hash64.h" > > #include "sysemu/numa.h" > > =20 > > +void spapr_cpu_parse_features(sPAPRMachineState *spapr) > > +{ > > + /* > > + * Backwards compatibility hack: > > + > > + * CPUs had a "compat=3D" property which didn't make sense for > > + * anything except pseries. It was replaced by "max-cpu-compat" > > + * machine option. This supports old command lines like > > + * -cpu POWER8,compat=3Dpower7 > > + * By stripping the compat option and applying it to the machine > > + * before passing it on to the cpu level parser. > > + */ > > + gchar **inpieces, **outpieces; > > + int n, i, j; > > + gchar *compat_str =3D NULL; > > + gchar *filtered_model; > > + > > + inpieces =3D g_strsplit(MACHINE(spapr)->cpu_model, ",", 0); > > + n =3D g_strv_length(inpieces); > > + outpieces =3D g_new0(gchar *, g_strv_length(inpieces)); > > + > > + /* inpieces[0] is the actual model string */ > > + for (i =3D 0, j =3D 0; i < n; i++) { > > + if (g_str_has_prefix(inpieces[i], "compat=3D")) { > > + compat_str =3D inpieces[i]; > > + } else { > > + outpieces[j++] =3D g_strdup(inpieces[i]); > > + } > > + } > > + > > + if (compat_str) { > > + char *val =3D compat_str + strlen("compat=3D"); > > + object_property_set_str(OBJECT(spapr), val, "max-cpu-compat", > > + &error_fatal); >=20 > This part is ok. >=20 > > + } > > + > > + filtered_model =3D g_strjoinv(",", outpieces); > > + ppc_cpu_parse_features(filtered_model); >=20 >=20 > Rather than reducing the CPU parameters string from the command line, I'd > keep "dc->props =3D powerpc_servercpu_properties" and make them noop + wa= rn > to use the machine option instead. One day QEMU may start calling the CPU > features parser itself and somebody will have to hack this thing > again. Hrm. A deprecation message like that only works if a human is reading it. Usually qemu will be invoked by libvirt and the message will probably disappear into some log file to scare someone unnecessarily. Meanwhile, what will the actual behaviour be? Pulling the CPU's property value into the machine instead would be really ugly. Ignoring it would break users with existing libvirt. The hack above is nasty, but I'm not really seeing a better option. >=20 >=20 > > + > > + g_strfreev(inpieces); > > + g_strfreev(outpieces); > > +} > > + > > static void spapr_cpu_reset(void *opaque) > > { > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > @@ -60,10 +103,10 @@ static void spapr_cpu_init(sPAPRMachineState *spap= r, PowerPCCPU *cpu, > > cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr)); > > cpu_ppc_set_papr(cpu); > > =20 > > - if (cpu->max_compat) { > > + if (spapr->max_compat_pvr) { > > Error *local_err =3D NULL; > > =20 > > - ppc_set_compat(cpu, cpu->max_compat, &local_err); > > + ppc_set_compat(cpu, spapr->max_compat_pvr, &local_err); > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > index 4eaf9a6..5efa57f 100644 > > --- a/hw/ppc/spapr_hcall.c > > +++ b/hw/ppc/spapr_hcall.c > > @@ -889,7 +889,7 @@ static target_ulong h_client_architecture_support(P= owerPCCPU *cpu, > > target_ulong list =3D ppc64_phys_to_real(args[0]); > > target_ulong ov_table; > > bool explicit_match =3D false; /* Matched the CPU's real PVR */ > > - uint32_t max_compat =3D cpu->max_compat; > > + uint32_t max_compat =3D spapr->max_compat_pvr; > > uint32_t best_compat =3D 0; > > int i; > > sPAPROptionVector *ov5_guest, *ov5_cas_old, *ov5_updates; > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 04d2821..d2a40e9 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -74,15 +74,18 @@ struct sPAPRMachineState { > > uint64_t rtc_offset; /* Now used only during incoming migration */ > > struct PPCTimebase tb; > > bool has_graphics; > > - sPAPROptionVector *ov5; /* QEMU-supported option vectors */ > > - sPAPROptionVector *ov5_cas; /* negotiated (via CAS) option vec= tors */ > > - bool cas_reboot; > > =20 > > Notifier epow_notifier; > > QTAILQ_HEAD(, sPAPREventLogEntry) pending_events; > > bool use_hotplug_event_source; > > sPAPREventSource *event_sources; > > =20 > > + /* ibm,client-architecture-support option negotiation */ > > + bool cas_reboot; > > + sPAPROptionVector *ov5; /* QEMU-supported option vectors */ > > + sPAPROptionVector *ov5_cas; /* negotiated (via CAS) option vec= tors */ > > + uint32_t max_compat_pvr; > > + >=20 > Nit: 3 of 5 lines were just moved while you could just add max_compat_pvr > and the comment before ov5. QEMU already has too many patches moving lines > around, sometime it is getting annoying while browsing with git blame. >=20 >=20 >=20 > > /* Migration state */ > > int htab_save_index; > > bool htab_first_pass; > > @@ -613,6 +616,7 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPRDR= ConnectorType drc_type, > > uint32_t count, uint32_t i= ndex); > > void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType dr= c_type, > > uint32_t count, uint32_= t index); > > +void spapr_cpu_parse_features(sPAPRMachineState *spapr); > > void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset, > > sPAPRMachineState *spapr); > > =20 > > diff --git a/target-ppc/compat.c b/target-ppc/compat.c > > index 0b12b58..c3ae52d 100644 > > --- a/target-ppc/compat.c > > +++ b/target-ppc/compat.c > > @@ -23,9 +23,11 @@ > > #include "sysemu/cpus.h" > > #include "qemu/error-report.h" > > #include "qapi/error.h" > > +#include "qapi/visitor.h" > > #include "cpu-models.h" > > =20 > > typedef struct { > > + const char *name; > > uint32_t pvr; > > uint64_t pcr; > > uint64_t pcr_level; > > @@ -37,6 +39,7 @@ static const CompatInfo compat_table[] =3D { > > * Ordered from oldest to newest - the code relies on this > > */ > > { /* POWER6, ISA2.05 */ > > + .name =3D "power6", > > .pvr =3D CPU_POWERPC_LOGICAL_2_05, > > .pcr =3D PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COMPAT_2_05 > > | PCR_TM_DIS | PCR_VSX_DIS, > > @@ -44,18 +47,21 @@ static const CompatInfo compat_table[] =3D { > > .max_threads =3D 2, > > }, > > { /* POWER7, ISA2.06 */ > > + .name =3D "power7", > > .pvr =3D CPU_POWERPC_LOGICAL_2_06, > > .pcr =3D PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS, > > .pcr_level =3D PCR_COMPAT_2_06, > > .max_threads =3D 4, > > }, > > { > > + .name =3D "power7+", > > .pvr =3D CPU_POWERPC_LOGICAL_2_06_PLUS, > > .pcr =3D PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS, > > .pcr_level =3D PCR_COMPAT_2_06, > > .max_threads =3D 4, > > }, > > { /* POWER8, ISA2.07 */ > > + .name =3D "power8", > > .pvr =3D CPU_POWERPC_LOGICAL_2_07, > > .pcr =3D PCR_COMPAT_2_07, > > .pcr_level =3D PCR_COMPAT_2_07, > > @@ -184,3 +190,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu) > > =20 > > return n_threads; > > } > > + > > +void ppc_compat_prop_get(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + uint32_t compat_pvr =3D *((uint32_t *)opaque); > > + const char *value; > > + > > + if (!compat_pvr) { > > + value =3D ""; > > + } else { > > + const CompatInfo *compat =3D compat_by_pvr(compat_pvr); > > + > > + g_assert(compat); > > + > > + value =3D compat->name; > > + } > > + > > + visit_type_str(v, name, (char **)&value, errp); > > +} > > + > > +void ppc_compat_prop_set(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + Error *error =3D NULL; > > + char *value; > > + uint32_t compat_pvr; > > + > > + visit_type_str(v, name, &value, &error); > > + if (error) { > > + error_propagate(errp, error); > > + return; > > + } > > + > > + if (strcmp(value, "") =3D=3D 0) { > > + compat_pvr =3D 0; > > + } else { > > + int i; > > + const CompatInfo *compat =3D NULL; > > + > > + for (i =3D 0; i < ARRAY_SIZE(compat_table); i++) { > > + if (strcmp(value, compat_table[i].name) =3D=3D 0) { > > + compat =3D &compat_table[i]; > > + break; > > + > > + } > > + } > > + > > + if (!compat) { > > + error_setg(errp, "Invalid compatibility mode \"%s\"", valu= e); > > + goto out; > > + } > > + compat_pvr =3D compat->pvr; > > + } > > + > > + *((uint32_t *)opaque) =3D compat_pvr; > > + > > +out: > > + g_free(value); > > +} > > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > > index 201a655..51d21bb 100644 > > --- a/target-ppc/cpu.h > > +++ b/target-ppc/cpu.h > > @@ -1155,7 +1155,6 @@ typedef struct PPCVirtualHypervisorClass PPCVirtu= alHypervisorClass; > > * PowerPCCPU: > > * @env: #CPUPPCState > > * @cpu_dt_id: CPU index used in the device tree. KVM uses this index = too > > - * @max_compat: Maximal supported logical PVR from the command line > > * @compat_pvr: Current logical PVR, zero if in "raw" mode > > * > > * A PowerPC CPU. > > @@ -1167,7 +1166,6 @@ struct PowerPCCPU { > > =20 > > CPUPPCState env; > > int cpu_dt_id; > > - uint32_t max_compat; > > uint32_t compat_pvr; > > PPCVirtualHypervisor *vhyp; > > }; > > @@ -1321,6 +1319,10 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t co= mpat_pvr, Error **errp); > > void ppc_set_compat_all(uint32_t compat_pvr, Error **errp); > > #endif > > int ppc_compat_max_threads(PowerPCCPU *cpu); > > +void ppc_compat_prop_get(Object *obj, Visitor *v, > > + const char *name, void *opaque, Error **err); > > +void ppc_compat_prop_set(Object *obj, Visitor *v, > > + const char *name, void *opaque, Error **err); > > #endif /* defined(TARGET_PPC64) */ > > =20 > > #include "exec/cpu-all.h" > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index ba48242..40abd23 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -8428,76 +8428,6 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *d= ata) > > pcc->l1_icache_size =3D 0x10000; > > } > > =20 > > -static void powerpc_get_compat(Object *obj, Visitor *v, const char *na= me, > > - void *opaque, Error **errp) > > -{ > > - char *value =3D (char *)""; > > - Property *prop =3D opaque; > > - uint32_t *max_compat =3D qdev_get_prop_ptr(DEVICE(obj), prop); > > - > > - switch (*max_compat) { > > - case CPU_POWERPC_LOGICAL_2_05: > > - value =3D (char *)"power6"; > > - break; > > - case CPU_POWERPC_LOGICAL_2_06: > > - value =3D (char *)"power7"; > > - break; > > - case CPU_POWERPC_LOGICAL_2_07: > > - value =3D (char *)"power8"; > > - break; > > - case 0: > > - break; > > - default: > > - error_report("Internal error: compat is set to %x", *max_compa= t); > > - abort(); > > - break; > > - } > > - > > - visit_type_str(v, name, &value, errp); > > -} > > - > > -static void powerpc_set_compat(Object *obj, Visitor *v, const char *na= me, > > - void *opaque, Error **errp) > > -{ > > - Error *error =3D NULL; > > - char *value =3D NULL; > > - Property *prop =3D opaque; > > - uint32_t *max_compat =3D qdev_get_prop_ptr(DEVICE(obj), prop); > > - > > - visit_type_str(v, name, &value, &error); > > - if (error) { > > - error_propagate(errp, error); > > - return; > > - } > > - > > - if (strcmp(value, "power6") =3D=3D 0) { > > - *max_compat =3D CPU_POWERPC_LOGICAL_2_05; > > - } else if (strcmp(value, "power7") =3D=3D 0) { > > - *max_compat =3D CPU_POWERPC_LOGICAL_2_06; > > - } else if (strcmp(value, "power8") =3D=3D 0) { > > - *max_compat =3D CPU_POWERPC_LOGICAL_2_07; > > - } else { > > - error_setg(errp, "Invalid compatibility mode \"%s\"", value); > > - } > > - > > - g_free(value); > > -} > > - > > -static PropertyInfo powerpc_compat_propinfo =3D { > > - .name =3D "str", > > - .description =3D "compatibility mode, power6/power7/power8", > > - .get =3D powerpc_get_compat, > > - .set =3D powerpc_set_compat, > > -}; > > - > > -#define DEFINE_PROP_POWERPC_COMPAT(_n, _s, _f) \ > > - DEFINE_PROP(_n, _s, _f, powerpc_compat_propinfo, uint32_t) > > - > > -static Property powerpc_servercpu_properties[] =3D { > > - DEFINE_PROP_POWERPC_COMPAT("compat", PowerPCCPU, max_compat), > > - DEFINE_PROP_END_OF_LIST(), > > -}; > > - > > #ifdef CONFIG_SOFTMMU > > static const struct ppc_segment_page_sizes POWER7_POWER8_sps =3D { > > .sps =3D { > > @@ -8586,7 +8516,6 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *dat= a) > > =20 > > dc->fw_name =3D "PowerPC,POWER7"; > > dc->desc =3D "POWER7"; > > - dc->props =3D powerpc_servercpu_properties; > > pcc->pvr_match =3D ppc_pvr_match_power7; > > pcc->pcr_mask =3D PCR_VEC_DIS | PCR_VSX_DIS | PCR_COMPAT_2_05; > > pcc->pcr_supported =3D PCR_COMPAT_2_06 | PCR_COMPAT_2_05; > > @@ -8713,7 +8642,6 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *dat= a) > > =20 > > dc->fw_name =3D "PowerPC,POWER8"; > > dc->desc =3D "POWER8"; > > - dc->props =3D powerpc_servercpu_properties; > > pcc->pvr_match =3D ppc_pvr_match_power8; > > pcc->pcr_mask =3D PCR_TM_DIS | PCR_COMPAT_2_06 | PCR_COMPAT_2_05; > > pcc->pcr_supported =3D PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COM= PAT_2_05; > > @@ -8794,7 +8722,6 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *dat= a) > > =20 > > dc->fw_name =3D "PowerPC,POWER9"; > > dc->desc =3D "POWER9"; > > - dc->props =3D powerpc_servercpu_properties; > > pcc->pvr_match =3D ppc_pvr_match_power9; > > pcc->pcr_mask =3D PCR_COMPAT_2_05 | PCR_COMPAT_2_06 | PCR_COMPAT_2= _07; > > pcc->init_proc =3D init_proc_POWER9; > >=20 >=20 >=20 --=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 --4WCFFtl4AQpQKunj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYIWIHAAoJEGw4ysog2bOS9d4P/i+WUVnaDqnRuQEHwHNgtjp4 PqNixcEwTyrH0oMBW/NQ4Ei2aaxIB+t45wISreyQCcdVXmP3gNk94qW3yvQbfz+N m6xDJMoyLf2S6u/vfH+eyQNPr08/laLoipRD1msLcy/LRiXSbbjRzYzx6rzsUfuM pAj4ACEAZFF0hxMa/GtjVeDYBeXjfISWRviC/Kle5Ys1daWWo5zzyhYAtWtaAneq +L5TQo/BMwm5OB2Wib41TuCh6fjXP+deaWWjTyb5FXT7yb+EScBN3qClmxxNrWqg 0qvjeZRiBkiG36Clx3Kv9PQMY2VPR4rxEBGSgQ5mQHg4QuFfw4+33uhKXNP9Y78o +Ipr6HIo/wAbDEieK+xVhxZLYTlU7s3Fe2TMb7sxexNLEru6zR+To/W1YcsXuVPr MhWYIdMCTuiScGQRpt2GnFHzCtz6fCO0s/ycWIY6aYg8aRG/YV86FyJApekJ69ow fERJboXr6JlsA4LFrDgRoG3OR9A/v7PehuvP3J6y9g9BGMm+xVfPhPmmtxksuDW4 xeijaunsz7CIQ+KNPvqcp5tCVVH4aG9EWadREwTJCBdYVk/DEfWz/ANm0ddIcFnH jYGycayeUfkNVFo0BZWb0z2UiajtLHA8d64eyqWW+6SV1JYq8VSn5SW5tgzjXKWp u09x1N4B1nXjNaQ0fPcc =pbpb -----END PGP SIGNATURE----- --4WCFFtl4AQpQKunj--