[PATCH 0/3] Reset PIT reinjection logic on irq unmask

[PATCH 0/3] Reset PIT reinjection logic on irq unmask

[Avi Kivity]

Currently, when a guest has the pit running but masked, it will accumulate
pending irq injection requests but will never ack them (as the interrupts
are masked).  When the interrupt is finally unmasked, the reinject logic
ignores pending interrupts since it sees unacked pending irqs.

Add a notifier on irq unmasking, and use that in the PIT to clear the
reinject logic.

Currently only ioapic mask notifications are implemented.

This could be used to disable the hrtimer if the guest masks the PIT and
uses some other timer.

Please review.

Avi Kivity (3):
  KVM: Add CONFIG_HAVE_KVM_IRQCHIP
  KVM: Interrupt mask notifiers for ioapic
  KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

 arch/ia64/kvm/Kconfig    |    4 ++++
 arch/powerpc/kvm/Kconfig |    3 +++
 arch/s390/kvm/Kconfig    |    3 +++
 arch/x86/kvm/Kconfig     |    4 ++++
 arch/x86/kvm/i8254.c     |   15 +++++++++++++++
 arch/x86/kvm/i8254.h     |    1 +
 include/linux/kvm_host.h |   17 +++++++++++++++++
 virt/kvm/ioapic.c        |    7 ++++++-
 virt/kvm/irq_comm.c      |   24 ++++++++++++++++++++++++
 virt/kvm/kvm_main.c      |    3 +++
 10 files changed, 80 insertions(+), 1 deletions(-)

[PATCH 1/3] KVM: Add CONFIG_HAVE_KVM_IRQCHIP

[Avi Kivity]

Two KVM archs support irqchips and two don't.  Add a Kconfig item to
make selecting between the two models easier.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/ia64/kvm/Kconfig    |    4 ++++
 arch/powerpc/kvm/Kconfig |    3 +++
 arch/s390/kvm/Kconfig    |    3 +++
 arch/x86/kvm/Kconfig     |    4 ++++
 4 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/kvm/Kconfig b/arch/ia64/kvm/Kconfig
index f833a0b..0a2d6b8 100644
--- a/arch/ia64/kvm/Kconfig
+++ b/arch/ia64/kvm/Kconfig
@@ -4,6 +4,10 @@
 config HAVE_KVM
 	bool
 
+config HAVE_KVM_IRQCHIP
+       bool
+       default y
+
 menuconfig VIRTUALIZATION
 	bool "Virtualization"
 	depends on HAVE_KVM || IA64
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index 1465705..5a152a5 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -2,6 +2,9 @@
 # KVM configuration
 #
 
+config HAVE_KVM_IRQCHIP
+       bool
+
 menuconfig VIRTUALIZATION
 	bool "Virtualization"
 	---help---
diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index e051cad..3e260b7 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -4,6 +4,9 @@
 config HAVE_KVM
        bool
 
+config HAVE_KVM_IRQCHIP
+       bool
+
 menuconfig VIRTUALIZATION
 	bool "Virtualization"
 	default y
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index b81125f..0a303c3 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -4,6 +4,10 @@
 config HAVE_KVM
        bool
 
+config HAVE_KVM_IRQCHIP
+       bool
+       default y
+
 menuconfig VIRTUALIZATION
 	bool "Virtualization"
 	depends on HAVE_KVM || X86
-- 
1.6.0.6

[PATCH 2/3] KVM: Interrupt mask notifiers for ioapic

[Avi Kivity]

Allow clients to request notifications when the guest masks or unmasks a
particular irq line.  This complements irq ack notifications, as the guest
will not ack an irq line that is masked.

Currently implemented for the ioapic only.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 include/linux/kvm_host.h |   17 +++++++++++++++++
 virt/kvm/ioapic.c        |    7 ++++++-
 virt/kvm/irq_comm.c      |   24 ++++++++++++++++++++++++
 virt/kvm/kvm_main.c      |    3 +++
 4 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 545a133..43385fa 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -127,6 +127,10 @@ struct kvm {
 	struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
 #endif
 
+#ifdef CONFIG_HAVE_KVM_IRQCHIP
+	struct hlist_head mask_notifier_list;
+#endif
+
 #ifdef KVM_ARCH_WANT_MMU_NOTIFIER
 	struct mmu_notifier mmu_notifier;
 	unsigned long mmu_notifier_seq;
@@ -320,6 +324,19 @@ struct kvm_assigned_dev_kernel {
 	struct pci_dev *dev;
 	struct kvm *kvm;
 };
+
+struct kvm_irq_mask_notifier {
+	void (*func)(struct kvm_irq_mask_notifier *kimn, int masked);
+	int irq;
+	struct hlist_node link;
+};
+
+void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
+				    struct kvm_irq_mask_notifier *kimn);
+void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
+				      struct kvm_irq_mask_notifier *kimn);
+void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, int mask);
+
 void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 23b81cf..15b9ddd 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -100,7 +100,7 @@ static void ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 {
-	unsigned index;
+	unsigned index, mask_before, mask_after;
 
 	switch (ioapic->ioregsel) {
 	case IOAPIC_REG_VERSION:
@@ -120,6 +120,7 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 		ioapic_debug("change redir index %x val %x\n", index, val);
 		if (index >= IOAPIC_NUM_PINS)
 			return;
+		mask_before = ioapic->redirtbl[index].fields.mask;
 		if (ioapic->ioregsel & 1) {
 			ioapic->redirtbl[index].bits &= 0xffffffff;
 			ioapic->redirtbl[index].bits |= (u64) val << 32;
@@ -128,6 +129,9 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 			ioapic->redirtbl[index].bits |= (u32) val;
 			ioapic->redirtbl[index].fields.remote_irr = 0;
 		}
+		mask_after = ioapic->redirtbl[index].fields.mask;
+		if (mask_before != mask_after)
+			kvm_fire_mask_notifiers(ioapic->kvm, index, mask_after);
 		if (ioapic->irr & (1 << index))
 			ioapic_service(ioapic, index);
 		break;
@@ -426,3 +430,4 @@ int kvm_ioapic_init(struct kvm *kvm)
 	kvm_io_bus_register_dev(&kvm->mmio_bus, &ioapic->dev);
 	return 0;
 }
+
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index aa5d1e5..2cbde68 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -99,3 +99,27 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
 		clear_bit(irq_source_id, &kvm->arch.irq_states[i]);
 	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
 }
+
+void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
+				    struct kvm_irq_mask_notifier *kimn)
+{
+	kimn->irq = irq;
+	hlist_add_head(&kimn->link, &kvm->mask_notifier_list);
+}
+
+void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
+				      struct kvm_irq_mask_notifier *kimn)
+{
+	hlist_del(&kimn->link);
+}
+
+void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, int mask)
+{
+	struct kvm_irq_mask_notifier *kimn;
+	struct hlist_node *n;
+
+	hlist_for_each_entry(kimn, n, &kvm->mask_notifier_list, link)
+		if (kimn->irq == irq)
+			kimn->func(kimn, mask);
+}
+
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5703557..dcd58d6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -806,6 +806,9 @@ static struct kvm *kvm_create_vm(void)
 
 	if (IS_ERR(kvm))
 		goto out;
+#ifdef CONFIG_HAVE_KVM_IRQCHIP
+	INIT_HLIST_HEAD(&kvm->mask_notifier_list);
+#endif
 
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
 	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
-- 
1.6.0.6

Re: [PATCH 2/3] KVM: Interrupt mask notifiers for ioapic

[Sheng Yang]

On Monday 05 January 2009 00:14:44 Avi Kivity wrote:
> Allow clients to request notifications when the guest masks or unmasks a
> particular irq line.  This complements irq ack notifications, as the guest
> will not ack an irq line that is masked.
>
> Currently implemented for the ioapic only.

Hi Avi

Need a lock for this list? Seems kvm_fire_mask_notifiers() implicit holding 
kvm->lock, but register/unregister didn't. And I think we need some comments 
for the necessary of lock.

-- 
regards
Yang, Sheng

>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  include/linux/kvm_host.h |   17 +++++++++++++++++
>  virt/kvm/ioapic.c        |    7 ++++++-
>  virt/kvm/irq_comm.c      |   24 ++++++++++++++++++++++++
>  virt/kvm/kvm_main.c      |    3 +++
>  4 files changed, 50 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 545a133..43385fa 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -127,6 +127,10 @@ struct kvm {
>  	struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
>  #endif
>
> +#ifdef CONFIG_HAVE_KVM_IRQCHIP
> +	struct hlist_head mask_notifier_list;
> +#endif
> +
>  #ifdef KVM_ARCH_WANT_MMU_NOTIFIER
>  	struct mmu_notifier mmu_notifier;
>  	unsigned long mmu_notifier_seq;
> @@ -320,6 +324,19 @@ struct kvm_assigned_dev_kernel {
>  	struct pci_dev *dev;
>  	struct kvm *kvm;
>  };
> +
> +struct kvm_irq_mask_notifier {
> +	void (*func)(struct kvm_irq_mask_notifier *kimn, int masked);
> +	int irq;
> +	struct hlist_node link;
> +};
> +
> +void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
> +				    struct kvm_irq_mask_notifier *kimn);
> +void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
> +				      struct kvm_irq_mask_notifier *kimn);
> +void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, int mask);
> +
>  void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
>  void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
>  void kvm_register_irq_ack_notifier(struct kvm *kvm,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 23b81cf..15b9ddd 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -100,7 +100,7 @@ static void ioapic_service(struct kvm_ioapic *ioapic,
> unsigned int idx)
>
>  static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  {
> -	unsigned index;
> +	unsigned index, mask_before, mask_after;
>
>  	switch (ioapic->ioregsel) {
>  	case IOAPIC_REG_VERSION:
> @@ -120,6 +120,7 @@ static void ioapic_write_indirect(struct kvm_ioapic
> *ioapic, u32 val) ioapic_debug("change redir index %x val %x\n", index,
> val);
>  		if (index >= IOAPIC_NUM_PINS)
>  			return;
> +		mask_before = ioapic->redirtbl[index].fields.mask;
>  		if (ioapic->ioregsel & 1) {
>  			ioapic->redirtbl[index].bits &= 0xffffffff;
>  			ioapic->redirtbl[index].bits |= (u64) val << 32;
> @@ -128,6 +129,9 @@ static void ioapic_write_indirect(struct kvm_ioapic
> *ioapic, u32 val) ioapic->redirtbl[index].bits |= (u32) val;
>  			ioapic->redirtbl[index].fields.remote_irr = 0;
>  		}
> +		mask_after = ioapic->redirtbl[index].fields.mask;
> +		if (mask_before != mask_after)
> +			kvm_fire_mask_notifiers(ioapic->kvm, index, mask_after);
>  		if (ioapic->irr & (1 << index))
>  			ioapic_service(ioapic, index);
>  		break;
> @@ -426,3 +430,4 @@ int kvm_ioapic_init(struct kvm *kvm)
>  	kvm_io_bus_register_dev(&kvm->mmio_bus, &ioapic->dev);
>  	return 0;
>  }
> +
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index aa5d1e5..2cbde68 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -99,3 +99,27 @@ void kvm_free_irq_source_id(struct kvm *kvm, int
> irq_source_id) clear_bit(irq_source_id, &kvm->arch.irq_states[i]);
>  	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
>  }
> +
> +void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
> +				    struct kvm_irq_mask_notifier *kimn)
> +{
> +	kimn->irq = irq;
> +	hlist_add_head(&kimn->link, &kvm->mask_notifier_list);
> +}
> +
> +void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
> +				      struct kvm_irq_mask_notifier *kimn)
> +{
> +	hlist_del(&kimn->link);
> +}
> +
> +void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, int mask)
> +{
> +	struct kvm_irq_mask_notifier *kimn;
> +	struct hlist_node *n;
> +
> +	hlist_for_each_entry(kimn, n, &kvm->mask_notifier_list, link)
> +		if (kimn->irq == irq)
> +			kimn->func(kimn, mask);
> +}
> +
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 5703557..dcd58d6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -806,6 +806,9 @@ static struct kvm *kvm_create_vm(void)
>
>  	if (IS_ERR(kvm))
>  		goto out;
> +#ifdef CONFIG_HAVE_KVM_IRQCHIP
> +	INIT_HLIST_HEAD(&kvm->mask_notifier_list);
> +#endif
>
>  #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
>  	page = alloc_page(GFP_KERNEL | __GFP_ZERO);

Re: [PATCH 2/3] KVM: Interrupt mask notifiers for ioapic

[Avi Kivity]

Sheng Yang wrote:
> On Monday 05 January 2009 00:14:44 Avi Kivity wrote:
>   
>> Allow clients to request notifications when the guest masks or unmasks a
>> particular irq line.  This complements irq ack notifications, as the guest
>> will not ack an irq line that is masked.
>>
>> Currently implemented for the ioapic only.
>>     
>
> Hi Avi
>
> Need a lock for this list? Seems kvm_fire_mask_notifiers() implicit holding 
> kvm->lock, but register/unregister didn't. And I think we need some comments 
> for the necessary of lock.
>   

Good catch.  Everything in struct kvm is protected by kvm->lock (except 
vcpus and the mmu... need a locking document).  I added locking around 
PIT creation so now registration is also protected (it also fixes a 
potential leak if two threads try to create a PIT simultaneously).

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

Re: [PATCH 2/3] KVM: Interrupt mask notifiers for ioapic

[Marcelo Tosatti]

On Sun, Jan 04, 2009 at 06:14:44PM +0200, Avi Kivity wrote:
> Allow clients to request notifications when the guest masks or unmasks a
> particular irq line.  This complements irq ack notifications, as the guest
> will not ack an irq line that is masked.
> 
> Currently implemented for the ioapic only.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  include/linux/kvm_host.h |   17 +++++++++++++++++
>  virt/kvm/ioapic.c        |    7 ++++++-
>  virt/kvm/irq_comm.c      |   24 ++++++++++++++++++++++++
>  virt/kvm/kvm_main.c      |    3 +++
>  4 files changed, 50 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 545a133..43385fa 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -127,6 +127,10 @@ struct kvm {
>  	struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
>  #endif
>  
> +#ifdef CONFIG_HAVE_KVM_IRQCHIP
> +	struct hlist_head mask_notifier_list;
> +#endif
> +
>  #ifdef KVM_ARCH_WANT_MMU_NOTIFIER
>  	struct mmu_notifier mmu_notifier;
>  	unsigned long mmu_notifier_seq;
> @@ -320,6 +324,19 @@ struct kvm_assigned_dev_kernel {
>  	struct pci_dev *dev;
>  	struct kvm *kvm;
>  };
> +
> +struct kvm_irq_mask_notifier {
> +	void (*func)(struct kvm_irq_mask_notifier *kimn, int masked);

bool masked?

> +	int irq;
> +	struct hlist_node link;
> +};
> +
> +void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
> +				    struct kvm_irq_mask_notifier *kimn);
> +void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
> +				      struct kvm_irq_mask_notifier *kimn);
> +void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, int mask);
> +
>  void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
>  void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
>  void kvm_register_irq_ack_notifier(struct kvm *kvm,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 23b81cf..15b9ddd 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -100,7 +100,7 @@ static void ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
>  
>  static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  {
> -	unsigned index;
> +	unsigned index, mask_before, mask_after;
>  
>  	switch (ioapic->ioregsel) {
>  	case IOAPIC_REG_VERSION:
> @@ -120,6 +120,7 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  		ioapic_debug("change redir index %x val %x\n", index, val);
>  		if (index >= IOAPIC_NUM_PINS)
>  			return;
> +		mask_before = ioapic->redirtbl[index].fields.mask;
>  		if (ioapic->ioregsel & 1) {
>  			ioapic->redirtbl[index].bits &= 0xffffffff;
>  			ioapic->redirtbl[index].bits |= (u64) val << 32;
> @@ -128,6 +129,9 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  			ioapic->redirtbl[index].bits |= (u32) val;
>  			ioapic->redirtbl[index].fields.remote_irr = 0;
>  		}
> +		mask_after = ioapic->redirtbl[index].fields.mask;
> +		if (mask_before != mask_after)
> +			kvm_fire_mask_notifiers(ioapic->kvm, index, mask_after);

This can generate "spurious acks", for example:

- inject irq
- guest masks, mask notifier cleans internal state to "acked".
- guest writes EOI

But I suppose thats fine, since PIT (for example) is getting spurious
acks even without mask notifiers and so has to handle them.

Re: [PATCH 2/3] KVM: Interrupt mask notifiers for ioapic

[Avi Kivity]

Marcelo Tosatti wrote:
> On Sun, Jan 04, 2009 at 06:14:44PM +0200, Avi Kivity wrote:
>   
>> Allow clients to request notifications when the guest masks or unmasks a
>> particular irq line.  This complements irq ack notifications, as the guest
>> will not ack an irq line that is masked.
>>
>> Currently implemented for the ioapic only.
>> +
>> +struct kvm_irq_mask_notifier {
>> +	void (*func)(struct kvm_irq_mask_notifier *kimn, int masked);
>>     
>
> bool masked?
>
>   

Hey, I'm the bool fanatic!

>> @@ -128,6 +129,9 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>>  			ioapic->redirtbl[index].bits |= (u32) val;
>>  			ioapic->redirtbl[index].fields.remote_irr = 0;
>>  		}
>> +		mask_after = ioapic->redirtbl[index].fields.mask;
>> +		if (mask_before != mask_after)
>> +			kvm_fire_mask_notifiers(ioapic->kvm, index, mask_after);
>>     
>
> This can generate "spurious acks", for example:
>
> - inject irq
> - guest masks, mask notifier cleans internal state to "acked".
> - guest writes EOI
>
> But I suppose thats fine, since PIT (for example) is getting spurious
> acks even without mask notifiers and so has to handle them.
>   

If the mask happened for a long time then the guest would expect not to 
receive those interrupts.  If it happened due to scheduling then we need 
to reinject them.

I suppose we could measure descheduled time and correct appropriately.  
There previously existed logic that limited the pending count to some 
value (10?) so there wouldn't be a stream of interrupts injected into 
the guest endlessly.

I expect in most cases, masking of the timer interrupt would not happen 
for short periods.  You'd use eflags.if or apic.tpr to mask for short 
periods.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[PATCH 3/3] KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

[Avi Kivity]

While the PIT is masked the guest cannot ack the irq, so the reinject logic
will never allow the interrupt to be injected.

Fix by resetting the reinjection counters on unmask.

Unbreaks Xen.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/i8254.c |   15 +++++++++++++++
 arch/x86/kvm/i8254.h |    1 +
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 528daad..d78d430 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -539,6 +539,16 @@ void kvm_pit_reset(struct kvm_pit *pit)
 	pit->pit_state.irq_ack = 1;
 }
 
+static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, int mask)
+{
+	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
+
+	if (!mask) {
+		atomic_set(&pit->pit_state.pit_timer.pending, 0);
+		pit->pit_state.irq_ack = 1;
+	}
+}
+
 struct kvm_pit *kvm_create_pit(struct kvm *kvm)
 {
 	struct kvm_pit *pit;
@@ -588,6 +598,9 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm)
 
 	kvm_pit_reset(pit);
 
+	pit->mask_notifier.func = pit_mask_notifer;
+	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
+
 	return pit;
 }
 
@@ -596,6 +609,8 @@ void kvm_free_pit(struct kvm *kvm)
 	struct hrtimer *timer;
 
 	if (kvm->arch.vpit) {
+		kvm_unregister_irq_mask_notifier(kvm, 0,
+					       &kvm->arch.vpit->mask_notifier);
 		mutex_lock(&kvm->arch.vpit->pit_state.lock);
 		timer = &kvm->arch.vpit->pit_state.pit_timer.timer;
 		hrtimer_cancel(timer);
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 76959c4..6acbe4b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -46,6 +46,7 @@ struct kvm_pit {
 	struct kvm *kvm;
 	struct kvm_kpit_state pit_state;
 	int irq_source_id;
+	struct kvm_irq_mask_notifier mask_notifier;
 };
 
 #define KVM_PIT_BASE_ADDRESS	    0x40
-- 
1.6.0.6

Re: [PATCH 3/3] KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

[Marcelo Tosatti]

On Sun, Jan 04, 2009 at 06:14:45PM +0200, Avi Kivity wrote:
> While the PIT is masked the guest cannot ack the irq, so the reinject logic
> will never allow the interrupt to be injected.
> 
> Fix by resetting the reinjection counters on unmask.
> 
> Unbreaks Xen.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/i8254.c |   15 +++++++++++++++
>  arch/x86/kvm/i8254.h |    1 +
>  2 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 528daad..d78d430 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -539,6 +539,16 @@ void kvm_pit_reset(struct kvm_pit *pit)
>  	pit->pit_state.irq_ack = 1;
>  }
>  
> +static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, int mask)
> +{
> +	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
> +
> +	if (!mask) {
> +		atomic_set(&pit->pit_state.pit_timer.pending, 0);
> +		pit->pit_state.irq_ack = 1;
> +	}
> +}

I'm not sure about zeroing the counter here. The guest can mask the
interrupt during normal operation, and in such cases you want the
pending count to be retained (and reinjected later).

I suppose setting irq_ack to one is enough.

Re: [PATCH 3/3] KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

[Avi Kivity]

Marcelo Tosatti wrote:
> On Sun, Jan 04, 2009 at 06:14:45PM +0200, Avi Kivity wrote:
>   
>> While the PIT is masked the guest cannot ack the irq, so the reinject logic
>> will never allow the interrupt to be injected.
>>
>> Fix by resetting the reinjection counters on unmask.
>>
>> Unbreaks Xen.
>>
>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>> index 528daad..d78d430 100644
>> --- a/arch/x86/kvm/i8254.c
>> +++ b/arch/x86/kvm/i8254.c
>> @@ -539,6 +539,16 @@ void kvm_pit_reset(struct kvm_pit *pit)
>>  	pit->pit_state.irq_ack = 1;
>>  }
>>  
>> +static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, int mask)
>> +{
>> +	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
>> +
>> +	if (!mask) {
>> +		atomic_set(&pit->pit_state.pit_timer.pending, 0);
>> +		pit->pit_state.irq_ack = 1;
>> +	}
>> +}
>>     
>
> I'm not sure about zeroing the counter here. The guest can mask the
> interrupt during normal operation, and in such cases you want the
> pending count to be retained (and reinjected later).
>
> I suppose setting irq_ack to one is enough.
>   

I'm worried about:

- boot guest using local apic timer
- reset
- boot with pit timer
- a zillion interrupts

So at the very least, we need a limiter.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 3/3] KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

[Marcelo Tosatti]

On Mon, Jan 05, 2009 at 10:59:01PM +0200, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> On Sun, Jan 04, 2009 at 06:14:45PM +0200, Avi Kivity wrote:
>>   
>>> While the PIT is masked the guest cannot ack the irq, so the reinject logic
>>> will never allow the interrupt to be injected.
>>>
>>> Fix by resetting the reinjection counters on unmask.
>>>
>>> Unbreaks Xen.
>>>
>>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>>> index 528daad..d78d430 100644
>>> --- a/arch/x86/kvm/i8254.c
>>> +++ b/arch/x86/kvm/i8254.c
>>> @@ -539,6 +539,16 @@ void kvm_pit_reset(struct kvm_pit *pit)
>>>  	pit->pit_state.irq_ack = 1;
>>>  }
>>>  +static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, 
>>> int mask)
>>> +{
>>> +	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
>>> +
>>> +	if (!mask) {
>>> +		atomic_set(&pit->pit_state.pit_timer.pending, 0);
>>> +		pit->pit_state.irq_ack = 1;
>>> +	}
>>> +}
>>>     
>>
>> I'm not sure about zeroing the counter here. The guest can mask the
>> interrupt during normal operation, and in such cases you want the
>> pending count to be retained (and reinjected later).
>>
>> I suppose setting irq_ack to one is enough.
>>   
>
> I'm worried about:
>
> - boot guest using local apic timer
> - reset
> - boot with pit timer
> - a zillion interrupts
>
> So at the very least, we need a limiter.

Or have a new notifier on kvm_pic_reset, instead of simply acking one
pending irq? That seems the appropriate place to zero the counter.

Re: [PATCH 3/3] KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

[Avi Kivity]

Marcelo Tosatti wrote:
>>
>> I'm worried about:
>>
>> - boot guest using local apic timer
>> - reset
>> - boot with pit timer
>> - a zillion interrupts
>>
>> So at the very least, we need a limiter.
>>     
>
> Or have a new notifier on kvm_pic_reset, instead of simply acking one
> pending irq? That seems the appropriate place to zero the counter.
>   

Clearing the counter on reset is good, but it doesn't solve the 
underlying problem, which is that there are two separate cases that 
appear to the host as the same thing:

- guest masks irqs, does a lot of work, unmasks irqs
- host deschedules guest, does a lot of work, reschedules guest

Right now we assume any missed interrupts are due to host load.  In the 
reboot case, that's clearly wrong, but that is only an example.  Maybe 
we can use preempt notifiers to detect whether the timer tick happened 
while the guest was scheduled or not.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 3/3] KVM: Reset PIT irq injection logic when the PIT IRQ is unmasked

[Dor Laor]

Avi Kivity wrote:
> Marcelo Tosatti wrote:
>>>
>>> I'm worried about:
>>>
>>> - boot guest using local apic timer
>>> - reset
>>> - boot with pit timer
>>> - a zillion interrupts
>>>
>>> So at the very least, we need a limiter.
>>>     
>>
>> Or have a new notifier on kvm_pic_reset, instead of simply acking one
>> pending irq? That seems the appropriate place to zero the counter.
>>   
>
> Clearing the counter on reset is good, but it doesn't solve the 
> underlying problem, which is that there are two separate cases that 
> appear to the host as the same thing:
>
> - guest masks irqs, does a lot of work, unmasks irqs
> - host deschedules guest, does a lot of work, reschedules guest
>
> Right now we assume any missed interrupts are due to host load.  In 
> the reboot case, that's clearly wrong, but that is only an example.  
> Maybe we can use preempt notifiers to detect whether the timer tick 
> happened while the guest was scheduled or not.
>
It might get too complex. It can be done inside the vcpu_run function too:
An irq needs reinjection if the irq window was not  open  from the  
timer  tick till  the next  timer tick
minus the deschedule time. You also need to know on the right vcpu that 
the pit irq it routed to.

Since scenarios like guests masking their pit and do a lot of work are 
rare and a bad guest behaviour anyway,
I don't think we should special case them. So the pit reset hook is enough.