RFC: PIC: enhance IPI avoidance

RFC: PIC: enhance IPI avoidance

[Marcelo Tosatti]

The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
unnecessary guest exits in some conditions.

For example, if the timer interrupt is routed through the IOAPIC, IRR
for IRQ 0 will get set but not cleared, since the APIC is handling the
acks.

This means that everytime an interrupt < 16 is triggered, the priority
logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
not masked, which is Linux's case).

Introduce a new variable isr_ack to represent the IRQ's for which the
guest has been signalled / cleared the ISR. Use it to avoid more than
one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
set in get_priority().

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index c31164e..b8665e3 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -191,6 +191,7 @@ void kvm_pic_reset(struct kvm_kpic_state *s)
 	s->irr = 0;
 	s->imr = 0;
 	s->isr = 0;
+	s->isr_ack = 0xff;
 	s->priority_add = 0;
 	s->irq_base = 0;
 	s->read_reg_select = 0;
@@ -244,6 +245,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
 				if (priority != 8) {
 					irq = (priority + s->priority_add) & 7;
 					s->isr &= ~(1 << irq);
+					s->isr_ack |= (1 << irq);
 					if (cmd == 5)
 						s->priority_add = (irq + 1) & 7;
 					pic_update_irq(s->pics_state);
@@ -252,6 +254,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
 			case 3:
 				irq = val & 7;
 				s->isr &= ~(1 << irq);
+				s->isr_ack |= (1 << irq);
 				pic_update_irq(s->pics_state);
 				break;
 			case 6:
@@ -261,6 +264,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
 			case 7:
 				irq = val & 7;
 				s->isr &= ~(1 << irq);
+				s->isr_ack |= (1 << irq);
 				s->priority_add = (irq + 1) & 7;
 				pic_update_irq(s->pics_state);
 				break;
@@ -300,10 +304,12 @@ static u32 pic_poll_read(struct kvm_kpic_state *s, u32 addr1)
 	if (ret >= 0) {
 		if (addr1 >> 7) {
 			s->pics_state->pics[0].isr &= ~(1 << 2);
+			s->pics_state->pics[0].isr_ack |= (1 << 2);
 			s->pics_state->pics[0].irr &= ~(1 << 2);
 		}
 		s->irr &= ~(1 << ret);
 		s->isr &= ~(1 << ret);
+		s->isr_ack |= (1 << ret);
 		if (addr1 >> 7 || ret != 2)
 			pic_update_irq(s->pics_state);
 	} else {
@@ -422,10 +428,14 @@ static void pic_irq_request(void *opaque, int level)
 {
 	struct kvm *kvm = opaque;
 	struct kvm_vcpu *vcpu = kvm->vcpus[0];
+	struct kvm_pic *s = pic_irqchip(kvm);
+	int irq = pic_get_irq(&s->pics[0]);
 
-	pic_irqchip(kvm)->output = level;
-	if (vcpu)
+	s->output = level;
+	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
+		s->pics[0].isr_ack &= ~(1 << irq);
 		kvm_vcpu_kick(vcpu);
+	}
 }
 
 struct kvm_pic *kvm_create_pic(struct kvm *kvm)
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 07ff2ae..25922ce 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -42,6 +42,7 @@ struct kvm_kpic_state {
 	u8 irr;		/* interrupt request register */
 	u8 imr;		/* interrupt mask register */
 	u8 isr;		/* interrupt service register */
+	u8 isr_ack;
 	u8 priority_add;	/* highest irq priority */
 	u8 irq_base;
 	u8 read_reg_select;

Re: RFC: PIC: enhance IPI avoidance

[Avi Kivity]

Marcelo Tosatti wrote:
> The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
> unnecessary guest exits in some conditions.
>
> For example, if the timer interrupt is routed through the IOAPIC, IRR
> for IRQ 0 will get set but not cleared, since the APIC is handling the
> acks.
>
> This means that everytime an interrupt < 16 is triggered, the priority
> logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
> not masked, which is Linux's case).
>
> Introduce a new variable isr_ack to represent the IRQ's for which the
> guest has been signalled / cleared the ISR. Use it to avoid more than
> one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
> set in get_priority().
>
>   

Looks good.

>  					s->isr &= ~(1 << irq);
> +					s->isr_ack |= (1 << irq);
>   

>  				s->isr &= ~(1 << irq);
> +				s->isr_ack |= (1 << irq);
>   

>  				s->isr &= ~(1 << irq);
> +				s->isr_ack |= (1 << irq);
>   

>  			s->pics_state->pics[0].isr &= ~(1 << 2);
> +			s->pics_state->pics[0].isr_ack |= (1 << 2);
>   

>  		s->isr &= ~(1 << ret);
> +		s->isr_ack |= (1 << ret);
>   

I already have a patch which consolidates isr clearing into a function, 
so I've queued it up and will push once it passes regression testing. 
Please rebase your patch on top of that.

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

Re: RFC: PIC: enhance IPI avoidance

[Avi Kivity]

Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> The PIC code makes little effort to avoid kvm_vcpu_kick(), resulting in
>> unnecessary guest exits in some conditions.
>>
>> For example, if the timer interrupt is routed through the IOAPIC, IRR
>> for IRQ 0 will get set but not cleared, since the APIC is handling the
>> acks.
>>
>> This means that everytime an interrupt < 16 is triggered, the priority
>> logic will find IRQ0 pending and send an IPI to vcpu0 (in case IRQ0 is
>> not masked, which is Linux's case).
>>
>> Introduce a new variable isr_ack to represent the IRQ's for which the
>> guest has been signalled / cleared the ISR. Use it to avoid more than
>> one IPI per trigger-ack cycle, in addition to the avoidance when ISR is
>> set in get_priority().
>>
>
> Looks good.

... but perhaps it can be done less intrusively. Presumably the PIC is 
masked through the local apic LVT? If so we can check that instead.

So when the lapic detects that extint delivery is disabled, it can call 
kvm_pic_disable_delivery(), or something, to reduce pic chatter.

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