From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33758) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anQtz-0007l9-Cs for qemu-devel@nongnu.org; Tue, 05 Apr 2016 09:20:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1anQtu-0006DT-0f for qemu-devel@nongnu.org; Tue, 05 Apr 2016 09:20:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33555) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anQtt-0006DO-R4 for qemu-devel@nongnu.org; Tue, 05 Apr 2016 09:20:09 -0400 References: <201604041442.46614.wpaul@windriver.com> From: Paolo Bonzini Message-ID: <5703BB85.2070200@redhat.com> Date: Tue, 5 Apr 2016 15:20:05 +0200 MIME-Version: 1.0 In-Reply-To: <201604041442.46614.wpaul@windriver.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] Question about hw/timer/hpet.c, hw/intc/ioapic.c and polarity List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bill Paul , qemu-devel@nongnu.org Cc: Eduardo Habkost , Richard Henderson 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=20 > I've encountered something which looks a little odd that I'm hoping som= eone=20 > can clarify for me. First, some background: >=20 > The HPET timer supports three kinds of interrupt delivery: >=20 > 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 t= o the=20 > local APIC (I/O APIC is bypassed) >=20 > By default, VxWorks uses front-side bus mode, and that seems to work fi= ne.=20 > However I wanted to try I/O APIC mode too, and that seems to behave in = a funny=20 > way. In particular, the following code in hw/timer/hpet.c puzzles me: >=20 > if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) { > s->isr &=3D ~mask; > if (!timer_fsb_route(timer)) { > /* fold the ICH PIRQ# pin's internal inversion logic into h= pet */ > if (route >=3D 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_UNSPEC= IFIED, > NULL); > } else if (timer->config & HPET_TN_TYPE_LEVEL) { > s->isr |=3D mask; > /* fold the ICH PIRQ# pin's internal inversion logic into hpet = */ > if (route >=3D ISA_NUM_IRQS) { > qemu_irq_lower(s->irqs[route]); > } else { > qemu_irq_raise(s->irqs[route]); > } > } else { > s->isr &=3D ~mask; > qemu_irq_pulse(s->irqs[route]); > } >=20 > Note the part that inverts the interrupt handling logic for ICH PIRQ pi= ns. I=20 > 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, t= he HPET=20 > timer interrupt service routine is immediately called, even though the = timer=20 > hasn't expired yet. The ISR reads 0 from the HPET status register, indi= cating=20 > that no timers have events pending, so it just returns. The first=20 > qemu_irq_raise() call is triggered because hpet_enabled() returns true,= but=20 > timer_enabled() returns false. >=20 > Researching the code history, I see that the inversion logic was added = in 2013=20 > in order to fix a problem with HPET usage in Linux. However something a= bout=20 > the way this was done looks wrong to me. In the case where we actually = want to=20 > signal an interrupt because the timer has expired, and the IRQ is large= r than=20 > 15, the code calls qemu_irq_lower() instead of qemu_irq_raise(). Eventu= ally=20 > this results in ioapic_set_irq() being called with level =3D 0. The pro= blem is,=20 > ioapic_set_irq() will only call ioapic_service() to notify the quest of= an=20 > interrupt if level =3D=3D 1. >=20 > Given this, I can't understand how this is supposed to work. The hpet.c= code=20 > seems to treat qemu_irq_raise() and qemu_irq_lower() as though they can= =20 > trigger active high or active low interrupts, but the ioapic.c code doe= sn't=20 > support any polarity settings. The only way to actually trigger an inte= rrupt=20 > to the guest is to "raise" (assert) the interrupt by calling qemu_irq_r= aise(). 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