Re: [Xen-devel] [PATCH RFC 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.02.14 at 18:32, Tim Deegan <tim@xxxxxxx> wrote:
> If the guest has not asked for interrupts, don't run the vpt timer
> to generate them.  This is a prerequisite for a patch to simplify how
> the vpt interacts with the RTC, and also gets rid of a timer series in
> Xen in a case where it's unlikely to be needed.
> 
> Instead, calculate the correct value for REG_C.PF whenever REG_C is
> read or PIE is enabled.  This allow a guest to poll for the PF bit
> while not asking for actual timer interrupts.  Such a guest would no
> longer get the benefit of the vpt's timer modes.
> 
> Signed-off-by: Tim Deegan <tim@xxxxxxx>

Looks okay to me. Two minor comments below.

> @@ -125,24 +144,28 @@ static void rtc_timer_update(RTCState *s)
>      case RTC_REF_CLCK_4MHZ:
>          if ( period_code != 0 )
>          {
> -            if ( period_code != s->pt_code )
> +            now = NOW();

This is needed only inside the next if, so perhaps move it there (and
I'd prefer the variable declaration to be moved there too).

> +            period = 1 << (period_code - 1); /* period in 32 Khz cycles */
> +            period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
> +            if ( period != s->period )
>              {
> -                s->pt_code = period_code;
> -                period = 1 << (period_code - 1); /* period in 32 Khz cycles 
> */
> -                period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns 
> */
> +                s->period = period;
>                  if ( v->domain->arch.hvm_domain.params[HVM_PARAM_VPT_ALIGN] )
>                      delta = 0;
>                  else
> -                    delta = period - ((NOW() - s->start_time) % period);
> -                create_periodic_time(v, &s->pt, delta, period,
> -                                     RTC_IRQ, NULL, s);
> +                    delta = period - ((now - s->start_time) % period);
> +                if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
> +                    create_periodic_time(v, &s->pt, delta, period,
> +                                         RTC_IRQ, NULL, s);
> +                else
> +                    s->check_ticks_since = now;
>              }
>              break;
>          }
> @@ -492,6 +516,11 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, 
> uint32_t data)
>          rtc_update_irq(s);
>          if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
>              rtc_timer_update(s);
> +        else if ( !(data & RTC_PIE) && (orig & RTC_PIE) )
> +        {
> +            destroy_periodic_time(&s->pt);
> +            rtc_timer_update(s);
> +        }

I think these two paths should be folded, along the lines of

        if ( (data ^ orig) & RTC_PIE )
        {
            if ( !(data & RTC_PIE) )
                destroy_periodic_time(&s->pt);
            rtc_timer_update(s);
        }

Jan


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