Re: [Xen-devel] [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 22 Aug 2012, Jan Beulich wrote:
> - don't call rtc_timer_update() on REG_A writes when the value didn't
>   change (doing the call always was reported to cause wall clock time
>   lagging with the JVM running on Windows)
> - don't call rtc_timer_update() on REG_B writes at all
> - only call alarm_timer_update() on REG_B writes when relevant bits
>   change
> - only call check_update_timer() on REG_B writes when SET changes
> - instead properly handle AF and PF when the guest is not also setting
>   AIE/PIE respectively (for UF this was already the case, only a
>   comment was slightly inaccurate)
> - raise the RTC IRQ not only when UIE gets set while UF was already
>   set, but generalize this to cover AIE and PIE as well
> - properly mask off bit 7 when retrieving the hour values in
>   alarm_timer_update(), and properly use RTC_HOURS_ALARM's bit 7 when
>   converting from 12- to 24-hour value
> - also handle the two other possible clock bases
> - use RTC_* names in a couple of places where literal numbers were used
>   so far
> 
> Note that this only improves the situation described in the thread at
> http://lists.xen.org/archives/html/xen-devel/2012-08/msg00664.html,
> there are still problems with the emulation when invoked at a high rate
> as described there.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Although this patch solves a real problem and should probably go in at
some point, I am a bit worried about drifting too much from the original
RTC emulator (that was taken from QEMU), because it would be nice to be
able to backport features like this one:

http://marc.info/?l=qemu-devel&m=134392375010304


> v2: Small adjustment to the pt_update_irq() change, avoiding to call
>     the RTC code for a HPET event (which also may pass 8 [aka RTC_IRQ]
>     as create_periodic_time()'s "irq" argument.
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -50,11 +50,24 @@ 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_periodic_cb(struct vcpu *v, void *opaque)
> +static void rtc_toggle_irq(RTCState *s)
> +{
> +    struct domain *d = vrtc_domain(s);
> +
> +    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);
> +}
> +
> +void rtc_periodic_interrupt(void *opaque)
>  {
>      RTCState *s = opaque;
> +
>      spin_lock(&s->lock);
> -    s->hw.cmos_data[RTC_REG_C] |= 0xc0;
> +    s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
> +    if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
> +        rtc_toggle_irq(s);
>      spin_unlock(&s->lock);
>  }
> 
> @@ -68,19 +81,25 @@ static void rtc_timer_update(RTCState *s
>      ASSERT(spin_is_locked(&s->lock));
> 
>      period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
> -    if ( (period_code != 0) && (s->hw.cmos_data[RTC_REG_B] & RTC_PIE) )
> +    switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
>      {
> -        if ( period_code <= 2 )
> +    case RTC_REF_CLCK_32KHZ:
> +        if ( (period_code != 0) && (period_code <= 2) )
>              period_code += 7;
> -
> -        period = 1 << (period_code - 1); /* period in 32 Khz cycles */
> -        period = DIV_ROUND((period * 1000000000ULL), 32768); /* period in ns 
> */
> -        create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
> -                             rtc_periodic_cb, s);
> -    }
> -    else
> -    {
> +        /* fall through */
> +    case RTC_REF_CLCK_1MHZ:
> +    case RTC_REF_CLCK_4MHZ:
> +        if ( period_code != 0 )
> +        {
> +            period = 1 << (period_code - 1); /* period in 32 Khz cycles */
> +            period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
> +            create_periodic_time(v, &s->pt, period, period, RTC_IRQ, NULL, 
> s);
> +            break;
> +        }
> +        /* fall through */
> +    default:
>          destroy_periodic_time(&s->pt);
> +        break;
>      }
>  }
> 
> @@ -102,7 +121,7 @@ static void check_update_timer(RTCState
>          guest_usec = get_localtime_us(d) % USEC_PER_SEC;
>          if (guest_usec >= (USEC_PER_SEC - 244))
>          {
> -            /* RTC is in update cycle when enabling UIE */
> +            /* RTC is in update cycle */
>              s->hw.cmos_data[RTC_REG_A] |= RTC_UIP;
>              next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC;
>              expire_time = NOW() + next_update_time;
> @@ -144,7 +163,6 @@ static void rtc_update_timer(void *opaqu
>  static void rtc_update_timer2(void *opaque)
>  {
>      RTCState *s = opaque;
> -    struct domain *d = vrtc_domain(s);
> 
>      spin_lock(&s->lock);
>      if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
> @@ -152,11 +170,7 @@ static void rtc_update_timer2(void *opaq
>          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))
> -        {
> -            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> -            hvm_isa_irq_deassert(d, RTC_IRQ);
> -            hvm_isa_irq_assert(d, RTC_IRQ);
> -        }
> +            rtc_toggle_irq(s);
>          check_update_timer(s);
>      }
>      spin_unlock(&s->lock);
> @@ -175,21 +189,18 @@ static void alarm_timer_update(RTCState
> 
>      stop_timer(&s->alarm_timer);
> 
> -    if ((s->hw.cmos_data[RTC_REG_B] & RTC_AIE) &&
> -            !(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
> +    if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
>      {
>          s->current_tm = gmtime(get_localtime(d));
>          rtc_copy_date(s);
> 
>          alarm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS_ALARM]);
>          alarm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES_ALARM]);
> -        alarm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS_ALARM]);
> -        alarm_hour = convert_hour(s, alarm_hour);
> +        alarm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS_ALARM]);
> 
>          cur_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
>          cur_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
> -        cur_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS]);
> -        cur_hour = convert_hour(s, cur_hour);
> +        cur_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);
> 
>          next_update_time = USEC_PER_SEC - (get_localtime_us(d) % 
> USEC_PER_SEC);
>          next_update_time = next_update_time * NS_PER_USEC + NOW();
> @@ -343,7 +354,6 @@ static void alarm_timer_update(RTCState
>  static void rtc_alarm_cb(void *opaque)
>  {
>      RTCState *s = opaque;
> -    struct domain *d = vrtc_domain(s);
> 
>      spin_lock(&s->lock);
>      if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
> @@ -351,11 +361,7 @@ static void rtc_alarm_cb(void *opaque)
>          s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
>          /* alarm interrupt */
>          if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
> -        {
> -            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> -            hvm_isa_irq_deassert(d, RTC_IRQ);
> -            hvm_isa_irq_assert(d, RTC_IRQ);
> -        }
> +            rtc_toggle_irq(s);
>          alarm_timer_update(s);
>      }
>      spin_unlock(&s->lock);
> @@ -365,6 +371,7 @@ static int rtc_ioport_write(void *opaque
>  {
>      RTCState *s = opaque;
>      struct domain *d = vrtc_domain(s);
> +    uint32_t orig, mask;
> 
>      spin_lock(&s->lock);
> 
> @@ -382,6 +389,7 @@ static int rtc_ioport_write(void *opaque
>          return 0;
>      }
> 
> +    orig = s->hw.cmos_data[s->hw.cmos_index];
>      switch ( s->hw.cmos_index )
>      {
>      case RTC_SECONDS_ALARM:
> @@ -405,9 +413,9 @@ static int rtc_ioport_write(void *opaque
>          break;
>      case RTC_REG_A:
>          /* UIP bit is read only */
> -        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) |
> -            (s->hw.cmos_data[RTC_REG_A] & RTC_UIP);
> -        rtc_timer_update(s);
> +        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) | (orig & RTC_UIP);
> +        if ( (data ^ orig) & (RTC_RATE_SELECT | RTC_DIV_CTL) )
> +            rtc_timer_update(s);
>          break;
>      case RTC_REG_B:
>          if ( data & RTC_SET )
> @@ -415,7 +423,7 @@ static int rtc_ioport_write(void *opaque
>              /* set mode: reset UIP mode */
>              s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
>              /* adjust cmos before stopping */
> -            if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
> +            if (!(orig & RTC_SET))
>              {
>                  s->current_tm = gmtime(get_localtime(d));
>                  rtc_copy_date(s);
> @@ -424,21 +432,26 @@ static int rtc_ioport_write(void *opaque
>          else
>          {
>              /* if disabling set mode, update the time */
> -            if ( s->hw.cmos_data[RTC_REG_B] & RTC_SET )
> +            if ( orig & RTC_SET )
>                  rtc_set_time(s);
>          }
> -        /* if the interrupt is already set when the interrupt become
> -         * enabled, raise an interrupt immediately*/
> -        if ((data & RTC_UIE) && !(s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
> -            if (s->hw.cmos_data[RTC_REG_C] & RTC_UF)
> +        /*
> +         * 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) )
>              {
> -                hvm_isa_irq_deassert(d, RTC_IRQ);
> -                hvm_isa_irq_assert(d, RTC_IRQ);
> +                rtc_toggle_irq(s);
> +                break;
>              }
>          s->hw.cmos_data[RTC_REG_B] = data;
> -        rtc_timer_update(s);
> -        check_update_timer(s);
> -        alarm_timer_update(s);
> +        if ( (data ^ orig) & RTC_SET )
> +            check_update_timer(s);
> +        if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
> +            alarm_timer_update(s);
>          break;
>      case RTC_REG_C:
>      case RTC_REG_D:
> @@ -453,7 +466,7 @@ static int rtc_ioport_write(void *opaque
> 
>  static inline int to_bcd(RTCState *s, int a)
>  {
> -    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
> +    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
>          return a;
>      else
>          return ((a / 10) << 4) | (a % 10);
> @@ -461,7 +474,7 @@ static inline int to_bcd(RTCState *s, in
> 
>  static inline int from_bcd(RTCState *s, int a)
>  {
> -    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
> +    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
>          return a;
>      else
>          return ((a >> 4) * 10) + (a & 0x0f);
> @@ -469,12 +482,14 @@ static inline int from_bcd(RTCState *s,
> 
>  /* Hours in 12 hour mode are in 1-12 range, not 0-11.
>   * So we need convert it before using it*/
> -static inline int convert_hour(RTCState *s, int hour)
> +static inline int convert_hour(RTCState *s, int raw)
>  {
> +    int hour = from_bcd(s, raw & 0x7f);
> +
>      if (!(s->hw.cmos_data[RTC_REG_B] & RTC_24H))
>      {
>          hour %= 12;
> -        if (s->hw.cmos_data[RTC_HOURS] & 0x80)
> +        if (raw & 0x80)
>              hour += 12;
>      }
>      return hour;
> @@ -493,8 +508,7 @@ static void rtc_set_time(RTCState *s)
> 
>      tm->tm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
>      tm->tm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
> -    tm->tm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS] & 0x7f);
> -    tm->tm_hour = convert_hour(s, tm->tm_hour);
> +    tm->tm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);
>      tm->tm_wday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_WEEK]);
>      tm->tm_mday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_MONTH]);
>      tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -22,6 +22,7 @@
>  #include <asm/hvm/vpt.h>
>  #include <asm/event.h>
>  #include <asm/apic.h>
> +#include <asm/mc146818rtc.h>
> 
>  #define mode_is(d, name) \
>      ((d)->arch.hvm_domain.params[HVM_PARAM_TIMER_MODE] == HVMPTM_##name)
> @@ -218,6 +219,7 @@ void pt_update_irq(struct vcpu *v)
>      struct periodic_time *pt, *temp, *earliest_pt = NULL;
>      uint64_t max_lag = -1ULL;
>      int irq, is_lapic;
> +    void *pt_priv;
> 
>      spin_lock(&v->arch.hvm_vcpu.tm_lock);
> 
> @@ -251,13 +253,14 @@ void pt_update_irq(struct vcpu *v)
>      earliest_pt->irq_issued = 1;
>      irq = earliest_pt->irq;
>      is_lapic = (earliest_pt->source == PTSRC_lapic);
> +    pt_priv = earliest_pt->priv;
> 
>      spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> 
>      if ( is_lapic )
> -    {
>          vlapic_set_irq(vcpu_vlapic(v), irq, 0);
> -    }
> +    else if ( irq == RTC_IRQ && pt_priv )
> +        rtc_periodic_interrupt(pt_priv);
>      else
>      {
>          hvm_isa_irq_deassert(v->domain, irq);
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -181,6 +181,7 @@ void rtc_migrate_timers(struct vcpu *v);
>  void rtc_deinit(struct domain *d);
>  void rtc_reset(struct domain *d);
>  void rtc_update_clock(struct domain *d);
> +void rtc_periodic_interrupt(void *);
> 
>  void pmtimer_init(struct vcpu *v);
>  void pmtimer_deinit(struct domain *d);
> 
> 
> 

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