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

[Tim Deegan <tim@xxxxxxx>]

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:
> >> > Signed-off-by: Tim Deegan <tim@xxxxxxx>
> >> > ---
> >> >  xen/arch/x86/hvm/rtc.c |   43 
> >> > ++++++++++++++++++++++---------------------
> >> >  1 file changed, 22 insertions(+), 21 deletions(-)
> >> > 
> >> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> >> > index c1e09d8..368d3c8 100644
> >> > --- a/xen/arch/x86/hvm/rtc.c
> >> > +++ b/xen/arch/x86/hvm/rtc.c
> >> > @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
> >> >  static inline int from_bcd(RTCState *s, int a);
> >> >  static inline int convert_hour(RTCState *s, int hour);
> >> >  
> >> > -static void rtc_toggle_irq(RTCState *s)
> >> > +static void rtc_update_irq(RTCState *s)
> >> >  {
> >> >      struct domain *d = vrtc_domain(s);
> >> > +    uint8_t irqf;
> >> >  
> >> >      ASSERT(spin_is_locked(&s->lock));
> >> > -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> >> > -    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.

Tim.

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