[Qemu-devel] [PATCH v4 0/2] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Qemu-devel] [PATCH v4 0/2] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

This patch series allows to use "-cpu POWER9" with a POWER9
DD1 host and KVM HV. With TCG, "-cpu POWER9" selects
POWER9_v2.0 (POWER9 DD2).

David, I send this series based on master, if you need a version
based on your ppc-for-2.10 branch let me know.

v4:
  update POWER9 comments
  move workaround to init_proc_POWER9()

v3:
  add the patch to move POWER9 DD1 workaround to
  the generic part (not only for "-cpu host").

v2:
  add POWER9_v2.0
  sent as RFC as "-cpu POWER9" breaks the kernel

Laurent Vivier (2):
  target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1
  target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

 hw/ppc/spapr_cpu_core.c     |  1 +
 target/ppc/cpu-models.c     |  6 ++++--
 target/ppc/cpu-models.h     |  1 +
 target/ppc/kvm.c            | 11 -----------
 target/ppc/translate_init.c | 15 ++++++++++++++-
 5 files changed, 20 insertions(+), 14 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v4 1/2] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.

When we run qemu on a POWER9 DD1 host, we must use either
"-cpu host" or "-cpu POWER9", but in the latter case it fails with

    Unable to find sPAPR CPU Core definition

because POWER9 DD1 doesn't appear in the list of known CPUs.

This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
PVR instead of CPU_POWERPC_POWER9_BASE. It also add POWER_v2.0
with POWER9 DD2 PVR to avoid to trigger kernel POWER9 DD1 workaround
in TCG mode.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c | 1 +
 target/ppc/cpu-models.c | 6 ++++--
 target/ppc/cpu-models.h | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ea278ce..b5ecc7c 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -293,6 +293,7 @@ static const char *spapr_core_models[] = {
 
     /* POWER9 */
     "POWER9_v1.0",
+    "POWER9_v2.0",
 };
 
 static Property spapr_cpu_core_properties[] = {
diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
index 4d3e635..ac7e299 100644
--- a/target/ppc/cpu-models.c
+++ b/target/ppc/cpu-models.c
@@ -1144,8 +1144,10 @@
     POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
                 "PowerPC 970 v2.2")
 
-    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
+    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
                 "POWER9 v1.0")
+    POWERPC_DEF("POWER9_v2.0",   CPU_POWERPC_POWER9_DD2,             POWER9,
+                "POWER9 v2.0")
 
     POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
                 "PowerPC 970FX v1.0 (G5)")
@@ -1391,7 +1393,7 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
     { "POWER8E", "POWER8E_v2.1" },
     { "POWER8", "POWER8_v2.0" },
     { "POWER8NVL", "POWER8NVL_v1.0" },
-    { "POWER9", "POWER9_v1.0" },
+    { "POWER9", "POWER9_v2.0" },
     { "970", "970_v2.2" },
     { "970fx", "970fx_v3.1" },
     { "970mp", "970mp_v1.1" },
diff --git a/target/ppc/cpu-models.h b/target/ppc/cpu-models.h
index b563c45..c10745e 100644
--- a/target/ppc/cpu-models.h
+++ b/target/ppc/cpu-models.h
@@ -562,6 +562,7 @@ enum {
     CPU_POWERPC_POWER8NVL_v10      = 0x004C0100,
     CPU_POWERPC_POWER9_BASE        = 0x004E0000,
     CPU_POWERPC_POWER9_DD1         = 0x004E0100,
+    CPU_POWERPC_POWER9_DD2         = 0x004E1200, /* Scale out 24 cores */
     CPU_POWERPC_970_v22            = 0x00390202,
     CPU_POWERPC_970FX_v10          = 0x00391100,
     CPU_POWERPC_970FX_v20          = 0x003C0200,
-- 
2.9.4

[Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Laurent Vivier]

Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
disables compatibility mode for POWER9 DD1 to allow to
boot on POWER9 DD1 host with KVM.

As the workaround has been added in kvmppc_host_cpu_class_init(),
it applies only on CPU created with "-cpu host".
As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
host, this patch moves the workaround from kvmppc_host_cpu_class_init()
to init_proc_POWER9().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 target/ppc/kvm.c            | 11 -----------
 target/ppc/translate_init.c | 15 ++++++++++++++-
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index f2f7c53..9d76817 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2381,17 +2381,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 
 #if defined(TARGET_PPC64)
     pcc->radix_page_info = kvm_get_radix_page_info();
-
-    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
-        /*
-         * POWER9 DD1 has some bugs which make it not really ISA 3.00
-         * compliant.  More importantly, advertising ISA 3.00
-         * architected mode may prevent guests from activating
-         * necessary DD1 workarounds.
-         */
-        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
-                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
-    }
 #endif /* defined(TARGET_PPC64) */
 }
 
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 783bf98..a41fe66 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8811,6 +8811,8 @@ static struct ppc_radix_page_info POWER9_radix_page_info = {
 
 static void init_proc_POWER9(CPUPPCState *env)
 {
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     /* Common Registers */
     init_proc_book3s_common(env);
     gen_spr_book3s_207_dbg(env);
@@ -8848,7 +8850,18 @@ static void init_proc_POWER9(CPUPPCState *env)
 
     /* Allocate hardware IRQ controller */
     init_excp_POWER8(env);
-    ppcPOWER7_irq_init(ppc_env_get_cpu(env));
+    ppcPOWER7_irq_init(cpu);
+
+    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
+        /*
+         * POWER9 DD1 has some bugs which make it not really ISA 3.00
+         * compliant.  More importantly, advertising ISA 3.00
+         * architected mode may prevent guests from activating
+         * necessary DD1 workarounds.
+         */
+        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
+                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
+    }
 }
 
 static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)
-- 
2.9.4

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Thomas Huth]

On 04.07.2017 13:01, Laurent Vivier wrote:
> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
> disables compatibility mode for POWER9 DD1 to allow to
> boot on POWER9 DD1 host with KVM.
> 
> As the workaround has been added in kvmppc_host_cpu_class_init(),
> it applies only on CPU created with "-cpu host".
> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
> to init_proc_POWER9().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  target/ppc/kvm.c            | 11 -----------
>  target/ppc/translate_init.c | 15 ++++++++++++++-
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index f2f7c53..9d76817 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2381,17 +2381,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>  
>  #if defined(TARGET_PPC64)
>      pcc->radix_page_info = kvm_get_radix_page_info();
> -
> -    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> -        /*
> -         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> -         * compliant.  More importantly, advertising ISA 3.00
> -         * architected mode may prevent guests from activating
> -         * necessary DD1 workarounds.
> -         */
> -        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> -                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> -    }
>  #endif /* defined(TARGET_PPC64) */
>  }
>  
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 783bf98..a41fe66 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8811,6 +8811,8 @@ static struct ppc_radix_page_info POWER9_radix_page_info = {
>  
>  static void init_proc_POWER9(CPUPPCState *env)
>  {
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      /* Common Registers */
>      init_proc_book3s_common(env);
>      gen_spr_book3s_207_dbg(env);
> @@ -8848,7 +8850,18 @@ static void init_proc_POWER9(CPUPPCState *env)
>  
>      /* Allocate hardware IRQ controller */
>      init_excp_POWER8(env);
> -    ppcPOWER7_irq_init(ppc_env_get_cpu(env));
> +    ppcPOWER7_irq_init(cpu);
> +
> +    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> +        /*
> +         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> +         * compliant.  More importantly, advertising ISA 3.00
> +         * architected mode may prevent guests from activating
> +         * necessary DD1 workarounds.
> +         */
> +        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> +                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> +    }
>  }
>  
>  static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Greg Kurz]

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

On Tue,  4 Jul 2017 13:01:26 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
> disables compatibility mode for POWER9 DD1 to allow to
> boot on POWER9 DD1 host with KVM.
> 
> As the workaround has been added in kvmppc_host_cpu_class_init(),
> it applies only on CPU created with "-cpu host".
> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
> to init_proc_POWER9().
> 

As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
class attribute...

What about moving the workaround to ppc_POWER9_cpu_family_class_init()
instead ? This would just require to expose mfpvr() in some header.

--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8913,8 +8913,10 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
     dc->props = powerpc_servercpu_properties;
     pcc->pvr_match = ppc_pvr_match_power9;
     pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06 | PCR_COMPAT_2_07;
-    pcc->pcr_supported = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
-                         PCR_COMPAT_2_05;
+    if (!kvm_enabled() || (mfpvr() & 0xffffff00) != CPU_POWERPC_POWER9_DD1) {
+        pcc->pcr_supported = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 |
+                             PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
+    }
     pcc->init_proc = init_proc_POWER9;
     pcc->check_pow = check_pow_nocheck;
     cc->has_work = cpu_has_work_POWER9;

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  target/ppc/kvm.c            | 11 -----------
>  target/ppc/translate_init.c | 15 ++++++++++++++-
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index f2f7c53..9d76817 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2381,17 +2381,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>  
>  #if defined(TARGET_PPC64)
>      pcc->radix_page_info = kvm_get_radix_page_info();
> -
> -    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> -        /*
> -         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> -         * compliant.  More importantly, advertising ISA 3.00
> -         * architected mode may prevent guests from activating
> -         * necessary DD1 workarounds.
> -         */
> -        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> -                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> -    }
>  #endif /* defined(TARGET_PPC64) */
>  }
>  
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 783bf98..a41fe66 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8811,6 +8811,8 @@ static struct ppc_radix_page_info POWER9_radix_page_info = {
>  
>  static void init_proc_POWER9(CPUPPCState *env)
>  {
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      /* Common Registers */
>      init_proc_book3s_common(env);
>      gen_spr_book3s_207_dbg(env);
> @@ -8848,7 +8850,18 @@ static void init_proc_POWER9(CPUPPCState *env)
>  
>      /* Allocate hardware IRQ controller */
>      init_excp_POWER8(env);
> -    ppcPOWER7_irq_init(ppc_env_get_cpu(env));
> +    ppcPOWER7_irq_init(cpu);
> +
> +    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> +        /*
> +         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> +         * compliant.  More importantly, advertising ISA 3.00
> +         * architected mode may prevent guests from activating
> +         * necessary DD1 workarounds.
> +         */
> +        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> +                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> +    }
>  }
>  
>  static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Laurent Vivier]

On 04/07/2017 13:15, Greg Kurz wrote:
> On Tue,  4 Jul 2017 13:01:26 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
>> disables compatibility mode for POWER9 DD1 to allow to
>> boot on POWER9 DD1 host with KVM.
>>
>> As the workaround has been added in kvmppc_host_cpu_class_init(),
>> it applies only on CPU created with "-cpu host".
>> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
>> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
>> to init_proc_POWER9().
>>
> 
> As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
> called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
> class attribute...
> 
> What about moving the workaround to ppc_POWER9_cpu_family_class_init()
> instead ? This would just require to expose mfpvr() in some header.

I think I have already tried something like that, and I'm not sure the
PVR is already set at this level of the initialization. But I'm going to
try your patch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Laurent Vivier]

On 04/07/2017 13:21, Laurent Vivier wrote:
> On 04/07/2017 13:15, Greg Kurz wrote:
>> On Tue,  4 Jul 2017 13:01:26 +0200
>> Laurent Vivier <lvivier@redhat.com> wrote:
>>
>>> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
>>> disables compatibility mode for POWER9 DD1 to allow to
>>> boot on POWER9 DD1 host with KVM.
>>>
>>> As the workaround has been added in kvmppc_host_cpu_class_init(),
>>> it applies only on CPU created with "-cpu host".
>>> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
>>> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
>>> to init_proc_POWER9().
>>>
>>
>> As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
>> called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
>> class attribute...
>>
>> What about moving the workaround to ppc_POWER9_cpu_family_class_init()
>> instead ? This would just require to expose mfpvr() in some header.
> 
> I think I have already tried something like that, and I'm not sure the
> PVR is already set at this level of the initialization. But I'm going to
> try your patch.

oh, you check the host PVR, not the guest PVR.

Well, I'm not sure it's a good idea to modify the guest CPU property
according to the host CPU version. I let David to decide what is the
best solution here...

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[David Gibson]

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

On Tue, Jul 04, 2017 at 01:15:00PM +0200, Greg Kurz wrote:
> On Tue,  4 Jul 2017 13:01:26 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
> > disables compatibility mode for POWER9 DD1 to allow to
> > boot on POWER9 DD1 host with KVM.
> > 
> > As the workaround has been added in kvmppc_host_cpu_class_init(),
> > it applies only on CPU created with "-cpu host".
> > As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
> > host, this patch moves the workaround from kvmppc_host_cpu_class_init()
> > to init_proc_POWER9().
> > 
> 
> As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
> called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
> class attribute...

Ah.. yeah.. I didn't notice that before.  That's definitely not right.

> What about moving the workaround to ppc_POWER9_cpu_family_class_init()
> instead ? This would just require to expose mfpvr() in some header.

Yeah, as someone else pointed out using the host PVR is also
definitely not right (unless you're in a function specifically
connected to the host cpu class).

> 
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8913,8 +8913,10 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>      dc->props = powerpc_servercpu_properties;
>      pcc->pvr_match = ppc_pvr_match_power9;
>      pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06 | PCR_COMPAT_2_07;
> -    pcc->pcr_supported = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> -                         PCR_COMPAT_2_05;
> +    if (!kvm_enabled() || (mfpvr() & 0xffffff00) != CPU_POWERPC_POWER9_DD1) {
> +        pcc->pcr_supported = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 |
> +                             PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
> +    }
>      pcc->init_proc = init_proc_POWER9;
>      pcc->check_pow = check_pow_nocheck;
>      cc->has_work = cpu_has_work_POWER9;
> 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  target/ppc/kvm.c            | 11 -----------
> >  target/ppc/translate_init.c | 15 ++++++++++++++-
> >  2 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index f2f7c53..9d76817 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2381,17 +2381,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> >  
> >  #if defined(TARGET_PPC64)
> >      pcc->radix_page_info = kvm_get_radix_page_info();
> > -
> > -    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> > -        /*
> > -         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> > -         * compliant.  More importantly, advertising ISA 3.00
> > -         * architected mode may prevent guests from activating
> > -         * necessary DD1 workarounds.
> > -         */
> > -        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> > -                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> > -    }
> >  #endif /* defined(TARGET_PPC64) */
> >  }
> >  
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 783bf98..a41fe66 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8811,6 +8811,8 @@ static struct ppc_radix_page_info POWER9_radix_page_info = {
> >  
> >  static void init_proc_POWER9(CPUPPCState *env)
> >  {
> > +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >      /* Common Registers */
> >      init_proc_book3s_common(env);
> >      gen_spr_book3s_207_dbg(env);
> > @@ -8848,7 +8850,18 @@ static void init_proc_POWER9(CPUPPCState *env)
> >  
> >      /* Allocate hardware IRQ controller */
> >      init_excp_POWER8(env);
> > -    ppcPOWER7_irq_init(ppc_env_get_cpu(env));
> > +    ppcPOWER7_irq_init(cpu);
> > +
> > +    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> > +        /*
> > +         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> > +         * compliant.  More importantly, advertising ISA 3.00
> > +         * architected mode may prevent guests from activating
> > +         * necessary DD1 workarounds.
> > +         */
> > +        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> > +                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> > +    }
> >  }
> >  
> >  static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)
> 



-- 
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 v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Greg Kurz]

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

On Tue, 4 Jul 2017 21:41:51 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jul 04, 2017 at 01:15:00PM +0200, Greg Kurz wrote:
> > On Tue,  4 Jul 2017 13:01:26 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> > > Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
> > > disables compatibility mode for POWER9 DD1 to allow to
> > > boot on POWER9 DD1 host with KVM.
> > > 
> > > As the workaround has been added in kvmppc_host_cpu_class_init(),
> > > it applies only on CPU created with "-cpu host".
> > > As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
> > > host, this patch moves the workaround from kvmppc_host_cpu_class_init()
> > > to init_proc_POWER9().
> > >   
> > 
> > As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
> > called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
> > class attribute...  
> 
> Ah.. yeah.. I didn't notice that before.  That's definitely not right.
> 
> > What about moving the workaround to ppc_POWER9_cpu_family_class_init()
> > instead ? This would just require to expose mfpvr() in some header.  
> 
> Yeah, as someone else pointed out using the host PVR is also
> definitely not right (unless you're in a function specifically
> connected to the host cpu class).
> 

I agree but the root issue is that we accept to pass -cpu POWER9 instead of
-cpu host with -enable-kvm. And the host cpu class isn't involved in this
case.

> > 
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8913,8 +8913,10 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
> >      dc->props = powerpc_servercpu_properties;
> >      pcc->pvr_match = ppc_pvr_match_power9;
> >      pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06 | PCR_COMPAT_2_07;
> > -    pcc->pcr_supported = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> > -                         PCR_COMPAT_2_05;
> > +    if (!kvm_enabled() || (mfpvr() & 0xffffff00) != CPU_POWERPC_POWER9_DD1) {
> > +        pcc->pcr_supported = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 |
> > +                             PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
> > +    }
> >      pcc->init_proc = init_proc_POWER9;
> >      pcc->check_pow = check_pow_nocheck;
> >      cc->has_work = cpu_has_work_POWER9;
> >   
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >  target/ppc/kvm.c            | 11 -----------
> > >  target/ppc/translate_init.c | 15 ++++++++++++++-
> > >  2 files changed, 14 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > > index f2f7c53..9d76817 100644
> > > --- a/target/ppc/kvm.c
> > > +++ b/target/ppc/kvm.c
> > > @@ -2381,17 +2381,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> > >  
> > >  #if defined(TARGET_PPC64)
> > >      pcc->radix_page_info = kvm_get_radix_page_info();
> > > -
> > > -    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> > > -        /*
> > > -         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> > > -         * compliant.  More importantly, advertising ISA 3.00
> > > -         * architected mode may prevent guests from activating
> > > -         * necessary DD1 workarounds.
> > > -         */
> > > -        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> > > -                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> > > -    }
> > >  #endif /* defined(TARGET_PPC64) */
> > >  }
> > >  
> > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > > index 783bf98..a41fe66 100644
> > > --- a/target/ppc/translate_init.c
> > > +++ b/target/ppc/translate_init.c
> > > @@ -8811,6 +8811,8 @@ static struct ppc_radix_page_info POWER9_radix_page_info = {
> > >  
> > >  static void init_proc_POWER9(CPUPPCState *env)
> > >  {
> > > +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > >      /* Common Registers */
> > >      init_proc_book3s_common(env);
> > >      gen_spr_book3s_207_dbg(env);
> > > @@ -8848,7 +8850,18 @@ static void init_proc_POWER9(CPUPPCState *env)
> > >  
> > >      /* Allocate hardware IRQ controller */
> > >      init_excp_POWER8(env);
> > > -    ppcPOWER7_irq_init(ppc_env_get_cpu(env));
> > > +    ppcPOWER7_irq_init(cpu);
> > > +
> > > +    if ((pcc->pvr & 0xffffff00) == CPU_POWERPC_POWER9_DD1) {
> > > +        /*
> > > +         * POWER9 DD1 has some bugs which make it not really ISA 3.00
> > > +         * compliant.  More importantly, advertising ISA 3.00
> > > +         * architected mode may prevent guests from activating
> > > +         * necessary DD1 workarounds.
> > > +         */
> > > +        pcc->pcr_supported &= ~(PCR_COMPAT_3_00 | PCR_COMPAT_2_07
> > > +                                | PCR_COMPAT_2_06 | PCR_COMPAT_2_05);
> > > +    }
> > >  }
> > >  
> > >  static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)  
> >   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Thomas Huth]

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

On 04.07.2017 15:02, Greg Kurz wrote:
> On Tue, 4 Jul 2017 21:41:51 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Tue, Jul 04, 2017 at 01:15:00PM +0200, Greg Kurz wrote:
>>> On Tue,  4 Jul 2017 13:01:26 +0200
>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>   
>>>> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
>>>> disables compatibility mode for POWER9 DD1 to allow to
>>>> boot on POWER9 DD1 host with KVM.
>>>>
>>>> As the workaround has been added in kvmppc_host_cpu_class_init(),
>>>> it applies only on CPU created with "-cpu host".
>>>> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
>>>> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
>>>> to init_proc_POWER9().
>>>>   
>>>
>>> As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
>>> called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
>>> class attribute...  
>>
>> Ah.. yeah.. I didn't notice that before.  That's definitely not right.

It's a hack for DD1 which will likely be removed soon again anyway (as
soon as the DD1 hardware is not in use anymore), so I think it's ok for
such a temporary solution?

>>> What about moving the workaround to ppc_POWER9_cpu_family_class_init()
>>> instead ? This would just require to expose mfpvr() in some header.  
>>
>> Yeah, as someone else pointed out using the host PVR is also
>> definitely not right (unless you're in a function specifically
>> connected to the host cpu class).
>>
> 
> I agree but the root issue is that we accept to pass -cpu POWER9 instead of
> -cpu host with -enable-kvm. And the host cpu class isn't involved in this
> case.

Allowing -cpu POWERx with -enable-kvm was likely a bad idea, but we've
agree with libvirt that we support this, so we can't revert this feature
so easily without changing libvirt first.

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[David Gibson]

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

On Tue, Jul 04, 2017 at 03:02:39PM +0200, Greg Kurz wrote:
> On Tue, 4 Jul 2017 21:41:51 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jul 04, 2017 at 01:15:00PM +0200, Greg Kurz wrote:
> > > On Tue,  4 Jul 2017 13:01:26 +0200
> > > Laurent Vivier <lvivier@redhat.com> wrote:
> > >   
> > > > Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
> > > > disables compatibility mode for POWER9 DD1 to allow to
> > > > boot on POWER9 DD1 host with KVM.
> > > > 
> > > > As the workaround has been added in kvmppc_host_cpu_class_init(),
> > > > it applies only on CPU created with "-cpu host".
> > > > As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
> > > > host, this patch moves the workaround from kvmppc_host_cpu_class_init()
> > > > to init_proc_POWER9().
> > > >   
> > > 
> > > As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
> > > called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
> > > class attribute...  
> > 
> > Ah.. yeah.. I didn't notice that before.  That's definitely not right.
> > 
> > > What about moving the workaround to ppc_POWER9_cpu_family_class_init()
> > > instead ? This would just require to expose mfpvr() in some header.  
> > 
> > Yeah, as someone else pointed out using the host PVR is also
> > definitely not right (unless you're in a function specifically
> > connected to the host cpu class).
> > 
> 
> I agree but the root issue is that we accept to pass -cpu POWER9 instead of
> -cpu host with -enable-kvm. And the host cpu class isn't involved in this
> case.

Well.. it sort of is.  I believe the way we make this work (since
Thomas' cleanup) is that when KVM is active, we alter the alias for
the host cpu's family to point to the host cpu class, instead of
whatever specific version it usually points to.

-- 
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 v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Thomas Huth]

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

On 05.07.2017 08:36, David Gibson wrote:
> On Tue, Jul 04, 2017 at 03:02:39PM +0200, Greg Kurz wrote:
>> On Tue, 4 Jul 2017 21:41:51 +1000
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> On Tue, Jul 04, 2017 at 01:15:00PM +0200, Greg Kurz wrote:
>>>> On Tue,  4 Jul 2017 13:01:26 +0200
>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>   
>>>>> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
>>>>> disables compatibility mode for POWER9 DD1 to allow to
>>>>> boot on POWER9 DD1 host with KVM.
>>>>>
>>>>> As the workaround has been added in kvmppc_host_cpu_class_init(),
>>>>> it applies only on CPU created with "-cpu host".
>>>>> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
>>>>> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
>>>>> to init_proc_POWER9().
>>>>>   
>>>>
>>>> As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
>>>> called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
>>>> class attribute...  
>>>
>>> Ah.. yeah.. I didn't notice that before.  That's definitely not right.
>>>
>>>> What about moving the workaround to ppc_POWER9_cpu_family_class_init()
>>>> instead ? This would just require to expose mfpvr() in some header.  
>>>
>>> Yeah, as someone else pointed out using the host PVR is also
>>> definitely not right (unless you're in a function specifically
>>> connected to the host cpu class).
>>>
>>
>> I agree but the root issue is that we accept to pass -cpu POWER9 instead of
>> -cpu host with -enable-kvm. And the host cpu class isn't involved in this
>> case.
> 
> Well.. it sort of is.  I believe the way we make this work (since
> Thomas' cleanup) is that when KVM is active, we alter the alias for
> the host cpu's family to point to the host cpu class, instead of
> whatever specific version it usually points to.

Right, it's the code at the and of the kvm_ppc_register_host_cpu_type()
function. Maybe that function could also be a good spot to move the DD1
workaround into (just a quick idea, I haven't checked whether it's
feasible)?

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v4 2/2] target/ppc: move POWER9 DD1 workaround to init_proc_POWER9()

[Greg Kurz]

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

On Wed, 5 Jul 2017 09:56:15 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 05.07.2017 08:36, David Gibson wrote:
> > On Tue, Jul 04, 2017 at 03:02:39PM +0200, Greg Kurz wrote:  
> >> On Tue, 4 Jul 2017 21:41:51 +1000
> >> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>  
> >>> On Tue, Jul 04, 2017 at 01:15:00PM +0200, Greg Kurz wrote:  
> >>>> On Tue,  4 Jul 2017 13:01:26 +0200
> >>>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>>     
> >>>>> Commit 5f3066d ("target/ppc: Allow workarounds for POWER9 DD1")
> >>>>> disables compatibility mode for POWER9 DD1 to allow to
> >>>>> boot on POWER9 DD1 host with KVM.
> >>>>>
> >>>>> As the workaround has been added in kvmppc_host_cpu_class_init(),
> >>>>> it applies only on CPU created with "-cpu host".
> >>>>> As we want to be able to use also "-cpu POWER9" on a POWER9 DD1
> >>>>> host, this patch moves the workaround from kvmppc_host_cpu_class_init()
> >>>>> to init_proc_POWER9().
> >>>>>     
> >>>>
> >>>> As with ppc_cpu_initfn() in your previous version, init_proc_POWER9() is
> >>>> called for every CPU instance.. ie, all CPU will adjust the @pcr_supported
> >>>> class attribute...    
> >>>
> >>> Ah.. yeah.. I didn't notice that before.  That's definitely not right.
> >>>  
> >>>> What about moving the workaround to ppc_POWER9_cpu_family_class_init()
> >>>> instead ? This would just require to expose mfpvr() in some header.    
> >>>
> >>> Yeah, as someone else pointed out using the host PVR is also
> >>> definitely not right (unless you're in a function specifically
> >>> connected to the host cpu class).
> >>>  
> >>
> >> I agree but the root issue is that we accept to pass -cpu POWER9 instead of
> >> -cpu host with -enable-kvm. And the host cpu class isn't involved in this
> >> case.  
> > 
> > Well.. it sort of is.  I believe the way we make this work (since
> > Thomas' cleanup) is that when KVM is active, we alter the alias for
> > the host cpu's family to point to the host cpu class, instead of
> > whatever specific version it usually points to.  
> 

Yeah, I saw that and I guess there's a problem: we alter the alias to
point to the class with the same PVR as the host CPU, not to the
host CPU class itself. The CPUs don't belong to TYPE_HOST_POWERPC_CPU
and aren't configured according to kvmppc_host_cpu_class_init().

> Right, it's the code at the and of the kvm_ppc_register_host_cpu_type()
> function. Maybe that function could also be a good spot to move the DD1
> workaround into (just a quick idea, I haven't checked whether it's
> feasible)?
> 

I have a patch to change the alias to point to TYPE_HOST_POWERPC_CPU. This
allows to use "-cpu POWER9" with KVM on a DD1 host.

Cheers,

--
Greg

>  Thomas
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]