[PATCH v5 0/5] Nonatomic interrupt injection

[PATCH v5 0/5] Nonatomic interrupt injection

[Avi Kivity]

This patchset changes interrupt injection to be done from normal process
context instead of interrupts disabled context.  This is useful for real
mode interrupt injection on Intel without the current hacks (injecting as
a software interrupt of a vm86 task), reducing latencies, and later, for
allowing nested virtualization code to use kvm_read_guest()/kvm_write_guest()
instead of kmap() to access the guest vmcb/vmcs.

Seems to survive a hack that cancels every 16th entry, after injection has
already taken place.

With the PIC reset fix posted earlier, this passes autotest on both AMD and
Intel, with in-kernel irqchip.  I'll run -no-kvm-irqchip tests shortly.

Please review carefully, esp. the first patch.  Any missing kvm_make_request()
there may result in a hung guest.

v5: add KVM_REQ_EVENT to KVM_INTERRUPT path
    change first patch to sample KVM_REQ_EVENT before checking vcpu->requests
      for the entry abort path; improves bisectability (noticed by Gleb)

v4: add KVM_REQ_EVENT after lapic restore

v3: close new race between injection and entry
    fix Intel real-mode injection cancellation

v2: svm support (easier than expected)
    fix silly vmx warning

Avi Kivity (5):
  KVM: Check for pending events before attempting injection
  KVM: VMX: Split up vmx_complete_interrupts()
  KVM: VMX: Move real-mode interrupt injection fixup to
    vmx_complete_interrupts()
  KVM: VMX: Parameterize vmx_complete_interrupts() for both exit and
    entry
  KVM: Non-atomic interrupt injection

 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/i8259.c            |    1 +
 arch/x86/kvm/lapic.c            |   13 +++++-
 arch/x86/kvm/svm.c              |   20 ++++++++-
 arch/x86/kvm/vmx.c              |   87 +++++++++++++++++++++++++++++----------
 arch/x86/kvm/x86.c              |   45 ++++++++++++++------
 include/linux/kvm_host.h        |    1 +
 7 files changed, 130 insertions(+), 38 deletions(-)

[PATCH v5 1/5] KVM: Check for pending events before attempting injection

[Avi Kivity]

Instead of blindly attempting to inject an event before each guest entry,
check for a possible event first in vcpu->requests.  Sites that can trigger
event injection are modified to set KVM_REQ_EVENT:

- interrupt, nmi window opening
- ppr updates
- i8259 output changes
- local apic irr changes
- rflags updates
- gif flag set
- event set on exit

This improves non-injecting entry performance, and sets the stage for
non-atomic injection.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/i8259.c     |    1 +
 arch/x86/kvm/lapic.c     |   13 +++++++++++--
 arch/x86/kvm/svm.c       |    8 +++++++-
 arch/x86/kvm/vmx.c       |    6 ++++++
 arch/x86/kvm/x86.c       |   41 ++++++++++++++++++++++++++++++++---------
 include/linux/kvm_host.h |    1 +
 6 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 5662ff6..6281a32 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -67,6 +67,7 @@ static void pic_unlock(struct kvm_pic *s)
 		if (!found)
 			return;
 
+		kvm_make_request(KVM_REQ_EVENT, found);
 		kvm_vcpu_kick(found);
 	}
 }
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 77d8c0f..c6f2f15 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -259,9 +259,10 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
-	u32 tpr, isrv, ppr;
+	u32 tpr, isrv, ppr, old_ppr;
 	int isr;
 
+	old_ppr = apic_get_reg(apic, APIC_PROCPRI);
 	tpr = apic_get_reg(apic, APIC_TASKPRI);
 	isr = apic_find_highest_isr(apic);
 	isrv = (isr != -1) ? isr : 0;
@@ -274,7 +275,10 @@ static void apic_update_ppr(struct kvm_lapic *apic)
 	apic_debug("vlapic %p, ppr 0x%x, isr 0x%x, isrv 0x%x",
 		   apic, ppr, isr, isrv);
 
-	apic_set_reg(apic, APIC_PROCPRI, ppr);
+	if (old_ppr != ppr) {
+		apic_set_reg(apic, APIC_PROCPRI, ppr);
+		kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
+	}
 }
 
 static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
@@ -391,6 +395,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			break;
 		}
 
+		kvm_make_request(KVM_REQ_EVENT, vcpu);
 		kvm_vcpu_kick(vcpu);
 		break;
 
@@ -416,6 +421,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 				       "INIT on a runnable vcpu %d\n",
 				       vcpu->vcpu_id);
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		} else {
 			apic_debug("Ignoring de-assert INIT to vcpu %d\n",
@@ -430,6 +436,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			result = 1;
 			vcpu->arch.sipi_vector = vector;
 			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
 			kvm_vcpu_kick(vcpu);
 		}
 		break;
@@ -475,6 +482,7 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 		trigger_mode = IOAPIC_EDGE_TRIG;
 	if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI))
 		kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode);
+	kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
 }
 
 static void apic_send_ipi(struct kvm_lapic *apic)
@@ -1152,6 +1160,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 	update_divide_count(apic);
 	start_apic_timer(apic);
 	apic->irr_pending = true;
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 }
 
 void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 69a32bb..8884e51 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2288,6 +2288,7 @@ static int stgi_interception(struct vcpu_svm *svm)
 
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
 	skip_emulated_instruction(&svm->vcpu);
+	kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
 
 	enable_gif(svm);
 
@@ -2663,6 +2664,7 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
 {
 	struct kvm_run *kvm_run = svm->vcpu.run;
 
+	kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
 	svm_clear_vintr(svm);
 	svm->vmcb->control.int_ctl &= ~V_IRQ_MASK;
 	/*
@@ -3108,8 +3110,10 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 
 	svm->int3_injected = 0;
 
-	if (svm->vcpu.arch.hflags & HF_IRET_MASK)
+	if (svm->vcpu.arch.hflags & HF_IRET_MASK) {
 		svm->vcpu.arch.hflags &= ~(HF_NMI_MASK | HF_IRET_MASK);
+		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
+	}
 
 	svm->vcpu.arch.nmi_injected = false;
 	kvm_clear_exception_queue(&svm->vcpu);
@@ -3118,6 +3122,8 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	if (!(exitintinfo & SVM_EXITINTINFO_VALID))
 		return;
 
+	kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
+
 	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
 	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 676555c..4255856 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3333,6 +3333,7 @@ static int handle_wrmsr(struct kvm_vcpu *vcpu)
 
 static int handle_tpr_below_threshold(struct kvm_vcpu *vcpu)
 {
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	return 1;
 }
 
@@ -3345,6 +3346,8 @@ static int handle_interrupt_window(struct kvm_vcpu *vcpu)
 	cpu_based_vm_exec_control &= ~CPU_BASED_VIRTUAL_INTR_PENDING;
 	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);
 
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
+
 	++vcpu->stat.irq_window_exits;
 
 	/*
@@ -3601,6 +3604,7 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu)
 	cpu_based_vm_exec_control &= ~CPU_BASED_VIRTUAL_NMI_PENDING;
 	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);
 	++vcpu->stat.nmi_window_exits;
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 
 	return 1;
 }
@@ -3834,6 +3838,8 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 	if (!idtv_info_valid)
 		return;
 
+	kvm_make_request(KVM_REQ_EVENT, &vmx->vcpu);
+
 	vector = idt_vectoring_info & VECTORING_INFO_VECTOR_MASK;
 	type = idt_vectoring_info & VECTORING_INFO_TYPE_MASK;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cc6d6cd..ac13791 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -284,6 +284,8 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
 	u32 prev_nr;
 	int class1, class2;
 
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
+
 	if (!vcpu->arch.exception.pending) {
 	queue:
 		vcpu->arch.exception.pending = true;
@@ -339,6 +341,7 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
 
 void kvm_inject_nmi(struct kvm_vcpu *vcpu)
 {
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	vcpu->arch.nmi_pending = 1;
 }
 EXPORT_SYMBOL_GPL(kvm_inject_nmi);
@@ -2317,6 +2320,7 @@ static int kvm_vcpu_ioctl_interrupt(struct kvm_vcpu *vcpu,
 		return -ENXIO;
 
 	kvm_queue_interrupt(vcpu, irq->irq, false);
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 
 	return 0;
 }
@@ -2470,6 +2474,8 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
 		vcpu->arch.sipi_vector = events->sipi_vector;
 
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
+
 	return 0;
 }
 
@@ -4201,6 +4207,7 @@ restart:
 
 	toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility);
 	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
 	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
 
@@ -4870,6 +4877,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	int r;
 	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
 		vcpu->run->request_interrupt_window;
+	bool req_event;
 
 	if (vcpu->requests) {
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
@@ -4917,8 +4925,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
 
+	req_event = kvm_check_request(KVM_REQ_EVENT, vcpu);
+
 	if (!atomic_read(&vcpu->guest_mode) || vcpu->requests
 	    || need_resched() || signal_pending(current)) {
+		if (req_event)
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
 		atomic_set(&vcpu->guest_mode, 0);
 		smp_wmb();
 		local_irq_enable();
@@ -4927,17 +4939,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		goto out;
 	}
 
-	inject_pending_event(vcpu);
+	if (req_event) {
+		inject_pending_event(vcpu);
 
-	/* enable NMI/IRQ window open exits if needed */
-	if (vcpu->arch.nmi_pending)
-		kvm_x86_ops->enable_nmi_window(vcpu);
-	else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
-		kvm_x86_ops->enable_irq_window(vcpu);
+		/* enable NMI/IRQ window open exits if needed */
+		if (vcpu->arch.nmi_pending)
+			kvm_x86_ops->enable_nmi_window(vcpu);
+		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
+			kvm_x86_ops->enable_irq_window(vcpu);
 
-	if (kvm_lapic_enabled(vcpu)) {
-		update_cr8_intercept(vcpu);
-		kvm_lapic_sync_to_vapic(vcpu);
+		if (kvm_lapic_enabled(vcpu)) {
+			update_cr8_intercept(vcpu);
+			kvm_lapic_sync_to_vapic(vcpu);
+		}
 	}
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
@@ -5177,6 +5191,8 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 
 	vcpu->arch.exception.pending = false;
 
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
+
 	return 0;
 }
 
@@ -5240,6 +5256,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
 	vcpu->arch.mp_state = mp_state->mp_state;
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	return 0;
 }
 
@@ -5261,6 +5278,7 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
 	memcpy(vcpu->arch.regs, c->regs, sizeof c->regs);
 	kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip);
 	kvm_x86_ops->set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags);
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	return EMULATE_DONE;
 }
 EXPORT_SYMBOL_GPL(kvm_task_switch);
@@ -5331,6 +5349,8 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	    !is_protmode(vcpu))
 		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
+
 	return 0;
 }
 
@@ -5563,6 +5583,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 	vcpu->arch.dr6 = DR6_FIXED_1;
 	vcpu->arch.dr7 = DR7_FIXED_1;
 
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
+
 	return kvm_x86_ops->vcpu_reset(vcpu);
 }
 
@@ -5870,6 +5892,7 @@ void kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	    kvm_is_linear_rip(vcpu, vcpu->arch.singlestep_rip))
 		rflags |= X86_EFLAGS_TF;
 	kvm_x86_ops->set_rflags(vcpu, rflags);
+	kvm_make_request(KVM_REQ_EVENT, vcpu);
 }
 EXPORT_SYMBOL_GPL(kvm_set_rflags);
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f2ecdd5..a08d267 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -39,6 +39,7 @@
 #define KVM_REQ_KVMCLOCK_UPDATE    8
 #define KVM_REQ_KICK               9
 #define KVM_REQ_DEACTIVATE_FPU    10
+#define KVM_REQ_EVENT             11
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID	0
 
-- 
1.7.1

[PATCH v5 2/5] KVM: VMX: Split up vmx_complete_interrupts()

[Avi Kivity]

vmx_complete_interrupts() does too much, split it up:
 - vmx_vcpu_run() gets the "cache important vmcs fields" part
 - a new vmx_complete_atomic_exit() gets the parts that must be done atomically
 - a new vmx_recover_nmi_blocking() does what its name says
 - vmx_complete_interrupts() retains the event injection recovery code

This helps in reducing the work done in atomic context.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |   39 +++++++++++++++++++++++++++------------
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4255856..521df28 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -125,6 +125,7 @@ struct vcpu_vmx {
 	unsigned long         host_rsp;
 	int                   launched;
 	u8                    fail;
+	u32                   exit_intr_info;
 	u32                   idt_vectoring_info;
 	struct shared_msr_entry *guest_msrs;
 	int                   nmsrs;
@@ -3781,18 +3782,9 @@ static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
 	vmcs_write32(TPR_THRESHOLD, irr);
 }
 
-static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
+static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
 {
-	u32 exit_intr_info;
-	u32 idt_vectoring_info = vmx->idt_vectoring_info;
-	bool unblock_nmi;
-	u8 vector;
-	int type;
-	bool idtv_info_valid;
-
-	exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
-
-	vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);
+	u32 exit_intr_info = vmx->exit_intr_info;
 
 	/* Handle machine checks before interrupts are enabled */
 	if ((vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
@@ -3807,8 +3799,16 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 		asm("int $2");
 		kvm_after_handle_nmi(&vmx->vcpu);
 	}
+}
 
-	idtv_info_valid = idt_vectoring_info & VECTORING_INFO_VALID_MASK;
+static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
+{
+	u32 exit_intr_info = vmx->exit_intr_info;
+	bool unblock_nmi;
+	u8 vector;
+	bool idtv_info_valid;
+
+	idtv_info_valid = vmx->idt_vectoring_info & VECTORING_INFO_VALID_MASK;
 
 	if (cpu_has_virtual_nmis()) {
 		unblock_nmi = (exit_intr_info & INTR_INFO_UNBLOCK_NMI) != 0;
@@ -3830,6 +3830,16 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 	} else if (unlikely(vmx->soft_vnmi_blocked))
 		vmx->vnmi_blocked_time +=
 			ktime_to_ns(ktime_sub(ktime_get(), vmx->entry_time));
+}
+
+static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
+{
+	u32 idt_vectoring_info = vmx->idt_vectoring_info;
+	u8 vector;
+	int type;
+	bool idtv_info_valid;
+
+	idtv_info_valid = idt_vectoring_info & VECTORING_INFO_VALID_MASK;
 
 	vmx->vcpu.arch.nmi_injected = false;
 	kvm_clear_exception_queue(&vmx->vcpu);
@@ -4042,6 +4052,11 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	asm("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));
 	vmx->launched = 1;
 
+	vmx->exit_reason = vmcs_read32(VM_EXIT_REASON);
+	vmx->exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
+
+	vmx_complete_atomic_exit(vmx);
+	vmx_recover_nmi_blocking(vmx);
 	vmx_complete_interrupts(vmx);
 }
 
-- 
1.7.1

[PATCH v5 3/5] KVM: VMX: Move real-mode interrupt injection fixup to vmx_complete_interrupts()

[Avi Kivity]

This allows reuse of vmx_complete_interrupts() for cancelling injections.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 521df28..39dd627 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -182,6 +182,7 @@ static int init_rmode(struct kvm *kvm);
 static u64 construct_eptp(unsigned long root_hpa);
 static void kvm_cpu_vmxon(u64 addr);
 static void kvm_cpu_vmxoff(void);
+static void fixup_rmode_irq(struct vcpu_vmx *vmx);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
@@ -3834,11 +3835,15 @@ static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
 
 static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 {
-	u32 idt_vectoring_info = vmx->idt_vectoring_info;
+	u32 idt_vectoring_info;
 	u8 vector;
 	int type;
 	bool idtv_info_valid;
 
+	if (vmx->rmode.irq.pending)
+		fixup_rmode_irq(vmx);
+
+	idt_vectoring_info = vmx->idt_vectoring_info;
 	idtv_info_valid = idt_vectoring_info & VECTORING_INFO_VALID_MASK;
 
 	vmx->vcpu.arch.nmi_injected = false;
@@ -4046,8 +4051,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	vcpu->arch.regs_dirty = 0;
 
 	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
-	if (vmx->rmode.irq.pending)
-		fixup_rmode_irq(vmx);
 
 	asm("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));
 	vmx->launched = 1;
-- 
1.7.1

[PATCH v5 4/5] KVM: VMX: Parameterize vmx_complete_interrupts() for both exit and entry

[Avi Kivity]

Currently vmx_complete_interrupts() can decode event information from vmx
exit fields into the generic kvm event queues.  Make it able to decode
the information from the entry fields as well by parametrizing it.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/vmx.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 39dd627..c6d37df 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -182,7 +182,7 @@ static int init_rmode(struct kvm *kvm);
 static u64 construct_eptp(unsigned long root_hpa);
 static void kvm_cpu_vmxon(u64 addr);
 static void kvm_cpu_vmxoff(void);
-static void fixup_rmode_irq(struct vcpu_vmx *vmx);
+static void fixup_rmode_irq(struct vcpu_vmx *vmx, u32 *idt_vectoring_info);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
@@ -3833,17 +3833,18 @@ static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
 			ktime_to_ns(ktime_sub(ktime_get(), vmx->entry_time));
 }
 
-static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
+static void __vmx_complete_interrupts(struct vcpu_vmx *vmx,
+				      u32 idt_vectoring_info,
+				      int instr_len_field,
+				      int error_code_field)
 {
-	u32 idt_vectoring_info;
 	u8 vector;
 	int type;
 	bool idtv_info_valid;
 
 	if (vmx->rmode.irq.pending)
-		fixup_rmode_irq(vmx);
+		fixup_rmode_irq(vmx, &idt_vectoring_info);
 
-	idt_vectoring_info = vmx->idt_vectoring_info;
 	idtv_info_valid = idt_vectoring_info & VECTORING_INFO_VALID_MASK;
 
 	vmx->vcpu.arch.nmi_injected = false;
@@ -3871,18 +3872,18 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 		break;
 	case INTR_TYPE_SOFT_EXCEPTION:
 		vmx->vcpu.arch.event_exit_inst_len =
-			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+			vmcs_read32(instr_len_field);
 		/* fall through */
 	case INTR_TYPE_HARD_EXCEPTION:
 		if (idt_vectoring_info & VECTORING_INFO_DELIVER_CODE_MASK) {
-			u32 err = vmcs_read32(IDT_VECTORING_ERROR_CODE);
+			u32 err = vmcs_read32(error_code_field);
 			kvm_queue_exception_e(&vmx->vcpu, vector, err);
 		} else
 			kvm_queue_exception(&vmx->vcpu, vector);
 		break;
 	case INTR_TYPE_SOFT_INTR:
 		vmx->vcpu.arch.event_exit_inst_len =
-			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+			vmcs_read32(instr_len_field);
 		/* fall through */
 	case INTR_TYPE_EXT_INTR:
 		kvm_queue_interrupt(&vmx->vcpu, vector,
@@ -3893,24 +3894,31 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 	}
 }
 
+static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
+{
+	__vmx_complete_interrupts(vmx, vmx->idt_vectoring_info,
+				  VM_EXIT_INSTRUCTION_LEN,
+				  IDT_VECTORING_ERROR_CODE);
+}
+
 /*
  * Failure to inject an interrupt should give us the information
  * in IDT_VECTORING_INFO_FIELD.  However, if the failure occurs
  * when fetching the interrupt redirection bitmap in the real-mode
  * tss, this doesn't happen.  So we do it ourselves.
  */
-static void fixup_rmode_irq(struct vcpu_vmx *vmx)
+static void fixup_rmode_irq(struct vcpu_vmx *vmx, u32 *idt_vectoring_info)
 {
 	vmx->rmode.irq.pending = 0;
 	if (kvm_rip_read(&vmx->vcpu) + 1 != vmx->rmode.irq.rip)
 		return;
 	kvm_rip_write(&vmx->vcpu, vmx->rmode.irq.rip);
-	if (vmx->idt_vectoring_info & VECTORING_INFO_VALID_MASK) {
-		vmx->idt_vectoring_info &= ~VECTORING_INFO_TYPE_MASK;
-		vmx->idt_vectoring_info |= INTR_TYPE_EXT_INTR;
+	if (*idt_vectoring_info & VECTORING_INFO_VALID_MASK) {
+		*idt_vectoring_info &= ~VECTORING_INFO_TYPE_MASK;
+		*idt_vectoring_info |= INTR_TYPE_EXT_INTR;
 		return;
 	}
-	vmx->idt_vectoring_info =
+	*idt_vectoring_info =
 		VECTORING_INFO_VALID_MASK
 		| INTR_TYPE_EXT_INTR
 		| vmx->rmode.irq.vector;
-- 
1.7.1

[PATCH v5 5/5] KVM: Non-atomic interrupt injection

[Avi Kivity]

Change the interrupt injection code to work from preemptible, interrupts
enabled context.  This works by adding a ->cancel_injection() operation
that undoes an injection in case we were not able to actually enter the guest
(this condition could never happen with atomic injection).

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/svm.c              |   12 ++++++++++++
 arch/x86/kvm/vmx.c              |   11 +++++++++++
 arch/x86/kvm/x86.c              |   36 ++++++++++++++++--------------------
 4 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9b30285..8004b34 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -510,6 +510,7 @@ struct kvm_x86_ops {
 	void (*queue_exception)(struct kvm_vcpu *vcpu, unsigned nr,
 				bool has_error_code, u32 error_code,
 				bool reinject);
+	void (*cancel_injection)(struct kvm_vcpu *vcpu);
 	int (*interrupt_allowed)(struct kvm_vcpu *vcpu);
 	int (*nmi_allowed)(struct kvm_vcpu *vcpu);
 	bool (*get_nmi_mask)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8884e51..e64a5b1 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3160,6 +3160,17 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	}
 }
 
+static void svm_cancel_injection(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	struct vmcb_control_area *control = &svm->vmcb->control;
+
+	control->exit_int_info = control->event_inj;
+	control->exit_int_info_err = control->event_inj_err;
+	control->event_inj = 0;
+	svm_complete_interrupts(svm);
+}
+
 #ifdef CONFIG_X86_64
 #define R "r"
 #else
@@ -3523,6 +3534,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_irq = svm_set_irq,
 	.set_nmi = svm_inject_nmi,
 	.queue_exception = svm_queue_exception,
+	.cancel_injection = svm_cancel_injection,
 	.interrupt_allowed = svm_interrupt_allowed,
 	.nmi_allowed = svm_nmi_allowed,
 	.get_nmi_mask = svm_get_nmi_mask,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6d37df..41b1ff6 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3901,6 +3901,16 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 				  IDT_VECTORING_ERROR_CODE);
 }
 
+static void vmx_cancel_injection(struct kvm_vcpu *vcpu)
+{
+	__vmx_complete_interrupts(to_vmx(vcpu),
+				  vmcs_read32(VM_ENTRY_INTR_INFO_FIELD),
+				  VM_ENTRY_INSTRUCTION_LEN,
+				  VM_ENTRY_EXCEPTION_ERROR_CODE);
+
+	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);
+}
+
 /*
  * Failure to inject an interrupt should give us the information
  * in IDT_VECTORING_INFO_FIELD.  However, if the failure occurs
@@ -4354,6 +4364,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_irq = vmx_inject_irq,
 	.set_nmi = vmx_inject_nmi,
 	.queue_exception = vmx_queue_exception,
+	.cancel_injection = vmx_cancel_injection,
 	.interrupt_allowed = vmx_interrupt_allowed,
 	.nmi_allowed = vmx_nmi_allowed,
 	.get_nmi_mask = vmx_get_nmi_mask,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ac13791..bfe8f51 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4877,7 +4877,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	int r;
 	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
 		vcpu->run->request_interrupt_window;
-	bool req_event;
 
 	if (vcpu->requests) {
 		if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
@@ -4913,6 +4912,21 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	if (unlikely(r))
 		goto out;
 
+	if (kvm_check_request(KVM_REQ_EVENT, vcpu)) {
+		inject_pending_event(vcpu);
+
+		/* enable NMI/IRQ window open exits if needed */
+		if (vcpu->arch.nmi_pending)
+			kvm_x86_ops->enable_nmi_window(vcpu);
+		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
+			kvm_x86_ops->enable_irq_window(vcpu);
+
+		if (kvm_lapic_enabled(vcpu)) {
+			update_cr8_intercept(vcpu);
+			kvm_lapic_sync_to_vapic(vcpu);
+		}
+	}
+
 	preempt_disable();
 
 	kvm_x86_ops->prepare_guest_switch(vcpu);
@@ -4925,35 +4939,17 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
 
-	req_event = kvm_check_request(KVM_REQ_EVENT, vcpu);
-
 	if (!atomic_read(&vcpu->guest_mode) || vcpu->requests
 	    || need_resched() || signal_pending(current)) {
-		if (req_event)
-			kvm_make_request(KVM_REQ_EVENT, vcpu);
 		atomic_set(&vcpu->guest_mode, 0);
 		smp_wmb();
 		local_irq_enable();
 		preempt_enable();
+		kvm_x86_ops->cancel_injection(vcpu);
 		r = 1;
 		goto out;
 	}
 
-	if (req_event) {
-		inject_pending_event(vcpu);
-
-		/* enable NMI/IRQ window open exits if needed */
-		if (vcpu->arch.nmi_pending)
-			kvm_x86_ops->enable_nmi_window(vcpu);
-		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
-			kvm_x86_ops->enable_irq_window(vcpu);
-
-		if (kvm_lapic_enabled(vcpu)) {
-			update_cr8_intercept(vcpu);
-			kvm_lapic_sync_to_vapic(vcpu);
-		}
-	}
-
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
 
 	kvm_guest_enter();
-- 
1.7.1

[regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Jonathan Nieder]

Hi kvm-ers,

When I boot the HURD with

	kvm -m 768 -net nic,model=ne2k_pci -net user hurd-installed.qemu

it hangs and eventually produces two messages:

	hd0: unexpected_intr: status=0x58 { DriveReady SeekComplete DataRequest }
	hd0: irq timeout: status=0x58 { DriveReady Seek Complete DataRequest }

More details below[1].

Adding -no-kvm-irqchip to the kvm command line fixes it --- no
more hangs or confusing messages (thanks, Guillem!).

Bisects (thanks to Michael for the idea) to

	v2.6.37-rc1~142^2~39 (KVM: Check for pending events before
	attempting injection, 2010-07-27).

Bisection log and kernel configuration available upon request (but
probably not too relevant --- the 2.6.37 distro kernel from Debian
exhibits the same problem).  Reproducible with kvm/master (2d4b4d26,
2011-02-01).

CPU is a dual-core AMD Athlon II P360, family 16, model 6.

Any hints for tracking this down?  For those wanting to follow along
at home, you can find a HURD cd to try at [2].

Thanks for kvm --- now that I have hardware that can handle it, it's
become a favorite toy. :)

Jonathan

[1] Background: http://bugs.debian.org/612105
Result:

 1. Normal boot procedure.  Eventually the following message from HURD
    is on the console:

	start ext2fs: Hurd server bootstrap: ext2fs[device:hd0s1] exec

 2. Hang.  I hit ctrl+alt, "info registers", and so on in the hope of
    waking it up.  Eventually...

 3. A flood of repeated

	kb_setleds1: unexpected state (1)

    messages.

 4. Another message and another hang:

	hd0: unexpected_intr: status=0x58 { DriveReady SeekComplete DataRequest }
	hd0: irq timeout: status=0x58 { DriveReady Seek Complete DataRequest }

    (the two messages overlap sometimes).
[2] http://people.debian.org/~sthibault/hurd-i386/installer/cdimage/

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Avi Kivity]

On 02/07/2011 08:00 AM, Jonathan Nieder wrote:
> Hi kvm-ers,
>
> When I boot the HURD with
>
> 	kvm -m 768 -net nic,model=ne2k_pci -net user hurd-installed.qemu
>
> it hangs and eventually produces two messages:
>
> 	hd0: unexpected_intr: status=0x58 { DriveReady SeekComplete DataRequest }
> 	hd0: irq timeout: status=0x58 { DriveReady Seek Complete DataRequest }
>
> More details below[1].
>
> Adding -no-kvm-irqchip to the kvm command line fixes it --- no
> more hangs or confusing messages (thanks, Guillem!).
>
> Bisects (thanks to Michael for the idea) to
>
> 	v2.6.37-rc1~142^2~39 (KVM: Check for pending events before
> 	attempting injection, 2010-07-27).
>
> Bisection log and kernel configuration available upon request (but
> probably not too relevant --- the 2.6.37 distro kernel from Debian
> exhibits the same problem).  Reproducible with kvm/master (2d4b4d26,
> 2011-02-01).
>
> CPU is a dual-core AMD Athlon II P360, family 16, model 6.
>
> Any hints for tracking this down?  For those wanting to follow along
> at home, you can find a HURD cd to try at [2].
>

Reproduced on AMD, not on Intel.  Given that I see a few PIC and PIT 
PIOs before the hang, the problem is likely at the PIT.  Will look further.

-- 
error compiling committee.c: too many arguments to function

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Gleb Natapov]

On Mon, Feb 07, 2011 at 02:39:01PM +0200, Avi Kivity wrote:
> On 02/07/2011 08:00 AM, Jonathan Nieder wrote:
> >Hi kvm-ers,
> >
> >When I boot the HURD with
> >
> >	kvm -m 768 -net nic,model=ne2k_pci -net user hurd-installed.qemu
> >
> >it hangs and eventually produces two messages:
> >
> >	hd0: unexpected_intr: status=0x58 { DriveReady SeekComplete DataRequest }
> >	hd0: irq timeout: status=0x58 { DriveReady Seek Complete DataRequest }
> >
> >More details below[1].
> >
> >Adding -no-kvm-irqchip to the kvm command line fixes it --- no
> >more hangs or confusing messages (thanks, Guillem!).
> >
> >Bisects (thanks to Michael for the idea) to
> >
> >	v2.6.37-rc1~142^2~39 (KVM: Check for pending events before
> >	attempting injection, 2010-07-27).
> >
> >Bisection log and kernel configuration available upon request (but
> >probably not too relevant --- the 2.6.37 distro kernel from Debian
> >exhibits the same problem).  Reproducible with kvm/master (2d4b4d26,
> >2011-02-01).
> >
> >CPU is a dual-core AMD Athlon II P360, family 16, model 6.
> >
> >Any hints for tracking this down?  For those wanting to follow along
> >at home, you can find a HURD cd to try at [2].
> >
> 
> Reproduced on AMD, not on Intel.  Given that I see a few PIC and PIT
> PIOs before the hang, the problem is likely at the PIT.  Will look
> further.
> 
I can reproduce it on Intel. Looking into it. Looks like PIC isr_ack
problem again.

--
			Gleb.

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Gleb Natapov]

On Mon, Feb 07, 2011 at 02:45:13PM +0200, Gleb Natapov wrote:
> On Mon, Feb 07, 2011 at 02:39:01PM +0200, Avi Kivity wrote:
> > On 02/07/2011 08:00 AM, Jonathan Nieder wrote:
> > >Hi kvm-ers,
> > >
> > >When I boot the HURD with
> > >
> > >	kvm -m 768 -net nic,model=ne2k_pci -net user hurd-installed.qemu
> > >
> > >it hangs and eventually produces two messages:
> > >
> > >	hd0: unexpected_intr: status=0x58 { DriveReady SeekComplete DataRequest }
> > >	hd0: irq timeout: status=0x58 { DriveReady Seek Complete DataRequest }
> > >
> > >More details below[1].
> > >
> > >Adding -no-kvm-irqchip to the kvm command line fixes it --- no
> > >more hangs or confusing messages (thanks, Guillem!).
> > >
> > >Bisects (thanks to Michael for the idea) to
> > >
> > >	v2.6.37-rc1~142^2~39 (KVM: Check for pending events before
> > >	attempting injection, 2010-07-27).
> > >
> > >Bisection log and kernel configuration available upon request (but
> > >probably not too relevant --- the 2.6.37 distro kernel from Debian
> > >exhibits the same problem).  Reproducible with kvm/master (2d4b4d26,
> > >2011-02-01).
> > >
> > >CPU is a dual-core AMD Athlon II P360, family 16, model 6.
> > >
> > >Any hints for tracking this down?  For those wanting to follow along
> > >at home, you can find a HURD cd to try at [2].
> > >
> > 
> > Reproduced on AMD, not on Intel.  Given that I see a few PIC and PIT
> > PIOs before the hang, the problem is likely at the PIT.  Will look
> > further.
> > 
> I can reproduce it on Intel. Looking into it. Looks like PIC isr_ack
> problem again.
> 

Is this patch helps?

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 3cece05..62b1dde 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -549,6 +549,9 @@ static void pic_irq_request(struct kvm *kvm, int level)
 	struct kvm_pic *s = pic_irqchip(kvm);
 	int irq = pic_get_irq(&s->pics[0]);
 
+	if (s->output && !level)
+		s->pics[0].isr_ack = 0xff;
+
 	s->output = level;
 	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
 		s->pics[0].isr_ack &= ~(1 << irq);
--
			Gleb.

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Jonathan Nieder]

Gleb Natapov wrote:

> Is this patch helps?
> 
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 3cece05..62b1dde 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -549,6 +549,9 @@ static void pic_irq_request(struct kvm *kvm, int level)
>  	struct kvm_pic *s = pic_irqchip(kvm);
>  	int irq = pic_get_irq(&s->pics[0]);
>  
> +	if (s->output && !level)
> +		s->pics[0].isr_ack = 0xff;
> +

Yes, it does (tested on top of kvm/master).  Thanks!

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Michael Tokarev]

08.02.2011 04:40, Jonathan Nieder wrote:
> Gleb Natapov wrote:
> 
>> Is this patch helps?
>>
>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>> index 3cece05..62b1dde 100644
>> --- a/arch/x86/kvm/i8259.c
>> +++ b/arch/x86/kvm/i8259.c
>> @@ -549,6 +549,9 @@ static void pic_irq_request(struct kvm *kvm, int level)
>>  	struct kvm_pic *s = pic_irqchip(kvm);
>>  	int irq = pic_get_irq(&s->pics[0]);
>>  
>> +	if (s->output && !level)
>> +		s->pics[0].isr_ack = 0xff;
>> +
> 
> Yes, it does (tested on top of kvm/master).  Thanks!

And FWIW, I still can't trigger this issue on my machines -
with any of my custom kernels (2.6.37 and 2.6.38git included),
but debian 2.6.37-trunk-amd64 triggers it.  On all my kernels
hurd installation goes way further.

/mjt

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Gleb Natapov]

On Mon, Feb 07, 2011 at 07:40:55PM -0600, Jonathan Nieder wrote:
> Gleb Natapov wrote:
> 
> > Is this patch helps?
> > 
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 3cece05..62b1dde 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -549,6 +549,9 @@ static void pic_irq_request(struct kvm *kvm, int level)
> >  	struct kvm_pic *s = pic_irqchip(kvm);
> >  	int irq = pic_get_irq(&s->pics[0]);
> >  
> > +	if (s->output && !level)
> > +		s->pics[0].isr_ack = 0xff;
> > +
> 
> Yes, it does (tested on top of kvm/master).  Thanks!
Thanks for testing. Can you test this one too please:

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 3cece05..5528484 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -62,9 +62,6 @@ static void pic_unlock(struct kvm_pic *s)
 		}
 
 		if (!found)
-			found = s->kvm->bsp_vcpu;
-
-		if (!found)
 			return;
 
 		kvm_make_request(KVM_REQ_EVENT, found);
@@ -75,7 +72,6 @@ static void pic_unlock(struct kvm_pic *s)
 static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 {
 	s->isr &= ~(1 << irq);
-	s->isr_ack |= (1 << irq);
 	if (s != &s->pics_state->pics[0])
 		irq += 8;
 	/*
@@ -89,16 +85,6 @@ static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 	pic_lock(s->pics_state);
 }
 
-void kvm_pic_clear_isr_ack(struct kvm *kvm)
-{
-	struct kvm_pic *s = pic_irqchip(kvm);
-
-	pic_lock(s);
-	s->pics[0].isr_ack = 0xff;
-	s->pics[1].isr_ack = 0xff;
-	pic_unlock(s);
-}
-
 /*
  * set irq level. If an edge is detected, then the IRR is set to 1
  */
@@ -281,7 +267,6 @@ void kvm_pic_reset(struct kvm_kpic_state *s)
 	s->irr = 0;
 	s->imr = 0;
 	s->isr = 0;
-	s->isr_ack = 0xff;
 	s->priority_add = 0;
 	s->irq_base = 0;
 	s->read_reg_select = 0;
@@ -545,15 +530,11 @@ static int picdev_read(struct kvm_io_device *this,
  */
 static void pic_irq_request(struct kvm *kvm, int level)
 {
-	struct kvm_vcpu *vcpu = kvm->bsp_vcpu;
 	struct kvm_pic *s = pic_irqchip(kvm);
-	int irq = pic_get_irq(&s->pics[0]);
 
-	s->output = level;
-	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
-		s->pics[0].isr_ack &= ~(1 << irq);
+	if (!s->output)
 		s->wakeup_needed = true;
-	}
+	s->output = level;	
 }
 
 static const struct kvm_io_device_ops picdev_ops = {
@@ -575,8 +556,6 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 	s->pics[1].elcr_mask = 0xde;
 	s->pics[0].pics_state = s;
 	s->pics[1].pics_state = s;
-	s->pics[0].isr_ack = 0xff;
-	s->pics[1].isr_ack = 0xff;
 
 	/*
 	 * Initialize PIO device
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bcc0efc..2dc5fc4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2648,8 +2648,6 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 	vcpu->arch.interrupt.pending = events->interrupt.injected;
 	vcpu->arch.interrupt.nr = events->interrupt.nr;
 	vcpu->arch.interrupt.soft = events->interrupt.soft;
-	if (vcpu->arch.interrupt.pending && irqchip_in_kernel(vcpu->kvm))
-		kvm_pic_clear_isr_ack(vcpu->kvm);
 	if (events->flags & KVM_VCPUEVENT_VALID_SHADOW)
 		kvm_x86_ops->set_interrupt_shadow(vcpu,
 						  events->interrupt.shadow);
@@ -5617,8 +5615,6 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	if (pending_vec < max_bits) {
 		kvm_queue_interrupt(vcpu, pending_vec, false);
 		pr_debug("Set back pending irq %d\n", pending_vec);
-		if (irqchip_in_kernel(vcpu->kvm))
-			kvm_pic_clear_isr_ack(vcpu->kvm);
 	}
 
 	kvm_set_segment(vcpu, &sregs->cs, VCPU_SREG_CS);
--
			Gleb.

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Marcelo Tosatti]

On Tue, Feb 08, 2011 at 02:00:37PM +0200, Gleb Natapov wrote:
> On Mon, Feb 07, 2011 at 07:40:55PM -0600, Jonathan Nieder wrote:
> > Gleb Natapov wrote:
> > 
> > > Is this patch helps?
> > > 
> > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > index 3cece05..62b1dde 100644
> > > --- a/arch/x86/kvm/i8259.c
> > > +++ b/arch/x86/kvm/i8259.c
> > > @@ -549,6 +549,9 @@ static void pic_irq_request(struct kvm *kvm, int level)
> > >  	struct kvm_pic *s = pic_irqchip(kvm);
> > >  	int irq = pic_get_irq(&s->pics[0]);
> > >  
> > > +	if (s->output && !level)
> > > +		s->pics[0].isr_ack = 0xff;
> > > +
> > 
> > Yes, it does (tested on top of kvm/master).  Thanks!
> Thanks for testing. Can you test this one too please:
> 
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 3cece05..5528484 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -62,9 +62,6 @@ static void pic_unlock(struct kvm_pic *s)
>  		}
>  
>  		if (!found)
> -			found = s->kvm->bsp_vcpu;
> -
> -		if (!found)
>  			return;
>  
>  		kvm_make_request(KVM_REQ_EVENT, found);

Gleb,

I don't think the isr_ack logic is overly complex that it should be
removed. For some cases it is still beneficial, see example case on
commit e48258009d941, which is not handled by kick coalescing of
kvm_vcpu_kick. 

What is the problem with Hurd exactly?

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Gleb Natapov]

On Tue, Feb 08, 2011 at 12:22:53PM -0200, Marcelo Tosatti wrote:
> On Tue, Feb 08, 2011 at 02:00:37PM +0200, Gleb Natapov wrote:
> > On Mon, Feb 07, 2011 at 07:40:55PM -0600, Jonathan Nieder wrote:
> > > Gleb Natapov wrote:
> > > 
> > > > Is this patch helps?
> > > > 
> > > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > > index 3cece05..62b1dde 100644
> > > > --- a/arch/x86/kvm/i8259.c
> > > > +++ b/arch/x86/kvm/i8259.c
> > > > @@ -549,6 +549,9 @@ static void pic_irq_request(struct kvm *kvm, int level)
> > > >  	struct kvm_pic *s = pic_irqchip(kvm);
> > > >  	int irq = pic_get_irq(&s->pics[0]);
> > > >  
> > > > +	if (s->output && !level)
> > > > +		s->pics[0].isr_ack = 0xff;
> > > > +
> > > 
> > > Yes, it does (tested on top of kvm/master).  Thanks!
> > Thanks for testing. Can you test this one too please:
> > 
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 3cece05..5528484 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -62,9 +62,6 @@ static void pic_unlock(struct kvm_pic *s)
> >  		}
> >  
> >  		if (!found)
> > -			found = s->kvm->bsp_vcpu;
> > -
> > -		if (!found)
> >  			return;
> >  
> >  		kvm_make_request(KVM_REQ_EVENT, found);
> 
> Gleb,
> 
> I don't think the isr_ack logic is overly complex that it should be
Well we continue to find bugs in it.
 
> removed. For some cases it is still beneficial, see example case on
> commit e48258009d941, which is not handled by kick coalescing of
> kvm_vcpu_kick. 
I think the code I've send will not exhibit the behaviour described by
this commit since in that case s->output will never become zero
(kvm_apic_accept_pic_intr() should return false).

> 
> What is the problem with Hurd exactly?
The problem is not with Hurd, but with our pic emulation. PIT sends
interrupt while it is unmasked in PIC, HURD never gets it (probably
interrupts are disabled by vcpu), HURD masks PIT interrupt, then it
unmasks it and never gets it since previous one was not acked yet and
isr_ack logic prevent vcpu from been kicked again (and since KVM_REQ_EVENT
is set as part of kick logic vcpu never checks interrupt availability
again).

--
			Gleb.

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Avi Kivity]

On 02/08/2011 04:22 PM, Marcelo Tosatti wrote:
> I don't think the isr_ack logic is overly complex that it should be
> removed. For some cases it is still beneficial, see example case on
> commit e48258009d941, which is not handled by kick coalescing of
> kvm_vcpu_kick.

On the other hand, I think it can be done differently.  For example LVT0 
is probably programmed to mask interrupts; we can simply look at it and 
not kick if that's the case.  We can use notifiers from the lapic to the 
pic to avoid looking at lapic data.

The advantage in this way is that we avoid introducing state, instead 
relying on existing state.  New state is always bad since it has to be 
kept in sync with guest visible state.

-- 
error compiling committee.c: too many arguments to function

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Gleb Natapov]

On Tue, Feb 08, 2011 at 04:43:33PM +0200, Avi Kivity wrote:
> On 02/08/2011 04:22 PM, Marcelo Tosatti wrote:
> >I don't think the isr_ack logic is overly complex that it should be
> >removed. For some cases it is still beneficial, see example case on
> >commit e48258009d941, which is not handled by kick coalescing of
> >kvm_vcpu_kick.
> 
> On the other hand, I think it can be done differently.  For example
> LVT0 is probably programmed to mask interrupts; we can simply look
> at it and not kick if that's the case.  We can use notifiers from
> the lapic to the pic to avoid looking at lapic data.
> 
I believe this is what my patch is doing. Look at pic_unlock(). The code
search for vcpu to kick by calling kvm_apic_accept_pic_intr() function
(which checks that LVT is masked). If no vcpu is found we kicks bsp.
Why? I removed that.

> The advantage in this way is that we avoid introducing state,
> instead relying on existing state.  New state is always bad since it
> has to be kept in sync with guest visible state.
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Avi Kivity]

On 02/08/2011 04:47 PM, Gleb Natapov wrote:
> On Tue, Feb 08, 2011 at 04:43:33PM +0200, Avi Kivity wrote:
> >  On 02/08/2011 04:22 PM, Marcelo Tosatti wrote:
> >  >I don't think the isr_ack logic is overly complex that it should be
> >  >removed. For some cases it is still beneficial, see example case on
> >  >commit e48258009d941, which is not handled by kick coalescing of
> >  >kvm_vcpu_kick.
> >
> >  On the other hand, I think it can be done differently.  For example
> >  LVT0 is probably programmed to mask interrupts; we can simply look
> >  at it and not kick if that's the case.  We can use notifiers from
> >  the lapic to the pic to avoid looking at lapic data.
> >
> I believe this is what my patch is doing. Look at pic_unlock(). The code
> search for vcpu to kick by calling kvm_apic_accept_pic_intr() function
> (which checks that LVT is masked).

It does indeed.

> If no vcpu is found we kicks bsp.
> Why? I removed that.

The code that looks for a vcpu that has LVT0 unmasked is newer than the 
isr_ack code (see cfe149e91b82).  So it looks like the isr_ack code is 
indeed unnecessary now.

-- 
error compiling committee.c: too many arguments to function

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Marcelo Tosatti]

On Tue, Feb 08, 2011 at 04:57:16PM +0200, Avi Kivity wrote:
> On 02/08/2011 04:47 PM, Gleb Natapov wrote:
> >On Tue, Feb 08, 2011 at 04:43:33PM +0200, Avi Kivity wrote:
> >>  On 02/08/2011 04:22 PM, Marcelo Tosatti wrote:
> >>  >I don't think the isr_ack logic is overly complex that it should be
> >>  >removed. For some cases it is still beneficial, see example case on
> >>  >commit e48258009d941, which is not handled by kick coalescing of
> >>  >kvm_vcpu_kick.
> >>
> >>  On the other hand, I think it can be done differently.  For example
> >>  LVT0 is probably programmed to mask interrupts; we can simply look
> >>  at it and not kick if that's the case.  We can use notifiers from
> >>  the lapic to the pic to avoid looking at lapic data.
> >>
> >I believe this is what my patch is doing. Look at pic_unlock(). The code
> >search for vcpu to kick by calling kvm_apic_accept_pic_intr() function
> >(which checks that LVT is masked).
> 
> It does indeed.
> 
> >If no vcpu is found we kicks bsp.
> >Why? I removed that.
> 
> The code that looks for a vcpu that has LVT0 unmasked is newer than
> the isr_ack code (see cfe149e91b82).  So it looks like the isr_ack
> code is indeed unnecessary now.

Right. Patch looks fine.

Re: [regression] KVM: hangs and "irq timeout" booting HURD unless -no-kvm-irqchip passed

[Jonathan Nieder]

Gleb Natapov wrote:

> Thanks for testing. Can you test this one too please:
>
>  i8259.c |   25 ++-----------------------
>  x86.c   |    4 ----
>  2 files changed, 2 insertions(+), 27 deletions(-)

Yes, it works, too.  (Tested against v2.6.37.)