RFC: kvm eoi PV using shared memory

I took a stub at implementing PV EOI using shared memory. This should reduce the number of exits an interrupt causes as much as by half.

A partially complete draft for both host and guest parts is below.

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
We set it before injecting an interrupt and clear before injecting a nested one. Guest tests it using a test and clear operation - this is necessary
so that host can detect interrupt nesting -
and if set, it can skip the EOI MSR.

There's a new MSR to set the address of said register in guest memory. Otherwise not much changes:
- Guest EOI is not required
- ISR is automatically cleared before injection

Some things are incomplete: add feature negotiation options, qemu support for said options.
No testing was done beyond compiling the kernel.

I would appreciate early feedback.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

arch/x86/include/asm/apic.h +0 -0

index d854101..8430f41 100644

arch/x86/include/asm/apic.h +0 -0

 	/*
 	 * ack_APIC_irq() actually gets compiled as a single instruction
 	 * ... yummie.

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

index e216ba0..0ee1472 100644

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

 	if (has_steal_clock)
 		kvm_register_steal_time();
 }

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

index 8584322..9e38e12 100644
@@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 			irq->level, irq->trig_mode);
 }
 
-static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
+{
+
+	return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
+				      sizeof(val));
+}
+
+static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
+{
+
+	return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
+				      sizeof(*val));
+}
+
+static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
+{
+	return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
+}
+
+static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
+{
+	u32 val;
+	if (eoi_get_user(vcpu, &val) < 0)
+		apic_debug("Can't read EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+	if (!(val & 0x1))
+		vcpu->arch.eoi.vector = -1;
+	return vcpu->arch.eoi.vector;
+}
+
+static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
+{
+	BUG_ON(vcpu->arch.eoi.vector != -1);
+	if (eoi_put_user(vcpu, 0x1) < 0) {
+		apic_debug("Can't set EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.vector = vector;
+}
+
+static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
+{
+	int vector;
+	vector = vcpu->arch.eoi.vector;
+	if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
+		apic_debug("Can't clear EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return -1;
+	}
+	vcpu->arch.eoi.vector = -1;
+	return vector;
+}
 
 
 
+
+static inline int __apic_find_highest_isr(struct kvm_lapic *apic)

{

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

 	int result;

@@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 

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

+
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;

@@ -488,6 +553,8 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 	if (vector == -1)
 		return;
 
+	if (eoi_enabled(apic->vcpu))
+		eoi_clr_pending_vector(apic->vcpu);
 	apic_clear_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 

@@ -1236,11 +1303,25 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 {
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;

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

+		}
+	}
+
+	if (set_isr)
+		apic_set_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);
 	return vector;

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

Quoted 35 lines · Michael S. Tsirkin

I took a stub at implementing PV EOI using shared memory. This should reduce the number of exits an interrupt causes as much as by half.

A partially complete draft for both host and guest parts is below.

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
We set it before injecting an interrupt and clear before injecting a nested one. Guest tests it using a test and clear operation - this is necessary
so that host can detect interrupt nesting -
and if set, it can skip the EOI MSR.

There's a new MSR to set the address of said register in guest memory. Otherwise not much changes:
- Guest EOI is not required
- ISR is automatically cleared before injection

Some things are incomplete: add feature negotiation options, qemu support for said options.
No testing was done beyond compiling the kernel.

I would appreciate early feedback.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

arch/x86/include/asm/apic.h +0 -0

index d854101..8430f41 100644

arch/x86/include/asm/apic.h static inline u32 safe_apic_wait_icr_idle(void) { return 0; } · line 466

Quoted 10 lines

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

@@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) { return 0; }
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
+
 static inline void ack_APIC_irq(void)
 {
+	if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
+		return;
+

While __test_and_clear_bit() is implemented in a single instruction, it's not required to be. Better have the instruction there explicitly.

Quoted 12 lines

arch/x86/include/asm/apic.h +0 -0

 	/*
 	 * ack_APIC_irq() actually gets compiled as a single instruction
 	 * ... yummie.

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

index e216ba0..0ee1472 100644
@@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
 		u64 length;
 		u64 status;
 	} osvw;
+

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 490

Quoted 6 lines

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

+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		int vector;
+	} eoi;
 };

Needs to be cleared on INIT.

arch/x86/include/asm/kvm_host.h void __cpuinit kvm_guest_cpu_init(void) · line 313

Quoted 9 lines

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

@@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
+	       MSR_KVM_EOI_ENABLED);
+

Clear on kexec.

Quoted 77 lines

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

 	if (has_steal_clock)
 		kvm_register_steal_time();
 }

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

index 8584322..9e38e12 100644
@@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 			irq->level, irq->trig_mode);
 }
 
-static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
+{
+
+	return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
+				      sizeof(val));
+}
+
+static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
+{
+
+	return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
+				      sizeof(*val));
+}
+
+static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
+{
+	return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
+}
+
+static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
+{
+	u32 val;
+	if (eoi_get_user(vcpu, &val) < 0)
+		apic_debug("Can't read EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+	if (!(val & 0x1))
+		vcpu->arch.eoi.vector = -1;
+	return vcpu->arch.eoi.vector;
+}
+
+static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
+{
+	BUG_ON(vcpu->arch.eoi.vector != -1);
+	if (eoi_put_user(vcpu, 0x1) < 0) {
+		apic_debug("Can't set EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.vector = vector;
+}
+
+static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
+{
+	int vector;
+	vector = vcpu->arch.eoi.vector;
+	if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
+		apic_debug("Can't clear EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return -1;
+	}
+	vcpu->arch.eoi.vector = -1;
+	return vector;
+}
 
 
 
+
+static inline int __apic_find_highest_isr(struct kvm_lapic *apic)

{

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

 	int result;

@@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 341

Quoted 10 lines

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

+static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (eoi_enabled(apic->vcpu)) {
+		vector = eoi_get_pending_vector(apic->vcpu);
+		if (vector != -1)
+			return vector;
+	}
+	return __apic_find_highest_isr(apic);
+}

Why aren't you modifying the ISR unconfitionally?

Quoted 22 lines

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

+
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;

@@ -488,6 +553,8 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 	if (vector == -1)
 		return;
 
+	if (eoi_enabled(apic->vcpu))
+		eoi_clr_pending_vector(apic->vcpu);
 	apic_clear_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 

@@ -1236,11 +1303,25 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 {
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
+	bool set_isr = true;
 
 	if (vector == -1)
 		return -1;
 

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1314

Quoted 5 lines

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

-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	if (eoi_enabled(vcpu)) {
+		/* Anything pending? If yes disable eoi optimization. */
+		if (unlikely(apic_find_highest_isr(apic) >= 0)) {
+			int v = eoi_clr_pending_vector(vcpu);

ISR != pending, that's IRR. If ISR vector has lower priority than the new vector, then we don't need to disable eoi avoidance.

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Quoted 10 lines

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

+		}
+	}
+
+	if (set_isr)
+		apic_set_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);
 	return vector;

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

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 11 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 490

Quoted 9 lines

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

 		u64 status;
 	} osvw;
+
+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		int vector;
+	} eoi;
 };

Needs to be cleared on INIT.

You mean kvm_arch_vcpu_reset?

Quoted 6 lines

Quoted 6 lines

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

@@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 

Quoted 5 lines

arch/x86/kernel/kvm.c void __cpuinit kvm_guest_cpu_init(void) · line 313

Quoted 3 lines

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

+	wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
+	       MSR_KVM_EOI_ENABLED);
+

Clear on kexec.

With register_reboot_notifier?

Quoted 9 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1314

Quoted 6 lines

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

-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	if (eoi_enabled(vcpu)) {
+		/* Anything pending? If yes disable eoi optimization. */
+		if (unlikely(apic_find_highest_isr(apic) >= 0)) {
+			int v = eoi_clr_pending_vector(vcpu);

ISR != pending, that's IRR. If ISR vector has lower priority than the new vector, then we don't need to disable eoi avoidance.

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?

I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Marcelo said linux does not normally read ISR - not true?

Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Disabling would be very sad. Can we sample on remote irr read?

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 490

Quoted 9 lines

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

 		u64 status;
 	} osvw;
+
+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		int vector;
+	} eoi;
 };

Needs to be cleared on INIT.

Quoted 1 line

You mean kvm_arch_vcpu_reset?

Yes, or kvm_lapic_reset().

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

@@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 

Quoted 7 lines

Quoted 5 lines

arch/x86/kernel/kvm.c void __cpuinit kvm_guest_cpu_init(void) · line 313

Quoted 3 lines

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

+	wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
+	       MSR_KVM_EOI_ENABLED);
+

Clear on kexec.

With register_reboot_notifier?

Yes, we already clear some kvm msrs there.

Quoted 10 lines

Quoted 9 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1314

Quoted 6 lines

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

-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	if (eoi_enabled(vcpu)) {
+		/* Anything pending? If yes disable eoi optimization. */
+		if (unlikely(apic_find_highest_isr(apic) >= 0)) {
+			int v = eoi_clr_pending_vector(vcpu);

ISR != pending, that's IRR. If ISR vector has lower priority than the new vector, then we don't need to disable eoi avoidance.

Quoted 2 lines

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?

It's pretty easy, if there is something in IRR but kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.

Quoted 11 lines

I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Quoted 1 line

Marcelo said linux does not normally read ISR - not true?

It's true and it's irrelevant. We aren't coding a feature to what linux does now, but for what linux or another guest may do in the future.

Quoted 8 lines

Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Disabling would be very sad. Can we sample on remote irr read?

That can be done from another vcpu.

Why do we care about
level-triggered interrupts? Everything uses MSI or edge-triggered IOAPIC interrupts these days.

Quoted 6 lines

Quoted 3 lines

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?

It's pretty easy, if there is something in IRR but kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.

I only see kvm_apic_has_interrupt - is this what you mean?

Quoted 11 lines

Quoted 11 lines

I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Quoted 4 lines

Quoted 1 line

Marcelo said linux does not normally read ISR - not true?

It's true and it's irrelevant. We aren't coding a feature to what linux does now, but for what linux or another guest may do in the future.

Right. So you think reading ISR has value in combination with PV EOI for future guests? I'm not arguing either way just curious.

Quoted 8 lines

Quoted 8 lines

Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Quoted 3 lines

Quoted 1 line

Disabling would be very sad. Can we sample on remote irr read?

That can be done from another vcpu.

We still can handle it, right? Where's the code that handles that read?

Quoted 4 lines

Why do we care about
level-triggered interrupts? Everything uses MSI or edge-triggered IOAPIC interrupts these days.

Well lots of emulated devices don't yet. They probably should but it's nice to be able to test with e.g. e1000 emulation not just virtio.

Besides, kvm_get_apic_interrupt
simply doesn't know about the triggering mode at the moment.

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 3 lines

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?

It's pretty easy, if there is something in IRR but kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.

I only see kvm_apic_has_interrupt - is this what you mean?

Yes, sorry.

It's not clear whether to do the check in kvm_apic_has_interrupt() or kvm_apic_get_interrupt() - the latter is called only after interrupts are enabled, so it looks like a better place (EOIs while interrupts are disabled have no effect). But need to make sure those functions are actually called, since they're protected by KVM_REQ_EVENT.

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Quoted 1 line

Marcelo said linux does not normally read ISR - not true?

It's true and it's irrelevant. We aren't coding a feature to what linux does now, but for what linux or another guest may do in the future.

Quoted 3 lines

Right. So you think reading ISR has value in combination with PV EOI for future guests? I'm not arguing either way just curious.

I don't. But we need to preserve the same interface the APIC has presented for thousands of years (well, almost).

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Disabling would be very sad. Can we sample on remote irr read?

That can be done from another vcpu.

We still can handle it, right? Where's the code that handles that read?

Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c

Quoted 8 lines

Quoted 4 lines

Why do we care about
level-triggered interrupts? Everything uses MSI or edge-triggered IOAPIC interrupts these days.

Well lots of emulated devices don't yet. They probably should but it's nice to be able to test with e.g. e1000 emulation not just virtio.

e1000 doesn't support msi?

Quoted 4 lines

Besides, kvm_get_apic_interrupt
simply doesn't know about the triggering mode at the moment.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 12 lines · Avi Kivity

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 3 lines

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?

It's pretty easy, if there is something in IRR but kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.

I only see kvm_apic_has_interrupt - is this what you mean?

Yes, sorry.

Quoted 5 lines

It's not clear whether to do the check in kvm_apic_has_interrupt() or kvm_apic_get_interrupt() - the latter is called only after interrupts are enabled, so it looks like a better place (EOIs while interrupts are disabled have no effect). But need to make sure those functions are actually called, since they're protected by KVM_REQ_EVENT.

Sorry not sure what you mean by "make sure" - read the code carefully?

Quoted 2 lines

Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c

Hmm. Disabling for level handles the ack notifiers issue as well, which I forgot about.
It's a tough call. You think looking at TMR in kvm_get_apic_interrupt is safe?

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Why do we care about
level-triggered interrupts? Everything uses MSI or edge-triggered IOAPIC interrupts these days.

Quoted 6 lines

Quoted 3 lines

Well lots of emulated devices don't yet.
They probably should but it's nice to be able to
test with e.g. e1000 emulation not just virtio.

e1000 doesn't support msi?

qemu emulation doesn't.

Quoted 19 lines · Michael S. Tsirkin

Quoted 17 lines · Avi Kivity

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 3 lines

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?

It's pretty easy, if there is something in IRR but kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.

I only see kvm_apic_has_interrupt - is this what you mean?

Yes, sorry.

It's not clear whether to do the check in kvm_apic_has_interrupt() or kvm_apic_get_interrupt() - the latter is called only after interrupts are enabled, so it looks like a better place (EOIs while interrupts are disabled have no effect). But need to make sure those functions are actually called, since they're protected by KVM_REQ_EVENT.

Quoted 1 line

Sorry not sure what you mean by "make sure" - read the code carefully?

Yes. And I mean, get called at the right time.

Quoted 7 lines

Quoted 2 lines

Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c

Hmm. Disabling for level handles the ack notifiers issue as well, which I forgot about.
It's a tough call. You think looking at TMR in kvm_get_apic_interrupt is safe?

Yes, it's read only from the guest point of view IIRC.

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

Quoted 4 lines

Why do we care about
level-triggered interrupts? Everything uses MSI or edge-triggered IOAPIC interrupts these days.

Well lots of emulated devices don't yet.
They probably should but it's nice to be able to
test with e.g. e1000 emulation not just virtio.

Quoted 4 lines

Quoted 1 line

e1000 doesn't support msi?

qemu emulation doesn't.

Can be changed if someone's really interested. But really, avoiding EOIs for e1000 won't help it much.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 62 lines · Avi Kivity

Quoted 35 lines · Michael S. Tsirkin

I took a stub at implementing PV EOI using shared memory. This should reduce the number of exits an interrupt causes as much as by half.

A partially complete draft for both host and guest parts is below.

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
We set it before injecting an interrupt and clear before injecting a nested one. Guest tests it using a test and clear operation - this is necessary
so that host can detect interrupt nesting -
and if set, it can skip the EOI MSR.

There's a new MSR to set the address of said register in guest memory. Otherwise not much changes:
- Guest EOI is not required
- ISR is automatically cleared before injection

Some things are incomplete: add feature negotiation options, qemu support for said options.
No testing was done beyond compiling the kernel.

I would appreciate early feedback.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

arch/x86/include/asm/apic.h +0 -0

index d854101..8430f41 100644

arch/x86/include/asm/apic.h static inline u32 safe_apic_wait_icr_idle(void) { return 0; } · line 466

Quoted 10 lines

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

@@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) { return 0; }
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
+
 static inline void ack_APIC_irq(void)
 {
+	if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
+		return;
+

While __test_and_clear_bit() is implemented in a single instruction, it's not required to be. Better have the instruction there explicitly.

Quoted 12 lines

arch/x86/include/asm/apic.h +0 -0

 	/*
 	 * ack_APIC_irq() actually gets compiled as a single instruction
 	 * ... yummie.

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

index e216ba0..0ee1472 100644
@@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
 		u64 length;
 		u64 status;
 	} osvw;
+

Quoted 8 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 490

Quoted 6 lines

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

+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		int vector;
+	} eoi;
 };

Needs to be cleared on INIT.

You mean kvm_arch_vcpu_reset?

Quoted 11 lines

arch/x86/include/asm/kvm_host.h void __cpuinit kvm_guest_cpu_init(void) · line 313

Quoted 9 lines

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

@@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
+	       MSR_KVM_EOI_ENABLED);
+

Clear on kexec.

With register_reboot_notifier?

Quoted 77 lines

Quoted 77 lines

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

 	if (has_steal_clock)
 		kvm_register_steal_time();
 }

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

index 8584322..9e38e12 100644
@@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 			irq->level, irq->trig_mode);
 }
 
-static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
+{
+
+	return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
+				      sizeof(val));
+}
+
+static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
+{
+
+	return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
+				      sizeof(*val));
+}
+
+static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
+{
+	return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
+}
+
+static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
+{
+	u32 val;
+	if (eoi_get_user(vcpu, &val) < 0)
+		apic_debug("Can't read EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+	if (!(val & 0x1))
+		vcpu->arch.eoi.vector = -1;
+	return vcpu->arch.eoi.vector;
+}
+
+static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
+{
+	BUG_ON(vcpu->arch.eoi.vector != -1);
+	if (eoi_put_user(vcpu, 0x1) < 0) {
+		apic_debug("Can't set EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.vector = vector;
+}
+
+static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
+{
+	int vector;
+	vector = vcpu->arch.eoi.vector;
+	if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
+		apic_debug("Can't clear EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return -1;
+	}
+	vcpu->arch.eoi.vector = -1;
+	return vector;
+}
 
 
 
+
+static inline int __apic_find_highest_isr(struct kvm_lapic *apic)

{

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

 	int result;

@@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 

Quoted 12 lines

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 341

Quoted 10 lines

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

+static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (eoi_enabled(apic->vcpu)) {
+		vector = eoi_get_pending_vector(apic->vcpu);
+		if (vector != -1)
+			return vector;
+	}
+	return __apic_find_highest_isr(apic);
+}

Why aren't you modifying the ISR unconfitionally?

ISR is not set if there won't be an EOI
since it's EOI that normally clears it.

Quoted 22 lines

Quoted 22 lines

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

+
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;

@@ -488,6 +553,8 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 	if (vector == -1)
 		return;
 
+	if (eoi_enabled(apic->vcpu))
+		eoi_clr_pending_vector(apic->vcpu);
 	apic_clear_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 

@@ -1236,11 +1303,25 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 {
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
+	bool set_isr = true;
 
 	if (vector == -1)
 		return -1;
 

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1314

Quoted 5 lines

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

-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	if (eoi_enabled(vcpu)) {
+		/* Anything pending? If yes disable eoi optimization. */
+		if (unlikely(apic_find_highest_isr(apic) >= 0)) {
+			int v = eoi_clr_pending_vector(vcpu);

ISR != pending, that's IRR. If ISR vector has lower priority than the new vector, then we don't need to disable eoi avoidance.

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?
I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Marcelo said linux does not normally read ISR - not true? Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Disabling would be very sad. Can we sample on remote irr read?

Quoted 13 lines

Quoted 10 lines

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

+		}
+	}
+
+	if (set_isr)
+		apic_set_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);
 	return vector;

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

Signature · 1 line

error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Heh, I was working on that too.

Quoted 233 lines · Michael S. Tsirkin

Quoted 62 lines · Avi Kivity

Quoted 35 lines · Michael S. Tsirkin

I took a stub at implementing PV EOI using shared memory. This should reduce the number of exits an interrupt causes as much as by half.

A partially complete draft for both host and guest parts is below.

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
We set it before injecting an interrupt and clear before injecting a nested one. Guest tests it using a test and clear operation - this is necessary
so that host can detect interrupt nesting -
and if set, it can skip the EOI MSR.

There's a new MSR to set the address of said register in guest memory. Otherwise not much changes:
- Guest EOI is not required
- ISR is automatically cleared before injection

Some things are incomplete: add feature negotiation options, qemu support for said options.
No testing was done beyond compiling the kernel.

I would appreciate early feedback.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

arch/x86/include/asm/apic.h +0 -0

index d854101..8430f41 100644

arch/x86/include/asm/apic.h static inline u32 safe_apic_wait_icr_idle(void) { return 0; } · line 466

Quoted 10 lines

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

@@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) { return 0; }
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
+
 static inline void ack_APIC_irq(void)
 {
+	if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
+		return;
+

While __test_and_clear_bit() is implemented in a single instruction, it's not required to be. Better have the instruction there explicitly.

Quoted 12 lines

arch/x86/include/asm/apic.h +0 -0

 	/*
 	 * ack_APIC_irq() actually gets compiled as a single instruction
 	 * ... yummie.

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

index e216ba0..0ee1472 100644
@@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
 		u64 length;
 		u64 status;
 	} osvw;
+

Quoted 8 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 490

Quoted 6 lines

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

+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		int vector;
+	} eoi;
 };

Needs to be cleared on INIT.

You mean kvm_arch_vcpu_reset?

Quoted 11 lines

arch/x86/include/asm/kvm_host.h void __cpuinit kvm_guest_cpu_init(void) · line 313

Quoted 9 lines

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

@@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
+	       MSR_KVM_EOI_ENABLED);
+

Clear on kexec.

With register_reboot_notifier?

Quoted 77 lines

Quoted 77 lines

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

 	if (has_steal_clock)
 		kvm_register_steal_time();
 }

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

index 8584322..9e38e12 100644
@@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 			irq->level, irq->trig_mode);
 }
 
-static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
+{
+
+	return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
+				      sizeof(val));
+}
+
+static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
+{
+
+	return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
+				      sizeof(*val));
+}
+
+static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
+{
+	return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
+}
+
+static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
+{
+	u32 val;
+	if (eoi_get_user(vcpu, &val) < 0)
+		apic_debug("Can't read EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+	if (!(val & 0x1))
+		vcpu->arch.eoi.vector = -1;
+	return vcpu->arch.eoi.vector;
+}
+
+static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
+{
+	BUG_ON(vcpu->arch.eoi.vector != -1);
+	if (eoi_put_user(vcpu, 0x1) < 0) {
+		apic_debug("Can't set EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.vector = vector;
+}
+
+static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
+{
+	int vector;
+	vector = vcpu->arch.eoi.vector;
+	if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
+		apic_debug("Can't clear EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return -1;
+	}
+	vcpu->arch.eoi.vector = -1;
+	return vector;
+}
 
 
 
+
+static inline int __apic_find_highest_isr(struct kvm_lapic *apic)

{

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

 	int result;

@@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 

Quoted 12 lines

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 341

Quoted 10 lines

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

+static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (eoi_enabled(apic->vcpu)) {
+		vector = eoi_get_pending_vector(apic->vcpu);
+		if (vector != -1)
+			return vector;
+	}
+	return __apic_find_highest_isr(apic);
+}

Why aren't you modifying the ISR unconfitionally?

ISR is not set if there won't be an EOI
since it's EOI that normally clears it.

Quoted 22 lines

Quoted 22 lines

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

+
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;

@@ -488,6 +553,8 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 	if (vector == -1)
 		return;
 
+	if (eoi_enabled(apic->vcpu))
+		eoi_clr_pending_vector(apic->vcpu);
 	apic_clear_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 

@@ -1236,11 +1303,25 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 {
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
+	bool set_isr = true;
 
 	if (vector == -1)
 		return -1;
 

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1314

Quoted 5 lines

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

-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	if (eoi_enabled(vcpu)) {
+		/* Anything pending? If yes disable eoi optimization. */
+		if (unlikely(apic_find_highest_isr(apic) >= 0)) {
+			int v = eoi_clr_pending_vector(vcpu);

ISR != pending, that's IRR. If ISR vector has lower priority than the new vector, then we don't need to disable eoi avoidance.

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?
I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Marcelo said linux does not normally read ISR - not true? Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Quoted 2 lines

Disabling would be very sad. Can we sample on remote irr read?

Nothing sad about it, just correct. MS requires this to be disabled for level triggered interrupts.

We have to notify IOAPIC about EOI ASAP. It
may hold another interrupt for us that has to be delivered.

I was going to avoid most of the trickery in apic code and just check if avoided EOI should be processed on each exit. This adds one if on exit path instead of couple on interrupt injection path though.

Quoted 13 lines

Quoted 13 lines

Quoted 10 lines

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

+		}
+	}
+
+	if (set_isr)
+		apic_set_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);
 	return vector;

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

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Quoted 236 lines · Gleb Natapov

Heh, I was working on that too.

Quoted 233 lines · Michael S. Tsirkin

Quoted 62 lines · Avi Kivity

Quoted 35 lines · Michael S. Tsirkin

I took a stub at implementing PV EOI using shared memory. This should reduce the number of exits an interrupt causes as much as by half.

A partially complete draft for both host and guest parts is below.

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
We set it before injecting an interrupt and clear before injecting a nested one. Guest tests it using a test and clear operation - this is necessary
so that host can detect interrupt nesting -
and if set, it can skip the EOI MSR.

There's a new MSR to set the address of said register in guest memory. Otherwise not much changes:
- Guest EOI is not required
- ISR is automatically cleared before injection

Some things are incomplete: add feature negotiation options, qemu support for said options.
No testing was done beyond compiling the kernel.

I would appreciate early feedback.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

arch/x86/include/asm/apic.h +0 -0

index d854101..8430f41 100644

arch/x86/include/asm/apic.h static inline u32 safe_apic_wait_icr_idle(void) { return 0; } · line 466

Quoted 10 lines

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

@@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) { return 0; }
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
+
 static inline void ack_APIC_irq(void)
 {
+	if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
+		return;
+

While __test_and_clear_bit() is implemented in a single instruction, it's not required to be. Better have the instruction there explicitly.

Quoted 12 lines

arch/x86/include/asm/apic.h +0 -0

 	/*
 	 * ack_APIC_irq() actually gets compiled as a single instruction
 	 * ... yummie.

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

index e216ba0..0ee1472 100644
@@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
 		u64 length;
 		u64 status;
 	} osvw;
+

Quoted 8 lines

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 490

Quoted 6 lines

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

+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		int vector;
+	} eoi;
 };

Needs to be cleared on INIT.

You mean kvm_arch_vcpu_reset?

Quoted 11 lines

arch/x86/include/asm/kvm_host.h void __cpuinit kvm_guest_cpu_init(void) · line 313

Quoted 9 lines

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

@@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
+	       MSR_KVM_EOI_ENABLED);
+

Clear on kexec.

With register_reboot_notifier?

Quoted 77 lines

Quoted 77 lines

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

 	if (has_steal_clock)
 		kvm_register_steal_time();
 }

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

index 8584322..9e38e12 100644
@@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 			irq->level, irq->trig_mode);
 }
 
-static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
+{
+
+	return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
+				      sizeof(val));
+}
+
+static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
+{
+
+	return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
+				      sizeof(*val));
+}
+
+static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
+{
+	return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
+}
+
+static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
+{
+	u32 val;
+	if (eoi_get_user(vcpu, &val) < 0)
+		apic_debug("Can't read EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+	if (!(val & 0x1))
+		vcpu->arch.eoi.vector = -1;
+	return vcpu->arch.eoi.vector;
+}
+
+static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
+{
+	BUG_ON(vcpu->arch.eoi.vector != -1);
+	if (eoi_put_user(vcpu, 0x1) < 0) {
+		apic_debug("Can't set EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.vector = vector;
+}
+
+static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
+{
+	int vector;
+	vector = vcpu->arch.eoi.vector;
+	if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
+		apic_debug("Can't clear EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return -1;
+	}
+	vcpu->arch.eoi.vector = -1;
+	return vector;
+}
 
 
 
+
+static inline int __apic_find_highest_isr(struct kvm_lapic *apic)

{

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

 	int result;

@@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 

Quoted 12 lines

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 341

Quoted 10 lines

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

+static inline int apic_find_highest_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (eoi_enabled(apic->vcpu)) {
+		vector = eoi_get_pending_vector(apic->vcpu);
+		if (vector != -1)
+			return vector;
+	}
+	return __apic_find_highest_isr(apic);
+}

Why aren't you modifying the ISR unconfitionally?

ISR is not set if there won't be an EOI
since it's EOI that normally clears it.

Quoted 22 lines

Quoted 22 lines

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

+
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;

@@ -488,6 +553,8 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 	if (vector == -1)
 		return;
 
+	if (eoi_enabled(apic->vcpu))
+		eoi_clr_pending_vector(apic->vcpu);
 	apic_clear_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 

@@ -1236,11 +1303,25 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 {
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
+	bool set_isr = true;
 
 	if (vector == -1)
 		return -1;
 

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1314

Quoted 5 lines

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

-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	if (eoi_enabled(vcpu)) {
+		/* Anything pending? If yes disable eoi optimization. */
+		if (unlikely(apic_find_highest_isr(apic) >= 0)) {
+			int v = eoi_clr_pending_vector(vcpu);

ISR != pending, that's IRR. If ISR vector has lower priority than the new vector, then we don't need to disable eoi avoidance.

Yes. But we can and it's easier than figuring out priorities. I am guessing such collisions are rare, right?
I'll add a trace to make sure.

Quoted 8 lines

arch/x86/kvm/lapic.c int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) · line 1319

Quoted 5 lines

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

+			if (v != -1)
+				apic_set_vector(v, apic->regs + APIC_ISR);
+		} else {
+			eoi_set_pending_vector(vcpu, vector);
+			set_isr = false;

Weird. Just set it normally. Remember that reading the ISR needs to return the correct value.

Marcelo said linux does not normally read ISR - not true? Note this has no effect if the PV optimization is not enabled.

Quoted 4 lines

We need to process the avoided EOI before any APIC read/writes, to be sure the guest sees the updated values. Same for IOAPIC, EOI affects remote_irr. That may been we need to sample it after every exit, or perhaps disable the feature for level-triggered interrupts.

Quoted 4 lines

Quoted 2 lines

Disabling would be very sad. Can we sample on remote irr read?

Nothing sad about it, just correct. MS requires this to be disabled for level triggered interrupts.

We don't try to match what HV does 100% anyway.

Quoted 2 lines

We have to notify IOAPIC about EOI ASAP. It
may hold another interrupt for us that has to be delivered.

You mean the ack notifiers? We can skip just for the vectors which have ack notifiers or only if there are no notifiers.

Quoted 17 lines

I was going to avoid most of the trickery in apic code and just check if avoided EOI should be processed on each exit. This adds one if on exit path instead of couple on interrupt injection path though.

Quoted 13 lines

Quoted 13 lines

Quoted 10 lines

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

+		}
+	}
+
+	if (set_isr)
+		apic_set_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);
 	return vector;

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

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 7 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

Quoted 2 lines

We don't try to match what HV does 100% anyway.

We should. The same code will be used for HV.

Quoted 6 lines

Quoted 2 lines

We have to notify IOAPIC about EOI ASAP. It
may hold another interrupt for us that has to be delivered.

You mean the ack notifiers? We can skip just for the vectors which have ack notifiers or only if there are no notifiers.

No. I mean:

                if (!ent->fields.mask && (ioapic->irr & (1 << i)))
                        ioapic_service(ioapic, i);

Quoted 3 lines · Gleb Natapov

Quoted 2 lines

We don't try to match what HV does 100% anyway.

We should. The same code will be used for HV.

Only where it makes sense, that is where the functionality is sufficiently similar.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

We have to notify IOAPIC about EOI ASAP. It
may hold another interrupt for us that has to be delivered.

You mean the ack notifiers? We can skip just for the vectors which have ack notifiers or only if there are no notifiers.

Quoted 4 lines

No. I mean:

                if (!ent->fields.mask && (ioapic->irr & (1 << i)))
                        ioapic_service(ioapic, i);

Hmm.

I got lots of useful feedback from v0 so I thought sending out a brain dump again would be a good idea. This is mainly to show how I'm trying to address the comments I got from the previous round. Flames/feedback are wellcome!

Changes from v0:
- Tweaked setup MSRs a bit
- Keep ISR bit set. Before reading ISR, test EOI in guest memory and clear
- Check priority for nested interrupts, we can
enable optimization if new one is high priority
- Disable optimization for any interrupt handled by ioapic
(This is because ioapic handles notifiers and pic and it generally gets messy.
It's possible that we can optimize some ioapic-handled
edge interrupts - but is it worth it?)
- A less intrusive change for guest apic (0 overhead without kvm)

---

I took a stub at implementing PV EOI using shared memory. This should reduce the number of exits an interrupt causes as much as by half.

A partially complete draft for both host and guest parts is below.

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
We set it before injecting an interrupt and clear before injecting a nested one. Guest tests it using a test and clear operation - this is necessary
so that host can detect interrupt nesting -
and if set, it can skip the EOI MSR.

There's a new MSR to set the address of said register in guest memory. Otherwise not much changed:
- Guest EOI is not required
- Register is tested ISR is automatically cleared before injection

qemu support is incomplete - mostly for feature negotiation. need to add some trace points to enable profiling. No testing was done beyond compiling the kernel.

Signed-off-by Michael S. Tsirkin <[email protected]>

arch/x86/include/asm/bitops.h +4 -2

index b97596e..c9c70ea 100644
@@ -26,11 +26,13 @@
 #if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)
 /* Technically wrong, but this avoids compilation errors on some gcc
    versions. */
-#define BITOP_ADDR(x) "=m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "=m"
 #else
-#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "+m"
 #endif
 
+#define BITOP_ADDR(x) BITOP_ADDR_CONSTRAINT (*(volatile long *) (x))
+
 #define ADDR				BITOP_ADDR(addr)
 
 /*

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

index e216ba0..3d09ef1 100644
@@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
 		u64 length;
 		u64 status;
 	} osvw;
+
+	struct {
+		u64 msr_val;
+		struct gfn_to_hva_cache data;
+		bool pending;
+	} eoi;
 };
 
 struct kvm_lpage_info {

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

index 734c376..164376a 100644
@@ -22,6 +22,7 @@
 #define KVM_FEATURE_CLOCKSOURCE2        3
 #define KVM_FEATURE_ASYNC_PF		4
 #define KVM_FEATURE_STEAL_TIME		5
+#define KVM_FEATURE_EOI			6
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.

arch/x86/include/asm/kvm_para.h +0 -0

 struct kvm_steal_time {
 	__u64 steal;

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

index b8ba6e4..450aae4 100644
@@ -39,6 +39,7 @@
 #include <asm/desc.h>
 #include <asm/tlbflush.h>
 #include <asm/idle.h>
+#include <asm/apic.h>
 
 static int kvmapf = 1;
 

@@ -290,6 +291,33 @@ static void kvm_register_steal_time(void)
 		cpu, __pa(st));
 }
 
+/* TODO: needs to be early? aligned? */
+static DEFINE_EARLY_PER_CPU(u8, apic_eoi, 0);
+
+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */
+static inline int kvm_test_and_clear_bit(int nr, volatile u8* addr)
+{
+	int oldbit;
+
+	asm volatile("btr %2,%1\n\t"
+		     "sbb %0,%0"
+		     : "=r" (oldbit),
+		     BITOP_ADDR_CONSTRAINT (*(volatile u8 *) (addr))
+		     : "Ir" (nr));
+	return oldbit;
+}
+
+static void (*kvm_guest_native_apic_write)(u32 reg, u32 val);
+static void kvm_guest_apic_write(u32 reg, u32 val)
+{
+	if (reg == APIC_EOI &&
+	    kvm_test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
+			return;
+
+	kvm_guest_native_apic_write(reg, val);
+}
+
 void __cpuinit kvm_guest_cpu_init(void)
 {
 	if (!kvm_para_available())

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

+
 	if (has_steal_clock)
 		kvm_register_steal_time();
 }
 
-static void kvm_pv_disable_apf(void *unused)
+static void kvm_pv_disable_apf(void)
 {
 	if (!__get_cpu_var(apf_reason).enabled)
 		return;

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

+	kvm_pv_disable_apf();
+}
+
 static int kvm_pv_reboot_notify(struct notifier_block *nb,
 				unsigned long code, void *unused)
 {
 	if (code == SYS_RESTART)
-		on_each_cpu(kvm_pv_disable_apf, NULL, 1);
+		on_each_cpu(kvm_pv_guest_cpu_reboot, NULL, 1);
 	return NOTIFY_DONE;
 }
 

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

+	kvm_pv_disable_apf();
 	apf_task_wake_all();
 }
 

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

index 9fed5be..7d00d2d 100644
@@ -408,6 +408,7 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 			     (1 << KVM_FEATURE_NOP_IO_DELAY) |
 			     (1 << KVM_FEATURE_CLOCKSOURCE2) |
 			     (1 << KVM_FEATURE_ASYNC_PF) |
+			     (1 << KVM_FEATURE_EOI) |
 			     (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
 
 		if (sched_info_on())

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

index 7e06ba1..07bdfab 100644
@@ -48,7 +48,7 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 	if (!irqchip_in_kernel(v->kvm))
 		return v->arch.interrupt.pending;
 
-	if (kvm_apic_has_interrupt(v) == -1) {	/* LAPIC */
+	if (kvm_apic_has_interrupt(v) < 0) {	/* LAPIC */
 		if (kvm_apic_accept_pic_intr(v)) {
 			s = pic_irqchip(v->kvm);	/* PIC */
 			return s->output;

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

index 992b4ea..c2118ab 100644
@@ -270,6 +270,54 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 			irq->level, irq->trig_mode);
 }
 
+static int eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
+{
+
+	return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
+				      sizeof(val));
+}
+
+static int eoi_get_user(struct kvm_vcpu *vcpu, u8 *val)
+{
+
+	return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
+				      sizeof(*val));
+}
+
+static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.eoi.msr_val != MSR_KVM_EOI_DISABLED;
+}
+
+static bool eoi_get_pending(struct kvm_vcpu *vcpu)
+{
+	u8 val;
+	if (eoi_get_user(vcpu, &val) < 0)
+		apic_debug("Can't read EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+	return val & 0x1;
+}
+
+static void eoi_set_pending(struct kvm_vcpu *vcpu)
+{
+	if (eoi_put_user(vcpu, 0x1) < 0) {
+		apic_debug("Can't set EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.pending = true;
+}
+
+static void eoi_clr_pending(struct kvm_vcpu *vcpu)
+{
+	if (eoi_put_user(vcpu, 0x0) < 0) {
+		apic_debug("Can't clear EOI MSR value: 0x%llx\n",
+			   (unsigned long long)vcpi->arch.eoi.msr_val);
+		return;
+	}
+	vcpu->arch.eoi.pending = false;
+}
+
 static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 {
 	int result;

@@ -280,6 +328,20 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 
+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||

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

 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;

@@ -287,6 +349,7 @@ static void apic_update_ppr(struct kvm_lapic *apic)
 
 	old_ppr = apic_get_reg(apic, APIC_PROCPRI);
 	tpr = apic_get_reg(apic, APIC_TASKPRI);
+	apic_update_isr(apic);
 	isr = apic_find_highest_isr(apic);
 	isrv = (isr != -1) ? isr : 0;
 

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

 	apic_clear_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 

@@ -1085,6 +1150,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 	atomic_set(&apic->lapic_timer.pending, 0);
 	if (kvm_vcpu_is_bsp(vcpu))
 		vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP;
+	vcpu->arch.eoi.msr_val = MSR_KVM_EOI_DISABLED;
+	vcpu->arch.eoi.pending = false;
 	apic_update_ppr(apic);
 
 	vcpu->arch.apic_arb_prio = 0;

@@ -1210,9 +1277,10 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
 
 	apic_update_ppr(apic);
 	highest_irr = apic_find_highest_irr(apic);
-	if ((highest_irr == -1) ||
-	    ((highest_irr & 0xF0) <= apic_get_reg(apic, APIC_PROCPRI)))
+	if (highest_irr == -1)
 		return -1;
+	if (((highest_irr & 0xF0) <= apic_get_reg(apic, APIC_PROCPRI)))
+		return -2;
 	return highest_irr;
 }
 

@@ -1244,9 +1312,20 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	int vector = kvm_apic_has_interrupt(vcpu);
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (vector == -1)
+	/* Detect interrupt nesting and disable EOI optimization */
+	if (eoi_enabled(vcpu) && vector == -2)
+		eoi_clr_pending(vcpu);
+
+	if (vector < 0)
 		return -1;
 
+	if (eoi_enabled(vcpu)) {
+		if (kvm_ioapic_handles_vector(vcpu->kvm, vector))
+			eoi_clr_pending(vcpu);
+		else
+			eoi_set_pending(vcpu);
+	}
+
 	apic_set_vector(vector, apic->regs + APIC_ISR);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);

@@ -1261,6 +1340,8 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 			     MSR_IA32_APICBASE_BASE;
 	kvm_apic_set_version(vcpu);
 
+	if (eoi_enabled(apic->vcpu))
+	    apic->vcpu->arch.eoi.pending = eoi_get_pending(apic->vcpu);
 	apic_update_ppr(apic);
 	hrtimer_cancel(&apic->lapic_timer.timer);
 	update_divide_count(apic);

@@ -1392,3 +1473,18 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 
 	return 0;
 }
+
+int kvm_pv_enable_apic_eoi(struct kvm_vcpu *vcpu, u64 data)
+{
+	if (data == MSR_KVM_EOI_DISABLED) {
+		struct kvm_lapic *apic = vcpu->arch.apic;
+		if (apic && apic_enabled(apic))
+			apic_update_isr(apic);
+	} else if (kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.eoi.data,
+					     data))
+		return 1;
+
+	vcpu->arch.eoi.msr_val = data;
+	vcpu->arch.eoi.pending = false;
+	return 0;
+}

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

index 6f4ce25..042dace 100644
@@ -60,4 +60,6 @@ static inline bool kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.hv_vapic & HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE;
 }
+
+int kvm_pv_enable_apic_eoi(struct kvm_vcpu *vcpu, u64 data);
 #endif

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

index 4044ce0..31d6762 100644
@@ -1627,6 +1627,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		if (kvm_pv_enable_async_pf(vcpu, data))
 			return 1;
 		break;
+	case MSR_KVM_EOI_EN:
+		if (kvm_pv_enable_apic_eoi(vcpu, data))
+			return 1;
+		break;
 	case MSR_KVM_STEAL_TIME:
 
 		if (unlikely(!sched_info_on()))
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

arch/x86/include/asm/kvm_para.h line 42

Quoted 6 lines

arch/x86/include/asm/kvm_para.h +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

Quoted 8 lines

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

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||
+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 344

Quoted 6 lines

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

+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

Thanks very much for the review. I'll address the comments. Some questions on your comments below.

Quoted 8 lines · Gleb Natapov

arch/x86/include/asm/kvm_para.h line 42

Quoted 6 lines

arch/x86/include/asm/kvm_para.h +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right?

Quoted 8 lines

Quoted 8 lines

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

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||
+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;

Quoted 8 lines

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 344

Quoted 6 lines

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

+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free.

Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there.

Is there anything else that I missed?

Quoted 4 lines

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.

Quoted 13 lines · Michael S. Tsirkin

Thanks very much for the review. I'll address the comments. Some questions on your comments below.

Quoted 8 lines · Gleb Natapov

arch/x86/include/asm/kvm_para.h line 42

Quoted 6 lines

arch/x86/include/asm/kvm_para.h +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

Quoted 4 lines

We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right?

We do not have shortage of memory.
Better make all MSRs works the same
way.

BTW have you added new MSR to msrs_to_save array? I forgot to checked.

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

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

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 344

Quoted 10 lines

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

+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;
+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

Quoted 4 lines

It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free.

How much time those extra things are taking compared to vmexit you already serving? And there is a good chance you will do them during vmentry anyway while trying to inject (or just check for) new interrupt.

Quoted 4 lines

Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there.

Is there anything else that I missed?

I think that simple things are better then complex things if the end result is the same :) Try it and see how much simpler it is.

Have you measured
that what you are trying to optimize actually worth optimizing? That you can measure the optimization at all?

Quoted 9 lines

Quoted 4 lines

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.

It does it only if vapic is in use (and it is usually not).

But the if()
is already there so we do not need to worry that one additional if() on the exit path will slow KVM to the crawl.

--
Gleb.

Quoted 18 lines · Gleb Natapov

Quoted 17 lines · Michael S. Tsirkin

Thanks very much for the review. I'll address the comments. Some questions on your comments below.

Quoted 8 lines · Gleb Natapov

arch/x86/include/asm/kvm_para.h line 42

Quoted 6 lines

arch/x86/include/asm/kvm_para.h +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right?

Quoted 3 lines

We do not have shortage of memory.
Better make all MSRs works the same
way.

I agree it's nice to have EOI and ASYNC_PF look similar but wasting memory is also bad. I'll ponder this some more.

Quoted 2 lines

BTW have you added new MSR to msrs_to_save array? I forgot to checked.

I didn't yet. Trying to understand how will that affect cross-version migration - any input?

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

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

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 344

Quoted 10 lines

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

+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;
+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

Quoted 7 lines

Quoted 4 lines

It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free.

How much time those extra things are taking compared to vmexit you already serving? And there is a good chance you will do them during vmentry anyway while trying to inject (or just check for) new interrupt.

No need to do them twice :)

Quoted 3 lines

Quoted 3 lines

Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there.

Quoted 3 lines

Quoted 1 line

Is there anything else that I missed?

I think that simple things are better then complex things if the end result is the same :) Try it and see how much simpler it is.

It doesn't seem to be simpler at all. The common functionality is about 4 lines.

Quoted 4 lines

Have you measured
that what you are trying to optimize actually worth optimizing? That you can measure the optimization at all?

The claim is not that it's measureable. The claim is that it does not scale to keep adding things to do on each entry.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

Quoted 5 lines

Quoted 4 lines

It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.

It does it only if vapic is in use (and it is usually not).

When it's not we don't need to update ppr and so
no need to update isr on this exit.

Quoted 4 lines

But the if()
is already there so we do not need to worry that one additional if() on the exit path will slow KVM to the crawl.

The number of things we need to do on each entry keeps going up, if we just keep adding stuff it won't end well.

Quoted 23 lines · Michael S. Tsirkin

Quoted 21 lines · Gleb Natapov

Quoted 13 lines · Michael S. Tsirkin

Thanks very much for the review. I'll address the comments. Some questions on your comments below.

Quoted 8 lines · Gleb Natapov

arch/x86/include/asm/kvm_para.h line 42

Quoted 6 lines

arch/x86/include/asm/kvm_para.h +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

Quoted 4 lines

We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right?

We do not have shortage of memory.
Better make all MSRs works the same
way.

Quoted 3 lines

I agree it's nice to have EOI and ASYNC_PF look similar but wasting memory is also bad. I'll ponder this some more.

Steal time and kvm clock too and may be others (if anything left at all). I hope you are kidding about wasting of 4 bytes per vcpu.

Quoted 6 lines

Quoted 2 lines

BTW have you added new MSR to msrs_to_save array? I forgot to checked.

I didn't yet. Trying to understand how will that affect cross-version migration - any input?

Not sure. You need to check what userspace does with them.

Quoted 30 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

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

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||

arch/x86/kvm/lapic.c static inline int apic_find_highest_isr(struct kvm_lapic *apic) · line 344

Quoted 10 lines

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

+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;
+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

Quoted 7 lines

Quoted 4 lines

It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free.

How much time those extra things are taking compared to vmexit you already serving? And there is a good chance you will do them during vmentry anyway while trying to inject (or just check for) new interrupt.

No need to do them twice :)

Quoted 3 lines

Quoted 3 lines

Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Is there anything else that I missed?

I think that simple things are better then complex things if the end result is the same :) Try it and see how much simpler it is.

It doesn't seem to be simpler at all. The common functionality is about 4 lines.

Send patch for us to see.

lapic changes should be minimal.

Quoted 5 lines

Quoted 4 lines

Have you measured
that what you are trying to optimize actually worth optimizing? That you can measure the optimization at all?

Quoted 3 lines

The claim is not that it's measureable. The claim is that it does not scale to keep adding things to do on each entry.

Only if there is something to do. "Premature optimization is the root of all evil". The PV eoi is about not exiting on eoi unnecessary. You are mixing this with trying to avoid calling eoi code for given interrupt at all.

Two different optimization, do not try lump them together.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.

Quoted 4 lines

Quoted 1 line

It does it only if vapic is in use (and it is usually not).

When it's not we don't need to update ppr and so
no need to update isr on this exit.

If there was eoi we need to update both.

Quoted 8 lines

Quoted 4 lines

But the if()
is already there so we do not need to worry that one additional if() on the exit path will slow KVM to the crawl.

The number of things we need to do on each entry keeps going up, if we just keep adding stuff it won't end well.

You do not add stuff. The if() is already there.

--
Gleb.

Quoted 24 lines · Gleb Natapov

Quoted 23 lines · Michael S. Tsirkin

Quoted 21 lines · Gleb Natapov

Quoted 13 lines · Michael S. Tsirkin

Thanks very much for the review. I'll address the comments. Some questions on your comments below.

Quoted 8 lines · Gleb Natapov

Review line 42

Quoted 6 lines

(quoted diff) +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

Quoted 4 lines

We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right?

We do not have shortage of memory.
Better make all MSRs works the same
way.

Quoted 5 lines

Quoted 3 lines

I agree it's nice to have EOI and ASYNC_PF look similar but wasting memory is also bad. I'll ponder this some more.

Steal time and kvm clock too and may be others (if anything left at all). I hope you are kidding about wasting of 4 bytes per vcpu.

Not vcpu - cpu. It's wasted whenever kernel/kvm.c is built so it has cost on physical machines as well.

Quoted 42 lines

Quoted 6 lines

Quoted 2 lines

BTW have you added new MSR to msrs_to_save array? I forgot to checked.

I didn't yet. Trying to understand how will that affect cross-version migration - any input?

Not sure. You need to check what userspace does with them.

Quoted 34 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

(quoted diff) +4 -0

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||

Review

Quoted 10 lines

(quoted diff) +10 -0

+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;
+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

Quoted 7 lines

Quoted 4 lines

It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free.

How much time those extra things are taking compared to vmexit you already serving? And there is a good chance you will do them during vmentry anyway while trying to inject (or just check for) new interrupt.

No need to do them twice :)

Quoted 6 lines

Quoted 4 lines

Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there.

Is there anything else that I missed?

I think that simple things are better then complex things if the end result is the same :) Try it and see how much simpler it is.

Quoted 3 lines

Quoted 2 lines

It doesn't seem to be simpler at all. The common functionality is about 4 lines.

Send patch for us to see.

That's what you are replying to, no?
You can see that it is 4 lines of code.

Quoted 2 lines

lapic changes should be minimal.

Exactly my motivation.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Have you measured
that what you are trying to optimize actually worth optimizing? That you can measure the optimization at all?

Quoted 7 lines

Quoted 3 lines

The claim is not that it's measureable. The claim is that it does not scale to keep adding things to do on each entry.

Only if there is something to do. "Premature optimization is the root of all evil". The PV eoi is about not exiting on eoi unnecessary. You are mixing this with trying to avoid calling eoi code for given interrupt at all.

I don't think this is what my patch does. EOI still clears ISR for each interrupt.

Quoted 10 lines

Two different optimization, do not try lump them together.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

It does it only if vapic is in use (and it is usually not).

When it's not we don't need to update ppr and so
no need to update isr on this exit.

If there was eoi we need to update both.

By same logic we should call update_ppr on each entry. The overhead is unlikely to be measureable either :).

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

But the if()
is already there so we do not need to worry that one additional if() on the exit path will slow KVM to the crawl.

Quoted 4 lines

Quoted 3 lines

The number of things we need to do on each entry keeps going up, if we just keep adding stuff it won't end well.

You do not add stuff. The if() is already there.

Your proposal was to check userspace eoi record
each time when eoi is pending, no?

This would certainly add some overhead.

I also find the logic easier to follow as is - it is contained in lapic.c without relying on being called from x86.c as just the right moment.

Quoted 4 lines

Quoted 1 line

lapic changes should be minimal.

Exactly my motivation.

My patch removes 13 lines more :)

Quoted 4 lines

I also find the logic easier to follow as is - it is contained in lapic.c without relying on being called from x86.c as just the right moment.

See the patch. It change nothing outside of lapic.c.

Quoted 5 lines · Gleb Natapov

Quoted 4 lines

Quoted 1 line

lapic changes should be minimal.

Exactly my motivation.

My patch removes 13 lines more :)

Haven't checked whether your patch is correct yet but I see it's checking the eoi register on each exit.

I think it's clear this would make code a bit shorter (not necessarily clearer as we are relying on ) but as I said even though the extra overhead is likely negligeable I have a feeling it's a wrong approach since this won't scale as we add more features.

Let's see what do others think.

Quoted 5 lines

Quoted 4 lines

I also find the logic easier to follow as is - it is contained in lapic.c without relying on being called from x86.c as just the right moment.

See the patch. It change nothing outside of lapic.c.

True but you rely on x86.c to call kvm_sync_lapic_from_vapic at the right time before injecting interrupts.

I haven't checked whether that is always the case but to me, this makes the code less clear and more fragile.

Again, it appears to be a matter of taste.

Signature · 1 line

MST

Quoted 3 lines · Michael S. Tsirkin

Quoted 2 lines · Gleb Natapov

Quoted 2 lines

Quoted 1 line

lapic changes should be minimal.

Quoted 7 lines

Quoted 3 lines

Quoted 2 lines

Exactly my motivation.

My patch removes 13 lines more :)

Haven't checked whether your patch is correct yet but I see it's checking the eoi register on each exit.

Only if eoi.pending == true on exit.

Quoted 8 lines

I think it's clear this would make code a bit shorter (not necessarily clearer as we are relying on ) but as I said even though the extra overhead is likely negligeable I have a feeling it's a wrong approach since this won't scale as we add more features.

Let's see what do others think.

What I do not like about not calling eoi here is that we have to reason about all the paths into apic and check that we clear isr on all of them.

And that is not all. eoi handler
set event request, is it OK to skip it? May be, or may be not.

Quoted 8 lines

Quoted 5 lines

Quoted 4 lines

I also find the logic easier to follow as is - it is contained in lapic.c without relying on being called from x86.c as just the right moment.

See the patch. It change nothing outside of lapic.c.

True but you rely on x86.c to call kvm_sync_lapic_from_vapic at the right time before injecting interrupts.

Not before injecting interrupts, but on vmexit.

Quoted 3 lines

I haven't checked whether that is always the case but to me, this makes the code less clear and more fragile.

We call a lot of apic functions from x86.c. That's were interrupt injection happens.

Quoted 4 lines

Again, it appears to be a matter of taste.

Signature · 1 line

MST

--
Gleb.

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Michael S. Tsirkin

Quoted 5 lines · Gleb Natapov

Quoted 4 lines

Quoted 1 line

lapic changes should be minimal.

Exactly my motivation.

My patch removes 13 lines more :)

Quoted 4 lines

Quoted 3 lines

Haven't checked whether your patch is correct yet but I see it's checking the eoi register on each exit.

Only if eoi.pending == true on exit.

it's checking eoi.pending always and eoi register when that is true.

Quoted 6 lines

Quoted 6 lines

I think it's clear this would make code a bit shorter (not necessarily clearer as we are relying on ) but as I said even though the extra overhead is likely negligeable I have a feeling it's a wrong approach since this won't scale as we add more features.

Quoted 5 lines

Quoted 2 lines

Let's see what do others think.

What I do not like about not calling eoi here is that we have to reason about all the paths into apic and check that we clear isr on all of them.

Not at all. Instead, check each time before we read isr or ppr.

Quoted 3 lines

And that is not all. eoi handler
set event request, is it OK to skip it? May be, or may be not.

I think it's for reinjection of lower prio interrupt.

Since eoi optimization is disabled in that case we don't need to set event request.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

I also find the logic easier to follow as is - it is contained in lapic.c without relying on being called from x86.c as just the right moment.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

See the patch. It change nothing outside of lapic.c.

True but you rely on x86.c to call kvm_sync_lapic_from_vapic at the right time before injecting interrupts.

Not before injecting interrupts, but on vmexit.

sync is called on entry, not on exit, no?

Quoted 8 lines

Quoted 5 lines

I haven't checked whether that is always the case but to me, this makes the code less clear and more fragile.

We call a lot of apic functions from x86.c. That's were interrupt injection happens.

Quoted 7 lines

Quoted 4 lines

Again, it appears to be a matter of taste.

Signature · 1 line

MST

--
Gleb.

Quoted 28 lines · Michael S. Tsirkin

Quoted 27 lines · Gleb Natapov

Quoted 26 lines · Michael S. Tsirkin

Quoted 14 lines · Gleb Natapov

Quoted 13 lines · Michael S. Tsirkin

Thanks very much for the review. I'll address the comments. Some questions on your comments below.

Quoted 8 lines · Gleb Natapov

Review line 42

Quoted 6 lines

(quoted diff) +2 -0

@@ -37,6 +38,8 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
+#define MSR_KVM_EOI_DISABLED 0x0L

This is valid gpa. Follow others MSR example i.e align the address to, lets say dword, and use lsb as enable bit.

Quoted 7 lines

Quoted 4 lines

We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right?

We do not have shortage of memory.
Better make all MSRs works the same
way.

I agree it's nice to have EOI and ASYNC_PF look similar but wasting memory is also bad. I'll ponder this some more.

Quoted 6 lines

Quoted 2 lines

Steal time and kvm clock too and may be others (if anything left at all). I hope you are kidding about wasting of 4 bytes per vcpu.

Not vcpu - cpu. It's wasted whenever kernel/kvm.c is built so it has cost on physical machines as well.

There are less real cpus than vcpus usually :)

Quoted 42 lines

Quoted 42 lines

Quoted 6 lines

Quoted 2 lines

BTW have you added new MSR to msrs_to_save array? I forgot to checked.

I didn't yet. Trying to understand how will that affect cross-version migration - any input?

Not sure. You need to check what userspace does with them.

Quoted 34 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

(quoted diff) +4 -0

+static void apic_update_isr(struct kvm_lapic *apic)
+{
+	int vector;
+	if (!eoi_enabled(apic->vcpu) ||

Review

Quoted 10 lines

(quoted diff) +10 -0

+	    !apic->vcpu->arch.eoi.pending ||
+	    eoi_get_pending(apic->vcpu))
+		return;
+	apic->vcpu->arch.eoi.pending = false;
+	vector = apic_find_highest_isr(apic);
+	if (vector == -1)
+		return;
+	apic_clear_vector(vector, apic->regs + APIC_ISR);
+}
+

We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). This removes the need for the function and its calls.

Quoted 7 lines

Quoted 4 lines

It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free.

How much time those extra things are taking compared to vmexit you already serving? And there is a good chance you will do them during vmentry anyway while trying to inject (or just check for) new interrupt.

No need to do them twice :)

Quoted 6 lines

Quoted 4 lines

Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there.

Is there anything else that I missed?

I think that simple things are better then complex things if the end result is the same :) Try it and see how much simpler it is.

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

It doesn't seem to be simpler at all. The common functionality is about 4 lines.

Send patch for us to see.

That's what you are replying to, no?
You can see that it is 4 lines of code.

No. I mean something like patch below. Applies on top of yours. Did not check that it works or even compiles.

Quoted 1 line

Quoted 4 lines

Quoted 1 line

lapic changes should be minimal.

Exactly my motivation.

My patch removes 13 lines more :)

Quoted 30 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Have you measured
that what you are trying to optimize actually worth optimizing? That you can measure the optimization at all?

Quoted 7 lines

Quoted 3 lines

The claim is not that it's measureable. The claim is that it does not scale to keep adding things to do on each entry.

Only if there is something to do. "Premature optimization is the root of all evil". The PV eoi is about not exiting on eoi unnecessary. You are mixing this with trying to avoid calling eoi code for given interrupt at all.

I don't think this is what my patch does. EOI still clears ISR for each interrupt.

Quoted 14 lines

Two different optimization, do not try lump them together.

Quoted 13 lines

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

We already have
call to kvm_lapic_sync_from_vapic() on exit path which should be extended to do the above.

It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.

It does it only if vapic is in use (and it is usually not).

When it's not we don't need to update ppr and so
no need to update isr on this exit.