diff mbox

spapr: Don't set the TM ibm, pa-features bit in PR KVM mode

Message ID 20160404210928.0d9ae644@kryten (mailing list archive)
State New, archived
Headers show

Commit Message

Anton Blanchard April 4, 2016, 11:09 a.m. UTC
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>
---

Comments

Alexander Graf April 4, 2016, 11:13 a.m. UTC | #1
> 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;
Paul Mackerras April 5, 2016, 2:12 a.m. UTC | #2
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.
David Gibson April 5, 2016, 4:09 a.m. UTC | #3
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.
Alexey Kardashevskiy April 5, 2016, 7:33 a.m. UTC | #4
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>
===
diff mbox

Patch

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;