| Message ID | fe96334e261ed8da1c03202ea88a139b479c0c23.1490763676.git.sam.bobroff@au1.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 29, 2017 at 04:01:28PM +1100, Sam Bobroff wrote: > On Power8 hosts it is currently theoretically possible for QEMU/KVM-HV guests > to receive a ibm,pa-features property indicating that HTM support is available > when it is not. The situation would occur if the platform firmware of > a Power8 host cleared the HTM bit of the ibm,pa-features property. > QEMU would query KVM for the availability of HTM, which will return no > support, but workaround code in kvm_arch_init_vcpu() would then > re-enable it because KVM_HV is in use and the processor is P8. > > This patch adjusts the workaround in kvm_arch_init_vcpu() so that it does not > enable HTM (in the above case) unless the host kernel indicates to the QEMU > process, via the auxiliary vector, that userspace can use HTM (via the HWCAP2 > bit KVM_FEATURE2_HTM). > > The reason to use the value from the auxiliary vector is that it is > set based only on what the host kernel found in the ibm,pa-features > HTM bit at boot time. > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> Applied to ppc-for-2.9. > --- > target/ppc/kvm.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 9f1f132cef..8a54709ae4 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -49,6 +49,7 @@ > #if defined(TARGET_PPC64) > #include "hw/ppc/spapr_cpu_core.h" > #endif > +#include "elf.h" > > //#define DEBUG_KVM > > @@ -509,8 +510,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > case POWERPC_MMU_2_07: > if (!cap_htm && !kvmppc_is_pr(cs->kvm_state)) { > /* KVM-HV has transactional memory on POWER8 also without the > - * KVM_CAP_PPC_HTM extension, so enable it here instead. */ > - cap_htm = true; > + * KVM_CAP_PPC_HTM extension, so enable it here instead as > + * long as it's availble to userspace on the host. */ > + if (qemu_getauxval(AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) { > + cap_htm = true; > + } > } > break; > default:
On 29.03.2017 07:01, Sam Bobroff wrote: > On Power8 hosts it is currently theoretically possible for QEMU/KVM-HV guests > to receive a ibm,pa-features property indicating that HTM support is available > when it is not. The situation would occur if the platform firmware of > a Power8 host cleared the HTM bit of the ibm,pa-features property. Out of curiosity: Is there a machine out there where this happens? > QEMU would query KVM for the availability of HTM, which will return no > support, but workaround code in kvm_arch_init_vcpu() would then > re-enable it because KVM_HV is in use and the processor is P8. > > This patch adjusts the workaround in kvm_arch_init_vcpu() so that it does not > enable HTM (in the above case) unless the host kernel indicates to the QEMU > process, via the auxiliary vector, that userspace can use HTM (via the HWCAP2 > bit KVM_FEATURE2_HTM). > > The reason to use the value from the auxiliary vector is that it is > set based only on what the host kernel found in the ibm,pa-features > HTM bit at boot time. > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> > --- > target/ppc/kvm.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index 9f1f132cef..8a54709ae4 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -49,6 +49,7 @@ > #if defined(TARGET_PPC64) > #include "hw/ppc/spapr_cpu_core.h" > #endif > +#include "elf.h" > > //#define DEBUG_KVM > > @@ -509,8 +510,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > case POWERPC_MMU_2_07: > if (!cap_htm && !kvmppc_is_pr(cs->kvm_state)) { > /* KVM-HV has transactional memory on POWER8 also without the > - * KVM_CAP_PPC_HTM extension, so enable it here instead. */ > - cap_htm = true; > + * KVM_CAP_PPC_HTM extension, so enable it here instead as > + * long as it's availble to userspace on the host. */ > + if (qemu_getauxval(AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) { > + cap_htm = true; > + } That's a very good idea! ... but I think you could also merge the two if-statements into one to save one level of indentation. Thomas
On Wed, Mar 29, 2017 at 07:39:25AM +0200, Thomas Huth wrote: > On 29.03.2017 07:01, Sam Bobroff wrote: > > On Power8 hosts it is currently theoretically possible for QEMU/KVM-HV guests > > to receive a ibm,pa-features property indicating that HTM support is available > > when it is not. The situation would occur if the platform firmware of > > a Power8 host cleared the HTM bit of the ibm,pa-features property. > > Out of curiosity: Is there a machine out there where this happens? Not that I know of... just the one who's firmware I broke on purpose for testing ;-) > > QEMU would query KVM for the availability of HTM, which will return no > > support, but workaround code in kvm_arch_init_vcpu() would then > > re-enable it because KVM_HV is in use and the processor is P8. > > > > This patch adjusts the workaround in kvm_arch_init_vcpu() so that it does not > > enable HTM (in the above case) unless the host kernel indicates to the QEMU > > process, via the auxiliary vector, that userspace can use HTM (via the HWCAP2 > > bit KVM_FEATURE2_HTM). > > > > The reason to use the value from the auxiliary vector is that it is > > set based only on what the host kernel found in the ibm,pa-features > > HTM bit at boot time. > > > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> > > --- > > target/ppc/kvm.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > index 9f1f132cef..8a54709ae4 100644 > > --- a/target/ppc/kvm.c > > +++ b/target/ppc/kvm.c > > @@ -49,6 +49,7 @@ > > #if defined(TARGET_PPC64) > > #include "hw/ppc/spapr_cpu_core.h" > > #endif > > +#include "elf.h" > > > > //#define DEBUG_KVM > > > > @@ -509,8 +510,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > > case POWERPC_MMU_2_07: > > if (!cap_htm && !kvmppc_is_pr(cs->kvm_state)) { > > /* KVM-HV has transactional memory on POWER8 also without the > > - * KVM_CAP_PPC_HTM extension, so enable it here instead. */ > > - cap_htm = true; > > + * KVM_CAP_PPC_HTM extension, so enable it here instead as > > + * long as it's availble to userspace on the host. */ > > + if (qemu_getauxval(AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) { > > + cap_htm = true; > > + } > > That's a very good idea! ... but I think you could also merge the two > if-statements into one to save one level of indentation. > > Thomas
On Thu, Mar 30, 2017 at 01:28:36PM +1100, Sam Bobroff wrote: > On Wed, Mar 29, 2017 at 07:39:25AM +0200, Thomas Huth wrote: > > On 29.03.2017 07:01, Sam Bobroff wrote: > > > On Power8 hosts it is currently theoretically possible for QEMU/KVM-HV guests > > > to receive a ibm,pa-features property indicating that HTM support is available > > > when it is not. The situation would occur if the platform firmware of > > > a Power8 host cleared the HTM bit of the ibm,pa-features property. > > > > Out of curiosity: Is there a machine out there where this happens? > > Not that I know of... just the one who's firmware I broke on purpose for > testing ;-) Ok, given that, I'm moving this fix from ppc-for-2.9 to ppc-for-2.10. > > > > QEMU would query KVM for the availability of HTM, which will return no > > > support, but workaround code in kvm_arch_init_vcpu() would then > > > re-enable it because KVM_HV is in use and the processor is P8. > > > > > > This patch adjusts the workaround in kvm_arch_init_vcpu() so that it does not > > > enable HTM (in the above case) unless the host kernel indicates to the QEMU > > > process, via the auxiliary vector, that userspace can use HTM (via the HWCAP2 > > > bit KVM_FEATURE2_HTM). > > > > > > The reason to use the value from the auxiliary vector is that it is > > > set based only on what the host kernel found in the ibm,pa-features > > > HTM bit at boot time. > > > > > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> > > > --- > > > target/ppc/kvm.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > > > index 9f1f132cef..8a54709ae4 100644 > > > --- a/target/ppc/kvm.c > > > +++ b/target/ppc/kvm.c > > > @@ -49,6 +49,7 @@ > > > #if defined(TARGET_PPC64) > > > #include "hw/ppc/spapr_cpu_core.h" > > > #endif > > > +#include "elf.h" > > > > > > //#define DEBUG_KVM > > > > > > @@ -509,8 +510,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > case POWERPC_MMU_2_07: > > > if (!cap_htm && !kvmppc_is_pr(cs->kvm_state)) { > > > /* KVM-HV has transactional memory on POWER8 also without the > > > - * KVM_CAP_PPC_HTM extension, so enable it here instead. */ > > > - cap_htm = true; > > > + * KVM_CAP_PPC_HTM extension, so enable it here instead as > > > + * long as it's availble to userspace on the host. */ > > > + if (qemu_getauxval(AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) { > > > + cap_htm = true; > > > + } > > > > That's a very good idea! ... but I think you could also merge the two > > if-statements into one to save one level of indentation. > > > > Thomas >
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index 9f1f132cef..8a54709ae4 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -49,6 +49,7 @@ #if defined(TARGET_PPC64) #include "hw/ppc/spapr_cpu_core.h" #endif +#include "elf.h" //#define DEBUG_KVM @@ -509,8 +510,11 @@ int kvm_arch_init_vcpu(CPUState *cs) case POWERPC_MMU_2_07: if (!cap_htm && !kvmppc_is_pr(cs->kvm_state)) { /* KVM-HV has transactional memory on POWER8 also without the - * KVM_CAP_PPC_HTM extension, so enable it here instead. */ - cap_htm = true; + * KVM_CAP_PPC_HTM extension, so enable it here instead as + * long as it's availble to userspace on the host. */ + if (qemu_getauxval(AT_HWCAP2) & PPC_FEATURE2_HAS_HTM) { + cap_htm = true; + } } break; default:
On Power8 hosts it is currently theoretically possible for QEMU/KVM-HV guests to receive a ibm,pa-features property indicating that HTM support is available when it is not. The situation would occur if the platform firmware of a Power8 host cleared the HTM bit of the ibm,pa-features property. QEMU would query KVM for the availability of HTM, which will return no support, but workaround code in kvm_arch_init_vcpu() would then re-enable it because KVM_HV is in use and the processor is P8. This patch adjusts the workaround in kvm_arch_init_vcpu() so that it does not enable HTM (in the above case) unless the host kernel indicates to the QEMU process, via the auxiliary vector, that userspace can use HTM (via the HWCAP2 bit KVM_FEATURE2_HTM). The reason to use the value from the auxiliary vector is that it is set based only on what the host kernel found in the ibm,pa-features HTM bit at boot time. Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com> --- target/ppc/kvm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)