Patchwork [v4,5/5] i8259: fix dynamically masking slave IRQs with IMR register

login
register
mail settings
Submitter Paolo Bonzini
Date Sept. 4, 2012, 2:42 p.m.
Message ID <5046135B.2080200@redhat.com>
Download mbox | patch
Permalink /patch/181598/
State New
Headers show

Comments

Paolo Bonzini - Sept. 4, 2012, 2:42 p.m.
Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
>  So first of all, the *output* of the 8259A is always edge triggered, 
> regardless of whether it's the master or one of the slaves (only one slave 
> is used in the PC/AT architecture, but up to eight are supported; the 
> PC/XT had none).  

I swear I read all your message :) but this seems to be the crux.  It means
that something like this ought to fix the bug too.  Matthew, can you post
your code or test it?


Both patches untested.

Paolo
Jan Kiszka - Sept. 4, 2012, 4:01 p.m.
On 2012-09-04 16:42, Paolo Bonzini wrote:
> Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
>>  So first of all, the *output* of the 8259A is always edge triggered, 
>> regardless of whether it's the master or one of the slaves (only one slave 
>> is used in the PC/AT architecture, but up to eight are supported; the 
>> PC/XT had none).  
> 
> I swear I read all your message :) but this seems to be the crux.  It means
> that something like this ought to fix the bug too.  Matthew, can you post
> your code or test it?
> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 53daf78..3dc1dff 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
>      int irq;
>  
>      irq = pic_get_irq(s);
> +    qemu_irq_lower(s->int_out[0]);
>      if (irq >= 0) {
>          DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
>                  s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
>          qemu_irq_raise(s->int_out[0]);
> -    } else {
> -        qemu_irq_lower(s->int_out[0]);
>      }
>  }

I don't think this can be correct in all scenario. E.g., we also call
pic_update_irq in case a level-triggered input was updated but didn't
change the output state of the PIC (high-high transition). With your
patch, the output will now generate edges.

What I'm trying to understand and translate from the description is
rather "note that for inputs a high-to-low transition cancels the
interrupt as in the level-triggered mode." This is surely not what we do
right now. OTOH, I'm afraid that switching to this mode in the PIC can
cause problems elsewhere, with devices that actually inject short
low-high-low signals. Still wrapping my head around it...

Jan
Maciej W. Rozycki - Sept. 4, 2012, 5:41 p.m.
On Tue, 4 Sep 2012, Jan Kiszka wrote:

> What I'm trying to understand and translate from the description is
> rather "note that for inputs a high-to-low transition cancels the
> interrupt as in the level-triggered mode." This is surely not what we do
> right now. OTOH, I'm afraid that switching to this mode in the PIC can
> cause problems elsewhere, with devices that actually inject short
> low-high-low signals. Still wrapping my head around it...

 That won't work reliably with true 8259A hardware -- for an 
edge-triggered interrupt to propagate up to the CPU first there must be a 
low-to-high transition and then the high logic state must be maintained up 
until the start of the second INTA cycle.  If the interrupt request drops 
before then (e.g. because CPU interrupts have been masked or a 
higher-priority 8259A has been serviced), then the corresponding IRR bit 
is cleared and either the interrupt is missed altogether or, if the CPU 
has already accepted the interrupt and started the first INTA cycle, then 
the spurious vector is supplied and no ISR bit is set.

 To put it in different words: the only actual difference between 
edge-triggered and level-triggered interrupts in the 8259A is that the 
formers require a leading edge to record another interrupt.  For both 
trigger modes the high level has to be maintained until the second INTA 
cycle for the interrupt to be correctly delivered to the CPU and also in 
both trigger modes a trailing edge cancels the interrupt.

 This is unlike the traditional edge-triggered mode where the level does 
not have to be maintained once a leading edge has been correctly recorded 
(there is usually spike filtering logic implemented on such IRQ inputs so 
appropriate timings have to be met; because of its unusual interpretation 
the 8259A obviously does not require such logic).

 The edge detector logic is also drawn in the 8259A datasheet (that for a 
change used to be available from one of the Intel sites in the PDF form) 
and I believe the functionality described can be inferred from that by the 
curious enough. ;)

  Maciej
Jan Kiszka - Sept. 4, 2012, 5:55 p.m.
On 2012-09-04 19:41, Maciej W. Rozycki wrote:
> On Tue, 4 Sep 2012, Jan Kiszka wrote:
> 
>> What I'm trying to understand and translate from the description is
>> rather "note that for inputs a high-to-low transition cancels the
>> interrupt as in the level-triggered mode." This is surely not what we do
>> right now. OTOH, I'm afraid that switching to this mode in the PIC can
>> cause problems elsewhere, with devices that actually inject short
>> low-high-low signals. Still wrapping my head around it...
> 
>  That won't work reliably with true 8259A hardware -- for an 

Ok, then we have to scan our code base for such device models that won't
survive with real 8259A hardware. That can only be devices attached to
edge-only inputs of the PIC, namely the PIT, the keyboard controller,
the RTC and FPU emulation. They basically need to generate high-low-high
transitions on new events, instead of low-high-low (via qemu_irq_pulse
e.g.). I'm I on the right track?

Thanks,
Jan

> edge-triggered interrupt to propagate up to the CPU first there must be a 
> low-to-high transition and then the high logic state must be maintained up 
> until the start of the second INTA cycle.  If the interrupt request drops 
> before then (e.g. because CPU interrupts have been masked or a 
> higher-priority 8259A has been serviced), then the corresponding IRR bit 
> is cleared and either the interrupt is missed altogether or, if the CPU 
> has already accepted the interrupt and started the first INTA cycle, then 
> the spurious vector is supplied and no ISR bit is set.
> 
>  To put it in different words: the only actual difference between 
> edge-triggered and level-triggered interrupts in the 8259A is that the 
> formers require a leading edge to record another interrupt.  For both 
> trigger modes the high level has to be maintained until the second INTA 
> cycle for the interrupt to be correctly delivered to the CPU and also in 
> both trigger modes a trailing edge cancels the interrupt.
> 
>  This is unlike the traditional edge-triggered mode where the level does 
> not have to be maintained once a leading edge has been correctly recorded 
> (there is usually spike filtering logic implemented on such IRQ inputs so 
> appropriate timings have to be met; because of its unusual interpretation 
> the 8259A obviously does not require such logic).
> 
>  The edge detector logic is also drawn in the 8259A datasheet (that for a 
> change used to be available from one of the Intel sites in the PDF form) 
> and I believe the functionality described can be inferred from that by the 
> curious enough. ;)
> 
>   Maciej
>
Maciej W. Rozycki - Sept. 4, 2012, 6:27 p.m.
On Tue, 4 Sep 2012, Jan Kiszka wrote:

> >> What I'm trying to understand and translate from the description is
> >> rather "note that for inputs a high-to-low transition cancels the
> >> interrupt as in the level-triggered mode." This is surely not what we do
> >> right now. OTOH, I'm afraid that switching to this mode in the PIC can
> >> cause problems elsewhere, with devices that actually inject short
> >> low-high-low signals. Still wrapping my head around it...
> > 
> >  That won't work reliably with true 8259A hardware -- for an 
> 
> Ok, then we have to scan our code base for such device models that won't
> survive with real 8259A hardware. That can only be devices attached to
> edge-only inputs of the PIC, namely the PIT, the keyboard controller,
> the RTC and FPU emulation. They basically need to generate high-low-high
> transitions on new events, instead of low-high-low (via qemu_irq_pulse
> e.g.). I'm I on the right track?

 They shouldn't be the problem unless we've got an implementation problem:

 * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT 
   architecture.  In the former its output is high (active) all the time 
   except from one (last) clock cycle.  In the latter a wave that has a 
   duty cycle close or equal to 0.5 (depending on whether the divider is 
   odd or even) is produced, so no short pulses either.  I don't remember 
   the other four modes -- have a look at the datasheet if interested, but 
   I reckon they're not really compatible with the wiring anyway, e.g. the 
   gate is hardwired enabled.

 * The 8042 keyboard controller requires an interrupt acknowledge (by 
   reading its data port, normally at the address 0x60 in the PC/AT port 
   I/O space) to clear its output, so no concern here either (this is BTW 
   how many years ago I actually learnt the hard way how the 
   edge-triggered mode works in the 8259A -- reading the KBC's data port 
   in the main program a tight loop with the keyboard interrupt enabled 
   will lead to a spurious interrupt each time data is supplied).  I 
   believe it's otherwise edge-triggered though (I don't have an 8042 
   datasheet handy, I would have to dig for a hardcopy that I left 
   abroad).

 * The MC146818 RTC interrupt is level triggered and has to be 
   acknowledged by reading Register C.

 * The i287/i387 FPU interrupt (in the PC/AT compatibility mode) is 
   latched and requires the location at the address 0xf0 in the PC/AT port 
   I/O space to be poked at to acknowledge.  It can therefore be 
   considered level-triggered (overall the design is busted and you have 
   to be very careful not to freeze the system when handling the FPU error 
   this way rather than via the FPU exception).

 Did I miss anything?

  Maciej
Jan Kiszka - Sept. 4, 2012, 6:39 p.m.
On 2012-09-04 20:27, Maciej W. Rozycki wrote:
> On Tue, 4 Sep 2012, Jan Kiszka wrote:
> 
>>>> What I'm trying to understand and translate from the description is
>>>> rather "note that for inputs a high-to-low transition cancels the
>>>> interrupt as in the level-triggered mode." This is surely not what we do
>>>> right now. OTOH, I'm afraid that switching to this mode in the PIC can
>>>> cause problems elsewhere, with devices that actually inject short
>>>> low-high-low signals. Still wrapping my head around it...
>>>
>>>  That won't work reliably with true 8259A hardware -- for an 
>>
>> Ok, then we have to scan our code base for such device models that won't
>> survive with real 8259A hardware. That can only be devices attached to
>> edge-only inputs of the PIC, namely the PIT, the keyboard controller,
>> the RTC and FPU emulation. They basically need to generate high-low-high
>> transitions on new events, instead of low-high-low (via qemu_irq_pulse
>> e.g.). I'm I on the right track?
> 
>  They shouldn't be the problem unless we've got an implementation problem:
> 
>  * The 8254 PIT is normally configured in mode 2 or 3 in the PC/AT 
>    architecture.  In the former its output is high (active) all the time 
>    except from one (last) clock cycle.  In the latter a wave that has a 
>    duty cycle close or equal to 0.5 (depending on whether the divider is 
>    odd or even) is produced, so no short pulses either.  I don't remember 
>    the other four modes -- have a look at the datasheet if interested, but 
>    I reckon they're not really compatible with the wiring anyway, e.g. the 
>    gate is hardwired enabled.
> 
>  * The 8042 keyboard controller requires an interrupt acknowledge (by 
>    reading its data port, normally at the address 0x60 in the PC/AT port 
>    I/O space) to clear its output, so no concern here either (this is BTW 
>    how many years ago I actually learnt the hard way how the 
>    edge-triggered mode works in the 8259A -- reading the KBC's data port 
>    in the main program a tight loop with the keyboard interrupt enabled 
>    will lead to a spurious interrupt each time data is supplied).  I 
>    believe it's otherwise edge-triggered though (I don't have an 8042 
>    datasheet handy, I would have to dig for a hardcopy that I left 
>    abroad).
> 
>  * The MC146818 RTC interrupt is level triggered and has to be 
>    acknowledged by reading Register C.
> 
>  * The i287/i387 FPU interrupt (in the PC/AT compatibility mode) is 
>    latched and requires the location at the address 0xf0 in the PC/AT port 
>    I/O space to be poked at to acknowledge.  It can therefore be 
>    considered level-triggered (overall the design is busted and you have 
>    to be very careful not to freeze the system when handling the FPU error 
>    this way rather than via the FPU exception).
> 
>  Did I miss anything?

Nope, and it looks like I was too pessimistic. I've checked PIT, 8042,
and RTC, and they all set their output levels properly. So it should be
safe to start clearing an IRQ on falling edges at a PIC input.

Jan

Patch

diff --git a/hw/i8259.c b/hw/i8259.c
index 53daf78..3dc1dff 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -104,12 +104,11 @@  static void pic_update_irq(PICCommonState *s)
     int irq;
 
     irq = pic_get_irq(s);
+    qemu_irq_lower(s->int_out[0]);
     if (irq >= 0) {
         DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
                 s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
         qemu_irq_raise(s->int_out[0]);
-    } else {
-        qemu_irq_lower(s->int_out[0]);
     }
 }
 
The logic of the in-kernel 8259 is a bit different, but something
like this should do it, too:

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 81cf4fa..feb6d5b 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -174,9 +174,11 @@  static void pic_update_irq(struct kvm_pic *s)
 		/*
 		 * if irq request by slave pic, signal master PIC
 		 */
+		pic_set_irq1(&s->pics[0], 2, 0);
 		pic_set_irq1(&s->pics[0], 2, 1);
+	} else if (s->pics[0].irr & (1 << 2))
 		pic_set_irq1(&s->pics[0], 2, 0);
-	}
+
 	irq = pic_get_irq(&s->pics[0]);
 	pic_irq_request(s->kvm, irq >= 0);
 }