Enhance x86 apic timer driver

[kwd-doodling]

1. Add 64bit cycle counter support.
2. Improve accuracy of last_announcement assignment.
3. Improve TSC mode accuracy by using TSC count

[teburd]

A few nits, and maybe a modest concern given the math was already being done this way previously

[teburd]

Commit 56392b8 wording could use a little cleanup

I'd suggest
"Change last_announcement to track the tick aligned cycle count of the last announcement rather than the absolute cycle count. This ensures the the number of ticks that have elapsed since last_announcement, provided by sys_clock_elapsed(), are accurate. Without this change the number of elapsed ticks may show up as one more than have actually elapsed."

[andyross]

This driver is a legacy thing. It exists to support platforms that have the old APIC countdown hardware and not TSC deadline support, which basically means "qemu" (which IIRC is using HPET in all our CI platforms anyway, I think?). Post-Quark, I don't think any such hardware exists or is envisioned?

Is there a reason you're trying to evolve this one instead of using apic_tsc? Honestly if you're going to be spending effort here I think I'd prefer to just see it removed.

[npitre]

Is there a reason you're trying to evolve this one instead of using apic_tsc? Honestly if you're going to be spending effort here I think I'd prefer to just see it removed.

Agreed. The only way this driver may provide a reliable time source is by _only_ using the periodic mode not the one-shot one, and therefore making it available only if CONFIG_TICKLESS_KERNEL=n.

[kwd-doodling]

This driver is a legacy thing. It exists to support platforms that have the old APIC countdown hardware and not TSC deadline support, which basically means "qemu" (which IIRC is using HPET in all our CI platforms anyway, I think?). Post-Quark, I don't think any such hardware exists or is envisioned?

Is there a reason you're trying to evolve this one instead of using apic_tsc? Honestly if you're going to be spending effort here I think I'd prefer to just see it removed.

ISH is a legacy CPU in fact :) Run apic_tsc on ISH already, ASSERT on "__ASSERT((ecx & BIT(24)) != 0, "No TSC Deadline support");"

[kwd-doodling]

Is there a reason you're trying to evolve this one instead of using apic_tsc? Honestly if you're going to be spending effort here I think I'd prefer to just see it removed.
Agreed. The only way this driver may provide a reliable time source is by only using the periodic mode not the one-shot one, and therefore making it available only if CONFIG_TICKLESS_KERNEL=n.

In this patch for TSC mode, TSC count is used to make it good for one-shot mode.

[andyross]

Sigh. Well, OK then. :) I'll remove the -1, clearly you're stuck with this one.

[kwd-doodling]

I'd suggest
"Change last_announcement to track the tick aligned cycle count of the last announcement rather than the absolute cycle count. This ensures the the number of ticks that have elapsed since last_announcement, provided by sys_clock_elapsed(), are accurate. Without this change the number of elapsed ticks may show up as one more than have actually elapsed."

thanks Tom. accept you suggestion. Add more, this change also make the next announcement calculation more accurate.

[npitre]

I still stand by my assertion that this driver is not accurate.

In the non-TSC case you have:

void sys_clock_set_timeout(int32_t n, bool idle)
{
        ARG_UNUSED(idle);

        int   full_ticks;       /* number of complete ticks we'll wait */
        uint32_t full_cycles;   /* full_ticks represented as cycles */
        uint32_t partial_cycles;        /* number of cycles to first tick boundary */

        if (n < 1) {
                full_ticks = 0;
        } else if ((n == K_TICKS_FOREVER) || (n > MAX_TICKS)) {
                full_ticks = MAX_TICKS - 1;
        } else {
                full_ticks = n - 1;
        }

        full_cycles = full_ticks * CYCLES_PER_TICK;

        k_spinlock_key_t key = k_spin_lock(&lock);

        total_cycles += (cached_icr - x86_read_loapic(LOAPIC_TIMER_CCR));
        partial_cycles = CYCLES_PER_TICK - (total_cycles % CYCLES_PER_TICK);
        cached_icr = full_cycles + partial_cycles;
        x86_write_loapic(LOAPIC_TIMER_ICR, cached_icr);

        k_spin_unlock(&lock, key);
}

Between the x86_read_loapic(LOAPIC_TIMER_CCR) and the
x86_write_loapic(LOAPIC_TIMER_ICR, cached_icr) there is an
unknown amount of unaccounted cycles going by. And to make it worse,
this interval includes a total_cycles % CYCLES_PER_TICK i.e. a costly
64-bit modulo operation.

The ISR contributes to this as well:

static void isr(const void *arg)
{
        int32_t ticks;
        uint64_t ticks_u64;

        k_spinlock_key_t key = k_spin_lock(&lock);

        /* Restart the timer as early as possible to minimize drift... */
        x86_write_loapic(LOAPIC_TIMER_ICR, MAX_TICKS * CYCLES_PER_TICK);

        total_cycles += cached_icr;
        cached_icr = MAX_TICKS * CYCLES_PER_TICK;

        ticks_u64 = (total_cycles - last_announcement) / CYCLES_PER_TICK;
        ticks = (ticks_u64 > INT_MAX) ? INT_MAX : (int32_t)ticks_u64;
        last_announcement += ticks_u64 * CYCLES_PER_TICK;
        k_spin_unlock(&lock, key);
        sys_clock_announce(ticks);
}

The "Restart the timer as early as possible to minimize drift" comment is
wishful thinking. You already drifted by the time you get there as an
undeterminate amount of unaccounted cycles went their way by the time
this ISR is serviced... assuming IRQs were not disabled for too long when
the timer expired or you'll get a major drift.

And then this roundabout computation via total_cycles derived from
cached_icr plus another costly 64-bit division. You could consider
having a cached_next_announce instead, given that you can't rely on
an actual running counter to announce the real amount of ticks in the case
of a laggy interrupt anyway.

So I'm saying it again: without CONFIG_APIC_TIMER_TSC this driver should
not pretend to be CONFIG_TICKLESS_KERNEL compatible. It should instead
use the APIC's periodic timer mode to be accurate and be done with it.

[kwd-doodling]

@npitre completely agree you on "So I'm saying it again: without CONFIG_APIC_TIMER_TSC this driver should
not pretend to be CONFIG_TICKLESS_KERNEL compatible. It should instead
use the APIC's periodic timer mode to be accurate and be done with it."

none-TSC mode is just a thing for playing, not for production, maybe just for CPU has no TSC at all. Just leave it as it is, and ISH won't use it at least.

[npitre]

Even with CONFIG_APIC_TIMER_TSC the code is wrong for this case:

#ifdef CONFIG_APIC_TIMER_TSC
       if (n == K_TICKS_FOREVER) {
               x86_write_loapic(LOAPIC_TIMER_ICR, 0x0);
               return;
       }
#endif

You could end up overflowing the announced tick count if the next event
is far enough in time. In fact
this line actually shows the issue:

ticks = (ticks_u64 > INT_MAX) ? INT_MAX : (int32_t)ticks_u64;

This is wrong of course. You should limit the timeout range to the
announce range up front or the total announced will be too short.
See riscv_machine_timer.c and commit 0ea64b3 for example.

But while at it, I think it would make sense to split this in two:

• a non-TSC APIC driver that uses periodic mode (should be really simple)

• and a TSC-enabled driver in a separate C file.

[kwd-doodling]

"You could end up overflowing the announced tick count if the next event
is far enough in time."
I know that, but intend to leave it be. Or ISH CPU has to wake up periodically even timeout is FOREVER, which will cost whole SoC power.
My point is to leave tick count for scheduling/timer/event, etc. but not for clock. implement clock on RTC or TSC.

[kwd-doodling]

INT_MAX ticks = 0x7fffffff/TICKS_PER_SEC(100 for example)/60/60/24 = 248 days. This is long enough for a timer of embedded system.
About splitting to two files, they still have lots of common logic/flows. currently one file seems to be still good.

[npitre]

In fact apic_tsc.c already exists. Can't you use that one already?

[teburd]

In fact apic_tsc.c already exists. Can't you use that one already?

As noted above by @kwd-doodling this x86 core doesn't have all the tsc features to use it as a timer. It has the functionality to read a counter but not setup a compare from what I understand.

[npitre]

As noted above by @kwd-doodling this x86 core doesn't have all the tsc
features to use it as a timer. It has the functionality to read a counter
but not setup a compare from what I understand.

Ah... If that is indeed the case, I think it would be much cleaner to
add APIC ICR in one-shot mode as a fallback IRQ source to apic_tsc.c than
still supporting tickless operation in apic_timer.c.

I'm proposing 2 PRs:
PR #73183 cleans up apic_tsc.c making it very easy to add a write to ICR.
PR #73185 moving apic_timer.c to periodic mode so it can always be accurate.

[npitre]

Please consider adding ICR-based fallback to apic_tsc on top of PR #73183 instead. This will most likely be much cleaner.

[kwd-doodling]

Hi @npitre, all modern CPU's APIC has TSC deadline capability. Things in apic_timer.c are legacy things. I suggest to keep them there and don't get apic_tsc.c tainted.
and more, it's true that TSC-mode and TSC-deadline mode are common in TSC reading and one-shot mode, but they have big difference in timer setting. Lots of #ifdef needed.

[npitre]

all modern CPU's APIC has TSC deadline capability. Things in apic_timer.c are
legacy things. I suggest to keep them there and don't get apic_tsc.c tainted.

Legacy or not, we still have to maintain the code if we're to keep supporting
those. And maintainable code has to be clean.

and more, it's true that TSC-mode and TSC-deadline mode are common in TSC
reading and one-shot mode, but they have big difference in timer setting.
Lots of #ifdef needed.

Please have a look at #73185. I don't think it is that bad.

[kwd-doodling]

Please have a look at #73185. I don't think it is that bad.

@npitre yes, you're right. Your work get apic_tsc.c enhanced. Please go forward with it and thanks.
I will close this one.

[kwd-doodling]

let's go with #73185.