Re: [Qemu-ppc] [RFC PATCH] target-ppc: Relax use of generic CPU name for KVM

[Alexey Kardashevskiy]

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