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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.13 at 16:30, Tim Deegan <tim@xxxxxxx> wrote:
> 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? :)

I'd favor keeping the patch, but with the IRQ raising itself left to
be a strobe:

    if ( irqf )
    {
        hvm_isa_irq_assert(d, RTC_IRQ);
        hvm_isa_irq_deassert(d, RTC_IRQ);
    }

Perhaps say a word about this being bogus in the description.

Jan



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