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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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