Re: [Qemu-devel] [PATCH 1/3] ich9: add TCO interface emulation

[Paulo Alcantara <pcacjr@zytor.com>]

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

-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.