Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux

["Andrew Jeffery" <andrew@aj.id.au>]

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 <joel@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 <joel@jms.id.au>
>> [clg: - merged a fix from Andrew Jeffery <andrew@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 <clg@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