[PATCH/RFC,2/2] KVM: push down irq_save from kvm_guest_exit

Message ID 1430217168-25504-3-git-send-email-borntraeger@de.ibm.com
State New, archived
Headers show

Commit Message

Christian Borntraeger April 28, 2015, 10:32 a.m.
Some architectures already have irq disabled when calling
kvm_guest_exit. Push down the disabling into the architectures
to avoid double disabling. This also allows to replace
irq_save with irq_disable which might be cheaper.
arm and mips already have interrupts disabled. s390/power/x86
need adoptions.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 2 ++
 arch/powerpc/kvm/book3s_pr.c | 2 ++
 arch/powerpc/kvm/booke.c     | 4 ++--
 arch/s390/kvm/kvm-s390.c     | 2 ++
 arch/x86/kvm/x86.c           | 2 ++
 include/linux/kvm_host.h     | 4 ----
 6 files changed, 10 insertions(+), 6 deletions(-)

Comments

Paolo Bonzini April 28, 2015, 11:37 a.m. | #1
On 28/04/2015 12:32, Christian Borntraeger wrote:
> Some architectures already have irq disabled when calling
> kvm_guest_exit. Push down the disabling into the architectures
> to avoid double disabling. This also allows to replace
> irq_save with irq_disable which might be cheaper.
> arm and mips already have interrupts disabled. s390/power/x86
> need adoptions.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 2 ++
>  arch/powerpc/kvm/book3s_pr.c | 2 ++
>  arch/powerpc/kvm/booke.c     | 4 ++--
>  arch/s390/kvm/kvm-s390.c     | 2 ++
>  arch/x86/kvm/x86.c           | 2 ++
>  include/linux/kvm_host.h     | 4 ----
>  6 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index a5f392d..63b35b1 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1811,7 +1811,9 @@ static void kvmppc_run_core(struct kvmppc_vcore *vc)
>  
>  	/* make sure updates to secondary vcpu structs are visible now */
>  	smp_mb();
> +	local_irq_disable();
>  	kvm_guest_exit();
> +	local_irq_enable();
>  
>  	preempt_enable();
>  	cond_resched();
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index f573839..eafcb8c 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -891,7 +891,9 @@ int kvmppc_handle_exit_pr(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  
>  	/* We get here with MSR.EE=1 */
>  
> +	local_irq_disable();
>  	trace_kvm_exit(exit_nr, vcpu);
> +	local_irq_enable();
>  	kvm_guest_exit();

This looks wrong.

>  
>  	switch (exit_nr) {
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 6c1316a..f1d6af3 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1004,11 +1004,11 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  		break;
>  	}
>  
> -	local_irq_enable();
> -
>  	trace_kvm_exit(exit_nr, vcpu);
>  	kvm_guest_exit();
>  
> +	local_irq_enable();
> +
>  	run->exit_reason = KVM_EXIT_UNKNOWN;
>  	run->ready_for_interrupt_injection = 1;
>  
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 9f4c954..0aa2534 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2015,7 +2015,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>  		local_irq_enable();
>  		exit_reason = sie64a(vcpu->arch.sie_block,
>  				     vcpu->run->s.regs.gprs);
> +		local_irq_disable();
>  		kvm_guest_exit();
> +		local_irq_enable();
>  		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
>  		rc = vcpu_post_run(vcpu, exit_reason);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 32bf19e..a5fbd45 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6351,7 +6351,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	 */
>  	barrier();
>  
> +	local_irq_disable();
>  	kvm_guest_exit();
> +	local_irq_enable();
>  
>  	preempt_enable();
>  
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index a34bf6ed..fe699fb 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -773,11 +773,7 @@ static inline void kvm_guest_enter(void)
>  
>  static inline void kvm_guest_exit(void)
>  {
> -	unsigned long flags;
> -
> -	local_irq_save(flags);

Why no WARN_ON here?

I think the patches are sensible, especially since you can add warnings.
 This kind of code definitely knows if it has interrupts disabled or enabled

Alternatively, the irq-disabled versions could be called
__kvm_guest_{enter,exit}.  Then you can use those directly when it makes
sense.

Paolo

>  	guest_exit();
> -	local_irq_restore(flags);
>  }
>  
>  /*
> 
--
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
Christian Borntraeger April 28, 2015, 2:10 p.m. | #2
Am 28.04.2015 um 13:37 schrieb Paolo Bonzini:
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -891,7 +891,9 @@ int kvmppc_handle_exit_pr(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>  
>>  	/* We get here with MSR.EE=1 */
>>  
>> +	local_irq_disable();
>>  	trace_kvm_exit(exit_nr, vcpu);
>> +	local_irq_enable();
>>  	kvm_guest_exit();
> 
> This looks wrong.
> 
Yes it is.

>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -773,11 +773,7 @@ static inline void kvm_guest_enter(void)
>>  
>>  static inline void kvm_guest_exit(void)
>>  {
>> -	unsigned long flags;
>> -
>> -	local_irq_save(flags);
> 
> Why no WARN_ON here?

Yes,WARN_ON makes sense.
Hmm, on the other hand the  irqs_disabled call might be somewhat expensive again
(would boil down on s390 to call stnsm (store and and system mask) once for 
query and once for setting.

so...
> 
> I think the patches are sensible, especially since you can add warnings.
>  This kind of code definitely knows if it has interrupts disabled or enabled
> 
> Alternatively, the irq-disabled versions could be called
> __kvm_guest_{enter,exit}.  Then you can use those directly when it makes
> sense.

..having a special  __kvm_guest_{enter,exit} without the WARN_ON might be even
the cheapest way. In that way I could leave everything besides s390 alone and
arch maintainers can do a followup patch if appropriate.

--
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
Paolo Bonzini April 28, 2015, 2:12 p.m. | #3
On 28/04/2015 16:10, Christian Borntraeger wrote:
> > Alternatively, the irq-disabled versions could be called
> > __kvm_guest_{enter,exit}.  Then you can use those directly when it makes
> > sense.
>
> ..having a special  __kvm_guest_{enter,exit} without the WARN_ON might be even
> the cheapest way. In that way I could leave everything besides s390 alone and
> arch maintainers can do a followup patch if appropriate.

That's certainly fine with me.

Paolo
--
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_hv.c b/arch/powerpc/kvm/book3s_hv.c
index a5f392d..63b35b1 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1811,7 +1811,9 @@  static void kvmppc_run_core(struct kvmppc_vcore *vc)
 
 	/* make sure updates to secondary vcpu structs are visible now */
 	smp_mb();
+	local_irq_disable();
 	kvm_guest_exit();
+	local_irq_enable();
 
 	preempt_enable();
 	cond_resched();
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index f573839..eafcb8c 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -891,7 +891,9 @@  int kvmppc_handle_exit_pr(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	/* We get here with MSR.EE=1 */
 
+	local_irq_disable();
 	trace_kvm_exit(exit_nr, vcpu);
+	local_irq_enable();
 	kvm_guest_exit();
 
 	switch (exit_nr) {
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 6c1316a..f1d6af3 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1004,11 +1004,11 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		break;
 	}
 
-	local_irq_enable();
-
 	trace_kvm_exit(exit_nr, vcpu);
 	kvm_guest_exit();
 
+	local_irq_enable();
+
 	run->exit_reason = KVM_EXIT_UNKNOWN;
 	run->ready_for_interrupt_injection = 1;
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 9f4c954..0aa2534 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2015,7 +2015,9 @@  static int __vcpu_run(struct kvm_vcpu *vcpu)
 		local_irq_enable();
 		exit_reason = sie64a(vcpu->arch.sie_block,
 				     vcpu->run->s.regs.gprs);
+		local_irq_disable();
 		kvm_guest_exit();
+		local_irq_enable();
 		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 
 		rc = vcpu_post_run(vcpu, exit_reason);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 32bf19e..a5fbd45 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6351,7 +6351,9 @@  static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	 */
 	barrier();
 
+	local_irq_disable();
 	kvm_guest_exit();
+	local_irq_enable();
 
 	preempt_enable();
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a34bf6ed..fe699fb 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -773,11 +773,7 @@  static inline void kvm_guest_enter(void)
 
 static inline void kvm_guest_exit(void)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
 	guest_exit();
-	local_irq_restore(flags);
 }
 
 /*