On Wed, 27 May 2015 13:58:57 +0200 Paolo Bonzini pbonzini@redhat.com wrote:
the main issue here is lack of migration support. You need to add a new subsection to vmstate_ich9_pm that is migrated if the registers are different from the default values.
Hrm - OK. I didn't even take that into account. I'll do it.
+static QEMUTimer *tco_timer; +static unsigned int timeouts_no;
These must not be globals. Instead, add them to TCOIORegs.
OK.
+static inline void tco_timer_reload(void) +{
- int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- timer_del(tco_timer);
- timer_mod(tco_timer, now + tco_ticks_per_sec());
No need for del+mod. Also, these are not tco_ticks_per_sec() but rather TCO_TICK_NSEC. It can be a #define instead of a function.
OK.
For a little extra challenge, you could the base value when the timer was started inside TCOIORegs. The timer can be made to expire directly after 0.6*TCO_TMR seconds; you can use the base value and the current time to compute the value of TCO_RLD. Something like:
case TCO_RLD: rld = tr->tco.rld; /* base_clock is set to -1 in tco_timer_stop, reloaded in * tco_timer_reload. */ if (base_clock != -1) { elapsed = (qemu_get_clock(QEMU_CLOCK_VIRTUAL) -base_clock) / TCO_TICK_NSEC; rld -= MIN(elapsed, rld); }
This makes it possible for QEMU to sleep most of the time when the guest is idle, instead of waking up every 0.6 seconds. But feel free to work on this afterwards.
Nice. I liked your approach and that will definitely avoid overheard waking up on every 0.6s. No, I will work on this approach right now so I can sleep well.
+void acpi_pm_tco_init(TCOIORegs *tr) +{
- *tr = TCO_IO_REGS_DEFAULTS_INIT();
Just inline the macro definition here.
OK.
- case TCO1_CNT:
val &= TCO1_CNT_MASK;/* TCO_LOCK bit cannot be changed once set */tr->tco.cnt1 = (val & ~TCO_LOCK) | (tr->tco.cnt1 &TCO_LOCK);
This means that TCO_LOCK is never set in tr->tco.cnt1, I think? If I'm correct, it should be covered by tests.
No. The TCO_LOCK bit is set in tr->tco.cnt1, but cannot be unset afterwards -- on reset, of course, it will. Maybe a test that covers setting it and then unsetting it might be interesting to have in tests/tco-test.c.
+#define TCO_IO_REGS_DEFAULTS_INIT() \
- (TCOIORegs) { \
.tco = { \.rld = TCO_RLD_DEFAULT, \.din = TCO_DAT_IN_DEFAULT, \.dout = TCO_DAT_OUT_DEFAULT, \.sts1 = TCO1_STS_DEFAULT, \.sts2 = TCO2_STS_DEFAULT, \.cnt1 = TCO1_CNT_DEFAULT, \.cnt2 = TCO2_CNT_DEFAULT, \.msg1 = TCO_MESSAGE1_DEFAULT, \.msg2 = TCO_MESSAGE2_DEFAULT, \.wdcnt = TCO_WDCNT_DEFAULT, \.tmr = TCO_TMR_DEFAULT, \}, \.sw_irq_gen = SW_IRQ_GEN_DEFAULT \- }
No need for this definition. This and the *_DEFAULT definitions as well should be in the .c file.
OK.
+/* tco.c */ +void acpi_pm_tco_init(TCOIORegs *tr); +uint32_t acpi_pm_tco_ioport_readw(TCOIORegs *tr, uint32_t addr); +void acpi_pm_tco_ioport_writew(TCOIORegs *tr, uint32_t addr, uint32_t val); + +/* As per ICH9 spec, the internal timer has an error of ~0.6s on every tick */ +static inline int64_t tco_ticks_per_sec(void) +{
- return 600000000LL;
+}
+static inline int is_valid_tco_time(uint32_t val) +{
- /* values of 0 or 1 will be ignored by ICH */
- return val > 1;
+}
+static inline int can_start_tco_timer(TCOIORegs *tr) +{
- return !(tr->tco.cnt1 & TCO_TMR_HLT) &&
is_valid_tco_time(tr->tco.tmr); +}
These three inlines should be in the .c file.
OK.
Don't be discouraged by the comments. It's good work!
Alright. Thank you for the comments! I'll send a v2 soon.
Paulo