On 25/6/21 00:09, Paolo Bonzini wrote:
On 23/06/21 16:24, Jason Thorpe wrote:
Second, the firmware could set a nonzero period, and this would cause
continuous interruptions of the guest after the firmware stops, due to
s->periodic_timer firing. This is "optimized" by the bug that you are
fixing. To keep the optimization you could:
- do the timer_mod in periodic_timer_update only if !PF || (PIE &&
lost_tick_policy==SLEW)
- in cmos_ioport_read, if !timer_pending(s->periodic_timer) call
periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
s->period, true);
to update s->next_periodic_time for the next tick and ensure PF will
be set.
I might be missing some subtlety here, but by my reading of
periodic_timer_update(), either one of those changes would result in a
delay of the next latching of PF by however many ns the CPU was late in
reading PF since the last time it was latched Please correct me if I’m
wrong about this!
No, it shouldn't. I may be wrong, but the process is the following:
- the current rtc_clock value is stored in cur_clock
- because period_change is true, the delay between writing PF and
reading C is stored in lost_clock
- then the delay is compensated by next_irq_clock = cur_clock + period -
lost_clock
Indeed we skip the periodic timer entirely when PIE is clear,
so PF never latched with PIE=0.
The following (based on top of f29ccc287a6c) seems to fix it:
-- >8 --
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index ba396435d1af..6a60d4cf3673 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -132,10 +132,6 @@ static uint32_t
rtc_periodic_clock_ticks(MC146818RtcState *s)
{
int period_code;
- if (!(s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
- return 0;
- }
-
period_code = s->cmos_data[RTC_REG_A] & 0x0f;
return periodic_period_to_clock(period_code);
@@ -160,6 +156,12 @@ static void periodic_timer_update(MC146818RtcState
*s, int64_t current_time,
return;
}
+ if (!(s->cmos_data[RTC_REG_B] & REG_B_PIE) &&
+ (s->cmos_data[RTC_REG_C] & REG_C_PF)) {
+ timer_del(s->periodic_timer);
+ return;
+ }
+
/* compute 32 khz clock */
cur_clock =
muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
@@ -192,7 +194,8 @@ static void periodic_timer_update(MC146818RtcState
*s, int64_t current_time,
* to a shorter period, scale up the missing ticks since the
* OS handler will treat past delayed ticks as shorter.
*/
- if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW &&
+ (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
uint32_t old_irq_coalesced = s->irq_coalesced;
lost_clock += old_irq_coalesced * old_period;
@@ -239,6 +242,8 @@ static void rtc_periodic_timer(void *opaque)
}
} else
qemu_irq_raise(s->irq);
+ } else {
+ timer_del(s->periodic_timer);
}
}
@@ -700,6 +705,10 @@ static uint64_t cmos_ioport_read(void *opaque,
hwaddr addr,
if (ret & (REG_C_UF | REG_C_AF)) {
check_update_timer(s);
}
+ if ((ret & REG_C_PF) && !(s->cmos_data[RTC_REG_B] &
REG_B_PIE)) {
+ periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
+ s->period, false);
+ }
if(s->irq_coalesced &&
(s->cmos_data[RTC_REG_B] & REG_B_PIE) &&
---
Jason, do you happen to still have your setup to test it?
The best way to confirm this would be by writing a testcase (there's
already an mc146818 suite in tests/qtest).
Paolo
There exists software out there in the wild that depends on PF
latching at regular intervals regardless if when the CPU reads, it, i.e.:
PF PF PF PF PF PF
C C C C C
-- thorpej
