[PATCH/RFC 0/2] KVM: micro-optimization and interrupt disabling

[PATCH/RFC 0/2] KVM: micro-optimization and interrupt disabling

[Christian Borntraeger]

I was able to get rid of some nanoseconds for a guest exit loop
on s390. I did my best to not break other architectures but
review and comments on the general approach is welcome.
Downside is that the existing irq_save things will just work
no matter what the callers have done, the new code must do
the right thing in the callers.

Is that approach acceptible? Does anybody else see some measurable
difference for guest exits?


Christian Borntraeger (2):
  KVM: Push down irq_save to architectures before kvm_guest_enter
  KVM: push down irq_save from kvm_guest_exit

 arch/powerpc/kvm/book3s_hv.c |  4 ++++
 arch/powerpc/kvm/book3s_pr.c |  2 ++
 arch/powerpc/kvm/booke.c     |  4 ++--
 arch/s390/kvm/kvm-s390.c     |  6 ++++--
 arch/x86/kvm/x86.c           |  2 ++
 include/linux/kvm_host.h     | 18 ++++++++----------
 6 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.3.0

[PATCH/RFC 1/2] KVM: Push down irq_save to architectures before kvm_guest_enter

[Christian Borntraeger]

local_irq_disable can be cheaper than local_irq_save, especially
when done only once instead of twice. We can push down the
local_irq_save (and replace it with local_irq_disable) to
save some cycles.
x86, mips and arm already disable the interrupts before calling
kvm_guest_enter. Here we save one local_irq_save/restore pair.
power and s390 are reworked to disable the interrupts before calling
kvm_guest_enter. s390 saves a preempt_disable/enable pair but also
saves some cycles as local_irq_disable/enable can be cheaper than
local_irq_save/restore on some machines.

power should be almost a no-op change (interrupts are disabled
slighty longer).

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

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index de74756..a5f392d 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1779,7 +1779,9 @@ static void kvmppc_run_core(struct kvmppc_vcore *vc)
 
 	spin_unlock(&vc->lock);
 
+	local_irq_disable();
 	kvm_guest_enter();
+	local_irq_enable();
 
 	srcu_idx = srcu_read_lock(&vc->kvm->srcu);
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 46f37df..9f4c954 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2010,9 +2010,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 		 * As PF_VCPU will be used in fault handler, between
 		 * guest_enter and guest_exit should be no uaccess.
 		 */
-		preempt_disable();
+		local_irq_disable();
 		kvm_guest_enter();
-		preempt_enable();
+		local_irq_enable();
 		exit_reason = sie64a(vcpu->arch.sie_block,
 				     vcpu->run->s.regs.gprs);
 		kvm_guest_exit();
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d12b210..a34bf6ed 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -751,13 +751,15 @@ static inline void kvm_iommu_unmap_pages(struct kvm *kvm,
 
 static inline void kvm_guest_enter(void)
 {
-	unsigned long flags;
-
-	BUG_ON(preemptible());
-
-	local_irq_save(flags);
+	/*
+	 * guest_enter needs disabled irqs and rcu_virt_note_context_switch
+	 * wants disabled preemption. Ensure that the caller has disabled
+	 * irqs for kvm_guest_enter. Please note: Some architectures (e.g.
+	 * s390) will reenable irqs before entering the guest, but this is
+	 * ok. We just need a stable CPU for the accounting itself.
+	 */
+	WARN_ON(!irqs_disabled());
 	guest_enter();
-	local_irq_restore(flags);
 
 	/* KVM does not hold any references to rcu protected data when it
 	 * switches CPU into a guest mode. In fact switching to a guest mode
-- 
2.3.0

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

[Christian Borntraeger]

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();
 
 	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);
 }
 
 /*
-- 
2.3.0

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

[Paolo Bonzini]

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);
>  }
>  
>  /*
> 

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

[Christian Borntraeger]

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.

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

[Paolo Bonzini]

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