Re: [Qemu-devel] Question about hw/timer/hpet.c, hw/intc/ioapic.c and polarity

[Paolo Bonzini <pbonzini@redhat.com>]

On 04/04/2016 23:42, Bill Paul wrote:
> I'm testing some of the HPET handling code in VxWorks using QEMU 2.4.1 and 
> I've encountered something which looks a little odd that I'm hoping someone 
> can clarify for me. First, some background:
> 
> The HPET timer supports three kinds of interrupt delivery:
> 
> Legacy: HPET can use the same IRQs as the old 8254 timer (IRQ2, IRQ8)
> I/O APIC: HPET can use any of the first 32 I/O APIC IRQs in the system
> FSB: HPET uses "front-side bus" mode, i.e interrupts are routed right to the 
> local APIC (I/O APIC is bypassed)
> 
> By default, VxWorks uses front-side bus mode, and that seems to work fine. 
> However I wanted to try I/O APIC mode too, and that seems to behave in a funny 
> way. In particular, the following code in hw/timer/hpet.c puzzles me:
> 
>     if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
>         s->isr &= ~mask;
>         if (!timer_fsb_route(timer)) {
>             /* fold the ICH PIRQ# pin's internal inversion logic into hpet */
>             if (route >= ISA_NUM_IRQS) {
>                 qemu_irq_raise(s->irqs[route]);
>             } else {
>                 qemu_irq_lower(s->irqs[route]);
>             }
>         }
>     } else if (timer_fsb_route(timer)) {
>         address_space_stl_le(&address_space_memory, timer->fsb >> 32,
>                              timer->fsb & 0xffffffff, MEMTXATTRS_UNSPECIFIED,
>                              NULL);
>     } else if (timer->config & HPET_TN_TYPE_LEVEL) {
>         s->isr |= mask;
>         /* fold the ICH PIRQ# pin's internal inversion logic into hpet */
>         if (route >= ISA_NUM_IRQS) {
>             qemu_irq_lower(s->irqs[route]);
>         } else {
>             qemu_irq_raise(s->irqs[route]);
>         }
>     } else {
>         s->isr &= ~mask;
>         qemu_irq_pulse(s->irqs[route]);
>     }
> 
> Note the part that inverts the interrupt handling logic for ICH PIRQ pins. I 
> don't understand how this is supposed to work.

I think t should be removed.

 If I use level triggered IRQ2
> or IRQ8 in VxWorks, then things work as expected. But if I use IRQ21, the HPET 
> timer interrupt service routine is immediately called, even though the timer 
> hasn't expired yet. The ISR reads 0 from the HPET status register, indicating 
> that no timers have events pending, so it just returns. The first 
> qemu_irq_raise() call is triggered because hpet_enabled() returns true, but 
> timer_enabled() returns false.
> 
> Researching the code history, I see that the inversion logic was added in 2013 
> in order to fix a problem with HPET usage in Linux. However something about 
> the way this was done looks wrong to me. In the case where we actually want to 
> signal an interrupt because the timer has expired, and the IRQ is larger than 
> 15, the code calls qemu_irq_lower() instead of qemu_irq_raise(). Eventually 
> this results in ioapic_set_irq() being called with level = 0. The problem is, 
> ioapic_set_irq() will only call ioapic_service() to notify the quest of an 
> interrupt if level == 1.
> 
> Given this, I can't understand how this is supposed to work. The hpet.c code 
> seems to treat qemu_irq_raise() and qemu_irq_lower() as though they can 
> trigger active high or active low interrupts, but the ioapic.c code doesn't 
> support any polarity settings. The only way to actually trigger an interrupt 
> to the guest is to "raise" (assert) the interrupt by calling qemu_irq_raise().

I think that commit 0d63b2d ("hpet: inverse polarity when pin above
ISA_NUM_IRQS", 2013-12-11) can be reverted.  The code was probably
tested on KVM only, but KVM now is *also* ignoring the IOAPIC polarity
(commit 100943c54e09, "kvm: x86: ignore ioapic polarity", 2014-03-13).

Does that work for you?  If so, can you post the patch for the revert?
Remember to add Signed-off-by, "git revert" doesn't do that for you.

Thanks,

Paolo