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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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