[PATCH 02/26] KVM: PPC: Book3S PR: add new parameter (guest MSR) for kvmppc_save_tm()/kvmppc_restore_tm()

[Simon Guo]

Hi Paul,
On Tue, Jan 23, 2018 at 04:42:09PM +1100, Paul Mackerras wrote:
> On Thu, Jan 11, 2018 at 06:11:15PM +0800, wei.guo.simon at gmail.com wrote:
> > From: Simon Guo <wei.guo.simon at gmail.com>
> > 
> > HV KVM and PR KVM need different MSR source to indicate whether
> > treclaim. or trecheckpoint. is necessary.
> > 
> > This patch add new parameter (guest MSR) for these kvmppc_save_tm/
> > kvmppc_restore_tm() APIs:
> > - For HV KVM, it is VCPU_MSR
> > - For PR KVM, it is current host MSR or VCPU_SHADOW_SRR1
> > 
> > This enhancement enables these 2 APIs to be reused by PR KVM later.
> > And the patch keeps HV KVM logic unchanged.
> > 
> > This patch also reworks kvmppc_save_tm()/kvmppc_restore_tm() to
> > have a clean ABI: r3 for vcpu and r4 for guest_msr.
> > 
> > Signed-off-by: Simon Guo <wei.guo.simon at gmail.com>
> 
> Question: why do you switch from using HSTATE_HOST_R1 to HSTATE_SCRATCH2
> 
> > @@ -42,11 +45,11 @@ _GLOBAL(kvmppc_save_tm)
> >  	rldimi	r8, r0, MSR_TM_LG, 63-MSR_TM_LG
> >  	mtmsrd	r8
> >  
> > -	ld	r5, VCPU_MSR(r9)
> > -	rldicl. r5, r5, 64 - MSR_TS_S_LG, 62
> > +	rldicl. r4, r4, 64 - MSR_TS_S_LG, 62
> >  	beq	1f	/* TM not active in guest. */
> >  
> > -	std	r1, HSTATE_HOST_R1(r13)
> > +	std	r1, HSTATE_SCRATCH2(r13)
> 
> ... here?
> 
> > @@ -166,17 +173,17 @@ _GLOBAL(kvmppc_restore_tm)
> >  	 * The user may change these outside of a transaction, so they must
> >  	 * always be context switched.
> >  	 */
> > -	ld	r5, VCPU_TFHAR(r4)
> > -	ld	r6, VCPU_TFIAR(r4)
> > -	ld	r7, VCPU_TEXASR(r4)
> > +	ld	r5, VCPU_TFHAR(r3)
> > +	ld	r6, VCPU_TFIAR(r3)
> > +	ld	r7, VCPU_TEXASR(r3)
> >  	mtspr	SPRN_TFHAR, r5
> >  	mtspr	SPRN_TFIAR, r6
> >  	mtspr	SPRN_TEXASR, r7
> >  
> > -	ld	r5, VCPU_MSR(r4)
> > +	mr	r5, r4
> >  	rldicl. r5, r5, 64 - MSR_TS_S_LG, 62
> >  	beqlr		/* TM not active in guest */
> > -	std	r1, HSTATE_HOST_R1(r13)
> > +	std	r1, HSTATE_SCRATCH2(r13)
> 
> and here?
> 
> Please add a paragraph to the patch description explaining why you are
> making that change.
In subsequent patches, kvmppc_save_tm/kvmppc_restore_tm() will be
invoked by wrapper function who setup addtional stack frame and 
update R1(then update HSTATE_HOST_R1 with addtional offset). Although 
HSTATE_HOST_R1 is now used safely(always PPC_STL before entering 
guest and PPC_LL in kvmppc_interrupt_pr()), I worried a future usage 
will take an assumption on HSTATE_HOST_R1 value and bring trouble.

As a result, in kvmppc_save_tm/kvmppc_restore_tm() case, I choose
HSTATE_SCRATCH2 to restore the r1. I will update the commit message. 


Thanks,
- Simon




> 
> Paul.