Re: [Qemu-devel] PR KVM and TM issues

Re: [Qemu-devel] PR KVM and TM issues

[Anton Blanchard]

Hi Alexey,

> > I can't get an Ubuntu Wily guest to boot on an Ubuntu Wily host in
> > PR KVM mode. The kernel in both cases is 4.2. To reproduce:
> >
> > wget -N https://cloud-images.ubuntu.com/wily/current/wily-server-cloudimg-ppc64el-disk1.img
> >
> > qemu-system-ppc64 -cpu POWER8 -enable-kvm -machine pseries,kvm-type=PR -m 4G -nographic -vga none -drive file=wily-server-cloudimg-ppc64el-disk1.img,if=virtio
> >
> > Should TM work inside a PR KVM guest?
> 
> If I read the kernel code correctly (kvmppc_set_one_reg_hv vs. 
> kvmppc_set_one_reg_pr), no, it should not be expected to work.

I see a couple of issues, patches to follow:

1. QEMU needs to clear the TM feature bit in the ibm,pa-features array
when running in PR KVM mode.

2. Linux needs to clear the user TM feature bits if TM gets disabled
at runtime via the ibm,pa-features bit.

Anton

[Qemu-devel] [PATCH] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[Anton Blanchard]

We don't support transactional memory in PR KVM, so don't tell
the OS that we do.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e7be21e..538bd87 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -696,6 +696,12 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
         pa_features = pa_features_207;
         pa_size = sizeof(pa_features_207);
+
+        /* Don't enable TM in PR KVM mode */
+        if (kvm_enabled() &&
+            kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
+            pa_features[24] &= ~0x80;
+        }
     }
     if (env->ci_large_pages) {
         pa_features[3] |= 0x20;

Re: [Qemu-devel] [PATCH] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[Alexander Graf]

> Am 04.04.2016 um 13:09 schrieb Anton Blanchard <anton@samba.org>:
> 
> We don't support transactional memory in PR KVM, so don't tell
> the OS that we do.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e7be21e..538bd87 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -696,6 +696,12 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>     } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
>         pa_features = pa_features_207;
>         pa_size = sizeof(pa_features_207);
> +
> +        /* Don't enable TM in PR KVM mode */
> +        if (kvm_enabled() &&
> +            kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO))

Does this compile on non-kvm systems?

Alex

> {
> +            pa_features[24] &= ~0x80;
> +        }
>     }
>     if (env->ci_large_pages) {
>         pa_features[3] |= 0x20;

[Qemu-devel] [PATCH v2] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[Anton Blanchard]

We don't support transactional memory in PR KVM, so don't tell
the OS that we do.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

v2: Fix build with CONFIG_KVM disabled, noticed by Alex.

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b69995e..dc3e3c9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -696,6 +696,14 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
         pa_features = pa_features_207;
         pa_size = sizeof(pa_features_207);
+
+#ifdef CONFIG_KVM
+        /* Don't enable TM in PR KVM mode */
+        if (kvm_enabled() &&
+            kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
+            pa_features[24] &= ~0x80;
+        }
+#endif
     }
     if (env->ci_large_pages) {
         pa_features[3] |= 0x20;

Re: [Qemu-devel] [PATCH v2] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[haris iqbal]

On Sat, Apr 30, 2016 at 6:18 AM, Anton Blanchard <anton@samba.org> wrote:
> We don't support transactional memory in PR KVM, so don't tell
> the OS that we do.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>
> v2: Fix build with CONFIG_KVM disabled, noticed by Alex.
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b69995e..dc3e3c9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -696,6 +696,14 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
>          pa_features = pa_features_207;
>          pa_size = sizeof(pa_features_207);
> +
> +#ifdef CONFIG_KVM
> +        /* Don't enable TM in PR KVM mode */
> +        if (kvm_enabled() &&
> +            kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> +            pa_features[24] &= ~0x80;
> +        }
> +#endif
>      }
>      if (env->ci_large_pages) {
>          pa_features[3] |= 0x20;
>

This email was put in the spam folder by gmail. The message said "It
has a from address in samba.org but has failed samba.org's required
tests for authentication". Just bringing this to peoples attention. I
thought a patch might go unnoticed else.

-- 

With regards,

Md Haris Iqbal,
Placement Coordinator, MTech IT
NITK Surathkal,
Contact: +91 8861996962

Re: [Qemu-devel] [PATCH v2] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[David Gibson]

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

On Sat, Apr 30, 2016 at 10:48:00AM +1000, Anton Blanchard wrote:
> We don't support transactional memory in PR KVM, so don't tell
> the OS that we do.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>

Relying on CAP_PPC_GET_PVINFO is a hack we need in some cases, but
it's not something to be encouraged.  I'd prefer to see this examining
a new capability specifically advertising TM support in KVM, with a
fallback to PVINFO if necessary.

Come to that, we probably shouldn't be advertising TM support in TCG,
either, since IIRC our "support" for the TM instructions there is
basically worse than useless.

> ---
> 
> v2: Fix build with CONFIG_KVM disabled, noticed by Alex.
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b69995e..dc3e3c9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -696,6 +696,14 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
>          pa_features = pa_features_207;
>          pa_size = sizeof(pa_features_207);
> +
> +#ifdef CONFIG_KVM
> +        /* Don't enable TM in PR KVM mode */
> +        if (kvm_enabled() &&
> +            kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> +            pa_features[24] &= ~0x80;
> +        }
> +#endif
>      }
>      if (env->ci_large_pages) {
>          pa_features[3] |= 0x20;
> 

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

[Qemu-devel] [PATCH 1/2] Add PowerPC AT_HWCAP2 definitions

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

We need the PPC_FEATURE2_HAS_HTM bit in a subsequent patch, so
add the PowerPC AT_HWCAP2 definitions.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

diff --git a/include/elf.h b/include/elf.h
index 28d448b..8533b2a 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -477,6 +477,19 @@ typedef struct {
 #define PPC_FEATURE_TRUE_LE             0x00000002
 #define PPC_FEATURE_PPC_LE              0x00000001
 
+/* Bits present in AT_HWCAP2 for PowerPC.  */
+
+#define PPC_FEATURE2_ARCH_2_07          0x80000000
+#define PPC_FEATURE2_HAS_HTM            0x40000000
+#define PPC_FEATURE2_HAS_DSCR           0x20000000
+#define PPC_FEATURE2_HAS_EBB            0x10000000
+#define PPC_FEATURE2_HAS_ISEL           0x08000000
+#define PPC_FEATURE2_HAS_TAR            0x04000000
+#define PPC_FEATURE2_HAS_VEC_CRYPTO     0x02000000
+#define PPC_FEATURE2_HTM_NOSC           0x01000000
+#define PPC_FEATURE2_ARCH_3_00          0x00800000
+#define PPC_FEATURE2_HAS_IEEE128        0x00400000
+
 /* Bits present in AT_HWCAP for Sparc.  */
 
 #define HWCAP_SPARC_FLUSH               0x00000001

[Qemu-devel] [PATCH 2/2] spapr: Better handling of ibm, pa-features TM bit

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

There are a few issues with our handling of the ibm,pa-features
TM bit:

- We don't support transactional memory in PR KVM, so don't tell
  the OS that we do.

- In full emulation we have a minimal implementation of TM that always
  fails, so for performance reasons lets not tell the OS that we
  support it either.

- In HV KVM mode, we should mirror the host TM enabled state by
  looking at the AT_HWCAP2 bit.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0636642..c403fbb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -620,7 +620,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
         0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
         0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
-        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
+        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
     uint8_t *pa_features;
     size_t pa_size;
 
@@ -697,6 +697,19 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
         pa_features = pa_features_207;
         pa_size = sizeof(pa_features_207);
+
+#ifdef CONFIG_KVM
+        /* Only enable TM in HV KVM mode */
+        if (kvm_enabled() &&
+            !kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
+            unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
+
+            /* Guest should inherit host TM enabled bit */
+            if (hwcap2 & PPC_FEATURE2_HAS_HTM) {
+                pa_features[24] |= 0x80;
+            }
+        }
+#endif
     }
     if (env->ci_large_pages) {
         pa_features[3] |= 0x20;

Re: [Qemu-devel] [PATCH 2/2] spapr: Better handling of ibm, pa-features TM bit

[David Gibson]

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

On Tue, Jun 07, 2016 at 10:32:10PM +1000, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> There are a few issues with our handling of the ibm,pa-features
> TM bit:
> 
> - We don't support transactional memory in PR KVM, so don't tell
>   the OS that we do.
> 
> - In full emulation we have a minimal implementation of TM that always
>   fails, so for performance reasons lets not tell the OS that we
>   support it either.
> 
> - In HV KVM mode, we should mirror the host TM enabled state by
>   looking at the AT_HWCAP2 bit.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>

So, we certainly need a change like this.  I'm not entirely happy with
the current implementation though.

> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0636642..c403fbb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -620,7 +620,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>          0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
>          0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
>          0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> -        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
>      uint8_t *pa_features;
>      size_t pa_size;
>  
> @@ -697,6 +697,19 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
>          pa_features = pa_features_207;
>          pa_size = sizeof(pa_features_207);
> +
> +#ifdef CONFIG_KVM
> +        /* Only enable TM in HV KVM mode */
> +        if (kvm_enabled() &&
> +            !kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> +            unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
> +
> +            /* Guest should inherit host TM enabled bit */
> +            if (hwcap2 & PPC_FEATURE2_HAS_HTM) {
> +                pa_features[24] |= 0x80;
> +            }
> +        }
> +#endif

So first, I think this stanza wants to move into target-ppc/kvm.c -
maybe a kvm_filter_pa_features() call or something.

Second, although using PVINFO to determine if we have HV KVM is a
standard trick, we don't want to use it as our first option.  We
really want to introduce an actual KVM CAP flag for TM support, then
fall back to checking PVINFO if we can't use that.

I wonder if we actually want to just blanket disable TM in one patch -
since it doesn't work at all with PR KVM, and "works" only in the most
rules-lawyering and useless way on TCG.  Then re-enable it on HV KVM
in a second patch.

>      }
>      if (env->ci_large_pages) {
>          pa_features[3] |= 0x20;
> 

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

[Qemu-devel] [PATCH 0/3] Rework spapr: Better handling of ibm, pa-features TM bit

[Sam Bobroff]

Hi David,

Anton asked me to have a look at this, so here is an attempt at a
re-implementation of his: "spapr: Better handling of ibm, pa-features TM bit"
addressing your comments and those from Paul Mackerras.  I've broken the
patch into one to unconditionally disable the HTM bit in pa-features and a
second one to set it conditionally based on a (new) KVM capability. It requires
a small patch to KVM to support the capability, presumably something like this:

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 02416fe..4a8ddab 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -588,6 +588,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
                r = 1;
                break;
 #endif
+       case KVM_CAP_PPC_HTM:
+               r = cpu_has_feature(CPU_FTR_TM);
+               break;
        default:
                r = 0;
                break;

Adding a new capability requires changing linux/kvm.h but I believe we can
avoid incrementing KVM_API_VERSION at this stage since kernels that don't yet
support it will simply return false. However, due to the ambiguity of that
result I've included Anton's initial fallback approach to be used in that case.
Once the API version is incremented (and support for the capability is
guaranteed) the ambiguity would be gone and we should be able to remove the
fallback. As long as that happens before KVM PR supports HTM, this should
address Paul's concern about using the PVINFO capability to discover KVM-HV.

Note: I'm not sure how to handle the change to linux/kvm.h, I've included the
patch here because it's needed to compile, but I suspect it needs to go via the
kernel. Let's see if this part looks good first.

Note also: I've changed TM to HTM where possible in an attempt to be consistent.


Sam Bobroff (3):
  spapr: Disable ibm, pa-features HTM bit
  Add KVM_CAP_PPC_HTM to linux/kvm.h
  spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

 hw/ppc/spapr.c            |  3 ++-
 linux-headers/linux/kvm.h |  1 +
 target-ppc/kvm.c          | 27 +++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h      |  6 ++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] spapr: Disable ibm, pa-features HTM bit

[Sam Bobroff]

There are a few issues with our handling of the ibm,pa-features
HTM bit:

- We don't support transactional memory in PR KVM, so don't tell
  the OS that we do.

- In full emulation we have a minimal implementation of HTM that always
  fails, so for performance reasons lets not tell the OS that we
  support it either.

- In HV KVM mode, we should mirror the host HTM enabled state by
  checking a KVM capability or looking at the AT_HWCAP2 bit.

For now unconditionally disable it by removing HTM from the
pa-features bits.  It will be re-enabled in a subsequent patch
specifically for HV KVM.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 78ebd9e..704aae7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -635,7 +635,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
         0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
         0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
-        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
+        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
     uint8_t *pa_features;
     size_t pa_size;
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/3] spapr: Disable ibm, pa-features HTM bit

[David Gibson]

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

On Tue, Jul 05, 2016 at 03:19:22PM +1000, Sam Bobroff wrote:
> There are a few issues with our handling of the ibm,pa-features
> HTM bit:
> 
> - We don't support transactional memory in PR KVM, so don't tell
>   the OS that we do.
> 
> - In full emulation we have a minimal implementation of HTM that always
>   fails, so for performance reasons lets not tell the OS that we
>   support it either.
> 
> - In HV KVM mode, we should mirror the host HTM enabled state by
>   checking a KVM capability or looking at the AT_HWCAP2 bit.
> 
> For now unconditionally disable it by removing HTM from the
> pa-features bits.  It will be re-enabled in a subsequent patch
> specifically for HV KVM.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

I'll hold off on merging until the rest of the series is ready, though.

> ---
>  hw/ppc/spapr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 78ebd9e..704aae7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -635,7 +635,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>          0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
>          0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
>          0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> -        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
>      uint8_t *pa_features;
>      size_t pa_size;
>  

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

[Qemu-devel] [PATCH 2/3] Add KVM_CAP_PPC_HTM to linux/kvm.h

[Sam Bobroff]

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 linux-headers/linux/kvm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index e60e21b..37cb3e8 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -866,6 +866,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_ARM_PMU_V3 126
 #define KVM_CAP_VCPU_ATTRIBUTES 127
 #define KVM_CAP_MAX_VCPU_ID 128
+#define KVM_CAP_PPC_HTM 129
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/3] Add KVM_CAP_PPC_HTM to linux/kvm.h

[David Gibson]

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

On Tue, Jul 05, 2016 at 03:19:23PM +1000, Sam Bobroff wrote:
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Ok, so the usual procedure for updates to linux-headers is this:
   1. Get the change merged on the kernel side

   2. Use scripts/update-linux-headers.sh to update the whole
      linux-headers subtree to the new kernel version

   3. Submit the patch - make sure to include the commit id of the
      kernel you updated to in the qemu commit message

> ---
>  linux-headers/linux/kvm.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index e60e21b..37cb3e8 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -866,6 +866,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_ARM_PMU_V3 126
>  #define KVM_CAP_VCPU_ATTRIBUTES 127
>  #define KVM_CAP_MAX_VCPU_ID 128
> +#define KVM_CAP_PPC_HTM 129
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  

-- 
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 2/3] Add KVM_CAP_PPC_HTM to linux/kvm.h

[Sam Bobroff]

On Tue, Jul 05, 2016 at 04:05:58PM +1000, David Gibson wrote:
> On Tue, Jul 05, 2016 at 03:19:23PM +1000, Sam Bobroff wrote:
> > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> 
> Ok, so the usual procedure for updates to linux-headers is this:
>    1. Get the change merged on the kernel side
> 
>    2. Use scripts/update-linux-headers.sh to update the whole
>       linux-headers subtree to the new kernel version
> 
>    3. Submit the patch - make sure to include the commit id of the
>       kernel you updated to in the qemu commit message

Thanks,

Do you think we're at the point with this patch set that I should start
progressing the kernel side change now?

Cheers,
Sam.

Re: [Qemu-devel] [PATCH 2/3] Add KVM_CAP_PPC_HTM to linux/kvm.h

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:41:52PM +1000, Sam Bobroff wrote:
> On Tue, Jul 05, 2016 at 04:05:58PM +1000, David Gibson wrote:
> > On Tue, Jul 05, 2016 at 03:19:23PM +1000, Sam Bobroff wrote:
> > > Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > 
> > Ok, so the usual procedure for updates to linux-headers is this:
> >    1. Get the change merged on the kernel side
> > 
> >    2. Use scripts/update-linux-headers.sh to update the whole
> >       linux-headers subtree to the new kernel version
> > 
> >    3. Submit the patch - make sure to include the commit id of the
> >       kernel you updated to in the qemu commit message
> 
> Thanks,
> 
> Do you think we're at the point with this patch set that I should start
> progressing the kernel side change now?

Yes.

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

[Qemu-devel] [PATCH 3/3] spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

[Sam Bobroff]

Advertise HTM support in ibm, pa-features if KVM indicates support when
queried via a new capability (KVM_CAP_PPC_HTM).

If KVM returns false for the capability (which may indicate that the
host kernel doesn't support the capability itself) attempt to
determine availability using a fallback method based on KVM being
KVM-HV and HTM being available to the QEMU process.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 hw/ppc/spapr.c       |  3 ++-
 target-ppc/kvm.c     | 27 +++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |  6 ++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 704aae7..e229532 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -606,6 +606,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
         : SPAPR_TIMEBASE_FREQ;
     uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
+    uint8_t htm = (kvm_enabled() && kvmppc_get_htm_support(env)) ? 0x80 : 0x00;
     uint32_t page_sizes_prop[64];
     size_t page_sizes_prop_size;
     uint32_t vcpus_per_socket = smp_threads * smp_cores;
@@ -635,7 +636,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
         0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
         0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
-        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
+        0x80, 0x00, 0x80, 0x00, 0x00 | htm, 0x00 };
     uint8_t *pa_features;
     size_t pa_size;
 
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 884d564..f94ce3b 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -20,6 +20,7 @@
 #include <sys/vfs.h>
 
 #include <linux/kvm.h>
+#include "elf.h"
 
 #include "qemu-common.h"
 #include "qemu/error-report.h"
@@ -1976,6 +1977,32 @@ uint32_t kvmppc_get_dfp(void)
     return kvmppc_read_int_cpu_dt("ibm,dfp");
 }
 
+bool kvmppc_get_htm_support(CPUPPCState *env)
+{
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    CPUState *cs = CPU(cpu);
+
+
+    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_HTM)) {
+        return true;
+    }
+    /*
+     * Fallback test for host kernels that don't yet support KVM_CAP_PPC_HTM.
+     * This will be unnecessary when KVM_API_VERSION is incremented (to 13 or
+     * above) because that will remove the ambiguity between the host kernel
+     * lacking support for KVM_CAP_PPC_HTM and it having support but reporting
+     * HTM as unavailable (both of which return 0, above).
+     */
+    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
+        /* Assume this means PR KVM, so no TM. */
+        return false;
+    } else {
+        /* Assume this means HV KVM, propagate whatever host userspace sees. */
+        unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
+        return !!(hwcap2 & PPC_FEATURE2_HAS_HTM);
+    }
+}
+
 static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo *pvinfo)
  {
      PowerPCCPU *cpu = ppc_env_get_cpu(env);
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 20bfb59..b01c717 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -17,6 +17,7 @@ uint32_t kvmppc_get_tbfreq(void);
 uint64_t kvmppc_get_clockfreq(void);
 uint32_t kvmppc_get_vmx(void);
 uint32_t kvmppc_get_dfp(void);
+bool kvmppc_get_htm_support(CPUPPCState *env);
 bool kvmppc_get_host_model(char **buf);
 bool kvmppc_get_host_serial(char **buf);
 int kvmppc_get_hasidle(CPUPPCState *env);
@@ -90,6 +91,11 @@ static inline uint32_t kvmppc_get_dfp(void)
     return 0;
 }
 
+static inline bool kvmppc_get_htm_support(KVMState *kvm_state)
+{
+    return false;
+}
+
 static inline int kvmppc_get_hasidle(CPUPPCState *env)
 {
     return 0;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/3] spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

[David Gibson]

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

On Tue, Jul 05, 2016 at 03:19:24PM +1000, Sam Bobroff wrote:
> Advertise HTM support in ibm, pa-features if KVM indicates support when
> queried via a new capability (KVM_CAP_PPC_HTM).
> 
> If KVM returns false for the capability (which may indicate that the
> host kernel doesn't support the capability itself) attempt to
> determine availability using a fallback method based on KVM being
> KVM-HV and HTM being available to the QEMU process.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
>  hw/ppc/spapr.c       |  3 ++-
>  target-ppc/kvm.c     | 27 +++++++++++++++++++++++++++
>  target-ppc/kvm_ppc.h |  6 ++++++
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 704aae7..e229532 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -606,6 +606,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
>          : SPAPR_TIMEBASE_FREQ;
>      uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> +    uint8_t htm = (kvm_enabled() && kvmppc_get_htm_support(env)) ? 0x80 : 0x00;

You can simplify this if you make kvmppc_get_htm_support() return
false when !kvm_enabled().

>      uint32_t page_sizes_prop[64];
>      size_t page_sizes_prop_size;
>      uint32_t vcpus_per_socket = smp_threads * smp_cores;
> @@ -635,7 +636,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>          0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
>          0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
>          0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> -        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
> +        0x80, 0x00, 0x80, 0x00, 0x00 | htm, 0x00 };

I think it would be easier to read if the initializer is kept constant
and you fold in the htm bit in the actual code.

>      uint8_t *pa_features;
>      size_t pa_size;
>  
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 884d564..f94ce3b 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -20,6 +20,7 @@
>  #include <sys/vfs.h>
>  
>  #include <linux/kvm.h>
> +#include "elf.h"
>  
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -1976,6 +1977,32 @@ uint32_t kvmppc_get_dfp(void)
>      return kvmppc_read_int_cpu_dt("ibm,dfp");
>  }
>  
> +bool kvmppc_get_htm_support(CPUPPCState *env)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUState *cs = CPU(cpu);
> +
> +
> +    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_HTM)) {
> +        return true;
> +    }
> +    /*
> +     * Fallback test for host kernels that don't yet support KVM_CAP_PPC_HTM.
> +     * This will be unnecessary when KVM_API_VERSION is incremented (to 13 or
> +     * above) because that will remove the ambiguity between the host kernel
> +     * lacking support for KVM_CAP_PPC_HTM and it having support but reporting
> +     * HTM as unavailable (both of which return 0, above).
> +     */
> +    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> +        /* Assume this means PR KVM, so no TM. */
> +        return false;
> +    } else {
> +        /* Assume this means HV KVM, propagate whatever host userspace sees. */
> +        unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
> +        return !!(hwcap2 & PPC_FEATURE2_HAS_HTM);
> +    }
> +}
> +
>  static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo *pvinfo)
>   {
>       PowerPCCPU *cpu = ppc_env_get_cpu(env);
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 20bfb59..b01c717 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -17,6 +17,7 @@ uint32_t kvmppc_get_tbfreq(void);
>  uint64_t kvmppc_get_clockfreq(void);
>  uint32_t kvmppc_get_vmx(void);
>  uint32_t kvmppc_get_dfp(void);
> +bool kvmppc_get_htm_support(CPUPPCState *env);
>  bool kvmppc_get_host_model(char **buf);
>  bool kvmppc_get_host_serial(char **buf);
>  int kvmppc_get_hasidle(CPUPPCState *env);
> @@ -90,6 +91,11 @@ static inline uint32_t kvmppc_get_dfp(void)
>      return 0;
>  }
>  
> +static inline bool kvmppc_get_htm_support(KVMState *kvm_state)
> +{
> +    return false;
> +}
> +
>  static inline int kvmppc_get_hasidle(CPUPPCState *env)
>  {
>      return 0;

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

[Qemu-devel] [PATCH v2 0/3] Rework spapr: Better handling of ibm, pa-features TM bit

[Sam Bobroff]

Here's version 2.


Changes v1 -> v2:
Patch 2/3: spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

* Improve readability of HTM bit set code.
* Move the test for KVM into kvmppc_get_htm_support().


Sam Bobroff (3):
  spapr: Disable ibm, pa-features HTM bit
  Add KVM_CAP_PPC_HTM to linux/kvm.h
  spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

 hw/ppc/spapr.c            |  5 ++++-
 linux-headers/linux/kvm.h |  1 +
 target-ppc/kvm.c          | 29 +++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h      |  6 ++++++
 4 files changed, 40 insertions(+), 1 deletion(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/3] spapr: Disable ibm, pa-features HTM bit

[Sam Bobroff]

There are a few issues with our handling of the ibm,pa-features
HTM bit:

- We don't support transactional memory in PR KVM, so don't tell
  the OS that we do.

- In full emulation we have a minimal implementation of HTM that always
  fails, so for performance reasons lets not tell the OS that we
  support it either.

- In HV KVM mode, we should mirror the host HTM enabled state by
  checking a KVM capability or looking at the AT_HWCAP2 bit.

For now unconditionally disable it by removing HTM from the
pa-features bits.  It will be re-enabled in a subsequent patch
specifically for HV KVM.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 78ebd9e..704aae7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -635,7 +635,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
         0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
         0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
-        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
+        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
     uint8_t *pa_features;
     size_t pa_size;
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 1/3] spapr: Disable ibm, pa-features HTM bit

[David Gibson]

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

On Wed, Jul 06, 2016 at 03:35:21PM +1000, Sam Bobroff wrote:
> There are a few issues with our handling of the ibm,pa-features
> HTM bit:
> 
> - We don't support transactional memory in PR KVM, so don't tell
>   the OS that we do.
> 
> - In full emulation we have a minimal implementation of HTM that always
>   fails, so for performance reasons lets not tell the OS that we
>   support it either.
> 
> - In HV KVM mode, we should mirror the host HTM enabled state by
>   checking a KVM capability or looking at the AT_HWCAP2 bit.
> 
> For now unconditionally disable it by removing HTM from the
> pa-features bits.  It will be re-enabled in a subsequent patch
> specifically for HV KVM.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

But I won't merge it until the rest of the series is ready to go.


> ---
>  hw/ppc/spapr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 78ebd9e..704aae7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -635,7 +635,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>          0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
>          0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
>          0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> -        0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
>      uint8_t *pa_features;
>      size_t pa_size;
>  

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

[Qemu-devel] [PATCH v2 2/3] Add KVM_CAP_PPC_HTM to linux/kvm.h

[Sam Bobroff]

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 linux-headers/linux/kvm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index e60e21b..37cb3e8 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -866,6 +866,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_ARM_PMU_V3 126
 #define KVM_CAP_VCPU_ATTRIBUTES 127
 #define KVM_CAP_MAX_VCPU_ID 128
+#define KVM_CAP_PPC_HTM 129
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 2/3] Add KVM_CAP_PPC_HTM to linux/kvm.h

[David Gibson]

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

On Wed, Jul 06, 2016 at 03:35:22PM +1000, Sam Bobroff wrote:
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

We need to wait for this to be merged on the kernel side.

> ---
>  linux-headers/linux/kvm.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index e60e21b..37cb3e8 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -866,6 +866,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_ARM_PMU_V3 126
>  #define KVM_CAP_VCPU_ATTRIBUTES 127
>  #define KVM_CAP_MAX_VCPU_ID 128
> +#define KVM_CAP_PPC_HTM 129
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  

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

[Qemu-devel] [PATCH v2 3/3] spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

[Sam Bobroff]

Advertise HTM support in ibm, pa-features if KVM indicates support when
queried via a new capability (KVM_CAP_PPC_HTM).

If KVM returns false for the capability (which may indicate that the
host kernel doesn't support the capability itself) attempt to
determine availability using a fallback method based on KVM being
KVM-HV and HTM being available to the QEMU process.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
v2:

* Improve readability of HTM bit set code.
* Move the test for KVM into kvmppc_get_htm_support().

 hw/ppc/spapr.c       |  3 +++
 target-ppc/kvm.c     | 29 +++++++++++++++++++++++++++++
 target-ppc/kvm_ppc.h |  6 ++++++
 3 files changed, 38 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 704aae7..843fbe7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -712,6 +712,9 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
         pa_features = pa_features_207;
         pa_size = sizeof(pa_features_207);
+        if (kvmppc_get_htm_support(env)) {
+            pa_features[24] |= 0x80;
+        }
     }
     if (env->ci_large_pages) {
         pa_features[3] |= 0x20;
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 884d564..9d13446 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -20,6 +20,7 @@
 #include <sys/vfs.h>
 
 #include <linux/kvm.h>
+#include "elf.h"
 
 #include "qemu-common.h"
 #include "qemu/error-report.h"
@@ -1976,6 +1977,34 @@ uint32_t kvmppc_get_dfp(void)
     return kvmppc_read_int_cpu_dt("ibm,dfp");
 }
 
+bool kvmppc_get_htm_support(CPUPPCState *env)
+{
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    CPUState *cs = CPU(cpu);
+
+    if (!kvm_enabled()) {
+        return false;
+    }
+    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_HTM)) {
+        return true;
+    }
+    /*
+     * Fallback test for host kernels that don't yet support KVM_CAP_PPC_HTM.
+     * This will be unnecessary when KVM_API_VERSION is incremented (to 13 or
+     * above) because that will remove the ambiguity between the host kernel
+     * lacking support for KVM_CAP_PPC_HTM and it having support but reporting
+     * HTM as unavailable (both of which return 0, above).
+     */
+    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
+        /* Assume this means PR KVM, so no TM. */
+        return false;
+    } else {
+        /* Assume this means HV KVM, propagate whatever host userspace sees. */
+        unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
+        return !!(hwcap2 & PPC_FEATURE2_HAS_HTM);
+    }
+}
+
 static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo *pvinfo)
  {
      PowerPCCPU *cpu = ppc_env_get_cpu(env);
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 20bfb59..b01c717 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -17,6 +17,7 @@ uint32_t kvmppc_get_tbfreq(void);
 uint64_t kvmppc_get_clockfreq(void);
 uint32_t kvmppc_get_vmx(void);
 uint32_t kvmppc_get_dfp(void);
+bool kvmppc_get_htm_support(CPUPPCState *env);
 bool kvmppc_get_host_model(char **buf);
 bool kvmppc_get_host_serial(char **buf);
 int kvmppc_get_hasidle(CPUPPCState *env);
@@ -90,6 +91,11 @@ static inline uint32_t kvmppc_get_dfp(void)
     return 0;
 }
 
+static inline bool kvmppc_get_htm_support(KVMState *kvm_state)
+{
+    return false;
+}
+
 static inline int kvmppc_get_hasidle(CPUPPCState *env)
 {
     return 0;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 3/3] spapr: Set ibm, pa-features HTM from KVM_CAP_PPC_HTM

[David Gibson]

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

On Wed, Jul 06, 2016 at 03:35:23PM +1000, Sam Bobroff wrote:
> Advertise HTM support in ibm, pa-features if KVM indicates support when
> queried via a new capability (KVM_CAP_PPC_HTM).
> 
> If KVM returns false for the capability (which may indicate that the
> host kernel doesn't support the capability itself) attempt to
> determine availability using a fallback method based on KVM being
> KVM-HV and HTM being available to the QEMU process.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2:
> 
> * Improve readability of HTM bit set code.
> * Move the test for KVM into kvmppc_get_htm_support().
> 
>  hw/ppc/spapr.c       |  3 +++
>  target-ppc/kvm.c     | 29 +++++++++++++++++++++++++++++
>  target-ppc/kvm_ppc.h |  6 ++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 704aae7..843fbe7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -712,6 +712,9 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      } else /* env->mmu_model == POWERPC_MMU_2_07 */ {
>          pa_features = pa_features_207;
>          pa_size = sizeof(pa_features_207);
> +        if (kvmppc_get_htm_support(env)) {
> +            pa_features[24] |= 0x80;
> +        }
>      }
>      if (env->ci_large_pages) {
>          pa_features[3] |= 0x20;
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 884d564..9d13446 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -20,6 +20,7 @@
>  #include <sys/vfs.h>
>  
>  #include <linux/kvm.h>
> +#include "elf.h"
>  
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -1976,6 +1977,34 @@ uint32_t kvmppc_get_dfp(void)
>      return kvmppc_read_int_cpu_dt("ibm,dfp");
>  }
>  
> +bool kvmppc_get_htm_support(CPUPPCState *env)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUState *cs = CPU(cpu);
> +
> +    if (!kvm_enabled()) {
> +        return false;
> +    }
> +    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_HTM)) {
> +        return true;
> +    }
> +    /*
> +     * Fallback test for host kernels that don't yet support KVM_CAP_PPC_HTM.
> +     * This will be unnecessary when KVM_API_VERSION is incremented (to 13 or
> +     * above) because that will remove the ambiguity between the host kernel
> +     * lacking support for KVM_CAP_PPC_HTM and it having support but reporting
> +     * HTM as unavailable (both of which return 0, above).
> +     */
> +    if (kvm_vm_check_extension(cs->kvm_state, KVM_CAP_PPC_GET_PVINFO)) {
> +        /* Assume this means PR KVM, so no TM. */
> +        return false;
> +    } else {
> +        /* Assume this means HV KVM, propagate whatever host userspace sees. */
> +        unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
> +        return !!(hwcap2 & PPC_FEATURE2_HAS_HTM);
> +    }
> +}
> +
>  static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo *pvinfo)
>   {
>       PowerPCCPU *cpu = ppc_env_get_cpu(env);
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 20bfb59..b01c717 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -17,6 +17,7 @@ uint32_t kvmppc_get_tbfreq(void);
>  uint64_t kvmppc_get_clockfreq(void);
>  uint32_t kvmppc_get_vmx(void);
>  uint32_t kvmppc_get_dfp(void);
> +bool kvmppc_get_htm_support(CPUPPCState *env);
>  bool kvmppc_get_host_model(char **buf);
>  bool kvmppc_get_host_serial(char **buf);
>  int kvmppc_get_hasidle(CPUPPCState *env);
> @@ -90,6 +91,11 @@ static inline uint32_t kvmppc_get_dfp(void)
>      return 0;
>  }
>  
> +static inline bool kvmppc_get_htm_support(KVMState *kvm_state)
> +{
> +    return false;
> +}
> +
>  static inline int kvmppc_get_hasidle(CPUPPCState *env)
>  {
>      return 0;

-- 
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/2] Add PowerPC AT_HWCAP2 definitions

[David Gibson]

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

On Tue, Jun 07, 2016 at 10:28:42PM +1000, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> We need the PPC_FEATURE2_HAS_HTM bit in a subsequent patch, so
> add the PowerPC AT_HWCAP2 definitions.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>

Applied to ppc-for-2.7.

Paolo or Peter: since this is a change to PPC specific bits, it seems
reasonable to go through my tree although it's technically a generic
header.  If someone wants to drop an explicit Ack, that wouldn't hurt
of course.

> ---
> 
> diff --git a/include/elf.h b/include/elf.h
> index 28d448b..8533b2a 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -477,6 +477,19 @@ typedef struct {
>  #define PPC_FEATURE_TRUE_LE             0x00000002
>  #define PPC_FEATURE_PPC_LE              0x00000001
>  
> +/* Bits present in AT_HWCAP2 for PowerPC.  */
> +
> +#define PPC_FEATURE2_ARCH_2_07          0x80000000
> +#define PPC_FEATURE2_HAS_HTM            0x40000000
> +#define PPC_FEATURE2_HAS_DSCR           0x20000000
> +#define PPC_FEATURE2_HAS_EBB            0x10000000
> +#define PPC_FEATURE2_HAS_ISEL           0x08000000
> +#define PPC_FEATURE2_HAS_TAR            0x04000000
> +#define PPC_FEATURE2_HAS_VEC_CRYPTO     0x02000000
> +#define PPC_FEATURE2_HTM_NOSC           0x01000000
> +#define PPC_FEATURE2_ARCH_3_00          0x00800000
> +#define PPC_FEATURE2_HAS_IEEE128        0x00400000
> +
>  /* Bits present in AT_HWCAP for Sparc.  */
>  
>  #define HWCAP_SPARC_FLUSH               0x00000001
> 

-- 
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] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[Paul Mackerras]

On Mon, Apr 04, 2016 at 09:09:28PM +1000, Anton Blanchard wrote:
> We don't support transactional memory in PR KVM, so don't tell
> the OS that we do.

This assumes PR KVM won't ever support TM, which is hopefully not
true.  If PR KVM does get TM support in future, then QEMU will have no
clear way to know whether it needs to clear the pa-features bit or
not.  I think we need to define some way for the KVM implementation to
tell qemu which of these kinds of CPU features it supports.

However, we could defer implementing that mechanism until PR KVM does
get support for TM, I guess.  In that case this patch could go in now,
though it seems slightly icky to be using the pvinfo stuff to
distinguish PR from HV.

Paul.

Re: [Qemu-devel] [PATCH] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[David Gibson]

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

On Tue, Apr 05, 2016 at 12:12:01PM +1000, Paul Mackerras wrote:
> On Mon, Apr 04, 2016 at 09:09:28PM +1000, Anton Blanchard wrote:
> > We don't support transactional memory in PR KVM, so don't tell
> > the OS that we do.
> 
> This assumes PR KVM won't ever support TM, which is hopefully not
> true.  If PR KVM does get TM support in future, then QEMU will have no
> clear way to know whether it needs to clear the pa-features bit or
> not.  I think we need to define some way for the KVM implementation to
> tell qemu which of these kinds of CPU features it supports.

Yeah, I think we need some sort of capability flag for this.  We also
need to isolate this KVM capability testing better into the KVM code,
so we won't break things on TCG.

Speaking of which... I don't imagine we implement TM instructions in
TCG either, so we should probably make sure TM isn't advertised there
either.

> However, we could defer implementing that mechanism until PR KVM does
> get support for TM, I guess.  In that case this patch could go in now,
> though it seems slightly icky to be using the pvinfo stuff to
> distinguish PR from HV.

It is icky, but it's the best idiom we have for distinguishing PR
KVM.  We try to only do it as a fallback when we can't determine
availability of the specific feature based on a capability, though.

-- 
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] spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

[Alexey Kardashevskiy]

On 04/05/2016 02:09 PM, David Gibson wrote:
> On Tue, Apr 05, 2016 at 12:12:01PM +1000, Paul Mackerras wrote:
>> On Mon, Apr 04, 2016 at 09:09:28PM +1000, Anton Blanchard wrote:
>>> We don't support transactional memory in PR KVM, so don't tell
>>> the OS that we do.
>>
>> This assumes PR KVM won't ever support TM, which is hopefully not
>> true.  If PR KVM does get TM support in future, then QEMU will have no
>> clear way to know whether it needs to clear the pa-features bit or
>> not.  I think we need to define some way for the KVM implementation to
>> tell qemu which of these kinds of CPU features it supports.
>
> Yeah, I think we need some sort of capability flag for this.  We also
> need to isolate this KVM capability testing better into the KVM code,
> so we won't break things on TCG.
>
> Speaking of which... I don't imagine we implement TM instructions in
> TCG either, so we should probably make sure TM isn't advertised there
> either.

TM is "supported" in TCG:

56a846157 "target-ppc: Introduce TM Noops"
===
     Add degenerate implementations of the non-privileged Transactional
     Memory instructions tend., tabort*. and tsr.  This implementation
     simply checks the MSR[TM] bit and then sets CR0 to 0b0000.  This
     is a reasonable degenerate implementation since transactions are
     never allowed to begin and hence MSR[TS] is always 0b00.

     Signed-off-by: Tom Musta <tommusta@gmail.com>
     Signed-off-by: Alexander Graf <agraf@suse.de>
===


-- 
Alexey

[Qemu-devel] [PATCH] powerpc: Clear user CPU feature bits if TM is disabled at runtime

[Anton Blanchard]

In check_cpu_pa_features() we check a number of bits in the
ibm,pa-features array and set and clear CPU features based on what
we find. One of these bits is CPU_FTR_TM, the transactional memory
feature bit.

If this does disable TM at runtime, then we need to tell userspace
about it by clearing the user CPU feature bits.

Without this patch userspace processes will think they can execute
TM instructions and get killed when they try.

Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
---

Michael I've added stable here because I'm seeing this on a number
of distros and would like to get it backported, but I'll leave it up
to you if it should go there.

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index f98be83..98c6c86 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -822,4 +822,18 @@ static int __init disable_hardlockup_detector(void)
 	return 0;
 }
 early_initcall(disable_hardlockup_detector);
+
+static int __init update_cpu_user_features(void)
+{
+	/*
+	 * Firmware might have disabled TM by clearing the relevant
+	 * bit in the ibm,pa-features array. In this case we need to
+	 * tell userspace.
+	 */
+	if (!cpu_has_feature(CPU_FTR_TM))
+		cur_cpu_spec->cpu_user_features2 &= ~(PPC_FEATURE2_HTM|PPC_FEATURE2_HTM_NOSC);
+
+	return 0;
+}
+early_initcall(update_cpu_user_features);
 #endif

Re: [Qemu-devel] [PATCH] powerpc: Clear user CPU feature bits if TM is disabled at runtime

[David Gibson]

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

On Mon, Apr 04, 2016 at 09:11:12PM +1000, Anton Blanchard wrote:
> In check_cpu_pa_features() we check a number of bits in the
> ibm,pa-features array and set and clear CPU features based on what
> we find. One of these bits is CPU_FTR_TM, the transactional memory
> feature bit.
> 
> If this does disable TM at runtime, then we need to tell userspace
> about it by clearing the user CPU feature bits.
> 
> Without this patch userspace processes will think they can execute
> TM instructions and get killed when they try.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> Cc: stable@vger.kernel.org
> ---
> 
> Michael I've added stable here because I'm seeing this on a number
> of distros and would like to get it backported, but I'll leave it up
> to you if it should go there.
> 
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index f98be83..98c6c86 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -822,4 +822,18 @@ static int __init disable_hardlockup_detector(void)
>  	return 0;
>  }
>  early_initcall(disable_hardlockup_detector);
> +
> +static int __init update_cpu_user_features(void)
> +{
> +	/*
> +	 * Firmware might have disabled TM by clearing the relevant
> +	 * bit in the ibm,pa-features array. In this case we need to
> +	 * tell userspace.
> +	 */
> +	if (!cpu_has_feature(CPU_FTR_TM))
> +		cur_cpu_spec->cpu_user_features2 &= ~(PPC_FEATURE2_HTM|PPC_FEATURE2_HTM_NOSC);
> +
> +	return 0;
> +}
> +early_initcall(update_cpu_user_features);
>  #endif
> 

-- 
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] powerpc: Clear user CPU feature bits if TM is disabled at runtime

[Michael Ellerman]

On Mon, 2016-04-04 at 11:11:12 UTC, Paul Mackerras via Linuxppc-dev wrote:
> In check_cpu_pa_features() we check a number of bits in the

Shouldn't we be clearing the user feature there too?

The ibm_pa_features array and the logic in scan_features() knows to flip the
cpu_user_features bits, it was just never updated to handle cpu_user_features2.

So it seems to me that's where the bug is.

> ibm,pa-features array and set and clear CPU features based on what
> we find. One of these bits is CPU_FTR_TM, the transactional memory
> feature bit.
> 
> If this does disable TM at runtime, then we need to tell userspace
> about it by clearing the user CPU feature bits.
> 
> Without this patch userspace processes will think they can execute
> TM instructions and get killed when they try.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 
> Michael I've added stable here because I'm seeing this on a number
> of distros and would like to get it backported, but I'll leave it up
> to you if it should go there.

Yeah it should definitely go to stable. Can we pinpoint which commit introduced
the bug, I guess whenever the TM support was merged.

cheers

Re: [Qemu-devel] powerpc: Clear user CPU feature bits if TM is disabled at runtime

[Benjamin Herrenschmidt]

On Tue, 2016-04-05 at 19:35 +1000, Michael Ellerman wrote:
> Shouldn't we be clearing the user feature there too?
> 
> The ibm_pa_features array and the logic in scan_features() knows to
> flip the
> cpu_user_features bits, it was just never updated to handle
> cpu_user_features2.
> 
> So it seems to me that's where the bug is.

I was about to make the same comment but then realized we are trying to
clear *2* bits. And since that logic will also, I think, set the bits
when the corresponding pa-feature is present, it means we will also set
those 2 bits if we put both in the mask...

Not sure that's quite what we want, but then I'm not sure what the 2
bits are about, which is why I postponed commenting :-)

Cheers,
Ben.

Re: [Qemu-devel] powerpc: Clear user CPU feature bits if TM is disabled at runtime

[Michael Ellerman]

On 5 April 2016 7:56:23 pm AEST, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>On Tue, 2016-04-05 at 19:35 +1000, Michael Ellerman wrote:
>> Shouldn't we be clearing the user feature there too?
>> 
>> The ibm_pa_features array and the logic in scan_features() knows to
>> flip the
>> cpu_user_features bits, it was just never updated to handle
>> cpu_user_features2.
>> 
>> So it seems to me that's where the bug is.
>
>I was about to make the same comment but then realized we are trying to
>clear *2* bits. And since that logic will also, I think, set the bits
>when the corresponding pa-feature is present, it means we will also set
>those 2 bits if we put both in the mask...

That's what we want in this case. The 2nd bit describes Linux's behaviour of aborting active transactions in syscalls, so kernels that have that bit defined should be setting/clearing it in lock step with the main TM bit. 

cheers
-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[Qemu-devel] [PATCH 1/3] powerpc: scan_features() updates incorrect bits

[Anton Blanchard]

The real LE feature entry in the ibm_pa_feature struct has the
wrong number of elements. Instead of checking for byte 5, bit 0,
we check for byte 0, bit 0, and we also incorrectly update cpu user
feature bit 5.

Fixes: 44ae3ab3358e ("powerpc: Free up some CPU feature bits by moving out MMU-related features")
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
---
 arch/powerpc/kernel/prom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 7030b03..9a3a7c6 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -158,7 +158,7 @@ static struct ibm_pa_feature {
 	{CPU_FTR_NOEXECUTE, 0, 0,	0, 6, 0},
 	{CPU_FTR_NODSISRALIGN, 0, 0,	1, 1, 1},
 	{0, MMU_FTR_CI_LARGE_PAGE, 0,	1, 2, 0},
-	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0, 0},
+	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 5, 0, 0},
 	/*
 	 * If the kernel doesn't support TM (ie. CONFIG_PPC_TRANSACTIONAL_MEM=n),
 	 * we don't want to turn on CPU_FTR_TM here, so we use CPU_FTR_TM_COMP
-- 
2.7.4

Re: [Qemu-devel] [1/3] powerpc: scan_features() updates incorrect bits

[Michael Ellerman]

On Fri, 2016-15-04 at 02:06:13 UTC, Unknown sender due to SPF wrote:
> The real LE feature entry in the ibm_pa_feature struct has the
> wrong number of elements. Instead of checking for byte 5, bit 0,
> we check for byte 0, bit 0, and we also incorrectly update cpu user
> feature bit 5.
>
> Fixes: 44ae3ab3358e ("powerpc: Free up some CPU feature bits by moving out MMU-related features")

So pulling that apart a bit.

> -	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5,         0,   0},   (invert 0)
	 ^                ^                    ^          ^    ^
	 cpu feat	  mmu feat	       user feat  byte bit

By checking byte 0 bit 0 (IBM numbering), means we're looking at the "Memory
Management Unit (MMU)" feature - ie. does the CPU have an MMU.

As you'd expect that bit is set on most platforms we care about. I see it set on
a P6 & P7 here running PowerVM, as well as by recent Qemu on P8.

Which means on all those systems we'll be enabling those bits.

As far as the cpu feature bits:

  #define CPU_FTR_REAL_LE			0x00400000

On a guest running on P8 on older qemu which has no ibm,pa-features property I
see:

  cpu_features      = 0x17fc7aec18500249
                                  ^

So it's set, but not by this logic, because it's already in cpu_features for P8.

On Power6, which does have the ibm,pa-feature property with bit 0,0 set:

  cpu_features      = 0x090d1ae018500049
                                  ^

Also set, from the property, but it's also in the Power6 mask by default. So all
we've lost there is the ability to turn it off.


For the MMU feature, we're setting:

  #define PPC_FEATURE_TRUE_LE		0x00000002

Which matches:

  #define MMU_FTR_TYPE_8xx		0x00000002

And on the Power6 I do indeed see:

  mmu_features      = 0x7c000003

Which says I have MMU_FTR_HPTE_TABLE and MMU_FTR_TYPE_8xx.

Luckily it looks like the only place that looks at MMU_FTR_TYPE_8xx is in Book3E
code, so will never be affected by this bug.


Finally the user feature, we're setting 5, ie. 0x4 | 0x1, which is:

#define PPC_FEATURE_PPC_LE		0x00000001

And nothing else, 0x4 is free.

On the Power6, I can see it in /proc/pid/auxv:

00000060  00 00 00 00 00 00 00 10  00 00 00 00 dc 00 74 47  |..............tG|
							 ^
							 0x4 | 0x2 | 0x1

LD_SHOW_AUXV=1 doesn't show it, but that's just because my glibc is old.


Net result:
 - on P6 & P7 (& P8 with newer Qemu) we can't disable CPU_FTR_REAL_LE - but
   luckily we've never wanted to.
 - we're stuffing up mmu_features but it doesn't matter
 - we're advertising PPC_LE when we shouldn't be, but *probably* nothing cares.
 - we're advertising 0x4 in HWCAP which is undefined, but *almost certainly*
   nothing cares.

So that could have been a doozy but turns out not actually that bad. Phew :)

cheers


> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -158,7 +158,7 @@ static struct ibm_pa_feature {
>  	{CPU_FTR_NOEXECUTE, 0, 0,	0, 6, 0},
>  	{CPU_FTR_NODSISRALIGN, 0, 0,	1, 1, 1},
>  	{0, MMU_FTR_CI_LARGE_PAGE, 0,	1, 2, 0},
> -	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0, 0},
> +	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 5, 0, 0},
>  	/*
>  	 * If the kernel doesn't support TM (ie. CONFIG_PPC_TRANSACTIONAL_MEM=n),
>  	 * we don't want to turn on CPU_FTR_TM here, so we use CPU_FTR_TM_COMP

Re: [Qemu-devel] [1/3] powerpc: scan_features() updates incorrect bits

[Michael Ellerman]

On Sat, 2016-04-16 at 00:27 +1000, Michael Ellerman wrote:
> On Fri, 2016-15-04 at 02:06:13 UTC, Unknown sender due to SPF wrote:
> > The real LE feature entry in the ibm_pa_feature struct has the
> > wrong number of elements. Instead of checking for byte 5, bit 0,
> > we check for byte 0, bit 0, and we also incorrectly update cpu user
> > feature bit 5.
> 
> Finally the user feature, we're setting 5, ie. 0x4 | 0x1, which is:
> 
> #define PPC_FEATURE_PPC_LE		0x00000001
> 
> And nothing else, 0x4 is free.

Buuuut, we should reserve 0x4 for the foreseeable future. Because it is
erroneously set on older kernels.

cheers

Re: [Qemu-devel] [1/3] powerpc: scan_features() updates incorrect bits

[Michael Ellerman]

On Fri, 2016-15-04 at 02:06:13 UTC, Unknown sender due to SPF wrote:
> The real LE feature entry in the ibm_pa_feature struct has the
> wrong number of elements. Instead of checking for byte 5, bit 0,
> we check for byte 0, bit 0, and we also incorrectly update cpu user
> feature bit 5.
> 
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 7030b03..9a3a7c6 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -158,7 +158,7 @@ static struct ibm_pa_feature {
>  	{CPU_FTR_NOEXECUTE, 0, 0,	0, 6, 0},
>  	{CPU_FTR_NODSISRALIGN, 0, 0,	1, 1, 1},
>  	{0, MMU_FTR_CI_LARGE_PAGE, 0,	1, 2, 0},
> -	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0, 0},
> +	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 5, 0, 0},

This should be:

> +	{CPU_FTR_REAL_LE, 0, PPC_FEATURE_TRUE_LE, 5, 0, 0},

Because the struct layout is:

static struct ibm_pa_feature {
	unsigned long	cpu_features;	/* CPU_FTR_xxx bit */
	unsigned long	mmu_features;	/* MMU_FTR_xxx bit */
	unsigned int	cpu_user_ftrs;	/* PPC_FEATURE_xxx bit */
	unsigned int	cpu_user_ftrs2;	/* PPC_FEATURE2_xxx bit */
	unsigned char	pabyte;		/* byte number in ibm,pa-features */
	unsigned char	pabit;		/* bit number (big-endian) */
	unsigned char	invert;		/* if 1, pa bit set => clear feature */
}


I'll fix it up locally.

cheers

[Qemu-devel] [PATCH v2 1/3] powerpc: scan_features() updates incorrect bits for REAL_LE

[Michael Ellerman]

From: Anton Blanchard <anton@samba.org>

The REAL_LE feature entry in the ibm_pa_feature struct is missing an MMU
feature value, meaning all the remaining elements initialise the wrong
values.

This means instead of checking for byte 5, bit 0, we check for byte 0,
bit 0, and then we incorrectly set the CPU feature bit as well as MMU
feature bit 1 and CPU user feature bits 0 and 2 (5).

Checking byte 0 bit 0 (IBM numbering), means we're looking at the
"Memory Management Unit (MMU)" feature - ie. does the CPU have an MMU.
In practice that bit is set on all platforms which have the property.

This means we set CPU_FTR_REAL_LE always. In practice that seems not to
matter because all the modern cpus which have this property also
implement REAL_LE, and we've never needed to disable it.

We're also incorrectly setting MMU feature bit 1, which is:

  #define MMU_FTR_TYPE_8xx		0x00000002

Luckily the only place that looks for MMU_FTR_TYPE_8xx is in Book3E
code, which can't run on the same cpus as scan_features(). So this also
doesn't matter in practice.

Finally in the CPU user feature mask, we're setting bits 0 and 2. Bit 2
is not currently used, and bit 0 is:

  #define PPC_FEATURE_PPC_LE		0x00000001

Which says the CPU supports the old style "PPC Little Endian" mode.
Again this should be harmless in practice as no 64-bit CPUs implement
that mode.

Fix the code by adding the missing initialisation of the MMU feature.

Also add a comment marking CPU user feature bit 2 (0x4) as reserved. It
would be unsafe to start using it as old kernels incorrectly set it.

Fixes: 44ae3ab3358e ("powerpc: Free up some CPU feature bits by moving out MMU-related features")
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
[mpe: Flesh out changelog, add comment reserving 0x4]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/uapi/asm/cputable.h | 1 +
 arch/powerpc/kernel/prom.c               | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/uapi/asm/cputable.h b/arch/powerpc/include/uapi/asm/cputable.h
index 8dde19962a5b..f63c96cd3608 100644
--- a/arch/powerpc/include/uapi/asm/cputable.h
+++ b/arch/powerpc/include/uapi/asm/cputable.h
@@ -31,6 +31,7 @@
 #define PPC_FEATURE_PSERIES_PERFMON_COMPAT \
 					0x00000040
 
+/* Reserved - do not use		0x00000004 */
 #define PPC_FEATURE_TRUE_LE		0x00000002
 #define PPC_FEATURE_PPC_LE		0x00000001
 
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 7030b035905d..080c96b44a7f 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -158,7 +158,7 @@ static struct ibm_pa_feature {
 	{CPU_FTR_NOEXECUTE, 0, 0,	0, 6, 0},
 	{CPU_FTR_NODSISRALIGN, 0, 0,	1, 1, 1},
 	{0, MMU_FTR_CI_LARGE_PAGE, 0,	1, 2, 0},
-	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0, 0},
+	{CPU_FTR_REAL_LE, 0, PPC_FEATURE_TRUE_LE, 5, 0, 0},
 	/*
 	 * If the kernel doesn't support TM (ie. CONFIG_PPC_TRANSACTIONAL_MEM=n),
 	 * we don't want to turn on CPU_FTR_TM here, so we use CPU_FTR_TM_COMP
-- 
2.5.0

Re: [Qemu-devel] [v2, 1/3] powerpc: scan_features() updates incorrect bits for REAL_LE

[Michael Ellerman]

On Mon, 2016-18-04 at 10:36:07 UTC, Michael Ellerman wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> The REAL_LE feature entry in the ibm_pa_feature struct is missing an MMU
> feature value, meaning all the remaining elements initialise the wrong
> values.
...
> 
> Fix the code by adding the missing initialisation of the MMU feature.
> 
> Also add a comment marking CPU user feature bit 2 (0x4) as reserved. It
> would be unsafe to start using it as old kernels incorrectly set it.
> 
> Fixes: 44ae3ab3358e ("powerpc: Free up some CPU feature bits by moving out MMU-related features")
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Cc: stable@vger.kernel.org
> [mpe: Flesh out changelog, add comment reserving 0x4]
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Applied to powerpc fixes.

https://git.kernel.org/powerpc/c/6997e57d693b07289694239e52

cheers

[Qemu-devel] [PATCH 2/3] powerpc: Update cpu_user_features2 in scan_features()

[Anton Blanchard]

scan_features() updates cpu_user_features but not cpu_user_features2.

Amongst other things, cpu_user_features2 contains the user TM feature
bits which we must keep in sync with the kernel TM feature bit.

Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
---
 arch/powerpc/kernel/prom.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 9a3a7c6..99709bb 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -148,23 +148,24 @@ static struct ibm_pa_feature {
 	unsigned long	cpu_features;	/* CPU_FTR_xxx bit */
 	unsigned long	mmu_features;	/* MMU_FTR_xxx bit */
 	unsigned int	cpu_user_ftrs;	/* PPC_FEATURE_xxx bit */
+	unsigned int	cpu_user_ftrs2;	/* PPC_FEATURE2_xxx bit */
 	unsigned char	pabyte;		/* byte number in ibm,pa-features */
 	unsigned char	pabit;		/* bit number (big-endian) */
 	unsigned char	invert;		/* if 1, pa bit set => clear feature */
 } ibm_pa_features[] __initdata = {
-	{0, 0, PPC_FEATURE_HAS_MMU,	0, 0, 0},
-	{0, 0, PPC_FEATURE_HAS_FPU,	0, 1, 0},
-	{CPU_FTR_CTRL, 0, 0,		0, 3, 0},
-	{CPU_FTR_NOEXECUTE, 0, 0,	0, 6, 0},
-	{CPU_FTR_NODSISRALIGN, 0, 0,	1, 1, 1},
-	{0, MMU_FTR_CI_LARGE_PAGE, 0,	1, 2, 0},
-	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 5, 0, 0},
+	{0, 0, PPC_FEATURE_HAS_MMU, 0,		0, 0, 0},
+	{0, 0, PPC_FEATURE_HAS_FPU, 0,		0, 1, 0},
+	{CPU_FTR_CTRL, 0, 0, 0,			0, 3, 0},
+	{CPU_FTR_NOEXECUTE, 0, 0, 0,		0, 6, 0},
+	{CPU_FTR_NODSISRALIGN, 0, 0, 0,		1, 1, 1},
+	{0, MMU_FTR_CI_LARGE_PAGE, 0, 0,		1, 2, 0},
+	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 0, 5, 0, 0},
 	/*
 	 * If the kernel doesn't support TM (ie. CONFIG_PPC_TRANSACTIONAL_MEM=n),
 	 * we don't want to turn on CPU_FTR_TM here, so we use CPU_FTR_TM_COMP
 	 * which is 0 if the kernel doesn't support TM.
 	 */
-	{CPU_FTR_TM_COMP, 0, 0,		22, 0, 0},
+	{CPU_FTR_TM_COMP, 0, 0, 0,		22, 0, 0},
 };
 
 static void __init scan_features(unsigned long node, const unsigned char *ftrs,
@@ -195,10 +196,12 @@ static void __init scan_features(unsigned long node, const unsigned char *ftrs,
 		if (bit ^ fp->invert) {
 			cur_cpu_spec->cpu_features |= fp->cpu_features;
 			cur_cpu_spec->cpu_user_features |= fp->cpu_user_ftrs;
+			cur_cpu_spec->cpu_user_features2 |= fp->cpu_user_ftrs2;
 			cur_cpu_spec->mmu_features |= fp->mmu_features;
 		} else {
 			cur_cpu_spec->cpu_features &= ~fp->cpu_features;
 			cur_cpu_spec->cpu_user_features &= ~fp->cpu_user_ftrs;
+			cur_cpu_spec->cpu_user_features2 &= ~fp->cpu_user_ftrs2;
 			cur_cpu_spec->mmu_features &= ~fp->mmu_features;
 		}
 	}
-- 
2.7.4

Re: [Qemu-devel] [2/3] powerpc: Update cpu_user_features2 in scan_features()

[Michael Ellerman]

On Fri, 2016-15-04 at 02:07:24 UTC, Unknown sender due to SPF wrote:
> scan_features() updates cpu_user_features but not cpu_user_features2.
> 
> Amongst other things, cpu_user_features2 contains the user TM feature
> bits which we must keep in sync with the kernel TM feature bit.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Cc: stable@vger.kernel.org

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/beff82374b259d726e2625ec6c

cheers

[Qemu-devel] [PATCH 3/3] powerpc: Update TM user feature bits in scan_features()

[Anton Blanchard]

We need to update the user TM feature bits (PPC_FEATURE2_HTM and
PPC_FEATURE2_HTM) to mirror what we do with the kernel TM feature
bit.

At the moment, if firmware reports TM is not available we turn off
the kernel TM feature bit but leave the userspace ones on. Userspace
thinks it can execute TM instructions and it dies trying.

This (together with a QEMU patch) fixes PR KVM, which doesn't currently
support TM.

Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: stable@vger.kernel.org
---
 arch/powerpc/kernel/prom.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 99709bb..5beffd7 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -161,11 +161,12 @@ static struct ibm_pa_feature {
 	{0, MMU_FTR_CI_LARGE_PAGE, 0, 0,		1, 2, 0},
 	{CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 0, 5, 0, 0},
 	/*
-	 * If the kernel doesn't support TM (ie. CONFIG_PPC_TRANSACTIONAL_MEM=n),
-	 * we don't want to turn on CPU_FTR_TM here, so we use CPU_FTR_TM_COMP
-	 * which is 0 if the kernel doesn't support TM.
+	 * If the kernel doesn't support TM (ie CONFIG_PPC_TRANSACTIONAL_MEM=n),
+	 * we don't want to turn on TM here, so we use the *_COMP versions
+	 * which are 0 if the kernel doesn't support TM.
 	 */
-	{CPU_FTR_TM_COMP, 0, 0, 0,		22, 0, 0},
+	{CPU_FTR_TM_COMP, 0, 0,
+	 PPC_FEATURE2_HTM_COMP|PPC_FEATURE2_HTM_NOSC_COMP, 22, 0, 0},
 };
 
 static void __init scan_features(unsigned long node, const unsigned char *ftrs,
-- 
2.7.4

Re: [Qemu-devel] [3/3] powerpc: Update TM user feature bits in scan_features()

[Michael Ellerman]

On Fri, 2016-15-04 at 02:08:19 UTC, Unknown sender due to SPF wrote:
> We need to update the user TM feature bits (PPC_FEATURE2_HTM and
> PPC_FEATURE2_HTM) to mirror what we do with the kernel TM feature
> bit.
> 
> At the moment, if firmware reports TM is not available we turn off
> the kernel TM feature bit but leave the userspace ones on. Userspace
> thinks it can execute TM instructions and it dies trying.
> 
> This (together with a QEMU patch) fixes PR KVM, which doesn't currently
> support TM.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Cc: stable@vger.kernel.org

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/4705e02498d6d5a7ab98dfee95

cheers