| Message ID | 1465313980-31281-5-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 07, 2016 at 05:39:39PM +0200, Thomas Huth wrote: > When using an olderr PowerISA level, all the upper compatibility > bits have to be enabled, too. For example when we want to run > something in PowerISA 2.05 compatibility mode on POWER8, the bit > for 2.06 has to be set beside the bit for 2.05. > Additionally, to make sure that we do not set bits that are not > supported by the host, we apply a mask with the known-to-be-good > bits here, too. > > Signed-off-by: Thomas Huth <thuth@redhat.com> This one confused me a bit until I realised that, roughly speaking, bits in the PCR turn features off, rather than turning features on. Does that sound correct? > --- > target-ppc/translate_init.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index fa09183..ee2bc14 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -9519,24 +9519,29 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp) > { > int ret = 0; > CPUPPCState *env = &cpu->env; > + PowerPCCPUClass *host_pcc; > > cpu->cpu_version = cpu_version; > > switch (cpu_version) { > case CPU_POWERPC_LOGICAL_2_05: > - env->spr[SPR_PCR] = PCR_COMPAT_2_05; > + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_VSX_DIS | PCR_COMPAT_2_07 | > + PCR_COMPAT_2_06 | PCR_COMPAT_2_05; > break; > case CPU_POWERPC_LOGICAL_2_06: > - env->spr[SPR_PCR] = PCR_COMPAT_2_06; > - break; > case CPU_POWERPC_LOGICAL_2_06_PLUS: > - env->spr[SPR_PCR] = PCR_COMPAT_2_06; > + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_COMPAT_2_07 | PCR_COMPAT_2_06; > break; > default: > env->spr[SPR_PCR] = 0; > break; > } > > + host_pcc = kvm_ppc_get_host_cpu_class(); > + if (host_pcc) { > + env->spr[SPR_PCR] &= host_pcc->pcr_mask; > + } > + > if (kvm_enabled()) { > ret = kvmppc_set_compat(cpu, cpu->cpu_version); > if (ret < 0) {
On Tue, Jun 07, 2016 at 05:39:39PM +0200, Thomas Huth wrote: > When using an olderr PowerISA level, all the upper compatibility > bits have to be enabled, too. For example when we want to run > something in PowerISA 2.05 compatibility mode on POWER8, the bit > for 2.06 has to be set beside the bit for 2.05. > Additionally, to make sure that we do not set bits that are not > supported by the host, we apply a mask with the known-to-be-good > bits here, too. > > Signed-off-by: Thomas Huth <thuth@redhat.com> So, this breaks compile on 32-bit targets, because the spr values are only 32-bit there, and the PCR constants exceed that. But ppc_set_compat() is only actually used on 64-bit machines, so I've added a change to #if it out for 64-bit targets. > --- > target-ppc/translate_init.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index fa09183..ee2bc14 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -9519,24 +9519,29 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp) > { > int ret = 0; > CPUPPCState *env = &cpu->env; > + PowerPCCPUClass *host_pcc; > > cpu->cpu_version = cpu_version; > > switch (cpu_version) { > case CPU_POWERPC_LOGICAL_2_05: > - env->spr[SPR_PCR] = PCR_COMPAT_2_05; > + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_VSX_DIS | PCR_COMPAT_2_07 | > + PCR_COMPAT_2_06 | PCR_COMPAT_2_05; > break; > case CPU_POWERPC_LOGICAL_2_06: > - env->spr[SPR_PCR] = PCR_COMPAT_2_06; > - break; > case CPU_POWERPC_LOGICAL_2_06_PLUS: > - env->spr[SPR_PCR] = PCR_COMPAT_2_06; > + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_COMPAT_2_07 | PCR_COMPAT_2_06; > break; > default: > env->spr[SPR_PCR] = 0; > break; > } > > + host_pcc = kvm_ppc_get_host_cpu_class(); > + if (host_pcc) { > + env->spr[SPR_PCR] &= host_pcc->pcr_mask; > + } > + > if (kvm_enabled()) { > ret = kvmppc_set_compat(cpu, cpu->cpu_version); > if (ret < 0) {
On 08.06.2016 03:12, David Gibson wrote: > On Tue, Jun 07, 2016 at 05:39:39PM +0200, Thomas Huth wrote: >> When using an olderr PowerISA level, all the upper compatibility s/olderr/older/ :-/ >> bits have to be enabled, too. For example when we want to run >> something in PowerISA 2.05 compatibility mode on POWER8, the bit >> for 2.06 has to be set beside the bit for 2.05. >> Additionally, to make sure that we do not set bits that are not >> supported by the host, we apply a mask with the known-to-be-good >> bits here, too. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > This one confused me a bit until I realised that, roughly speaking, > bits in the PCR turn features off, rather than turning features on. > Does that sound correct? Right. Turning on the 2.06 bit means "disable all things that have been added additionally in 2.07", for example. So if you turn on bit 2.05, but not bit 2.06 (which the old code was doing), I guess you end up in a strange state where new instructions from PowerISA 2.07 can be used, but the instructions that have been added in ISA 2.06 are disabled. >> --- >> target-ppc/translate_init.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index fa09183..ee2bc14 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -9519,24 +9519,29 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp) >> { >> int ret = 0; >> CPUPPCState *env = &cpu->env; >> + PowerPCCPUClass *host_pcc; >> >> cpu->cpu_version = cpu_version; >> >> switch (cpu_version) { >> case CPU_POWERPC_LOGICAL_2_05: >> - env->spr[SPR_PCR] = PCR_COMPAT_2_05; >> + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_VSX_DIS | PCR_COMPAT_2_07 | >> + PCR_COMPAT_2_06 | PCR_COMPAT_2_05; >> break; >> case CPU_POWERPC_LOGICAL_2_06: >> - env->spr[SPR_PCR] = PCR_COMPAT_2_06; >> - break; >> case CPU_POWERPC_LOGICAL_2_06_PLUS: >> - env->spr[SPR_PCR] = PCR_COMPAT_2_06; >> + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_COMPAT_2_07 | PCR_COMPAT_2_06; >> break; >> default: >> env->spr[SPR_PCR] = 0; >> break; >> } >> >> + host_pcc = kvm_ppc_get_host_cpu_class(); >> + if (host_pcc) { >> + env->spr[SPR_PCR] &= host_pcc->pcr_mask; >> + } >> + >> if (kvm_enabled()) { >> ret = kvmppc_set_compat(cpu, cpu->cpu_version); >> if (ret < 0) { > Thomas
On 08.06.2016 07:44, David Gibson wrote: > On Tue, Jun 07, 2016 at 05:39:39PM +0200, Thomas Huth wrote: >> When using an olderr PowerISA level, all the upper compatibility >> bits have to be enabled, too. For example when we want to run >> something in PowerISA 2.05 compatibility mode on POWER8, the bit >> for 2.06 has to be set beside the bit for 2.05. >> Additionally, to make sure that we do not set bits that are not >> supported by the host, we apply a mask with the known-to-be-good >> bits here, too. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > So, this breaks compile on 32-bit targets, because the spr values are > only 32-bit there, and the PCR constants exceed that. But > ppc_set_compat() is only actually used on 64-bit machines, so I've > added a change to #if it out for 64-bit targets. D'oh, I explicitly compiled everything with a mingw32 cross-compiler to catch such issues ... but apparently it compiled without -Werror here, so I did not notice the warning :-( Thanks for the fixup! Thomas
On Wed, Jun 08, 2016 at 08:59:30AM +0200, Thomas Huth wrote: > On 08.06.2016 07:44, David Gibson wrote: > > On Tue, Jun 07, 2016 at 05:39:39PM +0200, Thomas Huth wrote: > >> When using an olderr PowerISA level, all the upper compatibility > >> bits have to be enabled, too. For example when we want to run > >> something in PowerISA 2.05 compatibility mode on POWER8, the bit > >> for 2.06 has to be set beside the bit for 2.05. > >> Additionally, to make sure that we do not set bits that are not > >> supported by the host, we apply a mask with the known-to-be-good > >> bits here, too. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > So, this breaks compile on 32-bit targets, because the spr values are > > only 32-bit there, and the PCR constants exceed that. But > > ppc_set_compat() is only actually used on 64-bit machines, so I've > > added a change to #if it out for 64-bit targets. > > D'oh, I explicitly compiled everything with a mingw32 cross-compiler to > catch such issues ... but apparently it compiled without -Werror here, > so I did not notice the warning :-( No, not 32-bit *host*, 32-bit *target*, so mingw32 wouldn't help. Configuring in ppc-softmmu and ppcemb-softmmu would.
On 08.06.2016 09:24, David Gibson wrote: > On Wed, Jun 08, 2016 at 08:59:30AM +0200, Thomas Huth wrote: >> On 08.06.2016 07:44, David Gibson wrote: >>> On Tue, Jun 07, 2016 at 05:39:39PM +0200, Thomas Huth wrote: >>>> When using an olderr PowerISA level, all the upper compatibility >>>> bits have to be enabled, too. For example when we want to run >>>> something in PowerISA 2.05 compatibility mode on POWER8, the bit >>>> for 2.06 has to be set beside the bit for 2.05. >>>> Additionally, to make sure that we do not set bits that are not >>>> supported by the host, we apply a mask with the known-to-be-good >>>> bits here, too. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> >>> So, this breaks compile on 32-bit targets, because the spr values are >>> only 32-bit there, and the PCR constants exceed that. But >>> ppc_set_compat() is only actually used on 64-bit machines, so I've >>> added a change to #if it out for 64-bit targets. >> >> D'oh, I explicitly compiled everything with a mingw32 cross-compiler to >> catch such issues ... but apparently it compiled without -Werror here, >> so I did not notice the warning :-( > > No, not 32-bit *host*, 32-bit *target*, so mingw32 wouldn't help. > Configuring in ppc-softmmu and ppcemb-softmmu would. Ok, right, ... I've now enabled these in my compile-test folder, too. Anyway, seems like mingw is compiled without -Werror by default, unlike the Linux builds ... I guess we could enable -Werror by default on mingw nowadays, too... Thomas
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index fa09183..ee2bc14 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -9519,24 +9519,29 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp) { int ret = 0; CPUPPCState *env = &cpu->env; + PowerPCCPUClass *host_pcc; cpu->cpu_version = cpu_version; switch (cpu_version) { case CPU_POWERPC_LOGICAL_2_05: - env->spr[SPR_PCR] = PCR_COMPAT_2_05; + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_VSX_DIS | PCR_COMPAT_2_07 | + PCR_COMPAT_2_06 | PCR_COMPAT_2_05; break; case CPU_POWERPC_LOGICAL_2_06: - env->spr[SPR_PCR] = PCR_COMPAT_2_06; - break; case CPU_POWERPC_LOGICAL_2_06_PLUS: - env->spr[SPR_PCR] = PCR_COMPAT_2_06; + env->spr[SPR_PCR] = PCR_TM_DIS | PCR_COMPAT_2_07 | PCR_COMPAT_2_06; break; default: env->spr[SPR_PCR] = 0; break; } + host_pcc = kvm_ppc_get_host_cpu_class(); + if (host_pcc) { + env->spr[SPR_PCR] &= host_pcc->pcr_mask; + } + if (kvm_enabled()) { ret = kvmppc_set_compat(cpu, cpu->cpu_version); if (ret < 0) {
When using an olderr PowerISA level, all the upper compatibility bits have to be enabled, too. For example when we want to run something in PowerISA 2.05 compatibility mode on POWER8, the bit for 2.06 has to be set beside the bit for 2.05. Additionally, to make sure that we do not set bits that are not supported by the host, we apply a mask with the known-to-be-good bits here, too. Signed-off-by: Thomas Huth <thuth@redhat.com> --- target-ppc/translate_init.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)