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

[Qemu-devel] [PATCH for-2.7 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

Thomas Huth (5):
  ppc: Introduce a function to look up CPU alias strings
  hw/ppc/spapr: Look up CPU alias names instead of hard-coding the
    aliases
  hw/ppc/spapr: Do not leak the memory of the type string
  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              |  3 ++-
 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, 48 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/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 2/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 57564e5..ec65b07 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1829,7 +1829,7 @@ static void ppc_spapr_init(MachineState *machine)
                 char *type = spapr_get_cpu_core_type(machine->cpu_model);
                 Object *core;
 
-                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 3/5] hw/ppc/spapr: Do not leak the memory of the type string

[Thomas Huth]

The type string is allocated in spapr_get_cpu_core_type()
with g_strdup_printf(), so we should free this memory when we're
done with it.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ec65b07..424b506 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1840,6 +1840,7 @@ static void ppc_spapr_init(MachineState *machine)
                 object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
                                         &error_fatal);
                 object_property_set_bool(core, true, "realized", &error_fatal);
+                g_free(type);
             }
         }
         g_free(type);
-- 
1.8.3.1

[Qemu-devel] [PATCH 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 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 for-2.7 0/5] spapr: Fix regression in CPU alias handling

[Andrea Bolognani]

On Tue, 2016-08-09 at 11:17 +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
> 
> Thomas Huth (5):
>   ppc: Introduce a function to look up CPU alias strings
>   hw/ppc/spapr: Look up CPU alias names instead of hard-coding the
>     aliases
>   hw/ppc/spapr: Do not leak the memory of the type string
>   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              |  3 ++-
>  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, 48 insertions(+), 26 deletions(-)

Tested-By: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Bharata B Rao]

On Tue, Aug 09, 2016 at 11:17:04AM +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.

I believed that "-cpu POWER8" on POWER8E host was broken in a way as
the guest CPUs were getting reported as POWER8E instead of POWER8

(/proc/cpuinfo of guest)
cpu		: POWER8E (raw), altivec supported

I thought, the correct configuration should have been

cpu             : POWER8 (architected), altivec supported

which is what you get when you use POWER8 in compat mode on
POWER8E host like below:

-cpu host -global driver=host-powerpc64-cpu,property=compat,value=power8

However as you note libvirt is dependent on supporting POWER8 and
there have been discussions and conclusions on this earlier, I guess
it is better now to have your patchset to restore the expectations of
libvirt.

Regards,
Bharata.

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

[Thomas Huth]

On 09.08.2016 11:17, 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
> 
> Thomas Huth (5):
>   ppc: Introduce a function to look up CPU alias strings
>   hw/ppc/spapr: Look up CPU alias names instead of hard-coding the
>     aliases
>   hw/ppc/spapr: Do not leak the memory of the type string
>   ppc/kvm: Do not mess up the generic CPU family registration
>   ppc/kvm: Register also a generic spapr CPU core family type

I noticed that Cédric's "spar: remove extra type variable" conflicts
with the second patch of this series. Since his patch also obsoletes the
third patch in my series here (the memory leak does then not occur
anymore), I think I'll best send a v2 where I include his patch. Will do
that ASAP...

 Thomas

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

[Thomas Huth]

On 09.08.2016 17:39, Bharata B Rao wrote:
> On Tue, Aug 09, 2016 at 11:17:04AM +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.
> 
> I believed that "-cpu POWER8" on POWER8E host was broken in a way as
> the guest CPUs were getting reported as POWER8E instead of POWER8
> 
> (/proc/cpuinfo of guest)
> cpu		: POWER8E (raw), altivec supported
> 
> I thought, the correct configuration should have been
> 
> cpu             : POWER8 (architected), altivec supported
> 
> which is what you get when you use POWER8 in compat mode on
> POWER8E host like below:
> 
> -cpu host -global driver=host-powerpc64-cpu,property=compat,value=power8

As far as I've understood the (old) QEMU source code and the discussions
in the past, the "-cpu POWER8" was rather meant as some kind of alias
for any CPU in the POWER8 family, i.e. also for POWER8E CPUs. It's just
a little bit ugly that "-cpu ?" always lists this as an alias for
POWER8_v2.0 - it would be better if we'd somehow update it when we set
the POWER8 alias on KVM.

> However as you note libvirt is dependent on supporting POWER8 and
> there have been discussions and conclusions on this earlier, I guess
> it is better now to have your patchset to restore the expectations of
> libvirt.

Yes, I think we should keep the old behavior now to avoid to break
things ... we maybe might want to reconsider the behavior for future
CPUs (POWER9?) though.

 Thomas

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

[Andrea Bolognani]

On Tue, 2016-08-09 at 18:22 +0200, Thomas Huth wrote:
> > I believed that "-cpu POWER8" on POWER8E host was broken in a way as
> > the guest CPUs were getting reported as POWER8E instead of POWER8
> > 
> > (/proc/cpuinfo of guest)
> > cpu		: POWER8E (raw), altivec supported
> > 
> > I thought, the correct configuration should have been
> > 
> > cpu             : POWER8 (architected), altivec supported
> > 
> > which is what you get when you use POWER8 in compat mode on
> > POWER8E host like below:
> > 
> > -cpu host -global driver=host-powerpc64-cpu,property=compat,value=power8
> 
> As far as I've understood the (old) QEMU source code and the discussions
> in the past, the "-cpu POWER8" was rather meant as some kind of alias
> for any CPU in the POWER8 family, i.e. also for POWER8E CPUs.

Indeed. See

  https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg01993.html

and in particular

  https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg02007.html

for the rationale.

> It's just
> a little bit ugly that "-cpu ?" always lists this as an alias for
> POWER8_v2.0 - it would be better if we'd somehow update it when we set
> the POWER8 alias on KVM.

AFAIU that won't be possible, because QEMU doesn't know (and
doesn't want to know) about every single existing POWER CPU
ever built. But agreed, it's kinda ugly.

> > However as you note libvirt is dependent on supporting POWER8 and
> > there have been discussions and conclusions on this earlier, I guess
> > it is better now to have your patchset to restore the expectations of
> > libvirt.
> 
> Yes, I think we should keep the old behavior now to avoid to break
> things ... we maybe might want to reconsider the behavior for future
> CPUs (POWER9?) though.

If such a change is ever implemented, hopefully it will be
possible to keep the old behavior around not only for
POWER[6-8] but also for POWER9, while providing the new
behavior as an alternative.

In any case, when this or similar incompatible changes are
discussed, please try to keep libvirt folks in the loop. In
praticular, feel absolutely free to CC: me any time you feel
the discussion might be relevant :)

-- 
Andrea Bolognani / Red Hat / Virtualization