[Qemu-devel] [PATCH v2 0/5] spapr: Fix regression in CPU alias handling

[Qemu-devel] [PATCH v2 0/5] spapr: Fix regression in CPU alias handling

[Thomas Huth]

There is a regression with the "-cpu" parameter which has been
introduced by the spapr CPU hotplug code: We used to allow to specify
a "CPU family" name with the "-cpu" parameter when running on KVM so
that the user does not need to know the gory details of the exact
CPU version of the host CPU. For example, it was possible to
use "-cpu POWER8" on a POWER8E host CPU. This behavior does not
work anymore with the new hot-pluggable spapr-cpu-core types.
Since libvirt already heavily depends on the old behavior, this
is quite a severe regression in the QEMU parameter interface, thus
I think these patches should still go into 2.7 if possible, to avoid
that we break the "upper layers" with the final 2.7 release.

This patch series fixes the regression (and two more minor bugs)
by registering a family type for the spapr-cpu-core type, too
(see the last patch). However, since that name clashes with the
alias types that have been defined in spapr_cpu_core.c, we first
have to introduce there a better way to handle CPU aliases (see
the first two patches).
The third patch fixes a small memory leak along the way, and
the fourth patch makes sure that we do not mess up the generic
CPU family type registration anymore (which was another regression
introduced with the spapr CPU hotplug code).

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1363812

v2:
- Add the "remove extra type variable" patch from Cédric Le Goater.
- Remove the "Do not leak the memory of the type string" patch
  since it got obsoleted by Cédric's patch

Cédric Le Goater (1):
  spapr: remove extra type variable

Thomas Huth (4):
  ppc: Introduce a function to look up CPU alias strings
  hw/ppc/spapr: Look up CPU alias names instead of hard-coding the
    aliases
  ppc/kvm: Do not mess up the generic CPU family registration
  ppc/kvm: Register also a generic spapr CPU core family type

 hw/ppc/spapr.c              | 15 ++++++---------
 hw/ppc/spapr_cpu_core.c     | 38 +++++++++++++++++++++-----------------
 target-ppc/cpu.h            |  1 +
 target-ppc/kvm.c            | 19 +++++++++++--------
 target-ppc/translate_init.c | 13 +++++++++++++
 5 files changed, 52 insertions(+), 34 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/5] spapr: remove extra type variable

[Thomas Huth]

From: Cédric Le Goater <clg@kaod.org>

The sPAPR CPU core typename is already available in the upper
block. Let's use it and move the check upward also.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 57564e5..399dcc0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1815,6 +1815,11 @@ static void ppc_spapr_init(MachineState *machine)
     if (mc->query_hotpluggable_cpus) {
         char *type = spapr_get_cpu_core_type(machine->cpu_model);
 
+        if (!object_class_by_name(type)) {
+            error_report("Unable to find sPAPR CPU Core definition");
+            exit(1);
+        }
+
         spapr->cores = g_new0(Object *, spapr_max_cores);
         for (i = 0; i < spapr_max_cores; i++) {
             int core_id = i * smp_threads;
@@ -1826,15 +1831,7 @@ static void ppc_spapr_init(MachineState *machine)
             qemu_register_reset(spapr_drc_reset, drc);
 
             if (i < spapr_cores) {
-                char *type = spapr_get_cpu_core_type(machine->cpu_model);
-                Object *core;
-
-                if (!object_class_by_name(type)) {
-                    error_report("Unable to find sPAPR CPU Core definition");
-                    exit(1);
-                }
-
-                core  = object_new(type);
+                Object *core  = object_new(type);
                 object_property_set_int(core, smp_threads, "nr-threads",
                                         &error_fatal);
                 object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/5] ppc: Introduce a function to look up CPU alias strings

[Thomas Huth]

We will need this function to look up the aliases in the
spapr-cpu-core code, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target-ppc/cpu.h            |  1 +
 target-ppc/translate_init.c | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 5fce1ff..786ab5c 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1201,6 +1201,7 @@ extern const struct VMStateDescription vmstate_ppc_cpu;
 /*****************************************************************************/
 PowerPCCPU *cpu_ppc_init(const char *cpu_model);
 void ppc_translate_init(void);
+const char *ppc_cpu_lookup_alias(const char *alias);
 void gen_update_current_nip(void *opaque);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
    signal handlers to inform the virtual CPU of exceptions. non zero
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 5f28a36..7a9b15e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10012,6 +10012,19 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
     return NULL;
 }
 
+const char *ppc_cpu_lookup_alias(const char *alias)
+{
+    int ai;
+
+    for (ai = 0; ppc_cpu_aliases[ai].alias != NULL; ai++) {
+        if (strcmp(ppc_cpu_aliases[ai].alias, alias) == 0) {
+            return ppc_cpu_aliases[ai].model;
+        }
+    }
+
+    return NULL;
+}
+
 PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 {
     return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/5] hw/ppc/spapr: Look up CPU alias names instead of hard-coding the aliases

[Thomas Huth]

Hard-coding the CPU alias names in the spapr_cores[] array has
two big disadvantages:

1) We register a real type with the CPU alias name in
   spapr_cpu_core_register_types() - this prevents us from registering
   a CPU family name in kvm_ppc_register_host_cpu_type() with the same
   name (as we do it for the non-hotpluggable CPU types).

2) It's quite cumbersome to maintain the aliases here in sync with the
   ppc_cpu_aliases list from target-ppc/cpu-models.c.

So let's simply add proper alias lookup to the spapr cpu core code,
too (by checking whether the given model can be used directly, and
if not by trying to look up the given model as an alias name instead).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c          |  2 +-
 hw/ppc/spapr_cpu_core.c | 38 +++++++++++++++++++++-----------------
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 399dcc0..0787c66 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1815,7 +1815,7 @@ static void ppc_spapr_init(MachineState *machine)
     if (mc->query_hotpluggable_cpus) {
         char *type = spapr_get_cpu_core_type(machine->cpu_model);
 
-        if (!object_class_by_name(type)) {
+        if (type == NULL) {
             error_report("Unable to find sPAPR CPU Core definition");
             exit(1);
         }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 716f7c4..bcb483d 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -93,6 +93,19 @@ char *spapr_get_cpu_core_type(const char *model)
 
     core_type = g_strdup_printf("%s-%s", model_pieces[0], TYPE_SPAPR_CPU_CORE);
     g_strfreev(model_pieces);
+
+    /* Check whether it exists or whether we have to look up an alias name */
+    if (!object_class_by_name(core_type)) {
+        const char *realmodel;
+
+        g_free(core_type);
+        realmodel = ppc_cpu_lookup_alias(model);
+        if (realmodel) {
+            return spapr_get_cpu_core_type(realmodel);
+        }
+        return NULL;
+    }
+
     return core_type;
 }
 
@@ -354,41 +367,32 @@ typedef struct SPAPRCoreInfo {
 } SPAPRCoreInfo;
 
 static const SPAPRCoreInfo spapr_cores[] = {
-    /* 970 and aliaes */
+    /* 970 */
     { .name = "970_v2.2", .initfn = spapr_cpu_core_970_initfn },
-    { .name = "970", .initfn = spapr_cpu_core_970_initfn },
 
-    /* 970MP variants and aliases */
+    /* 970MP variants */
     { .name = "970MP_v1.0", .initfn = spapr_cpu_core_970MP_v10_initfn },
     { .name = "970mp_v1.0", .initfn = spapr_cpu_core_970MP_v10_initfn },
     { .name = "970MP_v1.1", .initfn = spapr_cpu_core_970MP_v11_initfn },
     { .name = "970mp_v1.1", .initfn = spapr_cpu_core_970MP_v11_initfn },
-    { .name = "970mp", .initfn = spapr_cpu_core_970MP_v11_initfn },
 
-    /* POWER5 and aliases */
+    /* POWER5+ */
     { .name = "POWER5+_v2.1", .initfn = spapr_cpu_core_POWER5plus_initfn },
-    { .name = "POWER5+", .initfn = spapr_cpu_core_POWER5plus_initfn },
 
-    /* POWER7 and aliases */
+    /* POWER7 */
     { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
-    { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn },
 
-    /* POWER7+ and aliases */
+    /* POWER7+ */
     { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
-    { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn },
 
-    /* POWER8 and aliases */
+    /* POWER8 */
     { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
-    { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn },
-    { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn },
 
-    /* POWER8E and aliases */
+    /* POWER8E */
     { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
-    { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn },
 
-    /* POWER8NVL and aliases */
+    /* POWER8NVL */
     { .name = "POWER8NVL_v1.0", .initfn = spapr_cpu_core_POWER8NVL_initfn },
-    { .name = "POWER8NVL", .initfn = spapr_cpu_core_POWER8NVL_initfn },
 
     { .name = NULL }
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/5] ppc/kvm: Do not mess up the generic CPU family registration

[Thomas Huth]

The code for registering the sPAPR CPU host core type has been
added inbetween the generic CPU host core type and the generic
CPU family type. That way the instance_init and the class_init
information got lost when registering the generic CPU family
type. Fix it by moving the generic family registration before
the spapr cpu core registration code.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target-ppc/kvm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 84764ed..82b1df9 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2394,6 +2394,13 @@ 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);
+
 #if defined(TARGET_PPC64)
     type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host");
     type_info.parent = TYPE_SPAPR_CPU_CORE,
@@ -2406,13 +2413,6 @@ static int kvm_ppc_register_host_cpu_type(void)
     type_info.instance_init = NULL;
 #endif
 
-    /* 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);
-
     return 0;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/5] ppc/kvm: Register also a generic spapr CPU core family type

[Thomas Huth]

There is a regression with the "-cpu" parameter introduced by
the spapr CPU hotplug code: We used to allow to specify a
"CPU family" name with the "-cpu" parameter when running on KVM so
that the user does not need to know the gory details of the exact
CPU version of the host CPU. For example, it was possible to
use "-cpu POWER8" on a POWER8E host CPU. This behavior does not
work anymore with the new hot-pluggable spapr-cpu-core types.
Since libvirt already heavily depends on the old behavior, this
is quite a severe regression in the QEMU parameter interface.
Let's fix it by supporting a CPU family type for the spapr-cpu-core
on KVM, too.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1363812
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target-ppc/kvm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 82b1df9..dcb68b9 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2409,8 +2409,11 @@ static int kvm_ppc_register_host_cpu_type(void)
     type_info.class_init = NULL;
     type_register(&type_info);
     g_free((void *)type_info.name);
-    type_info.instance_size = 0;
-    type_info.instance_init = NULL;
+
+    /* Register generic spapr CPU family class for current host CPU type */
+    type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, dc->desc);
+    type_register(&type_info);
+    g_free((void *)type_info.name);
 #endif
 
     return 0;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/5] spapr: Fix regression in CPU alias handling

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]

On Tue, Aug 09, 2016 at 06:59:56PM +0200, Thomas Huth wrote:
> There is a regression with the "-cpu" parameter which has been
> introduced by the spapr CPU hotplug code: We used to allow to specify
> a "CPU family" name with the "-cpu" parameter when running on KVM so
> that the user does not need to know the gory details of the exact
> CPU version of the host CPU. For example, it was possible to
> use "-cpu POWER8" on a POWER8E host CPU. This behavior does not
> work anymore with the new hot-pluggable spapr-cpu-core types.
> Since libvirt already heavily depends on the old behavior, this
> is quite a severe regression in the QEMU parameter interface, thus
> I think these patches should still go into 2.7 if possible, to avoid
> that we break the "upper layers" with the final 2.7 release.
> 
> This patch series fixes the regression (and two more minor bugs)
> by registering a family type for the spapr-cpu-core type, too
> (see the last patch). However, since that name clashes with the
> alias types that have been defined in spapr_cpu_core.c, we first
> have to introduce there a better way to handle CPU aliases (see
> the first two patches).
> The third patch fixes a small memory leak along the way, and
> the fourth patch makes sure that we do not mess up the generic
> CPU family type registration anymore (which was another regression
> introduced with the spapr CPU hotplug code).
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1363812

Applied to ppc-for-2.7, thanks.

> 
> v2:
> - Add the "remove extra type variable" patch from Cédric Le Goater.
> - Remove the "Do not leak the memory of the type string" patch
>   since it got obsoleted by Cédric's patch
> 
> Cédric Le Goater (1):
>   spapr: remove extra type variable
> 
> Thomas Huth (4):
>   ppc: Introduce a function to look up CPU alias strings
>   hw/ppc/spapr: Look up CPU alias names instead of hard-coding the
>     aliases
>   ppc/kvm: Do not mess up the generic CPU family registration
>   ppc/kvm: Register also a generic spapr CPU core family type
> 
>  hw/ppc/spapr.c              | 15 ++++++---------
>  hw/ppc/spapr_cpu_core.c     | 38 +++++++++++++++++++++-----------------
>  target-ppc/cpu.h            |  1 +
>  target-ppc/kvm.c            | 19 +++++++++++--------
>  target-ppc/translate_init.c | 13 +++++++++++++
>  5 files changed, 52 insertions(+), 34 deletions(-)
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 3/5] hw/ppc/spapr: Look up CPU alias names instead of hard-coding the aliases

[Cédric Le Goater]

On 08/09/2016 06:59 PM, Thomas Huth wrote:
> Hard-coding the CPU alias names in the spapr_cores[] array has
> two big disadvantages:
> 
> 1) We register a real type with the CPU alias name in
>    spapr_cpu_core_register_types() - this prevents us from registering
>    a CPU family name in kvm_ppc_register_host_cpu_type() with the same
>    name (as we do it for the non-hotpluggable CPU types).
> 
> 2) It's quite cumbersome to maintain the aliases here in sync with the
>    ppc_cpu_aliases list from target-ppc/cpu-models.c.

yes. 

may be, in the future, we could go a little further and add a cpu 
'family' in cpu-models.c, listing the cpu models used under a sPAPR 
platform. 

I have been looking at that with the PowerNVCPUCore object and I don't 
think we need the spapr_cpu_core_*_initfn(), we should be able to do 
the same with a class_data.  

Thanks,

C.

> So let's simply add proper alias lookup to the spapr cpu core code,
> too (by checking whether the given model can be used directly, and
> if not by trying to look up the given model as an alias name instead).
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c          |  2 +-
>  hw/ppc/spapr_cpu_core.c | 38 +++++++++++++++++++++-----------------
>  2 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 399dcc0..0787c66 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1815,7 +1815,7 @@ static void ppc_spapr_init(MachineState *machine)
>      if (mc->query_hotpluggable_cpus) {
>          char *type = spapr_get_cpu_core_type(machine->cpu_model);
>  
> -        if (!object_class_by_name(type)) {
> +        if (type == NULL) {
>              error_report("Unable to find sPAPR CPU Core definition");
>              exit(1);
>          }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 716f7c4..bcb483d 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -93,6 +93,19 @@ char *spapr_get_cpu_core_type(const char *model)
>  
>      core_type = g_strdup_printf("%s-%s", model_pieces[0], TYPE_SPAPR_CPU_CORE);
>      g_strfreev(model_pieces);
> +
> +    /* Check whether it exists or whether we have to look up an alias name */
> +    if (!object_class_by_name(core_type)) {
> +        const char *realmodel;
> +
> +        g_free(core_type);
> +        realmodel = ppc_cpu_lookup_alias(model);
> +        if (realmodel) {
> +            return spapr_get_cpu_core_type(realmodel);
> +        }
> +        return NULL;
> +    }
> +
>      return core_type;
>  }
>  
> @@ -354,41 +367,32 @@ typedef struct SPAPRCoreInfo {
>  } SPAPRCoreInfo;
>  
>  static const SPAPRCoreInfo spapr_cores[] = {
> -    /* 970 and aliaes */
> +    /* 970 */
>      { .name = "970_v2.2", .initfn = spapr_cpu_core_970_initfn },
> -    { .name = "970", .initfn = spapr_cpu_core_970_initfn },
>  
> -    /* 970MP variants and aliases */
> +    /* 970MP variants */
>      { .name = "970MP_v1.0", .initfn = spapr_cpu_core_970MP_v10_initfn },
>      { .name = "970mp_v1.0", .initfn = spapr_cpu_core_970MP_v10_initfn },
>      { .name = "970MP_v1.1", .initfn = spapr_cpu_core_970MP_v11_initfn },
>      { .name = "970mp_v1.1", .initfn = spapr_cpu_core_970MP_v11_initfn },
> -    { .name = "970mp", .initfn = spapr_cpu_core_970MP_v11_initfn },
>  
> -    /* POWER5 and aliases */
> +    /* POWER5+ */
>      { .name = "POWER5+_v2.1", .initfn = spapr_cpu_core_POWER5plus_initfn },
> -    { .name = "POWER5+", .initfn = spapr_cpu_core_POWER5plus_initfn },
>  
> -    /* POWER7 and aliases */
> +    /* POWER7 */
>      { .name = "POWER7_v2.3", .initfn = spapr_cpu_core_POWER7_initfn },
> -    { .name = "POWER7", .initfn = spapr_cpu_core_POWER7_initfn },
>  
> -    /* POWER7+ and aliases */
> +    /* POWER7+ */
>      { .name = "POWER7+_v2.1", .initfn = spapr_cpu_core_POWER7plus_initfn },
> -    { .name = "POWER7+", .initfn = spapr_cpu_core_POWER7plus_initfn },
>  
> -    /* POWER8 and aliases */
> +    /* POWER8 */
>      { .name = "POWER8_v2.0", .initfn = spapr_cpu_core_POWER8_initfn },
> -    { .name = "POWER8", .initfn = spapr_cpu_core_POWER8_initfn },
> -    { .name = "power8", .initfn = spapr_cpu_core_POWER8_initfn },
>  
> -    /* POWER8E and aliases */
> +    /* POWER8E */
>      { .name = "POWER8E_v2.1", .initfn = spapr_cpu_core_POWER8E_initfn },
> -    { .name = "POWER8E", .initfn = spapr_cpu_core_POWER8E_initfn },
>  
> -    /* POWER8NVL and aliases */
> +    /* POWER8NVL */
>      { .name = "POWER8NVL_v1.0", .initfn = spapr_cpu_core_POWER8NVL_initfn },
> -    { .name = "POWER8NVL", .initfn = spapr_cpu_core_POWER8NVL_initfn },
>  
>      { .name = NULL }
>  };
>