- 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@suse.com> --- 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@lists.xen.org http://lists.xen.org/xen-devel
Stefano Stabellini
2012-Aug-23 13:44 UTC
Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments
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@suse.com>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); > > >
Jan Beulich
2012-Aug-24 15:45 UTC
Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments
>>> On 23.08.12 at 15:44, Stefano Stabellini <stefano.stabellini@eu.citrix.com>wrote:> 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@suse.com> > > 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),Then does that emulator have similar problems?> because it would be nice to be able to backport features like this one: > > http://marc.info/?l=qemu-devel&m=134392375010304I agree this would be nice to have (albeit I''m not sure how much the original problem is actually present in the Xen code, particularly with the patch here applied, as I think it may implicitly clean up some of the unneccesary active timers). Jan
Stefano Stabellini
2012-Aug-24 17:34 UTC
Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments
On Fri, 24 Aug 2012, Jan Beulich wrote:> >>> On 23.08.12 at 15:44, Stefano Stabellini <stefano.stabellini@eu.citrix.com> > wrote: > > 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@suse.com> > > > > 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), > > Then does that emulator have similar problems?I am not sure, it probably does. I am afraid the code already drifted too much to make comparisons.> > because it would be nice to be able to backport features like this one: > > > > http://marc.info/?l=qemu-devel&m=134392375010304 > > I agree this would be nice to have (albeit I''m not sure how much > the original problem is actually present in the Xen code, particularly > with the patch here applied, as I think it may implicitly clean up some > of the unneccesary active timers).Maybe, but with your patch applied, are there going to be any timers running if the guest is not making use of the RTC?
Jan Beulich
2012-Aug-24 19:49 UTC
Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments
>>> On 24.08.12 at 19:34, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: >> > because it would be nice to be able to backport features like this one: >> > >> > http://marc.info/?l=qemu-devel&m=134392375010304 >> >> I agree this would be nice to have (albeit I''m not sure how much >> the original problem is actually present in the Xen code, particularly >> with the patch here applied, as I think it may implicitly clean up some >> of the unneccesary active timers). > > Maybe, but with your patch applied, are there going to be any timers > running if the guest is not making use of the RTC?Can''t tell without in depth analysis, as I didn''t pay too much attention to this aspect of the code. I just vaguely recall that in at least one case the changes might have resulted in not re-arming a timer under some condition where it previously got re-armed. Jan