powerpc: Clear user CPU feature bits if TM is disabled at runtime

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.

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
>

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

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.

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