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 |
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.
> > 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
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
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
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
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
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.
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
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 --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"); }