Re: [PATCH] powerpc/tm: Save and restore AMR on treclaim and trechkpt

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 9/18/20 9:35 AM, Gustavo Romero wrote:
> Althought AMR is stashed on the checkpoint area, currently we don't save
> it to the per thread checkpoint struct after a treclaim and so we don't
> restore it either from that struct when we trechkpt. As a consequence when
> the transaction is later rolled back kernel space AMR value when the
> trechkpt was done appears in userspace.
> 
> That commit saves and restores AMR accordingly on treclaim and trechkpt.
> Since AMR value is also used in kernel space in other functions, it also
> takes care of stashing kernel live AMR into PACA before treclaim and before
> trechkpt, restoring it later, just before returning from tm_reclaim and
> __tm_recheckpoint.
> 
> Is also fixes two nonrelated comments about CR and MSR.
> 
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/paca.h      |  1 +
>   arch/powerpc/include/asm/processor.h |  1 +
>   arch/powerpc/kernel/asm-offsets.c    |  2 ++
>   arch/powerpc/kernel/tm.S             | 31 +++++++++++++++++++++++-----
>   4 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index 9454d29ff4b4..44c605181529 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -179,6 +179,7 @@ struct paca_struct {
>   	u64 sprg_vdso;			/* Saved user-visible sprg */
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	u64 tm_scratch;                 /* TM scratch area for reclaim */
> +	u64 tm_amr;			/* Saved Kernel AMR for treclaim/trechkpt */
>   #endif
>   
>   #ifdef CONFIG_PPC_POWERNV
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index ed0d633ab5aa..9f4f6cc033ac 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -220,6 +220,7 @@ struct thread_struct {
>   	unsigned long	tm_tar;
>   	unsigned long	tm_ppr;
>   	unsigned long	tm_dscr;
> +	unsigned long   tm_amr;
>   
>   	/*
>   	 * Checkpointed FP and VSX 0-31 register set.
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 8711c2164b45..cf1a6d68a91f 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -170,12 +170,14 @@ int main(void)
>   
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	OFFSET(PACATMSCRATCH, paca_struct, tm_scratch);
> +	OFFSET(PACATMAMR, paca_struct, tm_amr);
>   	OFFSET(THREAD_TM_TFHAR, thread_struct, tm_tfhar);
>   	OFFSET(THREAD_TM_TEXASR, thread_struct, tm_texasr);
>   	OFFSET(THREAD_TM_TFIAR, thread_struct, tm_tfiar);
>   	OFFSET(THREAD_TM_TAR, thread_struct, tm_tar);
>   	OFFSET(THREAD_TM_PPR, thread_struct, tm_ppr);
>   	OFFSET(THREAD_TM_DSCR, thread_struct, tm_dscr);
> +	OFFSET(THREAD_TM_AMR, thread_struct, tm_amr);
>   	OFFSET(PT_CKPT_REGS, thread_struct, ckpt_regs);
>   	OFFSET(THREAD_CKVRSTATE, thread_struct, ckvr_state.vr);
>   	OFFSET(THREAD_CKVRSAVE, thread_struct, ckvrsave);
> diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
> index 6ba0fdd1e7f8..e178ddb43619 100644
> --- a/arch/powerpc/kernel/tm.S
> +++ b/arch/powerpc/kernel/tm.S
> @@ -152,6 +152,10 @@ _GLOBAL(tm_reclaim)
>   	li	r5, 0
>   	mtmsrd	r5, 1
>   
> +        /* Save AMR since it's used elsewhere in kernel space */
> +	mfspr	r8, SPRN_AMR
> +	std	r8, PACATMAMR(r13)


Can we save this in stack instead of PACA?


> +
>   	/*
>   	 * BE CAREFUL HERE:
>   	 * At this point we can't take an SLB miss since we have MSR_RI
> @@ -245,7 +249,7 @@ _GLOBAL(tm_reclaim)
>   	 * but is used in signal return to 'wind back' to the abort handler.
>   	 */
>   
> -	/* ******************** CR,LR,CCR,MSR ********** */
> +	/* ***************** CTR, LR, CR, XER ********** */
>   	mfctr	r3
>   	mflr	r4
>   	mfcr	r5
> @@ -256,7 +260,6 @@ _GLOBAL(tm_reclaim)
>   	std	r5, _CCR(r7)
>   	std	r6, _XER(r7)
>   
> -
>   	/* ******************** TAR, DSCR ********** */
>   	mfspr	r3, SPRN_TAR
>   	mfspr	r4, SPRN_DSCR
> @@ -264,6 +267,10 @@ _GLOBAL(tm_reclaim)
>   	std	r3, THREAD_TM_TAR(r12)
>   	std	r4, THREAD_TM_DSCR(r12)
>   
> +        /* ******************** AMR **************** */
> +        mfspr	r3, SPRN_AMR
> +        std	r3, THREAD_TM_AMR(r12)
> +
>   	/*
>   	 * MSR and flags: We don't change CRs, and we don't need to alter MSR.
>   	 */
> @@ -308,8 +315,6 @@ _GLOBAL(tm_reclaim)
>   	std	r3, THREAD_TM_TFHAR(r12)
>   	std	r4, THREAD_TM_TFIAR(r12)
>   
> -	/* AMR is checkpointed too, but is unsupported by Linux. */
> -
>   	/* Restore original MSR/IRQ state & clear TM mode */
>   	ld	r14, TM_FRAME_L0(r1)		/* Orig MSR */
>   
> @@ -330,6 +335,10 @@ _GLOBAL(tm_reclaim)
>   	ld	r0, PACA_DSCR_DEFAULT(r13)
>   	mtspr	SPRN_DSCR, r0
>   
> +        /* Restore kernel saved AMR */
> +	ld	r4, PACATMAMR(r13)
> +	mtspr	SPRN_AMR, r4
> +
>   	blr
>   
>   
> @@ -355,6 +364,10 @@ _GLOBAL(__tm_recheckpoint)
>   	 */
>   	SAVE_NVGPRS(r1)
>   
> +	/* Save kernel AMR since it's used elsewhare in kernel space */
> +	mfspr	r8, SPRN_AMR
> +	std	r8, PACATMAMR(r13)
> +
>   	/* Load complete register state from ts_ckpt* registers */
>   
>   	addi	r7, r3, PT_CKPT_REGS		/* Thread's ckpt_regs */
> @@ -404,7 +417,7 @@ _GLOBAL(__tm_recheckpoint)
>   
>   restore_gprs:
>   
> -	/* ******************** CR,LR,CCR,MSR ********** */
> +	/* ****************** CTR, LR, XER ************* */
>   	ld	r4, _CTR(r7)
>   	ld	r5, _LINK(r7)
>   	ld	r8, _XER(r7)
> @@ -417,6 +430,10 @@ restore_gprs:
>   	ld	r4, THREAD_TM_TAR(r3)
>   	mtspr	SPRN_TAR,	r4
>   
> +	/* ******************** AMR ******************** */
> +	ld	r4, THREAD_TM_AMR(r3)
> +	mtspr	SPRN_AMR, r4
> +
>   	/* Load up the PPR and DSCR in GPRs only at this stage */
>   	ld	r5, THREAD_TM_DSCR(r3)
>   	ld	r6, THREAD_TM_PPR(r3)
> @@ -522,6 +539,10 @@ restore_gprs:
>   	ld	r0, PACA_DSCR_DEFAULT(r13)
>   	mtspr	SPRN_DSCR, r0
>   
> +	/* Restore kernel saved AMR */
> +	ld	r4, PACATMAMR(r13)
> +	mtspr	SPRN_AMR, r4
> +
>   	blr
>   
>   	/* ****************************************************************** */
>