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

Submitted by Alexander Graf on Dec. 9, 2011, 3:26 p.m.

Details

Message ID 1323444412-18482-2-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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;
 }