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

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

[Bill Paul]

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

Right now all I know is that VxWorks' usage of the HPET seems to work on real 
hardware, but not on QEMU. I suspect that the changes made to hpet.c here may 
have only "fixed" the problem with Linux by introducing some non-standard 
behavior that happens to pacify Linux's particular usage model.

Can someone comment on whether or not this inversion logic is really still 
necessary in Linux? Is there maybe a better way to handle this?

-Bill

-- 
=============================================================================
-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
=============================================================================

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

[Paolo Bonzini]

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

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

[Bill Paul]

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
=============================================================================

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

[Paolo Bonzini]

On 05/04/2016 20:30, Bill Paul wrote:
>>> 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.

See commit 100943c54e09 (that's a kernel commit).  KVM actually did
invert the sense of the IRQ line, though that was very buggy too due to
doing so very naively (and that's why it was ripped out).

Thanks,

Paolo