Patchwork [02/23] KVM: PPC: Book3S PR: Don't corrupt guest state when kernel uses VMX

login
register
mail settings
Submitter Paul Mackerras
Date Aug. 6, 2013, 4:14 a.m.
Message ID <20130806041433.GH19254@iris.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/264864/
State New
Headers show

Comments

Paul Mackerras - Aug. 6, 2013, 4:14 a.m.
Currently the code assumes that once we load up guest FP/VSX or VMX
state into the CPU, it stays valid in the CPU registers until we
explicitly flush it to the thread_struct.  However, on POWER7,
copy_page() and memcpy() can use VMX.  These functions do flush the
VMX state to the thread_struct before using VMX instructions, but if
this happens while we have guest state in the VMX registers, and we
then re-enter the guest, we don't reload the VMX state from the
thread_struct, leading to guest corruption.  This has been observed
to cause guest processes to segfault.

To fix this, we check before re-entering the guest that all of the
bits corresponding to facilities owned by the guest, as expressed
in vcpu->arch.guest_owned_ext, are set in current->thread.regs->msr.
Any bits that have been cleared correspond to facilities that have
been used by kernel code and thus flushed to the thread_struct, so
for them we reload the state from the thread_struct.

We also need to check current->thread.regs->msr before calling
giveup_fpu() or giveup_altivec(), since if the relevant bit is
clear, the state has already been flushed to the thread_struct and
to flush it again would corrupt it.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kvm/book3s_pr.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)
Aneesh Kumar K.V - Aug. 8, 2013, 3:49 p.m.
Paul Mackerras <paulus@samba.org> writes:

> @@ -575,8 +577,6 @@ static int kvmppc_handle_ext(struct kvm_vcpu *vcpu, unsigned int exit_nr,
>  	printk(KERN_INFO "Loading up ext 0x%lx\n", msr);
>  #endif
>  
> -	current->thread.regs->msr |= msr;
> -
>  	if (msr & MSR_FP) {
>  		for (i = 0; i < ARRAY_SIZE(vcpu->arch.fpr); i++)
>  			thread_fpr[get_fpr_index(i)] = vcpu_fpr[i];
> @@ -598,12 +598,32 @@ static int kvmppc_handle_ext(struct kvm_vcpu *vcpu, unsigned int exit_nr,
>  #endif
>  	}
>  
> +	current->thread.regs->msr |= msr;
>  	vcpu->arch.guest_owned_ext |= msr;
>  	kvmppc_recalc_shadow_msr(vcpu);
>  
>  	return RESUME_GUEST;
>  }

Any specific reason you are doing the above ?

-aneesh

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Aug. 28, 2013, 10:51 p.m.
On 06.08.2013, at 06:14, Paul Mackerras wrote:

> Currently the code assumes that once we load up guest FP/VSX or VMX
> state into the CPU, it stays valid in the CPU registers until we
> explicitly flush it to the thread_struct.  However, on POWER7,
> copy_page() and memcpy() can use VMX.  These functions do flush the
> VMX state to the thread_struct before using VMX instructions, but if
> this happens while we have guest state in the VMX registers, and we
> then re-enter the guest, we don't reload the VMX state from the
> thread_struct, leading to guest corruption.  This has been observed
> to cause guest processes to segfault.
> 
> To fix this, we check before re-entering the guest that all of the
> bits corresponding to facilities owned by the guest, as expressed
> in vcpu->arch.guest_owned_ext, are set in current->thread.regs->msr.
> Any bits that have been cleared correspond to facilities that have
> been used by kernel code and thus flushed to the thread_struct, so
> for them we reload the state from the thread_struct.
> 
> We also need to check current->thread.regs->msr before calling
> giveup_fpu() or giveup_altivec(), since if the relevant bit is
> clear, the state has already been flushed to the thread_struct and
> to flush it again would corrupt it.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>

Thanks, applied to kvm-ppc-queue.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index ddfaf56..adeab19 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -468,7 +468,8 @@  void kvmppc_giveup_ext(struct kvm_vcpu *vcpu, ulong msr)
 		 * both the traditional FP registers and the added VSX
 		 * registers into thread.fpr[].
 		 */
-		giveup_fpu(current);
+		if (current->thread.regs->msr & MSR_FP)
+			giveup_fpu(current);
 		for (i = 0; i < ARRAY_SIZE(vcpu->arch.fpr); i++)
 			vcpu_fpr[i] = thread_fpr[get_fpr_index(i)];
 
@@ -483,7 +484,8 @@  void kvmppc_giveup_ext(struct kvm_vcpu *vcpu, ulong msr)
 
 #ifdef CONFIG_ALTIVEC
 	if (msr & MSR_VEC) {
-		giveup_altivec(current);
+		if (current->thread.regs->msr & MSR_VEC)
+			giveup_altivec(current);
 		memcpy(vcpu->arch.vr, t->vr, sizeof(vcpu->arch.vr));
 		vcpu->arch.vscr = t->vscr;
 	}
@@ -575,8 +577,6 @@  static int kvmppc_handle_ext(struct kvm_vcpu *vcpu, unsigned int exit_nr,
 	printk(KERN_INFO "Loading up ext 0x%lx\n", msr);
 #endif
 
-	current->thread.regs->msr |= msr;
-
 	if (msr & MSR_FP) {
 		for (i = 0; i < ARRAY_SIZE(vcpu->arch.fpr); i++)
 			thread_fpr[get_fpr_index(i)] = vcpu_fpr[i];
@@ -598,12 +598,32 @@  static int kvmppc_handle_ext(struct kvm_vcpu *vcpu, unsigned int exit_nr,
 #endif
 	}
 
+	current->thread.regs->msr |= msr;
 	vcpu->arch.guest_owned_ext |= msr;
 	kvmppc_recalc_shadow_msr(vcpu);
 
 	return RESUME_GUEST;
 }
 
+/*
+ * Kernel code using FP or VMX could have flushed guest state to
+ * the thread_struct; if so, get it back now.
+ */
+static void kvmppc_handle_lost_ext(struct kvm_vcpu *vcpu)
+{
+	unsigned long lost_ext;
+
+	lost_ext = vcpu->arch.guest_owned_ext & ~current->thread.regs->msr;
+	if (!lost_ext)
+		return;
+
+	if (lost_ext & MSR_FP)
+		kvmppc_load_up_fpu();
+	if (lost_ext & MSR_VEC)
+		kvmppc_load_up_altivec();
+	current->thread.regs->msr |= lost_ext;
+}
+
 int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
                        unsigned int exit_nr)
 {
@@ -892,6 +912,7 @@  program_interrupt:
 		} else {
 			kvmppc_fix_ee_before_entry();
 		}
+		kvmppc_handle_lost_ext(vcpu);
 	}
 
 	trace_kvm_book3s_reenter(r, vcpu);