[MODERATED] Re: [PATCH v5 04/11] TAAv5 4

[Andy Lutomirski <luto@amacapital.net>]

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

On 10/4/19 11:29 PM, speck for Pawan Gupta wrote:
> Add kernel cmdline parameter "tsx" to control the Transactional
> Synchronization Extensions (TSX) feature.  On CPUs that support TSX
> control, use "tsx=on|off" to enable or disable TSX.  Not specifying this
> option is equivalent to "tsx=off".
> 
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Reviewed-by: Mark Gross <mgross@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Tested-by: Neelima Krishnan <neelima.krishnan@intel.com>
> ---
>  .../admin-guide/kernel-parameters.txt         | 11 +++
>  arch/x86/kernel/cpu/tsx.c                     | 97 ++++++++++++++++---
>  2 files changed, 95 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4c1971960afa..832537d59562 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4813,6 +4813,17 @@
>  			interruptions from clocksource watchdog are not
>  			acceptable).
>  
> +	tsx=		[X86] Control Transactional Synchronization
> +			Extensions (TSX) feature in Intel processors that
> +			support TSX control.
> +
> +			This parameter controls the TSX feature. The options are:
> +
> +			on	- Enable TSX on the system.
> +			off	- Disable TSX on the system.
> +
> +			Not specifying this option is equivalent to tsx=off.
> +
>  	turbografx.map[2|3]=	[HW,JOY]
>  			TurboGraFX parallel port interface
>  			Format:
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index c549750dd7c8..1c0cee7a7d46 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -19,6 +19,30 @@ static enum tsx_ctrl_states {
>  	TSX_CTRL_NOT_SUPPORTED,
>  } tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
>  
> +static enum tsx_user_cmds {
> +	TSX_USER_CMD_NONE,
> +	TSX_USER_CMD_ON,
> +	TSX_USER_CMD_OFF,
> +} tsx_user_cmd = TSX_USER_CMD_NONE;
> +
> +static int __init tsx_cmdline(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +
> +	/*
> +	 * tsx_en/disable() are only called when
> +	 * X86_FEATURE_RTM and TSX_CTRL MSR are supported.
> +	 */
> +	if (!strcmp(str, "on"))
> +		tsx_user_cmd = TSX_USER_CMD_ON;
> +	else if (!strcmp(str, "off"))
> +		tsx_user_cmd = TSX_USER_CMD_OFF;
> +
> +	return 0;
> +}
> +early_param("tsx", tsx_cmdline);
> +
>  static void tsx_disable(void)
>  {
>  	u64 tsx;
> @@ -38,6 +62,24 @@ static void tsx_disable(void)
>  	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
>  }
>  
> +static void tsx_enable(void)
> +{
> +	u64 tsx;
> +
> +	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
> +
> +	/* Enable the RTM feature in the cpu */
> +	tsx &= ~TSX_CTRL_RTM_DISABLE;
> +	/*
> +	 * Ensure TSX support is enumerated in CPUID.
> +	 * This is visible to userspace and will ensure they
> +	 * can enumerate and use the TSX feature.
> +	 */
> +	tsx &= ~TSX_CTRL_CPUID_CLEAR;
> +
> +	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
> +}
> +
>  static bool tsx_ctrl_is_supported(void)
>  {
>  	u64 ia32_cap = read_ia32_arch_cap();
> @@ -55,18 +97,47 @@ void tsx_init(struct cpuinfo_x86 *c)
>  	if (!tsx_ctrl_is_supported())
>  		return;
>  
> -	/*
> -	 * Default to TSX_CTRL_DISABLE. This is because on certain processors
> -	 * TSX may be used as part of a speculative side channel attack.
> -	 */
> -	tsx_ctrl_state = TSX_CTRL_DISABLE;
> +	switch (tsx_user_cmd) {
> +	case TSX_USER_CMD_ON:
> +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> +		break;
> +	case TSX_USER_CMD_OFF:
> +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> +		break;
> +	case TSX_USER_CMD_NONE:
> +	default:
> +		/*
> +		 * If user provided an invalid option or tsx= is not provided
> +		 * on cmdline default to TSX_CTRL_DISABLE. This is because on
> +		 * certain processors TSX may be used as part of a speculative
> +		 * side channel attack.
> +		 */
> +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> +	}

This code is bizarre.  Why are you separating tsx_ctrl_state from
tsx_user_cmd?  Just have one copy of this, please.  You shouldn't be
writing to a *global* cpu state variable like this in a percpu init
function.  This code should just program the MSR.

Also, the new improved just-one-enum-for-everything global variable
should be __ro_after_init.

Given that you are doing the percpu config for each cpu, setup_clear_...
should be unnecessary.  It's just a helper that automatically clears the
bit for other CPUs.