KVM: VMX: Add Posted Interrupt supporting

PATCH Superseded Yang Zhang 3 participants 31 messages Mar 15, 2013 – Mar 20, 2013

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Yang Zhang

author

2013-03-15 13:35 UTC #0 lore raw

v6 cover KVM: VMX: Add Posted Interrupt supporting

From: Yang Zhang <[email protected]>

The follwoing patches are adding the Posted Interrupt supporting to KVM:
The first patch enables the feature 'acknowledge interrupt on vmexit'.Since it is required by Posted interrupt, we need to enable it firstly.

And the subsequent patches are adding the posted interrupt supporting:
Posted Interrupt allows APIC interrupts to inject into guest directly without any vmexit.

- When delivering a interrupt to guest, if target vcpu is running, update Posted-interrupt requests bitmap and send a notification event to the vcpu. Then the vcpu will handle this interrupt automatically, without any software involvemnt.

- If target vcpu is not running or there already a notification event pending in the vcpu, do nothing. The interrupt will be handled by next vm entry

NOTE: We don't turn on the Posted Interrupt until the coalesced issue is solved.

Changes from v5 to v6:
* Split sync_pir_to_irr into two functions one to query whether PIR is empty and the other to perform the sync.
* Add comments to explain how vmx_sync_pir_to_irr() work.
* Rebase on top of KVM upstream.

Changes from v4 to v5:
* Add per cpu count for posted IPI handler.
* Dont' check irr when delivering interrupt. Since we can not get interrupt coalesced info with Posted Interrupt. So there is no need to check the irr. There is another patch will changed current interrupt coalesced logic. Before it, we will not turn on Posted Interrupt.
* Clear outstanding notification bit after call local_irq_disable, but before check request. As Marcelo suggested, we can ensure the IPI not lost in this way
* Remove the spinlock. Same as item 2, if not need to get coalesced info, then no need the lock.
* Rebase on top of KVM upstream.

Yang Zhang (5):
KVM: VMX: Enable acknowledge interupt on vmexit
KVM: VMX: Register a new IPI for posted interrupt
KVM: VMX: Check the posted interrupt capability
KVM: VMX: Add the algorithm of deliver posted interrupt KVM : VMX: Use posted interrupt to deliver virtual interrupt

 arch/x86/include/asm/entry_arch.h  |    4 +
 arch/x86/include/asm/hardirq.h     |    3 +
 arch/x86/include/asm/hw_irq.h      |    1 +
 arch/x86/include/asm/irq_vectors.h |    5 +
 arch/x86/include/asm/kvm_host.h    |    5 +
 arch/x86/include/asm/vmx.h         |    4 +
 arch/x86/kernel/entry_64.S         |    5 +
 arch/x86/kernel/irq.c              |   22 ++++
 arch/x86/kernel/irqinit.c          |    4 +
 arch/x86/kvm/irq.c                 |    3 +-
 arch/x86/kvm/lapic.c               |   29 ++++-
 arch/x86/kvm/lapic.h               |    2 +
 arch/x86/kvm/svm.c                 |   30 +++++
 arch/x86/kvm/vmx.c                 |  223 ++++++++++++++++++++++++++++++++----
 arch/x86/kvm/x86.c                 |    8 +-
 virt/kvm/kvm_main.c                |    1 +
 16 files changed, 320 insertions(+), 29 deletions(-)

Yang Zhang

author

2013-03-15 13:35 UTC #1 lore raw

PATCH v6 1/5 KVM: VMX: Enable acknowledge interupt on vmexit

From: Yang Zhang <[email protected]>

The "acknowledge interrupt on exit" feature controls processor behavior for external interrupt acknowledgement. When this control is set, the processor acknowledges the interrupt controller to acquire the interrupt vector on VM exit.

After enabling this feature, an interrupt which arrived when target cpu is running in vmx non-root mode will be handled by vmx handler instead of handler in idt. Currently, vmx handler only fakes an interrupt stack and jump to idt table to let real handler to handle it. Further, we will recognize the interrupt and only delivery the interrupt which not belong to current vcpu through idt table. The interrupt which belonged to current vcpu will be handled inside vmx handler. This will reduce the interrupt handle cost of KVM.

Also, interrupt enable logic is changed if this feature is turnning on:
Before this patch, hypervior call local_irq_enable() to enable it directly. Now IF bit is set on interrupt stack frame, and will be enabled on a return from interrupt handler if exterrupt interrupt exists. If no external interrupt, still call local_irq_enable() to enable it.

Refer to Intel SDM volum 3, chapter 33.2.

Signed-off-by Yang Zhang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/svm.c              |    6 ++++
 arch/x86/kvm/vmx.c              |   58 ++++++++++++++++++++++++++++++++++++---
 arch/x86/kvm/x86.c              |    4 ++-
 4 files changed, 64 insertions(+), 5 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -0

index 635a74d..b8388e9 100644
@@ -730,6 +730,7 @@ struct kvm_x86_ops {
 	int (*check_intercept)(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
 			       enum x86_intercept_stage stage);
+	void (*handle_external_intr)(struct kvm_vcpu *vcpu);
 };
 
 struct kvm_arch_async_pf {

arch/x86/kvm/svm.c +6 -0

index e1b1ce2..a7d60d7 100644
@@ -4247,6 +4247,11 @@ out:
 	return ret;
 }
 
+static void svm_handle_external_intr(struct kvm_vcpu *vcpu)
+{
+	local_irq_enable();
+}
+
 static struct kvm_x86_ops svm_x86_ops = {
 	.cpu_has_kvm_support = has_svm,
 	.disabled_by_bios = is_disabled,

@@ -4342,6 +4347,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_tdp_cr3 = set_tdp_cr3,
 
 	.check_intercept = svm_check_intercept,
+	.handle_external_intr = svm_handle_external_intr,
 };
 
 static int __init svm_init(void)

arch/x86/kvm/vmx.c +54 -4

106 lines not on this page · raw message

arch/x86/kvm/x86.c +3 -1

index d0cf737..0baa90d 100644
@@ -5784,7 +5784,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	vcpu->mode = OUTSIDE_GUEST_MODE;
 	smp_wmb();
-	local_irq_enable();
+
+	/* Interrupt is enabled by handle_external_intr() */
+	kvm_x86_ops->handle_external_intr(vcpu);
 
 	++vcpu->stat.exits;

Signature · 1 line

1.7.1

Yang Zhang

author

2013-03-15 13:35 UTC #2 lore raw

PATCH v6 2/5 KVM: VMX: Register a new IPI for posted interrupt

In reply to Yang Zhang

From: Yang Zhang <[email protected]>

Posted Interrupt feature requires a special IPI to deliver posted interrupt to guest. And it should has a high priority so the interrupt will not be blocked by others.
Normally, the posted interrupt will be consumed by vcpu if target vcpu is running and transparent to OS. But in some cases, the interrupt will arrive when target vcpu is scheduled out. And host will see it. So we need to register a dump handler to handle it.

Signed-off-by Yang Zhang <[email protected]>

---
 arch/x86/include/asm/entry_arch.h  |    4 ++++
 arch/x86/include/asm/hardirq.h     |    3 +++
 arch/x86/include/asm/hw_irq.h      |    1 +
 arch/x86/include/asm/irq_vectors.h |    5 +++++
 arch/x86/kernel/entry_64.S         |    5 +++++
 arch/x86/kernel/irq.c              |   22 ++++++++++++++++++++++
 arch/x86/kernel/irqinit.c          |    4 ++++
 7 files changed, 44 insertions(+), 0 deletions(-)

arch/x86/include/asm/entry_arch.h +4 -0

index 40afa00..9bd4eca 100644
@@ -19,6 +19,10 @@ BUILD_INTERRUPT(reboot_interrupt,REBOOT_VECTOR)
 
 BUILD_INTERRUPT(x86_platform_ipi, X86_PLATFORM_IPI_VECTOR)
 
+#ifdef CONFIG_HAVE_KVM
+BUILD_INTERRUPT(kvm_posted_intr_ipi, POSTED_INTR_VECTOR)
+#endif
+
 /*
  * every pentium local APIC has two 'local interrupts', with a
  * soft-definable vector attached to both interrupts, one of

arch/x86/include/asm/hardirq.h +3 -0

index 81f04ce..ab0ae1a 100644
@@ -12,6 +12,9 @@ typedef struct {
 	unsigned int irq_spurious_count;
 	unsigned int icr_read_retry_count;
 #endif
+#ifdef CONFIG_HAVE_KVM
+	unsigned int kvm_posted_intr_ipis;
+#endif
 	unsigned int x86_platform_ipis;	/* arch dependent */
 	unsigned int apic_perf_irqs;
 	unsigned int apic_irq_work_irqs;

arch/x86/include/asm/hw_irq.h +1 -0

index eb92a6e..cebef02 100644
@@ -28,6 +28,7 @@
 /* Interrupt handlers registered during init_IRQ */
 extern void apic_timer_interrupt(void);
 extern void x86_platform_ipi(void);
+extern void kvm_posted_intr_ipi(void);
 extern void error_interrupt(void);
 extern void irq_work_interrupt(void);

arch/x86/include/asm/irq_vectors.h +5 -0

index 1508e51..774dc9f 100644
@@ -102,6 +102,11 @@
  */
 #define X86_PLATFORM_IPI_VECTOR		0xf7
 
+/* Vector for KVM to deliver posted interrupt IPI */
+#ifdef CONFIG_HAVE_KVM
+#define POSTED_INTR_VECTOR		0xf2
+#endif
+
 /*
  * IRQ work vector:
  */

arch/x86/kernel/entry_64.S +5 -0

index 70641af..781bc06 100644
@@ -1177,6 +1177,11 @@ apicinterrupt LOCAL_TIMER_VECTOR \
 apicinterrupt X86_PLATFORM_IPI_VECTOR \
 	x86_platform_ipi smp_x86_platform_ipi
 
+#ifdef CONFIG_HAVE_KVM
+apicinterrupt POSTED_INTR_VECTOR \
+	kvm_posted_intr_ipi smp_kvm_posted_intr_ipi
+#endif
+
 apicinterrupt THRESHOLD_APIC_VECTOR \
 	threshold_interrupt smp_threshold_interrupt
 apicinterrupt THERMAL_APIC_VECTOR \

arch/x86/kernel/irq.c +22 -0

index e4595f1..6ae6ea1 100644
@@ -228,6 +228,28 @@ void smp_x86_platform_ipi(struct pt_regs *regs)
 	set_irq_regs(old_regs);
 }
 
+#ifdef CONFIG_HAVE_KVM
+/*
+ * Handler for POSTED_INTERRUPT_VECTOR.
+ */
+void smp_kvm_posted_intr_ipi(struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	ack_APIC_irq();
+
+	irq_enter();
+
+	exit_idle();
+
+	inc_irq_stat(kvm_posted_intr_ipis);
+
+	irq_exit();
+
+	set_irq_regs(old_regs);
+}
+#endif
+
 EXPORT_SYMBOL_GPL(vector_used_by_percpu_irq);
 
 #ifdef CONFIG_HOTPLUG_CPU

arch/x86/kernel/irqinit.c +4 -0

index 6e03b0d..2329a54 100644
@@ -205,6 +205,10 @@ static void __init apic_intr_init(void)
 
 	/* IPI for X86 platform specific use */
 	alloc_intr_gate(X86_PLATFORM_IPI_VECTOR, x86_platform_ipi);
+#ifdef CONFIG_HAVE_KVM
+	/* IPI for KVM to deliver posted interrupt */
+	alloc_intr_gate(POSTED_INTR_VECTOR, kvm_posted_intr_ipi);
+#endif
 
 	/* IPI vectors for APIC spurious and error interrupts */
 	alloc_intr_gate(SPURIOUS_APIC_VECTOR, spurious_interrupt);

Signature · 1 line

1.7.1

Yang Zhang

author

2013-03-15 13:35 UTC #3 lore raw

PATCH v6 3/5 KVM: VMX: Check the posted interrupt capability

In reply to Yang Zhang

From: Yang Zhang <[email protected]>

Detect the posted interrupt feature. If it exists, then set it in vmcs_config.

Signed-off-by Yang Zhang <[email protected]>

---
 arch/x86/include/asm/vmx.h |    4 ++
 arch/x86/kvm/vmx.c         |   87 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 71 insertions(+), 20 deletions(-)

arch/x86/include/asm/vmx.h +4 -0

index 5c9dbad..ce8ac80 100644
@@ -158,6 +158,7 @@
 #define PIN_BASED_EXT_INTR_MASK                 0x00000001
 #define PIN_BASED_NMI_EXITING                   0x00000008
 #define PIN_BASED_VIRTUAL_NMIS                  0x00000020
+#define PIN_BASED_POSTED_INTR                   0x00000080
 
 #define VM_EXIT_SAVE_DEBUG_CONTROLS             0x00000002
 #define VM_EXIT_HOST_ADDR_SPACE_SIZE            0x00000200

@@ -180,6 +181,7 @@
 /* VMCS Encodings */
 enum vmcs_field {
 	VIRTUAL_PROCESSOR_ID            = 0x00000000,
+	POSTED_INTR_NV                  = 0x00000002,
 	GUEST_ES_SELECTOR               = 0x00000800,
 	GUEST_CS_SELECTOR               = 0x00000802,
 	GUEST_SS_SELECTOR               = 0x00000804,

@@ -214,6 +216,8 @@ enum vmcs_field {
 	VIRTUAL_APIC_PAGE_ADDR_HIGH     = 0x00002013,
 	APIC_ACCESS_ADDR		= 0x00002014,
 	APIC_ACCESS_ADDR_HIGH		= 0x00002015,
+	POSTED_INTR_DESC_ADDR           = 0x00002016,
+	POSTED_INTR_DESC_ADDR_HIGH      = 0x00002017,
 	EPT_POINTER                     = 0x0000201a,
 	EPT_POINTER_HIGH                = 0x0000201b,
 	EOI_EXIT_BITMAP0                = 0x0000201c,

arch/x86/kvm/vmx.c +67 ~~-20~~

178 lines not on this page · raw message

Signature · 1 line

1.7.1

Yang Zhang

author

2013-03-15 13:35 UTC #4 lore raw

PATCH v6 4/5 KVM: VMX: Add the algorithm of deliver posted interrupt

In reply to Yang Zhang

From: Yang Zhang <[email protected]>

Only deliver the posted interrupt when target vcpu is running and there is no previous interrupt pending in pir.

Signed-off-by Yang Zhang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    4 ++
 arch/x86/kvm/lapic.c            |   13 +++++++
 arch/x86/kvm/lapic.h            |    1 +
 arch/x86/kvm/svm.c              |   24 ++++++++++++++
 arch/x86/kvm/vmx.c              |   67 +++++++++++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c             |    1 +
 6 files changed, 110 insertions(+), 0 deletions(-)

arch/x86/include/asm/kvm_host.h +4 -0

index b8388e9..eac50d3 100644
@@ -704,6 +704,10 @@ struct kvm_x86_ops {
 	void (*hwapic_isr_update)(struct kvm *kvm, int isr);
 	void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
 	void (*set_virtual_x2apic_mode)(struct kvm_vcpu *vcpu, bool set);
+	bool (*deliver_posted_interrupt)(struct kvm_vcpu *vcpu, int vector);
+	void (*posted_intr_clear_on)(struct kvm_vcpu *vcpu);
+	void (*sync_pir_to_irr)(struct kvm_vcpu *vcpu);
+	bool (*hwapic_has_interrupt)(struct kvm_vcpu *vcpu);
 	int (*set_tss_addr)(struct kvm *kvm, unsigned int addr);
 	int (*get_tdp_level)(void);
 	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);

arch/x86/kvm/lapic.c +13 -0

index 02b51dd..b3ea50e 100644
@@ -357,6 +357,19 @@ static u8 count_vectors(void *bitmap)
 	return count;
 }
 
+void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir)
+{
+	u32 i, pir_val;
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	for (i = 0; i <= 7; i++) {
+		pir_val = xchg(&pir[i], 0);
+		if (pir_val)
+			*((u32 *)(apic->regs + APIC_IRR + i * 0x10)) |= pir_val;
+	}
+}
+EXPORT_SYMBOL_GPL(kvm_apic_update_irr);
+
 static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
 {
 	apic->irr_pending = true;

arch/x86/kvm/lapic.h +1 -0

index 1676d34..e5327be 100644
@@ -157,5 +157,6 @@ static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
 void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 				struct kvm_lapic_irq *irq,
 				u64 *eoi_bitmap);
+void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
 
 #endif

arch/x86/kvm/svm.c +24 -0

index a7d60d7..6382d09 100644
@@ -3591,6 +3591,26 @@ static void svm_hwapic_isr_update(struct kvm *kvm, int isr)
 	return;
 }
 
+static bool svm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+
+static void svm_sync_pir_to_irr(struct kvm_vcpu *vcpu)
+{
+	return;
+}
+
+static bool svm_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
+{
+	return false;
+}
+
+static void svm_posted_intr_clear_on(struct kvm_vcpu *vcpu)
+{
+	return;
+}
+
 static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4319,6 +4339,10 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.vm_has_apicv = svm_vm_has_apicv,
 	.load_eoi_exitmap = svm_load_eoi_exitmap,
 	.hwapic_isr_update = svm_hwapic_isr_update,
+	.sync_pir_to_irr = svm_sync_pir_to_irr,
+	.hwapic_has_interrupt = svm_hwapic_has_interrupt,
+	.deliver_posted_interrupt = svm_deliver_posted_interrupt,
+	.posted_intr_clear_on = svm_posted_intr_clear_on,
 
 	.set_tss_addr = svm_set_tss_addr,
 	.get_tdp_level = get_npt_level,

arch/x86/kvm/vmx.c +67 -0

85 lines not on this page · raw message

virt/kvm/kvm_main.c +1 -0

index adc68fe..cd73e0e 100644
@@ -1692,6 +1692,7 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 			smp_send_reschedule(cpu);
 	put_cpu();
 }
+EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
 #endif /* !CONFIG_S390 */
 
 void kvm_resched(struct kvm_vcpu *vcpu)

Signature · 1 line

1.7.1

Yang Zhang

author

2013-03-15 13:35 UTC #5 lore raw

PATCH v6 5/5 KVM : VMX: Use posted interrupt to deliver virtual interrupt

In reply to Yang Zhang

From: Yang Zhang <[email protected]>

If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.

Signed-off-by Yang Zhang <[email protected]>

---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)

arch/x86/kvm/irq.c +2 -1

index 484bc87..5179988 100644
@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 	if (kvm_cpu_has_extint(v))
 		return 1;
 
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);

arch/x86/kvm/lapic.c +13 -3

index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
 
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {

@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			break;
 		}
 
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;
 
 	case APIC_DM_REMRD:

@@ -1604,6 +1609,11 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
 	return highest_irr;
 }
 
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);

arch/x86/kvm/lapic.h +1 -0

index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);

arch/x86/kvm/vmx.c +11 -0

index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct kvm_vcpu *vcpu)
 	clear_bit(POSTED_INTR_ON, (unsigned long *)&vmx->pi_desc.u.control);
 }
 
+/*
+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */
 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

arch/x86/kvm/x86.c +4 -0

index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
 				    struct kvm_lapic_state *s)
 {
+	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0;

@@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
+		kvm_x86_ops->sync_pir_to_irr(vcpu);
 		inject_pending_event(vcpu);
 
 		/* enable NMI/IRQ window open exits if needed */

@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
 
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
 	if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
 	    || need_resched() || signal_pending(current)) {
 		vcpu->mode = OUTSIDE_GUEST_MODE;

Signature · 1 line

1.7.1

Gleb Natapov

2013-03-19 08:54 UTC #6 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 19 lines · Yang Zhang

From: Yang Zhang <[email protected]>
If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.
Signed-off-by Yang Zhang <[email protected]>
---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)
arch/x86/kvm/irq.c +0 -0
index 484bc87..5179988 100644

arch/x86/kvm/irq.c int kvm_cpu_has_interrupt(struct kvm_vcpu *v) · line 85

Quoted 6 lines

arch/x86/kvm/irq.c +2 -1

@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 	if (kvm_cpu_has_extint(v))
 		return 1;
 
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);

That's incorrect. kvm_cpu_has_interrupt() should return true only it there is IRR suitable to be injected, not just any IRR. kvm_apic_has_interrupt() should call kvm_apic_update_irr().

Quoted 20 lines

arch/x86/kvm/irq.c +0 -0

 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);

arch/x86/kvm/lapic.c +4 -1

index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
 
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {

arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 733

Quoted 10 lines

arch/x86/kvm/lapic.c +4 -2

@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			break;
 		}
 
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;

apicv code and non apicv code are completely different. What's the point checking for apicv twice here?
Just do:

if (kvm_x86_ops->deliver_posted_interrupt)

	kvm_x86_ops->deliver_posted_interrupt(vcpu, vector)
else {
	result = !apic_test_and_set_irr(vector, apic);
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
}

And set kvm_x86_ops->deliver_posted_interrupt only if apicv is enabled.

Also rearrange patches so that APIC_TMR handling goes before posted interrupt series.

Quoted 35 lines

arch/x86/kvm/lapic.c +5 -0

 	case APIC_DM_REMRD:

@@ -1604,6 +1609,11 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
 	return highest_irr;
 }
 
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);

arch/x86/kvm/lapic.h +1 -0

index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu);
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);

arch/x86/kvm/vmx.c +1 -0

index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct kvm_vcpu *vcpu)
 	clear_bit(POSTED_INTR_ON, (unsigned long *)&vmx->pi_desc.u.control);
 }
 
+/*

arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct kvm_vcpu *vcpu) · line 3945

Quoted 10 lines

arch/x86/kvm/vmx.c +10 -0

+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */

The comment should go into previous patch. Also I prefer to not check for posted interrupt inside the callback, but set it to NULL instead. This way we avoid calling a callback on a hot path needlessly.

Quoted 24 lines

arch/x86/kvm/vmx.c +0 -0

 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

arch/x86/kvm/x86.c +2 -0

index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
 				    struct kvm_lapic_state *s)
 {
+	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0;

@@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
+		kvm_x86_ops->sync_pir_to_irr(vcpu);
 		inject_pending_event(vcpu);
 
 		/* enable NMI/IRQ window open exits if needed */

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5747

Quoted 5 lines

arch/x86/kvm/x86.c +2 -0

@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
 
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+

Why is this separate from pir_to_irr syncing?

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

 	if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
 	    || need_resched() || signal_pending(current)) {
 		vcpu->mode = OUTSIDE_GUEST_MODE;

Signature · 1 line

1.7.1

--
Gleb.

Zhang, Yang Z

author

2013-03-19 12:12 UTC #7 lore raw

RE: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Gleb Natapov wrote on 2013-03-19:

Quoted 19 lines · Yang Zhang

Quoted 18 lines
From: Yang Zhang <[email protected]>
If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.
Signed-off-by Yang Zhang <[email protected]>
---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)
arch/x86/kvm/irq.c +0 -0
index 484bc87..5179988 100644
@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)

Quoted 8 lines

arch/x86/kvm/irq.c int kvm_cpu_has_interrupt(struct kvm_vcpu *v) · line 85
Quoted 5 lines
arch/x86/kvm/irq.c +2 -1
 	if (kvm_cpu_has_extint(v))
 		return 1;
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);
That's incorrect. kvm_cpu_has_interrupt() should return true only it there is IRR suitable to be injected, not just any IRR. kvm_apic_has_interrupt() should call kvm_apic_update_irr().

You are right.

Quoted 41 lines

arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 7 lines
arch/x86/kvm/irq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
arch/x86/kvm/lapic.c +0 -0
index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 11 lines
arch/x86/kvm/lapic.c +4 -1
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {
@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 733
Quoted 9 lines
arch/x86/kvm/lapic.c +4 -2
 			break;
 		}
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;
apicv code and non apicv code are completely different. What's the point checking for apicv twice here?
Just do:
if (kvm_x86_ops->deliver_posted_interrupt)
	kvm_x86_ops->deliver_posted_interrupt(vcpu, vector)
else {
	result = !apic_test_and_set_irr(vector, apic);
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
}

Quoted 4 lines

And set kvm_x86_ops->deliver_posted_interrupt only if apicv is enabled.
Also rearrange patches so that APIC_TMR handling goes before posted interrupt series.

Sure.

Quoted 42 lines

arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct

Quoted 28 lines

arch/x86/kvm/lapic.c +5 -0

 	case APIC_DM_REMRD: @@ -1604,6 +1609,11 @@ int
 kvm_apic_has_interrupt(struct kvm_vcpu *vcpu) 	return highest_irr; }
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);

arch/x86/kvm/lapic.h +0 -0

index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu); +bool
 kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu); int
 kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu); int
 kvm_get_apic_interrupt(struct kvm_vcpu *vcpu); void
 kvm_lapic_reset(struct kvm_vcpu *vcpu);

arch/x86/kvm/vmx.c +0 -0

index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct

kvm_vcpu *vcpu)

arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct kvm_vcpu *vcpu) · line 3945

Quoted 13 lines

arch/x86/kvm/vmx.c +11 -0

 	clear_bit(POSTED_INTR_ON, (unsigned long *)&vmx->pi_desc.u.control);
 }
+/*
+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */

Quoted 3 lines

The comment should go into previous patch. Also I prefer to not check for posted interrupt inside the callback, but set it to NULL instead. This way we avoid calling a callback on a hot path needlessly.

It's make sense. So just follow the logic you mentioned above?

Quoted 18 lines

arch/x86/kvm/vmx.c +0 -0

 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

arch/x86/kvm/x86.c +0 -0

index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu, 				   
 struct kvm_lapic_state *s) { +	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0; @@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct
 kvm_vcpu *vcpu) 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
 +		kvm_x86_ops->sync_pir_to_irr(vcpu); 		inject_pending_event(vcpu);

Quoted 7 lines

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5746
Quoted 6 lines
arch/x86/kvm/x86.c +2 -0
 		/* enable NMI/IRQ window open exits if needed */
@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?

This is the result of discussion with Marcelo.
It is more reasonable to put it here to avoid unnecessary posted interrupt between:

vcpu->mode = IN_GUEST_MODE;

<--interrupt may arrived here and this is unnecessary.

local_irq_disable();

Quoted 8 lines

Quoted 5 lines

arch/x86/kvm/x86.c +0 -1

 	if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
 	    || need_resched() || signal_pending(current)) {
 		vcpu->mode = OUTSIDE_GUEST_MODE;
--

1.7.1

--
Gleb.

Best regards,
Yang

Gleb Natapov

2013-03-19 12:24 UTC #8 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 119 lines · Yang Z

Gleb Natapov wrote on 2013-03-19:
Quoted 19 lines · Yang Zhang
Quoted 18 lines
From: Yang Zhang <[email protected]>
If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.
Signed-off-by Yang Zhang <[email protected]>
---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)
arch/x86/kvm/irq.c +0 -0
index 484bc87..5179988 100644
@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
Quoted 8 lines
arch/x86/kvm/irq.c int kvm_cpu_has_interrupt(struct kvm_vcpu *v) · line 85
Quoted 5 lines
arch/x86/kvm/irq.c +2 -1
 	if (kvm_cpu_has_extint(v))
 		return 1;
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);
That's incorrect. kvm_cpu_has_interrupt() should return true only it there is IRR suitable to be injected, not just any IRR. kvm_apic_has_interrupt() should call kvm_apic_update_irr().
You are right.
Quoted 41 lines
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 7 lines
arch/x86/kvm/irq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
arch/x86/kvm/lapic.c +0 -0
index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 11 lines
arch/x86/kvm/lapic.c +4 -1
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {
@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 733
Quoted 9 lines
arch/x86/kvm/lapic.c +4 -2
 			break;
 		}
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;
apicv code and non apicv code are completely different. What's the point checking for apicv twice here?
Just do:
if (kvm_x86_ops->deliver_posted_interrupt)
	kvm_x86_ops->deliver_posted_interrupt(vcpu, vector)
else {
	result = !apic_test_and_set_irr(vector, apic);
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
}
Quoted 4 lines
And set kvm_x86_ops->deliver_posted_interrupt only if apicv is enabled.
Also rearrange patches so that APIC_TMR handling goes before posted interrupt series.
Sure.
Quoted 42 lines
arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct
Quoted 28 lines
arch/x86/kvm/lapic.c +5 -0
 	case APIC_DM_REMRD: @@ -1604,6 +1609,11 @@ int
 kvm_apic_has_interrupt(struct kvm_vcpu *vcpu) 	return highest_irr; }
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);
arch/x86/kvm/lapic.h +0 -0
index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu); +bool
 kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu); int
 kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu); int
 kvm_get_apic_interrupt(struct kvm_vcpu *vcpu); void
 kvm_lapic_reset(struct kvm_vcpu *vcpu);
arch/x86/kvm/vmx.c +0 -0
index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct
kvm_vcpu *vcpu)
Quoted 13 lines
arch/x86/kvm/vmx.c +11 -0
 	clear_bit(POSTED_INTR_ON, (unsigned long *)&vmx->pi_desc.u.control);
 }
+/*
+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */

Quoted 5 lines

Quoted 3 lines
The comment should go into previous patch. Also I prefer to not check for posted interrupt inside the callback, but set it to NULL instead. This way we avoid calling a callback on a hot path needlessly.
It's make sense. So just follow the logic you mentioned above?

Yes.

Quoted 25 lines

Quoted 18 lines

arch/x86/kvm/vmx.c +0 -0

 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

arch/x86/kvm/x86.c +0 -0

index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu, 				   
 struct kvm_lapic_state *s) { +	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0; @@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct
 kvm_vcpu *vcpu) 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
 +		kvm_x86_ops->sync_pir_to_irr(vcpu); 		inject_pending_event(vcpu);

Quoted 7 lines

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5746
Quoted 6 lines
arch/x86/kvm/x86.c +2 -0
 		/* enable NMI/IRQ window open exits if needed */
@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?

Quoted 9 lines

This is the result of discussion with Marcelo.
It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();

But this still can happen as far as I see:

vcpu0 vcpu1:
pi_test_and_set_pir()
kvm_make_request(KVM_REQ_EVENT)

                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode = IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()

May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?

--
Gleb.

Zhang, Yang Z

author

2013-03-19 12:42 UTC #9 lore raw

RE: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Gleb Natapov wrote on 2013-03-19:

Quoted 170 lines · Yang Z

Quoted 30 lines
Gleb Natapov wrote on 2013-03-19:
Quoted 19 lines · Yang Zhang
Quoted 18 lines
From: Yang Zhang <[email protected]>
If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.
Signed-off-by Yang Zhang <[email protected]>
---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)
arch/x86/kvm/irq.c +0 -0
index 484bc87..5179988 100644
@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
Quoted 8 lines
arch/x86/kvm/irq.c int kvm_cpu_has_interrupt(struct kvm_vcpu *v) · line 85
Quoted 5 lines
arch/x86/kvm/irq.c +2 -1
 	if (kvm_cpu_has_extint(v))
 		return 1;
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);
That's incorrect. kvm_cpu_has_interrupt() should return true only it there is IRR suitable to be injected, not just any IRR. kvm_apic_has_interrupt() should call kvm_apic_update_irr().
You are right.
Quoted 7 lines
Quoted 7 lines
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic,
Quoted 7 lines
arch/x86/kvm/irq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
arch/x86/kvm/lapic.c +0 -0
index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic,
int
Quoted 12 lines
Quoted 12 lines
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic,
Quoted 11 lines
arch/x86/kvm/lapic.c +4 -1
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {
@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic,
int
Quoted 70 lines
Quoted 22 lines
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 733
Quoted 9 lines
arch/x86/kvm/lapic.c +4 -2
 			break;
 		}
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;
apicv code and non apicv code are completely different. What's the point checking for apicv twice here?
Just do:
if (kvm_x86_ops->deliver_posted_interrupt)
	kvm_x86_ops->deliver_posted_interrupt(vcpu, vector)
else {
	result = !apic_test_and_set_irr(vector, apic);
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
}
Quoted 4 lines
And set kvm_x86_ops->deliver_posted_interrupt only if apicv is enabled.
Also rearrange patches so that APIC_TMR handling goes before posted interrupt series.
Sure.
Quoted 42 lines
arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct
Quoted 28 lines
arch/x86/kvm/lapic.c +5 -0
 	case APIC_DM_REMRD: @@ -1604,6 +1609,11 @@ int
 kvm_apic_has_interrupt(struct kvm_vcpu *vcpu) 	return highest_irr; }
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);
arch/x86/kvm/lapic.h +0 -0
index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu); +bool
 kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu); int
 kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu); int
 kvm_get_apic_interrupt(struct kvm_vcpu *vcpu); void
 kvm_lapic_reset(struct kvm_vcpu *vcpu);
arch/x86/kvm/vmx.c +0 -0
index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct
kvm_vcpu *vcpu)
Quoted 13 lines
arch/x86/kvm/vmx.c +11 -0
 	clear_bit(POSTED_INTR_ON, (unsigned long
 *)&vmx->pi_desc.u.control); }
+/*
+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */
Quoted 5 lines
Quoted 3 lines
The comment should go into previous patch. Also I prefer to not check for posted interrupt inside the callback, but set it to NULL instead. This way we avoid calling a callback on a hot path needlessly.
It's make sense. So just follow the logic you mentioned above?
Yes.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
arch/x86/kvm/vmx.c +0 -0
 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int
 vector) { 	struct vcpu_vmx *vmx = to_vmx(vcpu);
arch/x86/kvm/x86.c +0 -0
index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
 
Quoted 8 lines
Quoted 8 lines
arch/x86/kvm/x86.c void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
Quoted 8 lines
arch/x86/kvm/x86.c +0 -0
 struct kvm_lapic_state *s) { +	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0; @@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct
 kvm_vcpu *vcpu) 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
 +		kvm_x86_ops->sync_pir_to_irr(vcpu);
	inject_pending_event(vcpu);
Quoted 9 lines
Quoted 9 lines
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu · line 5747
Quoted 8 lines
arch/x86/kvm/x86.c +2 -0
 		/* enable NMI/IRQ window open exits if needed */
@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu
 *vcpu)
 
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)

Quoted 10 lines

                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()

Current solution is trying to block other Posted Interrupt from other VCPUs at same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.

Quoted 1 line

May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?

Yes, this will solve it. But I am not sure whether it will introduce any regressions. Is there any check relies on this sequence?

Quoted 2 lines

--
Gleb.

Best regards,
Yang

Gleb Natapov

2013-03-19 13:29 UTC #10 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 28 lines · Yang Z

Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5747
Quoted 3 lines
arch/x86/kvm/x86.c +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
Quoted 10 lines
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()

Quoted 2 lines

Current solution is trying to block other Posted Interrupt from other VCPUs at same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.

I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:

cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A)
set KVM_REQ_EVENT

                                                        process REQ_EVENT
                                                        PIR-A->IRR

                                                        vcpu->mode=IN_GUEST

if (vcpu0->guest_mode)

        if (!t_a_s_bit(PIR notif))
                send IPI
                                                        linux_pir_handler

                                        t_a_s_b(PIR-B)=1
                                        no PIR IPI sent

But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on()
kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask()
else
kvm_vcpu_kick()

And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()

What are the downsides? Can we lost interrupts this way?

Quoted 3 lines

Quoted 1 line
May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?
Yes, this will solve it. But I am not sure whether it will introduce any regressions. Is there any check relies on this sequence?

Do not think so.

--
Gleb.

Zhang, Yang Z

author

2013-03-19 13:59 UTC #11 lore raw

RE: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Gleb Natapov wrote on 2013-03-19:

Quoted 29 lines · Yang Z

Quoted 28 lines
Quoted 28 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5747
Quoted 3 lines
arch/x86/kvm/x86.c +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()

Quoted 8 lines

Quoted 1 line
Current solution is trying to block other Posted Interrupt from other VCPUs at
same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
Quoted 1 line
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:

The IPI will not lost in his example(he misread the patch).

Quoted 16 lines

cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A) set KVM_REQ_EVENT
                                                        process
                                                        REQ_EVENT
                                                        PIR-A->IRR
vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
linux_pir_handler
t_a_s_b(PIR-B)=1 no PIR IPI sent

Quoted 11 lines

But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on() kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask() else kvm_vcpu_kick()
And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()
What are the downsides? Can we lost interrupts this way?

Need to check guest mode before sending IPI. Otherwise hypervisor may receive IPI.
I think current logic is ok. Only problem is that when to clear Outstanding Notification bit. Actually I prefer your suggestion to clear it before sync_pir_irr. But Marcelo prefer to clear ON bit after disabling irq.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?
Yes, this will solve it. But I am not sure whether it will introduce any regressions. Is there any check relies on this sequence?
Do not think so.
--
Gleb.

Best regards,
Yang

Gleb Natapov

2013-03-19 14:51 UTC #12 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 67 lines · Yang Z

Gleb Natapov wrote on 2013-03-19:
Quoted 29 lines · Yang Z
Quoted 28 lines
Quoted 28 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()
Quoted 8 lines
Quoted 1 line
Current solution is trying to block other Posted Interrupt from other VCPUs at
same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
Quoted 1 line
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:
The IPI will not lost in his example(he misread the patch).
Quoted 16 lines
cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A) set KVM_REQ_EVENT
                                                        process
                                                        REQ_EVENT
                                                        PIR-A->IRR
vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
linux_pir_handler
t_a_s_b(PIR-B)=1 no PIR IPI sent
Quoted 11 lines
But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on() kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask() else kvm_vcpu_kick()
And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()
What are the downsides? Can we lost interrupts this way?

Quoted 1 line

Need to check guest mode before sending IPI. Otherwise hypervisor may receive IPI.

Of course, forget it. So if (!r) should be if (!r && mode == IN_GUEST)

Quoted 1 line

I think current logic is ok. Only problem is that when to clear Outstanding Notification bit. Actually I prefer your suggestion to clear it before sync_pir_irr. But Marcelo prefer to clear ON bit after disabling irq.

Marcelo what is the problem with the logic above?

--
Gleb.

Gleb Natapov

2013-03-19 15:12 UTC #13 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 68 lines · Gleb Natapov

Quoted 67 lines · Yang Z
Gleb Natapov wrote on 2013-03-19:
Quoted 29 lines · Yang Z
Quoted 28 lines
Quoted 28 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()
Quoted 8 lines
Quoted 1 line
Current solution is trying to block other Posted Interrupt from other VCPUs at
same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
Quoted 1 line
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:
The IPI will not lost in his example(he misread the patch).
Quoted 27 lines
cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A) set KVM_REQ_EVENT
                                                        process
                                                        REQ_EVENT
                                                        PIR-A->IRR
vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
linux_pir_handler
t_a_s_b(PIR-B)=1 no PIR IPI sent
But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on() kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask() else kvm_vcpu_kick()
And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()
What are the downsides? Can we lost interrupts this way?

Quoted 6 lines

Quoted 1 line
Need to check guest mode before sending IPI. Otherwise hypervisor may receive IPI.
Of course, forget it. So if (!r) should be if (!r && mode == IN_GUEST)
Quoted 1 line
I think current logic is ok. Only problem is that when to clear Outstanding Notification bit. Actually I prefer your suggestion to clear it before sync_pir_irr. But Marcelo prefer to clear ON bit after disabling irq.
Marcelo what is the problem with the logic above?

Just to clarify the advantages that I see are: one less callback, no need to sync pir to irr on each event and, arguably, a little bit simpler logic.

--
Gleb.

Marcelo Tosatti

2013-03-19 15:20 UTC #14 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 70 lines · Gleb Natapov

Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Yang Z
Gleb Natapov wrote on 2013-03-19:
Quoted 29 lines · Yang Z
Quoted 28 lines
Quoted 28 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()
Quoted 8 lines
Quoted 1 line
Current solution is trying to block other Posted Interrupt from other VCPUs at
same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
Quoted 1 line
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:
The IPI will not lost in his example(he misread the patch).
Quoted 16 lines
cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A) set KVM_REQ_EVENT
                                                        process
                                                        REQ_EVENT
                                                        PIR-A->IRR
vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
linux_pir_handler
t_a_s_b(PIR-B)=1 no PIR IPI sent
Quoted 11 lines
But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on() kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask() else kvm_vcpu_kick()
And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()
What are the downsides? Can we lost interrupts this way?
Need to check guest mode before sending IPI. Otherwise hypervisor may receive IPI.

Quoted 8 lines

Quoted 5 lines
Of course, forget it. So if (!r) should be if (!r && mode == IN_GUEST)
Quoted 1 line
I think current logic is ok. Only problem is that when to clear Outstanding Notification bit. Actually I prefer your suggestion to clear it before sync_pir_irr. But Marcelo prefer to clear ON bit after disabling irq.
Marcelo what is the problem with the logic above?
Just to clarify the advantages that I see are: one less callback, no need to sync pir to irr on each event and, arguably, a little bit simpler logic.

See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:

context1 context2 context3

					set_bit(PIR-1)                   
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)

if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))

kvm_apic_update_irr() r = pi_test_and_set_on()

guest entry with PIR ON=1

Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.

Marcelo Tosatti

2013-03-19 15:27 UTC #15 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 10 lines · Marcelo Tosatti

See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:
context1 context2 context3
					set_bit(PIR-1)                   
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))

Quoted 8 lines

   kvm_apic_update_irr()						r = pi_test_and_set_on()
guest entry with PIR ON=1
Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.

Re: KVM_REQ_EVENT setting after set_bit(KVM_REQ_EVENT) assures no guest entry with PIR ON=1.

Might be, would have to verify. Its trickier though. Maybe add a FIXME:
to the callback and remove it later.

Gleb Natapov

2013-03-19 16:10 UTC #16 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 20 lines · Marcelo Tosatti

Quoted 10 lines · Marcelo Tosatti
See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:
context1 context2 context3
					set_bit(PIR-1)                   
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
Quoted 8 lines
   kvm_apic_update_irr()						r = pi_test_and_set_on()
guest entry with PIR ON=1
Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.

Quoted 5 lines

Re: KVM_REQ_EVENT setting after set_bit(KVM_REQ_EVENT) assures no guest entry with PIR ON=1.
Might be, would have to verify. Its trickier though. Maybe add a FIXME:
to the callback and remove it later.

We have time still. RTC series is not ready yet. I'll think hard and try to poke holes in the logic in this patch and you do the same for what I propose.

--
Gleb.

Zhang, Yang Z

author

2013-03-20 11:47 UTC #17 lore raw

RE: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Gleb Natapov wrote on 2013-03-20:

Quoted 21 lines · Marcelo Tosatti

Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines
See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:
context1 context2 context3
					set_bit(PIR-1)
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr() r =
pi_test_and_set_on()
Quoted 8 lines
Quoted 7 lines
guest entry with PIR ON=1
Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.

Quoted 8 lines

Quoted 5 lines
Re: KVM_REQ_EVENT setting after set_bit(KVM_REQ_EVENT) assures no guest entry with PIR ON=1.
Might be, would have to verify. Its trickier though. Maybe add a FIXME:
to the callback and remove it later.
We have time still. RTC series is not ready yet. I'll think hard and try to poke holes in the logic in this patch and you do the same for what I propose.

Any thought? As far as I see, the two solutions are ok. It's hard to say which is better. But clear ON bit when sync_pir_irr should be more clear and close to hardware's behavior.

Best regards,
Yang

Gleb Natapov

2013-03-20 11:49 UTC #18 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 25 lines · Yang Z

Gleb Natapov wrote on 2013-03-20:
Quoted 24 lines · Marcelo Tosatti
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines
See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:
context1 context2 context3
					set_bit(PIR-1)
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr() r =
pi_test_and_set_on()
Quoted 11 lines
Quoted 7 lines
guest entry with PIR ON=1
Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.
Re: KVM_REQ_EVENT setting after set_bit(KVM_REQ_EVENT) assures no guest entry with PIR ON=1.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Might be, would have to verify. Its trickier though. Maybe add a FIXME:
to the callback and remove it later.
We have time still. RTC series is not ready yet. I'll think hard and try to poke holes in the logic in this patch and you do the same for what I propose.
Any thought? As far as I see, the two solutions are ok. It's hard to say which is better. But clear ON bit when sync_pir_irr should be more clear and close to hardware's behavior.

Lets go with it unless we see why it will not work.

--
Gleb.

Zhang, Yang Z

author

2013-03-20 11:53 UTC #19 lore raw

RE: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Gleb Natapov wrote on 2013-03-20:

Quoted 28 lines · Yang Z

Quoted 27 lines
Gleb Natapov wrote on 2013-03-20:
Quoted 26 lines · Marcelo Tosatti
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines
See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:
context1 context2 context3
					set_bit(PIR-1)
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr() r =
pi_test_and_set_on()
Quoted 13 lines
Quoted 7 lines
guest entry with PIR ON=1
Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.
Re: KVM_REQ_EVENT setting after set_bit(KVM_REQ_EVENT) assures no guest entry with PIR ON=1.
Might be, would have to verify. Its trickier though. Maybe add a FIXME:
to the callback and remove it later.

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
We have time still. RTC series is not ready yet. I'll think hard and try to poke holes in the logic in this patch and you do the same for what I propose.
Any thought? As far as I see, the two solutions are ok. It's hard to say which is
better. But clear ON bit when sync_pir_irr should be more clear and close to hardware's behavior.
Quoted 1 line
Lets go with it unless we see why it will not work.

Sure.

Best regards,
Yang

Gleb Natapov

2013-03-19 15:30 UTC #20 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

In reply to Marcelo Tosatti

Quoted 87 lines · Marcelo Tosatti

Quoted 70 lines · Gleb Natapov
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Yang Z
Gleb Natapov wrote on 2013-03-19:
Quoted 29 lines · Yang Z
Quoted 28 lines
Quoted 28 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()
Quoted 8 lines
Quoted 1 line
Current solution is trying to block other Posted Interrupt from other VCPUs at
same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
Quoted 1 line
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:
The IPI will not lost in his example(he misread the patch).
Quoted 16 lines
cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A) set KVM_REQ_EVENT
                                                        process
                                                        REQ_EVENT
                                                        PIR-A->IRR
vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
linux_pir_handler
t_a_s_b(PIR-B)=1 no PIR IPI sent
Quoted 11 lines
But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on() kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask() else kvm_vcpu_kick()
And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()
What are the downsides? Can we lost interrupts this way?
Need to check guest mode before sending IPI. Otherwise hypervisor may receive IPI.
Quoted 8 lines
Quoted 5 lines
Of course, forget it. So if (!r) should be if (!r && mode == IN_GUEST)
Quoted 1 line
I think current logic is ok. Only problem is that when to clear Outstanding Notification bit. Actually I prefer your suggestion to clear it before sync_pir_irr. But Marcelo prefer to clear ON bit after disabling irq.
Marcelo what is the problem with the logic above?
Just to clarify the advantages that I see are: one less callback, no need to sync pir to irr on each event and, arguably, a little bit simpler logic.
See the previous argument: should never enter guest mode with PIR ON bit set. With logic above:
context1 context2 context3
					set_bit(PIR-1)                   
					r = pi_test_and_set_on()	set_bit(PIR-40)
					set_bit(KVM_REQ_EVENT)

Quoted 8 lines

if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
   kvm_apic_update_irr()						r = pi_test_and_set_on()
guest entry with PIR ON=1

But that's OK since context3 will send IPI and PIR will be processed by CPU. Let me amend myself: we should not enter guest mode with ON bit set and no PI IPI pending in LAPIC.

Quoted 3 lines

Thats the reason for unconditional clearing on guest entry: it is easy to verify its correct. I understand and agree the callback (and VMWRITE) is not nice.

--
Gleb.

Marcelo Tosatti

2013-03-19 15:13 UTC #21 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

In reply to Gleb Natapov

Quoted 62 lines · Gleb Natapov

Quoted 28 lines · Yang Z
Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5747
Quoted 3 lines
arch/x86/kvm/x86.c +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
Quoted 10 lines
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()
Quoted 2 lines
Current solution is trying to block other Posted Interrupt from other VCPUs at same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:
cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A)
set KVM_REQ_EVENT
                                                        process REQ_EVENT
                                                        PIR-A->IRR

                                                        vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
                                                        linux_pir_handler

                                        t_a_s_b(PIR-B)=1
                                        no PIR IPI sent
But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on()
kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask()
else
kvm_vcpu_kick()

Quoted 6 lines

And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()
What are the downsides? Can we lost interrupts this way?

You should not ever enter guest mode with ON bit set on PIR (because that will prevent PIR IPI from waking up interrupt injection logic). This is why the ON bit should be cleared on VM entry.

Quoted 4 lines

Quoted 3 lines
Quoted 1 line
May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?
Yes, this will solve it. But I am not sure whether it will introduce any regressions. Is there any check relies on this sequence?
Do not think so.

Can't see what is the problem with the code as it is?

Gleb Natapov

2013-03-19 15:21 UTC #22 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 68 lines · Marcelo Tosatti

Quoted 67 lines · Gleb Natapov
Quoted 28 lines · Yang Z
Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo. It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
Quoted 10 lines
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
                                            vcpu->mode =
IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()
Quoted 2 lines
Current solution is trying to block other Posted Interrupt from other VCPUs at same time. It only mitigates it but cannot solve it. The case you mentioned still exists but it should be rare.
I am not sure I follow. What scenario exactly are you talking about. I looked over past discussion about it and saw that Marcelo gives an example how IPI can be lost, but I think that's because we set "on" bit after KVM_REQ_EVENT:
cpu0 cpu1 vcpu0
test_and_set_bit(PIR-A)
set KVM_REQ_EVENT
                                                        process REQ_EVENT
                                                        PIR-A->IRR

                                                        vcpu->mode=IN_GUEST
if (vcpu0->guest_mode)
        if (!t_a_s_bit(PIR notif))
                send IPI
                                                        linux_pir_handler

                                        t_a_s_b(PIR-B)=1
                                        no PIR IPI sent
But what if on delivery we do:
pi_test_and_set_pir()
r = pi_test_and_set_on()
kvm_make_request(KVM_REQ_EVENT)
if (!r)
send_IPI_mask()
else
kvm_vcpu_kick()
And on vcpu entry we do:
if (kvm_check_request(KVM_REQ_EVENT)
if (test_and_clear_bit(on))
kvm_apic_update_irr()

Quoted 5 lines

Quoted 1 line
What are the downsides? Can we lost interrupts this way?
You should not ever enter guest mode with ON bit set on PIR (because that will prevent PIR IPI from waking up interrupt injection logic). This is why the ON bit should be cleared on VM entry.

I agree that we should not, but I claim that this will not happen since KVM_REQ_EVENT will prevent guest entry with ON bit set.

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?
Yes, this will solve it. But I am not sure whether it will introduce any regressions. Is there any check relies on this sequence?
Do not think so.
Can't see what is the problem with the code as it is?

Just the useless IPI that can be prevented. I agree that this is not a big deal unless shown otherwise.

--
Gleb.

Marcelo Tosatti

2013-03-19 15:13 UTC #23 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

In reply to Gleb Natapov

Quoted 162 lines · Gleb Natapov

Quoted 119 lines · Yang Z
Gleb Natapov wrote on 2013-03-19:
Quoted 19 lines · Yang Zhang
Quoted 18 lines
From: Yang Zhang <[email protected]>
If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.
Signed-off-by Yang Zhang <[email protected]>
---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)
arch/x86/kvm/irq.c +0 -0
index 484bc87..5179988 100644
@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
Quoted 8 lines
arch/x86/kvm/irq.c int kvm_cpu_has_interrupt(struct kvm_vcpu *v) · line 85
Quoted 5 lines
arch/x86/kvm/irq.c +2 -1
 	if (kvm_cpu_has_extint(v))
 		return 1;
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);
That's incorrect. kvm_cpu_has_interrupt() should return true only it there is IRR suitable to be injected, not just any IRR. kvm_apic_has_interrupt() should call kvm_apic_update_irr().
You are right.
Quoted 41 lines
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 7 lines
arch/x86/kvm/irq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
arch/x86/kvm/lapic.c +0 -0
index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 11 lines
arch/x86/kvm/lapic.c +4 -1
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {
@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 733
Quoted 9 lines
arch/x86/kvm/lapic.c +4 -2
 			break;
 		}
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;
apicv code and non apicv code are completely different. What's the point checking for apicv twice here?
Just do:
if (kvm_x86_ops->deliver_posted_interrupt)
	kvm_x86_ops->deliver_posted_interrupt(vcpu, vector)
else {
	result = !apic_test_and_set_irr(vector, apic);
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
}
Quoted 4 lines
And set kvm_x86_ops->deliver_posted_interrupt only if apicv is enabled.
Also rearrange patches so that APIC_TMR handling goes before posted interrupt series.
Sure.
Quoted 42 lines
arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct
Quoted 28 lines
arch/x86/kvm/lapic.c +5 -0
 	case APIC_DM_REMRD: @@ -1604,6 +1609,11 @@ int
 kvm_apic_has_interrupt(struct kvm_vcpu *vcpu) 	return highest_irr; }
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);
arch/x86/kvm/lapic.h +0 -0
index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu); +bool
 kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu); int
 kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu); int
 kvm_get_apic_interrupt(struct kvm_vcpu *vcpu); void
 kvm_lapic_reset(struct kvm_vcpu *vcpu);
arch/x86/kvm/vmx.c +0 -0
index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct
kvm_vcpu *vcpu)
Quoted 13 lines
arch/x86/kvm/vmx.c +11 -0
 	clear_bit(POSTED_INTR_ON, (unsigned long *)&vmx->pi_desc.u.control);
 }
+/*
+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */
Quoted 5 lines
Quoted 3 lines
The comment should go into previous patch. Also I prefer to not check for posted interrupt inside the callback, but set it to NULL instead. This way we avoid calling a callback on a hot path needlessly.
It's make sense. So just follow the logic you mentioned above?
Yes.
Quoted 25 lines
Quoted 18 lines
Quoted 18 lines
arch/x86/kvm/vmx.c +0 -0
 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
arch/x86/kvm/x86.c +0 -0
index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu, 				   
 struct kvm_lapic_state *s) { +	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0; @@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct
 kvm_vcpu *vcpu) 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
 +		kvm_x86_ops->sync_pir_to_irr(vcpu); 		inject_pending_event(vcpu);
Quoted 7 lines
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5746
Quoted 6 lines
arch/x86/kvm/x86.c +2 -0
 		/* enable NMI/IRQ window open exits if needed */
@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo.
It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();

Quoted 7 lines

But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir()
kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()

						^^^^^^^^^^^^^^^^^^

Quoted 10 lines

                                            vcpu->mode = IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()

May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?

Scenario is correct because injected PIR has been synced to IRR before guest entry.

Gleb Natapov

2013-03-19 15:18 UTC #24 lore raw

Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

Quoted 180 lines · Marcelo Tosatti

Quoted 162 lines · Gleb Natapov
Quoted 119 lines · Yang Z
Gleb Natapov wrote on 2013-03-19:
Quoted 19 lines · Yang Zhang
Quoted 18 lines
From: Yang Zhang <[email protected]>
If posted interrupt is avaliable, then uses it to inject virtual interrupt to guest.
Signed-off-by Yang Zhang <[email protected]>
---
 arch/x86/kvm/irq.c   |    3 ++-
 arch/x86/kvm/lapic.c |   16 +++++++++++++---
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/vmx.c   |   11 +++++++++++
 arch/x86/kvm/x86.c   |    4 ++++
 5 files changed, 31 insertions(+), 4 deletions(-)
arch/x86/kvm/irq.c +0 -0
index 484bc87..5179988 100644
@@ -81,7 +81,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
Quoted 8 lines
arch/x86/kvm/irq.c int kvm_cpu_has_interrupt(struct kvm_vcpu *v) · line 85
Quoted 5 lines
arch/x86/kvm/irq.c +2 -1
 	if (kvm_cpu_has_extint(v))
 		return 1;
-	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
+	return (kvm_apic_has_interrupt(v) != -1) ||
+		kvm_hwapic_has_interrupt(v);
That's incorrect. kvm_cpu_has_interrupt() should return true only it there is IRR suitable to be injected, not just any IRR. kvm_apic_has_interrupt() should call kvm_apic_update_irr().
You are right.
Quoted 41 lines
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 7 lines
arch/x86/kvm/irq.c +0 -0
 }
 EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
arch/x86/kvm/lapic.c +0 -0
index b3ea50e..46c7310 100644
@@ -713,7 +713,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int
Quoted 11 lines
arch/x86/kvm/lapic.c +4 -1
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
-		result = !apic_test_and_set_irr(vector, apic);
+		result = 1;
+		if (!kvm_x86_ops->deliver_posted_interrupt(vcpu, vector))
+			result = !apic_test_and_set_irr(vector, apic);
+
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
 		if (!result) {
@@ -723,8 +726,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int
delivery_mode,
arch/x86/kvm/lapic.c static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 733
Quoted 9 lines
arch/x86/kvm/lapic.c +4 -2
 			break;
 		}
-		kvm_make_request(KVM_REQ_EVENT, vcpu);
-		kvm_vcpu_kick(vcpu);
+		if (!kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
+			kvm_make_request(KVM_REQ_EVENT, vcpu);
+			kvm_vcpu_kick(vcpu);
+		}
 		break;
apicv code and non apicv code are completely different. What's the point checking for apicv twice here?
Just do:
if (kvm_x86_ops->deliver_posted_interrupt)
	kvm_x86_ops->deliver_posted_interrupt(vcpu, vector)
else {
	result = !apic_test_and_set_irr(vector, apic);
	kvm_make_request(KVM_REQ_EVENT, vcpu);
	kvm_vcpu_kick(vcpu);
}
Quoted 4 lines
And set kvm_x86_ops->deliver_posted_interrupt only if apicv is enabled.
Also rearrange patches so that APIC_TMR handling goes before posted interrupt series.
Sure.
Quoted 42 lines
arch/x86/kvm/vmx.c static void vmx_posted_intr_clear_on(struct
Quoted 28 lines
arch/x86/kvm/lapic.c +5 -0
 	case APIC_DM_REMRD: @@ -1604,6 +1609,11 @@ int
 kvm_apic_has_interrupt(struct kvm_vcpu *vcpu) 	return highest_irr; }
+bool kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu)
+{
+	return kvm_x86_ops->hwapic_has_interrupt(vcpu);
+}
+
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
 	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);
arch/x86/kvm/lapic.h +0 -0
index e5327be..c6abc63 100644
@@ -37,6 +37,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu); +bool
 kvm_hwapic_has_interrupt(struct kvm_vcpu *vcpu); int
 kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu); int
 kvm_get_apic_interrupt(struct kvm_vcpu *vcpu); void
 kvm_lapic_reset(struct kvm_vcpu *vcpu);
arch/x86/kvm/vmx.c +0 -0
index 0b5a8ae..48a2239 100644
@@ -3932,6 +3932,17 @@ static void vmx_posted_intr_clear_on(struct
kvm_vcpu *vcpu)
Quoted 13 lines
arch/x86/kvm/vmx.c +11 -0
 	clear_bit(POSTED_INTR_ON, (unsigned long *)&vmx->pi_desc.u.control);
 }
+/*
+ * Send interrupt to vcpu via posted interrupt way.
+ * Return false if posted interrupt is not supported and the caller will
+ * roll back to old way(via set vIRR).
+ * Return true if posted interrupt is avalialbe, the interrupt is set
+ * in pir(posted interrupt requests):
+ * 1. If target vcpu is running(non-root mode), send posted interrupt
+ * notification to vcpu and hardware will sync pir to vIRR atomically.
+ * 2. If target vcpu isn't running(root mode), kick it to pick up the
+ * interrupt from pir in next vmentry.
+ */
Quoted 5 lines
Quoted 3 lines
The comment should go into previous patch. Also I prefer to not check for posted interrupt inside the callback, but set it to NULL instead. This way we avoid calling a callback on a hot path needlessly.
It's make sense. So just follow the logic you mentioned above?
Yes.
Quoted 25 lines
Quoted 18 lines
Quoted 18 lines
arch/x86/kvm/vmx.c +0 -0
 static bool vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
arch/x86/kvm/x86.c +0 -0
index 0baa90d..0981100 100644
@@ -2679,6 +2679,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu, 				   
 struct kvm_lapic_state *s) { +	kvm_x86_ops->sync_pir_to_irr(vcpu);
 	memcpy(s->regs, vcpu->arch.apic->regs, sizeof *s);
 
 	return 0; @@ -5699,6 +5700,7 @@ static int vcpu_enter_guest(struct
 kvm_vcpu *vcpu) 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
 +		kvm_x86_ops->sync_pir_to_irr(vcpu); 		inject_pending_event(vcpu);
Quoted 7 lines
arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5746
Quoted 6 lines
arch/x86/kvm/x86.c +2 -0
 		/* enable NMI/IRQ window open exits if needed */
@@ -5741,6 +5743,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
+	kvm_x86_ops->posted_intr_clear_on(vcpu);
+
Why is this separate from pir_to_irr syncing?
Quoted 9 lines
This is the result of discussion with Marcelo.
It is more reasonable to put it here to avoid unnecessary posted interrupt between:
vcpu->mode = IN_GUEST_MODE;
<--interrupt may arrived here and this is unnecessary.
local_irq_disable();
Quoted 7 lines
But this still can happen as far as I see:
vcpu0 vcpu1:
pi_test_and_set_pir()
kvm_make_request(KVM_REQ_EVENT)
                                            if (KVM_REQ_EVENT)
                                                   sync_pir_to_irr()
						^^^^^^^^^^^^^^^^^^
Quoted 9 lines
                                            vcpu->mode = IN_GUEST_MODE;
if (vcpu->mode == IN_GUEST_MODE)
  if (!pi_test_and_set_on())
    apic->send_IPI_mask()
                                            --> IPI arrives here
                                            local_irq_disable();
                                            posted_intr_clear_on()

Quoted 4 lines

Quoted 1 line
May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?
Scenario is correct because injected PIR has been synced to IRR before guest entry.

I do not say it is not. Yang said that the code tries to prevent this from happening, I showed that this still can happen. There is not harm, just useless IPI.

--
Gleb.

Zhang, Yang Z

author

2013-03-18 02:49 UTC #25 lore raw

In reply to Yang Zhang

Zhang, Yang Z wrote on 2013-03-15:

Quoted 55 lines

From: Yang Zhang <[email protected]>
The follwoing patches are adding the Posted Interrupt supporting to KVM:
The first patch enables the feature 'acknowledge interrupt on vmexit'.Since it is required by Posted interrupt, we need to enable it firstly.
And the subsequent patches are adding the posted interrupt supporting:
Posted Interrupt allows APIC interrupts to inject into guest directly without any vmexit.
- When delivering a interrupt to guest, if target vcpu is running, update Posted-interrupt requests bitmap and send a notification event to the vcpu. Then the vcpu will handle this interrupt automatically, without any software involvemnt.
- If target vcpu is not running or there already a notification event pending in the vcpu, do nothing. The interrupt will be handled by next vm entry
NOTE: We don't turn on the Posted Interrupt until the coalesced issue is solved.
Changes from v5 to v6:
* Split sync_pir_to_irr into two functions one to query whether PIR is empty and the other to perform the sync.
* Add comments to explain how vmx_sync_pir_to_irr() work.
* Rebase on top of KVM upstream.
Changes from v4 to v5:
* Add per cpu count for posted IPI handler.
* Dont' check irr when delivering interrupt. Since we can not get interrupt coalesced info with Posted Interrupt. So there is no need to check the irr. There is another patch will changed current interrupt coalesced logic. Before it, we will not turn on Posted Interrupt. * Clear outstanding notification bit after call local_irq_disable, but before check request. As Marcelo suggested, we can ensure the IPI not lost in this way * Remove the spinlock. Same as item 2, if not need to get coalesced info, then no need the lock.
* Rebase on top of KVM upstream.
Yang Zhang (5):
KVM: VMX: Enable acknowledge interupt on vmexit
KVM: VMX: Register a new IPI for posted interrupt
KVM: VMX: Check the posted interrupt capability
KVM: VMX: Add the algorithm of deliver posted interrupt KVM : VMX: Use posted interrupt to deliver virtual interrupt
 arch/x86/include/asm/entry_arch.h  |    4 +
 arch/x86/include/asm/hardirq.h     |    3 +
 arch/x86/include/asm/hw_irq.h      |    1 +
 arch/x86/include/asm/irq_vectors.h |    5 +
 arch/x86/include/asm/kvm_host.h    |    5 + arch/x86/include/asm/vmx.h 
        |    4 + arch/x86/kernel/entry_64.S         |    5 +
 arch/x86/kernel/irq.c              |   22 ++++
 arch/x86/kernel/irqinit.c          |    4 + arch/x86/kvm/irq.c         
        |    3 +- arch/x86/kvm/lapic.c               |   29 ++++-
 arch/x86/kvm/lapic.h               |    2 + arch/x86/kvm/svm.c         
        |   30 +++++ arch/x86/kvm/vmx.c                 |  223

Quoted 3 lines

++++++++++++++++++++++++++++++++---- arch/x86/kvm/x86.c
| 8 +- virt/kvm/kvm_main.c | 1 + 16 files changed,
320 insertions(+), 29 deletions(-)

Are there any other comments with this patch?

For TMR issue, since it has nothing to do with APICv, if we really need to handle it later, then we may need a separate patch to fix it. But currently, we may focused on APICv only.

Best regards,
Yang

Gleb Natapov

2013-03-18 09:16 UTC #26 lore raw

Quoted 59 lines · Yang Z

Zhang, Yang Z wrote on 2013-03-15:
Quoted 55 lines
From: Yang Zhang <[email protected]>
The follwoing patches are adding the Posted Interrupt supporting to KVM:
The first patch enables the feature 'acknowledge interrupt on vmexit'.Since it is required by Posted interrupt, we need to enable it firstly.
And the subsequent patches are adding the posted interrupt supporting:
Posted Interrupt allows APIC interrupts to inject into guest directly without any vmexit.
- When delivering a interrupt to guest, if target vcpu is running, update Posted-interrupt requests bitmap and send a notification event to the vcpu. Then the vcpu will handle this interrupt automatically, without any software involvemnt.
- If target vcpu is not running or there already a notification event pending in the vcpu, do nothing. The interrupt will be handled by next vm entry
NOTE: We don't turn on the Posted Interrupt until the coalesced issue is solved.
Changes from v5 to v6:
* Split sync_pir_to_irr into two functions one to query whether PIR is empty and the other to perform the sync.
* Add comments to explain how vmx_sync_pir_to_irr() work.
* Rebase on top of KVM upstream.
Changes from v4 to v5:
* Add per cpu count for posted IPI handler.
* Dont' check irr when delivering interrupt. Since we can not get interrupt coalesced info with Posted Interrupt. So there is no need to check the irr. There is another patch will changed current interrupt coalesced logic. Before it, we will not turn on Posted Interrupt. * Clear outstanding notification bit after call local_irq_disable, but before check request. As Marcelo suggested, we can ensure the IPI not lost in this way * Remove the spinlock. Same as item 2, if not need to get coalesced info, then no need the lock.
* Rebase on top of KVM upstream.
Yang Zhang (5):
KVM: VMX: Enable acknowledge interupt on vmexit
KVM: VMX: Register a new IPI for posted interrupt
KVM: VMX: Check the posted interrupt capability
KVM: VMX: Add the algorithm of deliver posted interrupt KVM : VMX: Use posted interrupt to deliver virtual interrupt
 arch/x86/include/asm/entry_arch.h  |    4 +
 arch/x86/include/asm/hardirq.h     |    3 +
 arch/x86/include/asm/hw_irq.h      |    1 +
 arch/x86/include/asm/irq_vectors.h |    5 +
 arch/x86/include/asm/kvm_host.h    |    5 + arch/x86/include/asm/vmx.h 
        |    4 + arch/x86/kernel/entry_64.S         |    5 +
 arch/x86/kernel/irq.c              |   22 ++++
 arch/x86/kernel/irqinit.c          |    4 + arch/x86/kvm/irq.c         
        |    3 +- arch/x86/kvm/lapic.c               |   29 ++++-
 arch/x86/kvm/lapic.h               |    2 + arch/x86/kvm/svm.c         
        |   30 +++++ arch/x86/kvm/vmx.c                 |  223
Quoted 3 lines
++++++++++++++++++++++++++++++++---- arch/x86/kvm/x86.c
| 8 +- virt/kvm/kvm_main.c | 1 + 16 files changed,
320 insertions(+), 29 deletions(-)

Quoted 2 lines

Are there any other comments with this patch?

Haven't reviewed it yet, will do ASAP. "Use eoi to track RTC interrupt delivery status" series is pre-request for this one.

Quoted 2 lines

For TMR issue, since it has nothing to do with APICv, if we really need to handle it later, then we may need a separate patch to fix it. But currently, we may focused on APICv only.

What do you mean by "TMR has nothing to do with APICv"?

--
Gleb.

Zhang, Yang Z

author

2013-03-18 10:43 UTC #27 lore raw

Gleb Natapov wrote on 2013-03-18:

Quoted 62 lines · Yang Z

Quoted 49 lines
Zhang, Yang Z wrote on 2013-03-15:
Quoted 48 lines
From: Yang Zhang <[email protected]>
The follwoing patches are adding the Posted Interrupt supporting to KVM:
The first patch enables the feature 'acknowledge interrupt on vmexit'.Since it is required by Posted interrupt, we need to enable it firstly.
And the subsequent patches are adding the posted interrupt supporting:
Posted Interrupt allows APIC interrupts to inject into guest directly without any vmexit.
- When delivering a interrupt to guest, if target vcpu is running, update Posted-interrupt requests bitmap and send a notification event to the vcpu. Then the vcpu will handle this interrupt automatically, without any software involvemnt. - If target vcpu is not running or there already a notification event pending in the vcpu, do nothing. The interrupt will be handled by next vm entry
NOTE: We don't turn on the Posted Interrupt until the coalesced issue is solved.
Changes from v5 to v6:
* Split sync_pir_to_irr into two functions one to query whether PIR is empty and the other to perform the sync.
* Add comments to explain how vmx_sync_pir_to_irr() work.
* Rebase on top of KVM upstream.
Changes from v4 to v5:
* Add per cpu count for posted IPI handler.
* Dont' check irr when delivering interrupt. Since we can not get interrupt coalesced info with Posted Interrupt. So there is no need to check the irr. There is another patch will changed current interrupt coalesced logic. Before it, we will not turn on Posted Interrupt. * Clear outstanding notification bit after call local_irq_disable, but before check request. As Marcelo suggested, we can ensure the IPI not lost in this way * Remove the spinlock. Same as item 2, if not need to get coalesced info, then no need the lock.
* Rebase on top of KVM upstream.
Yang Zhang (5):
KVM: VMX: Enable acknowledge interupt on vmexit
KVM: VMX: Register a new IPI for posted interrupt
KVM: VMX: Check the posted interrupt capability
KVM: VMX: Add the algorithm of deliver posted interrupt KVM : VMX: Use posted interrupt to deliver virtual interrupt
 arch/x86/include/asm/entry_arch.h  |    4 +
 arch/x86/include/asm/hardirq.h     |    3 +
 arch/x86/include/asm/hw_irq.h      |    1 +
 arch/x86/include/asm/irq_vectors.h |    5 +
 arch/x86/include/asm/kvm_host.h    |    5 +
arch/x86/include/asm/vmx.h
Quoted 11 lines
Quoted 9 lines
        |    4 + arch/x86/kernel/entry_64.S         |    5 +
 arch/x86/kernel/irq.c              |   22 ++++
 arch/x86/kernel/irqinit.c          |    4 + arch/x86/kvm/irq.c
        |    3 +- arch/x86/kvm/lapic.c               |   29 ++++-
arch/x86/kvm/lapic.h | 2 + arch/x86/kvm/svm.c
| 30 +++++ arch/x86/kvm/vmx.c | 223
++++++++++++++++++++++++++++++++---- arch/x86/kvm/x86.c
| 8 +- virt/kvm/kvm_main.c | 1 + 16 files changed,
320 insertions(+), 29 deletions(-)
Are there any other comments with this patch?

Quoted 8 lines

Haven't reviewed it yet, will do ASAP. "Use eoi to track RTC interrupt delivery status" series is pre-request for this one.
Quoted 1 line
For TMR issue, since it has nothing to do with APICv, if we really need to handle
it later, then we may need a separate patch to fix it. But currently, we may focused on APICv only.
Quoted 1 line
What do you mean by "TMR has nothing to do with APICv"?

Just ignore it. I will send out the fixing with APICv patch.

Best regards,
Yang

Gleb Natapov

2013-03-18 11:28 UTC #28 lore raw

Quoted 7 lines · Yang Z

Quoted 5 lines
Quoted 1 line
For TMR issue, since it has nothing to do with APICv, if we really need to handle
it later, then we may need a separate patch to fix it. But currently, we may focused on APICv only.
Quoted 1 line
What do you mean by "TMR has nothing to do with APICv"?
Just ignore it. I will send out the fixing with APICv patch.

Does this mean we cannot get away with updating TMR like we do now? Because if we can I prefer to not complicate the code further.

--
Gleb.

Zhang, Yang Z

author

2013-03-18 11:44 UTC #29 lore raw

Gleb Natapov wrote on 2013-03-18:

Quoted 11 lines · Yang Z

Quoted 1 line
Quoted 1 line
Quoted 1 line
For TMR issue, since it has nothing to do with APICv, if we really need to
handle
Quoted 6 lines
Quoted 4 lines
it later, then we may need a separate patch to fix it. But currently, we may focused on APICv only.
Quoted 1 line
What do you mean by "TMR has nothing to do with APICv"?
Just ignore it. I will send out the fixing with APICv patch.
Does this mean we cannot get away with updating TMR like we do now? Because if we can I prefer to not complicate the code further.

No, we still need to update TMR but only in vcpu context.

Best regards,
Yang

Marcelo Tosatti

2013-03-18 22:56 UTC #30 lore raw

In reply to Yang Zhang

Quoted 15 lines · Yang Zhang

From: Yang Zhang <[email protected]>
The follwoing patches are adding the Posted Interrupt supporting to KVM:
The first patch enables the feature 'acknowledge interrupt on vmexit'.Since it is required by Posted interrupt, we need to enable it firstly.
And the subsequent patches are adding the posted interrupt supporting:
Posted Interrupt allows APIC interrupts to inject into guest directly without any vmexit.
- When delivering a interrupt to guest, if target vcpu is running, update Posted-interrupt requests bitmap and send a notification event to the vcpu. Then the vcpu will handle this interrupt automatically, without any software involvemnt.

Quoted 6 lines

- If target vcpu is not running or there already a notification event pending in the vcpu, do nothing. The interrupt will be handled by next vm entry
NOTE: We don't turn on the Posted Interrupt until the coalesced issue is solved.

Looks good, but RTC issue should be resolved.