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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/03/2013 13:22, Tim Deegan wrote:
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
>  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 */

s/sources is/sources are/

~Andrew

> +    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);
>  }
>  
>  void rtc_periodic_interrupt(void *opaque)
> @@ -70,8 +82,7 @@ void rtc_periodic_interrupt(void *opaque)
>      else
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
> -        if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
> -            rtc_toggle_irq(s);
> +        rtc_update_irq(s);
>      }
>      spin_unlock(&s->lock);
>  }
> @@ -174,8 +185,7 @@ static void rtc_update_timer2(void *opaque)
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_UF;
>          s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
> -        if ((s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
> -            rtc_toggle_irq(s);
> +        rtc_update_irq(s);
>          check_update_timer(s);
>      }
>      spin_unlock(&s->lock);
> @@ -365,9 +375,7 @@ static void rtc_alarm_cb(void *opaque)
>      if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
> -        /* alarm interrupt */
> -        if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
> -            rtc_toggle_irq(s);
> +        rtc_update_irq(s);
>          alarm_timer_update(s);
>      }
>      spin_unlock(&s->lock);
> @@ -377,7 +385,7 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>  {
>      RTCState *s = opaque;
>      struct domain *d = vrtc_domain(s);
> -    uint32_t orig, mask;
> +    uint32_t orig;
>  
>      spin_lock(&s->lock);
>  
> @@ -451,15 +459,8 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>          /*
>           * If the interrupt is already set when the interrupt becomes
>           * enabled, raise an interrupt immediately.
> -         * NB: RTC_{A,P,U}IE == RTC_{A,P,U}F respectively.
>           */
> -        for ( mask = RTC_UIE; mask <= RTC_PIE; mask <<= 1 )
> -            if ( (data & mask) && !(orig & mask) &&
> -                 (s->hw.cmos_data[RTC_REG_C] & mask) )
> -            {
> -                rtc_toggle_irq(s);
> -                break;
> -            }
> +        rtc_update_irq(s);
>          s->hw.cmos_data[RTC_REG_B] = data;
>          if ( (data ^ orig) & RTC_SET )
>              check_update_timer(s);
> @@ -615,8 +616,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
>          break;
>      case RTC_REG_C:
>          ret = s->hw.cmos_data[s->hw.cmos_index];
> -        hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
>          s->hw.cmos_data[RTC_REG_C] = 0x00;
> +        rtc_update_irq(s);
>          check_update_timer(s);
>          alarm_timer_update(s);
>          rtc_timer_update(s);