[3/4] powerpc/powernv: Enable TM without suspend if possible

Message ID 1507803439-12862-3-git-send-email-mpe@ellerman.id.au
State New
Headers show
Series
  • [1/4] powerpc/tm: Add commandline option to disable hardware transactional memory
Related show

Commit Message

Michael Ellerman Oct. 12, 2017, 10:17 a.m.
Some Power9 revisions can run in a mode where TM operates without
suspended state. If we find ourself on a CPU that might be in this
mode, we query OPAL to check, and if so we reenable TM in CPU
features, and enable a new user feature to signal to userspace that we
are in this mode.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/opal-api.h    |  2 ++
 arch/powerpc/include/asm/powernv.h     |  4 ++++
 arch/powerpc/include/asm/tm.h          |  2 ++
 arch/powerpc/kernel/process.c          |  7 +++++++
 arch/powerpc/kernel/prom.c             |  4 ++++
 arch/powerpc/platforms/powernv/setup.c | 19 +++++++++++++++++++
 6 files changed, 38 insertions(+)

Comments

Florian Weimer Oct. 19, 2017, 10:07 a.m. | #1
On 10/12/2017 12:17 PM, Michael Ellerman wrote:
> +	pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n");
> +	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
> +	cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND;
> +	tm_suspend_disabled = true;

This doesn't look right because if suspend is not available, you need to 
clear the original PPC_FEATURE2_HTM flag because the semantics are not 
right, so that applications can use fallback code.  Otherwise, 
applications may incorrectly select the HTM code and break if running on 
a system which supports HTM, but without the suspend state.

The new flag should say that HTM is supported, but without the suspend 
state, and it should be always set if PPC_FEATURE2_HTM is set.

Thanks,
Florian
Tulio Magno Quites Machado Filho Oct. 19, 2017, 12:04 p.m. | #2
Florian Weimer <fweimer@redhat.com> writes:

> On 10/12/2017 12:17 PM, Michael Ellerman wrote:
>> +	pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n");
>> +	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
>> +	cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND;
>> +	tm_suspend_disabled = true;
>
> This doesn't look right because if suspend is not available, you need to 
> clear the original PPC_FEATURE2_HTM flag because the semantics are not 
> right, so that applications can use fallback code.  Otherwise, 
> applications may incorrectly select the HTM code and break if running on 
> a system which supports HTM, but without the suspend state.

Just clarifying: it won't break an application, but abort the transaction,
which can cause a performance hit.

> The new flag should say that HTM is supported, but without the suspend 
> state, and it should be always set if PPC_FEATURE2_HTM is set.

If we change the semantics of this bit, old applications that don't suspend
transactions won't run on these processors, even if they're safe to run.

If we adopt the current semantics, only applications that enter in suspend
state will have to be modified in order to not get a performance regression.
Florian Weimer Oct. 19, 2017, 12:45 p.m. | #3
On 10/19/2017 02:04 PM, Tulio Magno Quites Machado Filho wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> On 10/12/2017 12:17 PM, Michael Ellerman wrote:
>>> +	pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n");
>>> +	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
>>> +	cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND;
>>> +	tm_suspend_disabled = true;
>>
>> This doesn't look right because if suspend is not available, you need to
>> clear the original PPC_FEATURE2_HTM flag because the semantics are not
>> right, so that applications can use fallback code.  Otherwise,
>> applications may incorrectly select the HTM code and break if running on
>> a system which supports HTM, but without the suspend state.
> 
> Just clarifying: it won't break an application, but abort the transaction,
> which can cause a performance hit.

And in this case, it is better not HTM at all.

>> The new flag should say that HTM is supported, but without the suspend
>> state, and it should be always set if PPC_FEATURE2_HTM is set.
> 
> If we change the semantics of this bit, old applications that don't suspend
> transactions won't run on these processors, even if they're safe to run.
> 
> If we adopt the current semantics, only applications that enter in suspend
> state will have to be modified in order to not get a performance regression.

In this light, the trade-off implied by the posted patch seems to be the 
right one.  But the other discussion of an ABI change still makes me a 
bit nervous.

Thanks,
Florian
Tulio Magno Quites Machado Filho Oct. 19, 2017, 1:34 p.m. | #4
Forwarding some comments from Adhemerval sent to libc-alpha [1]...

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>Florian Weimer <fweimer@redhat.com> writes:
>
>> On 10/12/2017 12:17 PM, Michael Ellerman wrote:
>>> +	pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n");
>>> +	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
>>> +	cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND;
>>> +	tm_suspend_disabled = true;
>>
>> This doesn't look right because if suspend is not available, you need to 
>> clear the original PPC_FEATURE2_HTM flag because the semantics are not 
>> right, so that applications can use fallback code.  Otherwise, 
>> applications may incorrectly select the HTM code and break if running on 
>> a system which supports HTM, but without the suspend state.
>>
>> The new flag should say that HTM is supported, but without the suspend 
>> state, and it should be always set if PPC_FEATURE2_HTM is set.
>
> Will it also change TEXARS with the abort information?

It should, with a permanent error cause so that old applications entering
suspended state can adopt another technique.
Michael, could you clarify if this is indeed happening, please?

> I completely agree with Florian here, this is as *ABI* change
> and the kernel need to advertise a different TM ABI instead
> of as an extension.

Adhemerval, could you elaborate which problems you're foreseeing, please?
Adhemerval Zanella Oct. 19, 2017, 3:13 p.m. | #5
On 19/10/2017 11:34, Tulio Magno Quites Machado Filho wrote:
> Forwarding some comments from Adhemerval sent to libc-alpha [1]...
> 
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> On 10/12/2017 12:17 PM, Michael Ellerman wrote:
>>>> +	pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n");
>>>> +	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
>>>> +	cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND;
>>>> +	tm_suspend_disabled = true;
>>>
>>> This doesn't look right because if suspend is not available, you need to 
>>> clear the original PPC_FEATURE2_HTM flag because the semantics are not 
>>> right, so that applications can use fallback code.  Otherwise, 
>>> applications may incorrectly select the HTM code and break if running on 
>>> a system which supports HTM, but without the suspend state.
>>>
>>> The new flag should say that HTM is supported, but without the suspend 
>>> state, and it should be always set if PPC_FEATURE2_HTM is set.
>>
>> Will it also change TEXARS with the abort information?
> 
> It should, with a permanent error cause so that old applications entering
> suspended state can adopt another technique.
> Michael, could you clarify if this is indeed happening, please?
> 
>> I completely agree with Florian here, this is as *ABI* change
>> and the kernel need to advertise a different TM ABI instead
>> of as an extension.
> 
> Adhemerval, could you elaborate which problems you're foreseeing, please?
> 

Pretty much the same Florian already stated: an application can not any
more assume for instance:

        tsr. 	0
        mfcr	r9,128
        andis. 	r10,r9,0x4000
        be 	cr0,L(suspend)
	andis.	r10,r9,0x2000
	be	cr0,L(transactional)

However thinking more about it I am not sure if this should be really a
problem: on default HTM mode the program must handle self-induced failures
as the tbegin. failure path and I assume trying to suspend/resume in this
case will trigger this.  For instance:

   if (__builtin_tbegin (0))
     {
       /* some transactional stuff.  */
       __builtin_tsuspend ();
       /* non transactional stuff.  */
       __builtin_tresume ();
       /* more transactional stuff.  */
     }
   else
     {
       /* fall-out code.  */
     }

So I assume for these chips without suspend/resume support the example
code will always run the fall-out code.
Michael Neuling Oct. 20, 2017, 2:47 a.m. | #6
On Thu, 2017-10-19 at 11:34 -0200, Tulio Magno Quites Machado Filho wrote:
> Forwarding some comments from Adhemerval sent to libc-alpha [1]...
> 
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> > Florian Weimer <fweimer@redhat.com> writes:
> > 
> > > On 10/12/2017 12:17 PM, Michael Ellerman wrote:
> > > > +	pr_info("Enabling TM (Transactional Memory) with Suspend
> > > > Disabled\n");
> > > > +	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
> > > > +	cur_cpu_spec->cpu_user_features2 |=
> > > > PPC_FEATURE2_HTM_NO_SUSPEND;
> > > > +	tm_suspend_disabled = true;
> > > 
> > > This doesn't look right because if suspend is not available, you need to 
> > > clear the original PPC_FEATURE2_HTM flag because the semantics are not 
> > > right, so that applications can use fallback code.  Otherwise, 
> > > applications may incorrectly select the HTM code and break if running on 
> > > a system which supports HTM, but without the suspend state.
> > > 
> > > The new flag should say that HTM is supported, but without the suspend 
> > > state, and it should be always set if PPC_FEATURE2_HTM is set.
> > 
> > Will it also change TEXARS with the abort information?
> 
> It should, with a permanent error cause so that old applications entering
> suspended state can adopt another technique.
> Michael, could you clarify if this is indeed happening, please?

It will.

If we hit a tsuspend and we are in this mode, the hardware will abort us and set
the Failure Persistent bit (unless there is some other exception pending at the
same time which causes the abort).

Mikey

Patch

diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index 9d191ebea706..233c7504b1f2 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -896,6 +896,8 @@  enum {
 	 */
 	OPAL_REINIT_CPUS_MMU_HASH	= (1 << 2),
 	OPAL_REINIT_CPUS_MMU_RADIX	= (1 << 3),
+
+	OPAL_REINIT_CPUS_TM_SUSPEND_DISABLED = (1 << 4),
 };
 
 typedef struct oppanel_line {
diff --git a/arch/powerpc/include/asm/powernv.h b/arch/powerpc/include/asm/powernv.h
index f62797702300..dc5f6a5d4575 100644
--- a/arch/powerpc/include/asm/powernv.h
+++ b/arch/powerpc/include/asm/powernv.h
@@ -22,6 +22,8 @@  extern void pnv_npu2_destroy_context(struct npu_context *context,
 extern int pnv_npu2_handle_fault(struct npu_context *context, uintptr_t *ea,
 				unsigned long *flags, unsigned long *status,
 				int count);
+
+void pnv_tm_init(void);
 #else
 static inline void powernv_set_nmmu_ptcr(unsigned long ptcr) { }
 static inline struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
@@ -36,6 +38,8 @@  static inline int pnv_npu2_handle_fault(struct npu_context *context,
 					unsigned long *status, int count) {
 	return -ENODEV;
 }
+
+static inline void pnv_tm_init(void) { }
 #endif
 
 #endif /* _ASM_POWERNV_H */
diff --git a/arch/powerpc/include/asm/tm.h b/arch/powerpc/include/asm/tm.h
index 82e06ca3a49b..ad19fe41931b 100644
--- a/arch/powerpc/include/asm/tm.h
+++ b/arch/powerpc/include/asm/tm.h
@@ -19,4 +19,6 @@  extern void tm_abort(uint8_t cause);
 extern void tm_save_sprs(struct thread_struct *thread);
 extern void tm_restore_sprs(struct thread_struct *thread);
 
+extern bool tm_suspend_disabled;
+
 #endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 166145b18728..b02807ea54dc 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -77,6 +77,13 @@ 
 extern unsigned long _get_SP(void);
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+/*
+ * Are we running in "Suspend disabled" mode? If so we have to block any
+ * sigreturn that would get us into suspended state, and we also warn in some
+ * other paths that we should never reach with suspend disabled.
+ */
+bool tm_suspend_disabled __ro_after_init = false;
+
 static void check_if_tm_restore_required(struct task_struct *tsk)
 {
 	/*
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index d9bd6555f980..101822be525a 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -47,6 +47,7 @@ 
 #include <asm/mmu.h>
 #include <asm/paca.h>
 #include <asm/pgtable.h>
+#include <asm/powernv.h>
 #include <asm/iommu.h>
 #include <asm/btext.h>
 #include <asm/sections.h>
@@ -681,7 +682,10 @@  static void __init tm_init(void)
 		cur_cpu_spec->cpu_user_features2 &=
 			~(PPC_FEATURE2_HTM_NOSC | PPC_FEATURE2_HTM);
 		cur_cpu_spec->cpu_features &= ~CPU_FTR_TM;
+		return;
 	}
+
+	pnv_tm_init();
 }
 #else
 static void tm_init(void) { }
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index cf52d53da460..8096c3352e2b 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -36,6 +36,7 @@ 
 #include <asm/opal.h>
 #include <asm/kexec.h>
 #include <asm/smp.h>
+#include <asm/tm.h>
 
 #include "powernv.h"
 
@@ -304,6 +305,24 @@  static int __init pnv_probe(void)
 	return 1;
 }
 
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+void __init pnv_tm_init(void)
+{
+	if (!firmware_has_feature(FW_FEATURE_OPAL) ||
+	    !pvr_version_is(PVR_POWER9) ||
+	    early_cpu_has_feature(CPU_FTR_TM))
+		return;
+
+	if (opal_reinit_cpus(OPAL_REINIT_CPUS_TM_SUSPEND_DISABLED) != OPAL_SUCCESS)
+		return;
+
+	pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n");
+	cur_cpu_spec->cpu_features |= CPU_FTR_TM;
+	cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND;
+	tm_suspend_disabled = true;
+}
+#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
+
 /*
  * Returns the cpu frequency for 'cpu' in Hz. This is used by
  * /proc/cpuinfo