On Tue, 2018-11-06 at 10:40 -0200, Breno Leitao wrote:
> This is the only place we are going to recheckpoint now. Now the task
> needs to have TIF_RESTORE_TM flag set, which will get into
> restore_tm_state() at exception exit path, and execute the recheckpoint
> depending on the MSR.
>
> Every time a task is required to recheckpoint, or just have the TM SPRs
> restore, the TIF_RESTORE_TM flag should be set and the task MSR should
> properly be in a transactional state, which will be checked by
> restore_tm_state().
>
> After the facility registers are recheckpointed, they are clobbered with
> the values that were recheckpointed (and are now also in the checkpoint
> area).
Which facility registers? I don't understand this.
> If facility is enabled at MSR that is being returned to user space, then
> the facility registers need to be restored, otherwise userspace will see
> invalid values.
>
> This patch simplify the restore_tm_state() to just restore the facility
> registers that are enabled when returning to userspace, i.e. the MSR will
> be the same that will be put into SRR1, which will be the MSR after RFID.
>
> Signed-off-by: Breno Leitao <lei...@debian.org>
> ---
> arch/powerpc/kernel/process.c | 38 ++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 4d5322cfad25..c7e758a42b8f 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1049,8 +1049,6 @@ static inline void __switch_to_tm(struct task_struct
> *prev,
> */
> void restore_tm_state(struct pt_regs *regs)
> {
> - unsigned long msr_diff;
> -
> /*
> * This is the only moment we should clear TIF_RESTORE_TM as
> * it is here that ckpt_regs.msr and pt_regs.msr become the same
> @@ -1061,19 +1059,35 @@ void restore_tm_state(struct pt_regs *regs)
> if (!MSR_TM_ACTIVE(regs->msr))
> return;
>
> - msr_diff = current->thread.ckpt_regs.msr & ~regs->msr;
> - msr_diff &= MSR_FP | MSR_VEC | MSR_VSX;
> + tm_enable();
> + /* The only place we recheckpoint */
> + tm_recheckpoint(¤t->thread);
>
> - /* Ensure that restore_math() will restore */
> - if (msr_diff & MSR_FP)
> - current->thread.load_fp = 1;
> + /*
> + * Restore the facility registers that were clobbered during
> + * recheckpoint.
> + */
> + if (regs->msr & MSR_FP) {
> + /*
> + * Using load_fp_state() instead of restore_fp() because we
> + * want to force the restore, independent of
> + * tsk->thread.load_fp. Same for other cases below.
> + */
> + load_fp_state(¤t->thread.fp_state);
> + }
> #ifdef CONFIG_ALTIVEC
> - if (cpu_has_feature(CPU_FTR_ALTIVEC) && msr_diff & MSR_VEC)
> - current->thread.load_vec = 1;
> + if (cpu_has_feature(CPU_FTR_ALTIVEC) && regs->msr & MSR_VEC)
> + load_vr_state(¤t->thread.vr_state);
> +#endif
> +#ifdef CONFIG_VSX
> + if (cpu_has_feature(CPU_FTR_VSX) && regs->msr & MSR_VSX) {
> + /*
> + * If VSX is enabled, it is expected that VEC and FP are
> + * also enabled and already restored the full register set.
> + * Cause a warning if that is not the case.
> + */
> + WARN_ON(!(regs->msr & MSR_VEC) || !(regs->msr & MSR_FP)); }
> #endif
> - restore_math(regs);
> -
> - regs->msr |= msr_diff;
> }
>
> #else
