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

["Cédric Le Goater" <clg@kaod.org>]

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/

Cheers,

C.