Re: [Xen-devel] [PATCH 1/4] x86/hvm: Centralize and simplify the RTC IRQ logic.

[Tim Deegan <tim@xxxxxxx>]

At 15:19 +0000 on 28 Mar (1364483946), Jan Beulich wrote:
> >>> On 28.03.13 at 15:38, Tim Deegan <tim@xxxxxxx> wrote:
> > At 14:22 +0000 on 28 Mar (1364480528), Jan Beulich wrote:
> >> >>> On 28.03.13 at 15:08, Tim Deegan <tim@xxxxxxx> wrote:
> >> > At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
> >> >> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xxxxxxx> wrote:
> >> >> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
> >> >> > -    hvm_isa_irq_assert(d, RTC_IRQ);
> >> >> > +
> >> >> > +    /* IRQ is raised if any of the sources is raised & enabled */
> >> >> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
> >> >> > +            & s->hw.cmos_data[RTC_REG_C]
> >> >> > +            & (RTC_PF|RTC_AF|RTC_UF))
> >> >> > +        ? RTC_IRQF : 0;
> >> >> > +
> >> >> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
> >> >> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
> >> >> > +
> >> >> > +    if ( irqf )
> >> >> > +        hvm_isa_irq_assert(d, RTC_IRQ);
> >> >> > +    else
> >> >> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
> >> >> 
> >> >> One fundamental question here is - do you or does anyone else
> >> >> know why originally the code did this odd looking assert-deassert
> >> >> sequence. I'm a little afraid that this change may cause observably
> >> >> different behavior. In particular, our ACPI MADT doesn't (and
> >> >> shouldn't) declare IRQ8 as level triggered, yet the way you change
> >> >> the code would seem to make the interrupt behave as if it was.
> >> > 
> >> > Hmm.  I was following the spec for the MC146818A RTC chip, which is
> >> > pretty clearly level-triggered.  Looking at the piix4 spec:
> >> > 
> >> >   IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
> >> >   not be modified by software.
> >> 
> >> So first you say level, then you quote edge? Now I'm even more
> >> confused.
> > 
> > Me too.  The original RTC seems to be level, but the piix4 (which ought
> > to be compatible with it) says edge.  The piix4 spec is pretty unhelpful
> > about whether it will send a second interrupt if IRQF is already set:
> > 
> >   Interrupt Request Flag (IRQF). Interrupt Request Flag=PF * PIE + AF *
> >   AIE + UF * UFE. This also causes the CH_IRQ_B signal to be asserted.
> > 
> > No mention of CH_IRQ_B anywhere else in the document.
> > 
> > The existing code is a weird mix: it deasserts the IRQ when REG_C is
> > read, and also strobes it whenever any of PF, AF or UF is set (and the
> > corresponding enable bit) - even if that bit is already set.
> 
> And it further doesn't help that we don't even have
> vioapic_irq_negative_edge() as counterpart to
> vioapic_irq_positive_edge(), i.e. we're not even capable of truly
> delivering an active low IRQ.

I get the impression that the xen IRQ model doesn't actually include a
concept of 'active high' vs 'active low', just 'asserted' or 'not
asserted'.  Keir?

> Which sadly makes me feel even more
> nervous about your change here. Plus if IRQ8 is active low in our
> emulation, then the if and else bodies would need to be switched
> (which then wouldn't work because of deassert_irq() being too
> simplistic).

OK, so should I just respin without patch 1/4 and pretend I never saw
any of this mess? :)

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel