Re: [PATCH 1/4] KVM: PPC: Book3S: PR: Don't clobber our exit handler id

[Paul Mackerras <paulus@samba.org>]

On Fri, Nov 29, 2013 at 03:54:58AM +0100, Alexander Graf wrote:
> We call a C helper to save all svcpu fields into our vcpu. The C
> ABI states that r12 is considered volatile. However, we keep our
> exit handler id in r12 currently.
> 
> So we need to save it away into a non-volatile register instead
> that definitely does get preserved across the C call.
> 
> This bug usually didn't hit anyone yet since gcc is smart enough
> to generate code that doesn't even need r12 which means it stayed
> identical throughout the call by sheer luck. But we can't rely on
> that.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/book3s_interrupts.S | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_interrupts.S b/arch/powerpc/kvm/book3s_interrupts.S
> index f4dd041..2f7d571 100644
> --- a/arch/powerpc/kvm/book3s_interrupts.S
> +++ b/arch/powerpc/kvm/book3s_interrupts.S
> @@ -132,9 +132,18 @@ kvm_start_lightweight:
>  	 *
>  	 */
>  
> +	PPC_LL	r3, GPR4(r1)		/* vcpu pointer */
> +
> +	/*
> +	 * kvmppc_copy_from_svcpu can clobber volatile registers, save
> +	 * r14 to get a spare scratch register for the exit handler id.
> +	 */
> +	PPC_STL	r14, VCPU_GPR(R14)(r3)
> +	mr	r14, r12

In the case where kvmppc_handle_exit_pr returns RESUME_GUEST and we do
the lightweight guest re-entry, I don't see anywhere that we re-load
r14 with the guest value.  So I think this results in guest-visible
corruption of r14.

Why not just use the vcpu->arch.trap field?  Store r12 to
VCPU_TRAP(r3) here and load r5 from VCPU_TRAP(r4) just before calling
kvmppc_handle_exit_pr().

Regards,
Paul.