[Qemu-devel] [PATCH 0/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[Qemu-devel] [PATCH 0/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[Sam Bobroff]

Hi QEMU,

See the patch itself for a description of the issue it's fixing.

Additionally, I've done some investigation on the effect of the patch on older
kernels. The discussion below only refers to the situation in which the
existing workaround would have an effect (system is P8, KVM is HV and KVM does
not indicate support for HTM):

PPC_FEATURE2_HTM has existed since mid 2013 [1], and at that time it was
unconditionally set for P8: nothing will change here because the new test will
always be true, always allowing the workaround to activate. The patch doesn't
help here.

In early 2016 [2] PPC_FEATURE2_HTM was linked to the HTM bit of
ibm,pa-features: the patch will help from here onwards.

So the patch doesn't fix all situations but it doesn't break any either, and it
fixes versions going forward.

Cheers,
Sam.

1: Around kernel commit cbbc6f1b1433ef553d57826eee87a84ca49645ce (v3.10-rc1)
2: Around kernel commit 4705e02498d6d5a7ab98dfee9595cd5e91db2017 (v4.6-rc1)


Sam Bobroff (1):
  target/ppc: Improve accuracy of guest HTM availability on P8s

 target/ppc/kvm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.12.1.382.gc0f9c7058

[Qemu-devel] [PATCH 1/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[Sam Bobroff]

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(-)

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:
-- 
2.12.1.382.gc0f9c7058

Re: [Qemu-devel] [PATCH 1/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[David Gibson]

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

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:

-- 
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 1/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH 1/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[Sam Bobroff]

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

Re: [Qemu-devel] [PATCH 1/1] target/ppc: Improve accuracy of guest HTM availability on P8s

[David Gibson]

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

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
> 

-- 
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 --]