| Message ID | 1430217168-25504-3-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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); } /*
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(-)