From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yxfzg-0002HU-Rb for qemu-devel@nongnu.org; Wed, 27 May 2015 14:23:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yxfzc-0003uy-NO for qemu-devel@nongnu.org; Wed, 27 May 2015 14:23:56 -0400 Received: from terminus.zytor.com ([2001:1868:205::10]:60095 helo=mail.zytor.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yxfzc-0003sf-GK for qemu-devel@nongnu.org; Wed, 27 May 2015 14:23:52 -0400 Date: Wed, 27 May 2015 15:23:28 -0300 From: Paulo Alcantara Message-ID: <20150527152328.33a8acf5@zytor.com> In-Reply-To: <5565B181.7020700@redhat.com> References: <1432686576-14816-1-git-send-email-pcacjr@zytor.com> <5565B181.7020700@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] ich9: add TCO interface emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: seabios@seabios.org, Paulo Alcantara , qemu-devel@nongnu.org On Wed, 27 May 2015 13:58:57 +0200 Paolo Bonzini 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 -- Paulo Alcantara, C.E.S.A.R Speaking for myself only.