From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Subject: Re: [PATCH 1/4] KVM: PPC: Book3S: PR: Don't clobber our exit handler id Date: Sat, 30 Nov 2013 18:20:35 +1100 Message-ID: <20131130072035.GA5969@iris.ozlabs.ibm.com> References: <1385693701-27632-1-git-send-email-agraf@suse.de> <1385693701-27632-2-git-send-email-agraf@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "\"" , "kvm@vger.kernel.org mailing list" To: Alexander Graf Return-path: Received: from ozlabs.org ([203.10.76.45]:60246 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750898Ab3K3I1F (ORCPT ); Sat, 30 Nov 2013 03:27:05 -0500 Content-Disposition: inline In-Reply-To: <1385693701-27632-2-git-send-email-agraf@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > 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.