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

[Bill Paul <wpaul@windriver.com>]

Of all the gin joints in all the towns in all the world, Paolo Bonzini had to 
walk into mine at 06:20:05 on Tuesday 05 April 2016 and say:

> 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).

I wonder how it worked on KVM.
 
> 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.

Yes, that does fix it for me. I can even share the interrupt with the 
simulated Intel PRO/1000 ethernet device and that works too (though obviously 
I wouldn't normally want to do that; I just wanted to test the VxWorks IRQ 
chaining support).

I'll see about getting a patch generated.

-Bill

 
> Thanks,
> 
> Paolo

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Member of Technical Staff,
                 wpaul@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================