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 <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
>
> I did ! Fixed there :
>
>
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-...@kaod.org/
>
Thanks. That one might be further down in my review queue 😅
Andrew
