[MODERATED] Re: ***UNCHECKED*** [PATCH v5 08/11] TAAv5 8

On Fri 04-10-19 23:33:31, speck for Pawan Gupta wrote:
> Platforms which are not affected by X86_BUG_TAA may want the TSX feature
> enabled. Add "auto" option to the TSX cmdline parameter. When tsx=auto,
> disable TSX when X86_BUG_TAA is present, otherwise enable TSX.
> 
> More details on X86_BUG_TAA can be found here:
> https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html

This patch is still keeping the default for tsx=off unless I got lost in
the enable/disable logic, right?
The earlier arguments for going this way was that there are no real
users of TSX in production. This is not really the case though. At least
SAP HANA seems to benefit from TSX - publicly available information can
be found [1][2][3]. We have talked to SAP guys today and this is still the
case.

I do understand that there are likely not that many other production
users of the feature but I believe that we should keep the default
update friendly and sticking with the auto semantic by default is both
in line with other mitigations and also reduces the risk of regressions.

Thoughts?

[1] https://blogs.saphana.com/2015/05/05/new-intel-xeon-haswell-processor-delivers-exceptional-performance-sap-hana-platform-2/
[2] https://blogs.saphana.com/2015/06/29/impact-of-haswell-on-hana/
[3] https://www.intel.com/content/dam/www/public/us/en/documents/solution-briefs/sap-hana-real-time-analytics-solution-brief.pdf

> Signed-off-by: Pawan Gupta <[email protected]>
> Reviewed-by: Tony Luck <[email protected]>
> Tested-by: Neelima Krishnan <[email protected]>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 5 +++++
>  arch/x86/kernel/cpu/tsx.c                       | 9 +++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 832537d59562..d7b48c38c6e5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4821,6 +4821,11 @@
>  
>  			on	- Enable TSX on the system.
>  			off	- Disable TSX on the system.
> +			auto	- Disable TSX if X86_BUG_TAA is present,
> +				  otherwise enable TSX on the system.
> +
> +			More details on X86_BUG_TAA are here:
> +			Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
>  
>  			Not specifying this option is equivalent to tsx=off.
>  
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index 1c0cee7a7d46..73a0e5af3720 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -23,6 +23,7 @@ static enum tsx_user_cmds {
>  	TSX_USER_CMD_NONE,
>  	TSX_USER_CMD_ON,
>  	TSX_USER_CMD_OFF,
> +	TSX_USER_CMD_AUTO,
>  } tsx_user_cmd = TSX_USER_CMD_NONE;
>  
>  static int __init tsx_cmdline(char *str)
> @@ -38,6 +39,8 @@ static int __init tsx_cmdline(char *str)
>  		tsx_user_cmd = TSX_USER_CMD_ON;
>  	else if (!strcmp(str, "off"))
>  		tsx_user_cmd = TSX_USER_CMD_OFF;
> +	else if (!strcmp(str, "auto"))
> +		tsx_user_cmd = TSX_USER_CMD_AUTO;
>  
>  	return 0;
>  }
> @@ -104,6 +107,12 @@ void tsx_init(struct cpuinfo_x86 *c)
>  	case TSX_USER_CMD_OFF:
>  		tsx_ctrl_state = TSX_CTRL_DISABLE;
>  		break;
> +	case TSX_USER_CMD_AUTO:
> +		if (boot_cpu_has_bug(X86_BUG_TAA))
> +			tsx_ctrl_state = TSX_CTRL_DISABLE;
> +		else
> +			tsx_ctrl_state = TSX_CTRL_ENABLE;
> +		break;
>  	case TSX_USER_CMD_NONE:
>  	default:
>  		/*
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs