Re: [PATCH 04/26] KVM: PPC: Book3S PR: add C function wrapper for _kvmppc_save/restore_tm()

Hi Paul,
On Tue, Jan 23, 2018 at 04:49:16PM +1100, Paul Mackerras wrote:

On Thu, Jan 11, 2018 at 06:11:17PM +0800, wei.guo.simon@gmail.com wrote:

quoted

From: Simon Guo <redacted>

Currently _kvmppc_save/restore_tm() APIs can only be invoked from
assembly function. This patch adds C function wrappers for them so
that they can be safely called from C function.

Signed-off-by: Simon Guo <redacted>

[snip]

quoted

--- a/arch/powerpc/include/asm/asm-prototypes.h
+++ b/arch/powerpc/include/asm/asm-prototypes.h

@@ -126,4 +126,11 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
 void _mcount(void);
 unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip);
 
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+/* Transaction memory related */
+struct kvm_vcpu;
+void _kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu, u64 guest_msr);
+void _kvmppc_save_tm_pr(struct kvm_vcpu *vcpu, u64 guest_msr);
+#endif

It's not generally necessary to have ifdefs around function
declarations.  If the function is never defined because the feature
is not configured in, that is fine.

Got it. Thanks.

quoted

@@ -149,6 +149,58 @@ _GLOBAL(kvmppc_save_tm)
 	blr
 
 /*
+ * _kvmppc_save_tm() is a wrapper around __kvmppc_save_tm(), so that it can
+ * be invoked from C function by PR KVM only.
+ */
+_GLOBAL(_kvmppc_save_tm_pr)
+	mflr	r5
+	std	r5, PPC_LR_STKOFF(r1)
+	stdu    r1, -SWITCH_FRAME_SIZE(r1)
+	SAVE_NVGPRS(r1)
+
+	/* save MSR since TM/math bits might be impacted
+	 * by __kvmppc_save_tm().
+	 */
+	mfmsr	r5
+	SAVE_GPR(5, r1)
+
+	/* also save DSCR/CR so that it can be recovered later */
+	mfspr   r6, SPRN_DSCR
+	SAVE_GPR(6, r1)
+
+	mfcr    r7
+	stw     r7, _CCR(r1)
+
+	/* allocate stack frame for __kvmppc_save_tm since
+	 * it will save LR into its stackframe and we don't
+	 * want to corrupt _kvmppc_save_tm_pr's.
+	 */
+	stdu    r1, -PPC_MIN_STKFRM(r1)

You don't need to do this.  In the PowerPC ELF ABI, functions always
save their LR (i.e. their return address) in their *caller's* stack
frame, not their own.  You have established a stack frame for
_kvmppc_save_tm_pr above, and that is sufficient.  Same comment
applies for _kvmppc_restore_tm_pr.

Ah..yes. I need remove that.

Thanks,
- Simon