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

Alexander Graf
2013-11-30T12:38:34Z

On 30.11.2013, at 08:20, Paul Mackerras <paulus@samba.org> wrote:

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

Ugh. You're right.

> 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().

I was trying to be smart and not do a memory access for a field that I always need. But then again I always do need r14 just as well, so it doesn't actually make a difference. I'll change it.

Thanks a lot!

Alex