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

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

On Mon, 25 Sep 2023, at 18:50, Cédric Le Goater wrote:
> On 9/25/23 09:54, Andrew Jeffery wrote:
>> 
>> 
>> 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
>
> I did ! Fixed there :
>
>    
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-clg@kaod.org/
>

Thanks. That one might be further down in my review queue 😅

Andrew