diff mbox series

[1/4] powerpc/tm: Add commandline option to disable hardware transactional memory

Message ID 1507803439-12862-1-git-send-email-mpe@ellerman.id.au (mailing list archive)
State Accepted
Commit 07fd1761e1cd2b5ecdb470fc228d52ccdf75e05b
Headers show
Series [1/4] powerpc/tm: Add commandline option to disable hardware transactional memory | expand

Commit Message

Michael Ellerman Oct. 12, 2017, 10:17 a.m. UTC 
From: Cyril Bur <cyrilbur@gmail.com>

Currently the kernel relies on firmware to inform it whether or not the
CPU supports HTM and as long as the kernel was built with
CONFIG_PPC_TRANSACTIONAL_MEM=y then it will allow userspace to make
use of the facility.

There may be situations where it would be advantageous for the kernel
to not allow userspace to use HTM, currently the only way to achieve
this is to recompile the kernel with CONFIG_PPC_TRANSACTIONAL_MEM=n.

This patch adds a simple commandline option so that HTM can be
disabled at boot time.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
[mpe: Simplify to a bool, move to prom.c, put doco in the right place.
 Always disable, regardless of initial state, to avoid user confusion.]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 arch/powerpc/kernel/prom.c                      | 31 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

Comments

Breno Leitao Oct. 20, 2017, 11:45 a.m. UTC | #1 
Mikey, Cyril,

On Thu, Oct 12, 2017 at 09:17:16PM +1100, Michael Ellerman wrote:
> From: Cyril Bur <cyrilbur@gmail.com>
> 
> Currently the kernel relies on firmware to inform it whether or not the
> CPU supports HTM and as long as the kernel was built with
> CONFIG_PPC_TRANSACTIONAL_MEM=y then it will allow userspace to make
> use of the facility.
> 
> There may be situations where it would be advantageous for the kernel
> to not allow userspace to use HTM, currently the only way to achieve
> this is to recompile the kernel with CONFIG_PPC_TRANSACTIONAL_MEM=n.
> 
> This patch adds a simple commandline option so that HTM can be
> disabled at boot time.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> [mpe: Simplify to a bool, move to prom.c, put doco in the right place.
>  Always disable, regardless of initial state, to avoid user confusion.]
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>  arch/powerpc/kernel/prom.c                      | 31 +++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 05496622b4ef..ef03e6e16bdb 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3185,6 +3185,10 @@
>  			allowed (eg kernel_enable_fpu()/kernel_disable_fpu()).
>  			There is some performance impact when enabling this.
>  
> +	ppc_tm=		[PPC]
> +			Format: {"off"}
> +			Disable Hardware Transactional Memory
> +
>  	print-fatal-signals=
>  			[KNL] debug: print fatal signals
>  
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index f83056297441..d9bd6555f980 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -658,6 +658,35 @@ static void __init early_reserve_mem(void)
>  #endif
>  }
>  
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +static bool tm_disabled __initdata;

I think the name 'tm_disabled' might cause more confusion on the TM
code. Mainly because we already have tm_enable() and tm_enabled()
functions which are related to the MSR register and TM bit, and, with
your new variable, tm_enabled() and tm_disabled are not going to be
exclusionary. Neither tm_enable() with be able to toggle the tm_disabled
value.

Anyway, just my 2 cents.
David Laight Oct. 20, 2017, 12:58 p.m. UTC | #2 
> > This patch adds a simple commandline option so that HTM can be
> > disabled at boot time.

ISTM that being able to disable it after boot would be more useful.
(ie in a startup script)

	David
Michael Neuling Oct. 21, 2017, 12:58 a.m. UTC | #3 
On Fri, 2017-10-20 at 09:45 -0200, Breno Leitao wrote:
> Mikey, Cyril,
> 
> On Thu, Oct 12, 2017 at 09:17:16PM +1100, Michael Ellerman wrote:
> > From: Cyril Bur <cyrilbur@gmail.com>
> > 
> > Currently the kernel relies on firmware to inform it whether or not the
> > CPU supports HTM and as long as the kernel was built with
> > CONFIG_PPC_TRANSACTIONAL_MEM=y then it will allow userspace to make
> > use of the facility.
> > 
> > There may be situations where it would be advantageous for the kernel
> > to not allow userspace to use HTM, currently the only way to achieve
> > this is to recompile the kernel with CONFIG_PPC_TRANSACTIONAL_MEM=n.
> > 
> > This patch adds a simple commandline option so that HTM can be
> > disabled at boot time.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > [mpe: Simplify to a bool, move to prom.c, put doco in the right place.
> >  Always disable, regardless of initial state, to avoid user confusion.]
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  4 ++++
> >  arch/powerpc/kernel/prom.c                      | 31
> > +++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 05496622b4ef..ef03e6e16bdb 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3185,6 +3185,10 @@
> >  			allowed (eg
> > kernel_enable_fpu()/kernel_disable_fpu()).
> >  			There is some performance impact when enabling
> > this.
> >  
> > +	ppc_tm=		[PPC]
> > +			Format: {"off"}
> > +			Disable Hardware Transactional Memory
> > +
> >  	print-fatal-signals=
> >  			[KNL] debug: print fatal signals
> >  
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index f83056297441..d9bd6555f980 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -658,6 +658,35 @@ static void __init early_reserve_mem(void)
> >  #endif
> >  }
> >  
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +static bool tm_disabled __initdata;
> 
> I think the name 'tm_disabled' might cause more confusion on the TM
> code. Mainly because we already have tm_enable() and tm_enabled()
> functions which are related to the MSR register and TM bit, and, with
> your new variable, tm_enabled() and tm_disabled are not going to be
> exclusionary. Neither tm_enable() with be able to toggle the tm_disabled
> value.

Got a proposal for better names?

Mikey
Michael Neuling Oct. 21, 2017, 1 a.m. UTC | #4 
On Fri, 2017-10-20 at 12:58 +0000, David Laight wrote:
> > > This patch adds a simple commandline option so that HTM can be
> > > disabled at boot time.
> 
> ISTM that being able to disable it after boot would be more useful.
> (ie in a startup script)

I agree bug unfortunately that's impossible.

If a process is already running in tm suspend, there is no way to stop it other
than killing the process.  At that point you may as well kexec with a new
cmdline option

Mikey
David Laight Oct. 23, 2017, 9:01 a.m. UTC | #5 
From: Michael Neuling

> Sent: 21 October 2017 02:00

> To: David Laight; 'Breno Leitao'; Michael Ellerman

> Cc: stewart@linux.vnet.ibm.com; linuxppc-dev@ozlabs.org; cyrilbur@gmail.com

> Subject: Re: [PATCH 1/4] powerpc/tm: Add commandline option to disable hardware transactional memory

> 

> On Fri, 2017-10-20 at 12:58 +0000, David Laight wrote:

> > > > This patch adds a simple commandline option so that HTM can be

> > > > disabled at boot time.

> >

> > ISTM that being able to disable it after boot would be more useful.

> > (ie in a startup script)

> 

> I agree bug unfortunately that's impossible.

> 

> If a process is already running in tm suspend, there is no way to stop it other

> than killing the process.  At that point you may as well kexec with a new

> cmdline option


Isn't that unlikely when the first rc scripts are being run?
Setting an early rc script is generally easier than adding a command line
parameter.

I don't know about ppc, but grub on x86 makes it painfully almost
impossible to have two boot entries that have different command line
options.

	David
Michael Neuling Oct. 23, 2017, 9:15 a.m. UTC | #6 
On Mon, 2017-10-23 at 09:01 +0000, David Laight wrote:
> From: Michael Neuling
> > Sent: 21 October 2017 02:00
> > To: David Laight; 'Breno Leitao'; Michael Ellerman
> > Cc: stewart@linux.vnet.ibm.com; linuxppc-dev@ozlabs.org; cyrilbur@gmail.com
> > Subject: Re: [PATCH 1/4] powerpc/tm: Add commandline option to disable
> > hardware transactional memory
> > 
> > On Fri, 2017-10-20 at 12:58 +0000, David Laight wrote:
> > > > > This patch adds a simple commandline option so that HTM can be
> > > > > disabled at boot time.
> > > 
> > > ISTM that being able to disable it after boot would be more useful.
> > > (ie in a startup script)
> > 
> > I agree bug unfortunately that's impossible.
> > 
> > If a process is already running in tm suspend, there is no way to stop it
> > other
> > than killing the process.  At that point you may as well kexec with a new
> > cmdline option
> 
> Isn't that unlikely when the first rc scripts are being run?
> Setting an early rc script is generally easier than adding a command line
> parameter.

Unlikely or not, it's a case we'd have to handle that would add significant
complexity compared to what's being proposed for (IMHO) little gain.

Also, this doesn't exclude that option later if we decided we could do it.

> I don't know about ppc, but grub on x86 makes it painfully almost
> impossible to have two boot entries that have different command line
> options.

Ok.

Mikey
Breno Leitao Oct. 23, 2017, 12:56 p.m. UTC | #7 
On Sat, Oct 21, 2017 at 11:58:47AM +1100, Michael Neuling wrote:
> On Fri, 2017-10-20 at 09:45 -0200, Breno Leitao wrote:
> > Mikey, Cyril,
> > 
> > On Thu, Oct 12, 2017 at 09:17:16PM +1100, Michael Ellerman wrote:
> > > From: Cyril Bur <cyrilbur@gmail.com>
> > > 
> > > Currently the kernel relies on firmware to inform it whether or not the
> > > CPU supports HTM and as long as the kernel was built with
> > > CONFIG_PPC_TRANSACTIONAL_MEM=y then it will allow userspace to make
> > > use of the facility.
> > > 
> > > There may be situations where it would be advantageous for the kernel
> > > to not allow userspace to use HTM, currently the only way to achieve
> > > this is to recompile the kernel with CONFIG_PPC_TRANSACTIONAL_MEM=n.
> > > 
> > > This patch adds a simple commandline option so that HTM can be
> > > disabled at boot time.
> > > 
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > [mpe: Simplify to a bool, move to prom.c, put doco in the right place.
> > >  Always disable, regardless of initial state, to avoid user confusion.]
> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > > ---
> > >  Documentation/admin-guide/kernel-parameters.txt |  4 ++++
> > >  arch/powerpc/kernel/prom.c                      | 31
> > > +++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > > b/Documentation/admin-guide/kernel-parameters.txt
> > > index 05496622b4ef..ef03e6e16bdb 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3185,6 +3185,10 @@
> > >  			allowed (eg
> > > kernel_enable_fpu()/kernel_disable_fpu()).
> > >  			There is some performance impact when enabling
> > > this.
> > >  
> > > +	ppc_tm=		[PPC]
> > > +			Format: {"off"}
> > > +			Disable Hardware Transactional Memory
> > > +
> > >  	print-fatal-signals=
> > >  			[KNL] debug: print fatal signals
> > >  
> > > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > > index f83056297441..d9bd6555f980 100644
> > > --- a/arch/powerpc/kernel/prom.c
> > > +++ b/arch/powerpc/kernel/prom.c
> > > @@ -658,6 +658,35 @@ static void __init early_reserve_mem(void)
> > >  #endif
> > >  }
> > >  
> > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > > +static bool tm_disabled __initdata;
> > 
> > I think the name 'tm_disabled' might cause more confusion on the TM
> > code. Mainly because we already have tm_enable() and tm_enabled()
> > functions which are related to the MSR register and TM bit, and, with
> > your new variable, tm_enabled() and tm_disabled are not going to be
> > exclusionary. Neither tm_enable() with be able to toggle the tm_disabled
> > value.
> 
> Got a proposal for better names?

That is the hardest part, but I thought about something as:

 * tm_disabled_on_boot
 * tm_off
 * tm_explicit_disabled
 * tm_feature_disabled
 * tm_enablement_disabled

I think these names, although a bit longer, might avoid the confusion
with tm_enable/tm_enabled nomenclature.
Michael Ellerman Oct. 24, 2017, 8:08 a.m. UTC | #8 
On Thu, 2017-10-12 at 10:17:16 UTC, Michael Ellerman wrote:
> From: Cyril Bur <cyrilbur@gmail.com>
> 
> Currently the kernel relies on firmware to inform it whether or not the
> CPU supports HTM and as long as the kernel was built with
> CONFIG_PPC_TRANSACTIONAL_MEM=y then it will allow userspace to make
> use of the facility.
> 
> There may be situations where it would be advantageous for the kernel
> to not allow userspace to use HTM, currently the only way to achieve
> this is to recompile the kernel with CONFIG_PPC_TRANSACTIONAL_MEM=n.
> 
> This patch adds a simple commandline option so that HTM can be
> disabled at boot time.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> [mpe: Simplify to a bool, move to prom.c, put doco in the right place.
>  Always disable, regardless of initial state, to avoid user confusion.]
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Series applied to powerpc next.

https://git.kernel.org/powerpc/c/07fd1761e1cd2b5ecdb470fc228d52

cheers
Michael Ellerman Oct. 24, 2017, 8:12 a.m. UTC | #9 
Breno Leitao <leitao@debian.org> writes:
> On Sat, Oct 21, 2017 at 11:58:47AM +1100, Michael Neuling wrote:
>> On Fri, 2017-10-20 at 09:45 -0200, Breno Leitao wrote:
>> > On Thu, Oct 12, 2017 at 09:17:16PM +1100, Michael Ellerman wrote:
>> > > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> > > index f83056297441..d9bd6555f980 100644
>> > > --- a/arch/powerpc/kernel/prom.c
>> > > +++ b/arch/powerpc/kernel/prom.c
>> > > @@ -658,6 +658,35 @@ static void __init early_reserve_mem(void)
>> > >  #endif
>> > >  }
>> > >  
>> > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> > > +static bool tm_disabled __initdata;
>> > 
>> > I think the name 'tm_disabled' might cause more confusion on the TM
>> > code. Mainly because we already have tm_enable() and tm_enabled()
>> > functions which are related to the MSR register and TM bit, and, with
>> > your new variable, tm_enabled() and tm_disabled are not going to be
>> > exclusionary. Neither tm_enable() with be able to toggle the tm_disabled
>> > value.

I've merged it with that name, but I'm happy to take an incremental
patch to give it a better name.

>> Got a proposal for better names?
>
> That is the hardest part, but I thought about something as:
>
>  * tm_disabled_on_boot

Maybe.

>  * tm_off

Nah.

>  * tm_explicit_disabled

Maybe.

>  * tm_feature_disabled

No that's not quite accurate.

>  * tm_enablement_disabled

I refuse to use "enablement" ;)

> I think these names, although a bit longer, might avoid the confusion
> with tm_enable/tm_enabled nomenclature.

tm_cmdline_disabled would be OK. Or tm_force_disable, or ...

cheers
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 05496622b4ef..ef03e6e16bdb 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3185,6 +3185,10 @@ 
 			allowed (eg kernel_enable_fpu()/kernel_disable_fpu()).
 			There is some performance impact when enabling this.
 
+	ppc_tm=		[PPC]
+			Format: {"off"}
+			Disable Hardware Transactional Memory
+
 	print-fatal-signals=
 			[KNL] debug: print fatal signals
 
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index f83056297441..d9bd6555f980 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -658,6 +658,35 @@  static void __init early_reserve_mem(void)
 #endif
 }
 
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+static bool tm_disabled __initdata;
+
+static int __init parse_ppc_tm(char *str)
+{
+	bool res;
+
+	if (kstrtobool(str, &res))
+		return -EINVAL;
+
+	tm_disabled = !res;
+
+	return 0;
+}
+early_param("ppc_tm", parse_ppc_tm);
+
+static void __init tm_init(void)
+{
+	if (tm_disabled) {
+		pr_info("Disabling hardware transactional memory (HTM)\n");
+		cur_cpu_spec->cpu_user_features2 &=
+			~(PPC_FEATURE2_HTM_NOSC | PPC_FEATURE2_HTM);
+		cur_cpu_spec->cpu_features &= ~CPU_FTR_TM;
+	}
+}
+#else
+static void tm_init(void) { }
+#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
+
 void __init early_init_devtree(void *params)
 {
 	phys_addr_t limit;
@@ -767,6 +796,8 @@  void __init early_init_devtree(void *params)
 		powerpc_firmware_features |= FW_FEATURE_PS3_POSSIBLE;
 #endif
 
+	tm_init();
+
 	DBG(" <- early_init_devtree()\n");
 }