| Message ID | 5046135B.2080200@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 >
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
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
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); }