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

[David Gibson <david@gibson.dropbear.id.au>]

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