On 04/11/2014 07:00 PM, Alexander Graf wrote: > > On 11.04.14 07:00, Alexey Kardashevskiy wrote: >> At the moment generic version-less CPUs are supported via hardcoded aliases. >> For example, POWER7 is an alias for POWER7_v2.1. So when QEMU is started >> with -cpu POWER7, the POWER7_v2.1 class instance is created. >> >> This approach works for TCG and KVMs other than HV KVM. HV KVM cannot >> emulate >> PVR value so the guest always sees the real PVR. HV KVM will not allow >> setting >> PVR other that the host PVR because of that (the kernel patch for it is on >> its way). So in most cases it is impossible to run QEMU with -cpu POWER7 >> unless the host PVR is exactly the same as the one from the alias (which >> is now POWER7_v2.3). It was decided that under HV KVM QEMU should use >> -cpu host. >> >> Using "host" CPU type creates a problem for management tools such as libvirt >> because they want to know in advance if the destination guest can possibly >> run on the destination. Since the "host" type is really not a type and will >> always work with any KVM, there is no way for libvirt to know if the >> migration >> will success. >> >> This patch changes aliases handling by lowering their priority and adding >> a new CPU generic class the same way as it is done for the "host" CPU class. >> >> This registers additional CPU class derived from the host CPU family. >> The name for it is taken from @desc field of the CPU family class. >> >> This moves aliases lookup after CPU class lookup. This is to let new generic >> CPU to be found first if it is present and only if it is not (TCG case), use > > The nice part about this is that we will also use the alias for CPUs that > are not the type we're running on. So if I use -cpu POWER7 on a POWER7 > host, it will give me my host's POWER7 CPU. But if I use -cpu 970 on that > POWER7 host it will use the alias. > >> aliases. >> >> Signed-off-by: Alexey Kardashevskiy <[email protected]> >> --- >> >> >> !!! THIS IS NOT FOR 2.0 !!! >> >> Just an RFC :) >> >> Is that the right direction to go? >> >> I would also remove POWER7_v2.0 and POWER7_v2.1 and leave just one versioned >> CPU per family (which is POWER7_v2.3 with POWER7 alias). We do not emulate >> these CPUs diffent so it does not make much sense to keep them, one per >> family >> is perfectly enough. >> >> >> --- >> target-ppc/cpu-models.c | 4 ---- >> target-ppc/cpu-models.h | 2 -- >> target-ppc/kvm.c | 20 ++++++++++++++++++++ >> target-ppc/translate_init.c | 18 +++++++++++------- >> 4 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c >> index f6c9b3a..57cb4e4 100644 >> --- a/target-ppc/cpu-models.c >> +++ b/target-ppc/cpu-models.c >> @@ -1134,10 +1134,6 @@ >> POWERPC_DEF("POWER6A", CPU_POWERPC_POWER6A, >> POWER6, >> "POWER6A") >> #endif >> - POWERPC_DEF("POWER7_v2.0", CPU_POWERPC_POWER7_v20, >> POWER7, >> - "POWER7 v2.0") >> - POWERPC_DEF("POWER7_v2.1", CPU_POWERPC_POWER7_v21, >> POWER7, >> - "POWER7 v2.1") >> POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, >> POWER7, >> "POWER7 v2.3") >> POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, >> POWER7P, >> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h >> index 644a126..9a003b4 100644 >> --- a/target-ppc/cpu-models.h >> +++ b/target-ppc/cpu-models.h >> @@ -555,8 +555,6 @@ enum { >> CPU_POWERPC_POWER6A = 0x0F000002, >> CPU_POWERPC_POWER7_BASE = 0x003F0000, >> CPU_POWERPC_POWER7_MASK = 0xFFFF0000, >> - CPU_POWERPC_POWER7_v20 = 0x003F0200, >> - CPU_POWERPC_POWER7_v21 = 0x003F0201, > > I think it makes sense to do the removal in a separate patch. > >> CPU_POWERPC_POWER7_v23 = 0x003F0203, >> CPU_POWERPC_POWER7P_BASE = 0x004A0000, >> CPU_POWERPC_POWER7P_MASK = 0xFFFF0000, >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> index ff952f0..b2e4db5 100644 >> --- a/target-ppc/kvm.c >> +++ b/target-ppc/kvm.c >> @@ -1785,6 +1785,17 @@ bool kvmppc_has_cap_htab_fd(void) >> return cap_htab_fd; >> } >> +static PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc) >> +{ >> + ObjectClass *oc = OBJECT_CLASS(pcc); >> + >> + while (oc && !object_class_is_abstract(oc)) { >> + oc = object_class_get_parent(oc); >> + } >> + >> + return POWERPC_CPU_CLASS(oc); > > What if we don't find any? It should never happen, but better put an > assert(oc) before the return. > >> +} >> + >> static int kvm_ppc_register_host_cpu_type(void) >> { >> TypeInfo type_info = { >> @@ -1794,6 +1805,7 @@ static int kvm_ppc_register_host_cpu_type(void) >> }; >> uint32_t host_pvr = mfpvr(); >> PowerPCCPUClass *pvr_pcc; >> + DeviceClass *dc; >> pvr_pcc = ppc_cpu_class_by_pvr(host_pvr); >> if (pvr_pcc == NULL) { >> @@ -1804,6 +1816,14 @@ static int kvm_ppc_register_host_cpu_type(void) >> } >> type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc)); >> type_register(&type_info); >> + >> + /* Register generic family CPU class for a family */ >> + pvr_pcc = ppc_cpu_get_family_class(pvr_pcc); >> + dc = DEVICE_CLASS(pvr_pcc); >> + type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc)); >> + type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc); >> + type_register(&type_info); > > Heh, nice trick. Just generate the class on the fly like we do for -cpu > host. I like it. > >> + >> return 0; >> } >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index 4d94015..823c63c 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -8218,12 +8218,6 @@ static ObjectClass *ppc_cpu_class_by_name(const >> char *name) >> } >> } >> - for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) { >> - if (strcmp(ppc_cpu_aliases[i].alias, name) == 0) { >> - return ppc_cpu_class_by_alias(&ppc_cpu_aliases[i]); >> - } >> - } >> - >> list = object_class_get_list(TYPE_POWERPC_CPU, false); >> item = g_slist_find_custom(list, name, ppc_cpu_compare_class_name); >> if (item != NULL) { >> @@ -8231,7 +8225,17 @@ static ObjectClass *ppc_cpu_class_by_name(const >> char *name) >> } >> g_slist_free(list); >> - return ret; >> + if (ret) { >> + return ret; >> + } >> + >> + for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) { >> + if (strcmp(ppc_cpu_aliases[i].alias, name) == 0) { >> + return ppc_cpu_class_by_alias(&ppc_cpu_aliases[i]); >> + } >> + } > > Classes first aliases later. Very good - makes a lot of sense. I would > split this into a separate patch. > > Otherwise I like the patch. It's a simple and clean approach to the > problem. Now the only thing missing to migration compatibility is the host > cpu type query mechanism you can check out with the s390 people.
Thanks :) > Also while at it, maybe you should poke your hardware people to ensure that > we can disable kernel level features from one POWER version to the next, We cannot disable these features, I mean we can for user space but not for the kernel, even for the guest kernel [1]. Eeever I suppose. Adding Paul to cc: in case if I am lying here :) > so > that we could support -cpu POWER8 on a POWER9 (or whatever it will be > called) system and give users a chance for easy migration. "compat" CPU option is supposed do it. With [1], I do not see how we could enable POWER8 on actual POWER9 machine... -- Alexey
