Re: [Qemu-devel] [PATCH v5 1/5] hpet: inverse polarity when pin above ISA_NUM_IRQS

[Michael S. Tsirkin]

On Sun, Sep 29, 2013 at 11:25:24AM +0800, liu ping fan wrote:
> On Sun, Sep 29, 2013 at 3:52 AM, Michael S. Tsirkin <address@hidden> wrote:
> > On Thu, Sep 12, 2013 at 11:25:14AM +0800, Liu Ping Fan wrote:
> >> According to hpet spec, hpet irq is high active. But according to
> >> ICH spec, there is inversion before the input of ioapic. So the OS
> >> will expect low active on this IRQ line.
> >
> >
> >>(And this is observed on
> >> bare metal).
> >
> > How does one test this on bare metal?
> >
> If changing the func hpet_timer_set_irq() in linux's hpet driver,
> from acpi_register_gsi(NULL, irq, ACPI_LEVEL_SENSITIVE,
> ACPI_ACTIVE_LOW);  to ACPI_ACTIVE_HIGH,  then run hpet_example.c on
> bare metal, the modified kernel will complain about spurious irq and
> disable the irq line.

ok that's useful info for the changelog.

> >>
> >> We fold the emulation of this inversion inside the hpet logic.
> >>
> >> Signed-off-by: Liu Ping Fan <address@hidden>
> >
> >
> > Doesn't this affect cross-version migration?
> > E.g. imagine that you migrate between systems
> > with/without this fix.
> >
> No. the changing only affect "route >= ISA_NUM_IRQS",  For linux
> guest, it use IRQ2/8 for hpet0/hpet1 which is reserved by kernel. It
> work without this fix (I think windows is the same). But the
> hpet_example.c(in linux) can not work without this fix. So no such
> run-time instance before this bug fix.
> 
> Regards,
> Pingfan

aha so the argument is it's already too broken to even mostly work,
we don't need to worry about migrating it to/from old qemu.

> >> ---
> >>  hw/timer/hpet.c | 14 ++++++++++++--
> >>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> >> index fcd22ae..8429eb3 100644
> >> --- a/hw/timer/hpet.c
> >> +++ b/hw/timer/hpet.c
> >> @@ -198,13 +198,23 @@ static void update_irq(struct HPETTimer *timer, int 
> >> set)
> >>      if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
> >>          s->isr &= ~mask;
> >>          if (!timer_fsb_route(timer)) {
> >> -            qemu_irq_lower(s->irqs[route]);
> >> +            /* 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)) {
> >>          stl_le_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
> >>      } else if (timer->config & HPET_TN_TYPE_LEVEL) {
> >>          s->isr |= mask;
> >> -        qemu_irq_raise(s->irqs[route]);
> >> +        /* 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]);
> >> --
> >> 1.8.1.4
> >>