| Message ID | 1489061763-3832-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/03/2017 13:16, Wanpeng Li wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > Commit b95234c84 (kvm: x86: do not use KVM_REQ_EVENT for APICv interrupt > injection) disables interrupts before setting vcpu->mode to fix an race: > > | The IPI for posted interrupts may be issued between setting vcpu->mode = > | IN_GUEST_MODE and disabling interrupts. If that happens, > | kvm_trigger_posted_interrupt returns true, but smp_kvm_posted_intr_ipi > | doesn't do anything about it. The guest is entered with PIR.ON, but the > | posted interrupt IPI has not been sent and the interrupt is only delivered > | to the guest on the next vmentry (if any). > > However the race has already been fixed by the side-effects of moving the > RVI update after IN_GUEST_MODE in the commit: > > - The IPI for posted interrupts is issued after setting vcpu->mode = > IN_GUEST_MODE and disabling interrupts, the posted interrupt IPI is > delayed until the guest enters non-root mode; it is then trapped by > the processor causing the interrupt to be injected. > - The IPI for posted interrupts is issued between setting vcpu->mode = > IN_GUEST_MODE and disabling interrupts, the movement of the RVI update > after IN_GUEST_MODE in the commit will catch the new PIR, and set RVI. > > This patch tries to reduce the local IRQ disable time by restoring the > place of disable local IRQ in order to improve host kernel responsibility > to some degree. This should not make any difference; we're talking of a few dozen clock cycles, and it's quite possible that the extra chance of getting a PI interrupt in the host negates it. Paolo > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- > arch/x86/kvm/x86.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 1faf620..9d97041 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6857,12 +6857,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > kvm_x86_ops->prepare_guest_switch(vcpu); > kvm_load_guest_fpu(vcpu); > > - /* > - * Disable IRQs before setting IN_GUEST_MODE. Posted interrupt > - * IPI are then delayed after guest entry, which ensures that they > - * result in virtual interrupt delivery. > - */ > - local_irq_disable(); > vcpu->mode = IN_GUEST_MODE; > > srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); > @@ -6881,6 +6875,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > */ > smp_mb__after_srcu_read_unlock(); > > + local_irq_disable(); > + > /* > * This handles the case where a posted interrupt was > * notified with kvm_vcpu_kick. >
Am 09.03.2017 um 13:33 schrieb Paolo Bonzini: > > > On 09/03/2017 13:16, Wanpeng Li wrote: >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> Commit b95234c84 (kvm: x86: do not use KVM_REQ_EVENT for APICv interrupt >> injection) disables interrupts before setting vcpu->mode to fix an race: >> >> | The IPI for posted interrupts may be issued between setting vcpu->mode = >> | IN_GUEST_MODE and disabling interrupts. If that happens, >> | kvm_trigger_posted_interrupt returns true, but smp_kvm_posted_intr_ipi >> | doesn't do anything about it. The guest is entered with PIR.ON, but the >> | posted interrupt IPI has not been sent and the interrupt is only delivered >> | to the guest on the next vmentry (if any). >> >> However the race has already been fixed by the side-effects of moving the >> RVI update after IN_GUEST_MODE in the commit: >> >> - The IPI for posted interrupts is issued after setting vcpu->mode = >> IN_GUEST_MODE and disabling interrupts, the posted interrupt IPI is >> delayed until the guest enters non-root mode; it is then trapped by >> the processor causing the interrupt to be injected. >> - The IPI for posted interrupts is issued between setting vcpu->mode = >> IN_GUEST_MODE and disabling interrupts, the movement of the RVI update >> after IN_GUEST_MODE in the commit will catch the new PIR, and set RVI. >> >> This patch tries to reduce the local IRQ disable time by restoring the >> place of disable local IRQ in order to improve host kernel responsibility >> to some degree. > > This should not make any difference; we're talking of a few dozen clock > cycles, and it's quite possible that the extra chance of getting a PI > interrupt in the host negates it. > > Paolo > Also, I think after all the discussion regarding handling vcpu->mode + preemption when it comes to a vcpu_kick(), keeping it that way is way cleaner.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1faf620..9d97041 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6857,12 +6857,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) kvm_x86_ops->prepare_guest_switch(vcpu); kvm_load_guest_fpu(vcpu); - /* - * Disable IRQs before setting IN_GUEST_MODE. Posted interrupt - * IPI are then delayed after guest entry, which ensures that they - * result in virtual interrupt delivery. - */ - local_irq_disable(); vcpu->mode = IN_GUEST_MODE; srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); @@ -6881,6 +6875,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) */ smp_mb__after_srcu_read_unlock(); + local_irq_disable(); + /* * This handles the case where a posted interrupt was * notified with kvm_vcpu_kick.