* KVM: fix i8259 reset irq acking
@ 2008-07-31 17:15 Marcelo Tosatti
2008-08-13 10:40 ` Avi Kivity
0 siblings, 1 reply; 6+ messages in thread
From: Marcelo Tosatti @ 2008-07-31 17:15 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm-devel
The irq ack during pic reset has three problems:
- Ignores slave/master PIC, using gsi 0-8 for both.
- Generates an ACK even if the APIC is in control.
- Depends upon IMR being clear, which is broken if the
irq was masked at the time it was generated.
The last one causes the BIOS to hang after the first reboot of Windows
installation, since PIT interrupts stop.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index de70499..2b42d48 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
void kvm_pic_reset(struct kvm_kpic_state *s)
{
- int irq;
+ int irq, irqbase;
struct kvm *kvm = s->pics_state->irq_request_opaque;
- for (irq = 0; irq < PIC_NUM_PINS; irq++) {
- if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
- s->isr & (1 << irq)))
- kvm_notify_acked_irq(kvm, irq);
- }
+ if (s == &s->pics_state->pics[0])
+ irqbase = 0;
+ else
+ irqbase = 8;
+
+ if (!kvm_lapic_enabled(kvm->vcpus[0]))
+ for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
+ if (s->irr & (1 << irq) || s->isr & (1 << irq))
+ kvm_notify_acked_irq(kvm, irq+irqbase);
s->last_irr = 0;
s->irr = 0;
s->imr = 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: KVM: fix i8259 reset irq acking
2008-07-31 17:15 KVM: fix i8259 reset irq acking Marcelo Tosatti
@ 2008-08-13 10:40 ` Avi Kivity
2008-08-14 23:53 ` Marcelo Tosatti
0 siblings, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2008-08-13 10:40 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
Marcelo Tosatti wrote:
> The irq ack during pic reset has three problems:
>
> - Ignores slave/master PIC, using gsi 0-8 for both.
> - Generates an ACK even if the APIC is in control.
> - Depends upon IMR being clear, which is broken if the
> irq was masked at the time it was generated.
>
> The last one causes the BIOS to hang after the first reboot of Windows
> installation, since PIT interrupts stop.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index de70499..2b42d48 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
>
> void kvm_pic_reset(struct kvm_kpic_state *s)
> {
> - int irq;
> + int irq, irqbase;
> struct kvm *kvm = s->pics_state->irq_request_opaque;
>
> - for (irq = 0; irq < PIC_NUM_PINS; irq++) {
> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
> - s->isr & (1 << irq)))
> - kvm_notify_acked_irq(kvm, irq);
> - }
> + if (s == &s->pics_state->pics[0])
> + irqbase = 0;
> + else
> + irqbase = 8;
> +
> + if (!kvm_lapic_enabled(kvm->vcpus[0]))
>
What if the lapic is enabled and passes through PIC interrupts in EXTINT
mode?
btw, this will oops if we have no vcpu 0.
> + for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
> + if (s->irr & (1 << irq) || s->isr & (1 << irq))
> + kvm_notify_acked_irq(kvm, irq+irqbase);
>
Yeah.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: KVM: fix i8259 reset irq acking
2008-08-13 10:40 ` Avi Kivity
@ 2008-08-14 23:53 ` Marcelo Tosatti
2008-08-17 6:22 ` Avi Kivity
0 siblings, 1 reply; 6+ messages in thread
From: Marcelo Tosatti @ 2008-08-14 23:53 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm-devel
On Wed, Aug 13, 2008 at 01:40:34PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> The irq ack during pic reset has three problems:
>>
>> - Ignores slave/master PIC, using gsi 0-8 for both.
>> - Generates an ACK even if the APIC is in control.
>> - Depends upon IMR being clear, which is broken if the irq was masked
>> at the time it was generated.
>>
>> The last one causes the BIOS to hang after the first reboot of Windows
>> installation, since PIT interrupts stop.
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>>
>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>> index de70499..2b42d48 100644
>> --- a/arch/x86/kvm/i8259.c
>> +++ b/arch/x86/kvm/i8259.c
>> @@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
>> void kvm_pic_reset(struct kvm_kpic_state *s)
>> {
>> - int irq;
>> + int irq, irqbase;
>> struct kvm *kvm = s->pics_state->irq_request_opaque;
>> - for (irq = 0; irq < PIC_NUM_PINS; irq++) {
>> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>> - s->isr & (1 << irq)))
>> - kvm_notify_acked_irq(kvm, irq);
>> - }
>> + if (s == &s->pics_state->pics[0])
>> + irqbase = 0;
>> + else
>> + irqbase = 8;
>> +
>> + if (!kvm_lapic_enabled(kvm->vcpus[0]))
>>
>
> What if the lapic is enabled and passes through PIC interrupts in EXTINT
> mode?
>
> btw, this will oops if we have no vcpu 0.
True. How's this:
The irq ack during pic reset has three problems:
- Ignores slave/master PIC, using gsi 0-8 for both.
- Generates an ACK even if the APIC is in control.
- Depends upon IMR being clear, which is broken if the irq was masked
at the time it was generated.
The last one causes the BIOS to hang after the first reboot of
Windows
installation, since PIT interrupts stop.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index de70499..59a2201 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -195,13 +195,23 @@ int kvm_pic_read_irq(struct kvm *kvm)
void kvm_pic_reset(struct kvm_kpic_state *s)
{
- int irq;
+ int irq, irqbase;
struct kvm *kvm = s->pics_state->irq_request_opaque;
+ struct kvm_vcpu *vcpu0 = kvm->vcpus[0];
+
+ if (s == &s->pics_state->pics[0])
+ irqbase = 0;
+ else
+ irqbase = 8;
for (irq = 0; irq < PIC_NUM_PINS; irq++) {
- if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
- s->isr & (1 << irq)))
- kvm_notify_acked_irq(kvm, irq);
+ union ioapic_redir_entry entry;
+ entry = kvm->arch.vioapic->redirtbl[irq+irqbase];
+ if ((vcpu0 && !kvm_lapic_enabled(vcpu0)) ||
+ entry.fields.delivery_mode == IOAPIC_EXTINT) {
+ if (s->irr & (1 << irq) || s->isr & (1 << irq))
+ kvm_notify_acked_irq(kvm, irq+irqbase);
+ }
}
s->last_irr = 0;
s->irr = 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: KVM: fix i8259 reset irq acking
2008-08-14 23:53 ` Marcelo Tosatti
@ 2008-08-17 6:22 ` Avi Kivity
2008-08-17 7:42 ` Avi Kivity
0 siblings, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2008-08-17 6:22 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
Marcelo Tosatti wrote:
> On Wed, Aug 13, 2008 at 01:40:34PM +0300, Avi Kivity wrote:
>
>> Marcelo Tosatti wrote:
>>
>>> The irq ack during pic reset has three problems:
>>>
>>> - Ignores slave/master PIC, using gsi 0-8 for both.
>>> - Generates an ACK even if the APIC is in control.
>>> - Depends upon IMR being clear, which is broken if the irq was masked
>>> at the time it was generated.
>>>
>>> The last one causes the BIOS to hang after the first reboot of Windows
>>> installation, since PIT interrupts stop.
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>
>>>
>>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>>> index de70499..2b42d48 100644
>>> --- a/arch/x86/kvm/i8259.c
>>> +++ b/arch/x86/kvm/i8259.c
>>> @@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
>>> void kvm_pic_reset(struct kvm_kpic_state *s)
>>> {
>>> - int irq;
>>> + int irq, irqbase;
>>> struct kvm *kvm = s->pics_state->irq_request_opaque;
>>> - for (irq = 0; irq < PIC_NUM_PINS; irq++) {
>>> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>>> - s->isr & (1 << irq)))
>>> - kvm_notify_acked_irq(kvm, irq);
>>> - }
>>> + if (s == &s->pics_state->pics[0])
>>> + irqbase = 0;
>>> + else
>>> + irqbase = 8;
>>> +
>>> + if (!kvm_lapic_enabled(kvm->vcpus[0]))
>>>
>>>
>> What if the lapic is enabled and passes through PIC interrupts in EXTINT
>> mode?
>>
>> btw, this will oops if we have no vcpu 0.
>>
>
> True. How's this:
> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
> - s->isr & (1 << irq)))
> - kvm_notify_acked_irq(kvm, irq);
> + union ioapic_redir_entry entry;
> + entry = kvm->arch.vioapic->redirtbl[irq+irqbase];
> + if ((vcpu0 && !kvm_lapic_enabled(vcpu0)) ||
> + entry.fields.delivery_mode == IOAPIC_EXTINT) {
> + if (s->irr & (1 << irq) || s->isr & (1 << irq))
> + kvm_notify_acked_irq(kvm, irq+irqbase);
> + }
>
Why look at the ioapic? You need to check lapic lvt0 delivery mode for
extint, not the ioapic.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: KVM: fix i8259 reset irq acking
2008-08-17 6:22 ` Avi Kivity
@ 2008-08-17 7:42 ` Avi Kivity
2008-08-17 14:36 ` Avi Kivity
0 siblings, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2008-08-17 7:42 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> On Wed, Aug 13, 2008 at 01:40:34PM +0300, Avi Kivity wrote:
>>
>>> Marcelo Tosatti wrote:
>>>
>>>> The irq ack during pic reset has three problems:
>>>>
>>>> - Ignores slave/master PIC, using gsi 0-8 for both.
>>>> - Generates an ACK even if the APIC is in control.
>>>> - Depends upon IMR being clear, which is broken if the irq was
>>>> masked at the time it was generated.
>>>>
>>>> The last one causes the BIOS to hang after the first reboot of Windows
>>>> installation, since PIT interrupts stop.
>>>>
>>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>
>>>>
>>>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>>>> index de70499..2b42d48 100644
>>>> --- a/arch/x86/kvm/i8259.c
>>>> +++ b/arch/x86/kvm/i8259.c
>>>> @@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
>>>> void kvm_pic_reset(struct kvm_kpic_state *s)
>>>> {
>>>> - int irq;
>>>> + int irq, irqbase;
>>>> struct kvm *kvm = s->pics_state->irq_request_opaque;
>>>> - for (irq = 0; irq < PIC_NUM_PINS; irq++) {
>>>> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>>>> - s->isr & (1 << irq)))
>>>> - kvm_notify_acked_irq(kvm, irq);
>>>> - }
>>>> + if (s == &s->pics_state->pics[0])
>>>> + irqbase = 0;
>>>> + else
>>>> + irqbase = 8;
>>>> +
>>>> + if (!kvm_lapic_enabled(kvm->vcpus[0]))
>>>>
>>> What if the lapic is enabled and passes through PIC interrupts in
>>> EXTINT mode?
>>>
>>> btw, this will oops if we have no vcpu 0.
>>>
>>
>> True. How's this:
>> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>> - s->isr & (1 << irq)))
>> - kvm_notify_acked_irq(kvm, irq);
>> + union ioapic_redir_entry entry;
>> + entry = kvm->arch.vioapic->redirtbl[irq+irqbase];
>> + if ((vcpu0 && !kvm_lapic_enabled(vcpu0)) ||
>> + entry.fields.delivery_mode == IOAPIC_EXTINT) {
>> + if (s->irr & (1 << irq) || s->isr & (1 << irq))
>> + kvm_notify_acked_irq(kvm, irq+irqbase);
>> + }
>>
>
> Why look at the ioapic? You need to check lapic lvt0 delivery mode for
> extint, not the ioapic.
>
I changed it to use kvm_apic_accept_pic_intr() and applied.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: KVM: fix i8259 reset irq acking
2008-08-17 7:42 ` Avi Kivity
@ 2008-08-17 14:36 ` Avi Kivity
0 siblings, 0 replies; 6+ messages in thread
From: Avi Kivity @ 2008-08-17 14:36 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
Avi Kivity wrote:
> Avi Kivity wrote:
>> Marcelo Tosatti wrote:
>>> On Wed, Aug 13, 2008 at 01:40:34PM +0300, Avi Kivity wrote:
>>>
>>>> Marcelo Tosatti wrote:
>>>>
>>>>> The irq ack during pic reset has three problems:
>>>>>
>>>>> - Ignores slave/master PIC, using gsi 0-8 for both.
>>>>> - Generates an ACK even if the APIC is in control.
>>>>> - Depends upon IMR being clear, which is broken if the irq was
>>>>> masked at the time it was generated.
>>>>>
>>>>> The last one causes the BIOS to hang after the first reboot of
>>>>> Windows
>>>>> installation, since PIT interrupts stop.
>>>>>
>>>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>>
>>>>>
>>>>> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
>>>>> index de70499..2b42d48 100644
>>>>> --- a/arch/x86/kvm/i8259.c
>>>>> +++ b/arch/x86/kvm/i8259.c
>>>>> @@ -195,14 +195,18 @@ int kvm_pic_read_irq(struct kvm *kvm)
>>>>> void kvm_pic_reset(struct kvm_kpic_state *s)
>>>>> {
>>>>> - int irq;
>>>>> + int irq, irqbase;
>>>>> struct kvm *kvm = s->pics_state->irq_request_opaque;
>>>>> - for (irq = 0; irq < PIC_NUM_PINS; irq++) {
>>>>> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>>>>> - s->isr & (1 << irq)))
>>>>> - kvm_notify_acked_irq(kvm, irq);
>>>>> - }
>>>>> + if (s == &s->pics_state->pics[0])
>>>>> + irqbase = 0;
>>>>> + else
>>>>> + irqbase = 8;
>>>>> +
>>>>> + if (!kvm_lapic_enabled(kvm->vcpus[0]))
>>>>>
>>>> What if the lapic is enabled and passes through PIC interrupts in
>>>> EXTINT mode?
>>>>
>>>> btw, this will oops if we have no vcpu 0.
>>>>
>>>
>>> True. How's this:
>>> - if (!(s->imr & (1 << irq)) && (s->irr & (1 << irq) ||
>>> - s->isr & (1 << irq)))
>>> - kvm_notify_acked_irq(kvm, irq);
>>> + union ioapic_redir_entry entry;
>>> + entry = kvm->arch.vioapic->redirtbl[irq+irqbase];
>>> + if ((vcpu0 && !kvm_lapic_enabled(vcpu0)) ||
>>> + entry.fields.delivery_mode == IOAPIC_EXTINT) {
>>> + if (s->irr & (1 << irq) || s->isr & (1 << irq))
>>> + kvm_notify_acked_irq(kvm, irq+irqbase);
>>> + }
>>>
>>
>> Why look at the ioapic? You need to check lapic lvt0 delivery mode
>> for extint, not the ioapic.
>>
>
> I changed it to use kvm_apic_accept_pic_intr() and applied.
>
>
It still triggered the warning when the pit pending interrupt count
dropped below zero (booting FC6-i386). I "fixed" it by making the pit
tolerate this condition, but there may be a bigger problem lurking in there.
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-08-17 14:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-31 17:15 KVM: fix i8259 reset irq acking Marcelo Tosatti
2008-08-13 10:40 ` Avi Kivity
2008-08-14 23:53 ` Marcelo Tosatti
2008-08-17 6:22 ` Avi Kivity
2008-08-17 7:42 ` Avi Kivity
2008-08-17 14:36 ` Avi Kivity
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox