[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC PATCH] target-ppc: Relax use of generic CPU name for
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-ppc] [RFC PATCH] target-ppc: Relax use of generic CPU name for KVM |
Date: |
Sat, 12 Apr 2014 03:16:34 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
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 <address@hidden>
>> ---
>>
>>
>> !!! 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