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

Active low is indeed the usual thing for IRQ8, yet our MADT table
doesn't appear to say so. Perhaps the PnP stuff does?

Jan


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