Patchwork [1/4] KVM: PPC: Book3s: PR: Disable preemption in vcpu_run

login
register
mail settings
Submitter Alexander Graf
Date Dec. 9, 2011, 3:26 p.m.
Message ID <1323444412-18482-2-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/130403/
State New
Headers show

Comments

Alexander Graf - Dec. 9, 2011, 3:26 p.m.
When entering the guest, we want to make sure we're not getting preempted
away, so let's disable preemption on entry, but enable it again while handling
guest exits.

Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s_pr.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
Scott Wood - Dec. 9, 2011, 6:19 p.m.
On 12/09/2011 09:26 AM, Alexander Graf wrote:
> When entering the guest, we want to make sure we're not getting preempted
> away, so let's disable preemption on entry, but enable it again while handling
> guest exits.
> 
> Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/book3s_pr.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 726512b..8e4f800 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -519,6 +519,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  	run->ready_for_interrupt_injection = 1;
>  
>  	trace_kvm_book3s_exit(exit_nr, vcpu);
> +	preempt_enable();
>  	kvm_resched(vcpu);
>  	switch (exit_nr) {
>  	case BOOK3S_INTERRUPT_INST_STORAGE:
> @@ -763,6 +764,8 @@ program_interrupt:
>  			run->exit_reason = KVM_EXIT_INTR;
>  			r = -EINTR;
>  		} else {
> +			preempt_disable();
> +
>  			/* In case an interrupt came in that was triggered
>  			 * from userspace (like DEC), we need to check what
>  			 * to inject now! */

Shouldn't you really have interrupts disabled here, as booke does?

Otherwise an interrupt (including an IPI kick) could send you a signal
or guest exception after you check.

Likewise for other guest entry points.

-Scott

--
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 - Dec. 9, 2011, 11:18 p.m.
On 09.12.2011, at 19:19, Scott Wood wrote:

> On 12/09/2011 09:26 AM, Alexander Graf wrote:
>> When entering the guest, we want to make sure we're not getting preempted
>> away, so let's disable preemption on entry, but enable it again while handling
>> guest exits.
>> 
>> Reported-by: Jörg Sommer <joerg@alea.gnuu.de>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/book3s_pr.c |    7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>> index 726512b..8e4f800 100644
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -519,6 +519,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 	run->ready_for_interrupt_injection = 1;
>> 
>> 	trace_kvm_book3s_exit(exit_nr, vcpu);
>> +	preempt_enable();
>> 	kvm_resched(vcpu);
>> 	switch (exit_nr) {
>> 	case BOOK3S_INTERRUPT_INST_STORAGE:
>> @@ -763,6 +764,8 @@ program_interrupt:
>> 			run->exit_reason = KVM_EXIT_INTR;
>> 			r = -EINTR;
>> 		} else {
>> +			preempt_disable();
>> +
>> 			/* In case an interrupt came in that was triggered
>> 			 * from userspace (like DEC), we need to check what
>> 			 * to inject now! */
> 
> Shouldn't you really have interrupts disabled here, as booke does?

Ah, thanks for the reminder. Yeah, we probably want to disable interrupts in parallel to checking for signals (basically from one signal check point to world switch). I'm just not 100% sure how to easily sync the C and asm code on the first entry though. Doing local_irq_disable in C and undoing it in asm could become ugly with lazy interrupt disabling.


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
Scott Wood - Dec. 13, 2011, 9:15 p.m.
On 12/09/2011 05:18 PM, Alexander Graf wrote:
> 
> On 09.12.2011, at 19:19, Scott Wood wrote:
> 
>> Shouldn't you really have interrupts disabled here, as booke does?
> 
> Ah, thanks for the reminder. Yeah, we probably want to disable
> interrupts in parallel to checking for signals (basically from one
> signal check point to world switch). I'm just not 100% sure how to
> easily sync the C and asm code on the first entry though. Doing
> local_irq_disable in C and undoing it in asm could become ugly with
> lazy interrupt disabling.

Lots of things get ugly with lazy interrupt disabling. :-(

There should be other examples of handling the lazy EE stuff in asm
code.  After you hard-disable, you should just need to poke the right
fields in the PACA with the state you plan to end up in after the rfi,
similar to exception return.

-Scott

--
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 726512b..8e4f800 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -519,6 +519,7 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	run->ready_for_interrupt_injection = 1;
 
 	trace_kvm_book3s_exit(exit_nr, vcpu);
+	preempt_enable();
 	kvm_resched(vcpu);
 	switch (exit_nr) {
 	case BOOK3S_INTERRUPT_INST_STORAGE:
@@ -763,6 +764,8 @@  program_interrupt:
 			run->exit_reason = KVM_EXIT_INTR;
 			r = -EINTR;
 		} else {
+			preempt_disable();
+
 			/* In case an interrupt came in that was triggered
 			 * from userspace (like DEC), we need to check what
 			 * to inject now! */
@@ -925,6 +928,8 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 #endif
 	ulong ext_msr;
 
+	preempt_disable();
+
 	/* Check if we can run the vcpu at all */
 	if (!vcpu->arch.sane) {
 		kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
@@ -1006,6 +1011,8 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	current->thread.used_vsr = used_vsr;
 #endif
 
+	preempt_enable();
+
 	return ret;
 }