| Message ID | 20170704131500.3ef448c8@bahia.lan |
|---|---|
| State | New |
| Headers | show |
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
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
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) >
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) > > > > >
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
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.
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
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 >
--- 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;