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

[Simon Guo <wei.guo.simon@gmail.com>]

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:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> > 
> > 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 <wei.guo.simon@gmail.com>
> 
> [snip]
> 
> > --- 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.

> > @@ -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