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

[Alexander Graf <agraf@suse.de>]

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 <aik@ozlabs.ru>
> ---
>
>
> !!! 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.

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, 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.


Alex