Re: [Xen-devel] [PATCH 3/6] x86/HVM: adjust IRQ (de-)assertion

[Tim Deegan <tim@xxxxxxx>]

At 14:53 +0100 on 29 Apr (1367247201), Jan Beulich wrote:
> De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
> REG_C reads. Assertion should be done only when the flag transitions
> from 0 to 1.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Well, this seems correct to me as far as the original (level-triggered)
RTC chip goes, but when we discussed changing this before, you suggested
that we should keep the old (somewhat bizarre) behaviour.

Unless this is fixing an observed bug, and unless you've tested it
widely, I don't think this is for 4.3.

Tim.

> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -52,23 +52,19 @@ static inline int convert_hour(RTCState 
>  
>  static void rtc_update_irq(RTCState *s)
>  {
> -    struct domain *d = vrtc_domain(s);
> -    uint8_t irqf;
> -
>      ASSERT(spin_is_locked(&s->lock));
>  
> +    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
> +        return;
> +
>      /* IRQ is raised if any source is both 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;
> -
> -    hvm_isa_irq_deassert(d, RTC_IRQ);
> -    if ( irqf )
> -        hvm_isa_irq_assert(d, RTC_IRQ);
> +    if ( !(s->hw.cmos_data[RTC_REG_B] &
> +           s->hw.cmos_data[RTC_REG_C] &
> +           (RTC_PF | RTC_AF | RTC_UF)) )
> +        return;
> +
> +    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> +    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
>  }
>  
>  void rtc_periodic_interrupt(void *opaque)
> @@ -631,6 +627,8 @@ static uint32_t rtc_ioport_read(RTCState
>      case RTC_REG_C:
>          ret = s->hw.cmos_data[s->hw.cmos_index];
>          s->hw.cmos_data[RTC_REG_C] = 0x00;
> +        if ( ret & RTC_IRQF )
> +            hvm_isa_irq_deassert(d, RTC_IRQ);
>          rtc_update_irq(s);
>          check_update_timer(s);
>          alarm_timer_update(s);
> 
> 
> 

> x86/HVM: adjust IRQ (de-)assertion
> 
> De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
> REG_C reads. Assertion should be done only when the flag transitions
> from 0 to 1.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -52,23 +52,19 @@ static inline int convert_hour(RTCState 
>  
>  static void rtc_update_irq(RTCState *s)
>  {
> -    struct domain *d = vrtc_domain(s);
> -    uint8_t irqf;
> -
>      ASSERT(spin_is_locked(&s->lock));
>  
> +    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
> +        return;
> +
>      /* IRQ is raised if any source is both 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;
> -
> -    hvm_isa_irq_deassert(d, RTC_IRQ);
> -    if ( irqf )
> -        hvm_isa_irq_assert(d, RTC_IRQ);
> +    if ( !(s->hw.cmos_data[RTC_REG_B] &
> +           s->hw.cmos_data[RTC_REG_C] &
> +           (RTC_PF | RTC_AF | RTC_UF)) )
> +        return;
> +
> +    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> +    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
>  }
>  
>  void rtc_periodic_interrupt(void *opaque)
> @@ -631,6 +627,8 @@ static uint32_t rtc_ioport_read(RTCState
>      case RTC_REG_C:
>          ret = s->hw.cmos_data[s->hw.cmos_index];
>          s->hw.cmos_data[RTC_REG_C] = 0x00;
> +        if ( ret & RTC_IRQF )
> +            hvm_isa_irq_deassert(d, RTC_IRQ);
>          rtc_update_irq(s);
>          check_update_timer(s);
>          alarm_timer_update(s);


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