[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] ich9: add TCO interface emulation
From: |
Paulo Alcantara |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] ich9: add TCO interface emulation |
Date: |
Wed, 27 May 2015 15:23:28 -0300 |
On Wed, 27 May 2015 13:58:57 +0200
Paolo Bonzini <address@hidden> 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.