Hello,
Zhaoming Luo, le sam. 22 mars 2025 15:24:50 +0800, a ecrit:
> The HPET counter is read once in every clock interrupt. When any of the
> functions used for getting time is used, the HPET counter is read again and
> the difference between these two read is multiplied by the HPET period
> and added to the read value from time or uptime to get a more accurate
> time read.
>
> It seems not bad, we can have the accuracy of 10ns:
> ```
> demo@debian:~$ ./main
> Seconds: 1405, Nanoseconds: 688178790
> demo@debian:~$ ./main
> Seconds: 1406, Nanoseconds: 216391110
> ```
Nice!
> BTW, I found that CLOCK_REALTIME is still using host_get_time() instead
> of host_get_time64() [0].
Indeed, contribution welcome ;)
> diff --git a/i386/i386/apic.h b/i386/i386/apic.h
> index 92fb900a..30aa8521 100644
> --- a/i386/i386/apic.h
> +++ b/i386/i386/apic.h
> @@ -268,6 +268,8 @@ void ioapic_configure(void);
> void hpet_init(void);
> void hpet_udelay(uint32_t us);
> void hpet_mdelay(uint32_t ms);
> +uint32_t read_hpet_counter(void);
> +uint32_t get_hpet_period_nsec(void);
>
> extern int timer_pin;
> extern void intnull(int unit);
Please rather put them into ./kern/mach_clock.h, and avoid
using "hpet" in the name, so that interface is general and not
specific to i386. You can call them e.g. clock_read_counter() and
clock_get_counter_period_nsec()
> diff --git a/i386/i386/model_dep.h b/i386/i386/model_dep.h
> index 5369e288..6a46cbbf 100644
> --- a/i386/i386/model_dep.h
> +++ b/i386/i386/model_dep.h
> @@ -65,4 +65,14 @@ extern void machine_relax (void);
> */
> extern void c_boot_entry(vm_offset_t bi);
>
> +/*
> + * Return the tick value of hpet
> + */
> +extern uint32_t read_hpet_counter (void);
> +
> +/*
> + * Return the period of HPET timer in nanoseconds
> + */
> +extern uint32_t get_hpet_period_nsec (void);
> +
> #endif /* _I386AT_MODEL_DEP_H_ */
No need to declare them several times :)
> diff --git a/i386/i386at/model_dep.c b/i386/i386at/model_dep.c
> index 30449c37..aa8451ac 100644
> --- a/i386/i386at/model_dep.c
> +++ b/i386/i386at/model_dep.c
> @@ -229,6 +229,11 @@ void machine_init(void)
> */
> hpet_init();
> #endif
> +
> + /*
> + * Initialize the HPET
> + */
> + hpet_init();
> }
>
> /* Conserve power on processor CPU. */
Rather move #endif above the hpet_init() call. And put a #ifdef APIC
around it.
You should also cope with the case where hpet_addr is NULL.
> diff --git a/kern/mach_clock.c b/kern/mach_clock.c
> index 5501b7b8..63a6988f 100644
> --- a/kern/mach_clock.c
> +++ b/kern/mach_clock.c
> @@ -83,6 +83,15 @@ unsigned tickadj = 500 / HZ; /* can adjust 100 usecs
> per second */
> unsigned bigadj = 1000000; /* adjust 10*tickadj if adjustment
> > bigadj */
>
> +/* HPET is taken into account to increase the accuracy of the functions
> + * used for getting time (e.g. host_get_time64()). The HPET counter is
> + * read once in every clock interrupt. When any of the functions used
> + * for getting time is used, the HPET counter is read again and the
> + * difference between these two read is multiplied by the HPET period
> + * and added to the read value from time or uptime to get a more accurate
> + * time read. */
Here as well, avoid using the HPET name, just call it "high-precision
hardware clock".
> +uint32_t last_hpet_read = 0;
> +
> /*
> * This update protocol, with a check value, allows
> * do {
> @@ -128,7 +137,8 @@ MACRO_BEGIN
> \
> __sync_synchronize(); \
> (time)->nanoseconds = mtime->time_value.nanoseconds; \
> __sync_synchronize(); \
> - } while ((time)->seconds != mtime->check_seconds64); \
> + } while ((time)->seconds != mtime->check_seconds64); \
> + time_value64_add_hpet(time); \
> MACRO_END
>
> #define read_mapped_uptime(uptime) \
> @@ -139,6 +149,7 @@ MACRO_BEGIN
> \
> (uptime)->nanoseconds = mtime->uptime_value.nanoseconds;\
> __sync_synchronize(); \
> } while ((uptime)->seconds != mtime->check_upseconds64); \
> + time_value64_add_hpet(uptime); \
> MACRO_END
>
> def_simple_lock_irq_data(static, timer_lock) /* lock for ... */
> @@ -292,6 +303,7 @@ void clock_interrupt(
> }
> }
> }
> + last_hpet_read = read_hpet_counter();
> }
>
> /*
> @@ -426,6 +438,19 @@ clock_boottime_update(const struct time_value64
> *new_time)
> time_value64_add(&clock_boottime_offset, &delta);
> }
>
> +/*
> + * Add the time value since last clock interrupt in nanosecond.
> + */
> +static void
> +time_value64_add_hpet(time_value64_t *value)
> +{
> + uint32_t now = read_hpet_counter();
> + /* Time since last clock interrupt in nanosecond. */
> + int ns = (now - last_hpet_read) * get_hpet_period_nsec();
Please cap that value to the period of a clock interrupt. Otherwise, if
for any reason the computation is not that precise and we end up having
ns greater than the clock interrupt period, time would go *back*, which
we really don't want :) Better see time stuck at the very end of the
clock interrupt period in case of such computation imprecision.
> + time_value64_add_nanos(value, ns);
> +}
Samuel
