On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote:
> Joel, Andrew,
>
> On 5/25/19 17:12, Cédric Le Goater wrote:
>> From: Joel Stanley <j...@jms.id.au>
>>
>> The Linux kernel driver was updated in commit 4451d3f59f2a
>> ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an
>> issue observed on hardware:
>>
>> > RELOAD register is loaded into COUNT register when the aspeed timer
>> > is enabled, which means the next event may be delayed because timer
>> > interrupt won't be generated until <0xFFFFFFFF - current_count +
>> > cycles>.
>>
>> When running under Qemu, the system appeared "laggy". The guest is now
>> scheduling timer events too regularly, starving the host of CPU time.
>>
>> This patch modifies the timer model to attempt to schedule the timer
>> expiry as the guest requests, but if we have missed the deadline we
>> re interrupt and try again, which allows the guest to catch up.
>>
>> Provides expected behaviour with old and new guest code.
>>
>> Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model")
>> Signed-off-by: Joel Stanley <j...@jms.id.au>
>> [clg: - merged a fix from Andrew Jeffery <and...@aj.id.au>
>> "Fire interrupt on failure to meet deadline"
>> https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html
>> - adapted commit log
>> - checkpatch fixes ]
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> hw/timer/aspeed_timer.c | 59 ++++++++++++++++++++++-------------------
>> 1 file changed, 31 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> index 5c786e512815..9ffd8e09f670 100644
>> --- a/hw/timer/aspeed_timer.c
>> +++ b/hw/timer/aspeed_timer.c
>> @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct
>> AspeedTimer *t, uint32_t ticks)
>>
>> static uint64_t calculate_next(struct AspeedTimer *t)
>> {
>> - uint64_t next = 0;
>> - uint32_t rate = calculate_rate(t);
>> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> + uint64_t next;
>>
>> - while (!next) {
>> - /* We don't know the relationship between the values in the match
>> - * registers, so sort using MAX/MIN/zero. We sort in that order as
>> the
>> - * timer counts down to zero. */
>> - uint64_t seq[] = {
>> - calculate_time(t, MAX(t->match[0], t->match[1])),
>> - calculate_time(t, MIN(t->match[0], t->match[1])),
>> - calculate_time(t, 0),
>> - };
>> - uint64_t reload_ns;
>> - uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> -
>> - if (now < seq[0]) {
>> - next = seq[0];
>> - } else if (now < seq[1]) {
>> - next = seq[1];
>> - } else if (now < seq[2]) {
>> - next = seq[2];
>> - } else if (t->reload) {
>> - reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
>> - t->start = now - ((now - t->start) % reload_ns);
>> - } else {
>> - /* no reload value, return 0 */
>> - break;
>> - }
>> + /*
>> + * We don't know the relationship between the values in the match
>> + * registers, so sort using MAX/MIN/zero. We sort in that order as
>> + * the timer counts down to zero.
>> + */
>> +
>> + next = calculate_time(t, MAX(t->match[0], t->match[1]));
>> + if (now < next) {
>> + return next;
>> + }
>> +
>> + next = calculate_time(t, MIN(t->match[0], t->match[1]));
>> + if (now < next) {
>> + return next;
>> + }
>> +
>> + next = calculate_time(t, 0);
>> + if (now < next) {
>> + return next;
>> + }
>> +
>> + /* We've missed all deadlines, fire interrupt and try again */
>> + timer_del(&t->timer);
>> +
>> + if (timer_overflow_interrupt(t)) {
>> + t->level = !t->level;
>> + qemu_set_irq(t->irq, t->level);
>> }
>>
>> - return next;
>> + t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> + return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
>
> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
> from ? Thanks,
The inner MAX() deals with the lack of ordering constraints between the match
values. I think the outer MAX() is redundant. We should probably remove it. The
match member is type uint32_t so it can't be negative. You did steal that from
an RFC patch :D
Andrew
