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

2009-01-04 Thread 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 
---
 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

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

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

2009-01-05 Thread 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 
> ---
>  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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2009-01-05 Thread 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.

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

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

2009-01-05 Thread 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.

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

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

2009-01-06 Thread 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.

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

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

2009-01-06 Thread 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.

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